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

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_s=
kb`, 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 <[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 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: [email protected]

> 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_de=
vice *dev,
>  	if (idx >=3D priv->echo_skb_max) {
>  		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of b=
ounds (%u/max %u)\n",
>  			   __func__, idx, priv->echo_skb_max);
> +		kfree_skb(skb);
>  		return -EINVAL;
>  	}
> =20
> 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 isn't a bug introduced by this patch, but missing memory barriers in t=
he
lockless ring buffer logic between the TX path and RX IRQ handler might all=
ow
concurrent CPUs to read a NULL echo_skb.

When CPU 0 calls rkcanfd_start_xmit(), it sets echo_skb[idx] =3D 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 =3D rkcanfd_get_tx_tail(priv);
    skb =3D 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 =3D rkcanfd_get_tx_head(priv);
const u32 reg_cmd =3D RKCANFD_REG_CMD_TX_REQ(tx_head);

Does this function incorrectly use tx_head (the next free, unpopulated buff=
er
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?

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