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