[PATCH] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[PATCH] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[Matthew Wilcox]

New features are being added to the READ CAPACITY 16 results, so we want
to try to issue it in preference to READ CAPACITY 10.  Unfortunately,
some USB devices hang when they see a READ CAPACITY 16, so we limit
our chances of causing a hang by restricting this command to devices
which claim conformance to SCSI-3.  USB devices are currently limited
to claiming at most SCSI-2 conformance.

Of course, it's entirely legitimate for devices to not implement READ
CAPACITY 16, so this patch also includes a fallback to READ CAPACITY 10
for SCSI-3 devices.

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

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index f244349..25a923b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1424,11 +1424,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 (sdp->scsi_level > SCSI_2) {
 		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 {

-- 
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] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[Martin K. Petersen]

>>>>> "Matthew" == Matthew Wilcox <matthew@wil.cx> writes:

Matthew> Of course, it's entirely legitimate for devices to not
Matthew> implement READ CAPACITY 16, so this patch also includes a
Matthew> fallback to READ CAPACITY 10 for SCSI-3 devices.

I've been running with this on my DIF drives for a few days and
everything looked fine.

Tonight I tried an all-spindles-in-the-lab test.  I have several drives
that report SCSI_3 but don't implement READ CAPACITY(16).  All of them
correctly send sense error back.  That's the good news.

The bad news is that I have one particular drive model that after a
failed READ CAPACITY(16) command responds correctly to READ
CAPACITY(10).  And *then* the drive firmware commits suicide.

FWIW, this is 2Gbps/10Krpm FC kit.  So not USB/FireWire and not exactly
90s gear either.

*sigh*

At least for DIF I have an innocuous INQUIRY field to key off of.  Long
term I think we should check the Block Limits thin provisioning bits.

For now may I suggest a much more conservative approach:

-       if (sdp->scsi_level > SCSI_2) {
+       if (scsi_device_protection(sdp) || sdp->scsi_level > SCSI_SPC_2) {

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[Matthew Wilcox]

On Wed, Dec 31, 2008 at 05:18:59AM -0500, Martin K. Petersen wrote:
> I've been running with this on my DIF drives for a few days and
> everything looked fine.

You really know how to set up a sense of foreboding ;-)

> The bad news is that I have one particular drive model that after a
> failed READ CAPACITY(16) command responds correctly to READ
> CAPACITY(10).  And *then* the drive firmware commits suicide.

That's really special.  *sigh*.

> At least for DIF I have an innocuous INQUIRY field to key off of.  Long
> term I think we should check the Block Limits thin provisioning bits.

Yes, I think you're right.  At least until we find drives that put
garbage into those fields ...

> For now may I suggest a much more conservative approach:
> 
> -       if (sdp->scsi_level > SCSI_2) {
> +       if (scsi_device_protection(sdp) || sdp->scsi_level > SCSI_SPC_2) {

It was first defined in SBC-2 (2005) which corresponds to SPC-3 (also
2005), so I'm OK with this change.  I'll send updated patches (the first
patch now needs an extra NULL argument to scsi_execute_request).
Patches also available as part of this git tree:
http://git.kernel.org/?p=linux/kernel/git/willy/ssd.git;a=shortlog;h=trim-20081231
aka
git://git.kernel.org/pub/scm/linux/kernel/git/willy/ssd.git trim-20081231

Here's the replacement for the second patch (I changed the line you had
above into a function so it's easier to add more conditions later).

commit de2d519582b141fb6ca4b1f6febcd93d243c3276
Author: Matthew Wilcox <matthew@wil.cx>
Date:   Sun Dec 21 13:55:05 2008 -0500

    sd: Try READ CAPACITY 16 first for SBC-2 devices
    
    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>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 06bb638..ef01aad 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1416,6 +1416,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
  */
@@ -1425,11 +1434,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 {

-- 
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] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[Matthew Wilcox]

On Wed, Dec 31, 2008 at 06:50:15AM -0700, Matthew Wilcox wrote:
> 2005), so I'm OK with this change.  I'll send updated patches (the first
> patch now needs an extra NULL argument to scsi_execute_request).

commit 039f215a451eaa0cb04ae75e7506f178759e24e3
Author: Matthew Wilcox <matthew@wil.cx>
Date:   Wed Dec 31 07:30:12 2008 -0500

    sd: Refactor sd_read_capacity()
    
    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>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 62b28d5..06bb638 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1275,42 +1275,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);
@@ -1318,72 +1337,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. */

-- 
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] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[Matthew Wilcox]

Any reason this patch hasn't been merged into scsi-misc yet?

On Wed, Dec 31, 2008 at 06:51:42AM -0700, Matthew Wilcox wrote:
> On Wed, Dec 31, 2008 at 06:50:15AM -0700, Matthew Wilcox wrote:
> > 2005), so I'm OK with this change.  I'll send updated patches (the first
> > patch now needs an extra NULL argument to scsi_execute_request).
> 
> commit 039f215a451eaa0cb04ae75e7506f178759e24e3
> Author: Matthew Wilcox <matthew@wil.cx>
> Date:   Wed Dec 31 07:30:12 2008 -0500
> 
>     sd: Refactor sd_read_capacity()
>     
>     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>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 62b28d5..06bb638 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1275,42 +1275,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);
> @@ -1318,72 +1337,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. */
> 
> -- 
> 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."

