Re: [PATCH net v9 08/10] can: rockchip_canfd: serialize TX with bus-off and restart

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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 adds priv->irq_suspended, a plain bool used as an
  ad-hoc gate for the IRQ handler.
- [Low] The commit message for "can: rockchip_canfd: serialize TX with
  bus-off and restart" carries Cc: stable@vger.kernel.org and Fixes:…
- [Low] rkcanfd_chip_set_reset_mode() writes RKCANFD_REG_INT_MASK =
  RKCANFD_REG_INT_ALL immediately after reset_control_deassert(), with…

--

can: rockchip_canfd: serialize TX with bus-off and restart

This patch adds a tx_enabled flag, protected by tx_lock, that stops TX
submission and RX echo handling before bus-off is published and while the
controller restarts. It also adds an irq_suspended flag that keeps the
shared IRQ handler out during reinitialization, restores INT_MASK right
after reset deassertion, and moves timestamp work initialization to probe.

> Fixes: ff60bfbaf67f ("can: rockchip_canfd: add driver for Rockchip CAN-FD controller")
> Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
> Cc: stable@vger.kernel.org

[Severity: Low]
This patch is tagged for stable. Should the commit message also list the
patches a backport depends on?

It relies on several earlier changes:

- the tx_lock critical sections in rkcanfd_start_xmit() and
  rkcanfd_handle_rx_int_one(), from "can: rockchip_canfd: serialize TX
  state and command writes"
- the close_candev_cancel_restart_sync() call in rkcanfd_stop(), from
  "can: rockchip_canfd: cancel bus-off restart before closing the device"
  and "can: dev: add close_candev_cancel_restart_sync()"
- the netif_running() check in can_restart(), from "can: dev: check
  netif_running() before bus-off recovery"
- the skb_get() echo ownership handling, from "can: rockchip_canfd:
  prevent TX stall on echo skb failure"

The commit that adds close_candev_cancel_restart_sync() has neither a Cc:
stable nor a Fixes: tag.

Earlier patches in the series state their dependencies ("Backports must
include...", "Backports need..."). This patch has the most dependencies
but lists none.

A backport that only follows the Fixes: tags would fail to apply. A version
adapted by hand would lack the serialization that the tx_enabled checks
depend on.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index d05264605efc2..a5c6331191d17 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c

[ ... ]

> @@ -116,6 +118,10 @@ static void rkcanfd_chip_set_reset_mode(const struct rkcanfd_priv *priv)
>  	udelay(2);
>  	reset_control_deassert(priv->reset);
>  
> +	/* Reset clears INT_MASK; restore it before configuring the controller. */
> +	rkcanfd_write(priv, RKCANFD_REG_INT_MASK, RKCANFD_REG_INT_ALL);
> +	rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
> +
>  	rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);
>  }

[Severity: Low]
Is the CAN core known to be out of reset when this INT_MASK write
arrives?

On Rockchip, reset_control_deassert() goes to rockchip_softrst_deassert().
That function does one posted write to the CRU, with no read-back and no
delay:

drivers/clk/rockchip/softrst.c:rockchip_softrst_deassert() {
    ...
	if (softrst->flags & ROCKCHIP_SOFTRST_HIWORD_MASK) {
		writel((BIT(offset) << 16), softrst->reg_base + (bank * 4));
    ...
}

The CRU and the CAN controller are separate peripherals. The barrier in
writel() orders when writes are issued, not when they reach the device.
So the INT_MASK write could reach the CAN IP while it is still in reset.

If that write is lost, INT_MASK stays at its reset value of 0, which
leaves every interrupt unmasked. That lasts through the FIFO, timestamp
and bittiming setup, until rkcanfd_chip_interrupts_disable() runs later
in rkcanfd_chip_start(). The read-back does not detect this because its
value is thrown away.

The commit message says:

  "Restore and flush the full mask immediately after deassertion so the
  remaining controller configuration stays masked."

On the restart path, irq_suspended is true during this window. An
interrupt raised then would get IRQ_NONE without an ack, and the shared
line would stay asserted until the next mask write.

This depends on the arrival order between the CRU and the CAN block and
on how long the reset takes to release. I could not confirm either
without the SoC documentation.

Should there be a short settle delay after deassertion, or a check of the
read-back value?

[ ... ]

> @@ -356,9 +390,17 @@ static int rkcanfd_set_mode(struct net_device *ndev,
>  
>  	switch (mode) {
>  	case CAN_MODE_START:
> +		rkcanfd_tx_disable(priv);
> +		rkcanfd_chip_interrupts_disable(priv);
> +		/* Flush the interrupt mask write before suspending the handler. */
> +		rkcanfd_read(priv, RKCANFD_REG_INT_MASK);
> +		WRITE_ONCE(priv->irq_suspended, true);
> +		/* Wait for handlers that already passed the suspended check. */
> +		synchronize_irq(ndev->irq);
> +		rkcanfd_timestamp_stop_sync(priv);
> +
>  		rkcanfd_chip_start(priv);
> -		rkcanfd_chip_interrupts_enable(priv);
> -		netif_wake_queue(ndev);
> +		rkcanfd_chip_start_finish(priv);
>  		break;

[Severity: Medium]
Is irq_suspended working as a lock here?

Two paths set it before a block of work. rkcanfd_set_mode() sets it
before the controller is reinitialized. rkcanfd_open() sets it before
rkcanfd_chip_start() and request_irq().

In both paths, rkcanfd_chip_start_finish() clears it:

	smp_store_release(&priv->irq_suspended, false);

rkcanfd_irq() checks the flag without taking a lock:

	if (smp_load_acquire(&priv->irq_suspended))
		return IRQ_NONE;

This keeps the handler away from state that rkcanfd_chip_start()
rewrites: reg_int_mask_default, reg_mode_default, bec, tx_head/tx_tail,
cc/tc and can.state.

I did not find a concrete failure in the current code. synchronize_irq()
takes desc->lock, which makes the WRITE_ONCE() visible to handlers that
start after it. The release/acquire pair makes the reinitialized state
visible.

Even so, the exclusion depends on synchronize_irq() and on the rule that
only one instance of a handler runs at a time. Lockdep cannot see either.

It also holds only as long as every path that sets the flag calls
synchronize_irq() afterwards. Other code that uses the same state, such
as the do_get_berr_counter callback rkcanfd_get_berr_counter(), is not
covered by the flag.

The networking driver guidelines treat a flag that is set on entry and
cleared on exit to block concurrent access as an ad-hoc lock. They ask
for a real lock or RCU instead. Could this use a real lock?

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