The load_cpu_fw function has no error return code
and always returns zero. Checking the value returned by
this function does not make sense.
As a result, bnx2_init_cpus() will also return only zero
Therefore, it will be safe to change the type of functions
to void and remove checking
Found by Security Code and Linux Verification
Center (linuxtesting.org) with SVACE
Signed-off-by: Maxim Korotkov <[email protected]>
---
drivers/net/ethernet/broadcom/bnx2.c | 31 +++++++---------------------
1 file changed, 8 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 9f473854b0f4..19b053c879b0 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -3829,7 +3829,7 @@ load_rv2p_fw(struct bnx2 *bp, u32 rv2p_proc,
return 0;
}
-static int
+static void
load_cpu_fw(struct bnx2 *bp, const struct cpu_reg *cpu_reg,
const struct bnx2_mips_fw_file_entry *fw_entry)
{
@@ -3897,48 +3897,34 @@ load_cpu_fw(struct bnx2 *bp, const struct cpu_reg *cpu_reg,
val &= ~cpu_reg->mode_value_halt;
bnx2_reg_wr_ind(bp, cpu_reg->state, cpu_reg->state_value_clear);
bnx2_reg_wr_ind(bp, cpu_reg->mode, val);
-
- return 0;
}
-static int
+static void
bnx2_init_cpus(struct bnx2 *bp)
{
const struct bnx2_mips_fw_file *mips_fw =
(const struct bnx2_mips_fw_file *) bp->mips_firmware->data;
const struct bnx2_rv2p_fw_file *rv2p_fw =
(const struct bnx2_rv2p_fw_file *) bp->rv2p_firmware->data;
- int rc;
/* Initialize the RV2P processor. */
load_rv2p_fw(bp, RV2P_PROC1, &rv2p_fw->proc1);
load_rv2p_fw(bp, RV2P_PROC2, &rv2p_fw->proc2);
/* Initialize the RX Processor. */
- rc = load_cpu_fw(bp, &cpu_reg_rxp, &mips_fw->rxp);
- if (rc)
- goto init_cpu_err;
+ load_cpu_fw(bp, &cpu_reg_rxp, &mips_fw->rxp);
/* Initialize the TX Processor. */
- rc = load_cpu_fw(bp, &cpu_reg_txp, &mips_fw->txp);
- if (rc)
- goto init_cpu_err;
+ load_cpu_fw(bp, &cpu_reg_txp, &mips_fw->txp);
/* Initialize the TX Patch-up Processor. */
- rc = load_cpu_fw(bp, &cpu_reg_tpat, &mips_fw->tpat);
- if (rc)
- goto init_cpu_err;
+ load_cpu_fw(bp, &cpu_reg_tpat, &mips_fw->tpat);
/* Initialize the Completion Processor. */
- rc = load_cpu_fw(bp, &cpu_reg_com, &mips_fw->com);
- if (rc)
- goto init_cpu_err;
+ load_cpu_fw(bp, &cpu_reg_com, &mips_fw->com);
/* Initialize the Command Processor. */
- rc = load_cpu_fw(bp, &cpu_reg_cp, &mips_fw->cp);
-
-init_cpu_err:
- return rc;
+ load_cpu_fw(bp, &cpu_reg_cp, &mips_fw->cp);
}
static void
@@ -4951,8 +4937,7 @@ bnx2_init_chip(struct bnx2 *bp)
} else
bnx2_init_context(bp);
- if ((rc = bnx2_init_cpus(bp)) != 0)
- return rc;
+ bnx2_init_cpus(bp);
bnx2_init_nvram(bp);
--
2.37.2
On Thu, 9 Mar 2023 20:42:31 +0300 Maxim Korotkov wrote:
> The load_cpu_fw function has no error return code
> and always returns zero. Checking the value returned by
> this function does not make sense.
> As a result, bnx2_init_cpus() will also return only zero
> Therefore, it will be safe to change the type of functions
> to void and remove checking
True, but you need to tell the reader why you're making the change.
One of the impossible-to-hit error handling paths is missing unwind
or some such?
On 10.03.2023 09:57, Jakub Kicinski wrote:
> On Thu, 9 Mar 2023 20:42:31 +0300 Maxim Korotkov wrote:
>> The load_cpu_fw function has no error return code
>> and always returns zero. Checking the value returned by
>> this function does not make sense.
>> As a result, bnx2_init_cpus() will also return only zero
>> Therefore, it will be safe to change the type of functions
>> to void and remove checking
>
> True, but you need to tell the reader why you're making the change.
> One of the impossible-to-hit error handling paths is missing unwind
> or some such?
Path with error handling was deleted in 57579f7629a3 ("bnx2: Use
request_firmware()"). This patch is needed to improving readability.
Now checking the value of the return value is misleading when reading
the code.
Do I need to add this argument to the patch description?
I also forgot to add mark Reviewed-by: Leon Romanovsky
<[email protected]> from the previous iteration
On Fri, 10 Mar 2023 10:33:46 +0300 Maxim Korotkov wrote:
> Path with error handling was deleted in 57579f7629a3 ("bnx2: Use
> request_firmware()"). This patch is needed to improving readability.
> Now checking the value of the return value is misleading when reading
> the code.
> Do I need to add this argument to the patch description?
Yes please.
> I also forgot to add mark Reviewed-by: Leon Romanovsky
> <[email protected]> from the previous iteration
So this is not the first revision? Please add Leon's tag and an
appropriate vN. e.g. [PATCH net-next v2].
In general we don't encourage cleanup of this sort because the number
of int functions which always return 0 is rather large in the kernel,
but if you already got an ack from Leon we'll consider it, so please
adjust and repost.