[PATCH wireless 0/2] wifi: mt76: npu: fix rx ring handling

[PATCH wireless 0/2] wifi: mt76: npu: fix rx ring handling

[Florian Maurer]

On a Gemtek W1700K (Airoha AN7581 + MT7996) with the NPU offload
enabled, 5 GHz rx load crashed the board within seconds. Patch 1 fixes
that crash.

With patch 1 applied, the two WARN_ON_ONCE() checks in
mt7996_mac_tx_free() fired on misframed rx frames. A debug patch counted
2-4 of these bogus events per 30 minutes. Patch 2 fixes the misframing.
It needs multi-slot frames, which were frequent here because of a patch
that leaves the de-aggregation of mesh A-MSDUs to mac80211 [1]. Without
that patch the race may be hard to hit.

Lorenzo, I have no documentation for the NPU. Everything here relies on
what I inferred from the code, so can you confirm these?
- dma_idx is the host's consumer index, and the NPU leaves one slot free
  in front of it.
- The NPU writes the info word before it sets DONE in ctrl.

Also, with patch 1 the NPU can no longer get slots without buffers, so
a lasting page pool failure can stall the ring until something else
polls it. mt76_dma_rx_poll() has the same behaviour, so I left it. I
haven't tested this case.

Tested on OpenWrt with kernel 6.18.55 and openwrt/mt76 at 467ae2e3. Its
npu.c differs from this tree only in mt76_npu_setup_tc_block_cb(). The
test image also carried [1], [2] and other local patches. The W1700K
was meshed with an ath11k node and ran iperf3 in both directions on 2.4
and 5 GHz for 60 minutes (5 GHz rx mostly around 220 Mbit/s). There was
no reboot, and neither WARN_ON_ONCE() fired. That image had no debug
patch to count bogus events directly.

Reset/restart: repeated wifi down/up rounds under about 30 Mbit/s of
rx load were clean, but they don't reach the rx queue reset that
patch 1 changes.

Recovery itself can't be tested on this board because of a separate
problem, which I don't think comes from these patches: a build without
them also became unreachable after `echo 0,8` into the mt7996
sys_recovery debugfs file (full system error recovery, the same path a
real firmware crash takes). I will report this separately.

With the patches, the same trigger wedges the board with and without
rx load: the MT7996 stops responding on PCIe (completion timeout), the
firmware reload fails, and the retry loop deadlocks in
mt76_worker_disable(). A sysrq dump shows kthread_park() blocked on a
TX worker kthread that is already parked. `echo 0,1` ended in the same
state.

On this tree the patches are only build-tested (arm64, W=1); the
above was tested on the 6.18.55 image. Not tested: a completed
recovery with patch 1, and MT7992.

The same code is also open as a pull request against the OpenWrt mt76
tree [3].

[1] https://github.com/openwrt/mt76/pull/1170
    ("wifi: mt76: mt7996: let mac80211 de-aggregate mesh A-MSDUs")
[2] https://github.com/openwrt/mt76/pull/1132
    ("wifi: mt76: mt7996: use own MAC as BSS BSSID for mesh interfaces")
[3] https://github.com/openwrt/mt76/pull/1169

Florian Maurer (2):
  wifi: mt76: npu: return rx slots to the NPU only after refilling them
  wifi: mt76: npu: read the rx slot count after the first slot is
    complete

 drivers/net/wireless/mediatek/mt76/npu.c | 28 +++++++++++++++++++-----
 1 file changed, 22 insertions(+), 6 deletions(-)


base-commit: c4328134f3f7c2ce141121a8f82a5ec0cc8a709c
-- 
2.47.3

[PATCH wireless 1/2] wifi: mt76: npu: return rx slots to the NPU only after refilling them

[Florian Maurer]

mt76_npu_dequeue() writes the new tail to the dma_idx register of the
NPU rx ring as soon as it has consumed a frame, while the slots are
only refilled with new buffers by mt76_npu_rx_poll() after the whole
NAPI budget. Until then, the descriptors of the released slots still
point to the buffers that were passed up the stack, and on the first
round of the ring the slot that was never filled points to address 0.
If the NPU completes one of these slots, the host processes an entry
without buffer, which crashes in dma_sync_single_for_cpu() under heavy
rx load:

  Unable to handle kernel paging request at virtual address ffffffff80000700
  pc : dcache_inval_poc+0x28/0x58
  lr : arch_sync_dma_for_cpu+0x28/0x38
  Call trace:
   dcache_inval_poc+0x28/0x58 (P)
   __dma_sync_single_for_cpu+0xe4/0xf8
   mt76_npu_rx_poll+0x184/0x390 [mt76]
   __napi_poll+0x34/0x180

