[PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()

[PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()

[Boaz Harrosh]

  - Abstract away scsi_cmnd->sense_buffer for later removal.

  - Removed a filtering out of a REQUEST_SENSE at .queuecommand
    In the case of sense beeing clean. This is no longer relevant
    since scsi-ml will always send a zero out sense buffer even
    on a resend, so this means outside REQUEST_SENSE would never
    go through. If this is intended then comment and check should
    change.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/dpt_i2o.c |   25 +++++++------------------
 1 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index c9dd839..6ee6fcd 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -71,6 +71,7 @@ MODULE_DESCRIPTION("Adaptec I2O RAID Driver");
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_tcq.h>
+#include <scsi/scsi_eh.h>
 
 #include "dpt/dptsig.h"
 #include "dpti.h"
@@ -385,18 +386,6 @@ static int adpt_queue(struct scsi_cmnd * cmd, void (*done) (struct scsi_cmnd *))
 	struct adpt_device* pDev = NULL;	/* dpt per device information */
 
 	cmd->scsi_done = done;
-	/*
-	 * SCSI REQUEST_SENSE commands will be executed automatically by the 
-	 * Host Adapter for any errors, so they should not be executed 
-	 * explicitly unless the Sense Data is zero indicating that no error 
-	 * occurred.
-	 */
-
-	if ((cmd->cmnd[0] == REQUEST_SENSE) && (cmd->sense_buffer[0] != 0)) {
-		cmd->result = (DID_OK << 16);
-		cmd->scsi_done(cmd);
-		return 0;
-	}
 
 	pHba = (adpt_hba*)cmd->device->host->hostdata[0];
 	if (!pHba) {
@@ -2226,8 +2215,6 @@ static s32 adpt_i2o_to_scsi(void __iomem *reply, struct scsi_cmnd* cmd)
 
 	pHba = (adpt_hba*) cmd->device->host->hostdata[0];
 
-	cmd->sense_buffer[0] = '\0';  // initialize sense valid flag to false
-
 	if(!(reply_flags & MSG_FAIL)) {
 		switch(detailed_status & I2O_SCSI_DSC_MASK) {
 		case I2O_SCSI_DSC_SUCCESS:
@@ -2297,11 +2284,13 @@ static s32 adpt_i2o_to_scsi(void __iomem *reply, struct scsi_cmnd* cmd)
 		// copy over the request sense data if it was a check
 		// condition status
 		if (dev_status == SAM_STAT_CHECK_CONDITION) {
-			u32 len = min(SCSI_SENSE_BUFFERSIZE, 40);
+			u8 sense_buffer[40];
+			u32 len = sizeof(sense_buffer);
 			// Copy over the sense data
-			memcpy_fromio(cmd->sense_buffer, (reply+28) , len);
-			if(cmd->sense_buffer[0] == 0x70 /* class 7 */ && 
-			   cmd->sense_buffer[2] == DATA_PROTECT ){
+			memcpy_fromio(sense_buffer, (reply+28) , len);
+			scsi_eh_cpy_sense(cmd, sense_buffer, len);
+			if (sense_buffer[0] == 0x70 /* class 7 */ &&
+			   sense_buffer[2] == DATA_PROTECT){
 				/* This is to handle an array failed */
 				cmd->result = (DID_TIME_OUT << 16);
 				printk(KERN_WARNING"%s: SCSI Data Protect-Device (%d,%d,%d) hba_status=0x%x, dev_status=0x%x, cmd=0x%x\n",
-- 
1.5.3.3

RE: [PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()

[Salyzyn, Mark]

ACK with condition that community accepts the RFC's entire premise.

The removed code that shunted the REQUEST_SENSE was based on the assumption that the sense data in the current scsi command packet was left over from the previous command's execution with a check condition as the scsi command packet is reused to issue the REQUEST_SENSE. For a new, or second from the target's point of view, request sense to the target issued by these older kernels would always return an erased sense. The dpt_i2o driver does not itself maintain the sense history, nor does the Firmware. This behavior, I believe, is not the case for current kernels so the code fragment made little sense (pun not intended). If my historical knowledge is correct, this (now removed) workaround makes no more sense because the scsi layer correctly manages adapters that produce auto-request sense an
 d does not ever turn around the command and send a second request for sense information.

Given this understanding, I have no problem with the removed fragment of REQUEST_SENSE shunting. However, I do urge some target error recovery testing, tape drives being the likely type of target affected by this change. I have no such hardware to confirm...

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Boaz Harrosh
> Sent: Monday, February 04, 2008 10:59 AM
> To: James Bottomley; FUJITA Tomonori; Christoph Hellwig; Jens
> Axboe; Jeff Garzik; linux-scsi
> Cc: Andrew Morton
> Subject: [PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()
>
>   - Abstract away scsi_cmnd->sense_buffer for later removal.
>
>   - Removed a filtering out of a REQUEST_SENSE at .queuecommand
>     In the case of sense beeing clean. This is no longer relevant
>     since scsi-ml will always send a zero out sense buffer even
>     on a resend, so this means outside REQUEST_SENSE would never
>     go through. If this is intended then comment and check should
>     change.
>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/dpt_i2o.c |   25 +++++++------------------
>  1 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index c9dd839..6ee6fcd 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -71,6 +71,7 @@ MODULE_DESCRIPTION("Adaptec I2O RAID Driver");
>  #include <scsi/scsi_device.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_tcq.h>
> +#include <scsi/scsi_eh.h>
>
>  #include "dpt/dptsig.h"
>  #include "dpti.h"
> @@ -385,18 +386,6 @@ static int adpt_queue(struct scsi_cmnd *
> cmd, void (*done) (struct scsi_cmnd *))
>         struct adpt_device* pDev = NULL;        /* dpt per
> device information */
>
>         cmd->scsi_done = done;
> -       /*
> -        * SCSI REQUEST_SENSE commands will be executed
> automatically by the
> -        * Host Adapter for any errors, so they should not be executed
> -        * explicitly unless the Sense Data is zero
> indicating that no error
> -        * occurred.
> -        */
> -
> -       if ((cmd->cmnd[0] == REQUEST_SENSE) &&
> (cmd->sense_buffer[0] != 0)) {
> -               cmd->result = (DID_OK << 16);
> -               cmd->scsi_done(cmd);
> -               return 0;
> -       }
>
>         pHba = (adpt_hba*)cmd->device->host->hostdata[0];
>         if (!pHba) {
> @@ -2226,8 +2215,6 @@ static s32 adpt_i2o_to_scsi(void
> __iomem *reply, struct scsi_cmnd* cmd)
>
>         pHba = (adpt_hba*) cmd->device->host->hostdata[0];
>
> -       cmd->sense_buffer[0] = '\0';  // initialize sense
> valid flag to false
> -
>         if(!(reply_flags & MSG_FAIL)) {
>                 switch(detailed_status & I2O_SCSI_DSC_MASK) {
>                 case I2O_SCSI_DSC_SUCCESS:
> @@ -2297,11 +2284,13 @@ static s32 adpt_i2o_to_scsi(void
> __iomem *reply, struct scsi_cmnd* cmd)
>                 // copy over the request sense data if it was a check
>                 // condition status
>                 if (dev_status == SAM_STAT_CHECK_CONDITION) {
> -                       u32 len = min(SCSI_SENSE_BUFFERSIZE, 40);
> +                       u8 sense_buffer[40];
> +                       u32 len = sizeof(sense_buffer);
>                         // Copy over the sense data
> -                       memcpy_fromio(cmd->sense_buffer,
> (reply+28) , len);
> -                       if(cmd->sense_buffer[0] == 0x70 /*
> class 7 */ &&
> -                          cmd->sense_buffer[2] == DATA_PROTECT ){
> +                       memcpy_fromio(sense_buffer, (reply+28) , len);
> +                       scsi_eh_cpy_sense(cmd, sense_buffer, len);
> +                       if (sense_buffer[0] == 0x70 /* class 7 */ &&
> +                          sense_buffer[2] == DATA_PROTECT){
>                                 /* This is to handle an array
> failed */
>                                 cmd->result = (DID_TIME_OUT << 16);
>                                 printk(KERN_WARNING"%s: SCSI
> Data Protect-Device (%d,%d,%d) hba_status=0x%x,
> dev_status=0x%x, cmd=0x%x\n",
> --
> 1.5.3.3
>
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()

[Boaz Harrosh]

On Mon, Feb 04 2008 at 20:32 +0200, "Salyzyn, Mark" <Mark_Salyzyn@adaptec.com> wrote:
> ACK with condition that community accepts the RFC's entire premise.
> 
> The removed code that shunted the REQUEST_SENSE was based on the assumption 
> that the sense data in the current scsi command packet was left over from the 
> previous command's execution with a check condition as the scsi command packet 
> is reused to issue the REQUEST_SENSE. For a new, or second from the target's point 
> of view, request sense to the target issued by these older kernels would always 
> return an erased sense. The dpt_i2o driver does not itself maintain the sense history, 
> nor does the Firmware. This behavior, I believe, is not the case for current kernels so 
> the code fragment made little sense (pun not intended). If my historical knowledge is 
> correct, this (now removed) workaround makes no more sense because the scsi layer correctly 
> manages adapters that produce auto-request sense and does not ever turn around the command 
> and send a second request for sense information.

> Given this understanding, I have no problem with the removed fragment of REQUEST_SENSE shunting. 
> However, I do urge some target error recovery testing, tape drives being the likely type of target 
> affected by this change. I have no such hardware to confirm...
> Sincerely -- Mark Salyzyn

I have removed this test because the midlayer does a scsi_eh_reset_sense() just before
the new invocation of a command. So even if the second bad REQUEST_SENSE comes this
will not filter it out anymore. If such a thing still happens? A driver state machine
must be used to filter it out, or of course midlayer should be fixed.

Boaz

RE: [PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()

[Salyzyn, Mark]

I do not think the midlayer needs to be fixed. I think this was a bug/feature that presented itself in the 2.2 tree when we were developing this driver in 1996...

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Boaz Harrosh [mailto:bharrosh@panasas.com]
> Sent: Tuesday, February 05, 2008 3:52 AM
> To: Salyzyn, Mark
> Cc: James Bottomley; FUJITA Tomonori; Christoph Hellwig; Jens
> Axboe; Jeff Garzik; linux-scsi; Andrew Morton
> Subject: Re: [PATCH 5/24][RFC] dpt_i2o: Use new scsi_eh_cpy_sense()
>
> On Mon, Feb 04 2008 at 20:32 +0200, "Salyzyn, Mark"
> <Mark_Salyzyn@adaptec.com> wrote:
> > ACK with condition that community accepts the RFC's entire premise.
> >
> > The removed code that shunted the REQUEST_SENSE was based
> on the assumption
> > that the sense data in the current scsi command packet was
> left over from the
> > previous command's execution with a check condition as the
> scsi command packet
> > is reused to issue the REQUEST_SENSE. For a new, or second
> from the target's point
> > of view, request sense to the target issued by these older
> kernels would always
> > return an erased sense. The dpt_i2o driver does not itself
> maintain the sense history,
> > nor does the Firmware. This behavior, I believe, is not the
> case for current kernels so
> > the code fragment made little sense (pun not intended). If
> my historical knowledge is
> > correct, this (now removed) workaround makes no more sense
> because the scsi layer correctly
> > manages adapters that produce auto-request sense and does
> not ever turn around the command
> > and send a second request for sense information.
>
> > Given this understanding, I have no problem with the
> removed fragment of REQUEST_SENSE shunting.
> > However, I do urge some target error recovery testing, tape
> drives being the likely type of target
> > affected by this change. I have no such hardware to confirm...
> > Sincerely -- Mark Salyzyn
>
> I have removed this test because the midlayer does a
> scsi_eh_reset_sense() just before
> the new invocation of a command. So even if the second bad
> REQUEST_SENSE comes this
> will not filter it out anymore. If such a thing still
> happens? A driver state machine
> must be used to filter it out, or of course midlayer should be fixed.
>
> Boaz
>