Re: [PATCH net v5 2/6] can: dev: add close_candev_cancel_restart_sync()

[netdev-bot+sashiko]

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] Concern: close_candev_cancel_restart_sync() is added to
  include/linux/can/dev.h, but the only call site is a no-op refactor in…
- [Low] Concern: the commit has 'Cc: stable@xxxxxxxxxxxxxxx' but no
  Fixes: tag and no '# prerequisite for ...' note, while being a no-op…

--

can: dev: add close_candev_cancel_restart_sync()

This adds an inline helper, close_candev_cancel_restart_sync(), to
include/linux/can/dev.h. It only cancels the bus-off restart work, so
drivers can cancel it before tearing down IRQ and hardware resources.
close_candev() now uses the helper and keeps its existing order: cancel
the restart work, then flush the echo skbs.

> Use the helper in close_candev(), preserving its existing cancellation
> and echo cleanup order.
>
> Cc: stable@xxxxxxxxxxxxxxx

[Severity: Low]
This isn't a bug, but could the commit message say why this is going to
stable?

On its own, the patch is a no-op refactor of close_candev() plus a new
helper. It has no Fixes: tag and no "# prerequisite for ..." annotation.

The only new user seems to be the next patch in the series, "can:
rockchip_canfd: cancel bus-off restart before closing the device". That
patch has its own Fixes: ff60bfbaf67f and Cc: stable.

Would it help backporters to mark this patch as a prerequisite for that
fix? If the rockchip fix were picked without it, the build would fail
rather than the code misbehaving, so this only affects commit metadata.

> 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

[ ... ]

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

[Severity: Low]
In this patch, the only caller of close_candev_cancel_restart_sync() is
close_candev() in drivers/net/can/dev/dev.c, and that conversion keeps
the old behaviour. As a result, no driver's ndo_stop gets the early
cancellation that the commit message describes.

Is the helper meant to be used only by later patches?

This looks resolved later in the series. In "can: rockchip_canfd: cancel
bus-off restart before closing the device", rkcanfd_stop() calls the
helper first:

	close_candev_cancel_restart_sync(ndev);
	rkcanfd_tx_disable(priv);

	rkcanfd_chip_interrupts_disable(priv);
	free_irq(ndev->irq, priv);

It then calls close_candev() after the IRQ teardown.

Could the commit message mention that user, so the helper doesn't look
unused when this patch is reviewed alone?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-rockchip-canfd-restart-close-v5-0-ae77327037c9%40qq.com