Re: [PATCH v4 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [High] rkcanfd_xmit_retry retransmits the wrong hardware buffer (tx_head =
instead of tx_tail), causing incorrect transmissions and eventually stallin=
g the TX queue.
--

--- Patch [1]: [PATCH v4 1/3] can: rockchip_canfd: prevent TX stall on echo=
 skb failure ---
commit 10806e8e98cd5083f34122a8793d28a8a961b0e4
Author: Cunhao Lu <[email protected]>

can: rockchip_canfd: prevent TX stall on echo skb failure

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. This creates a pending TX entry without the echo
skb that the RXSTX completion path needs to match the self-received frame.
The entry cannot be completed, and the netdev TX queue can remain stopped
after the two-entry software FIFO fills.

Link: https://lore.kernel.org/linux-can/tencent_944DADCC4B42C8484EC01DA2B15=
[email protected]

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/c=
an/rockchip/rockchip_canfd-tx.c
> index 12200dcfd3389..b1954b72560cf 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c

[ ... ]

> @@ -125,8 +125,11 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, =
struct net_device *ndev)
> =20
>  	frame_len =3D can_skb_get_frame_len(skb);
>  	err =3D can_put_echo_skb(skb, ndev, tx_head, frame_len);
> -	if (!err)
> -		netdev_sent_queue(priv->ndev, frame_len);
> +	if (err) {
> +		ndev->stats.tx_dropped++;
> +		return NETDEV_TX_OK;
> +	}
> +	netdev_sent_queue(priv->ndev, frame_len);
> =20
>  	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in this same
file regarding TX stalls during retransmission.

In rkcanfd_xmit_retry(), the hardware retransmit command is issued using
tx_head instead of tx_tail:

void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
{
	const unsigned int tx_head =3D rkcanfd_get_tx_head(priv);
	const u32 reg_cmd =3D RKCANFD_REG_CMD_TX_REQ(tx_head);

	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
}

Does this retransmit the wrong hardware buffer (the next available software
slot) instead of the actual corrupted frame at tx_tail?

It looks like this would leave the failed slot unacknowledged and tx_tail
permanently unadvanced, which could eventually stall the TX queue.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260730-master-v4-=
[email protected]?part=3D1