[PATCH v2] net: ethernet: Drop unnecessary continue

[PATCH v2] net: ethernet: Drop unnecessary continue

[Arushi Singhal]

Continue at the bottom of a loop are removed.
Issue found using drop_continue.cocci Coccinelle script.

Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
---
Changes in v2:
- Braces is dropped from if with single statement.

 drivers/net/ethernet/amd/ni65.c                   | 4 +---
 drivers/net/ethernet/neterion/s2io.c              | 4 +---
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c | 4 +---
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/amd/ni65.c b/drivers/net/ethernet/amd/ni65.c
index e248d1a..8931ce6 100644
--- a/drivers/net/ethernet/amd/ni65.c
+++ b/drivers/net/ethernet/amd/ni65.c
@@ -435,10 +435,8 @@ static int __init ni65_probe1(struct net_device *dev,int ioaddr)
 		}
 		if(cards[i].vendor_id) {
 			for(j=0;j<3;j++)
-				if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j]) {
+				if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
 					release_region(ioaddr, cards[i].total_size);
-					continue;
-			  }
 		}
 		break;
 	}
diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index b8983e7..4738bc7 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c
@@ -3679,11 +3679,9 @@ static void restore_xmsi_data(struct s2io_nic *nic)
 		writeq(nic->msix_info[i].data, &bar0->xmsi_data);
 		val64 = (s2BIT(7) | s2BIT(15) | vBIT(msix_index, 26, 6));
 		writeq(val64, &bar0->xmsi_access);
-		if (wait_for_msix_trans(nic, msix_index)) {
+		if (wait_for_msix_trans(nic, msix_index))
 			DBG_PRINT(ERR_DBG, "%s: index: %d failed\n",
 				  __func__, msix_index);
-			continue;
-		}
 	}
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index 15fa47f..5cd4f3f 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -258,10 +258,8 @@ nfp_net_pf_alloc_vnics(struct nfp_pf *pf, void __iomem *ctrl_bar,
 		ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
 
 		/* Kill the vNIC if app init marked it as invalid */
-		if (nn->port && nn->port->type == NFP_PORT_INVALID) {
+		if (nn->port && nn->port->type == NFP_PORT_INVALID)
 			nfp_net_pf_free_vnic(pf, nn);
-			continue;
-		}
 	}
 
 	if (list_empty(&pf->vnics))
-- 
2.7.4

Re: [PATCH v2] net: ethernet: Drop unnecessary continue

[Jakub Kicinski]

On Sat, 3 Mar 2018 21:44:56 +0530, Arushi Singhal wrote:
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
> index 15fa47f..5cd4f3f 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
> @@ -258,10 +258,8 @@ nfp_net_pf_alloc_vnics(struct nfp_pf *pf, void __iomem *ctrl_bar,
>  		ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
>  
>  		/* Kill the vNIC if app init marked it as invalid */
> -		if (nn->port && nn->port->type == NFP_PORT_INVALID) {
> +		if (nn->port && nn->port->type == NFP_PORT_INVALID)
>  			nfp_net_pf_free_vnic(pf, nn);
> -			continue;
> -		}

This is an error handling path so the continue makes sense here to
indicate the processing can't ever fall through if more statements are
ever added to the loop.  But OK.

>  	}
>  
>  	if (list_empty(&pf->vnics))

Re: [PATCH v2] net: ethernet: Drop unnecessary continue

[David Miller]

From: Arushi Singhal <arushisinghal19971997@gmail.com>
Date: Sat, 3 Mar 2018 21:44:56 +0530

> Continue at the bottom of a loop are removed.
> Issue found using drop_continue.cocci Coccinelle script.
> 
> Signed-off-by: Arushi Singhal <arushisinghal19971997@gmail.com>
> ---
> Changes in v2:
> - Braces is dropped from if with single statement.

Sorry there is no way I am applying this.

Just blindly removing continue statements that some static checker
or compiler warning mentions is completely unacceptable.

Actually _LOOK_ at this code:

>  		if(cards[i].vendor_id) {
>  			for(j=0;j<3;j++)
> -				if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j]) {
> +				if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
>  					release_region(ioaddr, cards[i].total_size);
> -					continue;
> -			  }
>  		}

The extraneous continue statement is the _least_ of the problems here.

