[net,v2,3/4] bnxt_en: Fix wrong return value check in bnxt_close_nic()

Commit Message

Michael Chan Dec. 7, 2023, 12:05 a.m. UTC

From: Kalesh AP <[e-mail]>

The wait_event_interruptible_timeout() function returns 0
if the timeout elapsed, -ERESTARTSYS if it was interrupted
by a signal, and the remaining jiffies otherwise if the
condition evaluated to true before the timeout elapsed.

Driver should have checked for zero return value instead of
a positive value.

Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Reviewed-by: Vikas Gupta <[e-mail]>
Reviewed-by: Somnath Kotur <[e-mail]>
Signed-off-by: Kalesh AP <[e-mail]>
Signed-off-by: Michael Chan <[e-mail]>
---
v2: Add missing SoB tag.
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski Dec. 7, 2023, 6:27 p.m. UTC | #1

On Wed,  6 Dec 2023 16:05:50 -0800 Michael Chan wrote:
> The wait_event_interruptible_timeout() function returns 0
> if the timeout elapsed, -ERESTARTSYS if it was interrupted
> by a signal, and the remaining jiffies otherwise if the
> condition evaluated to true before the timeout elapsed.
> 
> Driver should have checked for zero return value instead of
> a positive value.

Not sure how this was not caught earlier, maybe there is a more
complicated story behind it. Otherwise you should handle -ERESTARTSYS
as well.

Michael Chan Dec. 7, 2023, 10:24 p.m. UTC | #2

On Thu, Dec 7, 2023 at 10:27 AM Jakub Kicinski <[e-mail]> wrote:
>
> On Wed,  6 Dec 2023 16:05:50 -0800 Michael Chan wrote:
> > The wait_event_interruptible_timeout() function returns 0
> > if the timeout elapsed, -ERESTARTSYS if it was interrupted
> > by a signal, and the remaining jiffies otherwise if the
> > condition evaluated to true before the timeout elapsed.
> >
> > Driver should have checked for zero return value instead of
> > a positive value.
>
> Not sure how this was not caught earlier, maybe there is a more
> complicated story behind it. Otherwise you should handle -ERESTARTSYS
> as well.

The code will always proceed to do the close when
wait_event_interruptible_timeout() returns for any reason.  The check
is just to log a warning message that the wait has timed out and we're
closing anyway.

What I can do is to log another warning if the wait is interrupted by
a signal.  Since we do the close no matter what, the error code should
not be returned to the caller and the function should be changed to
void.  Does that sound reasonable?

Jakub Kicinski Dec. 7, 2023, 10:31 p.m. UTC | #3

On Thu, 7 Dec 2023 14:24:24 -0800 Michael Chan wrote:
> The code will always proceed to do the close when
> wait_event_interruptible_timeout() returns for any reason.  The check
> is just to log a warning message that the wait has timed out and we're
> closing anyway.
> 
> What I can do is to log another warning if the wait is interrupted by
> a signal.  Since we do the close no matter what, the error code should
> not be returned to the caller and the function should be changed to
> void.  Does that sound reasonable?

Yup!

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b4a5311bdeb5..16b7cf6b01a4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10753,7 +10753,7 @@  int bnxt_close_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 		rc = wait_event_interruptible_timeout(bp->sriov_cfg_wait,
 						      !bp->sriov_cfg,
 						      BNXT_SRIOV_CFG_WAIT_TMO);
-		if (rc)
+		if (!rc)
 			netdev_warn(bp->dev, "timeout waiting for SRIOV config operation to complete!\n");
 	}
 #endif