* [PATCH net v6 1/6] can: dev: prevent bus-off recovery when the interface is not running
2026-10-17 14:50 [PATCH net v6 0/6] can: fix bus-off restart and device close races Cunhao Lu via B4 Relay
@ 2026-10-17 14:50 ` Cunhao Lu via B4 Relay
2026-10-19 2:57 ` netdev-bot+sashiko
2026-10-17 14:50 ` [PATCH net v6 2/6] can: dev: add close_candev_cancel_restart_sync() Cunhao Lu via B4 Relay
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-10-17 14:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Heiko Stuebner,
David S. Miller, Wolfgang Grandegger, Oliver Hartkopp, kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Jonathan Corbet, Shuah Khan, Randy Dunlap
Cc: linux-can, linux-kernel, linux-arm-kernel, linux-rockchip, netdev,
linux-doc, stable, Cunhao Lu
From: Cunhao Lu <1579567540@qq.com>
The networking core clears __LINK_STATE_START before calling ndo_stop(),
but CAN bus-off recovery neither checks this state before queuing work
nor before calling the driver's restart callback. Recovery can therefore
be queued or started after the interface has begun closing, potentially
re-enabling hardware while the driver is tearing it down.
Check netif_running() in can_restart() before flushing echo skbs, sending
the restart notification or enabling the carrier. Keep the existing
unsupported-operation check first. In can_bus_off(), only schedule
recovery for a running interface and make the log message reflect that
decision. Keep reporting bus-off and disabling the carrier.
These checks do not wait for recovery that has already passed the check,
nor serialize the check with work submission. Drivers still need to
cancel restart work before tearing down resources and quiesce its
producers before their final cleanup.
Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
Changes in v6:
- Update the can_bus_off() comment and document the running-interface
requirement for automatic recovery.
---
Documentation/networking/can.rst | 6 ++++--
drivers/net/can/dev/dev.c | 11 ++++++++---
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
index 536ff411da1d1..861397e4ce906 100644
--- a/Documentation/networking/can.rst
+++ b/Documentation/networking/can.rst
@@ -1298,7 +1298,8 @@ More info to the above output:
Automatic restart delay time. If set to a non-zero value, a
restart of the CAN controller will be triggered automatically
in case of a bus-off condition after the specified delay time
- in milliseconds. By default it's off.
+ in milliseconds, provided the interface is running. By default
+ it's off.
"bitrate 125000 sample-point 0.875"
Shows the real bit-rate in bits/sec and the sample-point in the
@@ -1372,7 +1373,8 @@ before you can start it to avoid error-prone default settings::
A device may enter the "bus-off" state if too many errors occurred on
the CAN bus. Then no more messages are received or sent. An automatic
bus-off recovery can be enabled by setting the "restart-ms" to a
-non-zero value, e.g.::
+non-zero value. Recovery is only scheduled and started while the
+interface is running, e.g.::
$ ip link set canX type can restart-ms 100
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 769745e22a3c7..d5cb22db2a346 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -198,6 +198,9 @@ static int can_restart(struct net_device *dev)
if (!priv->do_set_mode)
return -EOPNOTSUPP;
+ if (!netif_running(dev))
+ return -ENETDOWN;
+
if (netif_carrier_ok(dev))
netdev_err(dev, "Attempt to restart for bus-off recovery, but carrier is OK?\n");
@@ -259,13 +262,15 @@ int can_restart_now(struct net_device *dev)
*
* This functions should be called when the device goes bus-off to
* tell the netif layer that no more packets can be sent or received.
- * If enabled, a timer is started to trigger bus-off recovery.
+ * If automatic recovery is enabled and the interface is running,
+ * delayed work is scheduled to trigger bus-off recovery.
*/
void can_bus_off(struct net_device *dev)
{
struct can_priv *priv = netdev_priv(dev);
+ const bool restart = priv->restart_ms && netif_running(dev);
- if (priv->restart_ms)
+ if (restart)
netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
priv->restart_ms);
else
@@ -273,7 +278,7 @@ void can_bus_off(struct net_device *dev)
netif_carrier_off(dev);
- if (priv->restart_ms)
+ if (restart)
schedule_delayed_work(&priv->restart_work,
msecs_to_jiffies(priv->restart_ms));
}
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH net v6 1/6] can: dev: prevent bus-off recovery when the interface is not running
2026-10-17 14:50 ` [PATCH net v6 1/6] can: dev: prevent bus-off recovery when the interface is not running Cunhao Lu via B4 Relay
@ 2026-10-19 2:57 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-10-19 2:57 UTC (permalink / raw)
To: 1579567540
Cc: mkl, mailhol, heiko, davem, wg, socketcan, kernel, edumazet, kuba,
pabeni, horms, corbet, skhan, rdunlap, linux-can, linux-kernel,
linux-arm-kernel, linux-rockchip, netdev, linux-doc, stable
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The patch is titled 'prevent bus-off recovery when the
interface is not running', but the netif_running() checks it adds are…
- [Low] The patch rewords the bus-off recovery paragraph in
Documentation/networking/can.rst so that the 'e.g.::' now follows the…
- [Low] The new user-facing text in Documentation/networking/can.rst
states that recovery happens only while the interface is running, with…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch only
narrows it.
--
can: dev: prevent bus-off recovery when the interface is not running
This patch adds a netif_running() check to can_restart() before the echo
skbs are flushed and the driver restart callback is called. It also changes
can_bus_off() so the restart work is only scheduled when restart_ms is set
and the interface is running. Documentation/networking/can.rst is updated to
describe the running-interface requirement.
> diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
> index 536ff411da1d1..861397e4ce906 100644
> --- a/Documentation/networking/can.rst
> +++ b/Documentation/networking/can.rst
> @@ -1298,7 +1298,8 @@ More info to the above output:
> Automatic restart delay time. If set to a non-zero value, a
> restart of the CAN controller will be triggered automatically
> in case of a bus-off condition after the specified delay time
> - in milliseconds. By default it's off.
> + in milliseconds, provided the interface is running. By default
> + it's off.
[Severity: Low]
Is this guarantee accurate as written? The bus-off paragraph below makes the
same claim with no qualification ("Recovery is only scheduled and started
while the interface is running").
Each new check is a single lockless netif_running() snapshot. Once the check
in can_restart() passes, the function still goes on to do:
can_flush_echo_skb(dev);
...
netif_carrier_on(dev);
err = priv->do_set_mode(dev, CAN_MODE_START);
Nothing orders this against __dev_close_many() clearing __LINK_STATE_START
and calling ndo_stop(). In the same way, can_bus_off() saves netif_running()
into restart and only queues the work later.
The only wait point is cancel_delayed_work_sync() in close_candev(). That
runs inside ndo_stop, after the interface has already stopped running.
The commit message says as much ("These checks do not wait for recovery
that has already passed the check, nor serialize the check with work
submission"), but the user documentation doesn't. Would softer wording, for
example that recovery is not scheduled and is skipped once the interface is
down, describe the behaviour more accurately?
> @@ -1372,7 +1373,8 @@ before you can start it to avoid error-prone default settings::
> A device may enter the "bus-off" state if too many errors occurred on
> the CAN bus. Then no more messages are received or sent. An automatic
> bus-off recovery can be enabled by setting the "restart-ms" to a
> -non-zero value, e.g.::
> +non-zero value. Recovery is only scheduled and started while the
> +interface is running, e.g.::
>
> $ ip link set canX type can restart-ms 100
[Severity: Low]
Is the "e.g.::" now attached to the wrong sentence? Before this patch, the
example showed how to enable recovery by setting restart-ms. Now it follows
the "while the interface is running" sentence, so it reads as a command to
run on a running interface.
can_changelink() rejects that case:
drivers/net/can/dev/netlink.c:can_changelink() {
...
/* Do not allow changing restart delay while running */
if (dev->flags & IFF_UP)
return -EBUSY;
...
}
Could the "e.g.::" stay on the enabling sentence, with the running-interface
note moved after the example? It might also be worth saying that restart-ms
must be set while the interface is down.
> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
> index 769745e22a3c7..d5cb22db2a346 100644
> --- a/drivers/net/can/dev/dev.c
> +++ b/drivers/net/can/dev/dev.c
> @@ -198,6 +198,9 @@ static int can_restart(struct net_device *dev)
> if (!priv->do_set_mode)
> return -EOPNOTSUPP;
>
> + if (!netif_running(dev))
> + return -ENETDOWN;
> +
[Severity: Medium]
Does this check actually close the window the subject line describes?
can_restart_work() runs on system_percpu_wq without RTNL. That means the
check can pass just before __dev_close_many() clears __LINK_STATE_START:
CPU0 (restart work) CPU1 (RTNL held)
can_restart_work()
can_restart()
netif_running() == true
__dev_close_many()
clear_bit(__LINK_STATE_START)
dev_deactivate_many()
flexcan_close()
free_irq()
flexcan_chip_stop_disable_on_error()
can_rx_offload_del()
flexcan_transceiver_disable()
netif_carrier_on()
flexcan_set_mode(CAN_MODE_START)
flexcan_chip_start()
flexcan_chip_interrupts_enable()
netif_wake_queue()
close_candev()
cancel_delayed_work_sync()
flexcan_close() calls close_candev() only after all of the hardware
teardown. So cancel_delayed_work_sync() just waits for the restart to
finish.
Could the controller then stay restarted and active on the bus after
ifdown, with its IRQ freed and the transceiver disabled?
can_bus_off() has a similar gap. restart comes from a snapshot, and
schedule_delayed_work() runs later. hi3110_stop() calls close_candev()
before free_irq(), so in drivers like hi3110 the work can be queued after
the only cancel. If the interface is reopened within restart_ms, that stale
work would run can_restart() on a live controller.
The commit message scopes this case out. A later patch in the series adds
close_candev_cancel_restart_sync(), but only rockchip_canfd uses it. At the
end of the series flexcan, hi3110 and the other in-tree drivers still have
this race.
Is real serialization needed here to stop recovery once the interface starts
closing? For example, the restart work could be cancelled at the start of
ndo_stop (in each driver, or through a core hook), or the work could use an
rtnl_trylock() and requeue on failure.
> @@ -259,13 +262,15 @@ int can_restart_now(struct net_device *dev)
[ ... ]
> void can_bus_off(struct net_device *dev)
> {
> struct can_priv *priv = netdev_priv(dev);
> + const bool restart = priv->restart_ms && netif_running(dev);
>
> - if (priv->restart_ms)
> + if (restart)
> netdev_info(dev, "bus-off, scheduling restart in %d ms\n",
> priv->restart_ms);
> else
> @@ -273,7 +278,7 @@ void can_bus_off(struct net_device *dev)
>
> netif_carrier_off(dev);
>
> - if (priv->restart_ms)
> + if (restart)
> schedule_delayed_work(&priv->restart_work,
> msecs_to_jiffies(priv->restart_ms));
> }
[Severity: High]
This is a pre-existing issue, but this patch only narrows it and doesn't
close it. Can restart_work still be queued after close_candev() and then
outlive the net_device?
Here is the teardown path with hi3110:
hi3110_can_remove()
unregister_candev()
__dev_close_many()
hi3110_stop()
close_candev()
cancel_delayed_work_sync() <- nothing pending yet
priv->force_quit = 1;
free_irq()
Meanwhile the threaded IST can be on its way into can_bus_off(). It calls
can_bus_off() whatever the value of restart_ms:
hi3110_can_ist()
...
if (new_state == CAN_STATE_BUS_OFF) {
if (skb)
netif_rx(skb);
can_bus_off(net);
Suppose the IST computed restart as true before __LINK_STATE_START was
cleared, and was then delayed until after close_candev(). In that case
schedule_delayed_work() queues the work after the only cancel.
hi3110_can_remove() then calls free_candev(), which is just:
void free_candev(struct net_device *dev)
{
free_netdev(dev);
}
When restart_ms expires, the delayed_work timer and can_restart_work() would
run on freed memory. That covers the container_of() to priv, the priv->dev
load, the new netif_running() read of dev->state, and the indirect call
through priv->do_set_mode. The new check in can_restart() can't help here,
because it reads the freed net_device itself.
Before this patch, any can_bus_off() after close_candev() queued the work,
so the window is now much smaller. The commit message leaves the remaining
case to drivers, and no later patch in the series changes hi3110 or the core
free path.
Would closing this need either serialization of the check and the queuing
against close_candev(), or a cancel/disable of restart_work in the
unregister or free path?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-rockchip-canfd-restart-close-v6-0-ed605feb4b6e%40qq.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net v6 2/6] can: dev: add close_candev_cancel_restart_sync()
2026-10-17 14:50 [PATCH net v6 0/6] can: fix bus-off restart and device close races Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 1/6] can: dev: prevent bus-off recovery when the interface is not running Cunhao Lu via B4 Relay
@ 2026-10-17 14:50 ` Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 3/6] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-10-17 14:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Heiko Stuebner,
David S. Miller, Wolfgang Grandegger, Oliver Hartkopp, kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Jonathan Corbet, Shuah Khan, Randy Dunlap
Cc: linux-can, linux-kernel, linux-arm-kernel, linux-rockchip, netdev,
linux-doc, stable, Cunhao Lu
From: Cunhao Lu <1579567540@qq.com>
Drivers may need to wait for bus-off recovery before tearing down the
IRQ and hardware resources used by their restart callback. Calling
close_candev() at that point also flushes echo skbs, which belongs after
the driver has quiesced the users of those buffers.
Add close_candev_cancel_restart_sync() in linux/can/dev.h so drivers can
cancel the core restart work separately, without accessing can_priv's
work item directly. The helper only cancels restart work; callers must
still stop its producers and call close_candev() for the final cleanup.
Use the helper in close_candev(), preserving its existing cancellation
and echo cleanup order.
The following Rockchip fix uses the helper at the start of rkcanfd_stop()
to wait for recovery before IRQ teardown, leaving echo cleanup to
close_candev().
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
Changes in v6:
- Describe the helper's use by the following Rockchip fix.
---
drivers/net/can/dev/dev.c | 4 +---
include/linux/can/dev.h | 12 ++++++++++++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index d5cb22db2a346..a5f4ce7cb5264 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -535,9 +535,7 @@ EXPORT_SYMBOL_GPL(of_can_transceiver);
*/
void close_candev(struct net_device *dev)
{
- struct can_priv *priv = netdev_priv(dev);
-
- cancel_delayed_work_sync(&priv->restart_work);
+ close_candev_cancel_restart_sync(dev);
can_flush_echo_skb(dev);
}
EXPORT_SYMBOL_GPL(close_candev);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6d0710d6f571d..a277d12332f15 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -22,6 +22,7 @@
#include <linux/can/skb.h>
#include <linux/ethtool.h>
#include <linux/netdevice.h>
+#include <linux/workqueue.h>
/*
* CAN mode
@@ -115,6 +116,17 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
int open_candev(struct net_device *dev);
void close_candev(struct net_device *dev);
+
+/* Cancel bus-off restart before tearing down resources used by the restart
+ * callback. The driver must still call close_candev() for the final cleanup.
+ */
+static inline void close_candev_cancel_restart_sync(struct net_device *dev)
+{
+ struct can_priv *priv = netdev_priv(dev);
+
+ cancel_delayed_work_sync(&priv->restart_work);
+}
+
void can_set_default_mtu(struct net_device *dev);
void can_set_cap_info(struct net_device *dev);
int __must_check can_set_static_ctrlmode(struct net_device *dev,
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH net v6 3/6] can: rockchip_canfd: cancel bus-off restart before closing the device
2026-10-17 14:50 [PATCH net v6 0/6] can: fix bus-off restart and device close races Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 1/6] can: dev: prevent bus-off recovery when the interface is not running Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 2/6] can: dev: add close_candev_cancel_restart_sync() Cunhao Lu via B4 Relay
@ 2026-10-17 14:50 ` Cunhao Lu via B4 Relay
2026-10-19 2:57 ` netdev-bot+sashiko
2026-10-17 14:50 ` [PATCH net v6 4/6] can: rockchip_canfd: prevent timestamp work from rearming after bus-off Cunhao Lu via B4 Relay
` (2 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-10-17 14:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Heiko Stuebner,
David S. Miller, Wolfgang Grandegger, Oliver Hartkopp, kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Jonathan Corbet, Shuah Khan, Randy Dunlap
Cc: linux-can, linux-kernel, linux-arm-kernel, linux-rockchip, netdev,
linux-doc, stable, Cunhao Lu
From: Cunhao Lu <1579567540@qq.com>
The close path cancels restart_work in close_candev() only after freeing
the IRQ, disabling RX offload and stopping the controller. A concurrent
restart can re-enable the controller and interrupts after IRQ teardown,
or requeue timestamp work that accesses registers after runtime suspend
has disabled the controller clocks.
Call close_candev_cancel_restart_sync() at the start of ndo_stop(), while
the resources needed by a running restart are still available. Keep
close_candev() after IRQ teardown for final cancellation and echo cleanup.
During restart, mask the device's interrupts and drain active handlers
without disabling the shared IRQ line. Suspend handler access to the
controller until reinitialization is complete, and publish the initialized
state before resuming IRQ handling.
Drain timestamp work before reinitializing the timecounter, and initialize
the delayed work once at probe. Keep asynchronous cancellation in the
bus-off path. Also disable RX offload if request_irq() fails during open.
Fixes: ff60bfbaf67f ("can: rockchip_canfd: add driver for Rockchip CAN-FD controller")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
Changes in v3:
- Replace disable_irq()/enable_irq() with device interrupt masking,
mask readback, an irq_suspended flag and synchronize_irq().
- Pair release/acquire accesses to publish reinitialized state before
the IRQ handler resumes, without disabling other shared IRQ users.
- Initialize timestamp delayed work once at probe and synchronously
cancel it before reinitializing the timestamp state during restart.
- Disable RX offload when request_irq() fails during open.
---
drivers/net/can/rockchip/rockchip_canfd-core.c | 20 ++++++++++++++++++++
drivers/net/can/rockchip/rockchip_canfd-timestamp.c | 6 +++++-
drivers/net/can/rockchip/rockchip_canfd.h | 2 ++
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 37c1c22c40c9f..4b3db07242395 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -20,6 +20,7 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/smp.h>
#include <linux/string.h>
#include "rockchip_canfd.h"
@@ -356,7 +357,18 @@ static int rkcanfd_set_mode(struct net_device *ndev,
switch (mode) {
case CAN_MODE_START:
+ rkcanfd_chip_interrupts_disable(priv);
+ /* Flush the interrupt mask write before suspending the handler. */
+ rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
+ /* Keep shared IRQs from accessing the device during restart. */
+ WRITE_ONCE(priv->irq_suspended, true);
+ /* Wait for handlers that already passed the suspended check. */
+ synchronize_irq(ndev->irq);
+ rkcanfd_timestamp_stop_sync(priv);
+
rkcanfd_chip_start(priv);
+ /* Publish the initialized state before resuming IRQ handling. */
+ smp_store_release(&priv->irq_suspended, false);
rkcanfd_chip_interrupts_enable(priv);
netif_wake_queue(ndev);
break;
@@ -666,6 +678,10 @@ static irqreturn_t rkcanfd_irq(int irq, void *dev_id)
struct rkcanfd_priv *priv = dev_id;
u32 reg_int_unmasked, reg_int;
+ /* Pairs with the release after restart has initialized the device. */
+ if (smp_load_acquire(&priv->irq_suspended))
+ return IRQ_NONE;
+
reg_int_unmasked = rkcanfd_read(priv, RKCANFD_REG_INT);
reg_int = reg_int_unmasked & ~priv->reg_int_mask_default;
@@ -747,6 +763,7 @@ static int rkcanfd_open(struct net_device *ndev)
return 0;
out_rkcanfd_chip_stop:
+ can_rx_offload_disable(&priv->offload);
rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
pm_runtime_put(ndev->dev.parent);
out_close_candev:
@@ -758,6 +775,7 @@ static int rkcanfd_stop(struct net_device *ndev)
{
struct rkcanfd_priv *priv = netdev_priv(ndev);
+ close_candev_cancel_restart_sync(ndev);
netif_stop_queue(ndev);
rkcanfd_chip_interrupts_disable(priv);
@@ -934,6 +952,8 @@ static int rkcanfd_probe(struct platform_device *pdev)
if (err)
goto out_free_candev;
+ rkcanfd_timestamp_work_init(priv);
+
err = rkcanfd_register(priv);
if (err)
goto out_can_rx_offload_del;
diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index 72774cd2f94b7..ad582afbbd682 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -37,6 +37,11 @@ static void rkcanfd_timestamp_work(struct work_struct *work)
schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
}
+void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv)
+{
+ INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
+}
+
void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
{
const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
@@ -73,7 +78,6 @@ void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
max_cycles = min(max_cycles, cc->mask);
work_delay_ns = clocksource_cyc2ns(max_cycles, cc->mult, cc->shift);
priv->work_delay_jiffies = div_u64(work_delay_ns, 3u * NSEC_PER_SEC / HZ);
- INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
netdev_dbg(priv->ndev, "clock=%lu.%02luMHz bitrate=%lu.%02luMBit/s div=%u rate=%lu.%02luMHz mult=%u shift=%u delay=%lus\n",
priv->can.clock.freq / MEGA,
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 95bea9bfd8a2f..e1debab54534e 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -480,6 +480,7 @@ struct rkcanfd_priv {
u32 reg_mode_default;
u32 reg_int_mask_default;
+ bool irq_suspended;
struct rkcanfd_devtype_data devtype_data;
struct cyclecounter cc;
@@ -551,6 +552,7 @@ int rkcanfd_handle_rx_int(struct rkcanfd_priv *priv);
void rkcanfd_skb_set_timestamp(const struct rkcanfd_priv *priv,
struct sk_buff *skb, const u32 timestamp);
+void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv);
void rkcanfd_timestamp_init(struct rkcanfd_priv *priv);
void rkcanfd_timestamp_start(struct rkcanfd_priv *priv);
void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH net v6 3/6] can: rockchip_canfd: cancel bus-off restart before closing the device
2026-10-17 14:50 ` [PATCH net v6 3/6] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
@ 2026-10-19 2:57 ` netdev-bot+sashiko
0 siblings, 0 replies; 9+ messages in thread
From: netdev-bot+sashiko @ 2026-10-19 2:57 UTC (permalink / raw)
To: 1579567540
Cc: mkl, mailhol, heiko, davem, wg, socketcan, kernel, edumazet, kuba,
pabeni, horms, corbet, skhan, rdunlap, linux-can, linux-kernel,
linux-arm-kernel, linux-rockchip, netdev, linux-doc, stable
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The patch adds a boolean gate, priv->irq_suspended.
Pre-existing issues:
- [Medium] At this commit, rkcanfd_start_xmit() is not serialized with
the bus-off stop or with restart reinitialisation.
- [Medium] At this commit, rkcanfd_timestamp_work() calls
timecounter_read(&priv->tc), which updates frac, cycle_last and nsec,…
- [Low] At this commit, rkcanfd_timestamp_stop() (rockchip_canfd-
timestamp.c:101-104) only calls cancel_delayed_work(), while…
--
can: rockchip_canfd: cancel bus-off restart before closing the device
This patch cancels the bus-off restart at the start of ndo_stop(). During
CAN_MODE_START it masks the device interrupts, sets an irq_suspended flag
and synchronizes the IRQ, and it drains the timestamp work before the
controller is reinitialized. It also moves INIT_DELAYED_WORK() to probe and
disables RX offload when request_irq() fails in open.
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 37c1c22c40c9f..4b3db07242395 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
[ ... ]
> @@ -356,7 +357,18 @@ static int rkcanfd_set_mode(struct net_device *ndev,
>
> switch (mode) {
> case CAN_MODE_START:
> + rkcanfd_chip_interrupts_disable(priv);
> + /* Flush the interrupt mask write before suspending the handler. */
> + rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
> + /* Keep shared IRQs from accessing the device during restart. */
> + WRITE_ONCE(priv->irq_suspended, true);
> + /* Wait for handlers that already passed the suspended check. */
> + synchronize_irq(ndev->irq);
> + rkcanfd_timestamp_stop_sync(priv);
> +
> rkcanfd_chip_start(priv);
[Severity: Medium]
This isn't a bug introduced by this patch, but is rkcanfd_start_xmit()
serialized against this reinitialization at all?
The restart path quiesces the IRQ handler and the timestamp work, but not
the TX path. The bus-off path only turns the carrier off, and can_restart()
calls netif_carrier_on() before do_set_mode(). So xmit can run while
rkcanfd_chip_start() resets the controller and rkcanfd_chip_fifo_setup()
does:
WRITE_ONCE(priv->tx_head, 0);
WRITE_ONCE(priv->tx_tail, 0);
netdev_reset_queue(priv->ndev);
A concurrent rkcanfd_start_xmit() could write the TX frame registers and
RKCANFD_REG_CMD into a controller that is being reset. It could also leave
an echo_skb slot occupied after the indices have been zeroed.
On RKCANFD_QUIRK_RK3568_ERRATUM_12 parts, rkcanfd_start_xmit_write_cmd()
also writes the full mode register:
if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_12)
rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default);
Could that take the controller out of reset while it is bus-off, or before
the bit timing has been programmed?
A later patch in the series, "can: rockchip_canfd: serialize TX with bus-off
and restart", appears to fix this. It adds tx_lock and tx_enabled, calls
rkcanfd_tx_disable() in the bus-off path and at the start of
CAN_MODE_START, and makes xmit return NETDEV_TX_BUSY while TX is disabled.
> + /* Publish the initialized state before resuming IRQ handling. */
> + smp_store_release(&priv->irq_suspended, false);
> + rkcanfd_chip_interrupts_enable(priv);
> + netif_wake_queue(ndev);
> break;
> @@ -666,6 +678,10 @@ static irqreturn_t rkcanfd_irq(int irq, void *dev_id)
> struct rkcanfd_priv *priv = dev_id;
> u32 reg_int_unmasked, reg_int;
>
> + /* Pairs with the release after restart has initialized the device. */
> + if (smp_load_acquire(&priv->irq_suspended))
> + return IRQ_NONE;
> +
[Severity: Medium]
irq_suspended is set at the start of the CAN_MODE_START region in
rkcanfd_set_mode() and cleared before the final unmask and queue wake.
While it is set, rkcanfd_irq() returns IRQ_NONE. Is this flag enough to
keep the handler out of the rest of the restart?
Once smp_store_release() clears the flag, nothing stops a shared IRQ on
another CPU from running the whole handler before the restart has finished.
restart_work is preemptible, so the window could be wide:
CPU0 (restart_work)
rkcanfd_set_mode(CAN_MODE_START)
rkcanfd_chip_start(priv);
smp_store_release(&priv->irq_suspended, false);
<preempted>
CPU1 (shared IRQ)
rkcanfd_irq()
smp_load_acquire(&priv->irq_suspended) returns false
reads RKCANFD_REG_INT, sees a new bus-off
rkcanfd_handle_state_error_int()
rkcanfd_chip_stop(priv, CAN_STATE_BUS_OFF);
can_bus_off(ndev);
CPU0 resumes
rkcanfd_chip_interrupts_enable(priv);
netif_wake_queue(ndev);
Would this leave INT_MASK unmasked and the TX queue awake on a controller
that is in reset with the CAN state at CAN_STATE_BUS_OFF? That would undo
the newer bus-off.
The later patch "can: rockchip_canfd: serialize TX with bus-off and
restart" seems to close this window. It clears the flag, unmasks and wakes
the queue under tx_lock in rkcanfd_chip_start_finish(), and the bus-off
path takes tx_lock through rkcanfd_tx_disable(). The flag-as-gate scheme is
still present at the end of the series, though, including in
rkcanfd_open().
Lockdep cannot see a flag like this. Would a real lock shared with the
handler be a better fit here?
[ ... ]
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> index 72774cd2f94b7..ad582afbbd682 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
> @@ -37,6 +37,11 @@ static void rkcanfd_timestamp_work(struct work_struct *work)
> schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> }
[Severity: Low]
This isn't a bug introduced by this patch, but rkcanfd_timestamp_work()
always requeues itself, and rkcanfd_timestamp_stop() only does:
cancel_delayed_work(&priv->timestamp);
Suppose the bus-off interrupt runs rkcanfd_chip_stop()->
rkcanfd_timestamp_stop() while the worker is executing. Can the worker
rearm itself and keep polling RKCANFD_REG_TIMESTAMP on a controller in
reset? That would continue until the restart path or ndo_stop cancels it
synchronously.
This appears to be fixed later in the series by "can: rockchip_canfd:
prevent timestamp work from rearming after bus-off". That patch adds
timestamp_lock and a timestamp_active flag, which the worker checks before
reading or requeueing.
[Severity: Medium]
This is also a pre-existing issue. In rkcanfd_timestamp_work(),
timecounter_read(&priv->tc) updates cycle_last, nsec and frac in process
context without any lock.
rkcanfd_skb_set_timestamp() reads the same state from the IRQ handler for
RX, TX-done and error frames, also without a lock:
ns = timecounter_cyc2time(&priv->tc, timestamp);
Can the handler see a new cycle_last together with an old nsec, and produce
a hardware timestamp that is off by a whole work period? On 32-bit the u64
fields could also tear.
The later patch "can: rockchip_canfd: serialize timecounter timestamp
conversion" appears to fix this by holding timestamp_lock across
timecounter_cyc2time().
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-rockchip-canfd-restart-close-v6-0-ed605feb4b6e%40qq.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net v6 4/6] can: rockchip_canfd: prevent timestamp work from rearming after bus-off
2026-10-17 14:50 [PATCH net v6 0/6] can: fix bus-off restart and device close races Cunhao Lu via B4 Relay
` (2 preceding siblings ...)
2026-10-17 14:50 ` [PATCH net v6 3/6] can: rockchip_canfd: cancel bus-off restart before closing the device Cunhao Lu via B4 Relay
@ 2026-10-17 14:50 ` Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 5/6] can: rockchip_canfd: serialize TX with bus-off and restart Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 6/6] can: rockchip_canfd: serialize timecounter timestamp conversion Cunhao Lu via B4 Relay
5 siblings, 0 replies; 9+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-10-17 14:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Heiko Stuebner,
David S. Miller, Wolfgang Grandegger, Oliver Hartkopp, kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Jonathan Corbet, Shuah Khan, Randy Dunlap
Cc: linux-can, linux-kernel, linux-arm-kernel, linux-rockchip, netdev,
linux-doc, stable, Cunhao Lu
From: Cunhao Lu <1579567540@qq.com>
The bus-off IRQ cancels timestamp work asynchronously. If the worker
is already running, it can requeue itself after cancellation and keep
polling while the controller remains bus-off.
Add a dedicated lock and an active flag, initialized once at probe.
Serialize the worker's active check, counter read and self-requeue with
start and stop. Clear the flag under the lock before cancelling work
so no worker can read the counter or rearm after deactivation. Enable
the flag and schedule the initial work under the same lock at start.
Keep both cancellation calls outside the lock so synchronous stop can
wait for the worker to finish.
Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
.../net/can/rockchip/rockchip_canfd-timestamp.c | 34 +++++++++++++++++++---
drivers/net/can/rockchip/rockchip_canfd.h | 4 +++
2 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index ad582afbbd68..0fa791849c95 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -30,18 +30,27 @@ static void rkcanfd_timestamp_work(struct work_struct *work)
{
const struct delayed_work *delayed_work = to_delayed_work(work);
struct rkcanfd_priv *priv;
+ unsigned long flags;
priv = container_of(delayed_work, struct rkcanfd_priv, timestamp);
- timecounter_read(&priv->tc);
-
- schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
+ spin_lock_irqsave(&priv->timestamp_lock, flags);
+ if (priv->timestamp_active) {
+ timecounter_read(&priv->tc);
+
+ /* Serialize requeueing with timestamp_stop(). */
+ schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
+ }
+ spin_unlock_irqrestore(&priv->timestamp_lock, flags);
}
void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv)
{
+ spin_lock_init(&priv->timestamp_lock);
+ priv->timestamp_active = false;
INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
}
+/* The caller must quiesce IRQs and timestamp work before reconfiguration. */
void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
{
const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
@@ -93,17 +102,34 @@ void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
void rkcanfd_timestamp_start(struct rkcanfd_priv *priv)
{
- timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
+ unsigned long flags;
+ spin_lock_irqsave(&priv->timestamp_lock, flags);
+ timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
+ priv->timestamp_active = true;
schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
+ spin_unlock_irqrestore(&priv->timestamp_lock, flags);
}
void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->timestamp_lock, flags);
+ priv->timestamp_active = false;
+ spin_unlock_irqrestore(&priv->timestamp_lock, flags);
+
cancel_delayed_work(&priv->timestamp);
}
void rkcanfd_timestamp_stop_sync(struct rkcanfd_priv *priv)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->timestamp_lock, flags);
+ priv->timestamp_active = false;
+ spin_unlock_irqrestore(&priv->timestamp_lock, flags);
+
+ /* The worker needs timestamp_lock to finish. */
cancel_delayed_work_sync(&priv->timestamp);
}
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index e1debab54534..2743c96ed42f 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -15,6 +15,7 @@
#include <linux/netdevice.h>
#include <linux/reset.h>
#include <linux/skbuff.h>
+#include <linux/spinlock.h>
#include <linux/timecounter.h>
#include <linux/types.h>
#include <linux/u64_stats_sync.h>
@@ -485,8 +486,11 @@ struct rkcanfd_priv {
struct cyclecounter cc;
struct timecounter tc;
+ /* Protects timestamp_active and worker reads/requeueing. */
+ spinlock_t timestamp_lock;
struct delayed_work timestamp;
unsigned long work_delay_jiffies;
+ bool timestamp_active;
struct can_berr_counter bec;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH net v6 5/6] can: rockchip_canfd: serialize TX with bus-off and restart
2026-10-17 14:50 [PATCH net v6 0/6] can: fix bus-off restart and device close races Cunhao Lu via B4 Relay
` (3 preceding siblings ...)
2026-10-17 14:50 ` [PATCH net v6 4/6] can: rockchip_canfd: prevent timestamp work from rearming after bus-off Cunhao Lu via B4 Relay
@ 2026-10-17 14:50 ` Cunhao Lu via B4 Relay
2026-10-17 14:50 ` [PATCH net v6 6/6] can: rockchip_canfd: serialize timecounter timestamp conversion Cunhao Lu via B4 Relay
5 siblings, 0 replies; 9+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-10-17 14:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Heiko Stuebner,
David S. Miller, Wolfgang Grandegger, Oliver Hartkopp, kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Jonathan Corbet, Shuah Khan, Randy Dunlap
Cc: linux-can, linux-kernel, linux-arm-kernel, linux-rockchip, netdev,
linux-doc, stable, Cunhao Lu
From: Cunhao Lu <1579567540@qq.com>
The bus-off path can reset the controller while ndo_start_xmit() is
active. CAN restart also flushes the echo ring before calling set_mode(),
so disabling TX in that callback is too late to protect echo slots.
Serialize TX submission, RX-side completion and retry with a private
lock and enabled flag. Disable TX and drain these users before publishing
CAN_STATE_BUS_OFF, protecting the core's echo flush during automatic and
manual recovery. Reject further submissions with NETDEV_TX_BUSY.
If close disables TX after an RX IRQ has consumed a self-transmitted
frame, skipping echo filtering alone would deliver it as ordinary RX.
Check tx_enabled under the same lock after consuming the FIFO entry. If
TX is disabled, drop the entry without accessing the echo ring, retrying
TX, waking the queue or allocating an ordinary RX skb. Return success so
the caller continues draining the FIFO.
After draining the old IRQ and reinitializing the controller, restore
carrier, enable TX, resume IRQ handling and wake the queue under the
lock. This prevents the old bus-off IRQ from leaving manual recovery
with carrier off, while allowing a new bus-off to take precedence.
Keep shared IRQ handling suspended during open until initialization is
complete, and leave TX disabled on close or failed open. Keep resets and
synchronous IRQ/work draining outside the lock.
Keep a reference to the original skb across can_put_echo_skb() and
release it after unlocking, so its socket destructor cannot run with
hard IRQs disabled. If echo preparation fails, drop the frame without
advancing tx_head, accounting BQL or requesting TX. This also fixes the
existing failure path that could stall TX on an empty echo slot.
Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
Changes in v6:
- Defer the original skb's release until after unlocking.
- Abort TX submission if echo preparation fails.
Changes in v5:
- Drop consumed RX FIFO entries when tx_enabled is false under tx_lock,
rather than skipping only echo filtering and delivering them as RX.
---
drivers/net/can/rockchip/rockchip_canfd-core.c | 51 ++++++++++++++++++++++----
drivers/net/can/rockchip/rockchip_canfd-rx.c | 26 +++++++++----
drivers/net/can/rockchip/rockchip_canfd-tx.c | 40 +++++++++++++++++---
drivers/net/can/rockchip/rockchip_canfd.h | 3 ++
4 files changed, 100 insertions(+), 20 deletions(-)
diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 4b3db07242395..150748c0b6845 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -326,6 +326,35 @@ static void rkcanfd_chip_start(struct rkcanfd_priv *priv)
rkcanfd_read(priv, RKCANFD_REG_MODE));
}
+static void rkcanfd_tx_disable(struct rkcanfd_priv *priv)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->tx_lock, flags);
+ priv->tx_enabled = false;
+ netif_stop_queue(priv->ndev);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+}
+
+static void rkcanfd_chip_start_finish(struct rkcanfd_priv *priv)
+{
+ unsigned long flags;
+
+ /* Serialize carrier, IRQ enable and queue wake with a new bus-off. */
+ spin_lock_irqsave(&priv->tx_lock, flags);
+ /* A manual restart may have raced with the old bus-off IRQ's
+ * netif_carrier_off(). That IRQ is now drained; restore carrier
+ * before resuming IRQ handling so a new bus-off takes precedence.
+ */
+ netif_carrier_on(priv->ndev);
+ priv->tx_enabled = true;
+ /* Publish the initialized state before resuming IRQ handling. */
+ smp_store_release(&priv->irq_suspended, false);
+ rkcanfd_chip_interrupts_enable(priv);
+ netif_wake_queue(priv->ndev);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+}
+
static void __rkcanfd_chip_stop(struct rkcanfd_priv *priv, const enum can_state state)
{
priv->can.state = state;
@@ -357,6 +386,7 @@ static int rkcanfd_set_mode(struct net_device *ndev,
switch (mode) {
case CAN_MODE_START:
+ rkcanfd_tx_disable(priv);
rkcanfd_chip_interrupts_disable(priv);
/* Flush the interrupt mask write before suspending the handler. */
rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
@@ -367,10 +397,7 @@ static int rkcanfd_set_mode(struct net_device *ndev,
rkcanfd_timestamp_stop_sync(priv);
rkcanfd_chip_start(priv);
- /* Publish the initialized state before resuming IRQ handling. */
- smp_store_release(&priv->irq_suspended, false);
- rkcanfd_chip_interrupts_enable(priv);
- netif_wake_queue(ndev);
+ rkcanfd_chip_start_finish(priv);
break;
default:
@@ -604,6 +631,12 @@ static int rkcanfd_handle_state_error_int(struct rkcanfd_priv *priv)
* handles cf == NULL.
*/
skb = rkcanfd_alloc_can_err_skb(priv, &cf, ×tamp);
+ if (new_state == CAN_STATE_BUS_OFF) {
+ /* Drain TX before publishing BUS_OFF, which permits a manual
+ * restart to flush echo SKBs before calling our set_mode().
+ */
+ rkcanfd_tx_disable(priv);
+ }
can_change_state(ndev, cf, tx_state, rx_state);
if (new_state == CAN_STATE_BUS_OFF) {
@@ -749,6 +782,8 @@ static int rkcanfd_open(struct net_device *ndev)
if (err)
goto out_close_candev;
+ /* A shared IRQ may run as soon as request_irq() succeeds. */
+ WRITE_ONCE(priv->irq_suspended, true);
rkcanfd_chip_start(priv);
can_rx_offload_enable(&priv->offload);
@@ -756,13 +791,12 @@ static int rkcanfd_open(struct net_device *ndev)
if (err)
goto out_rkcanfd_chip_stop;
- rkcanfd_chip_interrupts_enable(priv);
-
- netif_start_queue(ndev);
+ rkcanfd_chip_start_finish(priv);
return 0;
out_rkcanfd_chip_stop:
+ rkcanfd_tx_disable(priv);
can_rx_offload_disable(&priv->offload);
rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
pm_runtime_put(ndev->dev.parent);
@@ -776,7 +810,7 @@ static int rkcanfd_stop(struct net_device *ndev)
struct rkcanfd_priv *priv = netdev_priv(ndev);
close_candev_cancel_restart_sync(ndev);
- netif_stop_queue(ndev);
+ rkcanfd_tx_disable(priv);
rkcanfd_chip_interrupts_disable(priv);
free_irq(ndev->irq, priv);
@@ -939,6 +973,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
priv->can.do_set_mode = rkcanfd_set_mode;
priv->can.do_get_berr_counter = rkcanfd_get_berr_counter;
priv->ndev = ndev;
+ spin_lock_init(&priv->tx_lock);
match = device_get_match_data(&pdev->dev);
if (match) {
diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
index 24e87daa1df00..5e64317d4eee6 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
@@ -91,6 +91,7 @@ rkcanfd_fifo_header_to_cfd_header(const struct rkcanfd_priv *priv,
return len + cfd->len;
}
+/* Called with tx_lock held and TX enabled. */
static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
const struct canfd_frame *cfd_rx, const u32 ts,
bool *tx_done)
@@ -204,7 +205,9 @@ static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
struct canfd_frame cfd[1] = { }, *skb_cfd;
struct rkcanfd_fifo_header header[1] = { };
struct sk_buff *skb;
+ unsigned long flags;
unsigned int len;
+ bool tx_done = false;
int err;
/* read header into separate struct and convert it later */
@@ -237,15 +240,24 @@ static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
return 0;
}
- if (rkcanfd_get_tx_pending(priv)) {
- bool tx_done = false;
+ /* Drop FIFO entries while TX is disabled, as they may be self-
+ * received frames. Do not access the echo ring, retry TX or wake the
+ * queue while restart owns the ring. The entry is already consumed.
+ */
+ spin_lock_irqsave(&priv->tx_lock, flags);
+ if (unlikely(!priv->tx_enabled)) {
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+ return 0;
+ }
+ err = 0;
+ if (rkcanfd_get_tx_pending(priv))
err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
- if (err)
- return err;
- if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
- return 0;
- }
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+ if (err)
+ return err;
+ if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
+ return 0;
/* Manual handling of CAN Bus Error counters. See
* rkcanfd_get_corrected_berr_counter() for detailed
diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 12200dcfd3389..f86e31599b0a8 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -69,12 +69,19 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
u32 reg_frameinfo, reg_id, reg_cmd;
unsigned int tx_head, frame_len;
const struct canfd_frame *cfd;
+ unsigned long flags;
int err;
u8 i;
if (can_dev_dropped_skb(ndev, skb))
return NETDEV_TX_OK;
+ spin_lock_irqsave(&priv->tx_lock, flags);
+ if (unlikely(!priv->tx_enabled)) {
+ netif_stop_queue(ndev);
+ goto out_busy;
+ }
+
if (!netif_subqueue_maybe_stop(priv->ndev, 0,
rkcanfd_get_effective_tx_free(priv),
RKCANFD_TX_STOP_THRESHOLD,
@@ -85,7 +92,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
priv->tx_head, priv->tx_tail,
rkcanfd_get_tx_pending(priv));
- return NETDEV_TX_BUSY;
+ goto out_busy;
}
cfd = (struct canfd_frame *)skb->data;
@@ -117,16 +124,21 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
tx_head = rkcanfd_get_tx_head(priv);
reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
+ /* Keep the original skb alive until IRQs are restored so echo
+ * preparation cannot invoke its socket destructor under tx_lock.
+ */
+ frame_len = can_skb_get_frame_len(skb);
+ err = can_put_echo_skb(skb_get(skb), ndev, tx_head, frame_len);
+ if (unlikely(err || !priv->can.echo_skb[tx_head]))
+ goto out_drop;
+
rkcanfd_write(priv, RKCANFD_REG_FD_TXFRAMEINFO, reg_frameinfo);
rkcanfd_write(priv, RKCANFD_REG_FD_TXID, reg_id);
for (i = 0; i < cfd->len; i += 4)
rkcanfd_write(priv, RKCANFD_REG_FD_TXDATA0 + i,
*(u32 *)(cfd->data + i));
- frame_len = can_skb_get_frame_len(skb);
- err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
- if (!err)
- netdev_sent_queue(priv->ndev, frame_len);
+ netdev_sent_queue(priv->ndev, frame_len);
WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
@@ -137,7 +149,25 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
RKCANFD_TX_STOP_THRESHOLD,
RKCANFD_TX_START_THRESHOLD);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+ dev_consume_skb_any(skb);
+
+ return NETDEV_TX_OK;
+
+out_drop:
+ ndev->stats.tx_dropped++;
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+ /* An invalid index leaves the extra reference unconsumed. */
+ if (err == -EINVAL)
+ dev_kfree_skb_any(skb);
+ dev_kfree_skb_any(skb);
+
return NETDEV_TX_OK;
+
+out_busy:
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+ return NETDEV_TX_BUSY;
}
void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 2743c96ed42f0..4a2bdc205296e 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -476,6 +476,9 @@ struct rkcanfd_priv {
struct net_device *ndev;
void __iomem *regs;
+ /* Serialize TX submission/completion with bus-off and restart. */
+ spinlock_t tx_lock;
+ bool tx_enabled;
unsigned int tx_head;
unsigned int tx_tail;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH net v6 6/6] can: rockchip_canfd: serialize timecounter timestamp conversion
2026-10-17 14:50 [PATCH net v6 0/6] can: fix bus-off restart and device close races Cunhao Lu via B4 Relay
` (4 preceding siblings ...)
2026-10-17 14:50 ` [PATCH net v6 5/6] can: rockchip_canfd: serialize TX with bus-off and restart Cunhao Lu via B4 Relay
@ 2026-10-17 14:50 ` Cunhao Lu via B4 Relay
5 siblings, 0 replies; 9+ messages in thread
From: Cunhao Lu via B4 Relay @ 2026-10-17 14:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Heiko Stuebner,
David S. Miller, Wolfgang Grandegger, Oliver Hartkopp, kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Jonathan Corbet, Shuah Khan, Randy Dunlap
Cc: linux-can, linux-kernel, linux-arm-kernel, linux-rockchip, netdev,
linux-doc, stable, Cunhao Lu
From: Cunhao Lu <1579567540@qq.com>
The timestamp worker advances the timecounter with timecounter_read()
while RX, TX completion and error-frame paths use timecounter_cyc2time()
to convert hardware timestamps without holding timestamp_lock.
timecounter_read() updates frac, cycle_last and nsec in separate steps.
A concurrent conversion can observe the new cycle_last with the old
nsec and produce an incorrect timestamp.
Hold timestamp_lock across timecounter_cyc2time(), using the same lock
as timecounter_read() and timecounter_init(). Keep the SKB timestamp
assignment outside the critical section.
TX completion nests timestamp_lock inside tx_lock; no timestamp-lock
holder acquires tx_lock.
Fixes: 4e1a18bab124 ("can: rockchip_canfd: add hardware timestamping support")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
drivers/net/can/rockchip/rockchip_canfd-timestamp.c | 5 ++++-
drivers/net/can/rockchip/rockchip_canfd.h | 4 ++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index 0fa791849c95..05f1005ee506 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -15,13 +15,16 @@ static u64 rkcanfd_timestamp_read(struct cyclecounter *cc)
return rkcanfd_get_timestamp(priv);
}
-void rkcanfd_skb_set_timestamp(const struct rkcanfd_priv *priv,
+void rkcanfd_skb_set_timestamp(struct rkcanfd_priv *priv,
struct sk_buff *skb, const u32 timestamp)
{
struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+ unsigned long flags;
u64 ns;
+ spin_lock_irqsave(&priv->timestamp_lock, flags);
ns = timecounter_cyc2time(&priv->tc, timestamp);
+ spin_unlock_irqrestore(&priv->timestamp_lock, flags);
hwtstamps->hwtstamp = ns_to_ktime(ns);
}
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 4a2bdc205296..63f146ff8e6b 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -489,7 +489,7 @@ struct rkcanfd_priv {
struct cyclecounter cc;
struct timecounter tc;
- /* Protects timestamp_active and worker reads/requeueing. */
+ /* Protects tc, timestamp_active and worker requeueing. */
spinlock_t timestamp_lock;
struct delayed_work timestamp;
unsigned long work_delay_jiffies;
@@ -557,7 +557,7 @@ void rkcanfd_ethtool_init(struct rkcanfd_priv *priv);
int rkcanfd_handle_rx_int(struct rkcanfd_priv *priv);
-void rkcanfd_skb_set_timestamp(const struct rkcanfd_priv *priv,
+void rkcanfd_skb_set_timestamp(struct rkcanfd_priv *priv,
struct sk_buff *skb, const u32 timestamp);
void rkcanfd_timestamp_work_init(struct rkcanfd_priv *priv);
void rkcanfd_timestamp_init(struct rkcanfd_priv *priv);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread