Re: [PATCH] scsi_error fix

Re: [PATCH] scsi_error fix

[Andries.Brouwer]

    From: Patrick Mansfield <patmans@us.ibm.com>

    > [Further discussion and things I did not yet investigate:
    > What was changed to make this fail first in 2.5.63?
    > Experience shows that we get into a loop when something else
    > than SUCCESS is returned here. Probably that is a bug elsewhere.
    > Probably the commands that cause problems should never have been
    > sent in the first place.]

    The scsi error handler is also used to retrieve sense data for
    adapters/drivers that do not auto retrieve it. In such cases, it should
    not issue any aborts, resets etc.

Indeed.

    Your change effectively disables that support - we never hit the code in
    scsi_eh_get_sense() to request sense. It would be very nice if we could
    fix (or audit) all the scsi drivers, apply your change and remove
    scsi_eh_get_sense, but AFAIK that has not and is not happening.

No. What happened before was that we got into an infinite loop.
The right action is to read the code, understand why it gets
into a loop, and fix it. Once that has happened we may decide
to undo my change. Or we may decide to ask for sense at that very spot.

Today both James and Mike say that they can reproduce the loop,
so probably they'll fix that part. If not, I'll have a look again.

Andries

Re: [PATCH] scsi_error fix

[Mike Anderson]

Andries.Brouwer@cwi.nl [Andries.Brouwer@cwi.nl] wrote:
>     From: Patrick Mansfield <patmans@us.ibm.com>
> 
>     > [Further discussion and things I did not yet investigate:
>     > What was changed to make this fail first in 2.5.63?
>     > Experience shows that we get into a loop when something else
>     > than SUCCESS is returned here. Probably that is a bug elsewhere.
>     > Probably the commands that cause problems should never have been
>     > sent in the first place.]
> 
>     The scsi error handler is also used to retrieve sense data for
>     adapters/drivers that do not auto retrieve it. In such cases, it should
>     not issue any aborts, resets etc.
> 
> Indeed.
> 
>     Your change effectively disables that support - we never hit the code in
>     scsi_eh_get_sense() to request sense. It would be very nice if we could
>     fix (or audit) all the scsi drivers, apply your change and remove
>     scsi_eh_get_sense, but AFAIK that has not and is not happening.
> 
> No. What happened before was that we got into an infinite loop.
> The right action is to read the code, understand why it gets
> into a loop, and fix it. Once that has happened we may decide
> to undo my change. Or we may decide to ask for sense at that very spot.
> 
> Today both James and Mike say that they can reproduce the loop,
> so probably they'll fix that part. If not, I'll have a look again.

Sorry about that Patrick. I had sent some mail last might and thought it
went to the list.

Both James and I can reproduce this problem. I was able to reproduce
using a hack to scsi_debug. 

The loop problem is related to scsi error handling using the common code
of scsi_queue_insert / scsi_requesT_fn . When a command gets started the
scsi_init_cmd_errh function is called which sets retries to 0.

There maybe another issue with the scsi_eh_get_sense function, but I am
still looking at it.

I believe James is still pursuing a solution also.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] scsi_error fix

[James Bottomley]

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

On Fri, 2003-03-07 at 15:17, Mike Anderson wrote:
> I believe James is still pursuing a solution also.

The attached is the fix that works for me (plus fixing a few other
problems I noticed along the way).

This is against vanilla 2.5.64 (you have to take Andries work around out
to see the fix in the BK version).

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 3191 bytes --]

