Re: [PATCH net-next 2/4] net: airoha: recover RX ring after hw completion race

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

Hi Vitaliy,

this patch targets net-next tree that is closed at the moment for the merge
window.

> On AN7583, hardware can write a completed RX descriptor past the
> software-posted boundary (q->head) before airoha_qdma_fill_rx_queue()
> has actually posted a fresh buffer there, as if hardware advances its
> own completion pointer independently of software's posting
> bookkeeping. Since airoha_qdma_rx_process() consumes the ring strictly
> sequentially starting at q->tail, the consumer stalls forever waiting
> on a descriptor that hardware never writes, even though real,
> completed frames are sitting further along in the ring. In practice
> this is reachable during PPPoE/DHCP negotiation bursts on the small
> shared "force to CPU" ring, and the interface silently stops receiving
> on it.

I guess we should spend more time understanding if we can avoid the problem
instead of adding a recover mechanism.
IIUC the practical problem stems from the small size of this RX queue where
the airoha_qdma_fill_rx_queue() processing overwrites the RX descriptor DONE
bit written by the hw. Is my understanding correct? If so:
- Have you tried to just increase the queue size to check if the issue still
  occurs?
- Can we check QDMA_DESC_DONE_MASK bit in airoha_qdma_fill_rx_queue() and in
  some way bail out if it is still set? (we should probably need to clear it in
  airoha_qdma_rx_process()).
- Is QDMA_DESC_DROP_MASK bit set when the issue occurs?

> 
> Detect this without inspecting ring/descriptor memory content at all:
> REG_RX_DMA_IDX is hardware's own completion counter, independent of
> what has or hasn't been posted. If it keeps advancing across polls
> while the software consumer (tail) does not move, hardware is making
> progress the consumer can never observe - the ring is stuck. Idle
> rings, where hardware isn't advancing either, are correctly left
> alone. An earlier version of this recovery instead scanned ahead in
> the ring for a DONE descriptor and trusted its content; that caused a
> real OOM panic once it wandered into genuinely uninitialized DMA
> memory that coincidentally had the DONE bit set. Comparing a hardware
> register cannot misfire that way.
> 
> Once a stall is confirmed, defer to a work item (register access here
> can sleep) that disables RX DMA, waits for it to actually go idle,
> resyncs the ring via the existing cleanup_rx_queue()/fill_rx_queue()
> pair - which only ever touches the software-owned [tail, head) window
> and rewrites both RX_CPU_IDX and RX_DMA_IDX from it - and re-enables
> RX DMA. This deliberately drops whatever was in flight on the ring
> rather than trying to identify and preserve the specific descriptor
> hardware used; a prior attempt at the latter caused a page_pool
> double-free when the assumptions about which page was safe to free
> turned out not to hold in all cases.
> 
> GLOBAL_CFG_RX_DMA_EN_MASK in REG_QDMA_GLOBAL_CFG is per-QDMA-instance,
> not per-ring, so recovering one ring briefly pauses RX DMA on every
> ring behind that QDMA (bounded by the 50ms busy-wait below). No
> per-ring equivalent exists in the register map; this is the same bit
> airoha_qdma_start()/stop() already use for whole-device up/down. Log
> the actual measured duration of that pause alongside the recovery
> message, rather than just citing the 50ms read_poll_timeout() upper
> bound: that's the real cost paid by every other ring on the same QDMA
> instance each time recovery fires, and it's worth having the real
> number instead of the theoretical ceiling.

We should try to avoid it

