[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

[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

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

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

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

[PATCH net v2 0/3] net: airoha: fix silent RX loss on the shared CPU ring

[Vitaliy Sochnev]

All three target net. v1 split them across net/net-next; with the ring
size now shown to be load bearing, they belong together.

Answers to the v1 review:

- "fill_rx_queue() overwrites the DONE bit written by hw" - no. I
  implemented that check anyway (bail out in fill_rx_queue(), clear the
  bit in rx_process()) and it never fired once, while the ring was
  demonstrably stalled: the descriptor at q->head never had
  QDMA_DESC_DONE_MASK set, so there was nothing to catch. Worse,
  clearing desc->ctrl outright also wipes QDMA_DESC_LEN_MASK, which
  fill_rx_queue() writes as the buffer size and hw reads back - RX then
  delivers poisoned pages. That version is dropped.

- "have you tried to just increase the queue size" - yes, and that is
  the fix. Ring 4 at 16 stalls roughly every 35 s under repeated PPPoE
  dial-up and negotiation never completes; at 128 nothing triggered
  across 500 forced reconnects over 20 h. Patch 3.

- "is QDMA_DESC_DROP_MASK set when the issue occurs" - no, never, for
  the whole duration of the stall.

- default RX_DSCP_NUM raised 16 -> 32 per the vendor SDK, as asked.

- recovery pause measured at 986-1131 us over 13 events, against the
  50 ms read_poll_timeout() ceiling.

- style: RCT, verbose comments gone, and the !q->ndesc check dropped -
  the bit is only set from rx_process(), which cannot run on a ring
  without descriptors.

One cost worth flagging: the detector adds an uncached REG_RX_DMA_IDX
read to every rx_process() call that ends on a non-DONE descriptor. airoha_qdma_rx_napi_poll() loops while the last
pass reaped anything, so that is once or twice per NAPI poll, on every
ring, not just the one that can stall. Gating it on the previous poll
having reaped nothing would keep it off busy rings and only delays
detection by about one poll, since q->tail is frozen from the moment
the stall begins. I left it ungated as I have no profiling either way -
happy to add the gate if you prefer it.

A question for the airoha folks: during the stall REG_RX_DMA_IDX keeps advancing while
REG_RX_CPU_IDX stays put and the consumer never sees another DONE. Is
there a documented condition under which hw stops writing completions
back to a ring, or a constraint on RX_CPU_IDX that the driver is
violating by leaving one descriptor unposted?

The rx_stall_recover ethtool counter from v1 is dropped here - new ABI
does not belong in a fix - and will follow for net-next.

Tested on Nokia XG-040G-MF (AN7583) on a live PPPoE line, in both
configurations: as sent, and with stock ring sizes so the recovery path
actually executes.

Vitaliy Sochnev (3):
  net: airoha: handle RX_NO_CPU_DSCP interrupt, not just RX_DONE
  net: airoha: recover RX ring after hw completion stall
  net: airoha: grow the small RX rings

 drivers/net/ethernet/airoha/airoha_eth.c  | 114 ++++++++++++++++++++--
 drivers/net/ethernet/airoha/airoha_eth.h  |  11 ++-
 drivers/net/ethernet/airoha/airoha_regs.h |   2 +
 3 files changed, 120 insertions(+), 7 deletions(-)

-- 
2.55.0

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

[Vitaliy Sochnev]

airoha_qdma_hw_init() unmasks the per-ring NO_CPU_DSCP interrupt, which
fires when an RX ring runs out of free CPU descriptors, but
airoha_irq_handler() only extracts the RX_DONE bits from the same status
word. The NO_CPU_DSCP bits are acknowledged and dropped.

Once a ring is drained to zero posted descriptors no further RX_DONE can
fire for it - nothing is left for hw to receive into - so NAPI is never
rescheduled, airoha_qdma_fill_rx_queue() is never called again, and the
ring stays dead until the interface is reconfigured.

This is reachable on RX ring 4, which airoha_fe_vip_setup() force-routes
~15 VIP-classified protocols onto (BOOTP, PPPoE Discovery, ISAKMP,
DHCPv6, SIP, LLDP, PPP LCP/IPCP/CHAP/PAP/IPv6CP, ...) while it sits on
the 16-descriptor RX_DSCP_NUM() default. A DHCP renewal or a PPPoE
negotiation burst drains it faster than the CPU reposts, after which all
of those protocols silently stop being received.

Treat NO_CPU_DSCP like RX_DONE for scheduling NAPI:
airoha_qdma_rx_process() already calls airoha_qdma_fill_rx_queue()
unconditionally at the end of every poll, so scheduling NAPI is enough
to make an emptied ring refill itself. Re-enable the bit alongside
RX_DONE in airoha_qdma_rx_napi_poll() so it is not left masked after
first use.

Whether NO_CPU_DSCP re-latches while the ring is still empty is not
documented. If it does, a ring that cannot be refilled (page_pool
returning NULL) keeps reasserting it. Masking it until a refill succeeds
is worse: a starved ring never fires RX_DONE either, so it would stay
dead after the memory pressure clears.

Fixes: f252493e1835 ("net: airoha: Enable multiple IRQ lines support in airoha_eth driver.")
Link: https://github.com/openwrt/openwrt/issues/24715
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c  | 16 +++++++++++-----
 drivers/net/ethernet/airoha/airoha_regs.h |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 64619e9a704d..c59201aded26 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -784,13 +784,16 @@ 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 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));
