Re: [PATCH v4 2/3] can: rockchip_canfd: retry the outstanding TX buffer

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

Pre-existing issues:
- [High] The hardware is not configured for strict FIFO transmission order,=
 which breaks the RXSTX (self-reception) completion logic and can permanent=
ly stall the TX queue.
- [High] Dereferencing the SKB in `rkcanfd_tx_tail_is_eff()` without protec=
tion leads to a Use-After-Free on SMP systems.
--

--- Patch [2]: [PATCH v4 2/3] can: rockchip_canfd: retry the outstanding TX=
 buffer ---
commit ef53444fe266d2fe80406b3bc81b10580a7299c4
Author: Cunhao Lu <[email protected]>

can: rockchip_canfd: retry the outstanding TX buffer

rkcanfd_xmit_retry() originally operated with a TX FIFO depth of one. At
that depth, the masked head and tail indices both select buffer 0, so using
tx_head happened to select the correct buffer.

After the FIFO depth was increased to two, tx_head instead identifies the
next free buffer when one frame is outstanding. The erratum 6 workaround
therefore requests transmission from the wrong buffer, leaving the
outstanding echo entry incomplete and the netdev TX queue stopped.

Use tx_tail to select the outstanding buffer for retransmission.

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

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the hardware configuration here brea=
k the TX completion logic?

drivers/net/can/rockchip/rockchip_canfd-core.c:rkcanfd_chip_start() {
...
	priv->reg_mode_default =3D reg |
		RKCANFD_REG_MODE_CAN_FD_MODE_ENABLE |
		RKCANFD_REG_MODE_AUTO_RETX_MODE |
		RKCANFD_REG_MODE_COVER_MODE |
		RKCANFD_REG_MODE_RXSTX_MODE |
		RKCANFD_REG_MODE_WORK_MODE;
...
}

Without RKCANFD_REG_MODE_TXORDER_MODE to enforce strict FIFO order, could t=
he controller arbitrate pending TX buffers based on CAN ID priority instead=
?=20

If a higher-priority frame is queued after a lower-priority frame when TXFI=
FO_DEPTH > 1, the hardware would send the higher-priority frame first.=20

Since the self-reception handler (rkcanfd_rxstx_filter) assumes strict FIFO=
 order and compares the received echo against the oldest pending TX frame, =
would this out-of-order transmission fail the comparison? This seems like i=
t would leave tx_tail unadvanced and eventually stall the TX queue permanen=
tly.

[Severity: High]
This isn't a bug introduced by this patch, but can dereferencing the skb he=
re cause a use-after-free on SMP systems?

drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_tx_tail_is_eff() {
...
	tx_tail =3D rkcanfd_get_tx_tail(priv);
	skb =3D priv->can.echo_skb[tx_tail];
	if (!skb) {
		netdev_err(priv->ndev,
			   "%s: echo_skb[%u]=3DNULL tx_head=3D0x%08x tx_tail=3D0x%08x\n",
			   __func__, tx_tail,
			   priv->tx_head, priv->tx_tail);

		return false;
	}

	cfd =3D (struct canfd_frame *)skb->data;

	return cfd->can_id & CAN_EFF_FLAG;
...
}

Since this is called from ndo_start_xmit without disabling hardware IRQs, a=
 TX completion interrupt could fire on another CPU immediately after the sk=
b pointer is fetched.=20

The hard IRQ handler would then set priv->can.echo_skb[tx_tail] to NULL and=
 queue the skb to NAPI, which could consume and free the skb on the remote =
CPU before the original CPU dereferences skb->data.

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