[PATCH v4 3/3] can: rockchip_canfd: serialize TX state and command writes
Cunhao Lu
1579567540 at qq.com
Thu Jul 30 08:50:43 PDT 2026
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. 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.
rkcanfd_xmit_retry() runs 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.
The resumed softirq then issues CMD without SPACE_RX_MODE and bypasses 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 completion
and wakeup 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 v4:
- Use reverse Christmas tree order for the new local variables.
- Use the _unmasked suffix for both TX state snapshots.
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..02efc8c0cddc 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
@@ -100,14 +100,25 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
const struct canfd_frame *cfd_nominal;
const struct sk_buff *skb;
unsigned int tx_tail;
+ unsigned long flags;
+
+ 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_unmasked = priv->tx_head;
+ const unsigned int tx_tail_unmasked = 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_unmasked, tx_tail_unmasked);
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..c311b6990bc9 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 int tx_free;
+ unsigned long flags;
+
+ 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
More information about the Linux-rockchip
mailing list