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

[sashiko-bot@kernel.org]

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 permanently stall the TX queue.
- [High] Dereferencing the SKB in `rkcanfd_tx_tail_is_eff()` without protection 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 <1579567540@qq.com>

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/can/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 break the TX completion logic?

drivers/net/can/rockchip/rockchip_canfd-core.c:rkcanfd_chip_start() {
...
	priv->reg_mode_default = 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 the controller arbitrate pending TX buffers based on CAN ID priority instead? 

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

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 it would leave tx_tail unadvanced and eventually stall the TX queue permanently.

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

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

		return false;
	}

	cfd = (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 skb pointer is fetched. 

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.

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