Re: [PATCH] scsi: remove useless variable assignment

[James Bottomley]

On Thu, 2017-05-18 at 12:41 -0500, Gustavo A. R. Silva wrote:
> Hi James,
> 
> Quoting James Bottomley :
> 
> > On Wed, 2017-05-17 at 19:30 -0500, Gustavo A. R. Silva wrote:
> > > Remove this assignment once the value stored in variable _k_ is
> > > overwritten after a few lines.
> > > 
> > > Addresses-Coverity-ID: 1226927
> > > Signed-off-by: Gustavo A. R. Silva 
> > > ---
> > >  drivers/scsi/qlogicfas408.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/drivers/scsi/qlogicfas408.c
> > > b/drivers/scsi/qlogicfas408.c
> > > index c3a9151..269440a 100644
> > > --- a/drivers/scsi/qlogicfas408.c
> > > +++ b/drivers/scsi/qlogicfas408.c
> > > @@ -329,7 +329,6 @@ static unsigned int ql_pcmd(struct scsi_cmnd
> > > *cmd)
> > >*/
> > >   if ((k = ql_wai(priv)))
> > >   return (k << 16);
> > > - k = inb(qbase + 5); /* should be 0x10,
> > > bus
> > > service */
> > 
> > That doesn't look right to me.  inb() is a statement which has an
> > effect on the I/O device regardless of whether the returned value
> > is
> > used or discarded.  In this case I think it's being used to clear
> > pending interrupts, so removing it will likely cause a phase error.
> > 
> 
> You are right, I get it.
> 
> In this case I think a patch to ignore the return value could be
> applied:
> 
> index c3a9151..8f5339a 100644
> --- a/drivers/scsi/qlogicfas408.c
> +++ b/drivers/scsi/qlogicfas408.c
> @@ -329,7 +329,7 @@ static unsigned int ql_pcmd(struct scsi_cmnd
> *cmd)
>   */
>  if ((k = ql_wai(priv)))
>  return (k << 16);
> -   k = inb(qbase + 5); /* should be 0x10, bus
> service */
> +   inb(qbase + 5); /* should be 0x10, bus service */
>  }
> 
> What do you think?

Really, no ... fix coverity.  The pattern of  = inX(something) is
perfectly correct kernel code even if the actual value of  is
never used again.  Unless there's some security bug possibility I'm not
seeing, I don't think the pattern needs altering.

In theory (void)inX() is the slightly more correct way to do this in
that it tells the compiler you need to read from here and you're
deliberately discarding the value but I don't see any value to
enforcing that.

James

[PATCH] scsi: remove useless variable assignment

[Gustavo A. R. Silva]

Remove both variable assignments once the value stored in variable _reqlen_
is overwritten at some point either by line 2321: reqlen = mptr - msg;
or by line 2330: reqlen = 12;

