[PATCH 0/4] net: airoha: fix silent RX packet loss on ring 4

[PATCH 0/4] net: airoha: fix silent RX packet loss on ring 4

[Vitaliy Sochnev]

This series fixes silent RX packet loss on the airoha_eth driver that
shows up as PPPoE/DHCP negotiation randomly failing to complete
(reported and reproduced on Nokia XG-040G-MF, AN7583). Two independent
causes were found on the same ring - ring 4, the shared "force to CPU"
ring that several unrelated control protocols get routed onto - so
they're posted together for context even though patch 1 targets net
and patches 2-4 target net-next:

  1/4 (net):      RX_NO_CPU_DSCP interrupt was unmasked but never
                   handled, so a ring drained to zero descriptors
                   never got NAPI rescheduled to refill it. Plain bug
                   fix, no new behavior beyond "stuck ring recovers".

  2/4 (net-next): a second, independent cause: hardware can complete
                   a descriptor past the software-posted boundary
                   before it's been reposted, wedging the strictly
                   sequential consumer forever even though real frames
                   are sitting further along in the ring. Detected via
                   comparing hw's own RX_DMA_IDX against the sw
                   consumer across polls, recovered via a deferred
                   ring resync. Marked net-next rather than net
                   because I can't currently offer a fresh
                   reproduction to back it up - see the patch itself
                   for what evidence it does rest on.

  3/4 (net-next): new ethtool -S counter for observing when 2/4's
                   recovery fires. New ABI, split out from the fix
                   itself, no correctness impact either way.

  4/4 (net-next): grow ring 4 from 16 to 128 descriptors, since the
                   race in 2/4 is correlated with this specific ring's
                   idle-to-first-frame burst pattern. On its own this
                   doesn't fix anything - it exists to make the race
                   in 2/4 harder to hit - so it only makes sense
                   alongside 2/4, hence net-next timing here too.

Some background on how these were found and tested, and answers to a
couple of questions I expect to come up, since they already did during
downstream review:

