[PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Cunhao Lu]

The close path cancels restart_work after freeing the IRQ and stopping
hardware. A queued bus-off restart can therefore enable the controller
and interrupts after teardown. It can also restart timestamp work before
runtime suspend.

Serialize restart with the close transition using a mutex. Mark the
device closing and mask its interrupts before synchronizing the IRQ.
Release the mutex before cancelling restart_work, so a queued restart
can observe the closing state without deadlocking.

Exclude the IRQ handler while reinitializing the hardware on restart.
Do not disable the shared IRQ line while waiting for work to finish.

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>
---
When automatic bus-off recovery is enabled, bringing the interface down
can race with a pending or running restart. The driver currently relies
on close_candev() to cancel restart_work, but calls it only after freeing
the IRQ, disabling RX offload and stopping the controller. A restart in
that interval can re-enable the controller and its interrupts after IRQ
teardown, and requeue timestamp work after it has been stopped. That work
can subsequently access registers after runtime suspend disables the
controller clocks.

Serialize the restart callback and the close transition with a mutex and
a closing flag. The close path marks the device as closing and masks its
interrupts, then synchronizes the IRQ before cancelling restart_work.
This drains any handler that could still schedule a restart. Cancellation
runs outside the mutex so that a restart waiting for the lock can observe
the closing flag and exit. IRQ teardown, RX offload shutdown and the final
hardware stop follow only after restart work has finished. The shared IRQ
line is disabled only around hardware reinitialization in the restart
callback, not while cancelling work in the close path.

The standalone fix was tested on a LubanCat-5IO running a PREEMPT_RT
kernel. The 1 Mbit/s CAN bus was connected to four CANopen motors
operating at 100 Hz and four sensors operating at 20 Hz.
A bitrate mismatch forced bus-off, with automatic restart set to 100 ms;
25 iterations brought the interface down at delays spanning immediate
close through 130 ms after bus-off. Every iteration reached bus-off and
scheduled recovery, with no crashes or kernel traces observed. Automatic
recovery was also checked with the interface left up, including a 1 s
restart delay. The reported build completed without warnings.
---
 drivers/net/can/rockchip/rockchip_canfd-core.c | 40 +++++++++++++++++++-------
 drivers/net/can/rockchip/rockchip_canfd.h      |  5 ++++
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 37c1c22c40c9..e38ddd143a3a 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -244,6 +244,8 @@ static void rkcanfd_chip_interrupts_enable(const struct rkcanfd_priv *priv)
 static void rkcanfd_chip_interrupts_disable(const struct rkcanfd_priv *priv)
 {
 	rkcanfd_write(priv, RKCANFD_REG_INT_MASK, RKCANFD_REG_INT_ALL);
+	/* Flush the mask before synchronizing a shared IRQ. */
+	rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
 }
 
 static void rkcanfd_chip_fifo_setup(struct rkcanfd_priv *priv)
@@ -353,19 +355,27 @@ static int rkcanfd_set_mode(struct net_device *ndev,
 			    enum can_mode mode)
 {
 	struct rkcanfd_priv *priv = netdev_priv(ndev);
+	int err = 0;
 
-	switch (mode) {
-	case CAN_MODE_START:
-		rkcanfd_chip_start(priv);
-		rkcanfd_chip_interrupts_enable(priv);
-		netif_wake_queue(ndev);
-		break;
-
-	default:
+	if (mode != CAN_MODE_START)
 		return -EOPNOTSUPP;
+
+	mutex_lock(&priv->state_lock);
+	if (READ_ONCE(priv->closing)) {
+		err = -ENETDOWN;
+		goto out_unlock;
 	}
 
-	return 0;
+	disable_irq(ndev->irq);
+	rkcanfd_chip_start(priv);
+	rkcanfd_chip_interrupts_enable(priv);
+	enable_irq(ndev->irq);
+	netif_wake_queue(ndev);
+
+out_unlock:
+	mutex_unlock(&priv->state_lock);
+
+	return err;
 }
 
 static struct sk_buff *
@@ -666,6 +676,9 @@ static irqreturn_t rkcanfd_irq(int irq, void *dev_id)
 	struct rkcanfd_priv *priv = dev_id;
 	u32 reg_int_unmasked, reg_int;
 
+	if (READ_ONCE(priv->closing))
+		return IRQ_NONE;
+
 	reg_int_unmasked = rkcanfd_read(priv, RKCANFD_REG_INT);
 	reg_int = reg_int_unmasked & ~priv->reg_int_mask_default;
 
@@ -736,6 +749,7 @@ static int rkcanfd_open(struct net_device *ndev)
 	rkcanfd_chip_start(priv);
 	can_rx_offload_enable(&priv->offload);
 
+	WRITE_ONCE(priv->closing, false);
 	err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);
 	if (err)
 		goto out_rkcanfd_chip_stop;
