Re: [PATCH wireless 2/2] wifi: mt76: npu: read the rx slot count after the first slot is complete

[Lorenzo Bianconi <lorenzo@kernel.org>]

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