- All four were developed, build- and stress-tested against a 6.18
  base as part of an OpenWrt PR (openwrt/openwrt#24872) before being
  rebased here against current net/net-next. 1/4 and 4/4 have real
  hardware evidence: 1/4 fixes a reproduced-on-hardware bug tracked at
  the issue linked in its commit; 4/4's 128-descriptor ring survived
  500+ forced PPPoE/DHCP reconnect cycles (~20h+) on real hardware with
  zero drops, both with and without 2/4 present.

- 2/4's race itself did not reproduce during ~57 combined hours of
  fresh stress-testing this round (with and without the fix present,
  same test methodology as above) - its rx_stall_recover counter (3/4)
  never incremented once. The case for 2/4 rests on earlier, less
  controlled field/downstream testing that did point at this exact
  race, not on a fresh trigger from this round. I'm not confident
  enough in that to call it a `net` fix, hence net-next.

- 2/4 and 1/4 each carry an open question in their commit messages
  (an MMIO-read cost on the RX hot path for 2/4, an interrupt-storm
  question for 1/4) that came up during the downstream review -
  answered inline there rather than left for a first pass here.

Vitaliy Sochnev (4):
  net: airoha: handle RX_NO_CPU_DSCP interrupt, not just RX_DONE
  net: airoha: recover RX ring after hw completion race
  net: airoha: add rx_stall_recover ethtool counter
  net: airoha: grow RX ring 4 to 128 descriptors

 drivers/net/ethernet/airoha/airoha_eth.c | 207 ++++++++++++++++++++++-
 drivers/net/ethernet/airoha/airoha_eth.h |  22 +++
 2 files changed, 223 insertions(+), 6 deletions(-)

-- 
2.55.0

[PATCH net 1/4] net: airoha: handle RX_NO_CPU_DSCP interrupt, not just RX_DONE

[Vitaliy Sochnev]

The QDMA hardware raises a dedicated interrupt (NO_CPU_DSCP, one bit
per ring in QDMA_CSR_INT_ENABLE2/3) when an RX ring runs out of free
CPU descriptors. airoha_qdma_hw_init() already unmasks this interrupt
for every ring (INT_RX1_MASK()/INT_RX2_MASK() OR it together with the
RX_DONE bits before writing the enable register), but
airoha_irq_handler() only ever extracts the RX_DONE bits from the same
status word - the NO_CPU_DSCP bits are read and acknowledged (cleared)
along with everything else at the top of the handler, then silently
dropped.

This matters because once a ring is genuinely drained to zero posted
descriptors, no further RX_DONE interrupt can fire for it: hardware
has nothing left to receive a frame into, so NAPI is never rescheduled
and airoha_qdma_fill_rx_queue() (which reposts descriptors) is never
called again. The ring is stuck until the interface is brought down
and back up.

This is most visible on rings that carry low, bursty volumes of
protocol control traffic, in particular RX ring 4, to which
airoha_fe_vip_setup() force-routes ~15 unrelated VIP-classified
protocols (BOOTP, PPPoE Discovery, ISAKMP, DHCPv6, SIP, LLDP, PPP
LCP/IPCP/CHAP/PAP/IPv6CP, ...) via PATN_FCPU_EN_MASK, all sharing the
same RX_DSCP_NUM() default of 16 descriptors. A short burst on that
ring (e.g. a DHCP lease renewal exchange, or the LCP/IPCP/CHAP/PAP
negotiation that follows a PPPoE PADO) can drain it faster than the
CPU reposts descriptors, after which every one of those protocols
silently stops being received on that device until it is reconfigured
- with no error, warning, or netdev/ethtool counter indicating why.

Fix airoha_irq_handler() to treat NO_CPU_DSCP the same as RX_DONE for
the purpose of scheduling NAPI: airoha_qdma_rx_process() already calls
airoha_qdma_fill_rx_queue() unconditionally at the end of every poll,
even when zero descriptors were reaped, so scheduling NAPI in response
to NO_CPU_DSCP is sufficient to make an emptied ring recover on its
own. airoha_qdma_rx_napi_poll() is updated to re-enable the
NO_CPU_DSCP bit alongside RX_DONE when napi_complete() runs, mirroring
the existing disable/enable dance so the interrupt isn't left masked
after its first use.

One open question worth flagging explicitly: if the underlying
no-free-descriptor condition re-latches this bit immediately after the
ack write (rather than only on the next empty->non-empty transition),
a ring that airoha_qdma_fill_rx_queue() genuinely cannot repost into
(e.g. page_pool_dev_alloc_frag() returning NULL under memory pressure)
would turn this into a self-reasserting interrupt storm on the hard
IRQ path: mask -> napi_schedule() -> poll reaps 0, refills 0 ->
napi_complete() -> unmask -> NO_CPU_DSCP fires again immediately. I
don't have documentation confirming which behavior this bit actually
has. Regardless of the answer, masking NO_CPU_DSCP until a refill
actually succeeds - the natural-looking alternative - is worse: a
fully memory-starved ring can never fire RX_DONE either (nothing was
posted for hw to complete), so that would leave it permanently dead
once the memory pressure clears rather than self-healing. A bounded
storm tied to genuine memory pressure, if that's what this is, seems
preferable to a ring with no way back either way.

Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
Link: https://github.com/openwrt/openwrt/issues/24715
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c | 28 +++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 64619e9a704d..a3e5aaeb75b3 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -784,13 +784,15 @@ static int airoha_qdma_rx_napi_poll(struct napi_struct *napi, int budget)
 		int i, qid = q - &qdma->q_rx[0];
 		int intr_reg = qid < RX_DONE_HIGH_OFFSET ? QDMA_INT_REG_IDX1
 							 : QDMA_INT_REG_IDX2;
+		u32 bit = qid % RX_DONE_HIGH_OFFSET;
 
 		for (i = 0; i < ARRAY_SIZE(qdma->irq_banks); i++) {
 			if (!(BIT(qid) & RX_IRQ_BANK_PIN_MASK(i)))
 				continue;
 
 			airoha_qdma_irq_enable(&qdma->irq_banks[i], intr_reg,
-					       BIT(qid % RX_DONE_HIGH_OFFSET));
+					       BIT(bit) |
+					       BIT(bit + RX_NO_CPU_DSCP_LOW_OFFSET));
 		}
 	}
 