===== drivers/scsi/scsi_error.c 1.38 vs edited =====
--- 1.38/drivers/scsi/scsi_error.c	Sat Feb 22 10:17:01 2003
+++ edited/drivers/scsi/scsi_error.c	Fri Mar  7 15:39:51 2003
@@ -515,7 +515,7 @@
 	 * actually did complete normally.
 	 */
 	if (rtn == SUCCESS) {
-		int rtn = scsi_eh_completed_normally(scmd);
+		rtn = scsi_eh_completed_normally(scmd);
 		SCSI_LOG_ERROR_RECOVERY(3,
 			printk("%s: scsi_eh_completed_normally %x\n",
 			       __FUNCTION__, rtn));
@@ -545,20 +545,20 @@
 static int scsi_request_sense(struct scsi_cmnd *scmd)
 {
 	static unsigned char generic_sense[6] =
-	{REQUEST_SENSE, 0, 0, 0, 255, 0};
-	unsigned char scsi_result0[256], *scsi_result = &scsi_result0[0];
+	{REQUEST_SENSE, 0, 0, 0, 254, 0};
+	unsigned char *scsi_result;
 	int saved_result;
 	int rtn;
 
 	memcpy(scmd->cmnd, generic_sense, sizeof(generic_sense));
 
-	if (scmd->device->host->hostt->unchecked_isa_dma) {
-		scsi_result = kmalloc(512, GFP_ATOMIC | __GFP_DMA);
-		if (unlikely(!scsi_result)) {
-			printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
-					__FUNCTION__);
-			return FAILED;
-		}
+	scsi_result = kmalloc(254, GFP_ATOMIC | (scmd->device->host->hostt->unchecked_isa_dma) ? __GFP_DMA : 0);
+
+
+	if (unlikely(!scsi_result)) {
+		printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
+		       __FUNCTION__);
+		return FAILED;
 	}
 
 	/*
@@ -568,11 +568,11 @@
 	 * address (db).  0 is not a valid sense code. 
 	 */
 	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
-	memset(scsi_result, 0, 256);
+	memset(scsi_result, 0, 254);
 
 	saved_result = scmd->result;
 	scmd->request_buffer = scsi_result;
-	scmd->request_bufflen = 256;
+	scmd->request_bufflen = 254;
 	scmd->use_sg = 0;
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	scmd->sc_data_direction = SCSI_DATA_READ;
@@ -581,13 +581,12 @@
 	rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
 
 	/* last chance to have valid sense data */
-	if (!SCSI_SENSE_VALID(scmd)) {
+	if(!SCSI_SENSE_VALID(scmd)) {
 		memcpy(scmd->sense_buffer, scmd->request_buffer,
-				sizeof(scmd->sense_buffer));
+		       sizeof(scmd->sense_buffer));
 	}
 
-	if (scsi_result != &scsi_result0[0])
-		kfree(scsi_result);
+	kfree(scsi_result);
 
 	/*
 	 * when we eventually call scsi_finish, we really wish to complete
@@ -702,8 +701,14 @@
 		 * if the result was normal, then just pass it along to the
 		 * upper level.
 		 */
-		if (rtn == SUCCESS)
+		if (rtn == SUCCESS) {
+			/* we don't want this command reissued, just
+			 * finished with the sense data, so set
+			 * retries to the max allowed to ensure it
+			 * won't get reissued */
+			scmd->retries = scmd->allowed;
 			scsi_eh_finish_cmd(scmd, done_q);
+		}
 		if (rtn != NEEDS_RETRY)
 			continue;
 
===== drivers/scsi/scsi_lib.c 1.73 vs edited =====
--- 1.73/drivers/scsi/scsi_lib.c	Sat Feb 22 14:35:33 2003
+++ edited/drivers/scsi/scsi_lib.c	Fri Mar  7 15:19:46 2003
@@ -265,7 +265,6 @@
 	cmd->serial_number = 0;
 	cmd->serial_number_at_timeout = 0;
 	cmd->flags = 0;
-	cmd->retries = 0;
 	cmd->abort_reason = 0;
 
 	memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);

Re: [PATCH] scsi_error fix

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:

> @@ -702,8 +701,14 @@
>  		 * if the result was normal, then just pass it along to the
>  		 * upper level.
>  		 */
> -		if (rtn == SUCCESS)
> +		if (rtn == SUCCESS) {
> +			/* we don't want this command reissued, just
> +			 * finished with the sense data, so set
> +			 * retries to the max allowed to ensure it
> +			 * won't get reissued */
> +			scmd->retries = scmd->allowed;
>  			scsi_eh_finish_cmd(scmd, done_q);
> +		}
>  		if (rtn != NEEDS_RETRY)
>  			continue;
>  

We might need to cover the case down below if we succeed on retry without
reaching allowed.