Write dma_idx after refilling the ring instead, up to the slot kept
free in front of the tail, which is the same value as before once the
ring is full.

Also check that all slots of a frame are queued and complete before
building the skb. So far a frame whose later slots were not yet
done was freed together with the buffers of its first slots, which
were then used again on the next poll.

Fixes: 7fb554b1b623 ("wifi: mt76: Introduce the NPU generic layer")
Cc: stable@vger.kernel.org
Signed-off-by: Florian Maurer <fmaurer@disroot.org>
---
 drivers/net/wireless/mediatek/mt76/npu.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/npu.c b/drivers/net/wireless/mediatek/mt76/npu.c
index c4c7c0af6321..6e503a4a890c 100644
--- a/drivers/net/wireless/mediatek/mt76/npu.c
+++ b/drivers/net/wireless/mediatek/mt76/npu.c
@@ -40,6 +40,11 @@ int mt76_npu_fill_rx_queue(struct mt76_dev *dev, struct mt76_queue *q)
 		nframes++;
 	}
 
+	/* Hand the slots back to the NPU only once they have a new buffer,
+	 * up to the one kept free in front of the tail.
+	 */
+	Q_WRITE(q, dma_idx, (q->head + 1) % q->ndesc);
+
 	return nframes;
 }
 
@@ -69,17 +74,22 @@ static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
 
 	nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, desc[index].info);
 	nframes = max_t(int, nframes, 1);
+	if (nframes > q->queued)
+		return NULL;
+
+	/* Only take the frame once all of its slots are complete */
+	for (i = 0; i < nframes; i++) {
+		int idx = (index + i) % q->ndesc;
+
+		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[idx].ctrl))
+			return NULL;
+	}
 
 	for (i = 0; i < nframes; i++) {
 		struct mt76_queue_entry *e = &q->entry[index];
 		int len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK,
 				    desc[index].ctrl);
 
-		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[index].ctrl)) {
-			dev_kfree_skb(skb);
-			return NULL;
-		}
-
 		dma_sync_single_for_cpu(dev->dma_dev, e->dma_addr[0],
 					e->dma_len[0],
 					page_pool_get_dma_dir(q->page_pool));
@@ -108,7 +118,6 @@ static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
 	}
 	q->tail = index;
 	q->queued -= i;
-	Q_WRITE(q, dma_idx, q->tail);
 
 	return skb;
 }
-- 
2.47.3

Re: [PATCH wireless 1/2] wifi: mt76: npu: return rx slots to the NPU only after refilling them

[Lorenzo Bianconi]

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

> mt76_npu_dequeue() writes the new tail to the dma_idx register of the
> NPU rx ring as soon as it has consumed a frame, while the slots are
> only refilled with new buffers by mt76_npu_rx_poll() after the whole
> NAPI budget. Until then, the descriptors of the released slots still
> point to the buffers that were passed up the stack, and on the first
> round of the ring the slot that was never filled points to address 0.
> If the NPU completes one of these slots, the host processes an entry
> without buffer, which crashes in dma_sync_single_for_cpu() under heavy
> rx load:
> 
>   Unable to handle kernel paging request at virtual address ffffffff80000700
>   pc : dcache_inval_poc+0x28/0x58
>   lr : arch_sync_dma_for_cpu+0x28/0x38
>   Call trace:
>    dcache_inval_poc+0x28/0x58 (P)
>    __dma_sync_single_for_cpu+0xe4/0xf8
>    mt76_npu_rx_poll+0x184/0x390 [mt76]
>    __napi_poll+0x34/0x180
> 
> Write dma_idx after refilling the ring instead, up to the slot kept
> free in front of the tail, which is the same value as before once the
> ring is full.
> 
> Also check that all slots of a frame are queued and complete before
> building the skb. So far a frame whose later slots were not yet
> done was freed together with the buffers of its first slots, which
> were then used again on the next poll.
> 
> Fixes: 7fb554b1b623 ("wifi: mt76: Introduce the NPU generic layer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Florian Maurer <fmaurer@disroot.org>
> ---
>  drivers/net/wireless/mediatek/mt76/npu.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/npu.c b/drivers/net/wireless/mediatek/mt76/npu.c
> index c4c7c0af6321..6e503a4a890c 100644
> --- a/drivers/net/wireless/mediatek/mt76/npu.c
> +++ b/drivers/net/wireless/mediatek/mt76/npu.c
> @@ -40,6 +40,11 @@ int mt76_npu_fill_rx_queue(struct mt76_dev *dev, struct mt76_queue *q)
>  		nframes++;
>  	}
>  
> +	/* Hand the slots back to the NPU only once they have a new buffer,
> +	 * up to the one kept free in front of the tail.
> +	 */
> +	Q_WRITE(q, dma_idx, (q->head + 1) % q->ndesc);