@@ -1468,16 +1470,32 @@ static irqreturn_t airoha_irq_handler(int irq, void *dev_instance)
 	if (!test_bit(DEV_STATE_INITIALIZED, &qdma->eth->state))
 		return IRQ_NONE;
 
-	rx_intr1 = intr[1] & RX_DONE_LOW_INT_MASK;
+	/* A ring can also raise NO_CPU_DSCP when it runs out of free RX
+	 * descriptors (e.g. a burst of VIP-classified control traffic
+	 * forced onto a small ring). Once a ring is fully drained no more
+	 * RX_DONE interrupts can fire for it, since there are no free
+	 * descriptors left for hardware to receive into, so without this
+	 * NAPI is never rescheduled and the ring never gets refilled again.
+	 * Treat NO_CPU_DSCP the same as RX_DONE for scheduling NAPI:
+	 * airoha_qdma_rx_process() unconditionally calls
+	 * airoha_qdma_fill_rx_queue() at the end of every poll, even when
+	 * zero descriptors were reaped, so this alone is enough to recover
+	 * the ring.
+	 */
+	rx_intr1 = intr[1] & (RX_DONE_LOW_INT_MASK | RX_NO_CPU_DSCP_LOW_INT_MASK);
 	if (rx_intr1) {
 		airoha_qdma_irq_disable(irq_bank, QDMA_INT_REG_IDX1, rx_intr1);
-		rx_intr_mask |= rx_intr1;
+		rx_intr_mask |= (rx_intr1 & RX_DONE_LOW_INT_MASK) |
+				((rx_intr1 & RX_NO_CPU_DSCP_LOW_INT_MASK) >>
+				 RX_NO_CPU_DSCP_LOW_OFFSET);
 	}
 
