[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

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