* Re: [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-05-31 18:53 [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work Nathan Chancellor
@ 2019-06-02 10:15 ` Michael Ellerman
2019-06-03 3:23 ` Nathan Chancellor
2019-06-03 23:30 ` Tyrel Datwyler
2019-06-03 8:45 ` Christophe Leroy
2019-06-03 22:19 ` [PATCH v2] " Nathan Chancellor
2 siblings, 2 replies; 13+ messages in thread
From: Michael Ellerman @ 2019-06-02 10:15 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
Martin K. Petersen
Cc: clang-built-linux, Nathan Chancellor, linuxppc-dev, linux-kernel,
linux-scsi
Hi Nathan,
Nathan Chancellor <natechancellor@gmail.com> writes:
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
> if (rc) {
> ^~
>
> Initialize rc to zero so that the atomic_set and dev_err statement don't
> trigger for the cases that just break.
>
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..6714d8043e62 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2118,7 +2118,7 @@ static unsigned long ibmvscsi_get_desired_dma(struct vio_dev *vdev)
> static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
> {
> unsigned long flags;
> - int rc;
> + int rc = 0;
> char *action = "reset";
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
It's always preferable IMHO to keep any initialisation as localised as
possible, so that the compiler can continue to warn about uninitialised
usages elsewhere. In this case that would mean doing the rc = 0 in the
switch, something like:
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 727c31dc11a0..7ee5755cf636 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2123,9 +2123,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
- case IBMVSCSI_HOST_ACTION_NONE:
- case IBMVSCSI_HOST_ACTION_UNBLOCK:
- break;
case IBMVSCSI_HOST_ACTION_RESET:
spin_unlock_irqrestore(hostdata->host->host_lock, flags);
rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
@@ -2142,7 +2139,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
break;
+ case IBMVSCSI_HOST_ACTION_NONE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
default:
+ rc = 0;
break;
}
But then that makes me wonder if that's actually correct?
If we get an action that we don't recognise should we just throw it away
like that? (by doing hostdata->action = IBMVSCSI_HOST_ACTION_NONE). Tyrel?
cheers
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-02 10:15 ` Michael Ellerman
@ 2019-06-03 3:23 ` Nathan Chancellor
2019-06-03 23:30 ` Tyrel Datwyler
1 sibling, 0 replies; 13+ messages in thread
From: Nathan Chancellor @ 2019-06-03 3:23 UTC (permalink / raw)
To: Michael Ellerman
Cc: Tyrel Datwyler, linux-scsi, Martin K. Petersen,
James E.J. Bottomley, linux-kernel, clang-built-linux,
linuxppc-dev
Hi Michael,
On Sun, Jun 02, 2019 at 08:15:38PM +1000, Michael Ellerman wrote:
> Hi Nathan,
>
> It's always preferable IMHO to keep any initialisation as localised as
> possible, so that the compiler can continue to warn about uninitialised
> usages elsewhere. In this case that would mean doing the rc = 0 in the
> switch, something like:
I am certainly okay with implementing this in a v2. I mulled over which
would be preferred, I suppose I guessed wrong :) Thank you for the
review and input.
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..7ee5755cf636 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,9 +2123,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
> switch (hostdata->action) {
> - case IBMVSCSI_HOST_ACTION_NONE:
> - case IBMVSCSI_HOST_ACTION_UNBLOCK:
> - break;
> case IBMVSCSI_HOST_ACTION_RESET:
> spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
> @@ -2142,7 +2139,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
> if (!rc)
> rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
> break;
> + case IBMVSCSI_HOST_ACTION_NONE:
> + case IBMVSCSI_HOST_ACTION_UNBLOCK:
> default:
> + rc = 0;
> break;
> }
>
>
> But then that makes me wonder if that's actually correct?
>
> If we get an action that we don't recognise should we just throw it away
> like that? (by doing hostdata->action = IBMVSCSI_HOST_ACTION_NONE). Tyrel?
However, because of this, I will hold off on v2 until Tyrel can give
some feedback.
Thanks,
Nathan
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-02 10:15 ` Michael Ellerman
2019-06-03 3:23 ` Nathan Chancellor
@ 2019-06-03 23:30 ` Tyrel Datwyler
1 sibling, 0 replies; 13+ messages in thread
From: Tyrel Datwyler @ 2019-06-03 23:30 UTC (permalink / raw)
To: Michael Ellerman, Nathan Chancellor, Tyrel Datwyler,
James E.J. Bottomley, Martin K. Petersen
Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
On 06/02/2019 03:15 AM, Michael Ellerman wrote:
> Hi Nathan,
>
> Nathan Chancellor <natechancellor@gmail.com> writes:
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>> case IBMVSCSI_HOST_ACTION_NONE:
>> ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>> here
>> if (rc) {
>> ^~
>>
>> Initialize rc to zero so that the atomic_set and dev_err statement don't
>> trigger for the cases that just break.
>>
>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 727c31dc11a0..6714d8043e62 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -2118,7 +2118,7 @@ static unsigned long ibmvscsi_get_desired_dma(struct vio_dev *vdev)
>> static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>> {
>> unsigned long flags;
>> - int rc;
>> + int rc = 0;
>> char *action = "reset";
>>
>> spin_lock_irqsave(hostdata->host->host_lock, flags);
>
> It's always preferable IMHO to keep any initialisation as localised as
> possible, so that the compiler can continue to warn about uninitialised
> usages elsewhere. In this case that would mean doing the rc = 0 in the
> switch, something like:
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..7ee5755cf636 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,9 +2123,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
> switch (hostdata->action) {
> - case IBMVSCSI_HOST_ACTION_NONE:
> - case IBMVSCSI_HOST_ACTION_UNBLOCK:
> - break;
> case IBMVSCSI_HOST_ACTION_RESET:
> spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
> @@ -2142,7 +2139,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
> if (!rc)
> rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
> break;
> + case IBMVSCSI_HOST_ACTION_NONE:
> + case IBMVSCSI_HOST_ACTION_UNBLOCK:
> default:
> + rc = 0;
> break;
> }
>
>
> But then that makes me wonder if that's actually correct?
>
> If we get an action that we don't recognise should we just throw it away
> like that? (by doing hostdata->action = IBMVSCSI_HOST_ACTION_NONE). Tyrel?
On initial pass I was ok with this, but after thinking on it I think it is more
subtle.
The right approach is to set rc = 0 for HOST_ACTION_UNBLOCK as we want to fall
through. For HOST_ACTION_NONE and default we need to return directly from the
function.
-Tyrel
>
> cheers
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-05-31 18:53 [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work Nathan Chancellor
2019-06-02 10:15 ` Michael Ellerman
@ 2019-06-03 8:45 ` Christophe Leroy
2019-06-03 22:19 ` [PATCH v2] " Nathan Chancellor
2 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2019-06-03 8:45 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
Martin K. Petersen
Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
Le 31/05/2019 à 20:53, Nathan Chancellor a écrit :
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
> if (rc) {
> ^~
>
> Initialize rc to zero so that the atomic_set and dev_err statement don't
> trigger for the cases that just break.
>
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..6714d8043e62 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2118,7 +2118,7 @@ static unsigned long ibmvscsi_get_desired_dma(struct vio_dev *vdev)
> static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
> {
> unsigned long flags;
> - int rc;
> + int rc = 0;
I don't think the above is the best solution, as it hides the warning
instead of really fixing it.
Your problem is that some legs of the switch are missing setting the
value of rc, it would therefore be better to fix the legs instead of
setting a default value which may not be correct for every case,
allthough it may be at the time being.
Christophe
> char *action = "reset";
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-05-31 18:53 [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work Nathan Chancellor
2019-06-02 10:15 ` Michael Ellerman
2019-06-03 8:45 ` Christophe Leroy
@ 2019-06-03 22:19 ` Nathan Chancellor
2019-06-03 23:25 ` Tyrel Datwyler
2019-06-03 23:44 ` [PATCH v3] " Nathan Chancellor
2 siblings, 2 replies; 13+ messages in thread
From: Nathan Chancellor @ 2019-06-03 22:19 UTC (permalink / raw)
To: Tyrel Datwyler, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, clang-built-linux, Nathan Chancellor,
linuxppc-dev
clang warns:
drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
case IBMVSCSI_HOST_ACTION_NONE:
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
here
if (rc) {
^~
Initialize rc to zero in the case statements that clang mentions so that
the atomic_set and dev_err statement don't trigger for them.
Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
Link: https://github.com/ClangBuiltLinux/linux/issues/502
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Initialize rc in the case statements, rather than at the top of the
function, as suggested by Michael.
drivers/scsi/ibmvscsi/ibmvscsi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 65053daef5f7..3b5647d622d9 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2109,9 +2109,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
- case IBMVSCSI_HOST_ACTION_NONE:
- case IBMVSCSI_HOST_ACTION_UNBLOCK:
- break;
case IBMVSCSI_HOST_ACTION_RESET:
spin_unlock_irqrestore(hostdata->host->host_lock, flags);
rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
@@ -2128,7 +2125,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
break;
+ case IBMVSCSI_HOST_ACTION_NONE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
default:
+ rc = 0;
break;
}
--
2.22.0.rc3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-03 22:19 ` [PATCH v2] " Nathan Chancellor
@ 2019-06-03 23:25 ` Tyrel Datwyler
2019-06-03 23:34 ` Tyrel Datwyler
2019-06-03 23:44 ` [PATCH v3] " Nathan Chancellor
1 sibling, 1 reply; 13+ messages in thread
From: Tyrel Datwyler @ 2019-06-03 23:25 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
Martin K. Petersen
Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
> if (rc) {
> ^~
>
> Initialize rc to zero in the case statements that clang mentions so that
> the atomic_set and dev_err statement don't trigger for them.
>
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-03 23:25 ` Tyrel Datwyler
@ 2019-06-03 23:34 ` Tyrel Datwyler
2019-06-03 23:35 ` Tyrel Datwyler
0 siblings, 1 reply; 13+ messages in thread
From: Tyrel Datwyler @ 2019-06-03 23:34 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
Martin K. Petersen
Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
On 06/03/2019 04:25 PM, Tyrel Datwyler wrote:
> On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>> case IBMVSCSI_HOST_ACTION_NONE:
>> ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>> here
>> if (rc) {
>> ^~
>>
>> Initialize rc to zero in the case statements that clang mentions so that
>> the atomic_set and dev_err statement don't trigger for them.
>>
>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>
> Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>
On second thought NACK. See my response to Michael earlier in the thread.
I think this is the better solution:
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 727c31dc11a0..c3cf05dd8733 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2123,8 +2123,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
- case IBMVSCSI_HOST_ACTION_NONE:
case IBMVSCSI_HOST_ACTION_UNBLOCK:
+ rc = 0;
break;
case IBMVSCSI_HOST_ACTION_RESET:
spin_unlock_irqrestore(hostdata->host->host_lock, flags);
@@ -2142,8 +2142,9 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
break;
+ case IBMVSCSI_HOST_ACTION_NONE:
default:
- break;
+ return;
}
hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-03 23:34 ` Tyrel Datwyler
@ 2019-06-03 23:35 ` Tyrel Datwyler
0 siblings, 0 replies; 13+ messages in thread
From: Tyrel Datwyler @ 2019-06-03 23:35 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
Martin K. Petersen
Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
On 06/03/2019 04:34 PM, Tyrel Datwyler wrote:
> On 06/03/2019 04:25 PM, Tyrel Datwyler wrote:
>> On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
>>> clang warns:
>>>
>>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>>> case IBMVSCSI_HOST_ACTION_NONE:
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>>> here
>>> if (rc) {
>>> ^~
>>>
>>> Initialize rc to zero in the case statements that clang mentions so that
>>> the atomic_set and dev_err statement don't trigger for them.
>>>
>>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>>
>> Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>>
>
> On second thought NACK. See my response to Michael earlier in the thread.
>
> I think this is the better solution:
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..c3cf05dd8733 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,8 +2123,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
> switch (hostdata->action) {
> - case IBMVSCSI_HOST_ACTION_NONE:
> case IBMVSCSI_HOST_ACTION_UNBLOCK:
> + rc = 0;
> break;
> case IBMVSCSI_HOST_ACTION_RESET:
> spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> @@ -2142,8 +2142,9 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
> if (!rc)
> rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
> break;
> + case IBMVSCSI_HOST_ACTION_NONE:
> default:
> - break;
Need a spin_unlock_irqrestore() here before the return.
-Tyrel
> + return;
> }
>
> hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-03 22:19 ` [PATCH v2] " Nathan Chancellor
2019-06-03 23:25 ` Tyrel Datwyler
@ 2019-06-03 23:44 ` Nathan Chancellor
2019-06-03 23:58 ` Tyrel Datwyler
2019-06-05 2:39 ` Martin K. Petersen
1 sibling, 2 replies; 13+ messages in thread
From: Nathan Chancellor @ 2019-06-03 23:44 UTC (permalink / raw)
To: Tyrel Datwyler, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, clang-built-linux, Nathan Chancellor,
linuxppc-dev
clang warns:
drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
case IBMVSCSI_HOST_ACTION_NONE:
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
here
if (rc) {
^~
Initialize rc in the IBMVSCSI_HOST_ACTION_UNBLOCK case statement then
shuffle IBMVSCSI_HOST_ACTION_NONE down to the default case statement and
make it return early so that rc is never used uninitialized in this
function.
Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
Link: https://github.com/ClangBuiltLinux/linux/issues/502
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Suggested-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Initialize rc in the case statements, rather than at the top of the
function, as suggested by Michael.
v2 -> v3:
* default and IBMVSCSI_HOST_ACTION_NONE now return early from the
function, as requested by Tyrel.
drivers/scsi/ibmvscsi/ibmvscsi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 65053daef5f7..7f66a7783209 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2109,8 +2109,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
- case IBMVSCSI_HOST_ACTION_NONE:
case IBMVSCSI_HOST_ACTION_UNBLOCK:
+ rc = 0;
break;
case IBMVSCSI_HOST_ACTION_RESET:
spin_unlock_irqrestore(hostdata->host->host_lock, flags);
@@ -2128,8 +2128,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
break;
+ case IBMVSCSI_HOST_ACTION_NONE:
default:
- break;
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
+ return;
}
hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
--
2.22.0.rc3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-03 23:44 ` [PATCH v3] " Nathan Chancellor
@ 2019-06-03 23:58 ` Tyrel Datwyler
2019-06-05 2:39 ` Martin K. Petersen
1 sibling, 0 replies; 13+ messages in thread
From: Tyrel Datwyler @ 2019-06-03 23:58 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
Martin K. Petersen
Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
On 06/03/2019 04:44 PM, Nathan Chancellor wrote:
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
> if (rc) {
> ^~
>
> Initialize rc in the IBMVSCSI_HOST_ACTION_UNBLOCK case statement then
> shuffle IBMVSCSI_HOST_ACTION_NONE down to the default case statement and
> make it return early so that rc is never used uninitialized in this
> function.
>
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Suggested-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-03 23:44 ` [PATCH v3] " Nathan Chancellor
2019-06-03 23:58 ` Tyrel Datwyler
@ 2019-06-05 2:39 ` Martin K. Petersen
2019-06-05 11:08 ` Michael Ellerman
1 sibling, 1 reply; 13+ messages in thread
From: Martin K. Petersen @ 2019-06-05 2:39 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Tyrel Datwyler, Martin K. Petersen, linux-scsi,
James E.J. Bottomley, linux-kernel, clang-built-linux,
linuxppc-dev
Nathan,
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^~~~~~~~~~~~~~~~~~~~~~~~~
Applied to 5.3/scsi-queue, thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
2019-06-05 2:39 ` Martin K. Petersen
@ 2019-06-05 11:08 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2019-06-05 11:08 UTC (permalink / raw)
To: Martin K. Petersen, Nathan Chancellor
Cc: Tyrel Datwyler, Martin K. Petersen, linux-scsi,
James E.J. Bottomley, linux-kernel, clang-built-linux,
linuxppc-dev
"Martin K. Petersen" <martin.petersen@oracle.com> writes:
> Nathan,
>
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>> case IBMVSCSI_HOST_ACTION_NONE:
>> ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> Applied to 5.3/scsi-queue, thanks!
Thanks all.
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread