Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 07 Mar 2018 10:39:01 -0500 (EST)
Message-Id: <20180307.103901.1771176275743006915.davem@davemloft.net>
To:     arushisinghal19971997@gmail.com
Cc:     jdmason@kudzu.us, jakub.kicinski@netronome.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        oss-drivers@netronome.com, outreachy-kernel@googlegroups.com
Subject: Re: [PATCH v2] net: ethernet: Drop unnecessary continue
From:   David Miller <davem@davemloft.net>
In-Reply-To: <20180303161456.GA20194@seema-Inspiron-15-3567>
References: <20180303161456.GA20194@seema-Inspiron-15-3567>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.