why are you writing q->head here?

> +
>  	return nframes;
>  }
>  
> @@ -69,17 +74,22 @@ static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
>  
>  	nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, desc[index].info);
>  	nframes = max_t(int, nframes, 1);
> +	if (nframes > q->queued)
> +		return NULL;

can this really occur?

> +
> +	/* Only take the frame once all of its slots are complete */
> +	for (i = 0; i < nframes; i++) {
> +		int idx = (index + i) % q->ndesc;
> +
> +		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[idx].ctrl))
> +			return NULL;
> +	}

I got your time, and I agree this is an issue we need to fix but I guess we
should avoid a double loop. What about the patch below? (just compiled and not
tested).

Regards,
Lorenzo

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 2ade848fe3bb..9537c6d28e8d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -249,6 +249,7 @@ struct mt76_queue {
 	int ndesc;
 	int queued;
 	int buf_size;
+	int pending;
 	bool stopped;
 	bool blocked;
 
diff --git a/drivers/net/wireless/mediatek/mt76/npu.c b/drivers/net/wireless/mediatek/mt76/npu.c
index c4c7c0af6321..e2fc361a5248 100644
--- a/drivers/net/wireless/mediatek/mt76/npu.c
+++ b/drivers/net/wireless/mediatek/mt76/npu.c
@@ -56,61 +56,74 @@ void mt76_npu_queue_cleanup(struct mt76_dev *dev, struct mt76_queue *q)
 		q->tail = (q->tail + 1) % q->ndesc;
 		q->queued--;
 	}
+	if (q->rx_head) {
+		dev_kfree_skb(q->rx_head);
+		q->rx_head = NULL;
+		q->pending = 0;
+	}
 	spin_unlock_bh(&q->lock);
 }
 
-static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
-					struct mt76_queue *q,
-					u32 *info)
+static int mt76_npu_rx_process_entry(struct napi_struct *napi)
 {
+	struct mt76_dev *dev = mt76_priv(napi->dev);
+	enum mt76_rxq_id qid = napi - dev->napi;
+	struct mt76_queue *q = &dev->q_rx[qid];
 	struct airoha_npu_rx_dma_desc *desc = (void *)q->desc;
-	int i, nframes, index = q->tail;
-	struct sk_buff *skb = NULL;
+	int nframes = q->pending;
+	u32 info = 0;
 
-	nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, desc[index].info);
-	nframes = max_t(int, nframes, 1);
+	if (!nframes) {
+		nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK,
+				    desc[q->tail].info);
+		nframes = max_t(int, nframes, 1);
+	}
 
-	for (i = 0; i < nframes; i++) {
-		struct mt76_queue_entry *e = &q->entry[index];
+	while (nframes > 0) {
+		struct mt76_queue_entry *e = &q->entry[q->tail];
 		int len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK,
-				    desc[index].ctrl);
+				    desc[q->tail].ctrl);
 
-		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[index].ctrl)) {
-			dev_kfree_skb(skb);
-			return NULL;
-		}
+		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[q->tail].ctrl))
+			return -EBUSY;
 
 		dma_sync_single_for_cpu(dev->dma_dev, e->dma_addr[0],
 					e->dma_len[0],
 					page_pool_get_dma_dir(q->page_pool));
 
