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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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)