Support READ CAPACITY 16 on more drives

PATCH In review Matthew Wilcox 5 participants 14 messages Mar 12, 2009 – Mar 15, 2009 SCSI

Share & tools

Matthew Wilcox

author

2009-03-12 18:30 UTC #0 lore raw

We need to use READ CAPACITY 16 to determine features like 4k physical sector size, and support for UNMAP. This pair of patches let us try it on really recent SCSI drives and all LibATA drives (where it's emulated by libata so we know they support it). It will not be tried on USB devices, as the USB layer overrides their claims of support for recent SCSI versions.

Matthew Wilcox

author

2009-03-12 18:30 UTC #1 lore raw

PATCH 1/2 sd: Refactor sd_read_capacity()

From: Matthew Wilcox <[email protected]>

The sd_read_capacity() function was about 180 lines long and included a backwards goto and a tricky state variable. Splitting out read_capacity_10() and read_capacity_16() (about 50 lines each) reduces sd_read_capacity to about 100 lines and gets rid of the backwards goto and the state variable. I've tried to avoid any behaviour change with this patch.

Signed-off-by Matthew Wilcox <[email protected]>

---
 drivers/scsi/sd.c |  247 ++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 158 insertions(+), 89 deletions(-)

drivers/scsi/sd.c +155 ~~-86~~

index 55310db..6cf0c25 100644
@@ -1277,42 +1277,61 @@ disable:
 	sdkp->capacity = 0;
 }
 
-/*
- * read disk capacity
- */
-static void
-sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
+static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
+			struct scsi_sense_hdr *sshdr, int sense_valid,
+			int the_result)
+{
+	sd_print_result(sdkp, the_result);
+	if (driver_byte(the_result) & DRIVER_SENSE)
+		sd_print_sense_hdr(sdkp, sshdr);
+	else
+		sd_printk(KERN_NOTICE, sdkp, "Sense not available.\n");
+
+	/*
+	 * Set dirty bit for removable devices if not ready -
+	 * sometimes drives will not report this properly.
+	 */
+	if (sdp->removable &&
+	    sense_valid && sshdr->sense_key == NOT_READY)
+		sdp->changed = 1;
+
+	/*
+	 * We used to set media_present to 0 here to indicate no media
+	 * in the drive, but some drives fail read capacity even with
+	 * media present, so we can't do that.
+	 */
+	sdkp->capacity = 0; /* unknown mapped to zero - as usual */
+}
+
+#define RC16_LEN 13
+#if RC16_LEN > SD_BUF_SIZE
+#error RC16_LEN must not be more than SD_BUF_SIZE
+#endif
+
+static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
+						unsigned char *buffer)
 {
 	unsigned char cmd[16];
-	int the_result, retries;
-	int sector_size = 0;
-	/* Force READ CAPACITY(16) when PROTECT=1 */
-	int longrc = scsi_device_protection(sdkp->device) ? 1 : 0;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
-	struct scsi_device *sdp = sdkp->device;
+	int the_result;
+	int retries = 3;
+	unsigned long long lba;
+	unsigned sector_size;
 
-repeat:
-	retries = 3;
 	do {
-		if (longrc) {
-			memset((void *) cmd, 0, 16);
-			cmd[0] = SERVICE_ACTION_IN;
-			cmd[1] = SAI_READ_CAPACITY_16;
-			cmd[13] = 13;
-			memset((void *) buffer, 0, 13);
-		} else {
-			cmd[0] = READ_CAPACITY;
-			memset((void *) &cmd[1], 0, 9);
-			memset((void *) buffer, 0, 8);
-		}
-		
+		memset(cmd, 0, 16);
+		cmd[0] = SERVICE_ACTION_IN;
+		cmd[1] = SAI_READ_CAPACITY_16;
+		cmd[13] = RC16_LEN;
+		memset(buffer, 0, RC16_LEN);
+
 		the_result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
-					      buffer, longrc ? 13 : 8, &sshdr,
-					      SD_TIMEOUT, SD_MAX_RETRIES, NULL);
+					buffer, RC16_LEN, &sshdr,
+					SD_TIMEOUT, SD_MAX_RETRIES, NULL);
 
 		if (media_not_present(sdkp, &sshdr))
