Re: [PATCH net v8 03/10] can: rockchip_canfd: retry the outstanding TX buffer

[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 for "can: rockchip_canfd: retry the
  outstanding TX buffer" states the user-visible failure ("the pending…

--

can: rockchip_canfd: retry the outstanding TX buffer

This patch changes rkcanfd_xmit_retry() so the erratum 6 retransmission
uses tx_tail instead of tx_head to pick the TX buffer. The goal is to retry
the frame that is still pending rather than the next free buffer. It is
tagged for stable with a Fixes: tag pointing at a5605d61c7dd.

> Use tx_tail to retry the outstanding buffer. Otherwise, the pending echo
> entry can remain incomplete and leave the TX queue stopped.

[Severity: Low]
The tx_tail indexing looks correct. The stable justification is harder to
check, though, because the stall described here depends on how the
hardware handles TX_REQ(n), and the commit message doesn't say.

The controller has one TX frame register window (RKCANFD_REG_FD_TXFRAMEINFO,
RKCANFD_REG_FD_TXID, RKCANFD_REG_FD_TXDATA0..) and two request bits. Every
frame is written into that shared window in rkcanfd_start_xmit() before
TX_REQ(tx_head) is issued.

rkcanfd_xmit_retry() has one caller, rkcanfd_rxstx_filter(). That caller
only gets that far when the nominal frame echo_skb[tx_tail] is EFF:

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_rxstx_filter() {
    ...
	if (!(cfd_nominal->can_id & CAN_EFF_FLAG) ||
	    (cfd_rx->can_id & CAN_EFF_FLAG))
		return 0;
    ...
	rkcanfd_xmit_retry(priv);
    ...
}

While the tail frame is EFF, no further frame can be queued:

drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_get_effective_tx_free() {
    ...
	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
	    rkcanfd_tx_tail_is_eff(priv))
		return 0;
    ...
}

So whenever the retry runs, exactly one frame should be pending, and it
should be the last frame written into the shared window.

If writing TX_REQ(n) latches the current window into buffer n, wouldn't the
old TX_REQ(tx_head) have resent the correct frame? The echo would then
match echo_skb[tx_tail] and completion would go ahead normally.

As far as I can tell, the stall only happens if buffer n keeps its old
contents and TX_REQ(n) resends them without reloading from the window.

Was the stall actually seen on hardware, or is this mainly making the slot
indexing consistent with the echo_skb[tx_tail] model? The Erratum 6 comment
in rockchip_canfd.h has a canfdtest reproducer, and a5605d61c7dd carries a
Tested-by for the depth-2 change.

Could the commit message say which TX_REQ(n) behaviour the fix relies on,
or add a Tested-by or reproducer, so stable backporters can judge it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-rockchip-canfd-restart-close-v8-0-3e7a23fd9eae%40qq.com