* [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs
@ 2026-08-04 19:55 Vincent Mailhol
2026-08-04 19:55 ` [PATCH 1/4] can: slcan: do not allocate unused echo skb Vincent Mailhol
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Vincent Mailhol @ 2026-08-04 19:55 UTC (permalink / raw)
To: Marc Kleine-Budde, Oliver Hartkopp
Cc: linux-can, linux-kernel, Vincent Mailhol
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.
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.
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 <mailhol@kernel.org>
---
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,
--
Vincent Mailhol <mailhol@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/4] can: slcan: do not allocate unused echo skb 2026-08-04 19:55 [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Vincent Mailhol @ 2026-08-04 19:55 ` Vincent Mailhol 2026-08-04 19:55 ` [PATCH 2/4] can: fix IFF_ECHO example in documentation Vincent Mailhol ` (3 subsequent siblings) 4 siblings, 0 replies; 15+ messages in thread From: Vincent Mailhol @ 2026-08-04 19:55 UTC (permalink / raw) To: Marc Kleine-Budde, Oliver Hartkopp Cc: linux-can, linux-kernel, Vincent Mailhol The slcan driver does not use the generic CAN echo skb helpers. It leaves IFF_ECHO clear and relies on the PF_CAN fallback echo path instead. There is therefore no need to allocate one generic echo skb slot for slcan. Request zero echo skb slots when allocating the CAN netdevice. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> --- drivers/net/can/slcan/slcan-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c index 7439849d5c84..546b9dcb08cc 100644 --- a/drivers/net/can/slcan/slcan-core.c +++ b/drivers/net/can/slcan/slcan-core.c @@ -826,7 +826,7 @@ static int slcan_open(struct tty_struct *tty) if (!tty->ops->write) return -EOPNOTSUPP; - dev = alloc_candev(sizeof(*sl), 1); + dev = alloc_candev(sizeof(*sl), 0); if (!dev) return -ENFILE; -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/4] can: fix IFF_ECHO example in documentation 2026-08-04 19:55 [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Vincent Mailhol 2026-08-04 19:55 ` [PATCH 1/4] can: slcan: do not allocate unused echo skb Vincent Mailhol @ 2026-08-04 19:55 ` Vincent Mailhol 2026-08-05 6:08 ` Oliver Hartkopp 2026-08-04 19:55 ` [PATCH 3/4] can: dev: set IFF_ECHO when allocating echo skbs Vincent Mailhol ` (2 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Vincent Mailhol @ 2026-08-04 19:55 UTC (permalink / raw) To: Marc Kleine-Budde, Oliver Hartkopp Cc: linux-can, linux-kernel, Vincent Mailhol The documentation suggests doing: dev->flags = (IFF_NOARP | IFF_ECHO); to set the IFF_ECHO flag. This is problematic because by doing so, other potentially enabled flags would be overwritten. Furthermore, none of the drivers do it like that. Replace the example by: dev->flags |= IFF_ECHO; which is more robust and consistent with what all the drivers are doing. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> --- Documentation/networking/can.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst index 536ff411da1d..fbd0d501ea4e 100644 --- a/Documentation/networking/can.rst +++ b/Documentation/networking/can.rst @@ -1126,7 +1126,7 @@ e.g. of tty devices. In this case the driver flag IFF_ECHO has to be set to prevent the PF_CAN core from locally echoing sent frames (aka loopback) as fallback solution:: - dev->flags = (IFF_NOARP | IFF_ECHO); + dev->flags |= IFF_ECHO; CAN Controller Hardware Filters -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] can: fix IFF_ECHO example in documentation 2026-08-04 19:55 ` [PATCH 2/4] can: fix IFF_ECHO example in documentation Vincent Mailhol @ 2026-08-05 6:08 ` Oliver Hartkopp 0 siblings, 0 replies; 15+ messages in thread From: Oliver Hartkopp @ 2026-08-05 6:08 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can, linux-kernel On 04.08.26 21:55, Vincent Mailhol wrote: > The documentation suggests doing: > > dev->flags = (IFF_NOARP | IFF_ECHO); > > to set the IFF_ECHO flag. This is problematic because by doing so, > other potentially enabled flags would be overwritten. Furthermore, > none of the drivers do it like that. > > Replace the example by: > > dev->flags |= IFF_ECHO; IFF_NOARP is important for CAN interfaces and it looks like it could be missed now. Please document that IFF_NOARP is already set in can_setup(). E.g. out of tree drivers might stumble into problems when they set up the flags differently. > > which is more robust and consistent with what all the drivers are doing. > > Signed-off-by: Vincent Mailhol <mailhol@kernel.org> > --- > Documentation/networking/can.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst > index 536ff411da1d..fbd0d501ea4e 100644 > --- a/Documentation/networking/can.rst > +++ b/Documentation/networking/can.rst > @@ -1126,7 +1126,7 @@ e.g. of tty devices. In this case the driver flag IFF_ECHO has to be > set to prevent the PF_CAN core from locally echoing sent frames > (aka loopback) as fallback solution:: > > - dev->flags = (IFF_NOARP | IFF_ECHO); > + dev->flags |= IFF_ECHO; Here also a documentation about IFF_NOARP is needed now. Best regards, Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] can: dev: set IFF_ECHO when allocating echo skbs 2026-08-04 19:55 [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Vincent Mailhol 2026-08-04 19:55 ` [PATCH 1/4] can: slcan: do not allocate unused echo skb Vincent Mailhol 2026-08-04 19:55 ` [PATCH 2/4] can: fix IFF_ECHO example in documentation Vincent Mailhol @ 2026-08-04 19:55 ` Vincent Mailhol 2026-08-04 19:55 ` [PATCH 4/4] can: treewide: remove redundant IFF_ECHO assignments Vincent Mailhol 2026-08-05 6:29 ` [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Oliver Hartkopp 4 siblings, 0 replies; 15+ messages in thread From: Vincent Mailhol @ 2026-08-04 19:55 UTC (permalink / raw) To: Marc Kleine-Budde, Oliver Hartkopp Cc: linux-can, linux-kernel, Vincent Mailhol Drivers which request echo skb slots from alloc_candev() are expected to use the generic CAN echo skb helpers and handle local echo themselves. Such drivers also need to set IFF_ECHO to prevent PF_CAN from performing fallback echo. Set IFF_ECHO from alloc_candev_mqs() whenever echo_skb_max is non-zero. This ties the flag to the generic echo skb allocation and avoids requiring every driver to set it manually. This also covers ucan as a side effect. That driver already requests echo skb slots and uses can_put_echo_skb(), can_get_echo_skb(), and can_free_echo_skb(), but forgot to set IFF_ECHO. After this change, drivers which use the generic echo skb helpers have one less thing to remember during netdevice setup. Update the CAN documentation accordingly. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> --- Documentation/networking/can.rst | 5 ++++- drivers/net/can/dev/dev.c | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst index fbd0d501ea4e..bf3d8dc193a7 100644 --- a/Documentation/networking/can.rst +++ b/Documentation/networking/can.rst @@ -1124,7 +1124,10 @@ As described in :ref:`socketcan-local-loopback1` the CAN network device driver s support a local loopback functionality similar to the local echo e.g. of tty devices. In this case the driver flag IFF_ECHO has to be set to prevent the PF_CAN core from locally echoing sent frames -(aka loopback) as fallback solution:: +(aka loopback) as fallback solution. For CAN drivers which request +echo skb slots through ``alloc_candev()`` or ``alloc_candev_mqs()``, +the framework sets ``IFF_ECHO`` automatically. The other drivers need +to set it manually:: dev->flags |= IFF_ECHO; diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c index 769745e22a3c..7089ce871b24 100644 --- a/drivers/net/can/dev/dev.c +++ b/drivers/net/can/dev/dev.c @@ -338,6 +338,7 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, priv->echo_skb_max = echo_skb_max; priv->echo_skb = (void *)priv + (size - echo_skb_max * sizeof(struct sk_buff *)); + dev->flags |= IFF_ECHO; } priv->state = CAN_STATE_STOPPED; -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/4] can: treewide: remove redundant IFF_ECHO assignments 2026-08-04 19:55 [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Vincent Mailhol ` (2 preceding siblings ...) 2026-08-04 19:55 ` [PATCH 3/4] can: dev: set IFF_ECHO when allocating echo skbs Vincent Mailhol @ 2026-08-04 19:55 ` Vincent Mailhol 2026-08-05 6:29 ` [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Oliver Hartkopp 4 siblings, 0 replies; 15+ messages in thread From: Vincent Mailhol @ 2026-08-04 19:55 UTC (permalink / raw) To: Marc Kleine-Budde, Oliver Hartkopp Cc: linux-can, linux-kernel, Vincent Mailhol alloc_candev() and alloc_candev_mqs() now set IFF_ECHO automatically when echo skb slots are requested. Remove the explicit assignments from drivers which are covered by that generic setup. Keep the remaining explicit IFF_ECHO assignments for drivers with custom or virtual echo handling. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> --- 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/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/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 -- 35 files changed, 44 deletions(-) diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c index 58da323f14d7..aaa21697abb4 100644 --- a/drivers/net/can/at91_can.c +++ b/drivers/net/can/at91_can.c @@ -1105,7 +1105,6 @@ static int at91_can_probe(struct platform_device *pdev) dev->netdev_ops = &at91_netdev_ops; dev->ethtool_ops = &at91_ethtool_ops; dev->irq = irq; - dev->flags |= IFF_ECHO; priv = netdev_priv(dev); priv->can.clock.freq = clk_get_rate(clk); diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c index baf494d20bef..98e712488313 100644 --- a/drivers/net/can/bxcan.c +++ b/drivers/net/can/bxcan.c @@ -987,7 +987,6 @@ static int bxcan_probe(struct platform_device *pdev) ndev->netdev_ops = &bxcan_netdev_ops; ndev->ethtool_ops = &bxcan_ethtool_ops; ndev->irq = rx_irq; - ndev->flags |= IFF_ECHO; priv->dev = dev; priv->ndev = ndev; diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c index b3b321d9ce68..3a3ca718bb8d 100644 --- a/drivers/net/can/c_can/c_can_main.c +++ b/drivers/net/can/c_can/c_can_main.c @@ -1373,7 +1373,6 @@ int register_c_can_dev(struct net_device *dev) */ pinctrl_pm_select_sleep_state(dev->dev.parent); - dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &c_can_netdev_ops; dev->ethtool_ops = &c_can_ethtool_ops; diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c index 8d5abd643c06..16ac88426384 100644 --- a/drivers/net/can/cc770/cc770.c +++ b/drivers/net/can/cc770/cc770.c @@ -852,8 +852,6 @@ int register_cc770dev(struct net_device *dev) dev->netdev_ops = &cc770_netdev_ops; dev->ethtool_ops = &cc770_ethtool_ops; - dev->flags |= IFF_ECHO; /* we support local echo */ - /* Should we use additional functions? */ if (!i82527_compat && priv->control_normal_mode & CTRL_EAF) { priv->can.do_get_berr_counter = cc770_get_berr_counter; diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c index 07d4aa43c700..f54927863cf6 100644 --- a/drivers/net/can/ctucanfd/ctucanfd_base.c +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c @@ -1381,7 +1381,6 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne /* Get IRQ for the device */ ndev->irq = irq; - ndev->flags |= IFF_ECHO; /* We support local echo */ if (set_drvdata_fnc) set_drvdata_fnc(dev, ndev); diff --git a/drivers/net/can/dummy_can.c b/drivers/net/can/dummy_can.c index cd23de488edc..41953655e3d3 100644 --- a/drivers/net/can/dummy_can.c +++ b/drivers/net/can/dummy_can.c @@ -241,7 +241,6 @@ static int __init dummy_can_init(void) dev->netdev_ops = &dummy_can_netdev_ops; dev->ethtool_ops = &dummy_can_ethtool_ops; - dev->flags |= IFF_ECHO; /* enable echo handling */ priv = netdev_priv(dev); priv->can.bittiming_const = &dummy_can_bittiming_const; priv->can.bitrate_max = 20 * MEGA /* BPS */; diff --git a/drivers/net/can/esd/esd_402_pci-core.c b/drivers/net/can/esd/esd_402_pci-core.c index c826f00c551b..244bc387062d 100644 --- a/drivers/net/can/esd/esd_402_pci-core.c +++ b/drivers/net/can/esd/esd_402_pci-core.c @@ -362,7 +362,6 @@ static int pci402_init_cores(struct pci_dev *pdev) } core->netdev = netdev; - netdev->flags |= IFF_ECHO; netdev->dev_port = i; netdev->netdev_ops = &pci402_acc_netdev_ops; netdev->ethtool_ops = &pci402_acc_ethtool_ops; diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c index f5d22c61503f..8e06f462ee02 100644 --- a/drivers/net/can/flexcan/flexcan-core.c +++ b/drivers/net/can/flexcan/flexcan-core.c @@ -2182,7 +2182,6 @@ static int flexcan_probe(struct platform_device *pdev) dev->netdev_ops = &flexcan_netdev_ops; dev->ethtool_ops = &flexcan_ethtool_ops; dev->irq = irq; - dev->flags |= IFF_ECHO; priv = netdev_priv(dev); priv->devtype_data = *devtype_data; diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c index 0f83335e4d07..732b59017734 100644 --- a/drivers/net/can/ifi_canfd/ifi_canfd.c +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c @@ -985,7 +985,6 @@ static int ifi_canfd_plat_probe(struct platform_device *pdev) return -ENOMEM; ndev->irq = irq; - ndev->flags |= IFF_ECHO; /* we support local echo */ ndev->netdev_ops = &ifi_canfd_netdev_ops; ndev->ethtool_ops = &ifi_canfd_ethtool_ops; diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c index d8c9bfb20230..dbe6bd0f40f1 100644 --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c @@ -1005,7 +1005,6 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) if (status & KVASER_PCIEFD_KCAN_STAT_CAP) can->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT; - netdev->flags |= IFF_ECHO; SET_NETDEV_DEV(netdev, &pcie->pci->dev); iowrite32(GENMASK(31, 0), can->reg_base + KVASER_PCIEFD_KCAN_IRQ_REG); diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index eb856547ae7d..3cccb796aa40 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -2330,7 +2330,6 @@ static int register_m_can_dev(struct m_can_classdev *cdev) { struct net_device *dev = cdev->net; - dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &m_can_netdev_ops; if (dev->irq && cdev->is_peripheral) dev->ethtool_ops = &m_can_ethtool_ops_coalescing; diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c index 39c7aa2a0b2f..9285a441d326 100644 --- a/drivers/net/can/mscan/mscan.c +++ b/drivers/net/can/mscan/mscan.c @@ -675,8 +675,6 @@ struct net_device *alloc_mscandev(void) dev->netdev_ops = &mscan_netdev_ops; dev->ethtool_ops = &mscan_ethtool_ops; - dev->flags |= IFF_ECHO; /* we support local echo */ - netif_napi_add_weight(dev, &priv->napi, mscan_rx_poll, 8); priv->can.bittiming_const = &mscan_bittiming_const; diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c index 4fd1aefb780f..ce2ab0bcecf7 100644 --- a/drivers/net/can/peak_canfd/peak_canfd.c +++ b/drivers/net/can/peak_canfd/peak_canfd.c @@ -827,7 +827,6 @@ struct net_device *alloc_peak_canfd_dev(int sizeof_priv, int index, priv->cmd_len = 0; spin_lock_init(&priv->echo_lock); - ndev->flags |= IFF_ECHO; ndev->netdev_ops = &peak_canfd_netdev_ops; ndev->ethtool_ops = &peak_canfd_ethtool_ops; ndev->dev_id = index; diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c index 2727c5ce029c..1ed0b19111f3 100644 --- a/drivers/net/can/rcar/rcar_can.c +++ b/drivers/net/can/rcar/rcar_can.c @@ -792,7 +792,6 @@ static int rcar_can_probe(struct platform_device *pdev) ndev->netdev_ops = &rcar_can_netdev_ops; ndev->ethtool_ops = &rcar_can_ethtool_ops; ndev->irq = irq; - ndev->flags |= IFF_ECHO; priv->ndev = ndev; priv->regs = addr; priv->clock_select = clock_select; diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index fcc37b73ed43..6772117f18d0 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -1878,7 +1878,6 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, ndev->netdev_ops = &rcar_canfd_netdev_ops; ndev->ethtool_ops = &rcar_canfd_ethtool_ops; - ndev->flags |= IFF_ECHO; priv->ndev = ndev; priv->base = gpriv->base; priv->transceiver = transceiver; diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c index 29de0c01e4ed..a2a4ed624b6d 100644 --- a/drivers/net/can/rockchip/rockchip_canfd-core.c +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c @@ -893,7 +893,6 @@ static int rkcanfd_probe(struct platform_device *pdev) SET_NETDEV_DEV(ndev, &pdev->dev); ndev->netdev_ops = &rkcanfd_netdev_ops; - ndev->flags |= IFF_ECHO; platform_set_drvdata(pdev, priv); priv->can.clock.freq = clk_get_rate(priv->clks[0].clk); diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index 3cdb583ee3e5..423e9395e776 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -688,7 +688,6 @@ int register_sja1000dev(struct net_device *dev) if (!sja1000_probe_chip(dev)) return -ENODEV; - dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &sja1000_netdev_ops; dev->ethtool_ops = &sja1000_ethtool_ops; diff --git a/drivers/net/can/softing/softing_main.c b/drivers/net/can/softing/softing_main.c index 519ab3097f86..5ab67c8dc8ff 100644 --- a/drivers/net/can/softing/softing_main.c +++ b/drivers/net/can/softing/softing_main.c @@ -651,7 +651,6 @@ static struct net_device *softing_netdev_create(struct softing *card, priv->output = softing_default_output(netdev); SET_NETDEV_DEV(netdev, &card->pdev->dev); - netdev->flags |= IFF_ECHO; netdev->netdev_ops = &softing_netdev_ops; netdev->ethtool_ops = &softing_ethtool_ops; priv->can.do_set_mode = softing_candev_set_mode; diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c index ae90e6716de5..267b3404e699 100644 --- a/drivers/net/can/spi/hi311x.c +++ b/drivers/net/can/spi/hi311x.c @@ -861,7 +861,6 @@ static int hi3110_can_probe(struct spi_device *spi) net->netdev_ops = &hi3110_netdev_ops; net->ethtool_ops = &hi3110_ethtool_ops; - net->flags |= IFF_ECHO; priv = netdev_priv(net); priv->can.bittiming_const = &hi3110_bittiming_const; diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c index 0d0190ae094a..732a2cc1dd50 100644 --- a/drivers/net/can/spi/mcp251x.c +++ b/drivers/net/can/spi/mcp251x.c @@ -1361,7 +1361,6 @@ static int mcp251x_can_probe(struct spi_device *spi) net->netdev_ops = &mcp251x_netdev_ops; net->ethtool_ops = &mcp251x_ethtool_ops; - net->flags |= IFF_ECHO; priv = netdev_priv(net); priv->can.bittiming_const = &mcp251x_bittiming_const; diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index f441f2265299..07b67ac2f8c5 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -2281,7 +2281,6 @@ static int mcp251xfd_probe(struct spi_device *spi) ndev->netdev_ops = &mcp251xfd_netdev_ops; ndev->irq = spi->irq; - ndev->flags |= IFF_ECHO; priv = netdev_priv(ndev); spi_set_drvdata(spi, priv); diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c index af52285d5a4e..f15344126791 100644 --- a/drivers/net/can/sun4i_can.c +++ b/drivers/net/can/sun4i_can.c @@ -874,7 +874,6 @@ static int sun4ican_probe(struct platform_device *pdev) dev->netdev_ops = &sun4ican_netdev_ops; dev->ethtool_ops = &sun4ican_ethtool_ops; dev->irq = irq; - dev->flags |= IFF_ECHO; priv = netdev_priv(dev); priv->can.clock.freq = clk_get_rate(clk); diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c index 1d3dbf28b105..3831d23a4ab9 100644 --- a/drivers/net/can/ti_hecc.c +++ b/drivers/net/can/ti_hecc.c @@ -908,7 +908,6 @@ static int ti_hecc_probe(struct platform_device *pdev) priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; spin_lock_init(&priv->mbx_lock); - ndev->flags |= IFF_ECHO; platform_set_drvdata(pdev, ndev); SET_NETDEV_DEV(ndev, &pdev->dev); ndev->netdev_ops = &ti_hecc_netdev_ops; diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c index 24cf8f651f8f..8c9a1892e9e0 100644 --- a/drivers/net/can/usb/ems_usb.c +++ b/drivers/net/can/usb/ems_usb.c @@ -1058,8 +1058,6 @@ static int ems_usb_probe(struct usb_interface *intf, netdev->netdev_ops = &ems_usb_netdev_ops; netdev->ethtool_ops = &ems_usb_ethtool_ops; - netdev->flags |= IFF_ECHO; /* we support local echo */ - init_usb_anchor(&dev->rx_submitted); init_usb_anchor(&dev->tx_submitted); diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c index f41d4a0d140f..43a645a5c4a2 100644 --- a/drivers/net/can/usb/esd_usb.c +++ b/drivers/net/can/usb/esd_usb.c @@ -1263,8 +1263,6 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index) priv->can.do_set_mode = esd_usb_set_mode; priv->can.do_get_berr_counter = esd_usb_get_berr_counter; - netdev->flags |= IFF_ECHO; /* we support local echo */ - netdev->netdev_ops = &esd_usb_netdev_ops; netdev->ethtool_ops = &esd_usb_ethtool_ops; diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c index e1724ae79c5a..3ab5de7910c0 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_core.c +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c @@ -2107,7 +2107,6 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx) netdev->netdev_ops = &es58x_netdev_ops; netdev->ethtool_ops = &es58x_ethtool_ops; - netdev->flags |= IFF_ECHO; /* We support local echo */ netdev->dev_port = channel_idx; ret = register_candev(netdev); diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c index f12318268e46..7b6af2aa55f1 100644 --- a/drivers/net/can/usb/f81604.c +++ b/drivers/net/can/usb/f81604.c @@ -1201,7 +1201,6 @@ static int f81604_probe(struct usb_interface *intf, netdev->ethtool_ops = &f81604_ethtool_ops; netdev->netdev_ops = &f81604_netdev_ops; - netdev->flags |= IFF_ECHO; netdev->dev_port = i; SET_NETDEV_DEV(netdev, &intf->dev); diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c index 82508a865095..e4f04b5c95cd 100644 --- a/drivers/net/can/usb/gs_usb.c +++ b/drivers/net/can/usb/gs_usb.c @@ -1351,7 +1351,6 @@ static struct gs_can *gs_make_candev(unsigned int channel, netdev->netdev_ops = &gs_usb_netdev_ops; netdev->ethtool_ops = &gs_usb_ethtool_ops; - netdev->flags |= IFF_ECHO; /* we support full roundtrip echo */ netdev->dev_id = channel; netdev->dev_port = channel; diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c index d0a2a2a33c1c..1cbf1c0b9fea 100644 --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c @@ -886,8 +886,6 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev, int channel) priv->can.fd.do_set_data_bittiming = kvaser_usb_set_data_bittiming; } - netdev->flags |= IFF_ECHO; - netdev->netdev_ops = &kvaser_usb_netdev_ops; netdev->ethtool_ops = &kvaser_usb_ethtool_ops; SET_NETDEV_DEV(netdev, &dev->intf->dev); diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c index 04170326dc7e..8043c1e24345 100644 --- a/drivers/net/can/usb/mcba_usb.c +++ b/drivers/net/can/usb/mcba_usb.c @@ -853,8 +853,6 @@ static int mcba_usb_probe(struct usb_interface *intf, netdev->netdev_ops = &mcba_netdev_ops; netdev->ethtool_ops = &mcba_ethtool_ops; - netdev->flags |= IFF_ECHO; /* we support local echo */ - SET_NETDEV_DEV(netdev, &intf->dev); err = register_candev(netdev); diff --git a/drivers/net/can/usb/nct6694_canfd.c b/drivers/net/can/usb/nct6694_canfd.c index e5f7f8849a73..b6ac97dd78ca 100644 --- a/drivers/net/can/usb/nct6694_canfd.c +++ b/drivers/net/can/usb/nct6694_canfd.c @@ -743,7 +743,6 @@ static int nct6694_canfd_probe(struct platform_device *pdev) } ndev->irq = irq; - ndev->flags |= IFF_ECHO; ndev->dev_port = port; ndev->netdev_ops = &nct6694_canfd_netdev_ops; ndev->ethtool_ops = &nct6694_canfd_ethtool_ops; diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c index 55aad01cd8ca..a49af5a06194 100644 --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c @@ -957,8 +957,6 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter, netdev->netdev_ops = &peak_usb_netdev_ops; - netdev->flags |= IFF_ECHO; /* we support local echo */ - /* add ethtool support */ netdev->ethtool_ops = peak_usb_adapter->ethtool_ops; diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c index 3125cf59d002..6815d7fe987d 100644 --- a/drivers/net/can/usb/usb_8dev.c +++ b/drivers/net/can/usb/usb_8dev.c @@ -939,8 +939,6 @@ static int usb_8dev_probe(struct usb_interface *intf, netdev->netdev_ops = &usb_8dev_netdev_ops; netdev->ethtool_ops = &usb_8dev_ethtool_ops; - netdev->flags |= IFF_ECHO; /* we support local echo */ - init_usb_anchor(&priv->rx_submitted); init_usb_anchor(&priv->tx_submitted); diff --git a/drivers/net/can/virtio_can.c b/drivers/net/can/virtio_can.c index f67d0bf09681..863dfcf3f13d 100644 --- a/drivers/net/can/virtio_can.c +++ b/drivers/net/can/virtio_can.c @@ -425,7 +425,6 @@ static const struct net_device_ops virtio_can_netdev_ops = { static int register_virtio_can_dev(struct net_device *dev) { - dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &virtio_can_netdev_ops; return register_candev(dev); diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 43d7f22820b8..e63e3d0a0cf6 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -1996,8 +1996,6 @@ static int xcan_probe(struct platform_device *pdev) ndev->irq = ret; - ndev->flags |= IFF_ECHO; /* We support local echo */ - platform_set_drvdata(pdev, ndev); SET_NETDEV_DEV(ndev, &pdev->dev); ndev->netdev_ops = &xcan_netdev_ops; -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-04 19:55 [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Vincent Mailhol ` (3 preceding siblings ...) 2026-08-04 19:55 ` [PATCH 4/4] can: treewide: remove redundant IFF_ECHO assignments Vincent Mailhol @ 2026-08-05 6:29 ` Oliver Hartkopp 2026-08-05 7:25 ` Vincent Mailhol 4 siblings, 1 reply; 15+ messages in thread From: Oliver Hartkopp @ 2026-08-05 6:29 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can, linux-kernel 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 <mailhol@kernel.org> > --- > 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, ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-05 6:29 ` [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Oliver Hartkopp @ 2026-08-05 7:25 ` Vincent Mailhol 2026-08-05 16:17 ` Oliver Hartkopp 0 siblings, 1 reply; 15+ messages in thread From: Vincent Mailhol @ 2026-08-05 7:25 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde; +Cc: linux-can, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-05 7:25 ` Vincent Mailhol @ 2026-08-05 16:17 ` Oliver Hartkopp 2026-08-05 21:06 ` Vincent Mailhol 0 siblings, 1 reply; 15+ messages in thread From: Oliver Hartkopp @ 2026-08-05 16:17 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can, linux-kernel On 05.08.26 09:25, Vincent Mailhol wrote: > On 05/08/2026 at 08:29, Oliver Hartkopp wrote: >> 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 looks like a hack reverting bit settings. > 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. I'm not really done with my thoughts but ... IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO flag and the default queue len. What puzzles me is that the slcan driver is something in between which is neither a real CAN hardware nor a virtual CAN interface. My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only for real CAN hardware devices and open code slcan and the virtual CAN drivers ... which goes into the direction below. Any thoughts? Best regards, Oliver diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c index 769745e22a3c..5bdbe0c1d197 100644 --- a/drivers/net/can/dev/dev.c +++ b/drivers/net/can/dev/dev.c @@ -277,25 +277,10 @@ void can_bus_off(struct net_device *dev) schedule_delayed_work(&priv->restart_work, msecs_to_jiffies(priv->restart_ms)); } EXPORT_SYMBOL_GPL(can_bus_off); -void can_setup(struct net_device *dev) -{ - dev->type = ARPHRD_CAN; - dev->mtu = CAN_MTU; - dev->min_mtu = CAN_MTU; - dev->max_mtu = CAN_MTU; - dev->hard_header_len = 0; - dev->addr_len = 0; - dev->tx_queue_len = 10; - - /* New-style flags. */ - dev->flags = IFF_NOARP; - dev->features = NETIF_F_HW_CSUM; -} - /* Allocate and setup space for the CAN network device */ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, unsigned int txqs, unsigned int rxqs) { struct can_ml_priv *can_ml; @@ -332,10 +317,13 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, can_ml = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN); can_set_ml_priv(dev, can_ml); can_set_cap(dev, CAN_CAP_CC); + dev->tx_queue_len = CAN_TX_QUEUE_LEN; + dev->flags |= IFF_ECHO; + if (echo_skb_max) { priv->echo_skb_max = echo_skb_max; priv->echo_skb = (void *)priv + (size - echo_skb_max * sizeof(struct sk_buff *)); } diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c index 76e6b7b5c6a1..70263813ec40 100644 --- a/drivers/net/can/vcan.c +++ b/drivers/net/can/vcan.c @@ -167,16 +167,15 @@ static const struct ethtool_ops vcan_ethtool_ops = { .get_ts_info = ethtool_op_get_ts_info, }; static void vcan_setup(struct net_device *dev) { - dev->type = ARPHRD_CAN; - dev->mtu = CANXL_MTU; - dev->hard_header_len = 0; - dev->addr_len = 0; - dev->tx_queue_len = 0; - dev->flags = IFF_NOARP; + can_setup(dev); + dev->tx_queue_len = 0; + dev->mtu = CANXL_MTU; + dev->min_mtu = CAN_MTU; + dev->max_mtu = CANXL_MTU; can_set_ml_priv(dev, netdev_priv(dev)); vcan_set_cap_info(dev); /* set flags according to driver capabilities */ if (echo) diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c index e882250180ef..615a906203fa 100644 --- a/drivers/net/can/vxcan.c +++ b/drivers/net/can/vxcan.c @@ -180,19 +180,18 @@ static const struct ethtool_ops vxcan_ethtool_ops = { static void vxcan_setup(struct net_device *dev) { struct can_ml_priv *can_ml; - dev->type = ARPHRD_CAN; - dev->mtu = CANXL_MTU; - dev->hard_header_len = 0; - dev->addr_len = 0; - dev->tx_queue_len = 0; - dev->flags = IFF_NOARP; - dev->netdev_ops = &vxcan_netdev_ops; - dev->ethtool_ops = &vxcan_ethtool_ops; - dev->needs_free_netdev = true; + can_setup(dev); + dev->tx_queue_len = 0; + dev->mtu = CANXL_MTU; + dev->min_mtu = CAN_MTU; + dev->max_mtu = CANXL_MTU; + dev->netdev_ops = &vxcan_netdev_ops; + dev->ethtool_ops = &vxcan_ethtool_ops; + dev->needs_free_netdev = true; can_ml = netdev_priv(dev) + ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN); can_set_ml_priv(dev, can_ml); vxcan_set_cap_info(dev); } diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 6d0710d6f571..4619a74599cb 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -21,10 +21,12 @@ #include <linux/can/netlink.h> #include <linux/can/skb.h> #include <linux/ethtool.h> #include <linux/netdevice.h> +#define CAN_TX_QUEUE_LEN 10 /* default length for hardware interfaces */ + /* * CAN mode */ enum can_mode { CAN_MODE_STOP = 0, @@ -98,11 +100,23 @@ static inline u32 can_get_static_ctrlmode(struct can_priv *priv) static inline bool can_is_canxl_dev_mtu(unsigned int mtu) { return (mtu >= CANXL_MIN_MTU && mtu <= CANXL_MAX_MTU); } -void can_setup(struct net_device *dev); +void can_setup(struct net_device *dev) +{ + dev->type = ARPHRD_CAN; + dev->mtu = CAN_MTU; + dev->min_mtu = CAN_MTU; + dev->max_mtu = CAN_MTU; + dev->hard_header_len = 0; + dev->addr_len = 0; + + /* New-style flags. */ + dev->flags = IFF_NOARP; + dev->features = NETIF_F_HW_CSUM; +} struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, unsigned int txqs, unsigned int rxqs); #define alloc_candev(sizeof_priv, echo_skb_max) \ alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1) ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-05 16:17 ` Oliver Hartkopp @ 2026-08-05 21:06 ` Vincent Mailhol 2026-08-06 12:01 ` Oliver Hartkopp 0 siblings, 1 reply; 15+ messages in thread From: Vincent Mailhol @ 2026-08-05 21:06 UTC (permalink / raw) To: Oliver Hartkopp, Vincent Mailhol, Marc Kleine-Budde Cc: linux-can, linux-kernel On 05/08/2026 at 18:17, Oliver Hartkopp wrote: > On 05.08.26 09:25, Vincent Mailhol wrote: >> On 05/08/2026 at 08:29, Oliver Hartkopp wrote: > >>> 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 looks like a hack reverting bit settings. This was more to bounce on your remark that we need proper comments. I still prefer a line of code rather than a comment tight to nothing. But IFF_ECHO is the symptom, not the root cause. It is probably not this part which needs to be commented but the overall skb echo logic. >> 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. > > I'm not really done with my thoughts but ... > > IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO > flag and the default queue len. For IFF_ECHO, this is exactly what this series does! For the default queue len, why not. I have not study this particular topic. But I think the IFF_ECHO and the queue len should be in separate series. > What puzzles me is that the slcan driver is something in between which > is neither a real CAN hardware nor a virtual CAN interface. My understanding it that devices which do not have a TX completion handler (like slcan or can327) have no benefits to implement the echo_skb framework and can instead simply rely on the PF_CAN core. > My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only for > real CAN hardware devices and open code slcan and the virtual CAN > drivers ... which goes into the direction below. > > Any thoughts? The logic I tried to follow in this series is that alloc_candev{,_mqs}() has two arguments: 1. one for the priv structure 2. one for the number of echo_skb But then, when 2. is zero: alloc_candev{,_mqs}(..., 0) means to me: give me all the features expect from the echo_skb. With the above, there is no anomalies to see the slcan do: dev = alloc_candev(sizeof(*sl), 0); So I don't see the point to open code the allocations in slcan. After patch #1 which corrects the echo skb count, the code describes correctly the behaviour. > Best regards, > Oliver > > > diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c > index 769745e22a3c..5bdbe0c1d197 100644 > --- a/drivers/net/can/dev/dev.c > +++ b/drivers/net/can/dev/dev.c > @@ -277,25 +277,10 @@ void can_bus_off(struct net_device *dev) > schedule_delayed_work(&priv->restart_work, > msecs_to_jiffies(priv->restart_ms)); > } > EXPORT_SYMBOL_GPL(can_bus_off); > > -void can_setup(struct net_device *dev) > -{ > - dev->type = ARPHRD_CAN; > - dev->mtu = CAN_MTU; > - dev->min_mtu = CAN_MTU; > - dev->max_mtu = CAN_MTU; > - dev->hard_header_len = 0; > - dev->addr_len = 0; > - dev->tx_queue_len = 10; > - > - /* New-style flags. */ > - dev->flags = IFF_NOARP; > - dev->features = NETIF_F_HW_CSUM; > -} > - > /* Allocate and setup space for the CAN network device */ > struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int > echo_skb_max, > unsigned int txqs, unsigned int rxqs) > { > struct can_ml_priv *can_ml; > @@ -332,10 +317,13 @@ struct net_device *alloc_candev_mqs(int > sizeof_priv, unsigned int echo_skb_max, > > can_ml = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN); > can_set_ml_priv(dev, can_ml); > can_set_cap(dev, CAN_CAP_CC); > > + dev->tx_queue_len = CAN_TX_QUEUE_LEN; > + dev->flags |= IFF_ECHO; I really prefer to have the IFF_ECHO gated under the if (echo_skb_max) { because it is tightly linked to the echo skb framework. And yes, there are a couple drivers here and there which set IFF_ECHO without using the echo skb framework. But these are the drivers which implements their own custom echo skb logic. So it makes sense to have them open code the IFF_ECHO because they are also open coding the rest of the echo skb logic. This goes back to my previous point that: alloc_candev{,_mqs}(..., 0) means that the drivers do not use the framework echo skb. Such drivers fall in two categories: - No echo skb at all (e.g. slcan or can327): no IFF_ECHO - custom echo skb (e.g. grcan, janz-ican3): everything is open coded -> explicit IFF_ECHO flag > if (echo_skb_max) { > priv->echo_skb_max = echo_skb_max; > priv->echo_skb = (void *)priv + > (size - echo_skb_max * sizeof(struct sk_buff *)); > } > diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c > index 76e6b7b5c6a1..70263813ec40 100644 > --- a/drivers/net/can/vcan.c > +++ b/drivers/net/can/vcan.c > @@ -167,16 +167,15 @@ static const struct ethtool_ops vcan_ethtool_ops = { > .get_ts_info = ethtool_op_get_ts_info, > }; > > static void vcan_setup(struct net_device *dev) > { > - dev->type = ARPHRD_CAN; > - dev->mtu = CANXL_MTU; > - dev->hard_header_len = 0; > - dev->addr_len = 0; > - dev->tx_queue_len = 0; > - dev->flags = IFF_NOARP; > + can_setup(dev); > + dev->tx_queue_len = 0; > + dev->mtu = CANXL_MTU; > + dev->min_mtu = CAN_MTU; > + dev->max_mtu = CANXL_MTU; > can_set_ml_priv(dev, netdev_priv(dev)); > vcan_set_cap_info(dev); In such example, please don't add parasite white space changes. It makes it hard to grasp what you are actually modifying. > /* set flags according to driver capabilities */ > if (echo) > diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c > index e882250180ef..615a906203fa 100644 > --- a/drivers/net/can/vxcan.c > +++ b/drivers/net/can/vxcan.c > @@ -180,19 +180,18 @@ static const struct ethtool_ops vxcan_ethtool_ops = { > > static void vxcan_setup(struct net_device *dev) > { > struct can_ml_priv *can_ml; > > - dev->type = ARPHRD_CAN; > - dev->mtu = CANXL_MTU; > - dev->hard_header_len = 0; > - dev->addr_len = 0; > - dev->tx_queue_len = 0; > - dev->flags = IFF_NOARP; > - dev->netdev_ops = &vxcan_netdev_ops; > - dev->ethtool_ops = &vxcan_ethtool_ops; > - dev->needs_free_netdev = true; > + can_setup(dev); > + dev->tx_queue_len = 0; > + dev->mtu = CANXL_MTU; > + dev->min_mtu = CAN_MTU; > + dev->max_mtu = CANXL_MTU; > + dev->netdev_ops = &vxcan_netdev_ops; > + dev->ethtool_ops = &vxcan_ethtool_ops; > + dev->needs_free_netdev = true; > > can_ml = netdev_priv(dev) + ALIGN(sizeof(struct vxcan_priv), > NETDEV_ALIGN); > can_set_ml_priv(dev, can_ml); > vxcan_set_cap_info(dev); > } > diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h > index 6d0710d6f571..4619a74599cb 100644 > --- a/include/linux/can/dev.h > +++ b/include/linux/can/dev.h > @@ -21,10 +21,12 @@ > #include <linux/can/netlink.h> > #include <linux/can/skb.h> > #include <linux/ethtool.h> > #include <linux/netdevice.h> > > +#define CAN_TX_QUEUE_LEN 10 /* default length for hardware interfaces */ > + > /* > * CAN mode > */ > enum can_mode { > CAN_MODE_STOP = 0, > @@ -98,11 +100,23 @@ static inline u32 can_get_static_ctrlmode(struct > can_priv *priv) > static inline bool can_is_canxl_dev_mtu(unsigned int mtu) > { > return (mtu >= CANXL_MIN_MTU && mtu <= CANXL_MAX_MTU); > } > > -void can_setup(struct net_device *dev); > +void can_setup(struct net_device *dev) > +{ > + dev->type = ARPHRD_CAN; > + dev->mtu = CAN_MTU; > + dev->min_mtu = CAN_MTU; > + dev->max_mtu = CAN_MTU; > + dev->hard_header_len = 0; > + dev->addr_len = 0; > + > + /* New-style flags. */ > + dev->flags = IFF_NOARP; > + dev->features = NETIF_F_HW_CSUM; > +} It is strange to have a non static inline function in a header. What was the motivation for pulling this out of dev.c? > struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int > echo_skb_max, > unsigned int txqs, unsigned int rxqs); > #define alloc_candev(sizeof_priv, echo_skb_max) \ > alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1) > Yours sincerely, Vincent Mailhol ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-05 21:06 ` Vincent Mailhol @ 2026-08-06 12:01 ` Oliver Hartkopp 2026-08-06 20:55 ` Vincent Mailhol 0 siblings, 1 reply; 15+ messages in thread From: Oliver Hartkopp @ 2026-08-06 12:01 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can -CC linux-kernel@vger.kernel.org On 05.08.26 23:06, Vincent Mailhol wrote: > On 05/08/2026 at 18:17, Oliver Hartkopp wrote: >> IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO >> flag and the default queue len. > > For IFF_ECHO, this is exactly what this series does! I just wanted to second you. This does not mean that I fully support the way it is implemented. > For the default queue len, why not. I have not study this particular > topic. But I think the IFF_ECHO and the queue len should be in separate > series. My patch does not even compile. I just wanted to lead the dicsussion into a direction to find a more versatile solution that covers virtual CAN interfaces, non-echo CAN interfaces and full featured (echo'ing) CAn interfaces. >> What puzzles me is that the slcan driver is something in between which >> is neither a real CAN hardware nor a virtual CAN interface. > > My understanding it that devices which do not have a TX completion > handler (like slcan or can327) have no benefits to implement the > echo_skb framework and can instead simply rely on the PF_CAN core. Right. >> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only for >> real CAN hardware devices and open code slcan and the virtual CAN >> drivers ... which goes into the direction below. >> >> Any thoughts? > > The logic I tried to follow in this series is that alloc_candev{,_mqs}() > has two arguments: > > 1. one for the priv structure > > 2. one for the number of echo_skb > > But then, when 2. is zero: > > alloc_candev{,_mqs}(..., 0) > > means to me: give me all the features expect from the echo_skb. > > With the above, there is no anomalies to see the slcan do: > > dev = alloc_candev(sizeof(*sl), 0); > > So I don't see the point to open code the allocations in slcan. After > patch #1 which corrects the echo skb count, the code describes correctly > the behaviour. To me ", 0);" is a silent switch which does not make clear that slcan and can327 do something different here. We have 4 features: - support of IFF_ECHO mode using echo_skb's - support setting of bitrates via netlink - support setting of whatever via ethtool - use of TX queues (tx_queue_len != 0) And I would like these features to be separately selected to be transparent about what the CAN driver needs and supports. E.g. by defining a wrapper/define dev = alloc_non_echo_candev(sizeof(*sl)); which calls dev = alloc_candev(sizeof(*sl), 0); And the same applies to the other features. >> + dev->tx_queue_len = CAN_TX_QUEUE_LEN; > > > >> + dev->flags |= IFF_ECHO; > > I really prefer to have the IFF_ECHO gated under the > > if (echo_skb_max) { > > because it is tightly linked to the echo skb framework. Definitely not. This is not what I meant with transparency. > And yes, there are a couple drivers here and there which set IFF_ECHO > without using the echo skb framework. But these are the drivers which > implements their own custom echo skb logic. So it makes sense to have > them open code the IFF_ECHO because they are also open coding the rest > of the echo skb logic. > > This goes back to my previous point that: > > alloc_candev{,_mqs}(..., 0) > > means that the drivers do not use the framework echo skb. Such drivers > fall in two categories: > > - No echo skb at all (e.g. slcan or can327): no IFF_ECHO > > - custom echo skb (e.g. grcan, janz-ican3): everything is open coded > -> explicit IFF_ECHO flag > And that's why I would like to split these things up - at least by naming them differently. >> if (echo_skb_max) { >> priv->echo_skb_max = echo_skb_max; >> priv->echo_skb = (void *)priv + >> (size - echo_skb_max * sizeof(struct sk_buff *)); >> } >> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c >> index 76e6b7b5c6a1..70263813ec40 100644 >> --- a/drivers/net/can/vcan.c >> +++ b/drivers/net/can/vcan.c >> @@ -167,16 +167,15 @@ static const struct ethtool_ops vcan_ethtool_ops = { >> .get_ts_info = ethtool_op_get_ts_info, >> }; >> >> static void vcan_setup(struct net_device *dev) >> { >> - dev->type = ARPHRD_CAN; >> - dev->mtu = CANXL_MTU; >> - dev->hard_header_len = 0; >> - dev->addr_len = 0; >> - dev->tx_queue_len = 0; >> - dev->flags = IFF_NOARP; >> + can_setup(dev); >> + dev->tx_queue_len = 0; >> + dev->mtu = CANXL_MTU; >> + dev->min_mtu = CAN_MTU; >> + dev->max_mtu = CANXL_MTU; >> can_set_ml_priv(dev, netdev_priv(dev)); >> vcan_set_cap_info(dev); > > In such example, please don't add parasite white space changes. It makes > it hard to grasp what you are actually modifying. Agreed. As I wrote above - it does not even compile and was never intended to be used as upstream code. (..) >> -void can_setup(struct net_device *dev); >> +void can_setup(struct net_device *dev) >> +{ >> + dev->type = ARPHRD_CAN; >> + dev->mtu = CAN_MTU; >> + dev->min_mtu = CAN_MTU; >> + dev->max_mtu = CAN_MTU; >> + dev->hard_header_len = 0; >> + dev->addr_len = 0; >> + >> + /* New-style flags. */ >> + dev->flags = IFF_NOARP; >> + dev->features = NETIF_F_HW_CSUM; >> +} > > It is strange to have a non static inline function in a header. What was > the motivation for pulling this out of dev.c? Yeah. My thought was that we might reduce code duplication when we provice more granularity in those helper functions. I moved it to dev.h to avoid the building of dev.c for the virtual CAN interfaces. Best regards, Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-06 12:01 ` Oliver Hartkopp @ 2026-08-06 20:55 ` Vincent Mailhol 2026-08-07 10:56 ` Oliver Hartkopp 0 siblings, 1 reply; 15+ messages in thread From: Vincent Mailhol @ 2026-08-06 20:55 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde; +Cc: linux-can On 06/08/2026 at 14:01, Oliver Hartkopp wrote:> On 05.08.26 23:06, Vincent Mailhol wrote: >> On 05/08/2026 at 18:17, Oliver Hartkopp wrote: > >>> IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO >>> flag and the default queue len. >> >> For IFF_ECHO, this is exactly what this series does! > > I just wanted to second you. This does not mean that I fully support the > way it is implemented. > >> For the default queue len, why not. I have not study this particular >> topic. But I think the IFF_ECHO and the queue len should be in separate >> series. > > My patch does not even compile. I just wanted to lead the dicsussion > into a direction to find a more versatile solution that covers virtual > CAN interfaces, non-echo CAN interfaces and full featured (echo'ing) CAn > interfaces. > >>> What puzzles me is that the slcan driver is something in between which >>> is neither a real CAN hardware nor a virtual CAN interface. >> >> My understanding it that devices which do not have a TX completion >> handler (like slcan or can327) have no benefits to implement the >> echo_skb framework and can instead simply rely on the PF_CAN core. > > Right. > >>> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only for >>> real CAN hardware devices and open code slcan and the virtual CAN >>> drivers ... which goes into the direction below. >>> >>> Any thoughts? >> >> The logic I tried to follow in this series is that alloc_candev{,_mqs}() >> has two arguments: >> >> 1. one for the priv structure >> >> 2. one for the number of echo_skb >> >> But then, when 2. is zero: >> >> alloc_candev{,_mqs}(..., 0) >> >> means to me: give me all the features expect from the echo_skb. >> >> With the above, there is no anomalies to see the slcan do: >> >> dev = alloc_candev(sizeof(*sl), 0); >> >> So I don't see the point to open code the allocations in slcan. After >> patch #1 which corrects the echo skb count, the code describes correctly >> the behaviour. > > To me ", 0);" is a silent switch which does not make clear that slcan > and can327 do something different here. > > We have 4 features: > > - support of IFF_ECHO mode using echo_skb's > - support setting of bitrates via netlink > - support setting of whatever via ethtool > - use of TX queues (tx_queue_len != 0) > > And I would like these features to be separately selected to be > transparent about what the CAN driver needs and supports. > > E.g. by defining a wrapper/define > > dev = alloc_non_echo_candev(sizeof(*sl)); > > which calls > > dev = alloc_candev(sizeof(*sl), 0); Going this way, it should be the other way around. Have: alloc_candev(sizeof(*foo)); which just does the basic things and then: alloc_candev_echo_skb(sizeof(*bar), 0); which allocate the echo skbs on top of the basic things. To me, the alloc_non_echo_candev() feels a bit like my previous dev->flags &= ~IFF_ECHO; in the sense that it is not additive but subtractive. > And the same applies to the other features. But then, you reach a problem. If you do the Cartesian product of all the 4 features, you end up with 2^4 = 16 combinations. Of course, some of the combinations will not be used. But I definitely prefer a smaller set of functions which can adjust their behaviour based on their parameters value rather than multiply the number of functions. I would rather have one function with four parameters rather than starting to add one function per combination we need. Back to the echo skb, the prototype is: struct net_device * alloc_candev(int sizeof_priv, unsigned int echo_skb_max); So: alloc_candev(sizeof(*sl), 0); literally means that we are allocating a candev with a private scruture of sizeof(*sl) and with zero echo skb. I really fail to understand your point that setting echo_skb_max to zero is not transparent but that alloc_non_echo_candev() is. *echo_skb_max = 0* and *non_echo* are perfect synonymous to me, except that the first one allows for a more compact implementation. Yours sincerely, Vincent Mailhol ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-06 20:55 ` Vincent Mailhol @ 2026-08-07 10:56 ` Oliver Hartkopp 2026-08-07 11:52 ` Vincent Mailhol 0 siblings, 1 reply; 15+ messages in thread From: Oliver Hartkopp @ 2026-08-07 10:56 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can On 06.08.26 22:55, Vincent Mailhol wrote: > On 06/08/2026 at 14:01, Oliver Hartkopp wrote:> On 05.08.26 23:06, > Vincent Mailhol wrote: >>> On 05/08/2026 at 18:17, Oliver Hartkopp wrote: >> >>>> IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO >>>> flag and the default queue len. >>> >>> For IFF_ECHO, this is exactly what this series does! >> >> I just wanted to second you. This does not mean that I fully support the >> way it is implemented. >> >>> For the default queue len, why not. I have not study this particular >>> topic. But I think the IFF_ECHO and the queue len should be in separate >>> series. >> >> My patch does not even compile. I just wanted to lead the dicsussion >> into a direction to find a more versatile solution that covers virtual >> CAN interfaces, non-echo CAN interfaces and full featured (echo'ing) CAn >> interfaces. >> >>>> What puzzles me is that the slcan driver is something in between which >>>> is neither a real CAN hardware nor a virtual CAN interface. >>> >>> My understanding it that devices which do not have a TX completion >>> handler (like slcan or can327) have no benefits to implement the >>> echo_skb framework and can instead simply rely on the PF_CAN core. >> >> Right. >> >>>> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only for >>>> real CAN hardware devices and open code slcan and the virtual CAN >>>> drivers ... which goes into the direction below. >>>> >>>> Any thoughts? >>> >>> The logic I tried to follow in this series is that alloc_candev{,_mqs}() >>> has two arguments: >>> >>> 1. one for the priv structure >>> >>> 2. one for the number of echo_skb >>> >>> But then, when 2. is zero: >>> >>> alloc_candev{,_mqs}(..., 0) >>> >>> means to me: give me all the features expect from the echo_skb. >>> >>> With the above, there is no anomalies to see the slcan do: >>> >>> dev = alloc_candev(sizeof(*sl), 0); >>> >>> So I don't see the point to open code the allocations in slcan. After >>> patch #1 which corrects the echo skb count, the code describes correctly >>> the behaviour. >> >> To me ", 0);" is a silent switch which does not make clear that slcan >> and can327 do something different here. >> >> We have 4 features: >> >> - support of IFF_ECHO mode using echo_skb's >> - support setting of bitrates via netlink >> - support setting of whatever via ethtool >> - use of TX queues (tx_queue_len != 0) >> >> And I would like these features to be separately selected to be >> transparent about what the CAN driver needs and supports. >> >> E.g. by defining a wrapper/define >> >> dev = alloc_non_echo_candev(sizeof(*sl)); >> >> which calls >> >> dev = alloc_candev(sizeof(*sl), 0); > > Going this way, it should be the other way around. Have: > > alloc_candev(sizeof(*foo)); > > which just does the basic things and then: > > alloc_candev_echo_skb(sizeof(*bar), 0); > > which allocate the echo skbs on top of the basic things. > > To me, the alloc_non_echo_candev() feels a bit like my previous > > dev->flags &= ~IFF_ECHO; > > in the sense that it is not additive but subtractive. > >> And the same applies to the other features. > > But then, you reach a problem. If you do the Cartesian product of all > the 4 features, you end up with 2^4 = 16 combinations. > > Of course, some of the combinations will not be used. You likely got me wrong. We still have only about 3 cases that use those 4+ features: - support of IFF_ECHO mode using echo_skb's - support setting of bitrates via netlink - support setting of whatever via ethtool - use of TX queues (tx_queue_len != 0) My idea would be to have different functions to make clear what each of these drivers use. And not hide flags based on the number of echo skbs or shrink the number of helper functions by adding parameters. E.g. vcan calls: /* sizeof(struct can_ml_priv) is defined in vcan_link_ops */ can_set_ml_priv(dev, netdev_priv(dev)); can_setup(dev, (echo)?IFF_ECHO:0); vcan_set_mtu_info(dev); vcan_set_cap_info(dev); dev->tx_queue_len = 0; slcan calls: dev = alloc_candev(sizeof(struct slcan_priv)); can_setup(dev, 0); slcan_set_mtu_info(dev); slcan_set_cap_info(dev); dev->tx_queue_len = CAN_TX_QUEUE_LEN; m_can calls: dev = alloc_candev_echo_skb(sizeof(struct m_can_priv), 4); can_setup(dev, IFF_ECHO); m_can_set_mtu_info(dev); m_can_set_cap_info(dev); dev->tx_queue_len = CAN_TX_QUEUE_LEN; This is what I meant with transparency and code deduplication. E.g. where can_setup() has an extra_flags parameter which is simply or'ed to IFF_NOARP. That code needs to be invoked in all those cases anyway but I would like to make it visible and transparent which features and flags are enabled for which reason. Maybe _set_mtu_info(dev) and _set_cap_info(dev) could be merged. Best regards, Oliver ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-07 10:56 ` Oliver Hartkopp @ 2026-08-07 11:52 ` Vincent Mailhol 2026-08-10 18:07 ` Oliver Hartkopp 0 siblings, 1 reply; 15+ messages in thread From: Vincent Mailhol @ 2026-08-07 11:52 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde; +Cc: linux-can On 07/08/2026 at 12:56, Oliver Hartkopp wrote: > On 06.08.26 22:55, Vincent Mailhol wrote: >> On 06/08/2026 at 14:01, Oliver Hartkopp wrote:> On 05.08.26 23:06, >> Vincent Mailhol wrote: >>>> On 05/08/2026 at 18:17, Oliver Hartkopp wrote: >>> >>>>> IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO >>>>> flag and the default queue len. >>>> >>>> For IFF_ECHO, this is exactly what this series does! >>> >>> I just wanted to second you. This does not mean that I fully support the >>> way it is implemented. >>> >>>> For the default queue len, why not. I have not study this particular >>>> topic. But I think the IFF_ECHO and the queue len should be in separate >>>> series. >>> >>> My patch does not even compile. I just wanted to lead the dicsussion >>> into a direction to find a more versatile solution that covers virtual >>> CAN interfaces, non-echo CAN interfaces and full featured (echo'ing) CAn >>> interfaces. >>> >>>>> What puzzles me is that the slcan driver is something in between which >>>>> is neither a real CAN hardware nor a virtual CAN interface. >>>> >>>> My understanding it that devices which do not have a TX completion >>>> handler (like slcan or can327) have no benefits to implement the >>>> echo_skb framework and can instead simply rely on the PF_CAN core. >>> >>> Right. >>> >>>>> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only >>>>> for >>>>> real CAN hardware devices and open code slcan and the virtual CAN >>>>> drivers ... which goes into the direction below. >>>>> >>>>> Any thoughts? >>>> >>>> The logic I tried to follow in this series is that >>>> alloc_candev{,_mqs}() >>>> has two arguments: >>>> >>>> 1. one for the priv structure >>>> >>>> 2. one for the number of echo_skb >>>> >>>> But then, when 2. is zero: >>>> >>>> alloc_candev{,_mqs}(..., 0) >>>> >>>> means to me: give me all the features expect from the echo_skb. >>>> >>>> With the above, there is no anomalies to see the slcan do: >>>> >>>> dev = alloc_candev(sizeof(*sl), 0); >>>> >>>> So I don't see the point to open code the allocations in slcan. After >>>> patch #1 which corrects the echo skb count, the code describes >>>> correctly >>>> the behaviour. >>> >>> To me ", 0);" is a silent switch which does not make clear that slcan >>> and can327 do something different here. >>> >>> We have 4 features: >>> >>> - support of IFF_ECHO mode using echo_skb's >>> - support setting of bitrates via netlink >>> - support setting of whatever via ethtool >>> - use of TX queues (tx_queue_len != 0) >>> >>> And I would like these features to be separately selected to be >>> transparent about what the CAN driver needs and supports. >>> >>> E.g. by defining a wrapper/define >>> >>> dev = alloc_non_echo_candev(sizeof(*sl)); >>> >>> which calls >>> >>> dev = alloc_candev(sizeof(*sl), 0); >> >> Going this way, it should be the other way around. Have: >> >> alloc_candev(sizeof(*foo)); >> >> which just does the basic things and then: >> >> alloc_candev_echo_skb(sizeof(*bar), 0); >> >> which allocate the echo skbs on top of the basic things. >> >> To me, the alloc_non_echo_candev() feels a bit like my previous >> >> dev->flags &= ~IFF_ECHO; >> >> in the sense that it is not additive but subtractive. >> >>> And the same applies to the other features. >> >> But then, you reach a problem. If you do the Cartesian product of all >> the 4 features, you end up with 2^4 = 16 combinations. >> >> Of course, some of the combinations will not be used. > > You likely got me wrong. > > We still have only about 3 cases that use those 4+ features: > > - support of IFF_ECHO mode using echo_skb's > - support setting of bitrates via netlink > - support setting of whatever via ethtool > - use of TX queues (tx_queue_len != 0) > > My idea would be to have different functions to make clear what each of > these drivers use. And not hide flags based on the number of echo skbs > or shrink the number of helper functions by adding parameters. > > E.g. > > vcan calls: > > /* sizeof(struct can_ml_priv) is defined in vcan_link_ops */ > can_set_ml_priv(dev, netdev_priv(dev)); > can_setup(dev, (echo)?IFF_ECHO:0); > vcan_set_mtu_info(dev); > vcan_set_cap_info(dev); > dev->tx_queue_len = 0; > > slcan calls: > > dev = alloc_candev(sizeof(struct slcan_priv)); > can_setup(dev, 0); > slcan_set_mtu_info(dev); > slcan_set_cap_info(dev); > dev->tx_queue_len = CAN_TX_QUEUE_LEN; > > > m_can calls: > > dev = alloc_candev_echo_skb(sizeof(struct m_can_priv), 4); > can_setup(dev, IFF_ECHO); > m_can_set_mtu_info(dev); > m_can_set_cap_info(dev); > dev->tx_queue_len = CAN_TX_QUEUE_LEN; > > > This is what I meant with transparency and code deduplication. > E.g. where can_setup() has an extra_flags parameter which is simply > or'ed to IFF_NOARP. Now I understand. But I don't like the idea. If I understand correctly, for the average driver, we will replace one call to: alloc_candev_echo_skb() into roughly four calls. My goal in this series was to reduce boiler plate while making the framework more robust. Your proposal increases the boilerplate and reduces the robustest. Forgetting any one of these setup function is also a potential security issue. As a concrete example, this already occurred in the past with several drivers which forget to populate their MTU, for example: commit 17c8d794527f ("can: mcba_usb: populate ndo_change_mtu() to prevent buffer overflow"). I modified the framework so that the MTU is now correctly set by default in commit 23049938605b ("can: populate the minimum and maximum MTU values") so that today, it is now impossible for a driver to incorrectly set its MTU. Introducing a m_can_set_mtu_info() would be going backward to me. We would open back the gate for a kind of bug which is today closed. And yes, the v(x)can remains special case which need to open code the MTU, which is fine as these are really special. But for the majority, it is a winning choice to "hide" it in the framework rather than take the risk to trust the drivers to do the right thing. And so, my though for IFF_ECHO is exactly the same. If it is open coded, this is a risk (ok, it is less critical than the MTU, but still can lead to unexpected behaviour). And so, my wish is for IFF_ECHO to follow the same path as what was done last year for the MTU: handle it in the framework and forgot this class of bug for the vast majority of the drivers. > That code needs to be invoked in all those cases anyway but I would like > to make it visible and transparent which features and flags are enabled > for which reason. Yours sincerely, Vincent Mailhol ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs 2026-08-07 11:52 ` Vincent Mailhol @ 2026-08-10 18:07 ` Oliver Hartkopp 0 siblings, 0 replies; 15+ messages in thread From: Oliver Hartkopp @ 2026-08-10 18:07 UTC (permalink / raw) To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can On 07.08.26 13:52, Vincent Mailhol wrote: > On 07/08/2026 at 12:56, Oliver Hartkopp wrote: >> On 06.08.26 22:55, Vincent Mailhol wrote: >>> On 06/08/2026 at 14:01, Oliver Hartkopp wrote:> On 05.08.26 23:06, >>> Vincent Mailhol wrote: >>>>> On 05/08/2026 at 18:17, Oliver Hartkopp wrote: >>>> >>>>>> IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO >>>>>> flag and the default queue len. >>>>> >>>>> For IFF_ECHO, this is exactly what this series does! >>>> >>>> I just wanted to second you. This does not mean that I fully support the >>>> way it is implemented. >>>> >>>>> For the default queue len, why not. I have not study this particular >>>>> topic. But I think the IFF_ECHO and the queue len should be in separate >>>>> series. >>>> >>>> My patch does not even compile. I just wanted to lead the dicsussion >>>> into a direction to find a more versatile solution that covers virtual >>>> CAN interfaces, non-echo CAN interfaces and full featured (echo'ing) CAn >>>> interfaces. >>>> >>>>>> What puzzles me is that the slcan driver is something in between which >>>>>> is neither a real CAN hardware nor a virtual CAN interface. >>>>> >>>>> My understanding it that devices which do not have a TX completion >>>>> handler (like slcan or can327) have no benefits to implement the >>>>> echo_skb framework and can instead simply rely on the PF_CAN core. >>>> >>>> Right. >>>> >>>>>> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only >>>>>> for >>>>>> real CAN hardware devices and open code slcan and the virtual CAN >>>>>> drivers ... which goes into the direction below. >>>>>> >>>>>> Any thoughts? >>>>> >>>>> The logic I tried to follow in this series is that >>>>> alloc_candev{,_mqs}() >>>>> has two arguments: >>>>> >>>>> 1. one for the priv structure >>>>> >>>>> 2. one for the number of echo_skb >>>>> >>>>> But then, when 2. is zero: >>>>> >>>>> alloc_candev{,_mqs}(..., 0) >>>>> >>>>> means to me: give me all the features expect from the echo_skb. >>>>> >>>>> With the above, there is no anomalies to see the slcan do: >>>>> >>>>> dev = alloc_candev(sizeof(*sl), 0); >>>>> >>>>> So I don't see the point to open code the allocations in slcan. After >>>>> patch #1 which corrects the echo skb count, the code describes >>>>> correctly >>>>> the behaviour. >>>> >>>> To me ", 0);" is a silent switch which does not make clear that slcan >>>> and can327 do something different here. >>>> >>>> We have 4 features: >>>> >>>> - support of IFF_ECHO mode using echo_skb's >>>> - support setting of bitrates via netlink >>>> - support setting of whatever via ethtool >>>> - use of TX queues (tx_queue_len != 0) >>>> >>>> And I would like these features to be separately selected to be >>>> transparent about what the CAN driver needs and supports. >>>> >>>> E.g. by defining a wrapper/define >>>> >>>> dev = alloc_non_echo_candev(sizeof(*sl)); >>>> >>>> which calls >>>> >>>> dev = alloc_candev(sizeof(*sl), 0); >>> >>> Going this way, it should be the other way around. Have: >>> >>> alloc_candev(sizeof(*foo)); >>> >>> which just does the basic things and then: >>> >>> alloc_candev_echo_skb(sizeof(*bar), 0); >>> >>> which allocate the echo skbs on top of the basic things. >>> >>> To me, the alloc_non_echo_candev() feels a bit like my previous >>> >>> dev->flags &= ~IFF_ECHO; >>> >>> in the sense that it is not additive but subtractive. >>> >>>> And the same applies to the other features. >>> >>> But then, you reach a problem. If you do the Cartesian product of all >>> the 4 features, you end up with 2^4 = 16 combinations. >>> >>> Of course, some of the combinations will not be used. >> >> You likely got me wrong. >> >> We still have only about 3 cases that use those 4+ features: >> >> - support of IFF_ECHO mode using echo_skb's >> - support setting of bitrates via netlink >> - support setting of whatever via ethtool >> - use of TX queues (tx_queue_len != 0) >> >> My idea would be to have different functions to make clear what each of >> these drivers use. And not hide flags based on the number of echo skbs >> or shrink the number of helper functions by adding parameters. >> >> E.g. >> >> vcan calls: >> >> /* sizeof(struct can_ml_priv) is defined in vcan_link_ops */ >> can_set_ml_priv(dev, netdev_priv(dev)); >> can_setup(dev, (echo)?IFF_ECHO:0); >> vcan_set_mtu_info(dev); >> vcan_set_cap_info(dev); >> dev->tx_queue_len = 0; >> >> slcan calls: >> >> dev = alloc_candev(sizeof(struct slcan_priv)); >> can_setup(dev, 0); >> slcan_set_mtu_info(dev); >> slcan_set_cap_info(dev); >> dev->tx_queue_len = CAN_TX_QUEUE_LEN; >> >> >> m_can calls: >> >> dev = alloc_candev_echo_skb(sizeof(struct m_can_priv), 4); >> can_setup(dev, IFF_ECHO); >> m_can_set_mtu_info(dev); >> m_can_set_cap_info(dev); >> dev->tx_queue_len = CAN_TX_QUEUE_LEN; >> >> >> This is what I meant with transparency and code deduplication. >> E.g. where can_setup() has an extra_flags parameter which is simply >> or'ed to IFF_NOARP. > > Now I understand. But I don't like the idea. If I understand correctly, > for the average driver, we will replace one call to: > > alloc_candev_echo_skb() > > into roughly four calls. > > My goal in this series was to reduce boiler plate while making the > framework more robust. Your proposal increases the boilerplate and > reduces the robustest. Forgetting any one of these setup function is > also a potential security issue. > > As a concrete example, this already occurred in the past with several > drivers which forget to populate their MTU, for example: commit > 17c8d794527f ("can: mcba_usb: populate ndo_change_mtu() to prevent > buffer overflow"). > > I modified the framework so that the MTU is now correctly set by default > in commit 23049938605b ("can: populate the minimum and maximum MTU > values") so that today, it is now impossible for a driver to incorrectly > set its MTU. > > Introducing a m_can_set_mtu_info() would be going backward to me. We > would open back the gate for a kind of bug which is today closed. > > And yes, the v(x)can remains special case which need to open code the > MTU, which is fine as these are really special. But for the majority, it > is a winning choice to "hide" it in the framework rather than take the > risk to trust the drivers to do the right thing. Yes. I understand. So having - alloc_candev_echo_skb() - alloc_candev() or maybe even better - alloc_candev(sizeof(..), num_skbs) - alloc_candev_no_echo(sizeof(..)) /* for slcan / can327 */ make sense. And with these different names the EFF_ECHO setting is not really hidden anymore, which was my concern. 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. And the reason to have it in dev.h was that this would not trigger some additional code compilation for v(x)can (beyond today's usage). Best regards, Oliver > > And so, my though for IFF_ECHO is exactly the same. If it is open coded, > this is a risk (ok, it is less critical than the MTU, but still can lead > to unexpected behaviour). And so, my wish is for IFF_ECHO to follow the > same path as what was done last year for the MTU: handle it in the > framework and forgot this class of bug for the vast majority of the drivers. > >> That code needs to be invoked in all those cases anyway but I would like >> to make it visible and transparent which features and flags are enabled >> for which reason. > > > Yours sincerely, > Vincent Mailhol > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-08-10 18:07 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-04 19:55 [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Vincent Mailhol 2026-08-04 19:55 ` [PATCH 1/4] can: slcan: do not allocate unused echo skb Vincent Mailhol 2026-08-04 19:55 ` [PATCH 2/4] can: fix IFF_ECHO example in documentation Vincent Mailhol 2026-08-05 6:08 ` Oliver Hartkopp 2026-08-04 19:55 ` [PATCH 3/4] can: dev: set IFF_ECHO when allocating echo skbs Vincent Mailhol 2026-08-04 19:55 ` [PATCH 4/4] can: treewide: remove redundant IFF_ECHO assignments Vincent Mailhol 2026-08-05 6:29 ` [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs Oliver Hartkopp 2026-08-05 7:25 ` Vincent Mailhol 2026-08-05 16:17 ` Oliver Hartkopp 2026-08-05 21:06 ` Vincent Mailhol 2026-08-06 12:01 ` Oliver Hartkopp 2026-08-06 20:55 ` Vincent Mailhol 2026-08-07 10:56 ` Oliver Hartkopp 2026-08-07 11:52 ` Vincent Mailhol 2026-08-10 18:07 ` Oliver Hartkopp
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.