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

[Vincent Mailhol <mailhol@kernel.org>]

On 05/08/2026 at 08:29, Oliver Hartkopp wrote:
> On 04.08.26 21:55, Vincent Mailhol wrote:
>> Most CAN drivers allocate echo skb slots through alloc_candev() or
>> alloc_candev_mqs(), but still have to manually set IFF_ECHO to tell
>> PF_CAN that the driver handles local echo itself. This creates
>> boilerplate and makes it easy for drivers to forget one half of the
>> setup.
> 
> No one ever "forgot" this flag.
> 
>> A recent example is commit c77bfbdd6aac ("can: dummy_can:
>> dummy_can_init(): fix packet statistics"), where dummy_can was already
>> using the generic echo skb helpers but needed an explicit IFF_ECHO
>> assignment to make tx_bytes accounting work.
> 
> But you (ok us) :-D

Yes, this is the hidden motivation of this series. I did this mistake
and I was thinking if there were any way to prevent this from happening
again in the future.

But has a matter of fact, I am not the only one as the ucan driver also
omitted to set its IFF_ECHO (c.f. the note in Patch #3 message). And no
one noticed this one.

> To me this patch set does not really bring an improvement.
> You are now hiding the setting of this bit.
> 
> Today it is very transparent visible inside each drivers initialization
> section whether it supports IFF_ECHO or not. And e.g. vcan.c can also
> switch this feature with a module parameter.
> 
> I prefer this conscious setting in the driver setup. We should better
> add proper comments in drivers that do not set the flag, e.g. in slcan.c
> there's no hint that the af_can.c echo feature is used.

Then, what about setting IFF_ECHO for *all* drivers by default in
can_setup() and let the ones which have a special need to opt-out:

  dev->flags &= ~IFF_ECHO;

This way it remains transparent which one support IFF_ECHO or not. It is
also more important to highlight when things are done differently
(IFF_ECHO off) than when things go the normal case (IFF_ECHO on).

And this is more aligned with IFF_NOARP (c.f. you other message) in the
sense that both flags would now be set by default by the framework. It
looks odd to me that IFF_NOARP should be set by default by the framework
but not IFF_ECHO.


Yours sincerely,
Vincent Mailhol