Another option is to look into re-sending the command from the
scsi_queue_insert handler like we do with timeouts. The interface to the
device should be the same from the LLDD's point of view just delayed
some.


-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] scsi_error fix

[James Bottomley]

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

On Fri, 2003-03-07 at 16:43, Mike Anderson wrote:
> James Bottomley [James.Bottomley@steeleye.com] wrote:
> 
> > @@ -702,8 +701,14 @@
> >  		 * if the result was normal, then just pass it along to the
> >  		 * upper level.
> >  		 */
> > -		if (rtn == SUCCESS)
> > +		if (rtn == SUCCESS) {
> > +			/* we don't want this command reissued, just
> > +			 * finished with the sense data, so set
> > +			 * retries to the max allowed to ensure it
> > +			 * won't get reissued */
> > +			scmd->retries = scmd->allowed;
> >  			scsi_eh_finish_cmd(scmd, done_q);
> > +		}
> >  		if (rtn != NEEDS_RETRY)
> >  			continue;
> >  
> 
> We might need to cover the case down below if we succeed on retry without
> reaching allowed.
> 
> Another option is to look into re-sending the command from the
> scsi_queue_insert handler like we do with timeouts. The interface to the
> device should be the same from the LLDD's point of view just delayed
> some.

The attached should sweep up all of this.  For NEEDS_RETRY, we will
retry until we reach the max retry count.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 3600 bytes --]

===== drivers/scsi/scsi_error.c 1.38 vs edited =====
--- 1.38/drivers/scsi/scsi_error.c	Sat Feb 22 10:17:01 2003
+++ edited/drivers/scsi/scsi_error.c	Fri Mar  7 16:15:02 2003
@@ -515,7 +515,7 @@
 	 * actually did complete normally.
 	 */
 	if (rtn == SUCCESS) {
-		int rtn = scsi_eh_completed_normally(scmd);
+		rtn = scsi_eh_completed_normally(scmd);
 		SCSI_LOG_ERROR_RECOVERY(3,
 			printk("%s: scsi_eh_completed_normally %x\n",
 			       __FUNCTION__, rtn));
@@ -545,20 +545,20 @@
 static int scsi_request_sense(struct scsi_cmnd *scmd)
 {
 	static unsigned char generic_sense[6] =
-	{REQUEST_SENSE, 0, 0, 0, 255, 0};
-	unsigned char scsi_result0[256], *scsi_result = &scsi_result0[0];
+	{REQUEST_SENSE, 0, 0, 0, 254, 0};
+	unsigned char *scsi_result;
 	int saved_result;
 	int rtn;
 
 	memcpy(scmd->cmnd, generic_sense, sizeof(generic_sense));
 
-	if (scmd->device->host->hostt->unchecked_isa_dma) {
-		scsi_result = kmalloc(512, GFP_ATOMIC | __GFP_DMA);
-		if (unlikely(!scsi_result)) {
-			printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
-					__FUNCTION__);
-			return FAILED;
-		}
+	scsi_result = kmalloc(254, GFP_ATOMIC | (scmd->device->host->hostt->unchecked_isa_dma) ? __GFP_DMA : 0);
+
+
+	if (unlikely(!scsi_result)) {
+		printk(KERN_ERR "%s: cannot allocate scsi_result.\n",
+		       __FUNCTION__);
+		return FAILED;
 	}
 
 	/*
@@ -568,11 +568,11 @@
 	 * address (db).  0 is not a valid sense code. 
 	 */
 	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
-	memset(scsi_result, 0, 256);
+	memset(scsi_result, 0, 254);
 
 	saved_result = scmd->result;
 	scmd->request_buffer = scsi_result;
-	scmd->request_bufflen = 256;
+	scmd->request_bufflen = 254;
 	scmd->use_sg = 0;
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	scmd->sc_data_direction = SCSI_DATA_READ;
@@ -581,13 +581,12 @@
 	rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
 
 	/* last chance to have valid sense data */
-	if (!SCSI_SENSE_VALID(scmd)) {
+	if(!SCSI_SENSE_VALID(scmd)) {
 		memcpy(scmd->sense_buffer, scmd->request_buffer,
-				sizeof(scmd->sense_buffer));
+		       sizeof(scmd->sense_buffer));
 	}
 
-	if (scsi_result != &scsi_result0[0])
-		kfree(scsi_result);
+	kfree(scsi_result);
 
 	/*
 	 * when we eventually call scsi_finish, we really wish to complete
@@ -703,25 +702,14 @@
 		 * upper level.
 		 */
 		if (rtn == SUCCESS)
-			scsi_eh_finish_cmd(scmd, done_q);
-		if (rtn != NEEDS_RETRY)
+			/* we don't want this command reissued, just
+			 * finished with the sense data, so set
+			 * retries to the max allowed to ensure it
+			 * won't get reissued */
+			scmd->retries = scmd->allowed;
+		else if (rtn != NEEDS_RETRY)
 			continue;
 
-		/*
-		 * we only come in here if we want to retry a
-		 * command.  the test to see whether the command
-		 * should be retried should be keeping track of the
-		 * number of tries, so we don't end up looping, of
-		 * course.
-		 */
-		scmd->state = NEEDS_RETRY;
-		rtn = scsi_eh_retry_cmd(scmd);
-		if (rtn != SUCCESS)
-			continue;
-
-		/*
-		 * we eventually hand this one back to the top level.
-		 */
 		scsi_eh_finish_cmd(scmd, done_q);
 	}
 
===== drivers/scsi/scsi_lib.c 1.73 vs edited =====
--- 1.73/drivers/scsi/scsi_lib.c	Sat Feb 22 14:35:33 2003
+++ edited/drivers/scsi/scsi_lib.c	Fri Mar  7 15:19:46 2003
@@ -265,7 +265,6 @@
 	cmd->serial_number = 0;
 	cmd->serial_number_at_timeout = 0;
 	cmd->flags = 0;
-	cmd->retries = 0;
 	cmd->abort_reason = 0;
 
 	memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);

