[PATCH] qla2xxx: avoid maybe_uninitialized warning

[PATCH] qla2xxx: avoid maybe_uninitialized warning

[Arnd Bergmann]

The qlt_check_reserve_free_req() function produces an incorrect warning
when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:

drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   ql_dbg(ql_dbg_io, vha, 0x305a,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       vha->vp_idx, vha->req->ring_index,
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The problem is that gcc fails to track the state of the condition across
an annotated branch.

This slightly rearranges the code to move the second if() block
into the first one, to avoid the warning while retaining the
behavior of the code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 985231900aca..8a44d1541eb4 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
 		else
 			vha->req->cnt = vha->req->length -
 			    (vha->req->ring_index - cnt);
-	}
 
-	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
-		ql_dbg(ql_dbg_io, vha, 0x305a,
-		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
-		    vha->vp_idx, vha->req->ring_index,
-		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
-		return -EAGAIN;
+		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
+			ql_dbg(ql_dbg_io, vha, 0x305a,
+			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
+			    vha->vp_idx, vha->req->ring_index,
+			    vha->req->cnt, req_cnt, cnt, cnt_in,
+			    vha->req->length);
+			return -EAGAIN;
+		}
 	}
+
 	vha->req->cnt -= req_cnt;
 
 	return 0;
-- 
2.7.0

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[James Bottomley]

On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
> The qlt_check_reserve_free_req() function produces an incorrect 
> warning when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
> 
> drivers/scsi/qla2xxx/qla_target.c: In function
> 'qlt_check_reserve_free_req':
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>    ql_dbg(ql_dbg_io, vha, 0x305a,
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        "qla_target(%d): There is no room in the request ring: vha
> ->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req
> -in=%d Req-Length=%d\n",
>       
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~
>        vha->vp_idx, vha->req->ring_index,
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> 
> The problem is that gcc fails to track the state of the condition 
> across an annotated branch.
> 
> This slightly rearranges the code to move the second if() block
> into the first one, to avoid the warning while retaining the
> behavior of the code.

I thought our usual policy was to ask someone to fix the compiler when
it emitted a spurious warning.

James

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[Arnd Bergmann]

On Tuesday 15 March 2016 14:49:14 James Bottomley wrote:
> On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
> > 
> > This slightly rearranges the code to move the second if() block
> > into the first one, to avoid the warning while retaining the
> > behavior of the code.
> 
> I thought our usual policy was to ask someone to fix the compiler when
> it emitted a spurious warning.

No, the rule is that we shouldn't blindly add initializations to
the variables when the compiler should have figured it out.

In this case, I wouldn't expect the compiler to ever see through
the unlikely() macro, and I'm not adding a potentially counterproductive
initialization, so I see no reason not to apply the patch.

Making it easier for the compiler to figure out what is going
on should also lead to slightly better object code. If you think
my patch makes it less readable, an alternative would be to remove
the 'unlikely', which also gets rid of the warning.

	Arnd

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[Himanshu Madhani]

On 3/16/16, 5:59 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:

>On Tuesday 15 March 2016 14:49:14 James Bottomley wrote:
>> On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
>> > 
>> > This slightly rearranges the code to move the second if() block
>> > into the first one, to avoid the warning while retaining the
>> > behavior of the code.
>> 
>> I thought our usual policy was to ask someone to fix the compiler when
>> it emitted a spurious warning.
>
>No, the rule is that we shouldn't blindly add initializations to
>the variables when the compiler should have figured it out.
>
>In this case, I wouldn't expect the compiler to ever see through
>the unlikely() macro, and I'm not adding a potentially counterproductive
>initialization, so I see no reason not to apply the patch.

I would like to keep unlikely() macro in the code. This patch looks good.

Acked-By: Himanshu Madhani <himanshu.madhani@qlogic.com>

>
>Making it easier for the compiler to figure out what is going
>on should also lead to slightly better object code. If you think
>my patch makes it less readable, an alternative would be to remove
>the 'unlikely', which also gets rid of the warning.
>
>	Arnd

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[James Bottomley]

On Wed, 2016-03-16 at 14:05 +0000, Himanshu Madhani wrote:
> 
> On 3/16/16, 5:59 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> > On Tuesday 15 March 2016 14:49:14 James Bottomley wrote:
> > > On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
> > > > 
> > > > This slightly rearranges the code to move the second if() block
> > > > into the first one, to avoid the warning while retaining the
> > > > behavior of the code.
> > > 
> > > I thought our usual policy was to ask someone to fix the compiler 
> > > when it emitted a spurious warning.
> > 
> > No, the rule is that we shouldn't blindly add initializations to
> > the variables when the compiler should have figured it out.
> > 
> > In this case, I wouldn't expect the compiler to ever see through
> > the unlikely() macro, and I'm not adding a potentially 
> > counterproductive initialization, so I see no reason not to apply
> > the patch.

OK, as long as there's a good reason why the compiler can never be
fixed to sort out this case.

> I would like to keep unlikely() macro in the code. This patch looks
> good.
> 
> Acked-By: Himanshu Madhani <himanshu.madhani@qlogic.com>

Well, OK that's good enough for me.

James

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[Tomas Henzl]

On 15.3.2016 22:40, Arnd Bergmann wrote:
> The qlt_check_reserve_free_req() function produces an incorrect warning
> when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
>
> drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    ql_dbg(ql_dbg_io, vha, 0x305a,
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        vha->vp_idx, vha->req->ring_index,
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The problem is that gcc fails to track the state of the condition across
> an annotated branch.
>
> This slightly rearranges the code to move the second if() block
> into the first one, to avoid the warning while retaining the
> behavior of the code.

When the first 'if' is true the vha->req->ring_index gets a new value 
assigned - so it could be possible that the second 'if' wont be true any more.
The code should not be merged into that single 'if', or am I missing something?

tomash

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 985231900aca..8a44d1541eb4 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
>  		else
>  			vha->req->cnt = vha->req->length -
>  			    (vha->req->ring_index - cnt);
> -	}
>  
> -	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> -		ql_dbg(ql_dbg_io, vha, 0x305a,
> -		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> -		    vha->vp_idx, vha->req->ring_index,
> -		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> -		return -EAGAIN;
> +		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> +			ql_dbg(ql_dbg_io, vha, 0x305a,
> +			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> +			    vha->vp_idx, vha->req->ring_index,
> +			    vha->req->cnt, req_cnt, cnt, cnt_in,
> +			    vha->req->length);
> +			return -EAGAIN;
> +		}
>  	}
> +
>  	vha->req->cnt -= req_cnt;
>  
>  	return 0;

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[Tomas Henzl]