-		if (!skb) {
-			skb = napi_build_skb(e->buf, q->buf_size);
-			if (!skb)
-				return NULL;
+		if (!q->rx_head) {
+			q->rx_head = napi_build_skb(e->buf, q->buf_size);
+			if (!q->rx_head)
+				return -ENOMEM;
 
-			__skb_put(skb, len);
-			skb_reset_mac_header(skb);
-			skb_mark_for_recycle(skb);
+			__skb_put(q->rx_head, len);
+			skb_reset_mac_header(q->rx_head);
+			skb_mark_for_recycle(q->rx_head);
 		} else {
-			struct skb_shared_info *shinfo = skb_shinfo(skb);
+			struct skb_shared_info *shinfo = skb_shinfo(q->rx_head);
 			struct page *page = virt_to_head_page(e->buf);
 			int nr_frags = shinfo->nr_frags;
 
 			if (nr_frags < ARRAY_SIZE(shinfo->frags))
-				skb_add_rx_frag(skb, nr_frags, page,
+				skb_add_rx_frag(q->rx_head, nr_frags, page,
 						e->buf - page_address(page),
 						len, q->buf_size);
 		}
 
-		*info = desc[index].info;
-		index = (index + 1) % q->ndesc;
+		info = desc[q->tail].info;
+		q->tail = (q->tail + 1) % q->ndesc;
+		q->queued--;
+		nframes--;
 	}
-	q->tail = index;
-	q->queued -= i;
+
+	dev->drv->rx_skb(dev, qid, q->rx_head, &info);
+	mt76_rx_poll_complete(dev, qid, napi);
+	q->rx_head = NULL;
+	q->pending = 0;
+
 	Q_WRITE(q, dma_idx, q->tail);
 
-	return skb;
+	return 0;
 }
 
 void mt76_npu_check_ppe(struct mt76_dev *dev, struct sk_buff *skb,
@@ -155,18 +168,10 @@ static int mt76_npu_rx_poll(struct napi_struct *napi, int budget)
 		goto out;
 
 	while (done < budget) {
-		struct sk_buff *skb;
-		u32 info = 0;
-
-		skb = mt76_npu_dequeue(dev, &dev->q_rx[qid], &info);
-		if (!skb)
+		if (mt76_npu_rx_process_entry(napi))
 			break;
-
-		dev->drv->rx_skb(dev, qid, skb, &info);
-		mt76_rx_poll_complete(dev, qid, napi);
 		done++;
 	}
-
 	mt76_npu_fill_rx_queue(dev, &dev->q_rx[qid]);
 out:
 	if (done < budget && napi_complete(napi))

>  
>  	for (i = 0; i < nframes; i++) {
>  		struct mt76_queue_entry *e = &q->entry[index];
>  		int len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK,
>  				    desc[index].ctrl);
>  
> -		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[index].ctrl)) {
> -			dev_kfree_skb(skb);
> -			return NULL;
> -		}
> -
>  		dma_sync_single_for_cpu(dev->dma_dev, e->dma_addr[0],
>  					e->dma_len[0],
>  					page_pool_get_dma_dir(q->page_pool));
> @@ -108,7 +118,6 @@ static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
>  	}
>  	q->tail = index;
>  	q->queued -= i;
> -	Q_WRITE(q, dma_idx, q->tail);
>  
>  	return skb;
>  }
> -- 
> 2.47.3
> 

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

Re: [PATCH wireless 1/2] wifi: mt76: npu: return rx slots to the NPU only after refilling them

[Florian Maurer]

On Fri, Nov 06, 2026 at 03:21:52PM +0100, Lorenzo Bianconi wrote:
>> +	Q_WRITE(q, dma_idx, (q->head + 1) % q->ndesc);
>
> why are you writing q->head here?

Once the ring is full, (q->head + 1) % q->ndesc == q->tail, so the
value is the same as today. Only the timing changes: the slots go back
to the NPU after they have a new buffer, not before.

The +1 assumes the NPU completes slots only up to dma_idx - 2. Every
slot it can still complete then lies between its own position and
q->head - 1. The host has not consumed those yet, so they still have a
buffer. If the refill fails, the same value is written again and the
ring stalls instead of handing back an empty slot. A debug patch that
cleared DONE after each consumed slot saw no slot completed early in
60 minutes of rx load, though I can't tell how often the NPU reached
that slot. Can you confirm this is how the NPU treats dma_idx?

>> +	if (nframes > q->queued)
>> +		return NULL;
>
> can this really occur?

I have not seen it. The slot at q->head is never cleared while the ring
is full: it is the one kept free, consumed in the previous round, so it
still has DONE set. If the NPU has completed the first slots of a frame
up to q->head - 1, the count reaches past q->queued and the next DONE
check finds that stale bit. A page pool failure makes the stale region
larger.

> I got your time, and I agree this is an issue we need to fix but I guess we
> should avoid a double loop. What about the patch below? (just compiled and not
> tested).

Keeping the partial frame in q->rx_head is nicer, and it lets the slots
of a half-received frame be refilled early. Comments inline.

> +	if (!nframes) {
> +		nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK,
> +				    desc[q->tail].info);
> +		nframes = max_t(int, nframes, 1);
> +	}

PKT_COUNT is read before DONE of this slot, see my reply to 2/2. Once
q->pending keeps the count across polls, a stale count of 1 would
stick. On top of your patch, something like (not compiled):

	if (!nframes) {
		u32 ctrl = READ_ONCE(desc[q->tail].ctrl);

		if (!q->queued || !FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, ctrl))
			return -EBUSY;

		/* PKT_COUNT is only valid once the first slot is complete */
		dma_rmb();
		nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK,
				    desc[q->tail].info);
		nframes = max_t(int, nframes, 1);
	}