-	rx_intr2 = intr[2] & RX_DONE_HIGH_INT_MASK;
+	rx_intr2 = intr[2] & (RX_DONE_HIGH_INT_MASK | RX_NO_CPU_DSCP_HIGH_INT_MASK);
 	if (rx_intr2) {
 		airoha_qdma_irq_disable(irq_bank, QDMA_INT_REG_IDX2, rx_intr2);
-		rx_intr_mask |= (rx_intr2 << 16);
+		rx_intr_mask |= ((rx_intr2 & RX_DONE_HIGH_INT_MASK) |
+				 ((rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK) >>
+				  RX_NO_CPU_DSCP_LOW_OFFSET)) << 16;
 	}
 
 	for (i = 0; rx_intr_mask && i < ARRAY_SIZE(qdma->q_rx); i++) {
-- 
2.55.0

Re: [PATCH net 1/4] net: airoha: handle RX_NO_CPU_DSCP interrupt, not just RX_DONE

[Lorenzo Bianconi]

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

> The QDMA hardware raises a dedicated interrupt (NO_CPU_DSCP, one bit
> per ring in QDMA_CSR_INT_ENABLE2/3) when an RX ring runs out of free
> CPU descriptors. airoha_qdma_hw_init() already unmasks this interrupt
> for every ring (INT_RX1_MASK()/INT_RX2_MASK() OR it together with the
> RX_DONE bits before writing the enable register), but
> airoha_irq_handler() only ever extracts the RX_DONE bits from the same
> status word - the NO_CPU_DSCP bits are read and acknowledged (cleared)
> along with everything else at the top of the handler, then silently
> dropped.
> 
> This matters because once a ring is genuinely drained to zero posted
> descriptors, no further RX_DONE interrupt can fire for it: hardware
> has nothing left to receive a frame into, so NAPI is never rescheduled
> and airoha_qdma_fill_rx_queue() (which reposts descriptors) is never
> called again. The ring is stuck until the interface is brought down
> and back up.
> 
> This is most visible on rings that carry low, bursty volumes of
> protocol control traffic, in particular RX ring 4, to which
> airoha_fe_vip_setup() force-routes ~15 unrelated VIP-classified
> protocols (BOOTP, PPPoE Discovery, ISAKMP, DHCPv6, SIP, LLDP, PPP
> LCP/IPCP/CHAP/PAP/IPv6CP, ...) via PATN_FCPU_EN_MASK, all sharing the
> same RX_DSCP_NUM() default of 16 descriptors. A short burst on that
> ring (e.g. a DHCP lease renewal exchange, or the LCP/IPCP/CHAP/PAP
> negotiation that follows a PPPoE PADO) can drain it faster than the
> CPU reposts descriptors, after which every one of those protocols
> silently stops being received on that device until it is reconfigured
> - with no error, warning, or netdev/ethtool counter indicating why.
> 
> Fix airoha_irq_handler() to treat NO_CPU_DSCP the same as RX_DONE for
> the purpose of scheduling NAPI: airoha_qdma_rx_process() already calls
> airoha_qdma_fill_rx_queue() unconditionally at the end of every poll,
> even when zero descriptors were reaped, so scheduling NAPI in response
> to NO_CPU_DSCP is sufficient to make an emptied ring recover on its
> own. airoha_qdma_rx_napi_poll() is updated to re-enable the
> NO_CPU_DSCP bit alongside RX_DONE when napi_complete() runs, mirroring
> the existing disable/enable dance so the interrupt isn't left masked
> after its first use.
> 
> One open question worth flagging explicitly: if the underlying
> no-free-descriptor condition re-latches this bit immediately after the
> ack write (rather than only on the next empty->non-empty transition),
> a ring that airoha_qdma_fill_rx_queue() genuinely cannot repost into
> (e.g. page_pool_dev_alloc_frag() returning NULL under memory pressure)
> would turn this into a self-reasserting interrupt storm on the hard
> IRQ path: mask -> napi_schedule() -> poll reaps 0, refills 0 ->
> napi_complete() -> unmask -> NO_CPU_DSCP fires again immediately. I
> don't have documentation confirming which behavior this bit actually
> has. Regardless of the answer, masking NO_CPU_DSCP until a refill
> actually succeeds - the natural-looking alternative - is worse: a
> fully memory-starved ring can never fire RX_DONE either (nothing was
> posted for hw to complete), so that would leave it permanently dead
> once the memory pressure clears rather than self-healing. A bounded
> storm tied to genuine memory pressure, if that's what this is, seems
> preferable to a ring with no way back either way.

Hi Vitaliy,

I do not have info about it. Adding airoha folks in CC.

> 
> Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
> Link: https://github.com/openwrt/openwrt/issues/24715
> Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 28 +++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)

thx for working on it. Just a couple of nit inline. Fixing them:

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 64619e9a704d..a3e5aaeb75b3 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -784,13 +784,15 @@ static int airoha_qdma_rx_napi_poll(struct napi_struct *napi, int budget)
>  		int i, qid = q - &qdma->q_rx[0];
>  		int intr_reg = qid < RX_DONE_HIGH_OFFSET ? QDMA_INT_REG_IDX1
>  							 : QDMA_INT_REG_IDX2;
> +		u32 bit = qid % RX_DONE_HIGH_OFFSET;

nit: what about doing something like:

		u32 irq_id = qid % RX_DONE_HIGH_OFFSET;
		u32 intr_mask = BIT(irq_id) |
				BIT(irq_id + RX_NO_CPU_DSCP_LOW_OFFSET);

