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);