Skip to contentSkip to conversation

scsi: remove useless variable assignment

PATCH In review Gustavo A. R. Silva 2 participants 4 messages May 18, 2017 – Jun 10, 2017 SCSI

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 scsi: remove useless variable assignment

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 <[email protected]>
---
 drivers/scsi/qlogicfas408.c | 1 -
 1 file changed, 1 deletion(-)
drivers/scsi/qlogicfas408.c +0 -1
index c3a9151..269440a 100644
@@ -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 */
 	}
 
 	/*
Signature · 1 line
2.5.0
#1 lore raw
Quoted 20 lines · 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 <[email protected]>
---
 drivers/scsi/qlogicfas408.c | 1 -
 1 file changed, 1 deletion(-)
drivers/scsi/qlogicfas408.c +0 -1
index c3a9151..269440a 100644
@@ -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
Quoted 1 line

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

author
#2 lore raw

Hi James,

Quoting James Bottomley <[email protected]>:

Quoted 19 lines · Gustavo A. R. Silva
Quoted 18 lines

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 <[email protected]>
---
 drivers/scsi/qlogicfas408.c | 1 -
 1 file changed, 1 deletion(-)
drivers/scsi/qlogicfas408.c +0 -0
index c3a9151..269440a 100644
@@ -329,7 +329,6 @@ static unsigned int ql_pcmd(struct scsi_cmnd
 *cmd)
 		 */
 		if ((k = ql_wai(priv)))
Quoted 9 lines
Quoted 3 lines
drivers/scsi/qlogicfas408.c +0 -1
 			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

drivers/scsi/qlogicfas408.c +1 -1
@@ -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

#3 lore raw
Quoted 51 lines · Gustavo A. R. Silva

Hi James,

Quoting James Bottomley <[email protected]>:

Quoted 21 lines · Gustavo A. R. Silva
Quoted 20 lines

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 <[email protected]>
---
 drivers/scsi/qlogicfas408.c | 1 -
 1 file changed, 1 deletion(-)
drivers/scsi/qlogicfas408.c +0 -1
index c3a9151..269440a 100644
@@ -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,
Quoted 9 lines
Quoted 2 lines

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

drivers/scsi/qlogicfas408.c +0 -1
@@ -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 */

Quoted 3 lines
         }

What do you think?

Really, no ... fix coverity. The pattern of <var> = inX(something) is perfectly correct kernel code even if the actual value of <var> 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