Support READ CAPACITY 16 on more drives

Support READ CAPACITY 16 on more drives

[Matthew Wilcox]

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.

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

[Matthew Wilcox]

From: Matthew Wilcox <matthew@wil.cx>

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 <willy@linux.intel.com>
---
 drivers/scsi/sd.c |  247 ++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 158 insertions(+), 89 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 55310db..6cf0c25 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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. */
-- 
1.6.1.3

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

[Martin K. Petersen]

Matthew> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

Tested-by: Martin K. Petersen <martin.petersen@oracle.com>

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

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

[James Bottomley]

On Thu, 2009-03-12 at 14:20 -0400, Matthew Wilcox wrote:
> +#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

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

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

James> On Thu, 2009-03-12 at 14:20 -0400, Matthew Wilcox wrote:
>> +#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.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Matthew Wilcox]

On Fri, Mar 13, 2009 at 04:29:36PM -0500, James Bottomley wrote:
> On Thu, 2009-03-12 at 14:20 -0400, Matthew Wilcox wrote:
> > +#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?

-- 
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."

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

[James Bottomley]

On Fri, 2009-03-13 at 19:19 -0600, Matthew Wilcox wrote:
> On Fri, Mar 13, 2009 at 04:29:36PM -0500, James Bottomley wrote:
> > On Thu, 2009-03-12 at 14:20 -0400, Matthew Wilcox wrote:
> > > +#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?

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

James

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

[Matthew Wilcox]

From: Matthew Wilcox <matthew@wil.cx>

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 <willy@linux.intel.com>
Tested-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6cf0c25..b155488 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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 {
-- 
1.6.1.3

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

[James Bottomley]

On Thu, 2009-03-12 at 14:20 -0400, Matthew Wilcox wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> 
> 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

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

[Matthew Wilcox]

On Sat, Mar 14, 2009 at 03:41:31PM -0500, James Bottomley wrote:
> 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).

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index f8260c0..60b31ea 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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");

-- 
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."

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

[James Bottomley]

On Sat, 2009-03-14 at 16:48 -0600, Matthew Wilcox wrote:
> On Sat, Mar 14, 2009 at 03:41:31PM -0500, James Bottomley wrote:
> > 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).
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index f8260c0..60b31ea 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -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

---

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bab5698..19a7b98 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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);

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

[Matthew Wilcox]

On Sat, Mar 14, 2009 at 06:34:55PM -0500, James Bottomley wrote:
> 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!

> 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.

-- 
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."

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

[Douglas Gilbert]

James Bottomley wrote:
> On Sat, 2009-03-14 at 16:48 -0600, Matthew Wilcox wrote:
>> On Sat, Mar 14, 2009 at 03:41:31PM -0500, James Bottomley wrote:
>>> 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).
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index f8260c0..60b31ea 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -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.

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

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

[James Bottomley]

On Sat, 2009-03-14 at 22:36 -0400, Douglas Gilbert wrote:
> James Bottomley wrote:
> > On Sat, 2009-03-14 at 16:48 -0600, Matthew Wilcox wrote:
> >> On Sat, Mar 14, 2009 at 03:41:31PM -0500, James Bottomley wrote:
> >>> 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).
> >>
> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >> index f8260c0..60b31ea 100644
> >> --- a/drivers/scsi/sd.c
> >> +++ b/drivers/scsi/sd.c
> >> @@ -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.
> 
> 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.

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

James

---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 19a7b98..ec7f773 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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--;