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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 2 · Low: 0

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 now has rkcanfd_set_mode() return -ENETDOWN when it
  sees priv->closing.
- [Medium] rkcanfd_stop() publishes priv->closing = true (line 776)
  before it masks the controller interrupts with…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits in the
  restart path the patch hardens and it can corrupt kernel…
- [High] This problem wasn't introduced by this patch, but rkcanfd_open()
  calls can_rx_offload_enable() (napi_enable) at line 750, before the…

--

can: rockchip_canfd: stop bus-off restart before closing the device

This patch adds a mutex and a closing flag so that the bus-off restart
callback, rkcanfd_set_mode(), cannot run concurrently with rkcanfd_stop().
In the close path, interrupts are now masked and the IRQ synchronized
before restart_work is cancelled. During restart, the IRQ is disabled
while the hardware is reinitialized.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 37c1c22c40c9f..e38ddd143a3ad 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c

[ ... ]

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

[ ... ]

> +	if (mode != CAN_MODE_START)
>  		return -EOPNOTSUPP;
> +
> +	mutex_lock(&priv->state_lock);
> +	if (READ_ONCE(priv->closing)) {
> +		err = -ENETDOWN;
> +		goto out_unlock;
>  	}

[Severity: Medium]
Suppose restart_work runs after rkcanfd_stop() has set closing and dropped
state_lock, but before it reaches cancel_delayed_work_sync(). How does the
CAN core handle the -ENETDOWN returned here?

In can_restart() in drivers/net/can/dev/dev.c, several steps have already
run by the time do_set_mode() is called:

	/* send restart message upstream */
	skb = alloc_can_err_skb(dev, &cf);
	if (skb) {
		cf->can_id |= CAN_ERR_RESTARTED;
		netif_rx(skb);
	}

	/* Now restart the device */
	netif_carrier_on(dev);
	err = priv->do_set_mode(dev, CAN_MODE_START);
	if (err) {
		netdev_err(dev, "Restart failed, error %pe\n", ERR_PTR(err));
		netif_carrier_off(dev);

So in this window userspace receives a CAN_ERR_RESTARTED frame for a
restart that never happens. The carrier also goes on and then off again,
and a normal ifdown logs "Restart failed, error -ENETDOWN" at error level.

The commit message says the ordering is deliberate so that "a queued
restart can observe the closing state", which suggests this window is
expected to be hit.

Can the restart be aborted without these side effects? Alternatively,
can restart_work be cancelled before a restart can reach can_restart()?

>  
> -	return 0;
> +	disable_irq(ndev->irq);
> +	rkcanfd_chip_start(priv);

[Severity: High]
This isn't a bug introduced by this patch, but this patch hardens the
restart path, so is it safe to reinitialize the timestamp delayed_work
here?

On bus-off, rkcanfd_handle_state_error_int() runs in hardirq context and
calls rkcanfd_chip_stop()->rkcanfd_timestamp_stop(). That function only
does a non-sync cancel:

void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv)
{
	cancel_delayed_work(&priv->timestamp);
}

Meanwhile, rkcanfd_timestamp_work() always re-arms itself:

	timecounter_read(&priv->tc);

	schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);

If the worker is running when the bus-off interrupt arrives, the cancel
does nothing and the worker re-arms the timer. When restart_ms expires,
the restart path runs:

can_restart_work()->can_restart()->rkcanfd_set_mode()->rkcanfd_chip_start()
    ->rkcanfd_timestamp_init()
        INIT_DELAYED_WORK(&priv->timestamp, rkcanfd_timestamp_work);
    ->rkcanfd_timestamp_start()
        schedule_delayed_work(&priv->timestamp, ...);

Can this corrupt the timer wheel? do_init_timer() clears
timer->entry.pprev while the timer is still enqueued. The following
schedule_delayed_work() then enqueues the same timer_list a second time.

If the old callback is still running instead, INIT_WORK clears its pool
association. The later cancel_delayed_work_sync() in rkcanfd_stop() then
cannot wait for it. Could that callback then call timecounter_read() on
the registers and re-queue itself after pm_runtime_put()? The commit
message says this patch prevents exactly that.

The new state_lock and the disable_irq()/enable_irq() pair keep close and
the IRQ handler out, but they don't cover the timestamp worker. Also,
timecounter_init() in rkcanfd_timestamp_start() races with the lockless
timecounter_read() in the worker.