-			return;
+			return -ENODEV;
 
 		if (the_result)
 			sense_valid = scsi_sense_valid(&sshdr);

@@ -1320,72 +1339,122 @@ repeat:
 
 	} while (the_result && retries);
 
-	if (the_result && !longrc) {
+	if (the_result) {
+		sd_printk(KERN_NOTICE, sdkp, "READ CAPACITY(16) failed\n");
+		read_capacity_error(sdkp, sdp, &sshdr, sense_valid, the_result);
+		return -EINVAL;
+	}
+
+	sector_size =	(buffer[8] << 24) | (buffer[9] << 16) |
+			(buffer[10] << 8) | buffer[11];
+	lba =  (((u64)buffer[0] << 56) | ((u64)buffer[1] << 48) |
+		((u64)buffer[2] << 40) | ((u64)buffer[3] << 32) |
+		((u64)buffer[4] << 24) | ((u64)buffer[5] << 16) |
+		((u64)buffer[6] << 8) | (u64)buffer[7]);
+
+	sd_read_protection_type(sdkp, buffer);
+
+	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
+		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
+			"kernel compiled with support for large block "
+			"devices.\n");
+		sdkp->capacity = 0;
+		return -EOVERFLOW;
+	}
+
+	sdkp->capacity = lba + 1;
+	return sector_size;
+}
+
+static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
+						unsigned char *buffer)
+{
+	unsigned char cmd[16];
+	struct scsi_sense_hdr sshdr;
+	int sense_valid = 0;
+	int the_result;
+	int retries = 3;
+	sector_t lba;
+	unsigned sector_size;
+
+	do {
+		cmd[0] = READ_CAPACITY;
+		memset(&cmd[1], 0, 9);
+		memset(buffer, 0, 8);
+
+		the_result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
+					buffer, 8, &sshdr,
+					SD_TIMEOUT, SD_MAX_RETRIES, NULL);
+
+		if (media_not_present(sdkp, &sshdr))
+			return -ENODEV;
+
+		if (the_result)
+			sense_valid = scsi_sense_valid(&sshdr);
+		retries--;
+
+	} while (the_result && retries);
+
+	if (the_result) {
 		sd_printk(KERN_NOTICE, sdkp, "READ CAPACITY failed\n");
-		sd_print_result(sdkp, the_result);
-		if (driver_byte(the_result) & DRIVER_SENSE)
-			sd_print_sense_hdr(sdkp, &sshdr);
-		else
-			sd_printk(KERN_NOTICE, sdkp, "Sense not available.\n");
+		read_capacity_error(sdkp, sdp, &sshdr, sense_valid, the_result);
+		return -EINVAL;
+	}
 
-		/* Set dirty bit for removable devices if not ready -
-		 * sometimes drives will not report this properly. */
-		if (sdp->removable &&
-		    sense_valid && sshdr.sense_key == NOT_READY)
-			sdp->changed = 1;
+	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
+			(buffer[6] << 8) | buffer[7];
+	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
+		(buffer[2] << 8) | buffer[3];
 
-		/* Either no media are present but the drive didn't tell us,
-		   or they are present but the read capacity command fails */
-		/* sdkp->media_present = 0; -- not always correct */
-		sdkp->capacity = 0; /* unknown mapped to zero - as usual */
+	if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {
+		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
+			"kernel compiled with support for large block "
+			"devices.\n");
+		sdkp->capacity = 0;
+		return -EOVERFLOW;
+	}
 