Addresses-Coverity-ID: 1226930
Addresses-Coverity-ID: 1226931
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/scsi/dpt_i2o.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 256dd67..acad668 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2292,11 +2292,8 @@ static s32 adpt_scsi_to_i2o(adpt_hba* pHba,  
struct scsi_cmnd* cmd, struct adpt_d

mptr+=4;
lenptr=mptr++;  /* Remember me - fill in when we know */
if (dpt_dma64(pHba)) {
-   reqlen = 16;// SINGLE SGE
*mptr++ = (0x7C<<24)+(2<<16)+0x02; /* Enable 64 bit */
*mptr++ = 1 << PAGE_SHIFT;
-   } else {
-   reqlen = 14;// SINGLE SGE
}
/* Now fill in the SGList and command */

--
2.5.0




--- Begin Message ---
Remove both variable assignments once the value stored in variable _reqlen_
is overwritten at some point either by line 2321: reqlen = mptr - msg;
or by line 2330: reqlen = 12;

Addresses-Coverity-ID: 1226930
Addresses-Coverity-ID: 1226931
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/scsi/dpt_i2o.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 256dd67..acad668 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2292,11 +2292,8 @@ static s32 adpt_scsi_to_i2o(adpt_hba* pHba, struct 
scsi_cmnd* cmd, struct adpt_d
mptr+=4;
lenptr=mptr++;  /* Remember me - fill in when we know */
if (dpt_dma64(pHba)) {
-   reqlen = 16;// SINGLE SGE
*mptr++ = (0x7C<<24)+(2<<16)+0x02; /* Enable 64 bit */
*mptr++ = 1 << PAGE_SHIFT;
-   } else {
-   reqlen = 14;// SINGLE SGE
}
/* Now fill in the SGList and command */
 
-- 
2.5.0

--- End Message ---

Re: [PATCH] scsi: remove useless variable assignment

[Gustavo A. R. Silva]

Hi James,

Quoting James Bottomley :


On Wed, 2017-05-17 at 19:30 -0500, Gustavo A. R. Silva wrote:

Remove this assignment once the value stored in variable _k_ is
overwritten after a few lines.

Addresses-Coverity-ID: 1226927
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/scsi/qlogicfas408.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/qlogicfas408.c
b/drivers/scsi/qlogicfas408.c
index c3a9151..269440a 100644
--- a/drivers/scsi/qlogicfas408.c
+++ b/drivers/scsi/qlogicfas408.c
@@ -329,7 +329,6 @@ static unsigned int ql_pcmd(struct scsi_cmnd
*cmd)
 */
if ((k = ql_wai(priv)))
return (k << 16);
-   k = inb(qbase + 5); /* should be 0x10, bus
service */


That doesn't look right to me.  inb() is a statement which has an
effect on the I/O device regardless of whether the returned value is
used or discarded.  In this case I think it's being used to clear
pending interrupts, so removing it will likely cause a phase error.



You are right, I get it.

In this case I think a patch to ignore the return value could be applied:

index c3a9151..8f5339a 100644
--- a/drivers/scsi/qlogicfas408.c
+++ b/drivers/scsi/qlogicfas408.c
@@ -329,7 +329,7 @@ static unsigned int ql_pcmd(struct scsi_cmnd *cmd)
 */
if ((k = ql_wai(priv)))
return (k << 16);
-   k = inb(qbase + 5); /* should be 0x10, bus service */
+   inb(qbase + 5); /* should be 0x10, bus service */
}

What do you think?

Thank you for the clarification.
--
Gustavo A. R. Silva

Re: [PATCH] scsi: remove useless variable assignment

[James Bottomley]

On Wed, 2017-05-17 at 19:30 -0500, Gustavo A. R. Silva wrote:
> Remove this assignment once the value stored in variable _k_ is
> overwritten after a few lines.
> 
> Addresses-Coverity-ID: 1226927
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/scsi/qlogicfas408.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/qlogicfas408.c
> b/drivers/scsi/qlogicfas408.c
> index c3a9151..269440a 100644
> --- a/drivers/scsi/qlogicfas408.c
> +++ b/drivers/scsi/qlogicfas408.c
> @@ -329,7 +329,6 @@ static unsigned int ql_pcmd(struct scsi_cmnd
> *cmd)
>*/
>   if ((k = ql_wai(priv)))
>   return (k << 16);
> - k = inb(qbase + 5); /* should be 0x10, bus
> service */

That doesn't look right to me.  inb() is a statement which has an
effect on the I/O device regardless of whether the returned value is
used or discarded.  In this case I think it's being used to clear
pending interrupts, so removing it will likely cause a phase error.

James

[PATCH] scsi: remove useless variable assignment

[Gustavo A. R. Silva]

Remove both variable assignments once the value stored in variable _reqlen_
is overwritten at some point either by line 2321: reqlen = mptr - msg;
or by line 2330: reqlen = 12;

Addresses-Coverity-ID: 1226930
Addresses-Coverity-ID: 1226931
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/scsi/dpt_i2o.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 256dd67..acad668 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2292,11 +2292,8 @@ static s32 adpt_scsi_to_i2o(adpt_hba* pHba, struct 
scsi_cmnd* cmd, struct adpt_d
mptr+=4;
lenptr=mptr++;  /* Remember me - fill in when we know */
if (dpt_dma64(pHba)) {
-   reqlen = 16;// SINGLE SGE
*mptr++ = (0x7C<<24)+(2<<16)+0x02; /* Enable 64 bit */
*mptr++ = 1 << PAGE_SHIFT;
-   } else {
-   reqlen = 14;// SINGLE SGE
}
/* Now fill in the SGList and command */
 
-- 
2.5.0

[PATCH] scsi: remove useless variable assignment

[Gustavo A. R. Silva]

Remove this assignment once the value stored in variable _k_ is
overwritten after a few lines.

Addresses-Coverity-ID: 1226927
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/scsi/qlogicfas408.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/qlogicfas408.c b/drivers/scsi/qlogicfas408.c
index c3a9151..269440a 100644
--- a/drivers/scsi/qlogicfas408.c
+++ b/drivers/scsi/qlogicfas408.c
@@ -329,7 +329,6 @@ static unsigned int ql_pcmd(struct scsi_cmnd *cmd)
 */
if ((k = ql_wai(priv)))
return (k << 16);
-   k = inb(qbase + 5); /* should be 0x10, bus service */
}
 
/*
-- 
2.5.0