> +	while (nframes > 0) {
> +		struct mt76_queue_entry *e = &q->entry[q->tail];
>  		int len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK,
> -				    desc[index].ctrl);
> +				    desc[q->tail].ctrl);
[...]
> +		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[q->tail].ctrl))
> +			return -EBUSY;

Three things here:

- ctrl is loaded twice, so len can come from before the slot was
  completed.
- Nothing stops the loop at q->queued == 0, so it can take the stale
  slot at q->head, whose buffer already belongs to an skb, and
  q->queued goes negative.
- q->pending is never set, so after -EBUSY the next poll reads
  PKT_COUNT from desc[q->tail].info, which is now a later slot of the
  same frame.

So:

		u32 ctrl = READ_ONCE(desc[q->tail].ctrl);
		int len;

		if (!q->queued || !FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, ctrl)) {
			q->pending = nframes;
			return -EBUSY;
		}

		dma_rmb();
		len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK, ctrl);

[...]
>  	Q_WRITE(q, dma_idx, q->tail);

This still hands the slots back before mt76_npu_fill_rx_queue() refills
them, so the oops is still there. It needs to stay at the end of the
refill as in my patch. With your structure that also lets a frame that
runs into the free slot finish, since the NPU gets new slots after
every poll.

I can fold this into a v2, test it on the W1700K for 60 minutes under
rx load, this time with a counter for misframed frames, and send it
with the results. Since the structure is yours, I'd add
Co-developed-by and your Signed-off-by if that's fine with you. If
you'd rather post it yourself, I'll test yours instead.

Thanks,
Florian

Re: [PATCH wireless 1/2] wifi: mt76: npu: return rx slots to the NPU only after refilling them

[Lorenzo Bianconi]

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

> On Fri, Nov 06, 2026 at 03:21:52PM +0100, Lorenzo Bianconi wrote:
> >> +	Q_WRITE(q, dma_idx, (q->head + 1) % q->ndesc);
> >
> > why are you writing q->head here?
> 
> Once the ring is full, (q->head + 1) % q->ndesc == q->tail, so the
> value is the same as today. Only the timing changes: the slots go back
> to the NPU after they have a new buffer, not before.
> 
> The +1 assumes the NPU completes slots only up to dma_idx - 2. Every

I guess this is used to mark what is the next index the mt76 driver is
expecting but I guess airoha folks can provide more details about it.

> slot it can still complete then lies between its own position and
> q->head - 1. The host has not consumed those yet, so they still have a
> buffer. If the refill fails, the same value is written again and the
> ring stalls instead of handing back an empty slot. A debug patch that
> cleared DONE after each consumed slot saw no slot completed early in
> 60 minutes of rx load, though I can't tell how often the NPU reached
> that slot. Can you confirm this is how the NPU treats dma_idx?

q->head and q->tail are not the same when we have refilled the queue with new
buffers. Moreover, I think if mt76_npu_fill_rx_queue() fails (so mt76_get_page_pool_buf()
returns NULL) it is not correct to use q->head for dma_idx, we should use q->tail.

> 
> >> +	if (nframes > q->queued)
> >> +		return NULL;

Can you please check the updated patch below?

> >
> > can this really occur?
> 
> I have not seen it. The slot at q->head is never cleared while the ring
> is full: it is the one kept free, consumed in the previous round, so it
> still has DONE set. If the NPU has completed the first slots of a frame
> up to q->head - 1, the count reaches past q->queued and the next DONE
> check finds that stale bit. A page pool failure makes the stale region
> larger.
> 
> > I got your time, and I agree this is an issue we need to fix but I guess we
> > should avoid a double loop. What about the patch below? (just compiled and not
> > tested).
> 
> Keeping the partial frame in q->rx_head is nicer, and it lets the slots
> of a half-received frame be refilled early. Comments inline.
> 
> > +	if (!nframes) {
> > +		nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK,
> > +				    desc[q->tail].info);
> > +		nframes = max_t(int, nframes, 1);
> > +	}
> 
> PKT_COUNT is read before DONE of this slot, see my reply to 2/2. Once
> q->pending keeps the count across polls, a stale count of 1 would
> stick. On top of your patch, something like (not compiled):
> 
> 	if (!nframes) {
> 		u32 ctrl = READ_ONCE(desc[q->tail].ctrl);
> 
> 		if (!q->queued || !FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, ctrl))
> 			return -EBUSY;
> 
> 		/* PKT_COUNT is only valid once the first slot is complete */
> 		dma_rmb();
> 		nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK,
> 				    desc[q->tail].info);
> 		nframes = max_t(int, nframes, 1);

ack, I can fix it.

