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 <[email protected]>
---
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
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
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
On 3/16/16, 5:59 AM, "Arnd Bergmann" <[email protected]> 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 <[email protected]>
>
>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
On Wed, 2016-03-16 at 14:05 +0000, Himanshu Madhani wrote:
>
> On 3/16/16, 5:59 AM, "Arnd Bergmann" <[email protected]> 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 <[email protected]>
Well, OK that's good enough for me.
James
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 <[email protected]>
> ---
> 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;
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 <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> > ---
> > 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;
>
>>>>> "Arnd" == Arnd Bergmann <[email protected]> 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