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

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.

But I definitely prefer a smaller set of functions which can adjust
their behaviour based on their parameters value rather than multiply the
number of functions. I would rather have one function with four
parameters rather than starting to add one function per combination we need.

Back to the echo skb, the prototype is:

  struct net_device * alloc_candev(int sizeof_priv,
                                   unsigned int echo_skb_max);

So:

  alloc_candev(sizeof(*sl), 0);

literally means that we are allocating a candev with a private scruture
of sizeof(*sl) and with zero echo skb.

I really fail to understand your point that setting echo_skb_max to zero
is not transparent but that alloc_non_echo_candev() is.

*echo_skb_max = 0* and *non_echo* are perfect synonymous to me, except
that the first one allows for a more compact implementation.


Yours sincerely,
Vincent Mailhol