[PATCH linux-next] net:stmmac: Fix the unsigned expression compared with zero

From: Zhang Yunkai <redacted>

WARNING:  Unsigned expression "queue" compared with zero.
Reported-by: Zeal Robot <redacted>
Signed-off-by: Zhang Yunkai <redacted>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7b8404a21544..a4cf2c640531 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -1699,7 +1699,7 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
 	return 0;
 
 err_init_rx_buffers:
-	while (queue >= 0) {
+	do {
 		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
 
 		if (rx_q->xsk_pool)

@@ -1710,11 +1710,7 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
 		rx_q->buf_alloc_num = 0;
 		rx_q->xsk_pool = NULL;
 
-		if (queue == 0)
-			break;
-
-		queue--;
-	}
+	} while (queue--);
 
 	return ret;
 }

-- 
2.25.1

This is a real Coverity issue since queue variable is defined as u32, but there is no breakage from logic, it will break while loop when queue equal 0, and queue[0] actually need be handled.
After your code change, queue[0] will not be handled, right? It will break the logic. If you want to fix the this issue, I think the easiest way is to define queue variable to int.

Best Regards,
Joakim Zhang

--
2.25.1

On Thu, Jul 15, 2021 at 10:12:04AM +0000, Joakim Zhang wrote:

quoted

-----Original Message-----
From: menglong8.dong@gmail.com <redacted>
Sent: 2021年7月15日 15:46
To: davem@davemloft.net
Cc: peppe.cavallaro@st.com; alexandre.torgue@foss.st.com;
joabreu@synopsys.com; kuba@kernel.org; mcoquelin.stm32@gmail.com;
netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Zhang
Yunkai [off-list ref]; Zeal Robot [off-list ref]
Subject: [PATCH linux-next] net:stmmac: Fix the unsigned expression compared
with zero

From: Zhang Yunkai <redacted>

WARNING:  Unsigned expression "queue" compared with zero.
Reported-by: Zeal Robot <redacted>
Signed-off-by: Zhang Yunkai <redacted>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7b8404a21544..a4cf2c640531 100644

--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -1699,7 +1699,7 @@ static int init_dma_rx_desc_rings(struct net_device

*dev, gfp_t flags)
 	return 0;

 err_init_rx_buffers:
-	while (queue >= 0) {
+	do {
 		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];

 		if (rx_q->xsk_pool)

@@ -1710,11 +1710,7 @@ static int init_dma_rx_desc_rings(struct

net_device *dev, gfp_t flags)
 		rx_q->buf_alloc_num = 0;
 		rx_q->xsk_pool = NULL;

-		if (queue == 0)
-			break;
-
-		queue--;
-	}
+	} while (queue--);

 	return ret;
 }

This is a real Coverity issue since queue variable is defined as u32, but there is no breakage from logic, it will break while loop when queue equal 0, and queue[0] actually need be handled.
After your code change, queue[0] will not be handled, right? It will break the logic. If you want to fix the this issue, I think the easiest way is to define queue variable to int.

Best Regards,
Joakim Zhang

quoted

--
2.25.1

The function '__init_dma_rx_desc_rings' is expecting 'queue' to be u32 type.
I would suggest the following:-

@@ -1686,6 +1686,7 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
        struct stmmac_priv *priv = netdev_priv(dev);
        u32 rx_count = priv->plat->rx_queues_to_use;
        u32 queue;
+       u32 i;
        int ret;

        /* RX INITIALIZATION */

@@ -1701,21 +1702,16 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
        return 0;

 err_init_rx_buffers:
-       while (queue >= 0) {
-               struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
+       for (i = 0; i <= queue; i++) {
+               struct stmmac_rx_queue *rx_q = &priv->rx_queue[i];

                if (rx_q->xsk_pool)
-                       dma_free_rx_xskbufs(priv, queue);
+                       dma_free_rx_xskbufs(priv, i);
                else
-                       dma_free_rx_skbufs(priv, queue);
+                       dma_free_rx_skbufs(priv, i);

                rx_q->buf_alloc_num = 0;
                rx_q->xsk_pool = NULL;
-
-               if (queue == 0)
-                       break;
-
-               queue--;
        }

Regards,
 VK