Re: [PATCH v3 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect Transmit Retry Target Index causes hardware to retry the wrong buffer, leading to a permanent TX stall.
- [High] Missing memory barriers in the lockless ring buffer logic between the TX path and RX IRQ handler allow concurrent CPUs to read a NULL `echo_skb`, stalling the TX queue.
--

--- Patch [1]: [PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure ---
commit 39312269693a856e9d3d6b063ae673598025f7df
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 patch halts tx_head advancement and
transmission request if the echo skb cannot be installed, dropping the frame
instead. It also makes can_put_echo_skb() consume the skb on its -EINVAL
error path.

Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f80..44ebeba998370 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
[ ... ]
> @@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	if (idx >= priv->echo_skb_max) {
>  		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
>  			   __func__, idx, priv->echo_skb_max);
> +		kfree_skb(skb);
>  		return -EINVAL;
>  	}
>  
> 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 isn't a bug introduced by this patch, but missing memory barriers in the
lockless ring buffer logic between the TX path and RX IRQ handler might allow
concurrent CPUs to read a NULL echo_skb.

When CPU 0 calls rkcanfd_start_xmit(), it sets echo_skb[idx] = skb inside
can_put_echo_skb() and then advances tx_head. Without an smp_wmb() or
smp_store_release() here, could an ARM CPU reorder the tx_head store to
become globally visible before the echo_skb store?

If CPU 1 processes an RX interrupt, reads the updated tx_head, and proceeds
into rkcanfd_rxstx_filter():

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_rxstx_filter() {
    ...
    tx_tail = rkcanfd_get_tx_tail(priv);
    skb = priv->can.echo_skb[tx_tail];
    if (!skb) { ... }
    ...
}

Without an smp_rmb() or smp_load_acquire(), would CPU 1 observe a NULL
pointer, triggering an error and causing the TX frame to never be completed,
permanently stopping the netdev queue?

I also noticed a separate pre-existing issue in the transmit retry path.

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the hardware
might retry the wrong buffer during an erratum recovery, leading to a
permanent TX stall.

Looking at rkcanfd_xmit_retry() in
drivers/net/can/rockchip/rockchip_canfd-tx.c:

const unsigned int tx_head = rkcanfd_get_tx_head(priv);
const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);

Does this function incorrectly use tx_head (the next free, unpopulated buffer
index) instead of tx_tail (the index of the failed frame that actually needs
retrying)?

Since rkcanfd_start_xmit() has already advanced tx_head, would the driver
request transmission for an empty buffer, leaving the correct buffer
unacknowledged and the TX queue permanently stalled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-master-v3-0-91cf030c337d@qq.com?part=1