[PATCH] octeontx2-af: Fix uninitialized scalar variable

[PATCH] octeontx2-af: Fix uninitialized scalar variable

[Ethan Carter Edwards]

The variable *max_mtu* is uninitialized in the function
otx2_get_max_mtu. It is only assigned in the if-statement, leaving the
possibility of returning an uninitialized value.

1500 is the industry standard networking mtu and therefore should be the
default. If the function detects that the hardware custom sets the mtu,
then it will use it instead.

Addresses-Coverity-ID: 1636407 ("Uninitialized scalar variable")
Fixes: ab58a416c93f ("octeontx2-pf: cn10k: Get max mtu supported from admin function")
Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 2b49bfec78692cf1f63c793ec49511607cda7c3e..6c1b03690a9c24c5232ff9f07befb1cc553490f7 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -1909,7 +1909,7 @@ u16 otx2_get_max_mtu(struct otx2_nic *pfvf)
 {
 	struct nix_hw_info *rsp;
 	struct msg_req *req;
-	u16 max_mtu;
+	u16 max_mtu = 1500;
 	int rc;
 
 	mutex_lock(&pfvf->mbox.lock);
@@ -1948,7 +1948,6 @@ u16 otx2_get_max_mtu(struct otx2_nic *pfvf)
 	if (rc) {
 		dev_warn(pfvf->dev,
 			 "Failed to get MTU from hardware setting default value(1500)\n");
-		max_mtu = 1500;
 	}
 	return max_mtu;
 }

---
base-commit: febbc555cf0fff895546ddb8ba2c9a523692fb55
change-id: 20250210-otx2_common-453132aa0a24

Best regards,
-- 
Ethan Carter Edwards <ethan@ethancedwards.com>

Re: [PATCH] octeontx2-af: Fix uninitialized scalar variable

[Michal Swiatkowski]

On Mon, Feb 10, 2025 at 09:01:52PM -0500, Ethan Carter Edwards wrote:
> The variable *max_mtu* is uninitialized in the function
> otx2_get_max_mtu. It is only assigned in the if-statement, leaving the
> possibility of returning an uninitialized value.

In which case? If rc == 0 at the end of the function max_mtu is set to
custom value from HW. If rc != it will reach the if after goto label and
set max_mtu to default.

In my opinion this change is good. It is easier to see that the variable
is alwyas correct initialized, but I don't think it is a fix for real
issue.

Thanks,
Michal

> 
> 1500 is the industry standard networking mtu and therefore should be the
> default. If the function detects that the hardware custom sets the mtu,
> then it will use it instead.
> 
> Addresses-Coverity-ID: 1636407 ("Uninitialized scalar variable")
> Fixes: ab58a416c93f ("octeontx2-pf: cn10k: Get max mtu supported from admin function")
> Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 2b49bfec78692cf1f63c793ec49511607cda7c3e..6c1b03690a9c24c5232ff9f07befb1cc553490f7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -1909,7 +1909,7 @@ u16 otx2_get_max_mtu(struct otx2_nic *pfvf)
>  {
>  	struct nix_hw_info *rsp;
>  	struct msg_req *req;
> -	u16 max_mtu;
> +	u16 max_mtu = 1500;
>  	int rc;
>  
>  	mutex_lock(&pfvf->mbox.lock);
> @@ -1948,7 +1948,6 @@ u16 otx2_get_max_mtu(struct otx2_nic *pfvf)
>  	if (rc) {
>  		dev_warn(pfvf->dev,
>  			 "Failed to get MTU from hardware setting default value(1500)\n");
> -		max_mtu = 1500;
>  	}
>  	return max_mtu;
>  }
> 
> ---
> base-commit: febbc555cf0fff895546ddb8ba2c9a523692fb55
> change-id: 20250210-otx2_common-453132aa0a24
> 
> Best regards,
> -- 
> Ethan Carter Edwards <ethan@ethancedwards.com>

Re: [PATCH] octeontx2-af: Fix uninitialized scalar variable

