Re: [PATCH v3 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] Concurrent MMIO accesses to RKCANFD_REG_MODE from softirq and hardirq contexts without locking defeat the Erratum 12 workaround.
- [High] Lockless update of `tx_head` lacks memory barriers, risking a NULL pointer dereference when the RX IRQ handler reads `echo_skb`.
--

--- Patch [2]: [PATCH v3 2/3] can: rockchip_canfd: retry the outstanding TX buffer ---
commit c91fe3adb2c3d0a63ee8904bebefd12fa1335d29
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip_canfd: retry the outstanding TX buffer

This patch modifies rkcanfd_xmit_retry to use tx_tail instead of tx_head
to select the outstanding buffer for retransmission. This corrects an issue
introduced when the TX FIFO depth was increased to two.

> 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
> @@ -57,8 +57,8 @@ static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
>  
>  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);
> +	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);

[Severity: High]
This is a pre-existing issue, but does the lockless update of tx_head lack
memory barriers, risking a NULL pointer dereference when the RX IRQ handler
reads echo_skb?

In rkcanfd_start_xmit(), the CPU populates priv->can.echo_skb[tx_head]
locklessly via can_put_echo_skb() and then increments tx_head:

	err = can_put_echo_skb(skb, ndev, tx_head, 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);

Because there is no memory barrier separating these operations,
weakly-ordered architectures can reorder the store to tx_head to become
visible before the store to echo_skb.

A concurrent hardware interrupt on another CPU executing
rkcanfd_handle_rx_int_one() could read the updated tx_head and attempt to
read echo_skb[tx_tail] in rkcanfd_rxstx_filter():

	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 -ENOMSG;

Lacking a paired acquire barrier, it may read a NULL pointer.
This would terminate the RX FIFO processing loop prematurely,
leaving frames in the hardware FIFO. Should there be memory barriers ensuring
the echo_skb store completes before tx_head is incremented?

> +	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
>  
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
rkcanfd_start_xmit_write_cmd() from both softirq and hardirq contexts
without locking defeat the Erratum 12 workaround?

The rkcanfd_start_xmit() function running in NET_TX_SOFTIRQ context calls
rkcanfd_start_xmit_write_cmd() to execute a non-atomic read-modify-write
sequence on RKCANFD_REG_MODE:

static void rkcanfd_start_xmit_write_cmd(...)
{
	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_12)
		rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default |
			      RKCANFD_REG_MODE_SPACE_RX_MODE);

If a hardware interrupt occurs immediately after the softirq sets
RKCANFD_REG_MODE_SPACE_RX_MODE, the hardirq handler rkcanfd_irq() can run.
It can call rkcanfd_xmit_retry(), which also executes
rkcanfd_start_xmit_write_cmd(), restoring RKCANFD_REG_MODE to
priv->reg_mode_default on exit.

When the softirq resumes, it proceeds to write to RKCANFD_REG_CMD but
RKCANFD_REG_MODE is now back in its default state, meaning the required
Erratum 12 workaround is missing. Could this silent bypass cause incorrect
CAN bus behavior or TX stalls?

>  }

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