-		return;
-	} else if (the_result && longrc) {
-		/* READ CAPACITY(16) has been failed */
-		sd_printk(KERN_NOTICE, sdkp, "READ CAPACITY(16) failed\n");
-		sd_print_result(sdkp, the_result);
-		sd_printk(KERN_NOTICE, sdkp, "Use 0xffffffff as device size\n");
+	sdkp->capacity = lba + 1;
+	return sector_size;
+}
 
-		sdkp->capacity = 1 + (sector_t) 0xffffffff;		
-		goto got_data;
-	}	
-	
-	if (!longrc) {
-		sector_size = (buffer[4] << 24) |
-			(buffer[5] << 16) | (buffer[6] << 8) | buffer[7];
-		if (buffer[0] == 0xff && buffer[1] == 0xff &&
-		    buffer[2] == 0xff && buffer[3] == 0xff) {
-			if(sizeof(sdkp->capacity) > 4) {
-				sd_printk(KERN_NOTICE, sdkp, "Very big device. "
-					  "Trying to use READ CAPACITY(16).\n");
-				longrc = 1;
-				goto repeat;
-			}
-			sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use "
-				  "a kernel compiled with support for large "
-				  "block devices.\n");
-			sdkp->capacity = 0;
+/*
+ * read disk capacity
+ */
+static void
+sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
+{
+	int sector_size;
+	struct scsi_device *sdp = sdkp->device;
+
+	/* Force READ CAPACITY(16) when PROTECT=1 */
+	if (scsi_device_protection(sdp)) {
+		sector_size = read_capacity_16(sdkp, sdp, buffer);
+		if (sector_size == -EOVERFLOW)
 			goto got_data;
-		}
-		sdkp->capacity = 1 + (((sector_t)buffer[0] << 24) |
-			(buffer[1] << 16) |
-			(buffer[2] << 8) |
-			buffer[3]);			
+		if (sector_size < 0)
+			return;
 	} else {
-		sdkp->capacity = 1 + (((u64)buffer[0] << 56) |
-			((u64)buffer[1] << 48) |
-			((u64)buffer[2] << 40) |
-			((u64)buffer[3] << 32) |
-			((sector_t)buffer[4] << 24) |
-			((sector_t)buffer[5] << 16) |
-			((sector_t)buffer[6] << 8)  |
-			(sector_t)buffer[7]);
-			
-		sector_size = (buffer[8] << 24) |
-			(buffer[9] << 16) | (buffer[10] << 8) | buffer[11];
-
-		sd_read_protection_type(sdkp, buffer);
-	}	
+		sector_size = read_capacity_10(sdkp, sdp, buffer);
+		if (sector_size == -EOVERFLOW)
+			goto got_data;
+		if (sector_size < 0)
+			return;
+		if ((sizeof(sdkp->capacity) > 4) &&
+		    (sdkp->capacity > 0xffffffffULL)) {
+			int old_sector_size = sector_size;
+			sd_printk(KERN_NOTICE, sdkp, "Very big device. "
+					"Trying to use READ CAPACITY(16).\n");
+			sector_size = read_capacity_16(sdkp, sdp, buffer);
+			if (sector_size < 0) {
+				sd_printk(KERN_NOTICE, sdkp,
+					"Using 0xffffffff as device size\n");
+				sdkp->capacity = 1 + (sector_t) 0xffffffff;
+				sector_size = old_sector_size;
+				goto got_data;
+			}
+		}
+	}
 
 	/* Some devices return the total number of sectors, not the
 	 * highest sector number.  Make the necessary adjustment. */

Signature · 1 line

1.6.1.3

Martin K. Petersen

Tested-by on v1 1/2

2009-03-12 18:36 UTC #2 lore raw

Re: [PATCH 1/2] sd: Refactor sd_read_capacity()

Matthew> Signed-off-by: Matthew Wilcox <[email protected]>

Tested-by Martin K. Petersen <[email protected]>

My recent quiesce revalidate patch applies on top of these two, btw.

James Bottomley

maintainer

2009-03-13 21:29 UTC #3 lore raw

