Re: [PATCH net v9 02/10] can: rockchip_canfd: serialize TX state and command writes

[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 commit message carries Cc: stable and makes the backport
  depend on three other fixes, but only describes them loosely and never…

--

can: rockchip_canfd: serialize TX state and command writes

This patch adds a tx_lock spinlock to struct rkcanfd_priv. The lock
serializes rkcanfd_start_xmit() against RX-side echo completion and the
erratum 12 retry path. It covers the TX free-space checks, echo handling,
head/tail updates, BQL accounting and the MODE/CMD/MODE command sequence.
The retained original skb is now released after the lock is dropped.

> Release the original skb retained by the preceding echo-failure fix after
> unlocking and restoring IRQs, so its socket destructor cannot run under
> tx_lock. Backports need that retained-reference handling and the CAN skb
> ownership fix, or equivalent handling.

[Severity: Low]
This isn't a bug, but since the patch carries Cc: stable and depends on
other fixes for backports, could those fixes be named by subject (and by
hash where the hash is stable)?

The "preceding echo-failure fix" looks like the previous patch in this
series:

  can: rockchip_canfd: prevent TX stall on echo skb failure

The "CAN skb ownership fix" looks like:

  b4ebcbbf66ba ("can: dev: can_put_echo_skb(): free skb on invalid echo index")

The skb_get() passed to can_put_echo_skb() in rkcanfd_start_xmit(),
together with the dev_consume_skb_any() / dev_kfree_skb_any() calls after
the unlock, only balances if can_put_echo_skb() also frees the passed
reference on the -EINVAL path. That makes b4ebcbbf66ba a hard
prerequisite for any backport.

> This lock covers running TX, completion and retry. Synchronization with
> bus-off and controller reinitialization is provided by the later
> lifecycle fix.

Which patch is "the later lifecycle fix"? It appears to be "can:
rockchip_canfd: serialize TX with bus-off and restart" later in this
series.

At this commit, rkcanfd_chip_fifo_setup(), reached via
rkcanfd_set_mode(CAN_MODE_START) -> rkcanfd_chip_start(), still resets
the TX ring and BQL state without tx_lock:

drivers/net/can/rockchip/rockchip_canfd-core.c:rkcanfd_chip_fifo_setup() {
    ...
	WRITE_ONCE(priv->tx_head, 0);
	WRITE_ONCE(priv->tx_tail, 0);
	netdev_reset_queue(priv->ndev);
}

The chip stop / can_bus_off() handling in rkcanfd_handle_state_error_int()
also runs outside the lock.

Could the follow-up be named explicitly, so a stable backport of this
patch also picks up the change that closes that window?

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