| Message ID | 20190531185306.41290-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch next (8150a153c013aa2dd1ffae43370b89ac1347a7fb) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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
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
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); >
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 >
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);
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(-)