Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs

On 10/08/2026 at 20:07, Oliver Hartkopp wrote:
> On 07.08.26 13:52, Vincent Mailhol wrote:
>> On 07/08/2026 at 12:56, Oliver Hartkopp wrote:
>>> On 06.08.26 22:55, Vincent Mailhol wrote:
>>>> On 06/08/2026 at 14:01, Oliver Hartkopp wrote:> On 05.08.26 23:06,
>>>> Vincent Mailhol wrote:
>>>>>> On 05/08/2026 at 18:17, Oliver Hartkopp wrote:
>>>>>
>>>>>>> IMO it's the right approach that alloc_candev_mqs() sets the
>>>>>>> IFF_ECHO
>>>>>>> flag and the default queue len.
>>>>>>
>>>>>> For IFF_ECHO, this is exactly what this series does!
>>>>>
>>>>> I just wanted to second you. This does not mean that I fully
>>>>> support the
>>>>> way it is implemented.
>>>>>
>>>>>> For the default queue len, why not. I have not study this particular
>>>>>> topic. But I think the IFF_ECHO and the queue len should be in
>>>>>> separate
>>>>>> series.
>>>>>
>>>>> My patch does not even compile. I just wanted to lead the dicsussion
>>>>> into a direction to find a more versatile solution that covers virtual
>>>>> CAN interfaces, non-echo CAN interfaces and full featured
>>>>> (echo'ing) CAn
>>>>> interfaces.
>>>>>
>>>>>>> What puzzles me is that the slcan driver is something in between
>>>>>>> which
>>>>>>> is neither a real CAN hardware nor a virtual CAN interface.
>>>>>>
>>>>>> My understanding it that devices which do not have a TX completion
>>>>>> handler (like slcan or can327) have no benefits to implement the
>>>>>> echo_skb framework and can instead simply rely on the PF_CAN core.
>>>>>
>>>>> Right.
>>>>>
>>>>>>> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only
>>>>>>> for
>>>>>>> real CAN hardware devices and open code slcan and the virtual CAN
>>>>>>> drivers ... which goes into the direction below.
>>>>>>>
>>>>>>> Any thoughts?
>>>>>>
>>>>>> The logic I tried to follow in this series is that
>>>>>> alloc_candev{,_mqs}()
>>>>>> has two arguments:
>>>>>>
>>>>>>      1. one for the priv structure
>>>>>>
>>>>>>      2. one for the number of echo_skb
>>>>>>
>>>>>> But then, when 2. is zero:
>>>>>>
>>>>>>      alloc_candev{,_mqs}(..., 0)
>>>>>>
>>>>>> means to me: give me all the features expect from the echo_skb.
>>>>>>
>>>>>> With the above, there is no anomalies to see the slcan do:
>>>>>>
>>>>>>      dev = alloc_candev(sizeof(*sl), 0);
>>>>>>
>>>>>> So I don't see the point to open code the allocations in slcan. After
>>>>>> patch #1 which corrects the echo skb count, the code describes
>>>>>> correctly
>>>>>> the behaviour.
>>>>>
>>>>> To me ", 0);" is a silent switch which does not make clear that slcan
>>>>> and can327 do something different here.
>>>>>
>>>>> We have 4 features:
>>>>>
>>>>> - support of IFF_ECHO mode using echo_skb's
>>>>> - support setting of bitrates via netlink
>>>>> - support setting of whatever via ethtool
>>>>> - use of TX queues (tx_queue_len != 0)
>>>>>
>>>>> And I would like these features to be separately selected to be
>>>>> transparent about what the CAN driver needs and supports.
>>>>>
>>>>> E.g. by defining a wrapper/define
>>>>>
>>>>> dev = alloc_non_echo_candev(sizeof(*sl));
>>>>>
>>>>> which calls
>>>>>
>>>>> dev = alloc_candev(sizeof(*sl), 0);
>>>>
>>>> Going this way, it should be the other way around. Have:
>>>>
>>>>     alloc_candev(sizeof(*foo));
>>>>
>>>> which just does the basic things and then:
>>>>
>>>>     alloc_candev_echo_skb(sizeof(*bar), 0);
>>>>
>>>> which allocate the echo skbs on top of the basic things.
>>>>
>>>> To me, the alloc_non_echo_candev() feels a bit like my previous
>>>>
>>>>     dev->flags &= ~IFF_ECHO;
>>>>
>>>> in the sense that it is not additive but subtractive.
>>>>
>>>>> And the same applies to the other features.
>>>>
>>>> But then, you reach a problem. If you do the Cartesian product of all
>>>> the 4 features, you end up with 2^4 = 16 combinations.
>>>>
>>>> Of course, some of the combinations will not be used.
>>>
>>> You likely got me wrong.
>>>
>>> We still have only about 3 cases that use those 4+ features:
>>>
>>> - support of IFF_ECHO mode using echo_skb's
>>> - support setting of bitrates via netlink
>>> - support setting of whatever via ethtool
>>> - use of TX queues (tx_queue_len != 0)
>>>
>>> My idea would be to have different functions to make clear what each of
>>> these drivers use. And not hide flags based on the number of echo skbs
>>> or shrink the number of helper functions by adding parameters.
>>>
>>> E.g.
>>>
>>> vcan calls:
>>>
>>> /* sizeof(struct can_ml_priv) is defined in vcan_link_ops */
>>> can_set_ml_priv(dev, netdev_priv(dev));
>>> can_setup(dev, (echo)?IFF_ECHO:0);
>>> vcan_set_mtu_info(dev);
>>> vcan_set_cap_info(dev);
>>> dev->tx_queue_len = 0;
>>>
>>> slcan calls:
>>>
>>> dev = alloc_candev(sizeof(struct slcan_priv));
>>> can_setup(dev, 0);
>>> slcan_set_mtu_info(dev);
>>> slcan_set_cap_info(dev);
>>> dev->tx_queue_len = CAN_TX_QUEUE_LEN;
>>>
>>>
>>> m_can calls:
>>>
>>> dev = alloc_candev_echo_skb(sizeof(struct m_can_priv), 4);
>>> can_setup(dev, IFF_ECHO);
>>> m_can_set_mtu_info(dev);
>>> m_can_set_cap_info(dev);
>>> dev->tx_queue_len = CAN_TX_QUEUE_LEN;
>>>
>>>
>>> This is what I meant with transparency and code deduplication.
>>> E.g. where can_setup() has an extra_flags parameter which is simply
>>> or'ed to IFF_NOARP.
>>
>> Now I understand. But I don't like the idea. If I understand correctly,
>> for the average driver, we will replace one call to:
>>
>>    alloc_candev_echo_skb()
>>
>> into roughly four calls.
>>
>> My goal in this series was to reduce boiler plate while making the
>> framework more robust. Your proposal increases the boilerplate and
>> reduces the robustest. Forgetting any one of these setup function is
>> also a potential security issue.
>>
>> As a concrete example, this already occurred in the past with several
>> drivers which forget to populate their MTU, for example: commit
>> 17c8d794527f ("can: mcba_usb: populate ndo_change_mtu() to prevent
>> buffer overflow").
>>
>> I modified the framework so that the MTU is now correctly set by default
>> in commit 23049938605b ("can: populate the minimum and maximum MTU
>> values") so that today, it is now impossible for a driver to incorrectly
>> set its MTU.
>>
>> Introducing a m_can_set_mtu_info() would be going backward to me. We
>> would open back the gate for a kind of bug which is today closed.
>>
>> And yes, the v(x)can remains special case which need to open code the
>> MTU, which is fine as these are really special. But for the majority, it
>> is a winning choice to "hide" it in the framework rather than take the
>> risk to trust the drivers to do the right thing.
> 
> Yes. I understand.
> 
> So having
> 
> - alloc_candev_echo_skb()
> - alloc_candev()
> 
> or maybe even better
> 
> - alloc_candev(sizeof(..), num_skbs)
> - alloc_candev_no_echo(sizeof(..)) /* for slcan / can327 */
> 
> make sense.

But then, what do we do for the grcan and the janz-ican3 which rely on
the device for the echo skb handling and thus do not allocate any echo
skb through the framework?

Should these two call alloc_candev(sizeof(..), 0) or
alloc_candev_no_echo(sizeof(..))? And why?

> And with these different names the EFF_ECHO setting is not really hidden
> anymore, which was my concern.

I am still not convinced. If the goal is transparency, I would rather do
it through explicit comments.

In can327 and slcan:

  /* No echo skb: the device has no TX completion handler. Rely on the
   * PF_CAN core for the echo */
  alloc_candev(sizeof(..), 0);

In grcan and janz-ican3:

  /* The device has it own echo skb mechanism, don't use the framework
   * echo skb. */
  alloc_candev(sizeof(..), 0);
  dev->flags |= IFF_ECHO;

And that is what I would call transparent.

The IFF_ECHO works in pair with the echo skb. Drivers should either take
the full set or nothing.

Having a alloc_candev(sizeof(..), 0) share a different semantic than
alloc_candev_no_echo(sizeof(..)) is the opposite of transparency. Where
is it hinted in the name that one would set IFF_ECHO and not the other?

> Btw. although v(x)can are different I would be interested in some
> can_setup() function that sets the some common CAN device specific
> values (like IFF_NOARP, default MTUs, etc) that are shared between all
> kinds of CAN interfaces.

But that goes back to the previous problem: this increases the
boilerplate for most of the drivers. I don't mind having some setup
functions shared between the v(x)can, but adding one more call to
can_setup() to the existing drivers is IMHO a step backward.

> And the reason to have it in dev.h was that this would not trigger some
> additional code compilation for v(x)can (beyond today's usage).

Yours sincerely,
Vincent Mailhol