[PATCH v2] [net] net: airoha: Clean up RX queues in airoha_dev_stop

[PATCH v2] [net] net: airoha: Clean up RX queues in airoha_dev_stop

[Wayen Yan]

When the last port is stopped, airoha_dev_stop() clears TX queues
but neglects to clean up RX queues. This can lead to:
- RX ring buffer descriptors remaining valid after device close
- Potential DMA synchronization issues on device reopen
- Risk of use-after-free if pages are freed while DMA is still active

Add cleanup loop for RX queues to mirror the TX queue cleanup,
ensuring symmetric resource management.

Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
Signed-off-by: Wayen Yan <win847@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 31cdb11cd7..9ca5bbf64d 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -1771,6 +1771,13 @@ static int airoha_dev_stop(struct net_device *dev)
 
 			airoha_qdma_cleanup_tx_queue(&qdma->q_tx[i]);
 		}
+
+		for (i = 0; i < ARRAY_SIZE(qdma->q_rx); i++) {
+			if (!qdma->q_rx[i].ndesc)
+				continue;
+
+			airoha_qdma_cleanup_rx_queue(&qdma->q_rx[i]);
+		}
 	}
 
 	return 0;
-- 
2.51.0

Re: [PATCH v2] [net] net: airoha: Clean up RX queues in airoha_dev_stop

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

On Jun 16, Wayen Yan wrote:
> When the last port is stopped, airoha_dev_stop() clears TX queues
> but neglects to clean up RX queues. This can lead to:
> - RX ring buffer descriptors remaining valid after device close
> - Potential DMA synchronization issues on device reopen
> - Risk of use-after-free if pages are freed while DMA is still active
> 
> Add cleanup loop for RX queues to mirror the TX queue cleanup,
> ensuring symmetric resource management.
> 
> Fixes: 23020f049327 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> Signed-off-by: Wayen Yan <win847@gmail.com>

when you send a new revision:
- please add a note of what you changed with respect to the previous one.
- please give some time to reviewers to take a look to the previous revision.

> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 31cdb11cd7..9ca5bbf64d 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1771,6 +1771,13 @@ static int airoha_dev_stop(struct net_device *dev)
>  
>  			airoha_qdma_cleanup_tx_queue(&qdma->q_tx[i]);
>  		}
> +
> +		for (i = 0; i < ARRAY_SIZE(qdma->q_rx); i++) {
> +			if (!qdma->q_rx[i].ndesc)
> +				continue;
> +
> +			airoha_qdma_cleanup_rx_queue(&qdma->q_rx[i]);
> +		}
>  	}

I do not think this patch is needed since there is no point to remove all the
RX buffers from the hw RX queues stopping the device, this is necessary just
removing the module (I think we can avoid it for TX too, I have a patch for it
I need to post).
Moreover, doing so, when the device is opened again, RX queues will be empty.

Regards,
Lorenzo

>  
>  	return 0;
> -- 
> 2.51.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] [net] net: airoha: Clean up RX queues in airoha_dev_stop

[Wayen Yan]

Thanks Simon for forwarding the AI review. I've reviewed all three
concerns:

#1 (NAPI race) and #2 (RX refill) are valid. #2 is the decisive
issue: airoha_dev_open() has no RX ring refill, so draining the
queues in stop would cause RX stall on next open. This aligns with
Lorenzo's earlier feedback — RX queues don't need cleanup in
dev_stop(). I'll drop this patch.

#3 (q->skb leak) is a pre-existing issue, not introduced by this
patch. It exists even in the module unload path
(airoha_qdma_cleanup()). @Lorenzo — do you think this warrants a
fix? A one-liner in airoha_qdma_cleanup_rx_queue() would cover both
paths. Or is this too unlikely to matter in practice?

Re: [PATCH v2] [net] net: airoha: Clean up RX queues in airoha_dev_stop

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

> Thanks Simon for forwarding the AI review. I've reviewed all three
> concerns:
> 
> #1 (NAPI race) and #2 (RX refill) are valid. #2 is the decisive
> issue: airoha_dev_open() has no RX ring refill, so draining the
> queues in stop would cause RX stall on next open. This aligns with
> Lorenzo's earlier feedback — RX queues don't need cleanup in
> dev_stop(). I'll drop this patch.
> 
> #3 (q->skb leak) is a pre-existing issue, not introduced by this
> patch. It exists even in the module unload path
> (airoha_qdma_cleanup()). @Lorenzo — do you think this warrants a
> fix? A one-liner in airoha_qdma_cleanup_rx_queue() would cover both
> paths. Or is this too unlikely to matter in practice?

Soon I will post a patch to run airoha_qdma_cleanup_tx_queue() just in
airoha_hw_cleanup() so I think we should just drop this patch.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]