[2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[Adrian Bunk]

This patch removes dead code spotted by the Coverity checker.

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

 drivers/scsi/qla4xxx/ql4_isr.c |   18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

--- linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c.old	2008-02-19 20:29:16.000000000 +0200
+++ linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c	2008-02-19 20:30:37.000000000 +0200
@@ -91,38 +91,22 @@ static void qla4xxx_status_entry(struct 
 		if (scsi_status == 0) {
 			cmd->result = DID_OK << 16;
 			break;
 		}
 
 		if (sts_entry->iscsiFlags & ISCSI_FLAG_RESIDUAL_OVER) {
 			cmd->result = DID_ERROR << 16;
 			break;
 		}
 
-		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
+		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
 			scsi_set_resid(cmd, residual);
-			if (!scsi_status && ((scsi_bufflen(cmd) - residual) <
-				cmd->underflow)) {
-
-				cmd->result = DID_ERROR << 16;
-
-				DEBUG2(printk("scsi%ld:%d:%d:%d: %s: "
-					"Mid-layer Data underrun0, "
-					"xferlen = 0x%x, "
-					"residual = 0x%x\n", ha->host_no,
-					cmd->device->channel,
-					cmd->device->id,
-					cmd->device->lun, __func__,
-					scsi_bufflen(cmd), residual));
-				break;
-			}
-		}
 
 		cmd->result = DID_OK << 16 | scsi_status;
 
 		if (scsi_status != SCSI_CHECK_CONDITION)
 			break;
 
 		/* Copy Sense Data into sense buffer. */
 		memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 
 		sensebytecnt = le16_to_cpu(sts_entry->senseDataByteCnt);

Re: [2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[James Bottomley]

On Tue, 2008-02-19 at 21:29 +0200, Adrian Bunk wrote:
> This patch removes dead code spotted by the Coverity checker.
> 
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
> 
> ---
> 
>  drivers/scsi/qla4xxx/ql4_isr.c |   18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> --- linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c.old	2008-02-19 20:29:16.000000000 +0200
> +++ linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c	2008-02-19 20:30:37.000000000 +0200
> @@ -91,38 +91,22 @@ static void qla4xxx_status_entry(struct 
>  		if (scsi_status == 0) {
>  			cmd->result = DID_OK << 16;
>  			break;
>  		}
>  
>  		if (sts_entry->iscsiFlags & ISCSI_FLAG_RESIDUAL_OVER) {
>  			cmd->result = DID_ERROR << 16;
>  			break;
>  		}
>  
> -		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
> +		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
>  			scsi_set_resid(cmd, residual);
> -			if (!scsi_status && ((scsi_bufflen(cmd) - residual) <
> -				cmd->underflow)) {
> -
> -				cmd->result = DID_ERROR << 16;
> -
> -				DEBUG2(printk("scsi%ld:%d:%d:%d: %s: "
> -					"Mid-layer Data underrun0, "
> -					"xferlen = 0x%x, "
> -					"residual = 0x%x\n", ha->host_no,
> -					cmd->device->channel,
> -					cmd->device->id,
> -					cmd->device->lun, __func__,
> -					scsi_bufflen(cmd), residual));
> -				break;
> -			}
> -		}

This code doesn't look dead to me, it looks to be enforcing
cmd->underrun if set ... what makes the coverity checker think it can
never be executed?

James

Re: [2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[Andrew Vasquez]

On Tue, 19 Feb 2008, James Bottomley wrote:

> On Tue, 2008-02-19 at 21:29 +0200, Adrian Bunk wrote:
> > This patch removes dead code spotted by the Coverity checker.
> > 
> > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > 
> > ---
> > 
> >  drivers/scsi/qla4xxx/ql4_isr.c |   18 +-----------------
> >  1 file changed, 1 insertion(+), 17 deletions(-)
> > 
> > --- linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c.old	2008-02-19 20:29:16.000000000 +0200
> > +++ linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c	2008-02-19 20:30:37.000000000 +0200
> > @@ -91,38 +91,22 @@ static void qla4xxx_status_entry(struct 
> >  		if (scsi_status == 0) {
> >  			cmd->result = DID_OK << 16;
> >  			break;
> >  		}
> >  
> >  		if (sts_entry->iscsiFlags & ISCSI_FLAG_RESIDUAL_OVER) {
> >  			cmd->result = DID_ERROR << 16;
> >  			break;
> >  		}
> >  
> > -		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
> > +		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
> >  			scsi_set_resid(cmd, residual);
> > -			if (!scsi_status && ((scsi_bufflen(cmd) - residual) <
> > -				cmd->underflow)) {
> > -
> > -				cmd->result = DID_ERROR << 16;
> > -
> > -				DEBUG2(printk("scsi%ld:%d:%d:%d: %s: "
> > -					"Mid-layer Data underrun0, "
> > -					"xferlen = 0x%x, "
> > -					"residual = 0x%x\n", ha->host_no,
> > -					cmd->device->channel,
> > -					cmd->device->id,
> > -					cmd->device->lun, __func__,
> > -					scsi_bufflen(cmd), residual));
> > -				break;
> > -			}
> > -		}
> 
> This code doesn't look dead to me, it looks to be enforcing
> cmd->underrun if set ... what makes the coverity checker think it can
> never be executed?

Hmm, guess it's the earlier 'if (scsi_status == 0)' check a few lines
up...  Dave S., can you take a look at this...  Thanks, av

Re: [2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[James Bottomley]

On Tue, 2008-02-19 at 18:35 -0800, Andrew Vasquez wrote:
> On Tue, 19 Feb 2008, James Bottomley wrote:
> 
> > On Tue, 2008-02-19 at 21:29 +0200, Adrian Bunk wrote:
> > > This patch removes dead code spotted by the Coverity checker.
> > > 
> > > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > > 
> > > ---
> > > 
> > >  drivers/scsi/qla4xxx/ql4_isr.c |   18 +-----------------
> > >  1 file changed, 1 insertion(+), 17 deletions(-)
> > > 
> > > --- linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c.old	2008-02-19 20:29:16.000000000 +0200
> > > +++ linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c	2008-02-19 20:30:37.000000000 +0200
> > > @@ -91,38 +91,22 @@ static void qla4xxx_status_entry(struct 
> > >  		if (scsi_status == 0) {
> > >  			cmd->result = DID_OK << 16;
> > >  			break;
> > >  		}
> > >  
> > >  		if (sts_entry->iscsiFlags & ISCSI_FLAG_RESIDUAL_OVER) {
> > >  			cmd->result = DID_ERROR << 16;
> > >  			break;
> > >  		}
> > >  
> > > -		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
> > > +		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
> > >  			scsi_set_resid(cmd, residual);
> > > -			if (!scsi_status && ((scsi_bufflen(cmd) - residual) <
> > > -				cmd->underflow)) {
> > > -
> > > -				cmd->result = DID_ERROR << 16;
> > > -
> > > -				DEBUG2(printk("scsi%ld:%d:%d:%d: %s: "
> > > -					"Mid-layer Data underrun0, "
> > > -					"xferlen = 0x%x, "
> > > -					"residual = 0x%x\n", ha->host_no,
> > > -					cmd->device->channel,
> > > -					cmd->device->id,
> > > -					cmd->device->lun, __func__,
> > > -					scsi_bufflen(cmd), residual));
> > > -				break;
> > > -			}
> > > -		}
> > 
> > This code doesn't look dead to me, it looks to be enforcing
> > cmd->underrun if set ... what makes the coverity checker think it can
> > never be executed?
> 
> Hmm, guess it's the earlier 'if (scsi_status == 0)' check a few lines
> up...  Dave S., can you take a look at this...  Thanks, av

Ah, so the !scsi_status is wrong it was supposed to be scsi_status !=
0 ... and even then it can just be dropped.

James

Re: [2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[Andrew Vasquez]

On Tue, 19 Feb 2008, James Bottomley wrote:

> On Tue, 2008-02-19 at 18:35 -0800, Andrew Vasquez wrote:
> > On Tue, 19 Feb 2008, James Bottomley wrote:
> > 
> > > On Tue, 2008-02-19 at 21:29 +0200, Adrian Bunk wrote:
> > > > This patch removes dead code spotted by the Coverity checker.
> > > > 
> > > > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > > > 
> > > > ---
> > > > 
> > > >  drivers/scsi/qla4xxx/ql4_isr.c |   18 +-----------------
> > > >  1 file changed, 1 insertion(+), 17 deletions(-)
> > > > 
> > > > --- linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c.old	2008-02-19 20:29:16.000000000 +0200
> > > > +++ linux-2.6/drivers/scsi/qla4xxx/ql4_isr.c	2008-02-19 20:30:37.000000000 +0200
> > > > @@ -91,38 +91,22 @@ static void qla4xxx_status_entry(struct 
> > > >  		if (scsi_status == 0) {
> > > >  			cmd->result = DID_OK << 16;
> > > >  			break;
> > > >  		}
> > > >  
> > > >  		if (sts_entry->iscsiFlags & ISCSI_FLAG_RESIDUAL_OVER) {
> > > >  			cmd->result = DID_ERROR << 16;
> > > >  			break;
> > > >  		}
> > > >  
> > > > -		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
> > > > +		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
> > > >  			scsi_set_resid(cmd, residual);
> > > > -			if (!scsi_status && ((scsi_bufflen(cmd) - residual) <
> > > > -				cmd->underflow)) {
> > > > -
> > > > -				cmd->result = DID_ERROR << 16;
> > > > -
> > > > -				DEBUG2(printk("scsi%ld:%d:%d:%d: %s: "
> > > > -					"Mid-layer Data underrun0, "
> > > > -					"xferlen = 0x%x, "
> > > > -					"residual = 0x%x\n", ha->host_no,
> > > > -					cmd->device->channel,
> > > > -					cmd->device->id,
> > > > -					cmd->device->lun, __func__,
> > > > -					scsi_bufflen(cmd), residual));
> > > > -				break;
> > > > -			}
> > > > -		}
> > > 
> > > This code doesn't look dead to me, it looks to be enforcing
> > > cmd->underrun if set ... what makes the coverity checker think it can
> > > never be executed?
> > 
> > Hmm, guess it's the earlier 'if (scsi_status == 0)' check a few lines
> > up...  Dave S., can you take a look at this...  Thanks, av
> 
> Ah, so the !scsi_status is wrong it was supposed to be scsi_status !=
> 0 ... and even then it can just be dropped.

My guess is that the check should have been written as:

	...
	if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
		scsi_set_resid(cmd, residual);
		if ((scsi_bufflen(cmd) - residual) < cmd->underflow) {
			...

It looks to be a logic-error while porting from qla2xxx, where
scsi_status during CS_COMPLETE is the full 16-bit status (high-byte is
transport, low-byte SCSI status) from from the FCP_RSP frame (not so
in iSCSI, where it's just the SCSI-status) and the residual check
in qla_isr.c::qla2x00_status_entry() looks like:

	if (!lscsi_status &&
	    ((unsigned)(scsi_bufflen(cp) - resid) <
	     cp->underflow)) {
		...

I'll defer to Dave S. for verification.

RE: [2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[David Somayajulu]

 
Andrew Vasquez wrote:
<snip>
> > > Hmm, guess it's the earlier 'if (scsi_status == 0)' check 
> a few lines
> > > up...  Dave S., can you take a look at this...  Thanks, av
> > 
> > Ah, so the !scsi_status is wrong it was supposed to be 
> scsi_status !=
> > 0 ... and even then it can just be dropped.
> 
> My guess is that the check should have been written as:
> 
> 	...
> 	if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER)
> 		scsi_set_resid(cmd, residual);
> 		if ((scsi_bufflen(cmd) - residual) < cmd->underflow) {
> 			...
> 
> It looks to be a logic-error while porting from qla2xxx, where
> scsi_status during CS_COMPLETE is the full 16-bit status (high-byte is
> transport, low-byte SCSI status) from from the FCP_RSP frame (not so
> in iSCSI, where it's just the SCSI-status) and the residual check
> in qla_isr.c::qla2x00_status_entry() looks like:
> 
> 	if (!lscsi_status &&
> 	    ((unsigned)(scsi_bufflen(cp) - resid) <
> 	     cp->underflow)) {
> 		...
> 
> I'll defer to Dave S. for verification.
> 
The correct patch needs to be 

Signed-off-by: David C Somayajulu <david.somayajulu@qlogic.com>

---

diff --git a/drivers/scsi/qla4xxx/ql4_isr.c
b/drivers/scsi/qla4xxx/ql4_isr.c
index 0f029d0..fc84db4 100644
--- a/drivers/scsi/qla4xxx/ql4_isr.c
+++ b/drivers/scsi/qla4xxx/ql4_isr.c
@@ -100,8 +100,7 @@ static void qla4xxx_status_entry(struct
 
 		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
 			scsi_set_resid(cmd, residual);
-			if (!scsi_status && ((scsi_bufflen(cmd) -
residual) <
-				cmd->underflow)) {
+			if ((scsi_bufflen(cmd) - residual) <
cmd->underflow) {
 
 				cmd->result = DID_ERROR << 16;
 

Re: [2.6 patch] scsi/qla4xxx/ql4_isr.c: remove dead code

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

> The correct patch needs to be
>
> Signed-off-by: David C Somayajulu <david.somayajulu@qlogic.com>
>
> ---
>
> diff --git a/drivers/scsi/qla4xxx/ql4_isr.c
> b/drivers/scsi/qla4xxx/ql4_isr.c
> index 0f029d0..fc84db4 100644
> --- a/drivers/scsi/qla4xxx/ql4_isr.c
> +++ b/drivers/scsi/qla4xxx/ql4_isr.c
> @@ -100,8 +100,7 @@ static void qla4xxx_status_entry(struct
>
>  		if (sts_entry->iscsiFlags &ISCSI_FLAG_RESIDUAL_UNDER) {
>  			scsi_set_resid(cmd, residual);
> -			if (!scsi_status && ((scsi_bufflen(cmd) -
> residual) <
> -				cmd->underflow)) {
> +			if ((scsi_bufflen(cmd) - residual) <
> cmd->underflow) {

The patch is line-wrapped and can' be applied.

Greetings,

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]