>  
>  		for (i = 0; i < ARRAY_SIZE(qdma->irq_banks); i++) {
>  			if (!(BIT(qid) & RX_IRQ_BANK_PIN_MASK(i)))
>  				continue;
>  
>  			airoha_qdma_irq_enable(&qdma->irq_banks[i], intr_reg,
> -					       BIT(qid % RX_DONE_HIGH_OFFSET));
> +					       BIT(bit) |
> +					       BIT(bit + RX_NO_CPU_DSCP_LOW_OFFSET));
>  		}
>  	}
>  
> @@ -1468,16 +1470,32 @@ static irqreturn_t airoha_irq_handler(int irq, void *dev_instance)
>  	if (!test_bit(DEV_STATE_INITIALIZED, &qdma->eth->state))
>  		return IRQ_NONE;
>  
> -	rx_intr1 = intr[1] & RX_DONE_LOW_INT_MASK;
> +	/* A ring can also raise NO_CPU_DSCP when it runs out of free RX
> +	 * descriptors (e.g. a burst of VIP-classified control traffic
> +	 * forced onto a small ring). Once a ring is fully drained no more
> +	 * RX_DONE interrupts can fire for it, since there are no free
> +	 * descriptors left for hardware to receive into, so without this
> +	 * NAPI is never rescheduled and the ring never gets refilled again.
> +	 * Treat NO_CPU_DSCP the same as RX_DONE for scheduling NAPI:
> +	 * airoha_qdma_rx_process() unconditionally calls
> +	 * airoha_qdma_fill_rx_queue() at the end of every poll, even when
> +	 * zero descriptors were reaped, so this alone is enough to recover
> +	 * the ring.
> +	 */

I guess this comment (AI driven??) should go into the commit message, it seems too
verbose for the code and it is quite obvious what you want to do.

> +	rx_intr1 = intr[1] & (RX_DONE_LOW_INT_MASK | RX_NO_CPU_DSCP_LOW_INT_MASK);
>  	if (rx_intr1) {
>  		airoha_qdma_irq_disable(irq_bank, QDMA_INT_REG_IDX1, rx_intr1);
> -		rx_intr_mask |= rx_intr1;
> +		rx_intr_mask |= (rx_intr1 & RX_DONE_LOW_INT_MASK) |
> +				((rx_intr1 & RX_NO_CPU_DSCP_LOW_INT_MASK) >>
> +				 RX_NO_CPU_DSCP_LOW_OFFSET);

What about defining a macro in airoha_regs.h

#define RX_NO_CPU_DSCP_INT_RX1_MASK(_n) (((_n) & RX_NO_CPU_DSCP_LOW_INT_MASK) >>\
					 RX_NO_CPU_DSCP_LOW_OFFSET)

>  	}
>  
> -	rx_intr2 = intr[2] & RX_DONE_HIGH_INT_MASK;
> +	rx_intr2 = intr[2] & (RX_DONE_HIGH_INT_MASK | RX_NO_CPU_DSCP_HIGH_INT_MASK);
>  	if (rx_intr2) {
>  		airoha_qdma_irq_disable(irq_bank, QDMA_INT_REG_IDX2, rx_intr2);
> -		rx_intr_mask |= (rx_intr2 << 16);
> +		rx_intr_mask |= ((rx_intr2 & RX_DONE_HIGH_INT_MASK) |
> +				 ((rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK) >>
> +				  RX_NO_CPU_DSCP_LOW_OFFSET)) << 16;

I guess here you do not need to shift RX_NO_CPU_DSCP_HIGH_INT_MASK mask since
it is already in the range [31:16], just mask like:

	(rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK)

>  	}
>  
>  	for (i = 0; rx_intr_mask && i < ARRAY_SIZE(qdma->q_rx); i++) {
> -- 
> 2.55.0
> 

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

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

[Vitaliy Sochnev]

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.

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.

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);
+
+	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;
+
+		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);
+
+		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.
+	 */
+	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

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

[Lorenzo Bianconi]

[-- 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 --]

[PATCH net-next 3/4] net: airoha: add rx_stall_recover ethtool counter

[Vitaliy Sochnev]

Make the RX-ring hw-stall recovery added by the previous commit
observable without grepping dmesg for its dev_warn_ratelimited():
add a custom ethtool -S statistic, rx_stall_recover, incremented once
per completed recovery.