Re: [PATCH 1/2] sd: Refactor sd_read_capacity()

In reply to Matthew Wilcox

Quoted 1 line · Matthew Wilcox

+#define RC16_LEN 13

Shouldn't this be 32, the defined length of a READ CAPACITY 16 return?

In theory asking for less is fine, since the spec allows it, but it's setting a trap for expanded users of READ_CAPACITY 16 since they might blindly use a buffer[13] or beyond, not realising we didn't actually ask for data beyond buffer[12].

James

Martin K. Petersen

2009-03-13 21:45 UTC #4 lore raw

Re: [PATCH 1/2] sd: Refactor sd_read_capacity()

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"James" == James Bottomley <[email protected]> writes:

Quoted 1 line · Matthew Wilcox

Quoted 1 line
+#define RC16_LEN 13

James> Shouldn't this be 32, the defined length of a READ CAPACITY 16 James> return?

James> In theory asking for less is fine, since the spec allows it, but James> it's setting a trap for expanded users of READ_CAPACITY 16 since James> they might blindly use a buffer[13] or beyond, not realising we James> didn't actually ask for data beyond buffer[12].

I have this bumped to 16 in my tree (after being bitten by what you describe). I'm open to upping it further even if none of the remaining 16 bytes have been defined yet.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Matthew Wilcox

2009-03-14 01:19 UTC #5 lore raw

Re: [PATCH 1/2] sd: Refactor sd_read_capacity()

In reply to James Bottomley

Quoted 9 lines · James Bottomley

Quoted 1 line · Matthew Wilcox
+#define RC16_LEN 13
Shouldn't this be 32, the defined length of a READ CAPACITY 16 return?
In theory asking for less is fine, since the spec allows it, but it's setting a trap for expanded users of READ_CAPACITY 16 since they might blindly use a buffer[13] or beyond, not realising we didn't actually ask for data beyond buffer[12].

I'm perfectly fine with expanding it to 16 or even 32. Want me to repost the patch, or will you fix it up?

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

James Bottomley

maintainer

2009-03-14 13:40 UTC #6 lore raw

Re: [PATCH 1/2] sd: Refactor sd_read_capacity()

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · James Bottomley
Quoted 1 line · Matthew Wilcox
+#define RC16_LEN 13
Shouldn't this be 32, the defined length of a READ CAPACITY 16 return?

Quoted 7 lines

Quoted 4 lines
In theory asking for less is fine, since the spec allows it, but it's setting a trap for expanded users of READ_CAPACITY 16 since they might blindly use a buffer[13] or beyond, not realising we didn't actually ask for data beyond buffer[12].
I'm perfectly fine with expanding it to 16 or even 32. Want me to repost the patch, or will you fix it up?

It's a one liner ... I can do it (crosses fingers).

James

Matthew Wilcox

author

2009-03-12 18:30 UTC #7 lore raw

PATCH 2/2 sd: Try READ CAPACITY 16 first for SBC-2 devices

In reply to Matthew Wilcox

From: Matthew Wilcox <[email protected]>

New features are being added to the READ CAPACITY 16 results, so we want to issue it in preference to READ CAPACITY 10. Unfortunately, some devices misbehave when they see a READ CAPACITY 16, so we restrict this command to devices which claim conformance to SPC-3 (aka SBC-2), or claim they have features which are only reported in the READ CAPACITY 16 data.

The READ CAPACITY 16 command is optional, even for SBC-2 devices, so we fall back to READ CAPACITY 10 if READ CAPACITY 16 fails.

Signed-off-by Matthew Wilcox <[email protected]>

Tested-by Martin K. Petersen <[email protected]>

---
 drivers/scsi/sd.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

drivers/scsi/sd.c +14 -2

index 6cf0c25..b155488 100644
@@ -1418,6 +1418,15 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	return sector_size;
 }
 