@@ -758,9 +772,14 @@ static int rkcanfd_stop(struct net_device *ndev)
 {
 	struct rkcanfd_priv *priv = netdev_priv(ndev);
 
+	mutex_lock(&priv->state_lock);
+	WRITE_ONCE(priv->closing, true);
+	rkcanfd_chip_interrupts_disable(priv);
+	mutex_unlock(&priv->state_lock);
+	synchronize_irq(ndev->irq);
+	cancel_delayed_work_sync(&priv->can.restart_work);
 	netif_stop_queue(ndev);
 
-	rkcanfd_chip_interrupts_disable(priv);
 	free_irq(ndev->irq, priv);
 	can_rx_offload_disable(&priv->offload);
 	rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
@@ -921,6 +940,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;
+	mutex_init(&priv->state_lock);
 
 	match = device_get_match_data(&pdev->dev);
 	if (match) {
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 95bea9bfd8a2..c81233142a74 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -12,6 +12,7 @@
 #include <linux/can/rx-offload.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 #include <linux/netdevice.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
@@ -478,6 +479,10 @@ struct rkcanfd_priv {
 	unsigned int tx_head;
 	unsigned int tx_tail;
 
+	bool closing;
+	/* Serializes restart with the close transition. */
+	struct mutex state_lock;
+
 	u32 reg_mode_default;
 	u32 reg_int_mask_default;
 	struct rkcanfd_devtype_data devtype_data;

---
base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
change-id: 20261013-rockchip-canfd-restart-close-ebde5954f1bf

Best regards,
--  
Cunhao Lu <1579567540@qq.com>

Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Cunhao Lu]

Hi,

>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

Thanks for pointing this out. This work started with a real failure on a
production robot, rather than a general scan for potential driver bugs.

The production system used a LubanCat-5IO with an RK3588, running a
6.18.55-current-rockchip64 PREEMPT_RT kernel with local CAN patches. Its
1 Mbit/s CAN bus connected four CANopen motors operating at 100 Hz and
four sensors operating at 20 Hz.

>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

The observed failure was a persistent TX stall: RX continued, but the TX
queue remained stopped and subsequent sends returned ENOBUFS. The
interface still reported ERROR-ACTIVE, and the hardware indicated neither
a bus-off condition nor a pending TX request.

I will submit two patches for this issue; this is the first one.

This patch is not a direct fix for that failure. It fixes a race
that was already present in the driver and that I found during
fault-injection testing of the echo-loss fix: close_candev() cancels
restart_work only after the IRQ has been freed and the controller has
been stopped, so a queued bus-off restart can re-enable the controller
and its interrupts after IRQ teardown, and can requeue timestamp work
that may later access registers after runtime suspend has disabled the
clocks. The second patch builds on this serialization and addresses the
echo-loss stall itself.

Regards,
Cunhao Lu

Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

On 13.10.2026 11:22:37, Cunhao Lu wrote:
> The close path cancels restart_work after freeing the IRQ and stopping
> hardware. A queued bus-off restart can therefore enable the controller
> and interrupts after teardown. It can also restart timestamp work before
> runtime suspend.
>
> Serialize restart with the close transition using a mutex. Mark the
> device closing and mask its interrupts before synchronizing the IRQ.
> Release the mutex before cancelling restart_work, so a queued restart
> can observe the closing state without deadlocking.
>
> Exclude the IRQ handler while reinitializing the hardware on restart.
> Do not disable the shared IRQ line while waiting for work to finish.
>
> 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>

AFAICS this is a systematic problem and should IMHO not worked around in
one driver only.

netif_running() tests __LINK_STATE_START, which is removed in
__dev_close_many() prior to calling the driver's ndo_stop().

Can we make use of netif_running() prior to calling do_set_mode(dev,
CAN_MODE_START)? from can_restart()?

can_bus_off() should not schedule_delayed_work() if !netif_running().

We still have to call cancel_delayed_work_sync(&priv->can.restart_work)
from each driver, though.

What do you think?

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: 228 bytes --]

Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Cunhao Lu]