This code, if the if() statement triggers, will release the ioaddr
region and then _CONTINUE_ the for(j) loop, and try to access the
registers using the same 'ioaddr' again.

That's actually a _REAL_ bug, and you're making it seem like
the behavior is intentional by editing it like this.

And the bug probably exists because this entire sequence has
misaligned closing curly braces.  It makes it look like
the continue is for the outer loop, which would be fine.

But it's not.  It's for the inner loop, and this causes the "use
ioaddr after releasing it" bug.

Please do not submit patches like this one, it makes for a lot of
wasted auditing and review for people.  The onus is on you to read
and understand the code you are editing before submitting your
changes.

Thank you.

Re: [PATCH v2] net: ethernet: Drop unnecessary continue

[David Miller]

From: Arushi Singhal <arushisinghal19971997@gmail.com>
Date: Wed, 7 Mar 2018 22:22:13 +0530

> Yes I know it's for the inner loop.
> But I am not able to find, where I am wrong here

You are making what is essentially a nop change to a piece of code,
which if you had instead read carefully would have instead inspired
you to fix a real genuine bug instead.

Re: [Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary continue

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 4117 bytes --]



On Wed, 7 Mar 2018, Arushi Singhal wrote:

>
>
> On Wed, Mar 7, 2018 at 9:09 PM, David Miller <davem@davemloft.net> wrote:
>       From: Arushi Singhal <arushisinghal19971997@gmail.com>
>       Date: Sat, 3 Mar 2018 21:44:56 +0530
>
>       > Continue at the bottom of a loop are removed.
>       > Issue found using drop_continue.cocci Coccinelle script.
>       >
>       > Signed-off-by: Arushi Singhal
>       <arushisinghal19971997@gmail.com>
>       > ---
>       > Changes in v2:
>       > - Braces is dropped from if with single statement.
>
>       Sorry there is no way I am applying this.
>
>       Just blindly removing continue statements that some static
>       checker
>       or compiler warning mentions is completely unacceptable.
>
>       Actually _LOOK_ at this code:
>
>       >               if(cards[i].vendor_id) {
>       >                       for(j=0;j<3;j++)
>       > -                           
>        if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
>       {
>       > +                           
>        if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
>       >                                       release_region(ioaddr,
>       cards[i].total_size);
>       > -                                     continue;
>       > -                       }
>       >               }
>
>       The extraneous continue statement is the _least_ of the problems
>       here.
>
>
> Hello David
>
> Yes you are correct.
>  
>
>       This code, if the if() statement triggers, will release the
>       ioaddr
>       region and then _CONTINUE_ the for(j) loop, and try to access
>       the
>       registers using the same 'ioaddr' again.
>
>       That's actually a _REAL_ bug, and you're making it seem like
>       the behavior is intentional by editing it like this.
>
>       And the bug probably exists because this entire sequence has
>       misaligned closing curly braces.  It makes it look like
>       the continue is for the outer loop, which would be fine.
>
>       But it's not.  It's for the inner loop, and this causes the "use
>       ioaddr after releasing it" bug.
>
>
> Yes I know it's for the inner loop.
> But I am not able to find, where I am wrong here

Arushi,

You are preserving the current behavior and David is telling you that the
current behavior is incorrect.  He doesn't want a patch that changes the
code but preesrves the current (wrong) behavior, because that somehow
contributes to the illusion that the incorrect code is correct.

julia

>
> For example
> BEFORE:-
> for(i=1;i<10;i++)
>        if (expr1)
>           {
>              statement;
>              continue;
>           }
>  
> After My Patch
> for(i=1;i<10;i++)
>     if (expr1)        
>        statement;
>
> I am not understanding where I am getting wrong in understanding this.
> For me both are equivalent.
>
> I Agree with Jakub reply:-
> "This is an error handling path so the continue makes sense here to
> indicate the processing can't ever fall through if more statements are
> ever added to the loop.  But OK."
>
> Thanks
> Arushi
>
>
>       Please do not submit patches like this one, it makes for a lot
>       of
>       wasted auditing and review for people.  The onus is on you to
>       read
>       and understand the code you are editing before submitting your
>       changes.
>
>       Thank you.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF8_axeTnahPmokxm
> Dnm4NmWo8QWSEmLejN4fO1nAiDaBA%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>