* [PATCH] qla2xxx: avoid maybe_uninitialized warning
@ 2016-03-15 21:40 Arnd Bergmann
2016-03-15 21:49 ` James Bottomley
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Arnd Bergmann @ 2016-03-15 21:40 UTC (permalink / raw
To: qla2xxx-upstream, James E.J. Bottomley, Martin K. Petersen
Cc: Arnd Bergmann, Nicholas Bellinger, Himanshu Madhani, Quinn Tran,
Alexei Potashnik, Bart Van Assche, Swapnil Nagle, linux-scsi,
linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-15 21:40 [PATCH] qla2xxx: avoid maybe_uninitialized warning Arnd Bergmann
@ 2016-03-15 21:49 ` James Bottomley
2016-03-16 12:59 ` Arnd Bergmann
2016-03-16 15:03 ` Tomas Henzl
2016-03-18 19:26 ` Martin K. Petersen
2 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2016-03-15 21:49 UTC (permalink / raw
To: Arnd Bergmann, qla2xxx-upstream, Martin K. Petersen
Cc: Nicholas Bellinger, Himanshu Madhani, Quinn Tran,
Alexei Potashnik, Bart Van Assche, Swapnil Nagle, linux-scsi,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-15 21:49 ` James Bottomley
@ 2016-03-16 12:59 ` Arnd Bergmann
2016-03-16 14:05 ` Himanshu Madhani
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-03-16 12:59 UTC (permalink / raw
To: James Bottomley
Cc: qla2xxx-upstream, Martin K. Petersen, Nicholas Bellinger,
Himanshu Madhani, Quinn Tran, Alexei Potashnik, Bart Van Assche,
Swapnil Nagle, linux-scsi, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-16 12:59 ` Arnd Bergmann
@ 2016-03-16 14:05 ` Himanshu Madhani
0 siblings, 0 replies; 10+ messages in thread
From: Himanshu Madhani @ 2016-03-16 14:05 UTC (permalink / raw
To: Arnd Bergmann, James Bottomley
Cc: Dept-Eng QLA2xxx Upstream, Martin K. Petersen, Nicholas Bellinger,
Quinn Tran, Alexei Potashnik, Bart Van Assche, Swapnil Nagle,
linux-scsi, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
@ 2016-03-16 14:05 ` Himanshu Madhani
0 siblings, 0 replies; 10+ messages in thread
From: Himanshu Madhani @ 2016-03-16 14:05 UTC (permalink / raw
To: Arnd Bergmann, James Bottomley
Cc: Dept-Eng QLA2xxx Upstream, Martin K. Petersen, Nicholas Bellinger,
Quinn Tran, Alexei Potashnik, Bart Van Assche, Swapnil Nagle,
linux-scsi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]
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
[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4603 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-16 14:05 ` Himanshu Madhani
(?)
@ 2016-03-16 14:24 ` James Bottomley
-1 siblings, 0 replies; 10+ messages in thread
From: James Bottomley @ 2016-03-16 14:24 UTC (permalink / raw
To: Himanshu Madhani, Arnd Bergmann
Cc: Dept-Eng QLA2xxx Upstream, Martin K. Petersen, Nicholas Bellinger,
Quinn Tran, Alexei Potashnik, Bart Van Assche, Swapnil Nagle,
linux-scsi, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-15 21:40 [PATCH] qla2xxx: avoid maybe_uninitialized warning Arnd Bergmann
2016-03-15 21:49 ` James Bottomley
@ 2016-03-16 15:03 ` Tomas Henzl
2016-03-16 15:11 ` Tomas Henzl
2016-03-16 15:47 ` Ewan D. Milne
2016-03-18 19:26 ` Martin K. Petersen
2 siblings, 2 replies; 10+ messages in thread
From: Tomas Henzl @ 2016-03-16 15:03 UTC (permalink / raw
To: Arnd Bergmann, qla2xxx-upstream, James E.J. Bottomley,
Martin K. Petersen
Cc: Nicholas Bellinger, Himanshu Madhani, Quinn Tran,
Alexei Potashnik, Bart Van Assche, Swapnil Nagle, linux-scsi,
linux-kernel
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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-16 15:03 ` Tomas Henzl
@ 2016-03-16 15:11 ` Tomas Henzl
2016-03-16 15:47 ` Ewan D. Milne
1 sibling, 0 replies; 10+ messages in thread
From: Tomas Henzl @ 2016-03-16 15:11 UTC (permalink / raw
To: Arnd Bergmann, qla2xxx-upstream, James E.J. Bottomley,
Martin K. Petersen
Cc: Nicholas Bellinger, Himanshu Madhani, Quinn Tran,
Alexei Potashnik, Bart Van Assche, Swapnil Nagle, linux-scsi,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-16 15:03 ` Tomas Henzl
2016-03-16 15:11 ` Tomas Henzl
@ 2016-03-16 15:47 ` Ewan D. Milne
1 sibling, 0 replies; 10+ messages in thread
From: Ewan D. Milne @ 2016-03-16 15:47 UTC (permalink / raw
To: Tomas Henzl
Cc: Arnd Bergmann, qla2xxx-upstream, James E.J. Bottomley,
Martin K. Petersen, Nicholas Bellinger, Himanshu Madhani,
Quinn Tran, Alexei Potashnik, Bart Van Assche, Swapnil Nagle,
linux-scsi, linux-kernel
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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
2016-03-15 21:40 [PATCH] qla2xxx: avoid maybe_uninitialized warning Arnd Bergmann
2016-03-15 21:49 ` James Bottomley
2016-03-16 15:03 ` Tomas Henzl
@ 2016-03-18 19:26 ` Martin K. Petersen
2 siblings, 0 replies; 10+ messages in thread
From: Martin K. Petersen @ 2016-03-18 19:26 UTC (permalink / raw
To: Arnd Bergmann
Cc: qla2xxx-upstream, James E.J. Bottomley, Martin K. Petersen,
Nicholas Bellinger, Himanshu Madhani, Quinn Tran,
Alexei Potashnik, Bart Van Assche, Swapnil Nagle, linux-scsi,
linux-kernel
>>>>> "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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-18 19:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-15 21:40 [PATCH] qla2xxx: avoid maybe_uninitialized warning Arnd Bergmann
2016-03-15 21:49 ` James Bottomley
2016-03-16 12:59 ` Arnd Bergmann
2016-03-16 14:05 ` Himanshu Madhani
2016-03-16 14:05 ` Himanshu Madhani
2016-03-16 14:24 ` James Bottomley
2016-03-16 15:03 ` Tomas Henzl
2016-03-16 15:11 ` Tomas Henzl
2016-03-16 15:47 ` Ewan D. Milne
2016-03-18 19:26 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.