Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs
Oliver Hartkopp <[email protected]>
| Newsgroups | org.kernel.vger.linux-can,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
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
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.
Best regards,
Oliver
> Patch #1 cleans up slcan, which does not use the generic echo skb
> helpers and therefore should not allocate echo slots. Patch #2 fixes a
> small inaccuracy in the can.rst documentation in regard to the IFF_ECHO
> flag. Patch #3 sets IFF_ECHO automatically when echo skb slots are
> requested. And Patch #4, the final one, removes the now redundant
> IFF_ECHO assignments from drivers which are covered by alloc_candev()
> with a non-zero echo_skb_max.
>
> The remaining explicit IFF_ECHO assignments are special cases with
> custom or virtual echo handling.
>
> Signed-off-by: Vincent Mailhol <[email protected]>
> ---
> Vincent Mailhol (4):
> can: slcan: do not allocate unused echo skb
> can: fix IFF_ECHO example in documentation
> can: dev: set IFF_ECHO when allocating echo skbs
> can: treewide: remove redundant IFF_ECHO assignments
>
> Documentation/networking/can.rst | 7 +++++--
> drivers/net/can/at91_can.c | 1 -
> drivers/net/can/bxcan.c | 1 -
> drivers/net/can/c_can/c_can_main.c | 1 -
> drivers/net/can/cc770/cc770.c | 2 --
> drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
> drivers/net/can/dev/dev.c | 1 +
> drivers/net/can/dummy_can.c | 1 -
> drivers/net/can/esd/esd_402_pci-core.c | 1 -
> drivers/net/can/flexcan/flexcan-core.c | 1 -
> drivers/net/can/ifi_canfd/ifi_canfd.c | 1 -
> drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c | 1 -
> drivers/net/can/m_can/m_can.c | 1 -
> drivers/net/can/mscan/mscan.c | 2 --
> drivers/net/can/peak_canfd/peak_canfd.c | 1 -
> drivers/net/can/rcar/rcar_can.c | 1 -
> drivers/net/can/rcar/rcar_canfd.c | 1 -
> drivers/net/can/rockchip/rockchip_canfd-core.c | 1 -
> drivers/net/can/sja1000/sja1000.c | 1 -
> drivers/net/can/slcan/slcan-core.c | 2 +-
> drivers/net/can/softing/softing_main.c | 1 -
> drivers/net/can/spi/hi311x.c | 1 -
> drivers/net/can/spi/mcp251x.c | 1 -
> drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 1 -
> drivers/net/can/sun4i_can.c | 1 -
> drivers/net/can/ti_hecc.c | 1 -
> drivers/net/can/usb/ems_usb.c | 2 --
> drivers/net/can/usb/esd_usb.c | 2 --
> drivers/net/can/usb/etas_es58x/es58x_core.c | 1 -
> drivers/net/can/usb/f81604.c | 1 -
> drivers/net/can/usb/gs_usb.c | 1 -
> drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 2 --
> drivers/net/can/usb/mcba_usb.c | 2 --
> drivers/net/can/usb/nct6694_canfd.c | 1 -
> drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 --
> drivers/net/can/usb/usb_8dev.c | 2 --
> drivers/net/can/virtio_can.c | 1 -
> drivers/net/can/xilinx_can.c | 2 --
> 38 files changed, 7 insertions(+), 47 deletions(-)
> ---
> base-commit: 828c4a5a9518117f9f7bdc445a7eeca85fc91bf8
> change-id: 20260804-automate_iff_echo_flag-6ddd7f4def7a
>
> Best regards,