[Paolo Abeni]

On 2/11/25 6:50 AM, Michal Swiatkowski wrote:
> On Mon, Feb 10, 2025 at 09:01:52PM -0500, Ethan Carter Edwards wrote:
>> The variable *max_mtu* is uninitialized in the function
>> otx2_get_max_mtu. It is only assigned in the if-statement, leaving the
>> possibility of returning an uninitialized value.
> 
> In which case? If rc == 0 at the end of the function max_mtu is set to
> custom value from HW. If rc != it will reach the if after goto label and
> set max_mtu to default.
> 
> In my opinion this change is good. It is easier to see that the variable
> is alwyas correct initialized, but I don't think it is a fix for real
> issue.

Yep, this is not a fix, the 'Fixes' tag should be dropped. Also I think
the external tool related tag should not be included.

IMHO have the `max_mtu = 1500` initialization nearby the related warning
is preferable.

Since this is a refactor, I would instead rewrite the relevant function
to be more readable and hopefully please the checker, something alike
the following (completely untested):

---
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 2b49bfec7869..7f6c8945e1ef 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -1915,42 +1915,37 @@ u16 otx2_get_max_mtu(struct otx2_nic *pfvf)
 	mutex_lock(&pfvf->mbox.lock);

 	req = otx2_mbox_alloc_msg_nix_get_hw_info(&pfvf->mbox);
-	if (!req) {
-		rc =  -ENOMEM;
-		goto out;
-	}
+	if (!req)
+		goto fail;

 	rc = otx2_sync_mbox_msg(&pfvf->mbox);
-	if (!rc) {
-		rsp = (struct nix_hw_info *)
-		       otx2_mbox_get_rsp(&pfvf->mbox.mbox, 0, &req->hdr);
-		if (IS_ERR(rsp)) {
-			rc = PTR_ERR(rsp);
-			goto out;
-		}
+	if (rc)
+		goto fail;
+	rsp = (struct nix_hw_info *)
+	       otx2_mbox_get_rsp(&pfvf->mbox.mbox, 0, &req->hdr);
+	if (IS_ERR(rsp))
+		goto fail;

-		/* HW counts VLAN insertion bytes (8 for double tag)
-		 * irrespective of whether SQE is requesting to insert VLAN
-		 * in the packet or not. Hence these 8 bytes have to be
-		 * discounted from max packet size otherwise HW will throw
-		 * SMQ errors
-		 */
-		max_mtu = rsp->max_mtu - 8 - OTX2_ETH_HLEN;
+	/* HW counts VLAN insertion bytes (8 for double tag)
+	 * irrespective of whether SQE is requesting to insert VLAN
+	 * in the packet or not. Hence these 8 bytes have to be
+	 * discounted from max packet size otherwise HW will throw
+	 * SMQ errors
+	 */
+	max_mtu = rsp->max_mtu - 8 - OTX2_ETH_HLEN;

-		/* Also save DWRR MTU, needed for DWRR weight calculation */
-		pfvf->hw.dwrr_mtu = get_dwrr_mtu(pfvf, rsp);
-		if (!pfvf->hw.dwrr_mtu)
-			pfvf->hw.dwrr_mtu = 1;
-	}
+	/* Also save DWRR MTU, needed for DWRR weight calculation */
+	pfvf->hw.dwrr_mtu = get_dwrr_mtu(pfvf, rsp);
+	if (!pfvf->hw.dwrr_mtu)
+		pfvf->hw.dwrr_mtu = 1;
+	mutex_unlock(&pfvf->mbox.lock);
+	return max_mtu;

-out:
+fail:
 	mutex_unlock(&pfvf->mbox.lock);
-	if (rc) {
-		dev_warn(pfvf->dev,
+	dev_warn(pfvf->dev,
 			 "Failed to get MTU from hardware setting default value(1500)\n");
-		max_mtu = 1500;
-	}
-	return max_mtu;
+	return 1500;
 }
 EXPORT_SYMBOL(otx2_get_max_mtu);