Would it work to call INIT_DELAYED_WORK() once at probe time, and to call
cancel_delayed_work_sync(&priv->timestamp) in the process-context restart
path before the timecounter is reinitialized?

> +	rkcanfd_chip_interrupts_enable(priv);
> +	enable_irq(ndev->irq);
> +	netif_wake_queue(ndev);

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but this hunk touches the request_irq()
path in rkcanfd_open(). What happens to NAPI if request_irq() fails?

can_rx_offload_enable() has already called napi_enable() by this point,
and the error unwind never disables it:

out_rkcanfd_chip_stop:
	rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);
	pm_runtime_put(ndev->dev.parent);
out_close_candev:
	close_candev(ndev);
	return err;

On the next ifup, can_rx_offload_enable()->napi_enable()->
napi_enable_locked() would find NAPI already enabled and hit:

	BUG_ON(!test_bit(NAPI_STATE_SCHED, &val));

Should can_rx_offload_disable(&priv->offload) be added to this error path?

[ ... ]

> @@ -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);

[Severity: Medium]
Is this ordering safe on a level-triggered line? priv->closing is set
before the controller's interrupt mask is written. The new early return
in rkcanfd_irq() neither acks nor masks the source:

	if (READ_ONCE(priv->closing))
		return IRQ_NONE;

The RK3568 CAN-FD interrupts are IRQ_TYPE_LEVEL_HIGH in rk3568.dtsi.
Suppose the controller raises RX_FINISH or an error interrupt between the
WRITE_ONCE() and rkcanfd_chip_interrupts_disable(). The line then stays
asserted, and the handler rejects it every time it is redelivered.

If that IRQ is routed to the CPU running rkcanfd_stop(), could that CPU
livelock in hardirq and never reach the INT_MASK write? If it goes to
another CPU, the storm would last until the spurious IRQ detector reports
"nobody cared" and disables the line. That would also hit any other users
of this IRQF_SHARED line.

The mutex section is preemptible, so on PREEMPT and PREEMPT_RT kernels the
stop task can be preempted between these two statements, which widens the
window. On PREEMPT_RT, the forced-threaded handler returning IRQ_NONE
combined with the oneshot unmask could loop the same way and starve the
stop task.

Would masking the hardware first help here, i.e. write INT_MASK, read it
back, and only then set closing? Acking or masking inside the closing
branch of rkcanfd_irq() might also work.

> +	mutex_unlock(&priv->state_lock);
> +	synchronize_irq(ndev->irq);
> +	cancel_delayed_work_sync(&priv->can.restart_work);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_5802EE88434010229EDF7A53084D994C6106%40qq.com

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

[Cunhao Lu]

Hi,

I checked all four findings against the current branch.

> - [Medium] The patch now has rkcanfd_set_mode() return -ENETDOWN when it
>   sees priv->closing.
> - [Medium] rkcanfd_stop() publishes priv->closing = true (line 776)
>   before it masks the controller interrupts with…

The two issues involving closing were addressed in v2.

> [Severity: High]
> This isn't a bug introduced by this patch, but this patch hardens the
> restart path, so is it safe to reinitialize the timestamp delayed_work
> here?
> ...
> Would it work to call INIT_DELAYED_WORK() once at probe time, and to call
> cancel_delayed_work_sync(&priv->timestamp) in the process-context restart
> path before the timecounter is reinitialized?

Agreed. Excluding the IRQ handler does not stop the timestamp worker.
I will initialize the delayed_work once at probe time and synchronously
cancel it in the process-context restart path, after excluding the IRQ
handler and before rkcanfd_chip_start() reinitializes the timestamp
state. The bus-off IRQ path will keep its non-synchronous cancellation.


> [Severity: High]
> This is a pre-existing issue, but this hunk touches the request_irq()
> path in rkcanfd_open(). What happens to NAPI if request_irq() fails?
> ...
> Should can_rx_offload_disable(&priv->offload) be added to this error path?

Yes. I will add it to the request_irq() failure path, before stopping the
controller, to balance can_rx_offload_enable(). I will address this and
the timestamp work issue in separate patches.

I will fix these bugs in v3.

Regards,
Cunhao Lu