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