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

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.

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.

Maybe _set_mtu_info(dev) and _set_cap_info(dev) could be merged.

Best regards,
Oliver