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

[Vincent Mailhol <mailhol@kernel.org>]

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.

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