Re: [PATCH] scsi_error fix

[Osamu Tomita]

James Bottomley wrote:
> This is against vanilla 2.5.64 (you have to take Andries work around out
> to see the fix in the BK version).

Thanks!
My old PC98 box with wd33c93 and SCSI-1 HD/CD re-works fine
after apllied you patch.
With cset-1.1099 patch, kernel can boot but mis detect SCSI-1
CD drive.

Regards,
Osamu Tomita

Re: [PATCH] scsi_error fix

[Luben Tuikov]

Mike Anderson wrote:
> Andries.Brouwer@cwi.nl [Andries.Brouwer@cwi.nl] wrote:
> 
>>
>>    Your change effectively disables that support - we never hit the code in
>>    scsi_eh_get_sense() to request sense. It would be very nice if we could
>>    fix (or audit) all the scsi drivers, apply your change and remove
>>    scsi_eh_get_sense, but AFAIK that has not and is not happening.
>>
>>No. What happened before was that we got into an infinite loop.
>>The right action is to read the code, understand why it gets
>>into a loop, and fix it. Once that has happened we may decide
>>to undo my change. Or we may decide to ask for sense at that very spot.

``at that very spot'' or at a lower level (but not LLDD).

Yes, I agree with Andries here, and my reasoning follows:

> The loop problem is related to scsi error handling using the common code
> of scsi_queue_insert / scsi_requesT_fn . When a command gets started the
> scsi_init_cmd_errh function is called which sets retries to 0.

Ultimately the solution is fixing the LLDD.  Drawing from my own experience:
a quick and dirty way of simulating Autosense is to do it
*right in the scsi_done() fn* .

Add a flag sense_reqd:1 :: struct scsi_cmnd (stands for ``sense requested''),
which is normally 0, and at scsi_done() if we got CHECK CONDITION without
Sense Data, we (for older 2.4 kernels, NOT REAL C CODE):

    if we got CHECK COND and NO SENSE and SCpnt->sense_reqd == 0 then
	/* the original command is in the backup fields already */
	memcpy(SCpnt->cmnd, generic_sense, sizeof(generic_sense));
	memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
	SCpnt->sc_data_direction = SCSI_DATA_READ;
	SCpnt->request_buffer = &SCpnt->sense_buffer;
	SCpnt->request_bufflen = SCSI_SENSE_BUFFERSIZE;
	SCpnt->use_sg = 0;
	SCpnt->cmd_len = COMMAND_SIZE(SCpnt->cmnd[0]);
	SCpnt->result = 0;
	SCpnt->sense_reqd = 1;
	
	enqueue the command to be sent back to the LLDD.
  	(i.e. from pending_q to incoming_q at front -- the only instance of
	backwardness which I spoke of in an older email...)
   else
	/* normal process */
	enqueue the command into the done_q;
	schedule scsi soft irq;
   fi

