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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 28.08.26 11:22, Vincent Mailhol wrote:
> On 14/08/2026 at 15:27, Oliver Hartkopp wrote:
>> On 12.08.26 22:23, Vincent Mailhol wrote:
>>> On 10/08/2026 at 20:07, Oliver Hartkopp wrote:
>>
>>>
>>> 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.
>>
>> No. This is modifying bit values where you don't know where any why they
>> have been set. We need some top level function naming that makes
>> transparent what's going on.
> 
> But this is exactly my point: IFF_ECHO should be set next to where the
> echo mechanism is set up, so that we know why the flag is set.

The grcan example above shows that alloc_candev(sizeof(..), 0) is 
underspecified when you want to "automate" things that can not be 
automated based on the echo skb number.

And this "common" -vs- "special" case breaks the transparency and 
cleanliness about what is to be configured under the hood of 
alloc_candev().

Maybe we simply need to extend alloc_candev():

alloc_candev(int sizeof_priv, unsigned int echo_skb_max, bool echo_mode)

This would make clear, what is enabled and allocated without dubious 
implications nobody can follow easily.

Best regards,
Oliver


> 
> I just forgot in my previous message to mention that the custom echo skb
> logic would directly follow (making maybe my argument less clear).
> 
>>> 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?
>>>
>>
>> What about:
>>
>> #define NO_ECHO_SKB_ALLOC 0
>>
>> alloc_candev_echo(sizeof(..), 4) // usual case
>>
>> alloc_candev_echo(sizeof(..), NO_ECHO_SKB_ALLOC) // janz/grcan case
>>
>> alloc_candev_no_echo(sizeof(..)) // slcan/can327 case
>>
>> where
>>
>> alloc_candev_no_echo(unsigned int privsize) {
>>     struct netdevice dev;
>>
>>     dev = alloc_candev_echo(privsize, NO_ECHO_SKB_ALLOC)
>>     if (dev)
>>         dev->flags &= ~IFF_ECHO;
>>
>>     return dev;
>> }
> 
> I am still not convinced that IFF_ECHO should be exposed as an
> independent policy outside of the echo skb handling.
> 
> In particular,
> 
>    alloc_candev_echo(sizeof(..), NO_ECHO_SKB_ALLOC)
> 
> remains ambiguous to me. The NO_ECHO_SKB_ALLOC argument says that no
> echo skb is allocated by the framework (which the literal 0 already
> conveys), but it does not convey that IFF_ECHO would still be set. And
> even if we rename it to, for example:
> 
>    alloc_candev_echo(sizeof(..), ECHO_SKB_FLAG_ONLY)
> 
> we still have a semantic problem: echo_skb_max expects a scalar. What is
> the meaning of having a maximum of ECHO_SKB_FLAG_ONLY skbs?
> 
> I do not think this makes the interface more explicit.
> 
> My main question remains: why should IFF_ECHO be decoupled from the code
> which sets up the echo mechanism? Even the naming shows coupling:
> 
>    IFF_*ECHO*
>    *echo*_skb
>    alloc_candev_*echo*()
> 
> And why should we create a special case just for grcan and janz-ican3
> edge case drivers?
> 
> The current series maintains the coupling: IFF_ECHO is set by the code
> which owns the echo mechanism.
> 
> Finally, I would also prefer not to add another allocator name for a
> distinction which does not really relate to memory allocation.
> 
> 
> Yours sincerely,
> Vincent Mailhol