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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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