Re: [PATCH net v9 06/10] 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch has a missing stable dependency annotation.

--

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

The patch adds a call to close_candev_cancel_restart_sync() at the start
of rkcanfd_stop(). Today restart_work is only cancelled in close_candev(),
which runs after the IRQ has been freed and the controller stopped. The
earlier cancel is meant to stop a concurrent bus-off restart from turning
the controller back on after IRQ teardown.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 1cae86973da5a..59a79fe057e66 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -758,6 +758,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);

[Severity: Low]
This patch has Fixes: ff60bfbaf67f and Cc: stable@vger.kernel.org. Will it
build when backported on its own?

The close_candev_cancel_restart_sync() helper is only added to
include/linux/can/dev.h by the earlier commit "can: dev: add
close_candev_cancel_restart_sync()". That commit has no Fixes: tag and no
Cc: stable tag:

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

The early cancel also appears to depend on the earlier commit "can: dev:
check netif_running() before bus-off recovery". That commit adds these
checks in drivers/net/can/dev/dev.c:

can_bus_off():
	restart = priv->restart_ms && netif_running(dev);

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

Without those checks, a bus-off IRQ can still arrive after the new early
cancel but before free_irq(). In that case can_bus_off() would requeue
restart_work.

can_restart() would then call do_set_mode(CAN_MODE_START) unconditionally:

can_restart_work()
  can_restart()
    priv->do_set_mode(dev, CAN_MODE_START)
      rkcanfd_set_mode()
        rkcanfd_chip_start()

That turns the controller and its interrupts back on and schedules
timestamp work after free_irq() and rkcanfd_chip_stop_sync(). This is the
same failure the commit message describes. The cancel_delayed_work_sync()
in close_candev() runs after rkcanfd_chip_stop_sync(), so it comes too late
to prevent it.

In mainline both prerequisites come earlier in the series, so the in-tree
fix is complete. Other patches in this series say what their backports need
(for example, the "Backports must include ..." notes in "can:
rockchip_canfd: retry the outstanding TX buffer" and "can: rockchip_canfd:
serialize TX state and command writes").

Could this patch carry a similar note, or Stable-dep-of annotations, for
both of these dependencies?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com