[PATCH net v2] net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()

[PATCH net v2] net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()

[Lorenzo Bianconi]

Add missing dma_rmb() in airoha_qdma_rx_process routine to make sure the
DMA read operations are completed when the NIC reports the processing on
the current descriptor is done. Moreover, add missing READ_ONCE() in
airoha_qdma_rx_process() for DMA descriptor control fields in order to
avoid any compiler reordering.

Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes in v2:
- Use msg1 in airoha_qdma_get_gdm_port() signature to avoid missing
  READ_ONCE().
- Link to v1: https://lore.kernel.org/r/20260402-airoha_qdma_rx_process-fix-reordering-v1-1-53278474f062@kernel.org
---
 drivers/net/ethernet/airoha/airoha_eth.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 95ba99b89428..f1843bc5b991 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -581,10 +581,9 @@ static int airoha_qdma_fill_rx_queue(struct airoha_queue *q)
 	return nframes;
 }
 
-static int airoha_qdma_get_gdm_port(struct airoha_eth *eth,
-				    struct airoha_qdma_desc *desc)
+static int airoha_qdma_get_gdm_port(struct airoha_eth *eth, u32 msg1)
 {
-	u32 port, sport, msg1 = le32_to_cpu(desc->msg1);
+	u32 port, sport;
 
 	sport = FIELD_GET(QDMA_ETH_RXMSG_SPORT_MASK, msg1);
 	switch (sport) {
@@ -612,15 +611,17 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
 	while (done < budget) {
 		struct airoha_queue_entry *e = &q->entry[q->tail];
 		struct airoha_qdma_desc *desc = &q->desc[q->tail];
-		u32 hash, reason, msg1 = le32_to_cpu(desc->msg1);
-		struct page *page = virt_to_head_page(e->buf);
-		u32 desc_ctrl = le32_to_cpu(desc->ctrl);
+		u32 hash, reason, msg1, desc_ctrl;
 		struct airoha_gdm_port *port;
 		int data_len, len, p;
+		struct page *page;
 
+		desc_ctrl = le32_to_cpu(READ_ONCE(desc->ctrl));
 		if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
 			break;
 
+		dma_rmb();
+
 		q->tail = (q->tail + 1) % q->ndesc;
 		q->queued--;
 
@@ -633,10 +634,12 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
 		if (!len || data_len < len)
 			goto free_frag;
 
-		p = airoha_qdma_get_gdm_port(eth, desc);
+		msg1 = le32_to_cpu(READ_ONCE(desc->msg1));
+		p = airoha_qdma_get_gdm_port(eth, msg1);
 		if (p < 0 || !eth->ports[p])
 			goto free_frag;
 
+		page = virt_to_head_page(e->buf);
 		port = eth->ports[p];
 		if (!q->skb) { /* first buffer */
 			q->skb = napi_build_skb(e->buf, q->buf_size);
@@ -670,8 +673,8 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
 			 * DMA descriptor. Report DSA tag to the DSA stack
 			 * via skb dst info.
 			 */
-			u32 sptag = FIELD_GET(QDMA_ETH_RXMSG_SPTAG,
-					      le32_to_cpu(desc->msg0));
+			u32 msg0 = le32_to_cpu(READ_ONCE(desc->msg0));
+			u32 sptag = FIELD_GET(QDMA_ETH_RXMSG_SPTAG, msg0);
 
 			if (sptag < ARRAY_SIZE(port->dsa_meta) &&
 			    port->dsa_meta[sptag])

---
base-commit: ec7067e661193403a7a00980bda8612db5954142
change-id: 20260402-airoha_qdma_rx_process-fix-reordering-722308255b65

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

Re: [PATCH net v2] net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()

This commit adds a missing dma_rmb() to ensure DMA read operations are
completed and uses READ_ONCE() to prevent compiler reordering when accessing
descriptor fields.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 95ba99b89428e..f1843bc5b9914 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -612,15 +611,17 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  	while (done < budget) {
>  		struct airoha_queue_entry *e = &q->entry[q->tail];
>  		struct airoha_qdma_desc *desc = &q->desc[q->tail];
> -		u32 hash, reason, msg1 = le32_to_cpu(desc->msg1);
> -		struct page *page = virt_to_head_page(e->buf);
> -		u32 desc_ctrl = le32_to_cpu(desc->ctrl);
> +		u32 hash, reason, msg1, desc_ctrl;
>  		struct airoha_gdm_port *port;
>  		int data_len, len, p;
> +		struct page *page;
>  
> +		desc_ctrl = le32_to_cpu(READ_ONCE(desc->ctrl));
>  		if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
>  			break;
[ ... ]
> @@ -633,10 +634,12 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  		if (!len || data_len < len)
>  			goto free_frag;

Could this jump lead to an uninitialized pointer dereference?

The initialization of page was removed from the declarations above, so it
is uninitialized at this point.

Later in the function, the error path looks like this:

free_frag:
		if (q->skb) {
			dev_kfree_skb(q->skb);
			q->skb = NULL;
		} else {
			page_pool_put_full_page(q->page_pool, page, true);
		}

If q->skb is NULL, passing the uninitialized page to
page_pool_put_full_page() might cause a crash.

Also, this isn't a bug introduced by this patch, but does this error path
leak the page associated with the current descriptor?

If we hit an error on a scattered fragment while q->skb is not NULL, the
SKB is freed, but the current page (which hasn't been attached to the SKB
yet) bypasses the else branch and isn't returned to the page pool.

>  
> -		p = airoha_qdma_get_gdm_port(eth, desc);
> +		msg1 = le32_to_cpu(READ_ONCE(desc->msg1));
> +		p = airoha_qdma_get_gdm_port(eth, msg1);
>  		if (p < 0 || !eth->ports[p])
>  			goto free_frag;

Similarly, taking this jump leaves page uninitialized before reaching the
error handling path.

>  
> +		page = virt_to_head_page(e->buf);
>  		port = eth->ports[p];
>  		if (!q->skb) { /* first buffer */