> 
> One cost worth calling out explicitly: airoha_qdma_rx_check_stall()
> adds an uncached MMIO read of RX_DMA_IDX on the RX path, once per
> airoha_qdma_rx_process() call that ends via the non-DONE break - i.e.
> essentially every poll, on every ring, even though the condition it
> detects is rare and specific to one ring. Gating the read on whether
> the *previous* poll for this ring also reaped zero descriptors would
> keep it off rings that are actively receiving, and I traced that
> through for correctness: once the race actually happens, q->tail
> freezes permanently (consumption here is strictly sequential), so
> `done` is 0 on every poll after that point, not just some - the gate
> would only cost about one extra poll before AIROHA_RX_STALL_THRESHOLD
> is reached, not a suppression or false-negative risk. I haven't
> implemented that gating here for lack of profiling data justifying the
> added per-queue state against the (also unmeasured) cost of the
> current unconditional read; happy to add it if it turns out to matter
> in practice.
> 
> Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
> Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 135 ++++++++++++++++++++++-
>  drivers/net/ethernet/airoha/airoha_eth.h |  16 +++
>  2 files changed, 150 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index a3e5aaeb75b3..b53fe5b17653 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -3,12 +3,14 @@
>   * Copyright (c) 2024 AIROHA Inc
>   * Author: Lorenzo Bianconi <lorenzo@kernel.org>
>   */
> +#include <linux/ktime.h>
>  #include <linux/of.h>
>  #include <linux/of_net.h>
>  #include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/tcp.h>
>  #include <linux/u64_stats_sync.h>
> +#include <linux/workqueue.h>
>  #include <net/dst_metadata.h>
>  #include <net/page_pool/helpers.h>
>  #include <net/pkt_cls.h>
> @@ -657,6 +659,47 @@ airoha_qdma_get_gdm_dev(struct airoha_eth *eth, struct airoha_qdma_desc *desc)
>  	return port->devs[d] ? port->devs[d] : ERR_PTR(-ENODEV);
>  }
>  
> +/* number of consecutive polls where hw completion (RX_DMA_IDX) advances
> + * while the sw consumer (tail) doesn't, before declaring the ring stuck
> + */
> +#define AIROHA_RX_STALL_THRESHOLD	3
> +
> +/* Detect an RX ring where hw's own completion pointer (RX_DMA_IDX) keeps
> + * moving while the sw consumer (q->tail) doesn't - i.e. hw has written
> + * further descriptors somewhere in the ring, but the strictly sequential
> + * consumer can never reach them because the one at q->head, which it is
> + * waiting on, was never marked DONE. This happens when hw writes a
> + * completed descriptor past q->head before airoha_qdma_fill_rx_queue()
> + * has posted a fresh buffer there, decoupled from sw's own posting
> + * bookkeeping.
> + *
> + * Deliberately does not inspect ring/descriptor memory content to detect
> + * this: an earlier version scanned ahead for a DONE descriptor and trusted
> + * its content, which caused a real OOM panic after it wandered into
> + * genuinely uninitialized DMA memory that coincidentally had the DONE bit
> + * set. RX_DMA_IDX is a hw register with a well-defined value regardless of
> + * ring content, so this can't misfire on garbage memory, and idle rings
> + * (no hw progress either) are naturally left alone.
> + */
> +static void airoha_qdma_rx_check_stall(struct airoha_queue *q)
> +{
> +	struct airoha_qdma *qdma = q->qdma;
> +	int qid = q - &qdma->q_rx[0];
> +	u32 dma_idx = airoha_qdma_get(qdma, REG_RX_DMA_IDX(qid),
> +				      RX_RING_DMA_IDX_MASK);

Can you please respect the RCT code style?

> +
> +	if (q->stall_tail == q->tail && dma_idx != q->stall_dma_idx) {
> +		if (++q->stall_count >= AIROHA_RX_STALL_THRESHOLD &&
> +		    !test_and_set_bit(qid, qdma->rx_recover_mask))
> +			schedule_work(&qdma->rx_recover_work);
> +	} else {
> +		q->stall_count = 0;
> +	}
> +
> +	q->stall_tail = q->tail;
> +	q->stall_dma_idx = dma_idx;
> +}
> +
>  static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  {
>  	enum dma_data_direction dir = page_pool_get_dma_dir(q->page_pool);
> @@ -675,8 +718,10 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  		struct page *page;
>  
>  		desc_ctrl = le32_to_cpu(READ_ONCE(desc->ctrl));
> -		if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
> +		if (!(desc_ctrl & QDMA_DESC_DONE_MASK)) {
> +			airoha_qdma_rx_check_stall(q);
>  			break;
> +		}
>  
>  		dma_rmb();
>  
> @@ -894,6 +939,75 @@ static void airoha_qdma_cleanup_rx_queue(struct airoha_queue *q)
>  			FIELD_PREP(RX_RING_DMA_IDX_MASK, q->tail));
>  }
>  
> +static void airoha_qdma_rx_recover_work(struct work_struct *work)
> +{
> +	struct airoha_qdma *qdma = container_of(work, struct airoha_qdma,
> +						 rx_recover_work);
> +	int qid;
> +
> +	for_each_set_bit(qid, qdma->rx_recover_mask, AIROHA_NUM_RX_RING) {
> +		struct airoha_queue *q = &qdma->q_rx[qid];
> +		ktime_t rx_dma_off_ts;
> +		s64 rx_dma_off_us;
> +		u32 status;
> +
> +		if (!q->ndesc)
> +			goto next;

Can this really occurs?

> +
> +		napi_disable(&q->napi);
> +
> +		/* GLOBAL_CFG_RX_DMA_EN_MASK is per-QDMA, not per-ring, so
> +		 * this pauses every RX ring on this QDMA instance, not just
> +		 * the stalled one - track how long for, since that's the
> +		 * real-world cost of recovery on unrelated rings.
> +		 */
> +		rx_dma_off_ts = ktime_get();
> +
> +		airoha_qdma_clear(qdma, REG_QDMA_GLOBAL_CFG,
> +				  GLOBAL_CFG_RX_DMA_EN_MASK);
> +		if (read_poll_timeout(airoha_qdma_rr, status,
> +				      !(status & GLOBAL_CFG_RX_DMA_BUSY_MASK),
> +				      USEC_PER_MSEC, 50 * USEC_PER_MSEC, true,
> +				      qdma, REG_QDMA_GLOBAL_CFG))
> +			dev_warn(qdma->eth->dev,
> +				 "qid=%d RX DMA busy timeout during recovery\n",
> +				 qid);
> +
> +		/* Drop whatever is currently in flight on this ring and
> +		 * re-arm it from a known-clean state. cleanup_rx_queue()
> +		 * only ever touches the sw-owned [tail, head) window and
> +		 * resyncs both RX_CPU_IDX and RX_DMA_IDX to it, which is
> +		 * what un-wedges a ring where hw wrote past the sw head
> +		 * without the consumer ever advancing - no need to figure
> +		 * out which descriptor hw actually used.
> +		 */
> +		airoha_qdma_cleanup_rx_queue(q);
> +		if (q->skb) {
> +			/* discard whatever scatter-gather frame was
> +			 * mid-assembly when the stall was hit, cleanup_rx_queue()
> +			 * above only resyncs the ring, not this
> +			 */
> +			dev_kfree_skb(q->skb);
> +			q->skb = NULL;
> +		}
> +		airoha_qdma_fill_rx_queue(q);
> +
> +		airoha_qdma_set(qdma, REG_QDMA_GLOBAL_CFG,
> +				GLOBAL_CFG_RX_DMA_EN_MASK);
> +		rx_dma_off_us = ktime_us_delta(ktime_get(), rx_dma_off_ts);

Is this delta time really meaningful or is it just for debugging?

> +
> +		q->stall_count = 0;
> +		napi_enable(&q->napi);
> +		napi_schedule(&q->napi);
> +
> +		dev_warn_ratelimited(qdma->eth->dev,
> +				     "qid=%d RX ring recovered after hw stall (RX DMA paused for %lld us on this QDMA instance)\n",
> +				     qid, rx_dma_off_us);
> +next:
> +		clear_bit(qid, qdma->rx_recover_mask);
> +	}
> +}
> +
>  static int airoha_qdma_init_rx(struct airoha_qdma *qdma)
>  {
>  	int i;
> @@ -1594,6 +1708,8 @@ static void airoha_qdma_cleanup(struct airoha_eth *eth,
>  {
>  	int i;
>  
> +	cancel_work_sync(&qdma->rx_recover_work);
> +
>  	if (test_bit(DEV_STATE_INITIALIZED, &eth->state)) {
>  		u32 status;
>  
> @@ -1651,6 +1767,15 @@ static int airoha_hw_init(struct platform_device *pdev,
>  	if (err)
>  		return err;
>  
> +	/* INIT_WORK() every instance up front, before any of them can fail
> +	 * init and jump to the error path below, since that path tears down
> +	 * every eth->qdma[] slot unconditionally, including ones this loop
> +	 * never reached.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(eth->qdma); i++)
> +		INIT_WORK(&eth->qdma[i].rx_recover_work,
> +			  airoha_qdma_rx_recover_work);
> +
>  	for (i = 0; i < ARRAY_SIZE(eth->qdma); i++) {
>  		err = airoha_qdma_init(pdev, eth, &eth->qdma[i]);
>  		if (err)
> @@ -1699,6 +1824,14 @@ static void airoha_qdma_stop_napi(struct airoha_qdma *qdma)
>  {
>  	int i;
>  
> +	/* Make sure rx_recover_work is neither running nor able to re-arm
> +	 * before any napi_disable() below: it also calls napi_disable()/
> +	 * napi_enable() on q_rx[].napi, and napi_disable() on an
> +	 * already-disabled NAPI spins in napi_disable_locked() forever,
> +	 * since only napi_enable() clears the state it waits on.
> +	 */

I guess we can drop this comment. AI driven?

> +	disable_work_sync(&qdma->rx_recover_work);
> +
>  	for (i = 0; i < ARRAY_SIZE(qdma->q_tx_irq); i++)
>  		napi_disable(&qdma->q_tx_irq[i].napi);
>  
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index fa9a8edce22f..483d6b59c351 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -207,6 +207,15 @@ struct airoha_queue {
>  	bool txq_stopped;
>  	bool flushing;
>  
> +	/* RX hw stall detection: last REG_RX_DMA_IDX/tail snapshot taken
> +	 * whenever the head-of-line descriptor isn't DONE, and how many
> +	 * consecutive times hw made progress (DMA_IDX moved) while the
> +	 * consumer (tail) didn't. See airoha_qdma_rx_check_stall().
> +	 */
> +	u32 stall_dma_idx;
> +	u16 stall_tail;
> +	u8 stall_count;
> +
>  	struct napi_struct napi;
>  	struct page_pool *page_pool;
>  	struct sk_buff *skb;
> @@ -567,6 +576,13 @@ struct airoha_qdma {
>  	struct airoha_queue q_tx[AIROHA_NUM_TX_RING];
>  	struct airoha_queue q_rx[AIROHA_NUM_RX_RING];
>  
> +	/* recovery for RX rings whose hw completion pointer (RX_DMA_IDX)
> +	 * keeps moving while the sw consumer is stuck; see
> +	 * airoha_qdma_rx_check_stall() and airoha_qdma_rx_recover_work().
> +	 */
> +	struct work_struct rx_recover_work;
> +	DECLARE_BITMAP(rx_recover_mask, AIROHA_NUM_RX_RING);
> +
>  	DECLARE_BITMAP(qos_channel_map, AIROHA_NUM_QOS_CHANNELS);
>  };
>  
> -- 
> 2.55.0
> 

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