> 	}
> 
> > +	while (nframes > 0) {
> > +		struct mt76_queue_entry *e = &q->entry[q->tail];
> >  		int len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK,
> > -				    desc[index].ctrl);
> > +				    desc[q->tail].ctrl);
> [...]
> > +		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[q->tail].ctrl))
> > +			return -EBUSY;
> 
> Three things here:
> 
> - ctrl is loaded twice, so len can come from before the slot was
>   completed.

ack, I can fix it.

> - Nothing stops the loop at q->queued == 0, so it can take the stale
>   slot at q->head, whose buffer already belongs to an skb, and
>   q->queued goes negative.

please see above

> - q->pending is never set, so after -EBUSY the next poll reads
>   PKT_COUNT from desc[q->tail].info, which is now a later slot of the
>   same frame.

I can fix it.

> 
> So:
> 
> 		u32 ctrl = READ_ONCE(desc[q->tail].ctrl);
> 		int len;
> 
> 		if (!q->queued || !FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, ctrl)) {
> 			q->pending = nframes;
> 			return -EBUSY;
> 		}
> 
> 		dma_rmb();
> 		len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK, ctrl);
> 
> [...]
> >  	Q_WRITE(q, dma_idx, q->tail);
> 
> This still hands the slots back before mt76_npu_fill_rx_queue() refills
> them, so the oops is still there. It needs to stay at the end of the
> refill as in my patch. With your structure that also lets a frame that
> runs into the free slot finish, since the NPU gets new slots after
> every poll.

I guess q->tail should point to the next already allocated buffer, right? The
only corner case is when we consume the all the allocated buffers. In this case
we should wait to update it. Do you agree? Can you please check the updated
patch below?

> 
> I can fold this into a v2, test it on the W1700K for 60 minutes under
> rx load, this time with a counter for misframed frames, and send it
> with the results. Since the structure is yours, I'd add
> Co-developed-by and your Signed-off-by if that's fine with you. If
> you'd rather post it yourself, I'll test yours instead.

Sure, feel free to add your Co-developed-by tag if the patch below works.

Regards,
Lorenzo

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 2ade848fe3bb..9537c6d28e8d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -249,6 +249,7 @@ struct mt76_queue {
 	int ndesc;
 	int queued;
 	int buf_size;
+	int pending;
 	bool stopped;
 	bool blocked;
 
diff --git a/drivers/net/wireless/mediatek/mt76/npu.c b/drivers/net/wireless/mediatek/mt76/npu.c
index c4c7c0af6321..3ca83985376d 100644
--- a/drivers/net/wireless/mediatek/mt76/npu.c
+++ b/drivers/net/wireless/mediatek/mt76/npu.c
@@ -56,61 +56,81 @@ void mt76_npu_queue_cleanup(struct mt76_dev *dev, struct mt76_queue *q)
 		q->tail = (q->tail + 1) % q->ndesc;
 		q->queued--;
 	}
+	if (q->rx_head) {
+		dev_kfree_skb(q->rx_head);
+		q->rx_head = NULL;
+		q->pending = 0;
+	}
 	spin_unlock_bh(&q->lock);
 }
 
-static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
-					struct mt76_queue *q,
-					u32 *info)
+static int mt76_npu_rx_process_entry(struct napi_struct *napi)
 {
+	struct mt76_dev *dev = mt76_priv(napi->dev);
+	enum mt76_rxq_id qid = napi - dev->napi;
+	struct mt76_queue *q = &dev->q_rx[qid];
 	struct airoha_npu_rx_dma_desc *desc = (void *)q->desc;
-	int i, nframes, index = q->tail;
-	struct sk_buff *skb = NULL;
+	int nframes = q->pending;
+	u32 info = 0;
+
+	do {
+		struct mt76_queue_entry *e = &q->entry[q->tail];
+		u32 ctrl = READ_ONCE(desc[q->tail].ctrl);
+		int len;
 
-	nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, desc[index].info);
-	nframes = max_t(int, nframes, 1);
+		if (!q->queued) {
+			q->pending = nframes;
+			return -EBUSY;
+		}
 
-	for (i = 0; i < nframes; i++) {
-		struct mt76_queue_entry *e = &q->entry[index];
-		int len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK,
-				    desc[index].ctrl);
+		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, ctrl)) {
+			q->pending = nframes;
+			return -EBUSY;
+		}
 
-		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[index].ctrl)) {
-			dev_kfree_skb(skb);
-			return NULL;
+		dma_rmb();
+		len = FIELD_GET(NPU_RX_DMA_DESC_CUR_LEN_MASK, ctrl);
+		info = desc[q->tail].info;
+		if (nframes == q->pending && !nframes) {
+			nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, info);
+			nframes = max_t(int, nframes, 1);
 		}
 
 		dma_sync_single_for_cpu(dev->dma_dev, e->dma_addr[0],
 					e->dma_len[0],
 					page_pool_get_dma_dir(q->page_pool));
 