-- 
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] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[James Bottomley]

On Mon, 2009-02-23 at 14:51 -0700, Matthew Wilcox wrote:
> Any reason this patch hasn't been merged into scsi-misc yet?

There was a problem with it I drew your attention to here:

http://marc.info/?l=linux-scsi&m=123308499405071

James

Re: [PATCH] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[Matthew Wilcox]

On Wed, Feb 25, 2009 at 02:23:41PM -0800, James Bottomley wrote:
> On Mon, 2009-02-23 at 14:51 -0700, Matthew Wilcox wrote:
> > Any reason this patch hasn't been merged into scsi-misc yet?
> 
> There was a problem with it I drew your attention to here:
> 
> http://marc.info/?l=linux-scsi&m=123308499405071

This was the updated version, sent on Dec 31, not the version you
commented on on Dec 23.

(I didn't notice your earlier request because I was on vacation when
you sent it).

-- 
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] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[James Bottomley]

On Wed, 2009-02-25 at 15:31 -0700, Matthew Wilcox wrote:
> On Wed, Feb 25, 2009 at 02:23:41PM -0800, James Bottomley wrote:
> > On Mon, 2009-02-23 at 14:51 -0700, Matthew Wilcox wrote:
> > > Any reason this patch hasn't been merged into scsi-misc yet?
> > 
> > There was a problem with it I drew your attention to here:
> > 
> > http://marc.info/?l=linux-scsi&m=123308499405071
> 
> This was the updated version, sent on Dec 31, not the version you
> commented on on Dec 23.

Oh, sorry, missed the update.

James

Re: [PATCH] sd: Try READ CAPACITY 16 first for all SCSI-3 devices

[James Bottomley]

On Tue, 2008-12-23 at 09:47 -0700, Matthew Wilcox wrote:
> New features are being added to the READ CAPACITY 16 results, so we want
> to try to issue it in preference to READ CAPACITY 10.  Unfortunately,
> some USB devices hang when they see a READ CAPACITY 16, so we limit
> our chances of causing a hang by restricting this command to devices
> which claim conformance to SCSI-3.  USB devices are currently limited
> to claiming at most SCSI-2 conformance.
> 
> Of course, it's entirely legitimate for devices to not implement READ
> CAPACITY 16, so this patch also includes a fallback to READ CAPACITY 10
> for SCSI-3 devices.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> ---
>  drivers/scsi/sd.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index f244349..25a923b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1424,11 +1424,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 (sdp->scsi_level > SCSI_2) {

Were you going to update this?  Martin Petersen already found an early
SCSI-3 device that crashes upon receiving Read Capacity(16).

James