[PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Gilbert Adikankwu]

Reported by checkpatch:

WARNING: else is not generally useful after a break or return

The idea of the break statements in the if/else is so that the loop is
exited immediately the value of status is changed. And returned
immediately. For if/else conditionals, the block to be executed will
always be one of the two. Introduce a bool type variable 's_sig' that
evaluates to true when the value of status is changed within the if/else
block.

Signed-off-by: Gilbert Adikankwu <gilbertadikankwu@gmail.com>
---
 drivers/staging/qlge/qlge.h     | 1 +
 drivers/staging/qlge/qlge_mpi.c | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index d0dd659834ee..b846bca82571 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -11,6 +11,7 @@
 #include <linux/netdevice.h>
 #include <linux/rtnetlink.h>
 #include <linux/if_vlan.h>
+#include <linux/types.h>
 
 /*
  * General definitions...
diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 96a4de6d2b34..44cb879240a0 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -909,6 +909,7 @@ int qlge_mb_wol_set_magic(struct qlge_adapter *qdev, u32 enable_wol)
 static int qlge_idc_wait(struct qlge_adapter *qdev)
 {
 	int status = -ETIMEDOUT;
+	bool s_sig = false;
 	struct mbox_params *mbcp = &qdev->idc_mbc;
 	long wait_time;
 
@@ -934,14 +935,17 @@ static int qlge_idc_wait(struct qlge_adapter *qdev)
 		} else if (mbcp->mbox_out[0] == AEN_IDC_CMPLT) {
 			netif_err(qdev, drv, qdev->ndev, "IDC Success.\n");
 			status = 0;
-			break;
+			s_sig = true;
 		} else {
 			netif_err(qdev, drv, qdev->ndev,
 				  "IDC: Invalid State 0x%.04x.\n",
 				  mbcp->mbox_out[0]);
 			status = -EIO;
-			break;
+			s_sig = true;
 		}
+
+		if (s_sig)
+			break;
 	}
 
 	return status;
-- 
2.34.1

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Julia Lawall]

On Sat, 14 Oct 2023, Gilbert Adikankwu wrote:

> Reported by checkpatch:
>
> WARNING: else is not generally useful after a break or return
>
> The idea of the break statements in the if/else is so that the loop is
> exited immediately the value of status is changed. And returned
> immediately. For if/else conditionals, the block to be executed will
> always be one of the two. Introduce a bool type variable 's_sig' that
> evaluates to true when the value of status is changed within the if/else
> block.

The idea of the checkpatch warning is that eg

found = search();
if (!found)
  break;
else do_something();

is equvalent to:

found = search();
if (!found)
  break;
do_something();

Because now the normal computation is at top level and the if branches are
only used for error handling.

But that is not the case in your code.  In your code, it seems that there
are two cases where one would like to break out of the loop.  The code
would be better left as it is.

julia

>
> Signed-off-by: Gilbert Adikankwu <gilbertadikankwu@gmail.com>
> ---
>  drivers/staging/qlge/qlge.h     | 1 +
>  drivers/staging/qlge/qlge_mpi.c | 8 ++++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> index d0dd659834ee..b846bca82571 100644
> --- a/drivers/staging/qlge/qlge.h
> +++ b/drivers/staging/qlge/qlge.h
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/if_vlan.h>
> +#include <linux/types.h>
>
>  /*
>   * General definitions...
> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> index 96a4de6d2b34..44cb879240a0 100644
> --- a/drivers/staging/qlge/qlge_mpi.c
> +++ b/drivers/staging/qlge/qlge_mpi.c
> @@ -909,6 +909,7 @@ int qlge_mb_wol_set_magic(struct qlge_adapter *qdev, u32 enable_wol)
>  static int qlge_idc_wait(struct qlge_adapter *qdev)
>  {
>  	int status = -ETIMEDOUT;
> +	bool s_sig = false;
>  	struct mbox_params *mbcp = &qdev->idc_mbc;
>  	long wait_time;
>
> @@ -934,14 +935,17 @@ static int qlge_idc_wait(struct qlge_adapter *qdev)
>  		} else if (mbcp->mbox_out[0] == AEN_IDC_CMPLT) {
>  			netif_err(qdev, drv, qdev->ndev, "IDC Success.\n");
>  			status = 0;
> -			break;
> +			s_sig = true;
>  		} else {
>  			netif_err(qdev, drv, qdev->ndev,
>  				  "IDC: Invalid State 0x%.04x.\n",
>  				  mbcp->mbox_out[0]);
>  			status = -EIO;
> -			break;
> +			s_sig = true;
>  		}
> +
> +		if (s_sig)
> +			break;
>  	}
>
>  	return status;
> --
> 2.34.1
>
>
>

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Nam Cao]

On Sat, Oct 14, 2023 at 07:11:59AM +0100, Gilbert Adikankwu wrote:
> Reported by checkpatch:
> 
> WARNING: else is not generally useful after a break or return
>

What checkpatch is telling you here is that the "else" is redundant and
can be removed. Although your patch suppresses the warning, it makes the
code messier :(

Best regards,
Nam

> The idea of the break statements in the if/else is so that the loop is
> exited immediately the value of status is changed. And returned
> immediately. For if/else conditionals, the block to be executed will
> always be one of the two. Introduce a bool type variable 's_sig' that
> evaluates to true when the value of status is changed within the if/else
> block.
> 
> Signed-off-by: Gilbert Adikankwu <gilbertadikankwu@gmail.com>
> ---
>  drivers/staging/qlge/qlge.h     | 1 +
>  drivers/staging/qlge/qlge_mpi.c | 8 ++++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> index d0dd659834ee..b846bca82571 100644
> --- a/drivers/staging/qlge/qlge.h
> +++ b/drivers/staging/qlge/qlge.h
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/if_vlan.h>
> +#include <linux/types.h>
>  
>  /*
>   * General definitions...
> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> index 96a4de6d2b34..44cb879240a0 100644
> --- a/drivers/staging/qlge/qlge_mpi.c
> +++ b/drivers/staging/qlge/qlge_mpi.c
> @@ -909,6 +909,7 @@ int qlge_mb_wol_set_magic(struct qlge_adapter *qdev, u32 enable_wol)
>  static int qlge_idc_wait(struct qlge_adapter *qdev)
>  {
>  	int status = -ETIMEDOUT;
> +	bool s_sig = false;
>  	struct mbox_params *mbcp = &qdev->idc_mbc;
>  	long wait_time;
>  
> @@ -934,14 +935,17 @@ static int qlge_idc_wait(struct qlge_adapter *qdev)
>  		} else if (mbcp->mbox_out[0] == AEN_IDC_CMPLT) {
>  			netif_err(qdev, drv, qdev->ndev, "IDC Success.\n");
>  			status = 0;
> -			break;
> +			s_sig = true;
>  		} else {
>  			netif_err(qdev, drv, qdev->ndev,
>  				  "IDC: Invalid State 0x%.04x.\n",
>  				  mbcp->mbox_out[0]);
>  			status = -EIO;
> -			break;
> +			s_sig = true;
>  		}
> +
> +		if (s_sig)
> +			break;
>  	}
>  
>  	return status;
> -- 
> 2.34.1
> 

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Nam Cao]

On Sat, Oct 14, 2023 at 08:58:13AM +0200, Nam Cao wrote:
> On Sat, Oct 14, 2023 at 07:11:59AM +0100, Gilbert Adikankwu wrote:
> > Reported by checkpatch:
> > 
> > WARNING: else is not generally useful after a break or return
> >
> 
> What checkpatch is telling you here is that the "else" is redundant and
> can be removed. Although your patch suppresses the warning, it makes the
> code messier :(

Ah wait, after reading Julia's comment, I realize that the "else" is not
redundant at all. Seems like checkpatch.pl is lying. So ignore what I
said.

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Gilbert Adikankwu]

On Sat, Oct 14, 2023 at 08:23:13AM +0200, Julia Lawall wrote:
> 
> 
> On Sat, 14 Oct 2023, Gilbert Adikankwu wrote:
> 
> > Reported by checkpatch:
> >
> > WARNING: else is not generally useful after a break or return
> >
> > The idea of the break statements in the if/else is so that the loop is
> > exited immediately the value of status is changed. And returned
> > immediately. For if/else conditionals, the block to be executed will
> > always be one of the two. Introduce a bool type variable 's_sig' that
> > evaluates to true when the value of status is changed within the if/else
> > block.
> 
> The idea of the checkpatch warning is that eg
> 
> found = search();
> if (!found)
>   break;
> else do_something();
> 
> is equvalent to:
> 
> found = search();
> if (!found)
>   break;
> do_something();
> 
> Because now the normal computation is at top level and the if branches are
> only used for error handling.
> 
> But that is not the case in your code.  In your code, it seems that there
> are two cases where one would like to break out of the loop.  The code
> would be better left as it is.
> 
> julia

Thank you for the quick review. I thought about it the you described but
then realised the else was not redundant that was why I went the route
of trying to suppress the warning. I will revert the changes as you
have suggested

Gilbert
> 
> >
> > Signed-off-by: Gilbert Adikankwu <gilbertadikankwu@gmail.com>
> > ---
> >  drivers/staging/qlge/qlge.h     | 1 +
> >  drivers/staging/qlge/qlge_mpi.c | 8 ++++++--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> > index d0dd659834ee..b846bca82571 100644
> > --- a/drivers/staging/qlge/qlge.h
> > +++ b/drivers/staging/qlge/qlge.h
> > @@ -11,6 +11,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/rtnetlink.h>
> >  #include <linux/if_vlan.h>
> > +#include <linux/types.h>
> >
> >  /*
> >   * General definitions...
> > diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> > index 96a4de6d2b34..44cb879240a0 100644
> > --- a/drivers/staging/qlge/qlge_mpi.c
> > +++ b/drivers/staging/qlge/qlge_mpi.c
> > @@ -909,6 +909,7 @@ int qlge_mb_wol_set_magic(struct qlge_adapter *qdev, u32 enable_wol)
> >  static int qlge_idc_wait(struct qlge_adapter *qdev)
> >  {
> >  	int status = -ETIMEDOUT;
> > +	bool s_sig = false;
> >  	struct mbox_params *mbcp = &qdev->idc_mbc;
> >  	long wait_time;
> >
> > @@ -934,14 +935,17 @@ static int qlge_idc_wait(struct qlge_adapter *qdev)
> >  		} else if (mbcp->mbox_out[0] == AEN_IDC_CMPLT) {
> >  			netif_err(qdev, drv, qdev->ndev, "IDC Success.\n");
> >  			status = 0;
> > -			break;
> > +			s_sig = true;
> >  		} else {
> >  			netif_err(qdev, drv, qdev->ndev,
> >  				  "IDC: Invalid State 0x%.04x.\n",
> >  				  mbcp->mbox_out[0]);
> >  			status = -EIO;
> > -			break;
> > +			s_sig = true;
> >  		}
> > +
> > +		if (s_sig)
> > +			break;
> >  	}
> >
> >  	return status;
> > --
> > 2.34.1
> >
> >
> >

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Gilbert Adikankwu]

On Sat, Oct 14, 2023 at 09:14:23AM +0200, Nam Cao wrote:
> On Sat, Oct 14, 2023 at 08:58:13AM +0200, Nam Cao wrote:
> > On Sat, Oct 14, 2023 at 07:11:59AM +0100, Gilbert Adikankwu wrote:
> > > Reported by checkpatch:
> > > 
> > > WARNING: else is not generally useful after a break or return
> > >
> > 
> > What checkpatch is telling you here is that the "else" is redundant and
> > can be removed. Although your patch suppresses the warning, it makes the
> > code messier :(
> 
> Ah wait, after reading Julia's comment, I realize that the "else" is not
> redundant at all. Seems like checkpatch.pl is lying. So ignore what I
> said.

Thanks

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Przemek Kitszel]

On 10/14/23 09:50, Gilbert Adikankwu wrote:
> On Sat, Oct 14, 2023 at 09:14:23AM +0200, Nam Cao wrote:
>> On Sat, Oct 14, 2023 at 08:58:13AM +0200, Nam Cao wrote:
>>> On Sat, Oct 14, 2023 at 07:11:59AM +0100, Gilbert Adikankwu wrote:
>>>> Reported by checkpatch:
>>>>
>>>> WARNING: else is not generally useful after a break or return
>>>>
>>>
>>> What checkpatch is telling you here is that the "else" is redundant and
>>> can be removed. Although your patch suppresses the warning, it makes the
>>> code messier :(
>>
>> Ah wait, after reading Julia's comment, I realize that the "else" is not
>> redundant at all. Seems like checkpatch.pl is lying. So ignore what I
>> said.
> 
> Thanks
> 
> 

Could you consider fixing checkpatch instead?

Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()

[Dan Carpenter]

On Mon, Oct 16, 2023 at 04:19:09PM +0200, Przemek Kitszel wrote:
> On 10/14/23 09:50, Gilbert Adikankwu wrote:
> > On Sat, Oct 14, 2023 at 09:14:23AM +0200, Nam Cao wrote:
> > > On Sat, Oct 14, 2023 at 08:58:13AM +0200, Nam Cao wrote:
> > > > On Sat, Oct 14, 2023 at 07:11:59AM +0100, Gilbert Adikankwu wrote:
> > > > > Reported by checkpatch:
> > > > > 
> > > > > WARNING: else is not generally useful after a break or return
> > > > > 
> > > > 
> > > > What checkpatch is telling you here is that the "else" is redundant and
> > > > can be removed. Although your patch suppresses the warning, it makes the
> > > > code messier :(
> > > 
> > > Ah wait, after reading Julia's comment, I realize that the "else" is not
> > > redundant at all. Seems like checkpatch.pl is lying. So ignore what I
> > > said.
> > 
> > Thanks
> > 
> > 
> 
> Could you consider fixing checkpatch instead?

Parsing C is quite hard and checkpatch is never going to do it well.
It might be fun to start this project but it's kind of doomed.  Doomed
projects can be an educational experience as well.

The way to do it might be to add a new in_else_if variable which tracks
if you are in an else if block.  You would look at the indent level and
try the curly braces.  Then if the previous thing was an else_if silence
the warning.

But also it's fine that checkpatch prints the occasional incorrect
warning because it teaches people to be more careful.

Another thing is that when you introduce a bug, you should always
consider if other people have done that before as well.  Perhaps do
a `git log -p --grep "else is not generally useful"` and see if everyone
else did it correctly and if reviewers caught the mistakes.

regards,
dan carpenter