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

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.

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

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.

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).

Best regards,
Oliver

> 
> And so, my though for IFF_ECHO is exactly the same. If it is open coded,
> this is a risk (ok, it is less critical than the MTU, but still can lead
> to unexpected behaviour). And so, my wish is for IFF_ECHO to follow the
> same path as what was done last year for the MTU: handle it in the
> framework and forgot this class of bug for the vast majority of the drivers.
> 
>> That code needs to be invoked in all those cases anyway but I would like
>> to make it visible and transparent which features and flags are enabled
>> for which reason.
> 
> 
> Yours sincerely,
> Vincent Mailhol
>