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

[Vincent Mailhol <mailhol@kernel.org>]

On 28/08/2026 at 14:56, Oliver Hartkopp wrote:
> 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.

My point is precisely that grcan's echo handling can not be automated by
alloc_candev(). It allocates its echo skb array manually in grcan_open():

  priv->echo_skb = kzalloc_objs(*priv->echo_skb, dma->tx.size);

So, because the echo skb setup can not be automated, let's automate
*nothing*: neither the skb allocation nor IFF_ECHO.

The rule is simple: does the driver want the framework echo handling?

  - yes: we do everything
  - no: we do nothing

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

This makes the individual arguments explicit, but it also makes invalid
combinations possible. In particular,

  alloc_candev(sizeof_priv, n, false)

with n non-zero is incorrect. This is the same class of issue we have
today, where a driver can call:

  alloc_candev(sizeof_priv, n);

and forget to set IFF_ECHO. The extra boolean does not resolve the
problem, it just moves it from one place to another.

We have tried several API shapes to cover the grcan and janz-ican3
special case, and each of them had its own drawback. To me, this points
in one direction: this edge case does not fit naturally in the common
API and should remain explicit in the drivers.

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