Hi Marc,

> AFAICS this is a systematic problem and should IMHO not worked around in
> one driver only.

Agreed. I initially only noticed the problem in the Rockchip CAN-FD
driver and did not consider the common CAN code.

> netif_running() tests __LINK_STATE_START, which is removed in
> __dev_close_many() prior to calling the driver's ndo_stop().
>
> Can we make use of netif_running() prior to calling do_set_mode(dev,
> CAN_MODE_START)? from can_restart()?

If you have no objections, I think it would be better to put the
netif_running() check after the existing !priv->do_set_mode check and
before netif_carrier_ok():

        if (!priv->do_set_mode)
                return -EOPNOTSUPP;

        if (!netif_running(dev))
                return -ENETDOWN;

This would preserve the existing error precedence for devices that do
not support restart.

> can_bus_off() should not schedule_delayed_work() if !netif_running().

Agreed. Although this would change the behavior of some drivers, I think
it should help improve stability.

> We still have to call cancel_delayed_work_sync(&priv->can.restart_work)
> from each driver, though.

I can make changes to the other drivers. However, I do not have the
corresponding hardware and would not be able to test the changes on
real devices, so I am concerned about the risk. I would like to confirm:
do I need to audit and fix the restart behavior of every CAN device
driver?

> What do you think?

I will address these comments in v2. Thanks for your review.

Regards,
Cunhao Lu

Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]

On 13.10.2026 21:06:21, Cunhao Lu wrote:
> Hi Marc,
>
> > AFAICS this is a systematic problem and should IMHO not worked around in
> > one driver only.
>
> Agreed. I initially only noticed the problem in the Rockchip CAN-FD
> driver and did not consider the common CAN code.

Thanks to your analysis, it was easy to reach that conclusion. :)

> > netif_running() tests __LINK_STATE_START, which is removed in
> > __dev_close_many() prior to calling the driver's ndo_stop().
> >
> > Can we make use of netif_running() prior to calling do_set_mode(dev,
> > CAN_MODE_START)? from can_restart()?
>
> If you have no objections, I think it would be better to put the
> netif_running() check after the existing !priv->do_set_mode check and
> before netif_carrier_ok():
>
>         if (!priv->do_set_mode)
>                 return -EOPNOTSUPP;
>
>         if (!netif_running(dev))
>                 return -ENETDOWN;
>
> This would preserve the existing error precedence for devices that do
> not support restart.

ACK

> > can_bus_off() should not schedule_delayed_work() if !netif_running().
>
> Agreed. Although this would change the behavior of some drivers, I think
> it should help improve stability.

ok

> > We still have to call cancel_delayed_work_sync(&priv->can.restart_work)
> > from each driver, though.

Let's wrap that into a function and put this into linux/can/dev.h. It's
name could be something like close_candev_cancel_restart_sync().

> I can make changes to the other drivers. However, I do not have the
> corresponding hardware and would not be able to test the changes on
> real devices, so I am concerned about the risk. I would like to confirm:
> do I need to audit and fix the restart behavior of every CAN device
> driver?

I think it's OK to fix the rockchip driver only.

> > What do you think?
>
> I will address these comments in v2. Thanks for your review.

Please split the changes into several patches:
- fix the existing infrastructure
- add new close_candev_cancel_restart_sync() function
- use this function in the rockchip driver

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: 228 bytes --]

Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Marc Kleine-Budde]

On 13.10.2026 11:22:37, Cunhao Lu wrote:
> The close path cancels restart_work after freeing the IRQ and stopping
> hardware. A queued bus-off restart can therefore enable the controller
> and interrupts after teardown. It can also restart timestamp work before
> runtime suspend.
>
> Serialize restart with the close transition using a mutex. Mark the
> device closing and mask its interrupts before synchronizing the IRQ.
> Release the mutex before cancelling restart_work, so a queued restart
> can observe the closing state without deadlocking.
>
> Exclude the IRQ handler while reinitializing the hardware on restart.
> Do not disable the shared IRQ line while waiting for work to finish.
>
> 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>

pr-bot: cr

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