On 16.3.2016 16:03, Tomas Henzl wrote:
> On 15.3.2016 22:40, Arnd Bergmann wrote:
>> The qlt_check_reserve_free_req() function produces an incorrect warning
>> when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
>>
>> drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
>> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    ql_dbg(ql_dbg_io, vha, 0x305a,
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>        "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>        vha->vp_idx, vha->req->ring_index,
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> The problem is that gcc fails to track the state of the condition across
>> an annotated branch.
>>
>> This slightly rearranges the code to move the second if() block
>> into the first one, to avoid the warning while retaining the
>> behavior of the code.
> When the first 'if' is true the vha->req->ring_index gets a new value 
> assigned - so it could be possible that the second 'if' wont be true any more.
> The code should not be merged into that single 'if', or am I missing something?

Oh, I haven't noticed that the second if actually remained in place
- please ignore my previous comments.


>
> tomash
>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
>> index 985231900aca..8a44d1541eb4 100644
>> --- a/drivers/scsi/qla2xxx/qla_target.c
>> +++ b/drivers/scsi/qla2xxx/qla_target.c
>> @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
>>  		else
>>  			vha->req->cnt = vha->req->length -
>>  			    (vha->req->ring_index - cnt);
>> -	}
>>  
>> -	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
>> -		ql_dbg(ql_dbg_io, vha, 0x305a,
>> -		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>> -		    vha->vp_idx, vha->req->ring_index,
>> -		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>> -		return -EAGAIN;
>> +		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
>> +			ql_dbg(ql_dbg_io, vha, 0x305a,
>> +			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>> +			    vha->vp_idx, vha->req->ring_index,
>> +			    vha->req->cnt, req_cnt, cnt, cnt_in,
>> +			    vha->req->length);
>> +			return -EAGAIN;
>> +		}
>>  	}
>> +
>>  	vha->req->cnt -= req_cnt;
>>  
>>  	return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[Ewan D. Milne]

On Wed, 2016-03-16 at 16:03 +0100, Tomas Henzl wrote:
> On 15.3.2016 22:40, Arnd Bergmann wrote:
> > The qlt_check_reserve_free_req() function produces an incorrect warning
> > when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
> >
> > drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
> > drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    ql_dbg(ql_dbg_io, vha, 0x305a,
> >    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> >        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        vha->vp_idx, vha->req->ring_index,
> >        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> >        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >
> > The problem is that gcc fails to track the state of the condition across
> > an annotated branch.
> >
> > This slightly rearranges the code to move the second if() block
> > into the first one, to avoid the warning while retaining the
> > behavior of the code.
> 
> When the first 'if' is true the vha->req->ring_index gets a new value 
> assigned - so it could be possible that the second 'if' wont be true any more.
> The code should not be merged into that single 'if', or am I missing something?
> 
> tomash

If the first "if" is false, the second "if" will be false also, because
the vha->req->cnt value has not changed.  If the first "if" is true, the
nested second "if" will retest the condition.

The compiler is not at fault, because vha->req->cnt can't be tracked as
it could be modified by another thread/process.  It isn't, it's protected
by the ->hardware_lock, but the compiler doesn't know that.

-Ewan

> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> > index 985231900aca..8a44d1541eb4 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
> >  		else
> >  			vha->req->cnt = vha->req->length -
> >  			    (vha->req->ring_index - cnt);
> > -	}
> >  
> > -	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> > -		ql_dbg(ql_dbg_io, vha, 0x305a,
> > -		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> > -		    vha->vp_idx, vha->req->ring_index,
> > -		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> > -		return -EAGAIN;
> > +		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> > +			ql_dbg(ql_dbg_io, vha, 0x305a,
> > +			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> > +			    vha->vp_idx, vha->req->ring_index,
> > +			    vha->req->cnt, req_cnt, cnt, cnt_in,
> > +			    vha->req->length);
> > +			return -EAGAIN;
> > +		}
> >  	}
> > +
> >  	vha->req->cnt -= req_cnt;
> >  
> >  	return 0;
> 

Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning

[Martin K. Petersen]

>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:

Arnd> The qlt_check_reserve_free_req() function produces an incorrect
Arnd> warning when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:

Applied to 4.6/scsi-fixes.

-- 
Martin K. Petersen	Oracle Linux Engineering