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