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

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.

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

>> Btw. although v(x)can are different I would be interested in some
>> can_setup() function that sets the some common CAN device specific
>> values (like IFF_NOARP, default MTUs, etc) that are shared between all
>> kinds of CAN interfaces.
> 
> But that goes back to the previous problem: this increases the
> boilerplate for most of the drivers. I don't mind having some setup
> functions shared between the v(x)can, but adding one more call to
> can_setup() to the existing drivers is IMHO a step backward.
> 

Ok. v(x)can can stay completely open coded then.

Best regards,
Oliver