Re: [PATCH net 06/14] can: rockchip_canfd: retry the outstanding TX buffer

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: rockchip_canfd: retry the outstanding TX buffer

This changes rkcanfd_xmit_retry() to use rkcanfd_get_tx_tail() instead of
rkcanfd_get_tx_head() when building the TX_REQ command, so that the
erratum 6 workaround re-requests the buffer that is actually outstanding
now that the TX FIFO depth is two.

Link: https://patch.msgid.link/tencent_2CCE005FC440BF8026599A6999EC04BE1107@qq.com

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 86fa8f2e1c8ba..fc338ea865feb 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);
> +	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
>  
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
>  }

[Severity: Medium]

This isn't a bug introduced by this patch, but with rkcanfd_xmit_retry()
now aiming at the tx_tail buffer, is the buffer it re-requests the same
one that rkcanfd_start_xmit() may be arming at that moment?

rkcanfd_xmit_retry() is reached from the hard-IRQ handler, since
rkcanfd_open() registers it non-threaded:

	err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);

and rkcanfd_irq()->rkcanfd_handle_rx_int()->rkcanfd_rxstx_filter() calls
rkcanfd_xmit_retry() for the erratum 6 case. At this commit there is no
lock covering priv->tx_head, priv->tx_tail or the register writes.

Two interleavings look possible.

First, rkcanfd_start_xmit_write_cmd() is not atomic:

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

	rkcanfd_write(priv, RKCANFD_REG_CMD, reg_cmd);

	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_12)
		rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default);

If the interrupt path runs this sequence while the transmit path is
between its own MODE and CMD writes, the restore of reg_mode_default
lands before the other context's CMD write. Does that silently drop the
erratum 12 workaround for that frame? rk3568v2 carries both
RKCANFD_QUIRK_RK3568_ERRATUM_6 and RKCANFD_QUIRK_RK3568_ERRATUM_12.

Second, rkcanfd_start_xmit() publishes the new head before issuing the
command:

	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);

	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);

If the interrupt path completes the previous frame (advancing tx_tail)
in that window, rkcanfd_xmit_retry() now selects the buffer whose CMD
write has not been issued yet. Can that produce a second TX request for
the same buffer, a duplicated frame on the wire, and a desynchronized
echo ring, with rkcanfd_tx_tail_is_eff() then hitting the

	"%s: echo_skb[%u]=NULL tx_head=0x%08x tx_tail=0x%08x\n"

path?

For what it is worth, this appears to be addressed later in the same
series by the change that adds "spinlock_t tx_lock; /* protects tx_head,
tx_tail and echo_skb */" to struct rkcanfd_priv, wraps the
rkcanfd_start_xmit() echo/register/command sequence in
spin_lock_irqsave()/spin_unlock_irqrestore(), takes the same lock in
rkcanfd_rxstx_filter() and adds lockdep_assert_held(&priv->tx_lock) to
rkcanfd_xmit_retry(). Given that ordering, would it be worth noting the
dependency in this patch so that a standalone stable backport of this
change does not land without the locking?