The only problem with this solution is that if the driver does it's own
queuing and NACA = 0 (normally) in the CONTROL BYTE of the CDB, then
the Sense Data gets wiped out* at the recept of the next command by the Device
Server (unless it's REQUEST SENSE).  Thus, the only true solution is to fix
it at LLDD level, but with the above solution we get as close to it,
but no closer. :-)

* CA (Contingent Allegiance), now obsolete as of SAM-3r05, since Autosense
is mandatory.

> There maybe another issue with the scsi_eh_get_sense function, but I am
> still looking at it.

I have a problem with fixing Autosense at such higher level in SCSI Core.
I.e. having its own function and going through the normal path of SCSI Commands.
It should be a hack, since this is what Non-Autosense is.

I.e. SCSI Core itself should *not* know about Autosense and Non-Autosense
complient drivers.  Thus, this is best fixed at scsi_done() fn.

I.e. the workings of SCSI Core should assume Autosense -- first because
it makes sense, and secondly because it's mandatory as of SAM-3r05.

And if the Sense Data got back nil, i.e. it got wiped out, then we just bubble up
the command (since sense_reqd would == 1).

At this (low) level there's no need to count retries.  At higher level in
SCSI Core, it's ok, but for Autosense we just have to be as quiet as possible.

Note: there are devices which will report CHECK CONDITION but will
never give a valid Sense Data -- those are the ones which could
really break a shaky recovery logic (normally infinite loop).

I've also gotten into an infinite loop in simulating Autosense in my own
mini-scsi-core, but this was resolved by following more or less the
aforementioned framework.

-- 
Luben

[PATCH] scsi_error fix

[Andries.Brouwer]

Below:
imm.c: spelling
scsi.h: remove old and now incorrect comment
scsi_scan.c: remove superfluous final return
scsi_error.c: apart from similar trivialities the only change:

    If a command fails (e.g. because it belongs to a newer
    SCSI version than the device), it is fed to
    scsi_decide_disposition(). That routine must return
    SUCCESS, unless the error handler should be invoked.

    In the situation where host_byte is DID_OK, and message_byte
    is COMMAND_COMPLETE, and status is CHECK_CONDITION, there is
    no reason at all to invoke aborts and resets. The situation
    is normal. I see here UNIT ATTENTION, Power on occurred
    and ILLEGAL REQUEST, Invalid field in cdb.

    The 2.5.64 code does not return SUCCESS, but it returns the
    return code of scsi_check_sense(), and that may be FAILED
    in case we do not have valid sense.

    Further discussion is possible here, but I changed the return
    into "return SUCCESS" and 2.5.64 boots fine.

Andries

[Further discussion and things I did not yet investigate:
What was changed to make this fail first in 2.5.63?
Experience shows that we get into a loop when something else
than SUCCESS is returned here. Probably that is a bug elsewhere.
Probably the commands that cause problems should never have been
sent in the first place.]

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/imm.c b/drivers/scsi/imm.c
--- a/drivers/scsi/imm.c	Sat Feb 15 20:41:59 2003
+++ b/drivers/scsi/imm.c	Thu Mar  6 23:50:31 2003
@@ -174,6 +174,7 @@
 	    parport_unregister_device(imm_hosts[i].dev);
 	    continue;
 	}