+static int sd_try_rc16_first(struct scsi_device *sdp)
+{
+	if (sdp->scsi_level > SCSI_SPC_2)
+		return 1;
+	if (scsi_device_protection(sdp))
+		return 1;
+	return 0;
+}
+
 /*
  * read disk capacity
  */

@@ -1427,11 +1436,14 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
 	int sector_size;
 	struct scsi_device *sdp = sdkp->device;
 
-	/* Force READ CAPACITY(16) when PROTECT=1 */
-	if (scsi_device_protection(sdp)) {
+	if (sd_try_rc16_first(sdp)) {
 		sector_size = read_capacity_16(sdkp, sdp, buffer);
 		if (sector_size == -EOVERFLOW)
 			goto got_data;
+		if (sector_size == -ENODEV)
+			return;
+		if (sector_size < 0)
+			sector_size = read_capacity_10(sdkp, sdp, buffer);
 		if (sector_size < 0)
 			return;
 	} else {

Signature · 1 line

1.6.1.3

James Bottomley

maintainer

2009-03-14 20:41 UTC #8 lore raw

Re: [PATCH 2/2] sd: Try READ CAPACITY 16 first for SBC-2 devices

Quoted 10 lines · Matthew Wilcox

From: Matthew Wilcox <[email protected]>
New features are being added to the READ CAPACITY 16 results, so we want to issue it in preference to READ CAPACITY 10. Unfortunately, some devices misbehave when they see a READ CAPACITY 16, so we restrict this command to devices which claim conformance to SPC-3 (aka SBC-2), or claim they have features which are only reported in the READ CAPACITY 16 data.
The READ CAPACITY 16 command is optional, even for SBC-2 devices, so we fall back to READ CAPACITY 10 if READ CAPACITY 16 fails.

We're going to have to do something about the scary error messages on SBC-2 supporting drives, this is what mine say (and this is after mkp's chat reduction):

sd 1:0:1:0: [sdc] READ CAPACITY(16) failed
sd 1:0:1:0: [sdc] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sd 1:0:1:0: [sdc] Sense Key : Illegal Request [current]
sd 1:0:1:0: [sdc] Add. Sense: Invalid command operation code
sd 1:0:1:0: [sdc] 71096640 512-byte hardware sectors: (36.4 GB/33.9 GiB) sd 1:0:1:0: [sdc] Write Protect is off
sd 1:0:1:0: [sdc] Write cache: disabled, read cache: enabled, supports DPO and FUA sd 1:0:1:0: [sdc] READ CAPACITY(16) failed
sd 1:0:1:0: [sdc] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sd 1:0:1:0: [sdc] Sense Key : Illegal Request [current]
sd 1:0:1:0: [sdc] Add. Sense: Invalid command operation code
sdc: sdc1 sdc2 sdc3
sd 1:0:1:0: [sdc] Attached SCSI disk

What they're saying is that they don't support READ CAPACITY(16) which is perfectly legal for SBC-2 conforming devices which don't support protection information ... like almost every modern disk in the field.

James

Matthew Wilcox

2009-03-14 22:49 UTC #9 lore raw

Re: [PATCH 2/2] sd: Try READ CAPACITY 16 first for SBC-2 devices

Quoted 9 lines · James Bottomley

We're going to have to do something about the scary error messages on SBC-2 supporting drives, this is what mine say (and this is after mkp's chat reduction):
sd 1:0:1:0: [sdc] READ CAPACITY(16) failed
sd 1:0:1:0: [sdc] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE sd 1:0:1:0: [sdc] Sense Key : Illegal Request [current]
sd 1:0:1:0: [sdc] Add. Sense: Invalid command operation code sd 1:0:1:0: [sdc] 71096640 512-byte hardware sectors: (36.4 GB/33.9 GiB)

OK, that's relatively easy to fix. Simply return early if the drive claims not to understand the command, and it'll try rc10 without printing the scary messages. Like this, perhaps (note cunning factoring of code):