-		if (!skb) {
-			skb = napi_build_skb(e->buf, q->buf_size);
-			if (!skb)
-				return NULL;
+		if (!q->rx_head) {
+			q->rx_head = napi_build_skb(e->buf, q->buf_size);
+			if (!q->rx_head)
+				return -ENOMEM;
 
-			__skb_put(skb, len);
-			skb_reset_mac_header(skb);
-			skb_mark_for_recycle(skb);
+			__skb_put(q->rx_head, len);
+			skb_reset_mac_header(q->rx_head);
+			skb_mark_for_recycle(q->rx_head);
 		} else {
-			struct skb_shared_info *shinfo = skb_shinfo(skb);
+			struct skb_shared_info *shinfo = skb_shinfo(q->rx_head);
 			struct page *page = virt_to_head_page(e->buf);
 			int nr_frags = shinfo->nr_frags;
 
 			if (nr_frags < ARRAY_SIZE(shinfo->frags))
-				skb_add_rx_frag(skb, nr_frags, page,
+				skb_add_rx_frag(q->rx_head, nr_frags, page,
 						e->buf - page_address(page),
 						len, q->buf_size);
 		}
 
-		*info = desc[index].info;
-		index = (index + 1) % q->ndesc;
-	}
-	q->tail = index;
-	q->queued -= i;
+		q->tail = (q->tail + 1) % q->ndesc;
+		q->queued--;
+	} while (--nframes > 0);
+
+	dev->drv->rx_skb(dev, qid, q->rx_head, &info);
+	mt76_rx_poll_complete(dev, qid, napi);
+	q->rx_head = NULL;
+	q->pending = 0;
+
 	Q_WRITE(q, dma_idx, q->tail);
 
-	return skb;
+	return 0;
 }
 
 void mt76_npu_check_ppe(struct mt76_dev *dev, struct sk_buff *skb,
@@ -155,18 +175,10 @@ static int mt76_npu_rx_poll(struct napi_struct *napi, int budget)
 		goto out;
 
 	while (done < budget) {
-		struct sk_buff *skb;
-		u32 info = 0;
-
-		skb = mt76_npu_dequeue(dev, &dev->q_rx[qid], &info);
-		if (!skb)
+		if (mt76_npu_rx_process_entry(napi))
 			break;
-
-		dev->drv->rx_skb(dev, qid, skb, &info);
-		mt76_rx_poll_complete(dev, qid, napi);
 		done++;
 	}
-
 	mt76_npu_fill_rx_queue(dev, &dev->q_rx[qid]);
 out:
 	if (done < budget && napi_complete(napi))

> 
> Thanks,
> Florian

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

[PATCH wireless 2/2] wifi: mt76: npu: read the rx slot count after the first slot is complete

[Florian Maurer]

mt76_npu_dequeue() reads the slot count of a frame from the info word
of its first descriptor before checking the done bit of that slot. If
the NPU completes the slot in between, the count is still the 0 left
by the refill, so a frame spanning two slots is taken as a single slot
frame and its second slot is parsed as the start of the next frame.
With mt7996 this shows up as bogus tx-free events, which hit the
WARN_ON_ONCE() checks in mt7996_mac_tx_free(). From the code, such an
event can also release tokens that are still in use.

Check the done bit of the first slot before reading the slot count,
and order the descriptor reads with dma_rmb().

Fixes: 7fb554b1b623 ("wifi: mt76: Introduce the NPU generic layer")
Cc: stable@vger.kernel.org
Signed-off-by: Florian Maurer <fmaurer@disroot.org>
---
 drivers/net/wireless/mediatek/mt76/npu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/npu.c b/drivers/net/wireless/mediatek/mt76/npu.c
index 6e503a4a890c..f00ae683a7eb 100644
--- a/drivers/net/wireless/mediatek/mt76/npu.c
+++ b/drivers/net/wireless/mediatek/mt76/npu.c
@@ -72,18 +72,25 @@ static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
 	int i, nframes, index = q->tail;
 	struct sk_buff *skb = NULL;
 
+	if (!q->queued ||
+	    !FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[index].ctrl))
+		return NULL;
+
+	/* The slot count is only valid once the first slot is complete */
+	dma_rmb();
 	nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, desc[index].info);
 	nframes = max_t(int, nframes, 1);
 	if (nframes > q->queued)
 		return NULL;
 
 	/* Only take the frame once all of its slots are complete */