+					       intr_mask);
 		}
 	}
 
@@ -1468,16 +1471,19 @@ 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;
+	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_NO_CPU_DSCP_INT_RX1_MASK(rx_intr1);
 	}
 
-	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_DONE_HIGH_OFFSET) |
+				(rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK);
 	}
 
 	for (i = 0; rx_intr_mask && i < ARRAY_SIZE(qdma->q_rx); i++) {
diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
index 442b48c9b991..bde3d599d8b7 100644
--- a/drivers/net/ethernet/airoha/airoha_regs.h
+++ b/drivers/net/ethernet/airoha/airoha_regs.h
@@ -549,6 +549,8 @@
 #define INT_RX1_MASK(_n)							\
 	((((_n) << RX_NO_CPU_DSCP_LOW_OFFSET) & RX_NO_CPU_DSCP_LOW_INT_MASK) |	\
 	 (RX_DONE_LOW_INT_MASK & (_n)))
+#define RX_NO_CPU_DSCP_INT_RX1_MASK(_n)					\
+	(((_n) & RX_NO_CPU_DSCP_LOW_INT_MASK) >> RX_NO_CPU_DSCP_LOW_OFFSET)
 
 /* QDMA_CSR_INT_ENABLE3 */
 #define RX31_NO_CPU_DSCP_INT_MASK	BIT(31)
-- 
2.55.0

[PATCH net v2 2/3] net: airoha: recover RX ring after hw completion stall

[Vitaliy Sochnev]

On AN7583 hw can stop advancing the descriptor the sequential consumer in
airoha_qdma_rx_process() is waiting on, while its own completion counter
keeps moving. The ring is then dead: NAPI is scheduled, finds DONE clear
at q->tail, and does nothing, forever.

Observed directly on ring 4 at its 16-descriptor default (devmem, qdma0):
REG_RX_CPU_IDX frozen at 15 for over an hour while REG_RX_DMA_IDX advanced
29 -> 96, with a 60-byte frame left stranded in the ring.
QDMA_DESC_DROP_MASK was never set. In practice this is hit during PPPoE
negotiation bursts on the shared "force to CPU" ring, where it stops the
dial-up from ever completing.

Detect it without trusting ring content: REG_RX_DMA_IDX is hw's own
counter, independent of what sw posted. If it advances across polls while
q->tail does not, hw is making progress the consumer cannot observe. Idle
rings, where hw does not advance either, are left alone. An earlier
version scanned the ring for a DONE descriptor and trusted its content;
that OOMed once it reached uninitialised DMA memory that happened to have
the bit set. A register cannot misfire that way.

Recovery is deferred to a work item, since the register access can sleep.
It drops what is in flight and re-arms the ring through the existing
cleanup_rx_queue()/fill_rx_queue() pair, which only touch the sw-owned
[tail, head) window and rewrite both indices from it. Trying instead to
identify and keep the descriptor hw used caused a page_pool double free.

GLOBAL_CFG_RX_DMA_EN_MASK is per-QDMA, not per-ring, so this briefly
pauses every ring behind that instance; there is no per-ring equivalent.
The measured pause is 986-1131 us over 13 recoveries, not the 50 ms
read_poll_timeout() ceiling, so the logged value is worth having.

Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c | 98 +++++++++++++++++++++++-
 drivers/net/ethernet/airoha/airoha_eth.h |  8 ++
 2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index c59201aded26..177a0e10e372 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,34 @@ airoha_qdma_get_gdm_dev(struct airoha_eth *eth, struct airoha_qdma_desc *desc)
 	return port->devs[d] ? port->devs[d] : ERR_PTR(-ENODEV);
 }
 