It's tracked per-QDMA-instance rather than per-netdev, since the
stalled ring can carry traffic for more than one netdev at once (VIP
classification shares ring 4 across several protocols/ports on this
hardware) - there's no single netdev to attribute an individual event
to, so all netdevs behind the same QDMA instance report the same
aggregate count.

Kept separate from the actual recovery fix since this adds new
ethtool ABI and has no bearing on correctness; happy to have it queued
independently if that's preferred.

Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c | 44 ++++++++++++++++++++++++
 drivers/net/ethernet/airoha/airoha_eth.h |  5 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index b53fe5b17653..efb1dd69cc16 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -1000,6 +1000,7 @@ static void airoha_qdma_rx_recover_work(struct work_struct *work)
 		napi_enable(&q->napi);
 		napi_schedule(&q->napi);
 
+		qdma->rx_recover_count++;
 		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);
@@ -2600,6 +2601,46 @@ static void airoha_ethtool_get_drvinfo(struct net_device *netdev,
 	strscpy(info->bus_info, dev_name(eth->dev), sizeof(info->bus_info));
 }
 
+static const char airoha_ethtool_stats_str[][ETH_GSTRING_LEN] = {
+	"rx_stall_recover",
+};
+
+static void airoha_ethtool_get_strings(struct net_device *netdev, u32 sset,
+				       u8 *data)
+{
+	int i;
+
+	if (sset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_str); i++)
+		ethtool_puts(&data, airoha_ethtool_stats_str[i]);
+}
+
+static int airoha_ethtool_get_sset_count(struct net_device *netdev, int sset)
+{
+	return sset == ETH_SS_STATS ?
+	       ARRAY_SIZE(airoha_ethtool_stats_str) : -EOPNOTSUPP;
+}
+
+static void airoha_ethtool_get_ethtool_stats(struct net_device *netdev,
+					     struct ethtool_stats *stats,
+					     u64 *data)
+{
+	struct airoha_gdm_dev *dev = netdev_priv(netdev);
+	struct airoha_qdma *qdma;
+
+	rcu_read_lock();
+	qdma = rcu_dereference(dev->qdma);
+	/* aggregate recovery count for the whole qdma instance: the
+	 * stalled ring can carry traffic for more than one netdev (VIP
+	 * classification shares ring 4 across several protocols/ports),
+	 * so there's no single netdev to attribute an individual event to
+	 */
+	data[0] = qdma ? qdma->rx_recover_count : 0;
+	rcu_read_unlock();
+}
+
 static void airoha_ethtool_get_mac_stats(struct net_device *netdev,
 					 struct ethtool_eth_mac_stats *stats)
 {
@@ -3502,6 +3543,9 @@ static const struct net_device_ops airoha_netdev_ops = {
 
 static const struct ethtool_ops airoha_ethtool_ops = {
 	.get_drvinfo		= airoha_ethtool_get_drvinfo,
+	.get_strings		= airoha_ethtool_get_strings,
+	.get_sset_count		= airoha_ethtool_get_sset_count,
+	.get_ethtool_stats	= airoha_ethtool_get_ethtool_stats,
 	.get_eth_mac_stats      = airoha_ethtool_get_mac_stats,
 	.get_rmon_stats		= airoha_ethtool_get_rmon_stats,
 	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index 483d6b59c351..d6591a779743 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -582,6 +582,11 @@ struct airoha_qdma {
 	 */
 	struct work_struct rx_recover_work;
 	DECLARE_BITMAP(rx_recover_mask, AIROHA_NUM_RX_RING);
+	/* count of completed hw-stall recoveries, exposed via ethtool -S
+	 * so a recovery event (and the packets it drops) is observable
+	 * without grepping dmesg for the dev_warn_ratelimited() above
+	 */
+	u32 rx_recover_count;
 
 	DECLARE_BITMAP(qos_channel_map, AIROHA_NUM_QOS_CHANNELS);
 };
-- 
2.55.0

[PATCH net-next 4/4] net: airoha: grow RX ring 4 to 128 descriptors

[Vitaliy Sochnev]

Ring 4 is the shared "force to CPU" ring for a wide set of protocols
(BOOTP, PPPoE Discovery, PPP LCP/IPCP/CHAP/IPv6CP/PAP, ISAKMP, DHCPv6,
SIP, LLDP, ...) and currently falls into the 16-descriptor default in
RX_DSCP_NUM(), same as most other non-hashed rings. The hw completion
race recovered by airoha_qdma_rx_check_stall()/rx_recover_work() in
the previous commits appears strongly correlated with a ring going
from idle to receiving its first frame(s) - exactly the access pattern
this shared ring sees under protocol negotiation bursts. Give it the
same 128-descriptor allowance already used for rings 2/11/15, matching
the other rings that see bursty, non-hashed traffic, to reduce how
often that condition is hit in the first place.

Ring 4 and the VIP classification that forces these protocols onto it
are shared driver-wide, with no DT/hardware property distinguishing
one chip variant's ring 4 from another's, so this isn't scoped to
AN7583 specifically even though that's where the race was found and
reproduced. 24h+ stress runs forcing repeated PPPoE/DHCP renegotiation
on an AN7581 board, both with and without the two preceding fixes,
completed 570+ forced reconnect cycles each with no regressions from
the larger ring.

Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index d6591a779743..cd75c16d8d0c 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -41,6 +41,7 @@
 #define TX_DSCP_NUM			1024
 #define RX_DSCP_NUM(_n)			\
 	((_n) ==  2 ? 128 :		\
+	 (_n) ==  4 ? 128 :		\
 	 (_n) == 11 ? 128 :		\
 	 (_n) == 15 ? 128 :		\
 	 (_n) ==  0 ? 1024 : 16)
-- 
2.55.0

Re: [PATCH net-next 4/4] net: airoha: grow RX ring 4 to 128 descriptors

[Lorenzo Bianconi]

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

> Ring 4 is the shared "force to CPU" ring for a wide set of protocols
> (BOOTP, PPPoE Discovery, PPP LCP/IPCP/CHAP/IPv6CP/PAP, ISAKMP, DHCPv6,
> SIP, LLDP, ...) and currently falls into the 16-descriptor default in
> RX_DSCP_NUM(), same as most other non-hashed rings. The hw completion
> race recovered by airoha_qdma_rx_check_stall()/rx_recover_work() in
> the previous commits appears strongly correlated with a ring going
> from idle to receiving its first frame(s) - exactly the access pattern
> this shared ring sees under protocol negotiation bursts. Give it the
> same 128-descriptor allowance already used for rings 2/11/15, matching
> the other rings that see bursty, non-hashed traffic, to reduce how
> often that condition is hit in the first place.
> 
> Ring 4 and the VIP classification that forces these protocols onto it
> are shared driver-wide, with no DT/hardware property distinguishing
> one chip variant's ring 4 from another's, so this isn't scoped to
> AN7583 specifically even though that's where the race was found and
> reproduced. 24h+ stress runs forcing repeated PPPoE/DHCP renegotiation
> on an AN7581 board, both with and without the two preceding fixes,
> completed 570+ forced reconnect cycles each with no regressions from
> the larger ring.

If you set RX_DSCP_NUM to 128 on AN7583 for rx queue 4, are you able to
trigger the issue?

> 
> Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index d6591a779743..cd75c16d8d0c 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -41,6 +41,7 @@
>  #define TX_DSCP_NUM			1024
>  #define RX_DSCP_NUM(_n)			\
>  	((_n) ==  2 ? 128 :		\
> +	 (_n) ==  4 ? 128 :		\
>  	 (_n) == 11 ? 128 :		\
>  	 (_n) == 15 ? 128 :		\
>  	 (_n) ==  0 ? 1024 : 16)

Can you please change the default from 16 to 32? This is the default value used
in the vendor sdk.

Regards,
Lorenzo

> -- 
> 2.55.0
> 

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