[PATCH v3 3/3] can: rockchip_canfd: serialize TX state and command writes
Marc Kleine-Budde
mkl at pengutronix.de
Thu Jul 30 07:09:50 PDT 2026
On 30.07.2026 21:48:40, Cunhao Lu wrote:
> The TX completion path removes an echo skb before advancing tx_tail. In
> parallel, the transmit path reads tx_head, tx_tail and the tail echo slot
> when applying the erratum 6 queue restriction. There is no synchronization
> between these operations.
>
> On SMP, the transmit path can consequently observe a pending frame with
> an empty echo slot:
>
> rockchip_canfd fea60000.can can0: rkcanfd_tx_tail_is_eff:
> echo_skb[0]=NULL tx_head=0x00060f7d tx_tail=0x00060f7c
>
> It can also keep a pointer to an echo skb while the completion path removes
> and queues it for NAPI, where it can be freed on another CPU. The
> inconsistent snapshot can stop the netdev TX queue when there is no later
> completion to wake it.
>
> There is a second race in the command submission sequence.
> rkcanfd_start_xmit() runs in softirq context while rkcanfd_xmit_retry() is
> called from the RX interrupt handler. On controllers affected by erratum
> 12, both execute a MODE/CMD/MODE register sequence. The interrupt handler
> can restore the default MODE between the softirq writes, causing the
> resumed softirq to issue CMD without SPACE_RX_MODE and bypass the erratum
> 12 workaround.
>
> Add a TX state lock and use it to protect tx_head, tx_tail and the echo skb
> ring as one state. The same lock serializes the MODE/CMD/MODE sequence
> between the transmit and interrupt paths. Keep the queue completion and
> wake operation outside the lock to avoid nesting the TX state lock with
> the netdev TX queue lock.
>
> Tested on an RK3588 rev2.2 at 1 Mbit/s with 100,000 extended CAN frames.
> The run triggered 138 erratum 6 retries and completed without drops, queue
> stalls or driver warnings. RK3588 does not enable erratum 12, so this test
> does not exercise that hardware workaround.
>
> Fixes: ae002cc32ec4 ("can: rockchip_canfd: prepare to use full TX-FIFO depth")
> Fixes: 83f9bd6bf39d ("can: rockchip_canfd: implement workaround for erratum 12")
> Cc: stable at vger.kernel.org
> Signed-off-by: Cunhao Lu <1579567540 at qq.com>
>
> ---
> Changes in v3:
> - Adjust for the can_put_echo_skb() ownership change in patch 1; no
> functional changes.
>
> Changes in v2:
> - Serialize the erratum 12 MODE/CMD/MODE sequence with the TX lock.
> - Add the erratum 12 commit to the Fixes tags.
> - Add RK3588 extended-frame stress-test results.
> ---
> drivers/net/can/rockchip/rockchip_canfd-core.c | 1 +
> drivers/net/can/rockchip/rockchip_canfd-rx.c | 31 +++++++++++++++++++-------
> drivers/net/can/rockchip/rockchip_canfd-tx.c | 26 ++++++++++++++++++---
> drivers/net/can/rockchip/rockchip_canfd.h | 4 +++-
> 4 files changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 29de0c01e4ed..c8257858b452 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -904,6 +904,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
> priv->can.do_set_mode = rkcanfd_set_mode;
> priv->can.do_get_berr_counter = rkcanfd_get_berr_counter;
> priv->ndev = ndev;
> + spin_lock_init(&priv->tx_lock);
>
> match = device_get_match_data(&pdev->dev);
> if (match) {
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> index 475c0409e215..9f57a35672ed 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> @@ -99,15 +99,26 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
> struct rkcanfd_stats *rkcanfd_stats = &priv->stats;
> const struct canfd_frame *cfd_nominal;
> const struct sk_buff *skb;
> + unsigned long flags;
> unsigned int tx_tail;
nitpick: please keep reverse xmas tree for variable declaration, move
flags to the end.
>
> + spin_lock_irqsave(&priv->tx_lock, flags);
> +
> + if (!rkcanfd_get_tx_pending(priv))
> + goto out_unlock;
> +
> tx_tail = rkcanfd_get_tx_tail(priv);
> skb = priv->can.echo_skb[tx_tail];
> if (!skb) {
> + const unsigned int tx_head = priv->tx_head;
> + const unsigned int tx_tail_full = priv->tx_tail;
nitpick: As it's both the unmasked head and tail, please use the same
suffix for both.
> +
> + spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> 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);
> + tx_head, tx_tail_full);
>
> return -ENOMSG;
> }
> @@ -123,17 +134,18 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
> rkcanfd_handle_tx_done_one(priv, ts, &frame_len);
>
> WRITE_ONCE(priv->tx_tail, priv->tx_tail + 1);
> + *tx_done = true;
> +
> + spin_unlock_irqrestore(&priv->tx_lock, flags);
> netif_subqueue_completed_wake(priv->ndev, 0, 1, frame_len,
> rkcanfd_get_effective_tx_free(priv),
> RKCANFD_TX_START_THRESHOLD);
>
> - *tx_done = true;
> -
> return 0;
> }
>
> if (!(priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6))
> - return 0;
> + goto out_unlock;
>
> /* Erratum 6: Extended frames may be send as standard frames.
> *
> @@ -143,7 +155,7 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
> */
> if (!(cfd_nominal->can_id & CAN_EFF_FLAG) ||
> (cfd_rx->can_id & CAN_EFF_FLAG))
> - return 0;
> + goto out_unlock;
>
> /* Not affected if:
> * - standard part and RTR flag of the TX'ed frame
> @@ -151,20 +163,20 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
> */
> if ((cfd_nominal->can_id & (CAN_RTR_FLAG | CAN_SFF_MASK)) !=
> (cfd_rx->can_id & (CAN_RTR_FLAG | CAN_SFF_MASK)))
> - return 0;
> + goto out_unlock;
>
> /* Not affected if:
> * - length is not the same
> */
> if (cfd_nominal->len != cfd_rx->len)
> - return 0;
> + goto out_unlock;
>
> /* Not affected if:
> * - the data of non RTR frames is different
> */
> if (!(cfd_nominal->can_id & CAN_RTR_FLAG) &&
> memcmp(cfd_nominal->data, cfd_rx->data, cfd_nominal->len))
> - return 0;
> + goto out_unlock;
>
> /* Affected by Erratum 6 */
> u64_stats_update_begin(&rkcanfd_stats->syncp);
> @@ -185,6 +197,9 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
>
> rkcanfd_xmit_retry(priv);
>
> +out_unlock:
> + spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> return 0;
> }
>
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 2b5cd6aab31b..ff9d8b77499c 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> @@ -14,6 +14,8 @@ static bool rkcanfd_tx_tail_is_eff(const struct rkcanfd_priv *priv)
> const struct sk_buff *skb;
> unsigned int tx_tail;
>
> + lockdep_assert_held(&priv->tx_lock);
> +
> if (!rkcanfd_get_tx_pending(priv))
> return false;
>
> @@ -33,13 +35,22 @@ static bool rkcanfd_tx_tail_is_eff(const struct rkcanfd_priv *priv)
> return cfd->can_id & CAN_EFF_FLAG;
> }
>
> -unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv)
> +unsigned int rkcanfd_get_effective_tx_free(struct rkcanfd_priv *priv)
> {
> + unsigned long flags;
> + unsigned int tx_free;
nitpick: please use reverse xmas tree for the variables, move tx_free
first.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rockchip/attachments/20260730/6df14c70/attachment.sig>
More information about the Linux-rockchip
mailing list