(compile tested only, and I'll do you a nice changelog and sign-off for it if it fixes the problem and you approve of this approach).

drivers/scsi/sd.c +11 -4

index f8260c0..60b31ea 100644
@@ -1139,6 +1139,14 @@ static int media_not_present(struct scsi_disk *sdkp,
 	return 1;
 }
 
+static int invalid_field_in_cdb(struct scsi_sense_hdr *sshdr)
+{
+	if (!scsi_sense_valid(sshdr))
+		return 0;
+	return sshdr->sense_key == ILLEGAL_REQUEST &&
+			sshdr->asc == 0x24 && sshdr->ascq == 0x0;
+}
+
 /*
  * spinup disk - called only in sd_revalidate_disk()
  */

@@ -1362,6 +1370,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 
 		if (media_not_present(sdkp, &sshdr))
 			return -ENODEV;
+		if (invalid_field_in_cdb(&sshdr))
+			return -EINVAL;
 
 		if (the_result)
 			sense_valid = scsi_sense_valid(&sshdr);

@@ -1739,10 +1749,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
 	}
 
 bad_sense:
-	if (scsi_sense_valid(&sshdr) &&
-	    sshdr.sense_key == ILLEGAL_REQUEST &&
-	    sshdr.asc == 0x24 && sshdr.ascq == 0x0)
-		/* Invalid field in CDB */
+	if (invalid_field_in_cdb(&sshdr))
 		sd_printk(KERN_NOTICE, sdkp, "Cache data unavailable\n");
 	else
 		sd_printk(KERN_ERR, sdkp, "Asking for cache data failed\n");

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

James Bottomley

maintainer

2009-03-14 23:35 UTC #10 lore raw

Re: [PATCH 2/2] sd: Try READ CAPACITY 16 first for SBC-2 devices

Quoted 23 lines · Matthew Wilcox

