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

[sashiko-bot@kernel.org]

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 stalling the TX queue.
--

--- Patch [1]: [PATCH v4 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure ---
commit 10806e8e98cd5083f34122a8793d28a8a961b0e4
Author: Cunhao Lu <1579567540@qq.com>

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_944DADCC4B42C8484EC01DA2B15F42132906@qq.com

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/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)
>  
>  	frame_len = can_skb_get_frame_len(skb);
>  	err = 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);
>  
>  	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 = rkcanfd_get_tx_head(priv);
	const u32 reg_cmd = 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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-master-v4-0-07f7952f761d@qq.com?part=1