diff mbox series

[net-next] bnx2: remove deadcode in bnx2_init_cpus()

Message ID 20230309174231.3135-1-korotkov.maxim.s@gmail.com
State New
Headers
Series [net-next] bnx2: remove deadcode in bnx2_init_cpus() |

Commit Message

Maxim Korotkov March 9, 2023, 5:42 p.m. UTC 
  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 <korotkov.maxim.s@gmail.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 31 +++++++---------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Comments

Jakub Kicinski March 10, 2023, 6:57 a.m. UTC | #1 
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?
Maxim Korotkov March 10, 2023, 7:33 a.m. UTC | #2 
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 
<leonro@nvidia.com> from the previous iteration
Jakub Kicinski March 10, 2023, 7:41 a.m. UTC | #3 
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 
> <leonro@nvidia.com> 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.
diff mbox series

Patch

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