sd.c spinup code can go into a wild loop on unexpected device returns

[James Bottomley <James.Bottomley@steeleye.com>]

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

This problem was reported against 2.4 by Eddie.Williams@SteelEye.com

There's a problem in the sd spinup code in that if the unit returns NOT
READY, we begin to spin it up, but thereafter if it returns anything
other than NOT READY or success, the while loop in the spinup code will
be executed *without* the 1s delay that's in the NOT READY case.

The problem was seen with a real device: Compaq multi-path storage
arrays return NOT READY to probes down inactive paths, but when the
start unit is sent to activate the path, they can then respond back with
error conditions.

The fix is to terminate the while loop for any unexpected return.

James


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

===== drivers/scsi/sd.c 1.111 vs edited =====
--- 1.111/drivers/scsi/sd.c	Wed May  7 10:15:39 2003
+++ edited/drivers/scsi/sd.c	Thu May  8 10:16:20 2003
@@ -811,7 +811,8 @@
 	       struct scsi_request *SRpnt, unsigned char *buffer) {
 	unsigned char cmd[10];
 	unsigned long spintime_value = 0;
-	int the_result, retries, spintime;
+	int retries, spintime;
+	unsigned int the_result;
 
 	spintime = 0;
 
@@ -820,7 +821,7 @@
 	do {
 		retries = 0;
 
-		while (retries < 3) {
+		do {
 			cmd[0] = TEST_UNIT_READY;
 			memset((void *) &cmd[1], 0, 9);
 
@@ -834,10 +835,9 @@
 
 			the_result = SRpnt->sr_result;
 			retries++;
-			if (the_result == 0
-			    || SRpnt->sr_sense_buffer[2] != UNIT_ATTENTION)
-				break;
-		}
+		} while (retries < 3 && !scsi_status_is_good(the_result)
+			 && ((driver_byte(the_result) & DRIVER_SENSE)
+			     && SRpnt->sr_sense_buffer[2] == UNIT_ATTENTION));
 
 		/*
 		 * If the drive has indicated to us that it doesn't have
@@ -847,8 +847,15 @@
 		if (media_not_present(sdkp, SRpnt))
 			return;
 
-		if (the_result == 0)
-			break;		/* all is well now */
+		if ((driver_byte(the_result) & DRIVER_SENSE) == 0) {
+			/* no sense, TUR either succeeded or failed
+			 * with a status error */
+			if(!spintime && !scsi_status_is_good(the_result))
+				printk(KERN_NOTICE "%s: Unit Not Ready, error = 0x%x\n", diskname, the_result);
+			break;
+		}
+					
+					
 
 		/*
 		 * If manual intervention is required, or this is an
@@ -856,13 +863,13 @@
 		 */
 		if (SRpnt->sr_sense_buffer[2] == NOT_READY &&
 		    SRpnt->sr_sense_buffer[12] == 4 /* not ready */ &&
-		    SRpnt->sr_sense_buffer[13] == 3)
+		    SRpnt->sr_sense_buffer[13] == 3) {
 			break;		/* manual intervention required */
 
 		/*
 		 * Issue command to spin up drive when not ready
 		 */
-		if (SRpnt->sr_sense_buffer[2] == NOT_READY) {
+		} else if (SRpnt->sr_sense_buffer[2] == NOT_READY) {
 			unsigned long time1;
 			if (!spintime) {
 				printk(KERN_NOTICE "%s: Spinning up disk...",
@@ -889,15 +896,24 @@
 				time1 = schedule_timeout(time1);
 			} while(time1);
 			printk(".");
+		} else {
+			/* we don't understand the sense code, so it's
+			 * probably pointless to loop */
+			if(!spintime) {
+				printk(KERN_NOTICE "%s: Unit Not Ready, sense:\n", diskname);
+				print_req_sense("", SRpnt);
+			}
+			break;
 		}
+				
 	} while (spintime &&
 		 time_after(spintime_value + 100 * HZ, jiffies));
 
 	if (spintime) {
-		if (the_result)
-			printk("not responding...\n");
-		else
+		if (scsi_status_is_good(the_result))
 			printk("ready\n");
+		else
+			printk("not responding...\n");
 	}
 }