Re: [PATCH net v2 3/3] can: rockchip_canfd: cancel bus-off restart before closing the device

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · 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] can: rockchip_canfd: disable_irq() called on shared IRQ

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the restart
  callback the patch modifies re-initializes a delayed_work that may…

--

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

This commit calls close_candev_cancel_restart_sync() at the start of
rkcanfd_stop(), so a running bus-off restart finishes while the IRQ and
other resources are still available. It also brackets the hardware
reinitialization in rkcanfd_set_mode() with disable_irq()/enable_irq().
That keeps the IRQ handler out while the controller is restarted.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 37c1c22c40c9f..fb8f6b0675022 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -356,8 +356,10 @@ static int rkcanfd_set_mode(struct net_device *ndev,
>  
>  	switch (mode) {
>  	case CAN_MODE_START:
> +		disable_irq(ndev->irq);
>  		rkcanfd_chip_start(priv);

[Severity: Medium]
rkcanfd_open() requests ndev->irq with IRQF_SHARED, and the commit
message itself relies on the line being shared. disable_irq() masks
the whole line at the irqchip, not only this controller's source.
It also waits in synchronize_irq() for the handlers of every other
device on that line.

Every device sharing the line therefore loses interrupt delivery for
as long as rkcanfd_chip_start() and rkcanfd_chip_interrupts_enable()
run. That window covers:

- the reset and work mode transitions
- FIFO setup
- bittiming programming
- timestamp init and start

The window can be opened from outside the box. Another node on the
CAN bus can drive this controller into bus-off. With restart-ms
configured, restart_work then runs rkcanfd_set_mode(), and a manual
can_restart_now() takes the same path. Each restart stalls the
unrelated devices sharing the IRQ.

Drivers on a shared IRQ are generally expected not to use
disable_irq() for their own exclusion, for exactly this reason.

Would it be possible to exclude rkcanfd_irq() with a real lock
instead? For example, the handler could take a spinlock, and
spin_lock_irqsave() could be held around the chip_start and
interrupts_enable sequence here.

That only works if everything reached from rkcanfd_chip_start() is
safe in atomic context. Can you confirm that? Or is there a reason
the restart path has to be able to sleep while the handler is held
off?

I would rather not see a software flag checked in the handler for
this. Without a lock it does not actually exclude a handler that
has already passed the check.

[Severity: High]
This issue was not introduced by this patch. It sits in the restart path
the patch modifies, though. Can rkcanfd_chip_start() here re-initialize
priv->timestamp while its timer is still queued?

On bus-off, the IRQ path stops the timestamp work without waiting for it:

rkcanfd_irq()
  rkcanfd_handle_state_error_int()
    rkcanfd_chip_stop(priv, CAN_STATE_BUS_OFF)
      rkcanfd_timestamp_stop()
        cancel_delayed_work(&priv->timestamp);

The work callback always re-arms itself:

drivers/net/can/rockchip/rockchip_canfd-timestamp.c:rkcanfd_timestamp_work() {
    ...
	timecounter_read(&priv->tc);

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

Bus-off may be handled while rkcanfd_timestamp_work() is running, either on
another CPU or because the IRQ preempted it on the same CPU. In that case
cancel_delayed_work() finds nothing pending, and the callback then re-queues
itself. From then on the work keeps re-arming.

When the restart runs (restart_work or can_restart_now()), it goes through:

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

INIT_DELAYED_WORK() clears the PENDING bit and the timer's linkage while the
timer_list is still in a timer-wheel bucket. The following
schedule_delayed_work() then sees the work as idle and enqueues the same
timer_list a second time. Can that corrupt the timer wheel hlist?
timecounter_init() also writes priv->tc while the old work may still be
using it.

The new disable_irq()/enable_irq() pair keeps the IRQ handler out. It does
not stop the timestamp work. One option is to cancel priv->timestamp
synchronously in this sleepable restart path before re-initializing it.
Another is to call INIT_DELAYED_WORK() once at probe or open time rather
than on every rkcanfd_chip_start().

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

[ ... ]

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