Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs
From: Oliver Hartkopp
Date: Wed Aug 05 2026 - 12:30:08 EST
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)