Re: [PATCH net v9 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" leaves out a likely bus-visible effect of the…

--

can: rockchip_canfd: retry the outstanding TX buffer

This patch changes rkcanfd_xmit_retry() to pick the TX buffer for the
erratum 6 retransmission with tx_tail instead of tx_head. With a TX FIFO
depth of two and one frame outstanding, tx_head points at the free buffer
and not at the pending one.

> 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_head to tx_tail change looks correct for the one-pending case. Does
this paragraph describe the full impact of the old code, though?

The erratum 6 retry can only run when exactly one frame is pending and
that frame is EFF. In that state rkcanfd_get_effective_tx_free() returns 0:

	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
	    rkcanfd_tx_tail_is_eff(priv))
		return 0;

So before this patch, the masked tx_head in rkcanfd_xmit_retry() always
pointed at the other hardware buffer.

The fix assumes that RKCANFD_REG_CMD_TX_REQ(i) sends whatever is already
stored in buffer i. If so, did every erratum 6 retry with the old code
re-send the stale contents of the non-pending buffer instead of the
mangled EFF frame? That would usually be an earlier SFF frame that had
already completed.

If that happened, other nodes on the bus would see a duplicate frame.
Locally, the self-received copy would not match echo_skb[tx_tail] in
rkcanfd_rxstx_filter(). It would then go to local sockets as a normal
received frame through rkcanfd_handle_rx_int_one().

If instead TX_REQ(i) latches the shared FD_TXFRAMEINFO, FD_TXID and
FD_TXDATA registers written in rkcanfd_start_xmit(), the old and new code
would both send the EFF frame. In that case the stall described above
would not happen.

The driver code alone doesn't show which of these is the real hardware
behavior. Could the commit message say which TX_REQ semantics the fix
depends on? If they apply, could it also mention the duplicate
transmission and the phantom local RX frame?

Was this reproduced or tested on RK3568 hardware? If so, a note on that
or a Tested-by tag would help.

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