* [PATCH net 01/10] can: hi311x: fix null pointer dereference when resuming from sleep before interface was enabled
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 02/10] can: rcar_canfd: Fix controller mode setting Marc Kleine-Budde
` (9 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Chen Yufeng, Marc Kleine-Budde
From: Chen Yufeng <chenyufeng@iie.ac.cn>
This issue is similar to the vulnerability in the `mcp251x` driver,
which was fixed in commit 03c427147b2d ("can: mcp251x: fix resume from
sleep before interface was brought up").
In the `hi311x` driver, when the device resumes from sleep, the driver
schedules `priv->restart_work`. However, if the network interface was
not previously enabled, the `priv->wq` (workqueue) is not allocated and
initialized, leading to a null pointer dereference.
To fix this, we move the allocation and initialization of the workqueue
from the `hi3110_open` function to the `hi3110_can_probe` function.
This ensures that the workqueue is properly initialized before it is
used during device resume. And added logic to destroy the workqueue
in the error handling paths of `hi3110_can_probe` and in the
`hi3110_can_remove` function to prevent resource leaks.
Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn>
Link: https://patch.msgid.link/20250911150820.250-1-chenyufeng@iie.ac.cn
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/spi/hi311x.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index 09ae218315d7..96bef8f384c4 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -545,8 +545,6 @@ static int hi3110_stop(struct net_device *net)
priv->force_quit = 1;
free_irq(spi->irq, priv);
- destroy_workqueue(priv->wq);
- priv->wq = NULL;
mutex_lock(&priv->hi3110_lock);
@@ -770,34 +768,23 @@ static int hi3110_open(struct net_device *net)
goto out_close;
}
- priv->wq = alloc_workqueue("hi3110_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
- 0);
- if (!priv->wq) {
- ret = -ENOMEM;
- goto out_free_irq;
- }
- INIT_WORK(&priv->tx_work, hi3110_tx_work_handler);
- INIT_WORK(&priv->restart_work, hi3110_restart_work_handler);
-
ret = hi3110_hw_reset(spi);
if (ret)
- goto out_free_wq;
+ goto out_free_irq;
ret = hi3110_setup(net);
if (ret)
- goto out_free_wq;
+ goto out_free_irq;
ret = hi3110_set_normal_mode(spi);
if (ret)
- goto out_free_wq;
+ goto out_free_irq;
netif_wake_queue(net);
mutex_unlock(&priv->hi3110_lock);
return 0;
- out_free_wq:
- destroy_workqueue(priv->wq);
out_free_irq:
free_irq(spi->irq, priv);
hi3110_hw_sleep(spi);
@@ -908,6 +895,15 @@ static int hi3110_can_probe(struct spi_device *spi)
if (ret)
goto out_clk;
+ priv->wq = alloc_workqueue("hi3110_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
+ 0);
+ if (!priv->wq) {
+ ret = -ENOMEM;
+ goto out_clk;
+ }
+ INIT_WORK(&priv->tx_work, hi3110_tx_work_handler);
+ INIT_WORK(&priv->restart_work, hi3110_restart_work_handler);
+
priv->spi = spi;
mutex_init(&priv->hi3110_lock);
@@ -943,6 +939,8 @@ static int hi3110_can_probe(struct spi_device *spi)
return 0;
error_probe:
+ destroy_workqueue(priv->wq);
+ priv->wq = NULL;
hi3110_power_enable(priv->power, 0);
out_clk:
@@ -963,6 +961,9 @@ static void hi3110_can_remove(struct spi_device *spi)
hi3110_power_enable(priv->power, 0);
+ destroy_workqueue(priv->wq);
+ priv->wq = NULL;
+
clk_disable_unprepare(priv->clk);
free_candev(net);
base-commit: cbf658dd09419f1ef9de11b9604e950bdd5c170b
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 02/10] can: rcar_canfd: Fix controller mode setting
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 01/10] can: hi311x: fix null pointer dereference when resuming from sleep before interface was enabled Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 03/10] can: etas_es58x: populate ndo_change_mtu() to prevent buffer overflow Marc Kleine-Budde
` (8 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Duy Nguyen, Tranh Ha,
Marc Kleine-Budde
From: Duy Nguyen <duy.nguyen.rh@renesas.com>
Driver configures register to choose controller mode before
setting all channels to reset mode leading to failure.
The patch corrects operation of mode setting.
Signed-off-by: Duy Nguyen <duy.nguyen.rh@renesas.com>
Signed-off-by: Tranh Ha <tranh.ha.xb@renesas.com>
Link: https://patch.msgid.link/TYWPR01MB87434739F83E27EDCD23DF44B416A@TYWPR01MB8743.jpnprd01.prod.outlook.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/rcar/rcar_canfd.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index b3c8c592fb0e..7e8b1d2f1af6 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -823,9 +823,6 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
/* Reset Global error flags */
rcar_canfd_write(gpriv->base, RCANFD_GERFL, 0x0);
- /* Set the controller into appropriate mode */
- rcar_canfd_set_mode(gpriv);
-
/* Transition all Channels to reset mode */
for_each_set_bit(ch, &gpriv->channels_mask, gpriv->info->max_channels) {
rcar_canfd_clear_bit(gpriv->base,
@@ -844,6 +841,10 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
return err;
}
}
+
+ /* Set the controller into appropriate mode */
+ rcar_canfd_set_mode(gpriv);
+
return 0;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 03/10] can: etas_es58x: populate ndo_change_mtu() to prevent buffer overflow
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 01/10] can: hi311x: fix null pointer dereference when resuming from sleep before interface was enabled Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 02/10] can: rcar_canfd: Fix controller mode setting Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 04/10] can: hi311x: " Marc Kleine-Budde
` (7 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Vincent Mailhol,
Marc Kleine-Budde
From: Vincent Mailhol <mailhol@kernel.org>
Sending an PF_PACKET allows to bypass the CAN framework logic and to
directly reach the xmit() function of a CAN driver. The only check
which is performed by the PF_PACKET framework is to make sure that
skb->len fits the interface's MTU.
Unfortunately, because the etas_es58x driver does not populate its
net_device_ops->ndo_change_mtu(), it is possible for an attacker to
configure an invalid MTU by doing, for example:
$ ip link set can0 mtu 9999
After doing so, the attacker could open a PF_PACKET socket using the
ETH_P_CANXL protocol:
socket(PF_PACKET, SOCK_RAW, htons(ETH_P_CANXL));
to inject a malicious CAN XL frames. For example:
struct canxl_frame frame = {
.flags = 0xff,
.len = 2048,
};
The CAN drivers' xmit() function are calling can_dev_dropped_skb() to
check that the skb is valid, unfortunately under above conditions, the
malicious packet is able to go through can_dev_dropped_skb() checks:
1. the skb->protocol is set to ETH_P_CANXL which is valid (the
function does not check the actual device capabilities).
2. the length is a valid CAN XL length.
And so, es58x_start_xmit() receives a CAN XL frame which it is not
able to correctly handle and will thus misinterpret it as a CAN(FD)
frame.
This can result in a buffer overflow. For example, using the es581.4
variant, the frame will be dispatched to es581_4_tx_can_msg(), go
through the last check at the beginning of this function:
if (can_is_canfd_skb(skb))
return -EMSGSIZE;
and reach this line:
memcpy(tx_can_msg->data, cf->data, cf->len);
Here, cf->len corresponds to the flags field of the CAN XL frame. In
our previous example, we set canxl_frame->flags to 0xff. Because the
maximum expected length is 8, a buffer overflow of 247 bytes occurs!
Populate net_device_ops->ndo_change_mtu() to ensure that the
interface's MTU can not be set to anything bigger than CAN_MTU or
CANFD_MTU (depending on the device capabilities). By fixing the root
cause, this prevents the buffer overflow.
Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
Link: https://patch.msgid.link/20250918-can-fix-mtu-v1-1-0d1cada9393b@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/etas_es58x/es58x_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index db1acf6d504c..adc91873c083 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -7,7 +7,7 @@
*
* Copyright (c) 2019 Robert Bosch Engineering and Business Solutions. All rights reserved.
* Copyright (c) 2020 ETAS K.K.. All rights reserved.
- * Copyright (c) 2020-2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (c) 2020-2025 Vincent Mailhol <mailhol@kernel.org>
*/
#include <linux/unaligned.h>
@@ -1977,6 +1977,7 @@ static const struct net_device_ops es58x_netdev_ops = {
.ndo_stop = es58x_stop,
.ndo_start_xmit = es58x_start_xmit,
.ndo_eth_ioctl = can_eth_ioctl_hwts,
+ .ndo_change_mtu = can_change_mtu,
};
static const struct ethtool_ops es58x_ethtool_ops = {
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 04/10] can: hi311x: populate ndo_change_mtu() to prevent buffer overflow
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (2 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 03/10] can: etas_es58x: populate ndo_change_mtu() to prevent buffer overflow Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 05/10] can: sun4i_can: " Marc Kleine-Budde
` (6 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Vincent Mailhol,
Marc Kleine-Budde
From: Vincent Mailhol <mailhol@kernel.org>
Sending an PF_PACKET allows to bypass the CAN framework logic and to
directly reach the xmit() function of a CAN driver. The only check
which is performed by the PF_PACKET framework is to make sure that
skb->len fits the interface's MTU.
Unfortunately, because the sun4i_can driver does not populate its
net_device_ops->ndo_change_mtu(), it is possible for an attacker to
configure an invalid MTU by doing, for example:
$ ip link set can0 mtu 9999
After doing so, the attacker could open a PF_PACKET socket using the
ETH_P_CANXL protocol:
socket(PF_PACKET, SOCK_RAW, htons(ETH_P_CANXL))
to inject a malicious CAN XL frames. For example:
struct canxl_frame frame = {
.flags = 0xff,
.len = 2048,
};
The CAN drivers' xmit() function are calling can_dev_dropped_skb() to
check that the skb is valid, unfortunately under above conditions, the
malicious packet is able to go through can_dev_dropped_skb() checks:
1. the skb->protocol is set to ETH_P_CANXL which is valid (the
function does not check the actual device capabilities).
2. the length is a valid CAN XL length.
And so, hi3110_hard_start_xmit() receives a CAN XL frame which it is
not able to correctly handle and will thus misinterpret it as a CAN
frame. The driver will consume frame->len as-is with no further
checks.
This can result in a buffer overflow later on in hi3110_hw_tx() on
this line:
memcpy(buf + HI3110_FIFO_EXT_DATA_OFF,
frame->data, frame->len);
Here, frame->len corresponds to the flags field of the CAN XL frame.
In our previous example, we set canxl_frame->flags to 0xff. Because
the maximum expected length is 8, a buffer overflow of 247 bytes
occurs!
Populate net_device_ops->ndo_change_mtu() to ensure that the
interface's MTU can not be set to anything bigger than CAN_MTU. By
fixing the root cause, this prevents the buffer overflow.
Fixes: 57e83fb9b746 ("can: hi311x: Add Holt HI-311x CAN driver")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
Link: https://patch.msgid.link/20250918-can-fix-mtu-v1-2-0d1cada9393b@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/spi/hi311x.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index 96bef8f384c4..963ea8510dd9 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -799,6 +799,7 @@ static const struct net_device_ops hi3110_netdev_ops = {
.ndo_open = hi3110_open,
.ndo_stop = hi3110_stop,
.ndo_start_xmit = hi3110_hard_start_xmit,
+ .ndo_change_mtu = can_change_mtu,
};
static const struct ethtool_ops hi3110_ethtool_ops = {
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 05/10] can: sun4i_can: populate ndo_change_mtu() to prevent buffer overflow
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (3 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 04/10] can: hi311x: " Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 06/10] can: mcba_usb: " Marc Kleine-Budde
` (5 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Vincent Mailhol,
Marc Kleine-Budde
From: Vincent Mailhol <mailhol@kernel.org>
Sending an PF_PACKET allows to bypass the CAN framework logic and to
directly reach the xmit() function of a CAN driver. The only check
which is performed by the PF_PACKET framework is to make sure that
skb->len fits the interface's MTU.
Unfortunately, because the sun4i_can driver does not populate its
net_device_ops->ndo_change_mtu(), it is possible for an attacker to
configure an invalid MTU by doing, for example:
$ ip link set can0 mtu 9999
After doing so, the attacker could open a PF_PACKET socket using the
ETH_P_CANXL protocol:
socket(PF_PACKET, SOCK_RAW, htons(ETH_P_CANXL))
to inject a malicious CAN XL frames. For example:
struct canxl_frame frame = {
.flags = 0xff,
.len = 2048,
};
The CAN drivers' xmit() function are calling can_dev_dropped_skb() to
check that the skb is valid, unfortunately under above conditions, the
malicious packet is able to go through can_dev_dropped_skb() checks:
1. the skb->protocol is set to ETH_P_CANXL which is valid (the
function does not check the actual device capabilities).
2. the length is a valid CAN XL length.
And so, sun4ican_start_xmit() receives a CAN XL frame which it is not
able to correctly handle and will thus misinterpret it as a CAN frame.
This can result in a buffer overflow. The driver will consume cf->len
as-is with no further checks on this line:
dlc = cf->len;
Here, cf->len corresponds to the flags field of the CAN XL frame. In
our previous example, we set canxl_frame->flags to 0xff. Because the
maximum expected length is 8, a buffer overflow of 247 bytes occurs a
couple line below when doing:
for (i = 0; i < dlc; i++)
writel(cf->data[i], priv->base + (dreg + i * 4));
Populate net_device_ops->ndo_change_mtu() to ensure that the
interface's MTU can not be set to anything bigger than CAN_MTU. By
fixing the root cause, this prevents the buffer overflow.
Fixes: 0738eff14d81 ("can: Allwinner A10/A20 CAN Controller support - Kernel module")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
Link: https://patch.msgid.link/20250918-can-fix-mtu-v1-3-0d1cada9393b@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/sun4i_can.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
index 6fcb301ef611..53bfd873de9b 100644
--- a/drivers/net/can/sun4i_can.c
+++ b/drivers/net/can/sun4i_can.c
@@ -768,6 +768,7 @@ static const struct net_device_ops sun4ican_netdev_ops = {
.ndo_open = sun4ican_open,
.ndo_stop = sun4ican_close,
.ndo_start_xmit = sun4ican_start_xmit,
+ .ndo_change_mtu = can_change_mtu,
};
static const struct ethtool_ops sun4ican_ethtool_ops = {
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 06/10] can: mcba_usb: populate ndo_change_mtu() to prevent buffer overflow
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (4 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 05/10] can: sun4i_can: " Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 07/10] can: peak_usb: fix shift-out-of-bounds issue Marc Kleine-Budde
` (4 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Vincent Mailhol,
Marc Kleine-Budde
From: Vincent Mailhol <mailhol@kernel.org>
Sending an PF_PACKET allows to bypass the CAN framework logic and to
directly reach the xmit() function of a CAN driver. The only check
which is performed by the PF_PACKET framework is to make sure that
skb->len fits the interface's MTU.
Unfortunately, because the mcba_usb driver does not populate its
net_device_ops->ndo_change_mtu(), it is possible for an attacker to
configure an invalid MTU by doing, for example:
$ ip link set can0 mtu 9999
After doing so, the attacker could open a PF_PACKET socket using the
ETH_P_CANXL protocol:
socket(PF_PACKET, SOCK_RAW, htons(ETH_P_CANXL))
to inject a malicious CAN XL frames. For example:
struct canxl_frame frame = {
.flags = 0xff,
.len = 2048,
};
The CAN drivers' xmit() function are calling can_dev_dropped_skb() to
check that the skb is valid, unfortunately under above conditions, the
malicious packet is able to go through can_dev_dropped_skb() checks:
1. the skb->protocol is set to ETH_P_CANXL which is valid (the
function does not check the actual device capabilities).
2. the length is a valid CAN XL length.
And so, mcba_usb_start_xmit() receives a CAN XL frame which it is not
able to correctly handle and will thus misinterpret it as a CAN frame.
This can result in a buffer overflow. The driver will consume cf->len
as-is with no further checks on these lines:
usb_msg.dlc = cf->len;
memcpy(usb_msg.data, cf->data, usb_msg.dlc);
Here, cf->len corresponds to the flags field of the CAN XL frame. In
our previous example, we set canxl_frame->flags to 0xff. Because the
maximum expected length is 8, a buffer overflow of 247 bytes occurs!
Populate net_device_ops->ndo_change_mtu() to ensure that the
interface's MTU can not be set to anything bigger than CAN_MTU. By
fixing the root cause, this prevents the buffer overflow.
Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
Link: https://patch.msgid.link/20250918-can-fix-mtu-v1-4-0d1cada9393b@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/mcba_usb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index 41c0a1c399bf..1f9b915094e6 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -761,6 +761,7 @@ static const struct net_device_ops mcba_netdev_ops = {
.ndo_open = mcba_usb_open,
.ndo_stop = mcba_usb_close,
.ndo_start_xmit = mcba_usb_start_xmit,
+ .ndo_change_mtu = can_change_mtu,
};
static const struct ethtool_ops mcba_ethtool_ops = {
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 07/10] can: peak_usb: fix shift-out-of-bounds issue
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (5 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 06/10] can: mcba_usb: " Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-22 10:07 ` [PATCH net 08/10] can: esd_usb: Fix not detecting version reply in probe routine Marc Kleine-Budde
` (3 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Stéphane Grosjean,
Marc Kleine-Budde
From: Stéphane Grosjean <stephane.grosjean@hms-networks.com>
Explicitly uses a 64-bit constant when the number of bits used for its
shifting is 32 (which is the case for PC CAN FD interfaces supported by
this driver).
Signed-off-by: Stéphane Grosjean <stephane.grosjean@hms-networks.com>
Link: https://patch.msgid.link/20250918132413.30071-1-stephane.grosjean@free.fr
Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Closes: https://lore.kernel.org/20250917-aboriginal-refined-honeybee-82b1aa-mkl@pengutronix.de
Fixes: bb4785551f64 ("can: usb: PEAK-System Technik USB adapters driver core")
[mkl: update subject, apply manually]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 117637b9b995..dd5caa1c302b 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -111,7 +111,7 @@ void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
u32 delta_ts = time_ref->ts_dev_2 - time_ref->ts_dev_1;
if (time_ref->ts_dev_2 < time_ref->ts_dev_1)
- delta_ts &= (1 << time_ref->adapter->ts_used_bits) - 1;
+ delta_ts &= (1ULL << time_ref->adapter->ts_used_bits) - 1;
time_ref->ts_total += delta_ts;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net 08/10] can: esd_usb: Fix not detecting version reply in probe routine
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (6 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 07/10] can: peak_usb: fix shift-out-of-bounds issue Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-23 0:16 ` Jakub Kicinski
2025-09-22 10:07 ` [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects Marc Kleine-Budde
` (2 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Stefan Mätje,
Marc Kleine-Budde
From: Stefan Mätje <stefan.maetje@esd.eu>
This patch fixes some problems in the esd_usb_probe() routine that render
the CAN interface unusable.
The probe routine sends a version request message to the USB device to
receive a version reply with the number of CAN ports and the hard-
& firmware versions. Then for each CAN port a CAN netdev is registered.
The previous code assumed that the version reply would be received
immediately. But if the driver was reloaded without power cycling the
USB device (i. e. on a reboot) there could already be other incoming
messages in the USB buffers. These would be in front of the version
reply and need to be skipped.
In the previous code these problems were present:
- Only one usb_bulk_msg() read was done into a buffer of
sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
which could lead to an overflow error from the USB stack.
- The first bytes of the received data were taken without checking for
the message type. This could lead to zero detected CAN interfaces.
To mitigate these problems:
- Move the code to send the version request message into a standalone
function esd_usb_req_version().
- Add a function esd_usb_recv_version() using a transfer buffer
with ESD_USB_RX_BUFFER_SIZE. It reads and skips incoming "esd_usb_msg"
messages until a version reply message is found. This is evaluated to
return the count of CAN ports and version information.
- The data drain loop is limited by a maximum # of bytes to read from
the device based on its internal buffer sizes and a timeout
ESD_USB_DRAIN_TIMEOUT_MS.
This version of the patch incorporates changes recommended on the
linux-can list for a very first version [1].
[1] https://lore.kernel.org/linux-can/20250203145810.1286331-1-stefan.maetje@esd.eu
Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
Link: https://patch.msgid.link/20250821143422.3567029-2-stefan.maetje@esd.eu
[mkl: minor change patch description to imperative language]
[mkl: squash error format changes from patch 4]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/esd_usb.c | 149 +++++++++++++++++++++++++---------
1 file changed, 112 insertions(+), 37 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 27a3818885c2..ed1d6ba779dc 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -3,7 +3,7 @@
* CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro
*
* Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <socketcan@esd.eu>
- * Copyright (C) 2022-2024 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
+ * Copyright (C) 2022-2025 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
*/
#include <linux/can.h>
@@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
#define ESD_USB_CMD_TS 5 /* also used for TS_REPLY */
#define ESD_USB_CMD_IDADD 6 /* also used for IDADD_REPLY */
+/* esd version message name size */
+#define ESD_USB_FW_NAME_SZ 16
+
/* esd CAN message flags - dlc field */
#define ESD_USB_RTR BIT(4)
#define ESD_USB_NO_BRS BIT(4)
@@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
#define ESD_USB_RX_BUFFER_SIZE 1024
#define ESD_USB_MAX_RX_URBS 4
#define ESD_USB_MAX_TX_URBS 16 /* must be power of 2 */
+#define ESD_USB_DRAIN_TIMEOUT_MS 100
/* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
#define ESD_USB_3_BAUDRATE_MODE_DISABLE 0 /* remove from bus */
@@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
u8 nets;
u8 features;
__le32 version;
- u8 name[16];
+ u8 name[ESD_USB_FW_NAME_SZ];
__le32 rsvd;
__le32 ts;
};
@@ -625,17 +629,106 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
1000);
}
-static int esd_usb_wait_msg(struct esd_usb *dev,
- union esd_usb_msg *msg)
+static int esd_usb_req_version(struct esd_usb *dev)
{
- int actual_length;
+ union esd_usb_msg *msg;
+ int err;
- return usb_bulk_msg(dev->udev,
- usb_rcvbulkpipe(dev->udev, 1),
- msg,
- sizeof(*msg),
- &actual_length,
- 1000);
+ msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ msg->hdr.cmd = ESD_USB_CMD_VERSION;
+ msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
+ msg->version.rsvd = 0;
+ msg->version.flags = 0;
+ msg->version.drv_version = 0;
+
+ err = esd_usb_send_msg(dev, msg);
+ kfree(msg);
+ return err;
+}
+
+static int esd_usb_recv_version(struct esd_usb *dev)
+{
+ /* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
+ const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
+ unsigned long end_jiffies;
+ void *rx_buf;
+ int cnt_other = 0;
+ int cnt_ts = 0;
+ int cnt_vs = 0;
+ int len_sum = 0;
+ int attempt = 0;
+ int err;
+
+ rx_buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
+ if (!rx_buf)
+ return -ENOMEM;
+
+ end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
+ do {
+ int actual_length;
+ int pos;
+
+ err = usb_bulk_msg(dev->udev,
+ usb_rcvbulkpipe(dev->udev, 1),
+ rx_buf,
+ ESD_USB_RX_BUFFER_SIZE,
+ &actual_length,
+ ESD_USB_DRAIN_TIMEOUT_MS);
+ dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
+ ++attempt;
+ if (err)
+ goto bail;
+ if (actual_length == 0)
+ continue;
+
+ err = -ENOENT;
+ len_sum += actual_length;
+ pos = 0;
+ while (pos < actual_length - sizeof(struct esd_usb_header_msg)) {
+ union esd_usb_msg *p_msg;
+
+ p_msg = (union esd_usb_msg *)(rx_buf + pos);
+
+ pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */
+ if (pos > actual_length) {
+ dev_err(&dev->udev->dev, "format error\n");
+ err = -EPROTO;
+ goto bail;
+ }
+
+ switch (p_msg->hdr.cmd) {
+ case ESD_USB_CMD_VERSION:
+ ++cnt_vs;
+ dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
+ dev->version = le32_to_cpu(p_msg->version_reply.version);
+ err = 0;
+ dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.*s\n",
+ le32_to_cpu(p_msg->version_reply.ts),
+ le32_to_cpu(p_msg->version_reply.version),
+ p_msg->version_reply.nets,
+ p_msg->version_reply.features,
+ ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
+ break;
+ case ESD_USB_CMD_TS:
+ ++cnt_ts;
+ dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
+ le32_to_cpu(p_msg->rx.ts));
+ break;
+ default:
+ ++cnt_other;
+ dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
+ break;
+ }
+ }
+ } while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
+bail:
+ dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
+ err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
+ kfree(rx_buf);
+ return err;
}
static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
@@ -1273,13 +1366,12 @@ static int esd_usb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct esd_usb *dev;
- union esd_usb_msg *msg;
int i, err;
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev) {
err = -ENOMEM;
- goto done;
+ goto bail;
}
dev->udev = interface_to_usbdev(intf);
@@ -1288,34 +1380,19 @@ static int esd_usb_probe(struct usb_interface *intf,
usb_set_intfdata(intf, dev);
- msg = kmalloc(sizeof(*msg), GFP_KERNEL);
- if (!msg) {
- err = -ENOMEM;
- goto free_msg;
- }
-
/* query number of CAN interfaces (nets) */
- msg->hdr.cmd = ESD_USB_CMD_VERSION;
- msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
- msg->version.rsvd = 0;
- msg->version.flags = 0;
- msg->version.drv_version = 0;
-
- err = esd_usb_send_msg(dev, msg);
+ err = esd_usb_req_version(dev);
if (err < 0) {
- dev_err(&intf->dev, "sending version message failed\n");
- goto free_msg;
+ dev_err(&intf->dev, "sending version message failed: %pe\n", ERR_PTR(err));
+ goto bail;
}
- err = esd_usb_wait_msg(dev, msg);
+ err = esd_usb_recv_version(dev);
if (err < 0) {
- dev_err(&intf->dev, "no version message answer\n");
- goto free_msg;
+ dev_err(&intf->dev, "no version message answer: %pe\n", ERR_PTR(err));
+ goto bail;
}
- dev->net_count = (int)msg->version_reply.nets;
- dev->version = le32_to_cpu(msg->version_reply.version);
-
if (device_create_file(&intf->dev, &dev_attr_firmware))
dev_err(&intf->dev,
"Couldn't create device file for firmware\n");
@@ -1332,11 +1409,9 @@ static int esd_usb_probe(struct usb_interface *intf,
for (i = 0; i < dev->net_count; i++)
esd_usb_probe_one_net(intf, i);
-free_msg:
- kfree(msg);
+bail:
if (err)
kfree(dev);
-done:
return err;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH net 08/10] can: esd_usb: Fix not detecting version reply in probe routine
2025-09-22 10:07 ` [PATCH net 08/10] can: esd_usb: Fix not detecting version reply in probe routine Marc Kleine-Budde
@ 2025-09-23 0:16 ` Jakub Kicinski
2025-09-24 10:05 ` Stefan Mätje
0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-09-23 0:16 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, linux-can, kernel, Stefan Mätje
On Mon, 22 Sep 2025 12:07:38 +0200 Marc Kleine-Budde wrote:
> + do {
> + int actual_length;
> + int pos;
> +
> + err = usb_bulk_msg(dev->udev,
> + usb_rcvbulkpipe(dev->udev, 1),
> + rx_buf,
> + ESD_USB_RX_BUFFER_SIZE,
> + &actual_length,
> + ESD_USB_DRAIN_TIMEOUT_MS);
> + dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
> + ++attempt;
> + if (err)
> + goto bail;
> + if (actual_length == 0)
> + continue;
continue in do-while loops doesn't check the condition.
This looks like a potential infinite loop?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net 08/10] can: esd_usb: Fix not detecting version reply in probe routine
2025-09-23 0:16 ` Jakub Kicinski
@ 2025-09-24 10:05 ` Stefan Mätje
0 siblings, 0 replies; 18+ messages in thread
From: Stefan Mätje @ 2025-09-24 10:05 UTC (permalink / raw)
To: mkl@pengutronix.de, kuba@kernel.org
Cc: linux-can@vger.kernel.org, kernel@pengutronix.de,
netdev@vger.kernel.org, davem@davemloft.net
Am Montag, dem 22.09.2025 um 17:16 -0700 schrieb Jakub Kicinski:
> On Mon, 22 Sep 2025 12:07:38 +0200 Marc Kleine-Budde wrote:
> > + do {
> > + int actual_length;
> > + int pos;
> > +
> > + err = usb_bulk_msg(dev->udev,
> > + usb_rcvbulkpipe(dev->udev, 1),
> > + rx_buf,
> > + ESD_USB_RX_BUFFER_SIZE,
> > + &actual_length,
> > + ESD_USB_DRAIN_TIMEOUT_MS);
> > + dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
> > + ++attempt;
> > + if (err)
> > + goto bail;
> > + if (actual_length == 0)
> > + continue;
>
> continue in do-while loops doesn't check the condition.
> This looks like a potential infinite loop?
I don't think so. A continue statement in a do, while or for loop
always jumps to the end of the loop body.
See a citation of the C standard there:
https://stackoverflow.com/a/64120354
Therefore there is no potential for an infinite loop due to the continue
statement.
Refer to following code and its output:
----------------------------------------------
#include <stdio.h>
#define LIMIT 4
int main(void)
{
int cnt = 0;
do {
printf("Top: %d\n", cnt);
++cnt;
if (cnt > 2) continue;
printf("Bottom: %d\n", cnt);
} while (printf("Condition: %d\n\n", cnt), cnt < LIMIT);
return 0;
}
----------------------------------------------
Output:
----------------------------------------------
stefanm@pc-stefanm64:~/Tmp$ ./do_continue
Top: 0
Bottom: 1
Condition: 1
Top: 1
Bottom: 2
Condition: 2
Top: 2
Condition: 3
Top: 3
Condition: 4
----------------------------------------------
Sorry being late with this.
Best regards,
Stefan
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (7 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 08/10] can: esd_usb: Fix not detecting version reply in probe routine Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-23 0:17 ` Jakub Kicinski
2025-09-22 10:07 ` [PATCH net 10/10] can: esd_usb: Add watermark handling for TX jobs Marc Kleine-Budde
2025-09-23 0:19 ` [PATCH net 0/10] pull-request: can 2025-09-22 Jakub Kicinski
10 siblings, 1 reply; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Stefan Mätje,
Marc Kleine-Budde
From: Stefan Mätje <stefan.maetje@esd.eu>
For each TX CAN frame submitted to the USB device the driver saves the
echo skb index in struct esd_tx_urb_context context objects. If the
driver runs out of free context objects CAN transmission stops.
Fixe some spots where such context objects are not freed correctly.
In esd_usb_tx_done_msg() move the check for netif_device_present()
after the identification and release of TX context and the release of
the echo skb. This is allowed even if netif_device_present() would
return false because the mentioned operations don't touch the device
itself but only free local acquired resources. This keeps the context
handling with the acknowledged TX jobs in sync.
In esd_usb_start_xmit() performed a check to see whether a context
object could be allocated. Add a netif_stop_queue() there before the
function is aborted. This makes sure the network queue is stopped and
avoids getting tons of log messages in a situation without free TX
objects. The adjacent log message now also prints the active jobs
counter making a cross check between active jobs and "no free context"
condition possible.
In esd_usb_start_xmit() the error handling of usb_submit_urb() missed to
free the context object together with the echo skb and decreasing the
job count.
Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
Link: https://patch.msgid.link/20250821143422.3567029-3-stefan.maetje@esd.eu
[mkl: minor change patch description to imperative language]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/esd_usb.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index ed1d6ba779dc..efc96619ee9a 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -460,9 +460,6 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
struct net_device *netdev = priv->netdev;
struct esd_tx_urb_context *context;
- if (!netif_device_present(netdev))
- return;
-
context = &priv->tx_contexts[msg->txdone.hnd & (ESD_USB_MAX_TX_URBS - 1)];
if (!msg->txdone.status) {
@@ -478,6 +475,9 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
context->echo_index = ESD_USB_MAX_TX_URBS;
atomic_dec(&priv->active_tx_jobs);
+ if (!netif_device_present(netdev))
+ return;
+
netif_wake_queue(netdev);
}
@@ -975,9 +975,11 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
}
}
- /* This may never happen */
+ /* This should never happen */
if (!context) {
- netdev_warn(netdev, "couldn't find free context\n");
+ netdev_warn(netdev, "No free context. Jobs: %d\n",
+ atomic_read(&priv->active_tx_jobs));
+ netif_stop_queue(netdev);
ret = NETDEV_TX_BUSY;
goto releasebuf;
}
@@ -1008,6 +1010,8 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
if (err) {
can_free_echo_skb(netdev, context->echo_index, NULL);
+ /* Release used context on error */
+ context->echo_index = ESD_USB_MAX_TX_URBS;
atomic_dec(&priv->active_tx_jobs);
usb_unanchor_urb(urb);
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects
2025-09-22 10:07 ` [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects Marc Kleine-Budde
@ 2025-09-23 0:17 ` Jakub Kicinski
2025-09-24 13:26 ` Stefan Mätje
0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-09-23 0:17 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, linux-can, kernel, Stefan Mätje
On Mon, 22 Sep 2025 12:07:39 +0200 Marc Kleine-Budde wrote:
> - netdev_warn(netdev, "couldn't find free context\n");
> + netdev_warn(netdev, "No free context. Jobs: %d\n",
> + atomic_read(&priv->active_tx_jobs));
This should really be rate limited or _once while we touch it.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects
2025-09-23 0:17 ` Jakub Kicinski
@ 2025-09-24 13:26 ` Stefan Mätje
2025-09-24 14:12 ` Marc Kleine-Budde
0 siblings, 1 reply; 18+ messages in thread
From: Stefan Mätje @ 2025-09-24 13:26 UTC (permalink / raw)
To: mkl@pengutronix.de, kuba@kernel.org
Cc: linux-can@vger.kernel.org, kernel@pengutronix.de,
netdev@vger.kernel.org, davem@davemloft.net
Am Montag, dem 22.09.2025 um 17:17 -0700 schrieb Jakub Kicinski:
> On Mon, 22 Sep 2025 12:07:39 +0200 Marc Kleine-Budde wrote:
> > - netdev_warn(netdev, "couldn't find free context\n");
> > + netdev_warn(netdev, "No free context. Jobs: %d\n",
> > + atomic_read(&priv->active_tx_jobs));
>
> This should really be rate limited or _once while we touch it.
>
Changing this to a rate limited version would be fine with me.
@Marc:
How to proceed further? Should I send a V3 of the original patch
set or should I split the patch set in two patch sets like you did?
The code would look then like:
if (!context) {
if (net_ratelimit())
netdev_warn(netdev, "No free context. Jobs: %d\n",
atomic_read(&priv->active_tx_jobs));
netif_stop_queue(netdev);
ret = NETDEV_TX_BUSY;
goto releasebuf;
}
Best regards,
Stefan
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects
2025-09-24 13:26 ` Stefan Mätje
@ 2025-09-24 14:12 ` Marc Kleine-Budde
0 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-24 14:12 UTC (permalink / raw)
To: Stefan Mätje
Cc: kuba@kernel.org, linux-can@vger.kernel.org, kernel@pengutronix.de,
netdev@vger.kernel.org, davem@davemloft.net
[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]
On 24.09.2025 13:26:39, Stefan Mätje wrote:
> Am Montag, dem 22.09.2025 um 17:17 -0700 schrieb Jakub Kicinski:
> > On Mon, 22 Sep 2025 12:07:39 +0200 Marc Kleine-Budde wrote:
> > > - netdev_warn(netdev, "couldn't find free context\n");
> > > + netdev_warn(netdev, "No free context. Jobs: %d\n",
> > > + atomic_read(&priv->active_tx_jobs));
> >
> > This should really be rate limited or _once while we touch it.
> >
>
> Changing this to a rate limited version would be fine with me.
>
> @Marc:
> How to proceed further? Should I send a V3 of the original patch
> set or should I split the patch set in two patch sets like you did?
Please split the patches as I did, or even better use the patches from
the pull request, as I've moved the some of the error print changes.
> The code would look then like:
>
> if (!context) {
> if (net_ratelimit())
> netdev_warn(netdev, "No free context. Jobs: %d\n",
> atomic_read(&priv->active_tx_jobs));
> netif_stop_queue(netdev);
> ret = NETDEV_TX_BUSY;
> goto releasebuf;
> }
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net 10/10] can: esd_usb: Add watermark handling for TX jobs
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (8 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 09/10] can: esd_usb: Fix handling of TX context objects Marc Kleine-Budde
@ 2025-09-22 10:07 ` Marc Kleine-Budde
2025-09-23 0:19 ` [PATCH net 0/10] pull-request: can 2025-09-22 Jakub Kicinski
10 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-22 10:07 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Stefan Mätje,
Marc Kleine-Budde
From: Stefan Mätje <stefan.maetje@esd.eu>
The driver tried to keep as much CAN frames as possible submitted to the
USB device (ESD_USB_MAX_TX_URBS). This has led to occasional "No free
context" error messages in high load situations like with
"cangen -g 0 -p 10 canX".
Now call netif_stop_queue() already if the number of active jobs
reaches ESD_USB_TX_URBS_HI_WM which is < ESD_USB_MAX_TX_URBS. The
netif_start_queue() is called in esd_usb_tx_done_msg() only if the
number of active jobs is <= ESD_USB_TX_URBS_LO_WM.
This change eliminates the occasional error messages and significantly
reduces the number of calls to netif_start_queue() and
netif_stop_queue().
The watermark limits have been chosen with the CAN-USB/Micro in mind to
not to compromise its TX throughput. This device is running on USB 1.1
only with its 1ms USB polling cycle where a ESD_USB_TX_URBS_LO_WM
value below 9 decreases the TX throughput.
Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
Link: https://patch.msgid.link/20250821143422.3567029-4-stefan.maetje@esd.eu
[mkl: minor change patch description to imperative language]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/esd_usb.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index efc96619ee9a..40ffac0e73e9 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -98,6 +98,8 @@ MODULE_LICENSE("GPL v2");
#define ESD_USB_RX_BUFFER_SIZE 1024
#define ESD_USB_MAX_RX_URBS 4
#define ESD_USB_MAX_TX_URBS 16 /* must be power of 2 */
+#define ESD_USB_TX_URBS_HI_WM ((15 * ESD_USB_MAX_TX_URBS) / 16)
+#define ESD_USB_TX_URBS_LO_WM ((9 * ESD_USB_MAX_TX_URBS) / 16)
#define ESD_USB_DRAIN_TIMEOUT_MS 100
/* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
@@ -478,7 +480,8 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
if (!netif_device_present(netdev))
return;
- netif_wake_queue(netdev);
+ if (atomic_read(&priv->active_tx_jobs) <= ESD_USB_TX_URBS_LO_WM)
+ netif_wake_queue(netdev);
}
static void esd_usb_read_bulk_callback(struct urb *urb)
@@ -987,7 +990,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
context->priv = priv;
context->echo_index = i;
- /* hnd must not be 0 - MSB is stripped in txdone handling */
+ /* hnd must not be 0 - MSB is stripped in TX done handling */
msg->tx.hnd = BIT(31) | i; /* returned in TX done message */
usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
@@ -1002,8 +1005,8 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
atomic_inc(&priv->active_tx_jobs);
- /* Slow down tx path */
- if (atomic_read(&priv->active_tx_jobs) >= ESD_USB_MAX_TX_URBS)
+ /* Slow down TX path */
+ if (atomic_read(&priv->active_tx_jobs) >= ESD_USB_TX_URBS_HI_WM)
netif_stop_queue(netdev);
err = usb_submit_urb(urb, GFP_ATOMIC);
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH net 0/10] pull-request: can 2025-09-22
2025-09-22 10:07 [PATCH net 0/10] pull-request: can 2025-09-22 Marc Kleine-Budde
` (9 preceding siblings ...)
2025-09-22 10:07 ` [PATCH net 10/10] can: esd_usb: Add watermark handling for TX jobs Marc Kleine-Budde
@ 2025-09-23 0:19 ` Jakub Kicinski
2025-09-23 7:56 ` Marc Kleine-Budde
10 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-09-23 0:19 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, linux-can, kernel
On Mon, 22 Sep 2025 12:07:30 +0200 Marc Kleine-Budde wrote:
> Stefan Mätje (3):
> can: esd_usb: Fix not detecting version reply in probe routine
> can: esd_usb: Fix handling of TX context objects
> can: esd_usb: Add watermark handling for TX jobs
I have mixed feelings about these. Could you resend the PR for just
the first 7 ASAP? Feel free to also send another with the (fixed) esd
patches but whether those reach 6.17 final or not will depend on
whether we can review them in time..
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net 0/10] pull-request: can 2025-09-22
2025-09-23 0:19 ` [PATCH net 0/10] pull-request: can 2025-09-22 Jakub Kicinski
@ 2025-09-23 7:56 ` Marc Kleine-Budde
0 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2025-09-23 7:56 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem, linux-can, kernel
[-- Attachment #1: Type: text/plain, Size: 989 bytes --]
On 22.09.2025 17:19:26, Jakub Kicinski wrote:
> On Mon, 22 Sep 2025 12:07:30 +0200 Marc Kleine-Budde wrote:
> > Stefan Mätje (3):
> > can: esd_usb: Fix not detecting version reply in probe routine
> > can: esd_usb: Fix handling of TX context objects
> > can: esd_usb: Add watermark handling for TX jobs
>
> I have mixed feelings about these. Could you resend the PR for just
> the first 7 ASAP? Feel free to also send another with the (fixed) esd
> patches but whether those reach 6.17 final or not will depend on
> whether we can review them in time..
Makes sense, here's the updated one:
| https://lore.kernel.org/all/20250923073427.493034-1-mkl@pengutronix.de/
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread