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

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

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

[PATCH 1/4] can: slcan: do not allocate unused echo skb

[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

[PATCH 2/4] can: fix IFF_ECHO example in documentation

[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

[PATCH 3/4] can: dev: set IFF_ECHO when allocating echo skbs

[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

[PATCH 4/4] can: treewide: remove redundant IFF_ECHO assignments

[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

Re: [PATCH 2/4] can: fix IFF_ECHO example in documentation

[Oliver Hartkopp]

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

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

[Oliver Hartkopp]

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,

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

[Vincent Mailhol]

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

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

[Oliver Hartkopp]

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)

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

[Vincent Mailhol]

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