+
 	/* now the glue ... */
 	switch (imm_hosts[i].mode) {
 	case IMM_NIBBLE:
@@ -218,7 +219,7 @@
 
 /* This is to give the imm driver a way to modify the timings (and other
  * parameters) by writing to the /proc/scsi/imm/0 file.
- * Very simple method really... (To simple, no error checking :( )
+ * Very simple method really... (Too simple, no error checking :( )
  * Reason: Kernel hackers HATE having to unload and reload modules for
  * testing...
  * Also gives a method to use a script to obtain optimum timings (TODO)
@@ -948,7 +949,7 @@
     unsigned char l = 0, h = 0;
     int retv, x;
 
-    /* First check for any errors that may of occurred
+    /* First check for any errors that may have occurred
      * Here we check for internal errors
      */
     if (tmp->failed)
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/scsi.h b/drivers/scsi/scsi.h
--- a/drivers/scsi/scsi.h	Wed Mar  5 10:47:26 2003
+++ b/drivers/scsi/scsi.h	Thu Mar  6 23:50:31 2003
@@ -280,26 +280,7 @@
 #define SCSI_SET_IOCTL_LOGGING(LEVEL)  \
         SCSI_SET_LOGGING(SCSI_LOG_IOCTL_SHIFT, SCSI_LOG_IOCTL_BITS, LEVEL);
 
-/*
- *  the return of the status word will be in the following format :
- *  The low byte is the status returned by the SCSI command, 
- *  with vendor specific bits masked.
- *  
- *  The next byte is the message which followed the SCSI status.
- *  This allows a stos to be used, since the Intel is a little
- *  endian machine.
- *  
- *  The final byte is a host return code, which is one of the following.
- *  
- *  IE 
- *  lsb     msb
- *  status  msg host code   
- *  
- *  Our errors returned by OUR driver, NOT SCSI message.  Or'd with
- *  SCSI message passed back to driver <IF any>.
- */
-
-
+/* host byte codes */
 #define DID_OK          0x00	/* NO error                                */
 #define DID_NO_CONNECT  0x01	/* Couldn't connect before timeout period  */
 #define DID_BUS_BUSY    0x02	/* BUS stayed busy through time out period */
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
--- a/drivers/scsi/scsi_error.c	Mon Feb 24 23:02:52 2003
+++ b/drivers/scsi/scsi_error.c	Thu Mar  6 23:50:31 2003
@@ -92,10 +92,8 @@
  *
  * Notes:
  *    This should be turned into an inline function.  Each scsi command
- *    has it's own timer, and as it is added to the queue, we set up the
- *    timer.  When the command completes, we cancel the timer.  Pretty
- *    simple, really, especially compared to the old way of handling this
- *    crap.
+ *    has its own timer, and as it is added to the queue, we set up the
+ *    timer.  When the command completes, we cancel the timer.
  **/
 void scsi_add_timer(struct scsi_cmnd *scmd, int timeout,
 		    void (*complete)(struct scsi_cmnd *))
@@ -249,6 +247,7 @@
 {
 	if (!SCSI_SENSE_VALID(scmd))
 		return FAILED;
+
 	if (scmd->sense_buffer[2] & 0xe0)
 		return SUCCESS;
 
@@ -1193,12 +1192,12 @@
  * Notes:
  *    This is *only* called when we are examining the status after sending
  *    out the actual data command.  any commands that are queued for error
- *    recovery (i.e. test_unit_ready) do *not* come through here.
+ *    recovery (e.g. test_unit_ready) do *not* come through here.
  *
  *    When this routine returns failed, it means the error handler thread
- *    is woken.  in cases where the error code indicates an error that
+ *    is woken.  In cases where the error code indicates an error that
  *    doesn't require the error handler read (i.e. we don't need to
- *    abort/reset), then this function should return SUCCESS.
+ *    abort/reset), this function should return SUCCESS.
  **/
 int scsi_decide_disposition(struct scsi_cmnd *scmd)
 {
@@ -1214,11 +1213,11 @@
 						  __FUNCTION__));
 		return SUCCESS;
 	}
+
 	/*
 	 * first check the host byte, to see if there is anything in there
 	 * that would indicate what we need to do.
 	 */
