From: Cunhao Lu <1579567540@qq.com> 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. Install the echo skb before loading the hardware TX buffer so an echo setup failure cannot desynchronize the hardware and software TX state. 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> Link: https://patch.msgid.link/tencent_AF224EFBC9343DD238C80824AC8CA805480A@qq.com Signed-off-by: Marc Kleine-Budde --- .../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 37c1c22c40c9..1cae86973da5 100644 --- a/drivers/net/can/rockchip/rockchip_canfd-core.c +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c @@ -921,6 +921,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 24e87daa1df0..59420233c918 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 fc338ea865fe..b367341dd0ae 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); - return rkcanfd_get_tx_free(priv); + spin_unlock_irqrestore(&priv->tx_lock, flags); + + 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; @@ -88,10 +102,13 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev) return NETDEV_TX_BUSY; } + spin_lock_irqsave(&priv->tx_lock, flags); tx_head = rkcanfd_get_tx_head(priv); frame_len = can_skb_get_frame_len(skb); 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; } @@ -136,6 +153,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), @@ -152,6 +170,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 95bea9bfd8a2..2dc3a41afe32 100644 --- a/drivers/net/can/rockchip/rockchip_canfd.h +++ b/drivers/net/can/rockchip/rockchip_canfd.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -474,6 +475,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; @@ -556,7 +558,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.53.0