-	for (i = 0; i < nframes; i++) {
+	for (i = 1; i < nframes; i++) {
 		int idx = (index + i) % q->ndesc;
 
 		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[idx].ctrl))
 			return NULL;
 	}
+	dma_rmb();
 
 	for (i = 0; i < nframes; i++) {
 		struct mt76_queue_entry *e = &q->entry[index];
-- 
2.47.3

Re: [PATCH wireless 2/2] wifi: mt76: npu: read the rx slot count after the first slot is complete

[Lorenzo Bianconi]

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

> mt76_npu_dequeue() reads the slot count of a frame from the info word
> of its first descriptor before checking the done bit of that slot. If
> the NPU completes the slot in between, the count is still the 0 left
> by the refill, so a frame spanning two slots is taken as a single slot
> frame and its second slot is parsed as the start of the next frame.
> With mt7996 this shows up as bogus tx-free events, which hit the
> WARN_ON_ONCE() checks in mt7996_mac_tx_free(). From the code, such an
> event can also release tokens that are still in use.
> 
> Check the done bit of the first slot before reading the slot count,
> and order the descriptor reads with dma_rmb().

Relying on the patch I shared for 1/2, I guess this is no more a problem since
we will just delay the skb allocation. Agree or am I missing something?

Regards,
Lorenzo

> 
> Fixes: 7fb554b1b623 ("wifi: mt76: Introduce the NPU generic layer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Florian Maurer <fmaurer@disroot.org>
> ---
>  drivers/net/wireless/mediatek/mt76/npu.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/npu.c b/drivers/net/wireless/mediatek/mt76/npu.c
> index 6e503a4a890c..f00ae683a7eb 100644
> --- a/drivers/net/wireless/mediatek/mt76/npu.c
> +++ b/drivers/net/wireless/mediatek/mt76/npu.c
> @@ -72,18 +72,25 @@ static struct sk_buff *mt76_npu_dequeue(struct mt76_dev *dev,
>  	int i, nframes, index = q->tail;
>  	struct sk_buff *skb = NULL;
>  
> +	if (!q->queued ||
> +	    !FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[index].ctrl))
> +		return NULL;
> +
> +	/* The slot count is only valid once the first slot is complete */
> +	dma_rmb();
>  	nframes = FIELD_GET(NPU_RX_DMA_PKT_COUNT_MASK, desc[index].info);
>  	nframes = max_t(int, nframes, 1);
>  	if (nframes > q->queued)
>  		return NULL;
>  
>  	/* Only take the frame once all of its slots are complete */
> -	for (i = 0; i < nframes; i++) {
> +	for (i = 1; i < nframes; i++) {
>  		int idx = (index + i) % q->ndesc;
>  
>  		if (!FIELD_GET(NPU_RX_DMA_DESC_DONE_MASK, desc[idx].ctrl))
>  			return NULL;
>  	}
> +	dma_rmb();
>  
>  	for (i = 0; i < nframes; i++) {
>  		struct mt76_queue_entry *e = &q->entry[index];
> -- 
> 2.47.3
> 

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

Re: [PATCH wireless 2/2] wifi: mt76: npu: read the rx slot count after the first slot is complete

[Florian Maurer]

On Fri, Nov 06, 2026 at 03:26:36PM +0100, Lorenzo Bianconi wrote:
> Relying on the patch I shared for 1/2, I guess this is no more a problem since
> we will just delay the skb allocation. Agree or am I missing something?

I think the race is still there. It is in the order of the two
descriptor reads, not in the skb allocation: your version still reads
PKT_COUNT from desc[q->tail].info first and checks DONE of that slot
afterwards. If the NPU completes the slot between the two reads, the
count is the 0 left by the memset in the refill, max_t() turns it into
1, and the DONE check then passes. A two-slot frame is delivered as one
slot, and its second slot is taken as the start of the next frame.

That matches what I saw on the W1700K: with patch 1 alone, 2-4 bogus
tx-free events per 30 minutes, each 2792 bytes over two slots
(1000 + 1792), i.e. the last slot of one frame plus the first slot of
the next. With patch 2, the same debug counter saw none in 60 minutes.

It gets worse once q->pending keeps the count across polls, as long as
the count is still read before DONE: a poll nearly always ends on a
first slot that is not done yet, so the frame that starts the next poll
would usually be stored with a count of 1. Checking DONE of the first
slot, then dma_rmb(), then reading PKT_COUNT avoids both. I sketched
it in my reply to 1/2:

https://lore.kernel.org/linux-wireless/20261108205350.2956138-1-fmaurer@disroot.org/

and v2 would carry it as one patch on top of your structure.

Thanks,
Florian