[PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

[PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

[Cunhao Lu]

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. This creates a pending TX entry without the echo
skb that the RXSTX completion path needs to match the self-received frame.
The entry cannot be completed, and the netdev TX queue can remain stopped
after the two-entry software FIFO fills.

Do not advance tx_head or request transmission if the echo skb cannot be
installed, and account the frame as dropped. Make can_put_echo_skb()
consume the skb on its remaining -EINVAL error path so that callers have
consistent skb ownership after an error.

Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v3:
- Move the -EINVAL skb free into can_put_echo_skb().
- Drop the redundant error message from rkcanfd_start_xmit().
---
 drivers/net/can/dev/skb.c                    | 1 +
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..44ebeba99837 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
 			   __func__, idx, priv->echo_skb_max);
+		kfree_skb(skb);
 		return -EINVAL;
 	}
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 12200dcfd338..b1954b72560c 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -125,8 +125,11 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	frame_len = can_skb_get_frame_len(skb);
 	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, 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);
 

-- 
2.34.1

[PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

[Cunhao Lu]

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. This creates a pending TX entry without the echo
skb that the RXSTX completion path needs to match the self-received frame.
The entry cannot be completed, and the netdev TX queue can remain stopped
after the two-entry software FIFO fills.

Do not advance tx_head or request transmission if the echo skb cannot be
installed, and account the frame as dropped. Make can_put_echo_skb()
consume the skb on its remaining -EINVAL error path so that callers have
consistent skb ownership after an error.

Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

---
Changes in v3:
- Move the -EINVAL skb free into can_put_echo_skb().
- Drop the redundant error message from rkcanfd_start_xmit().
---
 drivers/net/can/dev/skb.c                    | 1 +
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..44ebeba99837 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
 			   __func__, idx, priv->echo_skb_max);
+		kfree_skb(skb);
 		return -EINVAL;
 	}
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 12200dcfd338..b1954b72560c 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -125,8 +125,11 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	frame_len = can_skb_get_frame_len(skb);
 	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, 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);
 

-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Cunhao Lu]

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.

Fixes: a5605d61c7dd ("can: rockchip_canfd: enable full TX-FIFO depth of 2")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index b1954b72560c..2b5cd6aab31b 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);
 }

-- 
2.34.1

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

[Cunhao Lu]

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.

Fixes: a5605d61c7dd ("can: rockchip_canfd: enable full TX-FIFO depth of 2")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index b1954b72560c..2b5cd6aab31b 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);
 }

-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 3/3] can: rockchip_canfd: serialize TX state and command writes

[Cunhao Lu]

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@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@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;
 
+	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;
+
+		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;
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
 	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
 	    rkcanfd_tx_tail_is_eff(priv))
-		return 0;
+		tx_free = 0;
+	else
+		tx_free = rkcanfd_get_tx_free(priv);
+
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
-	return rkcanfd_get_tx_free(priv);
+	return tx_free;
 }
 
 static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
@@ -60,6 +71,8 @@ void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
 	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
 	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
 }
 
@@ -69,6 +82,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 reg_frameinfo, reg_id, reg_cmd;
 	unsigned int tx_head, frame_len;
 	const struct canfd_frame *cfd;
+	unsigned long flags;
 	int err;
 	u8 i;
 
@@ -124,8 +138,11 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			      *(u32 *)(cfd->data + i));
 
 	frame_len = can_skb_get_frame_len(skb);
+	spin_lock_irqsave(&priv->tx_lock, flags);
 	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
 	if (err) {
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 		ndev->stats.tx_dropped++;
 		return NETDEV_TX_OK;
 	}