Quoted 9 lines · James Bottomley
We're going to have to do something about the scary error messages on SBC-2 supporting drives, this is what mine say (and this is after mkp's chat reduction):
sd 1:0:1:0: [sdc] READ CAPACITY(16) failed
sd 1:0:1:0: [sdc] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE sd 1:0:1:0: [sdc] Sense Key : Illegal Request [current]
sd 1:0:1:0: [sdc] Add. Sense: Invalid command operation code sd 1:0:1:0: [sdc] 71096640 512-byte hardware sectors: (36.4 GB/33.9 GiB)
OK, that's relatively easy to fix. Simply return early if the drive claims not to understand the command, and it'll try rc10 without printing the scary messages. Like this, perhaps (note cunning factoring of code):
(compile tested only, and I'll do you a nice changelog and sign-off for it if it fixes the problem and you approve of this approach).
drivers/scsi/sd.c +0 -0
index f8260c0..60b31ea 100644

drivers/scsi/sd.c static int media_not_present(struct scsi_disk *sdkp, · line 1148

Quoted 10 lines

drivers/scsi/sd.c +7 -0

@@ -1139,6 +1139,14 @@ static int media_not_present(struct scsi_disk *sdkp,
 	return 1;
 }
 
+static int invalid_field_in_cdb(struct scsi_sense_hdr *sshdr)
+{
+	if (!scsi_sense_valid(sshdr))
+		return 0;
+	return sshdr->sense_key == ILLEGAL_REQUEST &&
+			sshdr->asc == 0x24 && sshdr->ascq == 0x0;
+

Actually, afraid not, you're trapped in the confusing maze of ASC/ASCQ codes, all sounding alike but meaning slightly different things:
0x24/0x00 is Invalid Field in CDB. The problem I'm having is 0x20/00 (Invalid Command Operation Code).

This will fix it, though ... I'll just merge it into your patch.

James

---

drivers/scsi/sd.c +8 -1

index bab5698..19a7b98 100644
@@ -1329,8 +1329,15 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		if (media_not_present(sdkp, &sshdr))
 			return -ENODEV;
 
-		if (the_result)
+		if (the_result) {
 			sense_valid = scsi_sense_valid(&sshdr);
+			if (sense_valid &&
+			    sshdr.sense_key == ILLEGAL_REQUEST &&
+			    sshdr.asc == 0x20 && sshdr.ascq == 0x00)
+				/* Invalid Command Operation Code,
+				 * just retry silently with RC10 */
+				return -EINVAL;
+		}
 		retries--;
 
 	} while (the_result && retries);

Matthew Wilcox

2009-03-14 23:47 UTC #11 lore raw

Re: [PATCH 2/2] sd: Try READ CAPACITY 16 first for SBC-2 devices

Quoted 4 lines · James Bottomley

Actually, afraid not, you're trapped in the confusing maze of ASC/ASCQ codes, all sounding alike but meaning slightly different things:
0x24/0x00 is Invalid Field in CDB. The problem I'm having is 0x20/00 (Invalid Command Operation Code).

Ooh, so close!

Quoted 1 line

This will fix it, though ... I'll just merge it into your patch.

That's fine. I'd like it if we had a set of inline functions called things like 'invalid_command_operation_code()', but we don't, so I have no objections to this patch. I think merging it in is probably best.

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Douglas Gilbert

2009-03-15 02:36 UTC #12 lore raw

Re: [PATCH 2/2] sd: Try READ CAPACITY 16 first for SBC-2 devices

In reply to James Bottomley

Quoted 38 lines · James Bottomley

Quoted 21 lines · Matthew Wilcox
Quoted 8 lines · James Bottomley
We're going to have to do something about the scary error messages on SBC-2 supporting drives, this is what mine say (and this is after mkp's chat reduction):
sd 1:0:1:0: [sdc] READ CAPACITY(16) failed
sd 1:0:1:0: [sdc] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sd 1:0:1:0: [sdc] Sense Key : Illegal Request [current] sd 1:0:1:0: [sdc] Add. Sense: Invalid command operation code sd 1:0:1:0: [sdc] 71096640 512-byte hardware sectors: (36.4 GB/33.9 GiB)
OK, that's relatively easy to fix. Simply return early if the drive claims not to understand the command, and it'll try rc10 without printing the scary messages. Like this, perhaps (note cunning factoring of code):
(compile tested only, and I'll do you a nice changelog and sign-off for it if it fixes the problem and you approve of this approach).
drivers/scsi/sd.c +0 -0
index f8260c0..60b31ea 100644
drivers/scsi/sd.c static int media_not_present(struct scsi_disk *sdkp, · line 1148
Quoted 10 lines
drivers/scsi/sd.c +7 -0
@@ -1139,6 +1139,14 @@ static int media_not_present(struct scsi_disk *sdkp,
 	return 1;
 }
 
+static int invalid_field_in_cdb(struct scsi_sense_hdr *sshdr)
+{
+	if (!scsi_sense_valid(sshdr))
+		return 0;
+	return sshdr->sense_key == ILLEGAL_REQUEST &&
+			sshdr->asc == 0x24 && sshdr->ascq == 0x0;
+
Actually, afraid not, you're trapped in the confusing maze of ASC/ASCQ codes, all sounding alike but meaning slightly different things:
0x24/0x00 is Invalid Field in CDB. The problem I'm having is 0x20/00 (Invalid Command Operation Code).

Quoted 1 line

This will fix it, though ... I'll just merge it into your patch.

Read Capacity(16) is actually Service Action In(16) with a Service Action field of 10h. My understanding is that if the device server doesn't support Service Action(16) (i.e. the "operation code" is the first byte of the cdb) then 20h/0h is the ASC/ASCQ response. However it if does support Service Action In(16) but not a Read Capacity(16) then 24h/0h is the correct ASC/ASCQ response.

The only example I can see of the latter case is if Read Long(16) is supported and Read Capacity(16) isn't. Then opcode 9eh (Service Action In(16)) is valid.

I suspect that the folks who implement SCSI disk firmware are also confused. I'm pretty sure that I have seen these two ASC/ASCQ combinations used interchangeably for unsupported commands that have a service action field.

Doug Gilbert

James Bottomley

maintainer

2009-03-15 03:30 UTC #13 lore raw

Re: [PATCH 2/2] sd: Try READ CAPACITY 16 first for SBC-2 devices

Quoted 55 lines · Douglas Gilbert

Quoted 39 lines · James Bottomley
Quoted 22 lines · Matthew Wilcox
Quoted 9 lines · James Bottomley
We're going to have to do something about the scary error messages on SBC-2 supporting drives, this is what mine say (and this is after mkp's chat reduction):
sd 1:0:1:0: [sdc] READ CAPACITY(16) failed
sd 1:0:1:0: [sdc] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE sd 1:0:1:0: [sdc] Sense Key : Illegal Request [current]
sd 1:0:1:0: [sdc] Add. Sense: Invalid command operation code sd 1:0:1:0: [sdc] 71096640 512-byte hardware sectors: (36.4 GB/33.9 GiB)
OK, that's relatively easy to fix. Simply return early if the drive claims not to understand the command, and it'll try rc10 without printing the scary messages. Like this, perhaps (note cunning factoring of code):
(compile tested only, and I'll do you a nice changelog and sign-off for it if it fixes the problem and you approve of this approach).
drivers/scsi/sd.c +0 -0
index f8260c0..60b31ea 100644
drivers/scsi/sd.c static int media_not_present(struct scsi_disk *sdkp, · line 1148
Quoted 10 lines
drivers/scsi/sd.c +7 -0
@@ -1139,6 +1139,14 @@ static int media_not_present(struct scsi_disk *sdkp,
 	return 1;
 }
 
+static int invalid_field_in_cdb(struct scsi_sense_hdr *sshdr)
+{
+	if (!scsi_sense_valid(sshdr))
+		return 0;
+	return sshdr->sense_key == ILLEGAL_REQUEST &&
+			sshdr->asc == 0x24 && sshdr->ascq == 0x0;
+
Actually, afraid not, you're trapped in the confusing maze of ASC/ASCQ codes, all sounding alike but meaning slightly different things:
0x24/0x00 is Invalid Field in CDB. The problem I'm having is 0x20/00 (Invalid Command Operation Code).
Quoted 1 line
This will fix it, though ... I'll just merge it into your patch.
Read Capacity(16) is actually Service Action In(16) with a Service Action field of 10h. My understanding is that if the device server doesn't support Service Action(16) (i.e. the "operation code" is the first byte of the cdb) then 20h/0h is the ASC/ASCQ response. However it if does support Service Action In(16) but not a Read Capacity(16) then 24h/0h is the correct ASC/ASCQ response.
The only example I can see of the latter case is if Read Long(16) is supported and Read Capacity(16) isn't. Then opcode 9eh (Service Action In(16)) is valid.

Quoted 4 lines

I suspect that the folks who implement SCSI disk firmware are also confused. I'm pretty sure that I have seen these two ASC/ASCQ combinations used interchangeably for unsupported commands that have a service action field.

Well, better safe than sorry, so this should cover all eventualities?

James

---

drivers/scsi/sd.c +5 -3

index 19a7b98..ec7f773 100644
@@ -1333,9 +1333,11 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 			sense_valid = scsi_sense_valid(&sshdr);
 			if (sense_valid &&
 			    sshdr.sense_key == ILLEGAL_REQUEST &&
-			    sshdr.asc == 0x20 && sshdr.ascq == 0x00)
-				/* Invalid Command Operation Code,
-				 * just retry silently with RC10 */
+			    (sshdr.asc == 0x20 || sshdr.asc == 0x24) &&
+			    sshdr.ascq == 0x00)
+				/* Invalid Command Operation Code or
+ 				 * Invalid Field in CDB, just retry
+ 				 * silently with RC10 */
 				return -EINVAL;
 		}
 		retries--;