-
 	switch (host_byte(scmd->result)) {
 	case DID_PASSTHROUGH:
 		/*
@@ -1296,11 +1295,11 @@
 	/*
 	 * next, check the message byte.
 	 */
-	if (msg_byte(scmd->result) != COMMAND_COMPLETE) {
+	if (msg_byte(scmd->result) != COMMAND_COMPLETE)
 		return FAILED;
-	}
+
 	/*
-	 * now, check the status byte to see if this indicates anything special.
+	 * check the status byte to see if this indicates anything special.
 	 */
 	switch (status_byte(scmd->result)) {
 	case QUEUE_FULL:
@@ -1321,10 +1320,11 @@
 		return SUCCESS;
 	case CHECK_CONDITION:
 		rtn = scsi_check_sense(scmd);
-		if (rtn == NEEDS_RETRY) {
+		if (rtn == NEEDS_RETRY)
 			goto maybe_retry;
-		}
-		return rtn;
+		/* if rtn == FAILED, we have no sense information */
+		/* was: return rtn; */
+		return SUCCESS;
 	case CONDITION_GOOD:
 	case INTERMEDIATE_GOOD:
 	case INTERMEDIATE_C_GOOD:
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Wed Mar  5 10:47:26 2003
+++ b/drivers/scsi/scsi_scan.c	Fri Mar  7 00:02:09 2003
@@ -960,7 +960,6 @@
 	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: host %d channel %d"
 	    " id %d lun %d name/id: '%s'\n", sdev->host->host_no,
 	    sdev->channel, sdev->id, sdev->lun, sdev->sdev_driverfs_dev.name));
-	return;
 }
 
 /**

Re: [PATCH] scsi_error fix

[Rob Radez]

On Fri, Mar 07, 2003 at 12:37:57AM +0100, Andries.Brouwer@cwi.nl wrote:
> Below:
> scsi_error.c: apart from similar trivialities the only change:
> 
>     If a command fails (e.g. because it belongs to a newer
>     SCSI version than the device), it is fed to
>     scsi_decide_disposition(). That routine must return
>     SUCCESS, unless the error handler should be invoked.
[snip patch]

This patch gets me booting again.

Regards,
Rob Radez

Re: [PATCH] scsi_error fix

[Patrick Mansfield]

On Fri, Mar 07, 2003 at 12:37:57AM +0100, Andries.Brouwer@cwi.nl wrote:

> scsi_error.c: apart from similar trivialities the only change:
> 
>     If a command fails (e.g. because it belongs to a newer
>     SCSI version than the device), it is fed to
>     scsi_decide_disposition(). That routine must return
>     SUCCESS, unless the error handler should be invoked.
> 
>     In the situation where host_byte is DID_OK, and message_byte
>     is COMMAND_COMPLETE, and status is CHECK_CONDITION, there is
>     no reason at all to invoke aborts and resets. The situation
>     is normal. I see here UNIT ATTENTION, Power on occurred
>     and ILLEGAL REQUEST, Invalid field in cdb.
> 
>     The 2.5.64 code does not return SUCCESS, but it returns the
>     return code of scsi_check_sense(), and that may be FAILED
>     in case we do not have valid sense.
> 
>     Further discussion is possible here, but I changed the return
>     into "return SUCCESS" and 2.5.64 boots fine.
> 
> Andries
> 
> [Further discussion and things I did not yet investigate:
> What was changed to make this fail first in 2.5.63?
> Experience shows that we get into a loop when something else
> than SUCCESS is returned here. Probably that is a bug elsewhere.
> Probably the commands that cause problems should never have been
> sent in the first place.]

The scsi error handler is also used to retrieve sense data for
adapters/drivers that do not auto retrieve it. In such cases, it should
not issue any aborts, resets etc.

Your change effectively disables that support - we never hit the code in
scsi_eh_get_sense() to request sense. It would be very nice if we could
fix (or audit) all the scsi drivers, apply your change and remove
scsi_eh_get_sense, but AFAIK that has not and is not happening.

I don't know if your adapter/driver has auto sense retrieval (imm.c?).

I would guess a change in scsi_error.c related to sense (scsi_eh_get_sense)
handling is causing problems, and if your driver has auto sense then
something is wrong with the scmd->sense_buffer.

It would still be useful to see the console log output with scsi_logging=1
for your failure case.

-- Patrick Mansfield