@@ -134,6 +151,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
 
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
 	netif_subqueue_maybe_stop(priv->ndev, 0,
 				  rkcanfd_get_effective_tx_free(priv),
@@ -150,6 +168,8 @@ void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
 	unsigned int tx_tail;
 	struct sk_buff *skb;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	tx_tail = rkcanfd_get_tx_tail(priv);
 	skb = priv->can.echo_skb[tx_tail];
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 93131c7d7f54..3cf283a7b380 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -15,6 +15,7 @@
 #include <linux/netdevice.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
+#include <linux/spinlock.h>
 #include <linux/timecounter.h>
 #include <linux/types.h>
 #include <linux/u64_stats_sync.h>
@@ -462,6 +463,7 @@ struct rkcanfd_priv {
 	struct can_rx_offload offload;
 	struct net_device *ndev;
 
+	spinlock_t tx_lock; /* protects tx_head, tx_tail and echo_skb */
 	void __iomem *regs;
 	unsigned int tx_head;
 	unsigned int tx_tail;
@@ -544,7 +546,7 @@ void rkcanfd_timestamp_start(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop_sync(struct rkcanfd_priv *priv);
 
-unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv);
+unsigned int rkcanfd_get_effective_tx_free(struct rkcanfd_priv *priv);
 void rkcanfd_xmit_retry(struct rkcanfd_priv *priv);
 netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev);
 void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,

-- 
2.34.1

[PATCH v3 3/3] can: rockchip_canfd: serialize TX state and command writes

[Cunhao Lu]

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@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@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;
 
+	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;
+
+		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;
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
 	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
 	    rkcanfd_tx_tail_is_eff(priv))
-		return 0;
+		tx_free = 0;
+	else
+		tx_free = rkcanfd_get_tx_free(priv);
+
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
-	return rkcanfd_get_tx_free(priv);
+	return tx_free;
 }
 
 static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
@@ -60,6 +71,8 @@ void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
 	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
 	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
 }
 
@@ -69,6 +82,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 reg_frameinfo, reg_id, reg_cmd;
 	unsigned int tx_head, frame_len;
 	const struct canfd_frame *cfd;
+	unsigned long flags;
 	int err;
 	u8 i;
 
@@ -124,8 +138,11 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			      *(u32 *)(cfd->data + i));
 
 	frame_len = can_skb_get_frame_len(skb);
+	spin_lock_irqsave(&priv->tx_lock, flags);
 	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
 	if (err) {
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 		ndev->stats.tx_dropped++;
 		return NETDEV_TX_OK;
 	}
@@ -134,6 +151,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
 
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
 	netif_subqueue_maybe_stop(priv->ndev, 0,
 				  rkcanfd_get_effective_tx_free(priv),
@@ -150,6 +168,8 @@ void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
 	unsigned int tx_tail;
 	struct sk_buff *skb;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	tx_tail = rkcanfd_get_tx_tail(priv);
 	skb = priv->can.echo_skb[tx_tail];
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 93131c7d7f54..3cf283a7b380 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -15,6 +15,7 @@
 #include <linux/netdevice.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
+#include <linux/spinlock.h>
 #include <linux/timecounter.h>
 #include <linux/types.h>
 #include <linux/u64_stats_sync.h>
@@ -462,6 +463,7 @@ struct rkcanfd_priv {
 	struct can_rx_offload offload;
 	struct net_device *ndev;
 
+	spinlock_t tx_lock; /* protects tx_head, tx_tail and echo_skb */
 	void __iomem *regs;
 	unsigned int tx_head;
 	unsigned int tx_tail;
@@ -544,7 +546,7 @@ void rkcanfd_timestamp_start(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop_sync(struct rkcanfd_priv *priv);
 
-unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv);
+unsigned int rkcanfd_get_effective_tx_free(struct rkcanfd_priv *priv);
 void rkcanfd_xmit_retry(struct rkcanfd_priv *priv);
 netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev);
 void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,

-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1470 bytes --]

On 30.07.2026 21:48:38, Cunhao Lu wrote:
> rkcanfd_start_xmit() advances tx_head and requests transmission even when
> can_put_echo_skb() fails. This creates a pending TX entry without the echo
> skb that the RXSTX completion path needs to match the self-received frame.
> The entry cannot be completed, and the netdev TX queue can remain stopped
> after the two-entry software FIFO fills.
>
> Do not advance tx_head or request transmission if the echo skb cannot be
> installed, and account the frame as dropped. Make can_put_echo_skb()
> consume the skb on its remaining -EINVAL error path so that callers have
> consistent skb ownership after an error.
>
> Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cunhao Lu <1579567540@qq.com>
>
> ---
> Changes in v3:
> - Move the -EINVAL skb free into can_put_echo_skb().
> - Drop the redundant error message from rkcanfd_start_xmit().
> ---
>  drivers/net/can/dev/skb.c                    | 1 +
>  drivers/net/can/rockchip/rockchip_canfd-tx.c | 7 +++++--

Please split this and send drivers/net/can/dev/skb.c as a stand alone
patch outside of this series.