+#define AIROHA_RX_STALL_THRESHOLD	3
+
+/* REG_RX_DMA_IDX is hw's own completion counter, independent of what sw has
+ * posted, so comparing it against q->tail spots the stall without trusting
+ * ring content: if hw keeps advancing while the strictly sequential consumer
+ * does not, it is completing descriptors that consumer can never reach.
+ */
+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;
+
+	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 +705,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();
 
@@ -895,6 +927,56 @@ 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;
+
+		napi_disable(&q->napi);
+
+		/* per-QDMA, not per-ring: this pauses every RX ring behind
+		 * this instance, hence the measured duration below
+		 */
+		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);
+
+		airoha_qdma_cleanup_rx_queue(q);
+		if (q->skb) {
+			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 %lld us)\n",
+				     qid, rx_dma_off_us);
+		clear_bit(qid, qdma->rx_recover_mask);
+	}
+}
+
 static int airoha_qdma_init_rx(struct airoha_qdma *qdma)
 {
 	int i;
@@ -1582,6 +1664,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;
 
@@ -1639,6 +1723,13 @@ static int airoha_hw_init(struct platform_device *pdev,
 	if (err)
 		return err;
 
+	/* init every instance up front: the error path below tears down all
+	 * of eth->qdma[], including entries the init 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)
@@ -1687,6 +1778,11 @@ static void airoha_qdma_stop_napi(struct airoha_qdma *qdma)
 {
 	int i;
 
+	/* must not run or re-arm past this point: the work calls
+	 * napi_disable() too, and doing that twice spins forever
+	 */
+	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..c195dad5ed58 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -207,6 +207,11 @@ struct airoha_queue {
 	bool txq_stopped;
 	bool flushing;
 
+	/* 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 +572,9 @@ struct airoha_qdma {
 	struct airoha_queue q_tx[AIROHA_NUM_TX_RING];
 	struct airoha_queue q_rx[AIROHA_NUM_RX_RING];
 
+	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

[PATCH net v2 3/3] net: airoha: grow the small RX rings

[Vitaliy Sochnev]

Raise the RX_DSCP_NUM() fallback from 16 to 32, the vendor SDK default,
and give ring 4 the 128 descriptors already used for rings 2/11/15.

Ring 4 is the shared "force to CPU" ring that airoha_fe_vip_setup()
routes BOOTP, PPPoE Discovery, PPP LCP/IPCP/CHAP/PAP/IPv6CP, ISAKMP,
DHCPv6, SIP and LLDP onto, so it sees the same bursty non-hashed traffic
as the rings already on 128 while sitting on the smallest allowance.

At 16 descriptors the completion stall handled by the previous patch is
readily reachable: on a Nokia XG-040G-MF the recovery fires roughly every
35 s under repeated PPPoE dial-up, and negotiation never completes. At
128 it did not trigger once across 500 forced PPPoE reconnects over 20 h.

Ring 4 and the VIP classification feeding it are shared driver-wide, with
no DT or hardware property distinguishing one variant's ring 4 from
another's, so this is not scoped to a particular SoC.

Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index c195dad5ed58..0b7c90940272 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -41,9 +41,10 @@
 #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)
+	 (_n) ==  0 ? 1024 : 32)
 
 #define PSE_RSV_PAGES			128
 #define PSE_QUEUE_RSV_PAGES		64
-- 
2.55.0

Re: [PATCH net v2 3/3] net: airoha: grow the small RX rings

[Lorenzo Bianconi]

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

> Raise the RX_DSCP_NUM() fallback from 16 to 32, the vendor SDK default,
> and give ring 4 the 128 descriptors already used for rings 2/11/15.
> 
> Ring 4 is the shared "force to CPU" ring that airoha_fe_vip_setup()
> routes BOOTP, PPPoE Discovery, PPP LCP/IPCP/CHAP/PAP/IPv6CP, ISAKMP,
> DHCPv6, SIP and LLDP onto, so it sees the same bursty non-hashed traffic
> as the rings already on 128 while sitting on the smallest allowance.
> 
> At 16 descriptors the completion stall handled by the previous patch is
> readily reachable: on a Nokia XG-040G-MF the recovery fires roughly every
> 35 s under repeated PPPoE dial-up, and negotiation never completes. At
> 128 it did not trigger once across 500 forced PPPoE reconnects over 20 h.
> 
> Ring 4 and the VIP classification feeding it are shared driver-wide, with
> no DT or hardware property distinguishing one variant's ring 4 from
> another's, so this is not scoped to a particular SoC.
> 
> Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>

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

> ---
>  drivers/net/ethernet/airoha/airoha_eth.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index c195dad5ed58..0b7c90940272 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -41,9 +41,10 @@
>  #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)
> +	 (_n) ==  0 ? 1024 : 32)
>  
>  #define PSE_RSV_PAGES			128
>  #define PSE_QUEUE_RSV_PAGES		64
> -- 
> 2.55.0
> 

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

Re: [PATCH net v2 2/3] net: airoha: recover RX ring after hw completion stall

[Lorenzo Bianconi]

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

> On AN7583 hw can stop advancing the descriptor the sequential consumer in
> airoha_qdma_rx_process() is waiting on, while its own completion counter
> keeps moving. The ring is then dead: NAPI is scheduled, finds DONE clear
> at q->tail, and does nothing, forever.
> 
> Observed directly on ring 4 at its 16-descriptor default (devmem, qdma0):
> REG_RX_CPU_IDX frozen at 15 for over an hour while REG_RX_DMA_IDX advanced
> 29 -> 96, with a 60-byte frame left stranded in the ring.
> QDMA_DESC_DROP_MASK was never set. In practice this is hit during PPPoE
> negotiation bursts on the shared "force to CPU" ring, where it stops the
> dial-up from ever completing.
> 
> Detect it without trusting ring content: REG_RX_DMA_IDX is hw's own
> counter, independent of what sw posted. If it advances across polls while
> q->tail does not, hw is making progress the consumer cannot observe. Idle
> rings, where hw does not advance either, are left alone. An earlier
> version scanned the ring for a DONE descriptor and trusted its content;
> that OOMed once it reached uninitialised DMA memory that happened to have
> the bit set. A register cannot misfire that way.
> 
> Recovery is deferred to a work item, since the register access can sleep.
> It drops what is in flight and re-arms the ring through the existing
> cleanup_rx_queue()/fill_rx_queue() pair, which only touch the sw-owned
> [tail, head) window and rewrite both indices from it. Trying instead to
> identify and keep the descriptor hw used caused a page_pool double free.
> 
> GLOBAL_CFG_RX_DMA_EN_MASK is per-QDMA, not per-ring, so this briefly
> pauses every ring behind that instance; there is no per-ring equivalent.
> The measured pause is 986-1131 us over 13 recoveries, not the 50 ms
> read_poll_timeout() ceiling, so the logged value is worth having.
> 
> Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>

IIUC increasing the ring size the issue never occurs again (correct?). If so, I
would drop this patch for the moment and add it if it is really necessary.

Regards,
Lorenzo

> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 98 +++++++++++++++++++++++-
>  drivers/net/ethernet/airoha/airoha_eth.h |  8 ++
>  2 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index c59201aded26..177a0e10e372 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,34 @@ airoha_qdma_get_gdm_dev(struct airoha_eth *eth, struct airoha_qdma_desc *desc)
>  	return port->devs[d] ? port->devs[d] : ERR_PTR(-ENODEV);
>  }
>  
> +#define AIROHA_RX_STALL_THRESHOLD	3
> +
> +/* REG_RX_DMA_IDX is hw's own completion counter, independent of what sw has
> + * posted, so comparing it against q->tail spots the stall without trusting
> + * ring content: if hw keeps advancing while the strictly sequential consumer
> + * does not, it is completing descriptors that consumer can never reach.
> + */
> +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;
> +
> +	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 +705,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();
>  
> @@ -895,6 +927,56 @@ 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;
> +
> +		napi_disable(&q->napi);
> +
> +		/* per-QDMA, not per-ring: this pauses every RX ring behind
> +		 * this instance, hence the measured duration below
> +		 */
> +		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);
> +
> +		airoha_qdma_cleanup_rx_queue(q);
> +		if (q->skb) {
> +			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 %lld us)\n",
> +				     qid, rx_dma_off_us);
> +		clear_bit(qid, qdma->rx_recover_mask);
> +	}
> +}
> +
>  static int airoha_qdma_init_rx(struct airoha_qdma *qdma)
>  {
>  	int i;
> @@ -1582,6 +1664,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;
>  
> @@ -1639,6 +1723,13 @@ static int airoha_hw_init(struct platform_device *pdev,
>  	if (err)
>  		return err;
>  
> +	/* init every instance up front: the error path below tears down all
> +	 * of eth->qdma[], including entries the init 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)
> @@ -1687,6 +1778,11 @@ static void airoha_qdma_stop_napi(struct airoha_qdma *qdma)
>  {
>  	int i;
>  
> +	/* must not run or re-arm past this point: the work calls
> +	 * napi_disable() too, and doing that twice spins forever
> +	 */
> +	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..c195dad5ed58 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -207,6 +207,11 @@ struct airoha_queue {
>  	bool txq_stopped;
>  	bool flushing;
>  
> +	/* 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 +572,9 @@ struct airoha_qdma {
>  	struct airoha_queue q_tx[AIROHA_NUM_TX_RING];
>  	struct airoha_queue q_rx[AIROHA_NUM_RX_RING];
>  
> +	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 v3 0/2] net: airoha: fix silent RX loss on the shared CPU ring

[Vitaliy Sochnev]

v3: dropped the RX ring stall recovery (v2 2/3) as asked [1]. At 128 the
stall does not occur - 500 forced PPPoE reconnects over 20 h with the
detector compiled in and armed, zero triggers - and it did not fix the
bug on its own anyway. I will resend it if the stall turns up at the
larger ring.

2/2 keeps its code and its Acked-by; the commit message changed. It now
cites the register capture taken with no recovery in the tree instead of
numbers from builds carrying it, states that 1/2 does not cover this
failure, and quantifies what the bigger rings cost in memory.

v2 [2] answered the v1 review: DONE-bit overwrite hypothesis disproven,
QDMA_DESC_DROP_MASK never set, RX_DSCP_NUM default raised to the vendor
SDK's 32.

Still open for the airoha folks: in 2/2 hw set DONE on descriptor 15
while 0-14 were untouched and the driver's consumer sat at 0. Is
out-of-order completion within an RX ring expected, or is the driver
violating a constraint on RX_CPU_IDX by leaving one descriptor unposted?
Growing the ring avoids the symptom; the rule behind it is still unknown.

Tested on Nokia XG-040G-MF (AN7583) on a live PPPoE line. These two
patches without the recovery are what ran longest here: 508 forced
reconnects over 20 h 32 min, zero rx_dropped/rx_errors across 39158
samples.

[1] https://lore.kernel.org/netdev/apaA5jDYH71F0JaS@lore-desk/
[2] https://lore.kernel.org/netdev/20260831234701.206021-1-sochnev.v.74@gmail.com/

Vitaliy Sochnev (2):
  net: airoha: handle RX_NO_CPU_DSCP interrupt, not just RX_DONE
  net: airoha: grow the small RX rings

 drivers/net/ethernet/airoha/airoha_eth.c  | 16 +++++++++++-----
 drivers/net/ethernet/airoha/airoha_eth.h  |  3 ++-
 drivers/net/ethernet/airoha/airoha_regs.h |  2 ++
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.55.0

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

[Vitaliy Sochnev]

airoha_qdma_hw_init() unmasks the per-ring NO_CPU_DSCP interrupt, which
fires when an RX ring runs out of free CPU descriptors, but
airoha_irq_handler() only extracts the RX_DONE bits from the same status
word. The NO_CPU_DSCP bits are acknowledged and dropped.

Once a ring is drained to zero posted descriptors no further RX_DONE can
fire for it - nothing is left for hw to receive into - so NAPI is never
rescheduled, airoha_qdma_fill_rx_queue() is never called again, and the
ring stays dead until the interface is reconfigured.

This is reachable on RX ring 4, which airoha_fe_vip_setup() force-routes
~15 VIP-classified protocols onto (BOOTP, PPPoE Discovery, ISAKMP,
DHCPv6, SIP, LLDP, PPP LCP/IPCP/CHAP/PAP/IPv6CP, ...) while it sits on
the 16-descriptor RX_DSCP_NUM() default. A DHCP renewal or a PPPoE
negotiation burst drains it faster than the CPU reposts, after which all
of those protocols silently stop being received.

Treat NO_CPU_DSCP like RX_DONE for scheduling NAPI:
airoha_qdma_rx_process() already calls airoha_qdma_fill_rx_queue()
unconditionally at the end of every poll, so scheduling NAPI is enough
to make an emptied ring refill itself. Re-enable the bit alongside
RX_DONE in airoha_qdma_rx_napi_poll() so it is not left masked after
first use.

Whether NO_CPU_DSCP re-latches while the ring is still empty is not
documented. If it does, a ring that cannot be refilled (page_pool
returning NULL) keeps reasserting it. Masking it until a refill succeeds
is worse: a starved ring never fires RX_DONE either, so it would stay
dead after the memory pressure clears.

Fixes: f252493e1835 ("net: airoha: Enable multiple IRQ lines support in airoha_eth driver.")
Link: https://github.com/openwrt/openwrt/issues/24715
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c  | 16 +++++++++++-----
 drivers/net/ethernet/airoha/airoha_regs.h |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 64619e9a704d..c59201aded26 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -784,13 +784,16 @@ 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 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));
+					       intr_mask);
 		}
 	}
 
@@ -1468,16 +1471,19 @@ 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;
+	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_NO_CPU_DSCP_INT_RX1_MASK(rx_intr1);
 	}
 
-	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_DONE_HIGH_OFFSET) |
+				(rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK);
 	}
 
 	for (i = 0; rx_intr_mask && i < ARRAY_SIZE(qdma->q_rx); i++) {
diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
index 442b48c9b991..bde3d599d8b7 100644
--- a/drivers/net/ethernet/airoha/airoha_regs.h
+++ b/drivers/net/ethernet/airoha/airoha_regs.h
@@ -549,6 +549,8 @@
 #define INT_RX1_MASK(_n)							\
 	((((_n) << RX_NO_CPU_DSCP_LOW_OFFSET) & RX_NO_CPU_DSCP_LOW_INT_MASK) |	\
 	 (RX_DONE_LOW_INT_MASK & (_n)))
+#define RX_NO_CPU_DSCP_INT_RX1_MASK(_n)					\
+	(((_n) & RX_NO_CPU_DSCP_LOW_INT_MASK) >> RX_NO_CPU_DSCP_LOW_OFFSET)
 
 /* QDMA_CSR_INT_ENABLE3 */
 #define RX31_NO_CPU_DSCP_INT_MASK	BIT(31)
-- 
2.55.0

[PATCH net v3 2/2] net: airoha: grow the small RX rings

[Vitaliy Sochnev]

Raise the RX_DSCP_NUM() fallback from 16 to 32, the vendor SDK default,
and give ring 4 the 128 descriptors already used for rings 2/11/15.

Ring 4 is the shared "force to CPU" ring that airoha_fe_vip_setup()
routes BOOTP, PPPoE Discovery, PPP LCP/IPCP/CHAP/PAP/IPv6CP, ISAKMP,
DHCPv6, SIP and LLDP onto, so it sees the same bursty non-hashed traffic
as the rings already on 128 while sitting on the smallest allowance.
Both CDMs forward to it: REG_CDM_FWD_CFG(1) and (2) each set
CDM_VIP_QSEL_MASK to 4.

The NO_CPU_DSCP fix does not cover this. With it in place, at 16
descriptors the ring still dies during PPPoE negotiation on a Nokia
XG-040G-MF: over 6455 devmem samples in 60 s, REG_RX_DMA_IDX advanced
0x2c -> 0x52 while REG_RX_CPU_IDX stayed at 15, descriptor 15 had DONE
set on a 60-byte frame, descriptors 0-14 were empty, and q->tail was 0.
Hw had completed a descriptor the strictly sequential consumer in
airoha_qdma_rx_process() cannot reach, and negotiation never completes.
At 16 DHCP still works while PPPoE does not: a dial-up bursts Discovery,
LCP, IPCP and CHAP where DHCP sends one packet every ~150 s. With this
series, 508 forced reconnects over 20 h saw no recurrence.

RX_DONE_INT_MASK covers all 32 rings on both QDMA instances, so this
costs 544 more descriptors per instance, 1088 in total: 2.1 MiB of RX
buffers at PAGE_SIZE/2 each, plus 34 KiB of coherent descriptor memory.

Ring 4 and the VIP classification feeding it are shared driver-wide, with
no DT or hardware property distinguishing one variant's ring 4 from
another's, so this is not scoped to a particular SoC.

Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index fa9a8edce22f..7fc37f159d2b 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -41,9 +41,10 @@
 #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)
+	 (_n) ==  0 ? 1024 : 32)
 
 #define PSE_RSV_PAGES			128
 #define PSE_QUEUE_RSV_PAGES		64
-- 
2.55.0

Re: [PATCH net v2 2/3] net: airoha: recover RX ring after hw completion stall

[Vitaliy Sochnev]

> IIUC increasing the ring size the issue never occurs again (correct?). If so, I
> would drop this patch for the moment and add it if it is really necessary.

Correct. At 128 the stall never occurred: 500 forced PPPoE reconnects
over 20 h, ~37500 samples, zero triggers, plus ~994 cycles in earlier
runs. This detector was compiled in and armed throughout, so that is a
measured absence, not just the absence of a visible symptom.

It is also not sufficient on its own: at 16 descriptors the recovery
fires roughly every 35 s and PPPoE still never completes, since each
recovery flushes the ring and negotiation restarts. The ring size is
what fixes the bug - and the other two patches on their own are what
ran the longest here: 508 forced reconnects over 20 h 32 min, zero
rx_dropped/rx_errors across 39158 samples.

Dropped in v3, just sent. I will resend this one with evidence if the
stall ever turns up at the larger ring.

Thanks for the review.

Vitaliy