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

[Lorenzo Bianconi]

> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Florian Maurer <fmaurer@xxxxxxxxxxx>
> ---
>  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: signature.asc
Description: PGP signature