regards,
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   |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On 30.07.2026 21:48:38, Cunhao Lu wrote:
> rkcanfd_start_xmit() advances tx_head and requests transmission even when
> can_put_echo_skb() fails. This creates a pending TX entry without the echo
> skb that the RXSTX completion path needs to match the self-received frame.
> The entry cannot be completed, and the netdev TX queue can remain stopped
> after the two-entry software FIFO fills.
>
> Do not advance tx_head or request transmission if the echo skb cannot be
> installed, and account the frame as dropped. Make can_put_echo_skb()
> consume the skb on its remaining -EINVAL error path so that callers have
> consistent skb ownership after an error.
>
> Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cunhao Lu <1579567540@qq.com>
>
> ---
> Changes in v3:
> - Move the -EINVAL skb free into can_put_echo_skb().
> - Drop the redundant error message from rkcanfd_start_xmit().
> ---
>  drivers/net/can/dev/skb.c                    | 1 +
>  drivers/net/can/rockchip/rockchip_canfd-tx.c | 7 +++++--

Please split this and send drivers/net/can/dev/skb.c as a stand alone
patch outside of this series.

regards,
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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect Transmit Retry Target Index causes hardware to retry the wrong buffer, leading to a permanent TX stall.
- [High] Missing memory barriers in the lockless ring buffer logic between the TX path and RX IRQ handler allow concurrent CPUs to read a NULL `echo_skb`, stalling the TX queue.
--

--- Patch [1]: [PATCH v3 1/3] can: rockchip_canfd: prevent TX stall on echo skb failure ---
commit 39312269693a856e9d3d6b063ae673598025f7df
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip_canfd: prevent TX stall on echo skb failure

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. This patch halts tx_head advancement and
transmission request if the echo skb cannot be installed, dropping the frame
instead. It also makes can_put_echo_skb() consume the skb on its -EINVAL
error path.

Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f80..44ebeba998370 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
[ ... ]
> @@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	if (idx >= priv->echo_skb_max) {
>  		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
>  			   __func__, idx, priv->echo_skb_max);
> +		kfree_skb(skb);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 12200dcfd3389..b1954b72560cf 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
[ ... ]
> @@ -125,8 +125,11 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	frame_len = can_skb_get_frame_len(skb);
>  	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
> -	if (!err)
> -		netdev_sent_queue(priv->ndev, 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);

[Severity: High]
This isn't a bug introduced by this patch, but missing memory barriers in the
lockless ring buffer logic between the TX path and RX IRQ handler might allow
concurrent CPUs to read a NULL echo_skb.

When CPU 0 calls rkcanfd_start_xmit(), it sets echo_skb[idx] = skb inside
can_put_echo_skb() and then advances tx_head. Without an smp_wmb() or
smp_store_release() here, could an ARM CPU reorder the tx_head store to
become globally visible before the echo_skb store?

If CPU 1 processes an RX interrupt, reads the updated tx_head, and proceeds
into rkcanfd_rxstx_filter():

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_rxstx_filter() {
    ...
    tx_tail = rkcanfd_get_tx_tail(priv);
    skb = priv->can.echo_skb[tx_tail];
    if (!skb) { ... }
    ...
}

Without an smp_rmb() or smp_load_acquire(), would CPU 1 observe a NULL
pointer, triggering an error and causing the TX frame to never be completed,
permanently stopping the netdev queue?

I also noticed a separate pre-existing issue in the transmit retry path.

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the hardware
might retry the wrong buffer during an erratum recovery, leading to a
permanent TX stall.

Looking at rkcanfd_xmit_retry() in
drivers/net/can/rockchip/rockchip_canfd-tx.c:

const unsigned int tx_head = rkcanfd_get_tx_head(priv);
const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);

Does this function incorrectly use tx_head (the next free, unpopulated buffer
index) instead of tx_tail (the index of the failed frame that actually needs
retrying)?

Since rkcanfd_start_xmit() has already advanced tx_head, would the driver
request transmission for an empty buffer, leaving the correct buffer
unacknowledged and the TX queue permanently stalled?

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

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

[sashiko-bot]

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

Re: [PATCH v3 3/3] can: rockchip_canfd: serialize TX state and command writes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 7867 bytes --]

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@vger.kernel.org
> Signed-off-by: Cunhao Lu <1579567540@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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/3] can: rockchip_canfd: serialize TX state and command writes

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 7867 bytes --]

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@vger.kernel.org
> Signed-off-by: Cunhao Lu <1579567540@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   |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip