[PATCH] USB enclosures seem to require read(16) with >2TB drives

[PATCH] USB enclosures seem to require read(16) with >2TB drives

[Jason J. Herne]

From: "Jason J. Herne" <hernejj@gmail.com>

Force large capacity (> 2TB) drives in USB enclosures to use READ(16) instead
of READ(10).  Some(most/all?) enclosures do not like READ(10) commands when a
large capacity drive is installed.

Signed-off-by: Jason J. Herne <hernejj@gmail.com>
---
 drivers/scsi/sd.c              |    7 +++++--
 drivers/usb/storage/scsiglue.c |    5 +++++
 include/scsi/scsi_device.h     |    1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4b63c73..9b65ddf 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -649,7 +649,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	sector_t block = blk_rq_pos(rq);
 	sector_t threshold;
 	unsigned int this_count = blk_rq_sectors(rq);
-	int ret, host_dif;
+	int ret, host_dif, force_read16;
 	unsigned char protect;
 
 	/*
@@ -797,6 +797,9 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	else
 		protect = 0;
 
+	/* Many large capacity USB drives/controllers require the use of read(16). */
+	force_read16 = (sdkp->capacity > 0xffffffffULL && sdp->force_read_16);
+
 	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
 		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
 
@@ -833,7 +836,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
 		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
 		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
-	} else if (block > 0xffffffff) {
+	} else if (block > 0xffffffff || force_read16) {
 		SCpnt->cmnd[0] += READ_16 - READ_6;
 		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
 		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 13b8bcd..6ff785e 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device *sdev)
 					US_FL_SCM_MULT_TARG)) &&
 				us->protocol == USB_PR_BULK)
 			us->use_last_sector_hacks = 1;
+
+		/* Force read-16 for large capacity drives. */
+		sdev->force_read_16 = 1;
+
+
 	} else {
 
 		/* Non-disk-type devices don't need to blacklist any pages
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5591ed5..e92b846 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -134,6 +134,7 @@ struct scsi_device {
 				     * because we did a bus reset. */
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
+	unsigned force_read_16:1; /* Use read(16) over read(10) */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
 	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
-- 
1.7.9.5

RE: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Elliott, Robert (Server Storage)]

I recommend broadening this patch.  T10 is discussing making READ (10), WRITE (10), etc. obsolete in SBC-4 in favor of their 16-byte CDB counterparts.  

The algorithm should be:
1. During discovery, determine if 16-byte CDBs are supported.  There are several ways to determine this:
a) REPORT SUPPORTED OPERATION CODES command succeeds and reports that READ (16) et al are supported.
b) READ (16) command specifying a Transfer Length of zero succeeds.
c) READ CAPACITY (16) command succeeds and reports that the capacity is > 2 TiB.
d) (future) INQUIRY command succeeds fetching the Block Device Characteristics VPD page and notices a new field added by the SBC-4 simplified SCSI feature set proposal.

Since REPORT SUPPORTED OPERATION CODES is optional, it won't always work. READ CAPACITY (16) used to be optional for < 2 TiB drives, so it won't always work either. READ (16) will always work, but requires the drive to be spun up beforehand (e.g., with START STOP UNIT).
 
2. if 16-byte CDBs are supported, then use them; only drop down to 10-byte CDBs if 16-byte CDBs are unavailable.  Don't make the decision by comparing the LBA on every IO.



-----Original Message-----
From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jason J. Herne
Sent: Friday, November 09, 2012 9:08 AM
To: linux-scsi@vger.kernel.org
Cc: linux-usb@vger.kernel.org; stern@rowland.harvard.edu; Jason J. Herne
Subject: [PATCH] USB enclosures seem to require read(16) with >2TB drives

From: "Jason J. Herne" <hernejj@gmail.com>

Force large capacity (> 2TB) drives in USB enclosures to use READ(16) instead
of READ(10).  Some(most/all?) enclosures do not like READ(10) commands when a
large capacity drive is installed.

Signed-off-by: Jason J. Herne <hernejj@gmail.com>
---
 drivers/scsi/sd.c              |    7 +++++--
 drivers/usb/storage/scsiglue.c |    5 +++++
 include/scsi/scsi_device.h     |    1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4b63c73..9b65ddf 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -649,7 +649,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	sector_t block = blk_rq_pos(rq);
 	sector_t threshold;
 	unsigned int this_count = blk_rq_sectors(rq);
-	int ret, host_dif;
+	int ret, host_dif, force_read16;
 	unsigned char protect;
 
 	/*
@@ -797,6 +797,9 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	else
 		protect = 0;
 
+	/* Many large capacity USB drives/controllers require the use of read(16). */
+	force_read16 = (sdkp->capacity > 0xffffffffULL && sdp->force_read_16);
+
 	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
 		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
 
@@ -833,7 +836,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
 		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
 		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
-	} else if (block > 0xffffffff) {
+	} else if (block > 0xffffffff || force_read16) {
 		SCpnt->cmnd[0] += READ_16 - READ_6;
 		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
 		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 13b8bcd..6ff785e 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device *sdev)
 					US_FL_SCM_MULT_TARG)) &&
 				us->protocol == USB_PR_BULK)
 			us->use_last_sector_hacks = 1;
+
+		/* Force read-16 for large capacity drives. */
+		sdev->force_read_16 = 1;
+
+
 	} else {
 
 		/* Non-disk-type devices don't need to blacklist any pages
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5591ed5..e92b846 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -134,6 +134,7 @@ struct scsi_device {
 				     * because we did a bus reset. */
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
+	unsigned force_read_16:1; /* Use read(16) over read(10) */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
 	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Stefan Richter]

On Nov 09 Elliott, Robert (Server Storage) wrote:
> I recommend broadening this patch.  T10 is discussing making READ (10), WRITE (10), etc. obsolete in SBC-4 in favor of their 16-byte CDB counterparts.  
> 
> The algorithm should be:
> 1. During discovery, determine if 16-byte CDBs are supported.  There are several ways to determine this:
> a) REPORT SUPPORTED OPERATION CODES command succeeds and reports that READ (16) et al are supported.
> b) READ (16) command specifying a Transfer Length of zero succeeds.
> c) READ CAPACITY (16) command succeeds and reports that the capacity is > 2 TiB.
> d) (future) INQUIRY command succeeds fetching the Block Device Characteristics VPD page and notices a new field added by the SBC-4 simplified SCSI feature set proposal.
> 
> Since REPORT SUPPORTED OPERATION CODES is optional, it won't always work. READ CAPACITY (16) used to be optional for < 2 TiB drives, so it won't always work either. READ (16) will always work, but requires the drive to be spun up beforehand (e.g., with START STOP UNIT).

This won't work.  It will crash badly written device firmwares.

Instead, try the (10) commands on the SBC-4 device and let it respond that
it does not implement these commands.  Or have other means to be certain of
SBC-4 compliance without issuing commands that were optional in or not
defined by earlier revisions of the spec.  I wonder whether testing for
INQUIRY_data.VERSION >= something is a sufficiently safe test.

> 2. if 16-byte CDBs are supported, then use them; only drop down to 10-byte CDBs if 16-byte CDBs are unavailable.  Don't make the decision by comparing the LBA on every IO.

-- 
Stefan Richter
-=====-===-- =-== -=-==
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Stefan Richter]

On Nov 11 Stefan Richter wrote:
> On Nov 09 Elliott, Robert (Server Storage) wrote:
> > I recommend broadening this patch.  T10 is discussing making READ (10), WRITE (10), etc. obsolete in SBC-4 in favor of their 16-byte CDB counterparts.  
> > 
> > The algorithm should be:
> > 1. During discovery, determine if 16-byte CDBs are supported.  There are several ways to determine this:
> > a) REPORT SUPPORTED OPERATION CODES command succeeds and reports that READ (16) et al are supported.
> > b) READ (16) command specifying a Transfer Length of zero succeeds.
> > c) READ CAPACITY (16) command succeeds and reports that the capacity is > 2 TiB.
> > d) (future) INQUIRY command succeeds fetching the Block Device Characteristics VPD page and notices a new field added by the SBC-4 simplified SCSI feature set proposal.
> > 
> > Since REPORT SUPPORTED OPERATION CODES is optional, it won't always work. READ CAPACITY (16) used to be optional for < 2 TiB drives, so it won't always work either. READ (16) will always work, but requires the drive to be spun up beforehand (e.g., with START STOP UNIT).
> 
> This won't work.  It will crash badly written device firmwares.
> 
> Instead, try the (10) commands on the SBC-4 device and let it respond that
> it does not implement these commands.  Or have other means to be certain of
> SBC-4 compliance without issuing commands that were optional in or not
> defined by earlier revisions of the spec.  I wonder whether testing for
> INQUIRY_data.VERSION >= something is a sufficiently safe test.

Let me revise this:  Try READ CAPACITY (10) first (unless SPC-3's
INQUIRY_data.PROTECT is set, in which case I don't know what is safer;
READ CAPACITY (16) first or READ CAPACITY (10) first).

If this showed a capacity > 2 TB, then Jason's suggestion to always only
issue READ (16) to all USB attached devices makes a lot of sense to me if
it is true that Windows 7 never issues READ (10) to them.

That would be what the proposed patch does.  What about WRITE and the
various other (10)/(16) pairs of commands though?

I don't know what's best in case of transports other than USB (after
capacity > 2 TB was established).
-- 
Stefan Richter
-=====-===-- =-== -==--
http://arcgraph.de/sr/

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[James Bottomley]

On Fri, 2012-11-09 at 16:33 +0000, Elliott, Robert (Server Storage)
wrote:
> I recommend broadening this patch.  T10 is discussing making READ
> (10), WRITE (10), etc. obsolete in SBC-4 in favor of their 16-byte CDB
> counterparts.  
> 
> The algorithm should be:
> 1. During discovery, determine if 16-byte CDBs are supported.  There
> are several ways to determine this:
> a) REPORT SUPPORTED OPERATION CODES command succeeds and reports that
> READ (16) et al are supported.
> b) READ (16) command specifying a Transfer Length of zero succeeds.
> c) READ CAPACITY (16) command succeeds and reports that the capacity
> is > 2 TiB.
> d) (future) INQUIRY command succeeds fetching the Block Device
> Characteristics VPD page and notices a new field added by the SBC-4
> simplified SCSI feature set proposal.

When you consider the problem that we must support all devices (which
means the older ones as well), including the annoying USB ones which
crash on unexpected commands, you can see that d) is about the only
viable option.

We can also force RC16 if the capacity is over 2^32 blocks, because the
command will be required, so that's probably the place to start.

James

> Since REPORT SUPPORTED OPERATION CODES is optional, it won't always
> work. READ CAPACITY (16) used to be optional for < 2 TiB drives, so it
> won't always work either. READ (16) will always work, but requires the
> drive to be spun up beforehand (e.g., with START STOP UNIT).
>  
> 2. if 16-byte CDBs are supported, then use them; only drop down to
> 10-byte CDBs if 16-byte CDBs are unavailable.  Don't make the decision
> by comparing the LBA on every IO.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[James Bottomley]

On Fri, 2012-11-09 at 11:08 -0500, Jason J. Herne wrote:
> diff --git a/drivers/usb/storage/scsiglue.c
> b/drivers/usb/storage/scsiglue.c
> index 13b8bcd..6ff785e 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device
> *sdev)
>                                         US_FL_SCM_MULT_TARG)) &&
>                                 us->protocol == USB_PR_BULK)
>                         us->use_last_sector_hacks = 1;
> +
> +               /* Force read-16 for large capacity drives. */
> +               sdev->force_read_16 = 1;
> +
> +

This turns READ_16 on unconditionally for all USB disks ... is that
safe?  As in have you tested this with older USB thumb drives?

Also do we have to worry about TYPE_RBC? ... this looks like a bit of a
global omission in usbstorage.c


> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 5591ed5..e92b846 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -134,6 +134,7 @@ struct scsi_device {
>                                      * because we did a bus reset. */
>         unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>         unsigned use_10_for_ms:1; /* first try 10-byte mode
> sense/select */
> +       unsigned force_read_16:1; /* Use read(16) over read(10) */

This should probably be use_16_for_rw:1 for consistency.

James

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Paolo Bonzini]

Il 12/11/2012 12:33, James Bottomley ha scritto:
> On Fri, 2012-11-09 at 11:08 -0500, Jason J. Herne wrote:
>> diff --git a/drivers/usb/storage/scsiglue.c
>> b/drivers/usb/storage/scsiglue.c
>> index 13b8bcd..6ff785e 100644
>> --- a/drivers/usb/storage/scsiglue.c
>> +++ b/drivers/usb/storage/scsiglue.c
>> @@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device
>> *sdev)
>>                                         US_FL_SCM_MULT_TARG)) &&
>>                                 us->protocol == USB_PR_BULK)
>>                         us->use_last_sector_hacks = 1;
>> +
>> +               /* Force read-16 for large capacity drives. */
>> +               sdev->force_read_16 = 1;
>> +
>> +
> 
> This turns READ_16 on unconditionally for all USB disks ... is that
> safe?  As in have you tested this with older USB thumb drives?

Actually it only turns it on for large capacity drives, as said in the
comment.  sdp->force_read_16 only matters for >2TB drives: 
 
>> +	/* Many large capacity USB drives/controllers require the use of read(16). */
>> +	force_read16 = (sdkp->capacity > 0xffffffffULL && sdp->force_read_16);
>> +
>>  	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
>>  		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
>>  
>> @@ -833,7 +836,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>>  		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
>>  		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
>>  		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
>> -	} else if (block > 0xffffffff) {
>> +	} else if (block > 0xffffffff || force_read16) {

so the better name would be never_use_10_for_rw_on_large_disks.  For
some definitions of better. :)

Any reason not to do this always on >2TB drives, which basically means
changing this:

-	} else if (block > 0xffffffff) {
+	} else if (sdkp->capacity > 0xffffffff) {

and nothing else?

Paolo

> 
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 5591ed5..e92b846 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -134,6 +134,7 @@ struct scsi_device {
>>                                      * because we did a bus reset. */
>>         unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>>         unsigned use_10_for_ms:1; /* first try 10-byte mode
>> sense/select */
>> +       unsigned force_read_16:1; /* Use read(16) over read(10) */
> 
> This should probably be use_16_for_rw:1 for consistency.
> 
> James
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Jason J. Herne]

On Mon, Nov 12, 2012 at 9:31 AM, Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Il 12/11/2012 12:33, James Bottomley ha scritto:
>> On Fri, 2012-11-09 at 11:08 -0500, Jason J. Herne wrote:
>>> diff --git a/drivers/usb/storage/scsiglue.c
>>> b/drivers/usb/storage/scsiglue.c
>>> index 13b8bcd..6ff785e 100644
>>> --- a/drivers/usb/storage/scsiglue.c
>>> +++ b/drivers/usb/storage/scsiglue.c
>>> @@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device
>>> *sdev)
>>>                                         US_FL_SCM_MULT_TARG)) &&
>>>                                 us->protocol == USB_PR_BULK)
>>>                         us->use_last_sector_hacks = 1;
>>> +
>>> +               /* Force read-16 for large capacity drives. */
>>> +               sdev->force_read_16 = 1;
>>> +
>>> +
>>
>> This turns READ_16 on unconditionally for all USB disks ... is that
>> safe?  As in have you tested this with older USB thumb drives?
>
> Actually it only turns it on for large capacity drives, as said in the
> comment.  sdp->force_read_16 only matters for >2TB drives:
>

I should have made this clearer.  Sorry for any confusion.

> Any reason not to do this always on >2TB drives, which basically means
> changing this:
>
> -       } else if (block > 0xffffffff) {
> +       } else if (sdkp->capacity > 0xffffffff) {
>
> and nothing else?

This was the intent of my patch, except I wanted to *only* affect USB based
drives as my drive was functioning perfecting when connected to an internal
Sata port.  I was adopting the "Do not fix what isn't broke" mentality.

If there is no reason to do otherwise, I like this suggestion.  It is simple
and removes the need to provide an extra bit in the scsi_device struct.
It also eliminates the need to do any additional probe/init time checking.
We just want to be sure it won't cause problems for currently working
devices.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[James Bottomley]

On Mon, 2012-11-12 at 10:01 -0500, Jason J. Herne wrote:
> > Any reason not to do this always on >2TB drives, which basically means
> > changing this:
> >
> > -       } else if (block > 0xffffffff) {
> > +       } else if (sdkp->capacity > 0xffffffff) {
> >
> > and nothing else?
> 
> This was the intent of my patch, except I wanted to *only* affect USB based
> drives as my drive was functioning perfecting when connected to an internal
> Sata port.  I was adopting the "Do not fix what isn't broke" mentality.

There's a subtlety here: block is in units of the disk sector size,
sdkp->capacity is in units of 512 bytes (the linux native sector size),
so it would need shifting before doing the determination.

James

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[James Bottomley]

On Mon, 2012-11-12 at 15:31 +0100, Paolo Bonzini wrote:
> Il 12/11/2012 12:33, James Bottomley ha scritto:
> > On Fri, 2012-11-09 at 11:08 -0500, Jason J. Herne wrote:
> >> diff --git a/drivers/usb/storage/scsiglue.c
> >> b/drivers/usb/storage/scsiglue.c
> >> index 13b8bcd..6ff785e 100644
> >> --- a/drivers/usb/storage/scsiglue.c
> >> +++ b/drivers/usb/storage/scsiglue.c
> >> @@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device
> >> *sdev)
> >>                                         US_FL_SCM_MULT_TARG)) &&
> >>                                 us->protocol == USB_PR_BULK)
> >>                         us->use_last_sector_hacks = 1;
> >> +
> >> +               /* Force read-16 for large capacity drives. */
> >> +               sdev->force_read_16 = 1;
> >> +
> >> +
> > 
> > This turns READ_16 on unconditionally for all USB disks ... is that
> > safe?  As in have you tested this with older USB thumb drives?
> 
> Actually it only turns it on for large capacity drives, as said in the
> comment.  sdp->force_read_16 only matters for >2TB drives: 

If you follow the discussion, we'll need to turn it on for some drives
regardless of size.

> >> +	/* Many large capacity USB drives/controllers require the use of read(16). */
> >> +	force_read16 = (sdkp->capacity > 0xffffffffULL && sdp->force_read_16);
> >> +
> >>  	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
> >>  		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
> >>  
> >> @@ -833,7 +836,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
> >>  		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
> >>  		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
> >>  		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
> >> -	} else if (block > 0xffffffff) {
> >> +	} else if (block > 0xffffffff || force_read16) {
> 
> so the better name would be never_use_10_for_rw_on_large_disks.  For
> some definitions of better. :)

Hm.

> Any reason not to do this always on >2TB drives, which basically means
> changing this:
> 
> -	} else if (block > 0xffffffff) {
> +	} else if (sdkp->capacity > 0xffffffff) {
> 
> and nothing else?

Because of the coming T10 mandate in SBC-4 deprecating everything other
than the 16 byte commands.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Jason J. Herne]

On Mon, Nov 12, 2012 at 10:10 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2012-11-12 at 15:31 +0100, Paolo Bonzini wrote:
>> Il 12/11/2012 12:33, James Bottomley ha scritto:
>> > On Fri, 2012-11-09 at 11:08 -0500, Jason J. Herne wrote:
>> >> diff --git a/drivers/usb/storage/scsiglue.c
>> >> b/drivers/usb/storage/scsiglue.c
>> >> index 13b8bcd..6ff785e 100644
>> >> --- a/drivers/usb/storage/scsiglue.c
>> >> +++ b/drivers/usb/storage/scsiglue.c
>> >> @@ -251,6 +251,11 @@ static int slave_configure(struct scsi_device
>> >> *sdev)
>> >>                                         US_FL_SCM_MULT_TARG)) &&
>> >>                                 us->protocol == USB_PR_BULK)
>> >>                         us->use_last_sector_hacks = 1;
>> >> +
>> >> +               /* Force read-16 for large capacity drives. */
>> >> +               sdev->force_read_16 = 1;
>> >> +
>> >> +
>> >
>> > This turns READ_16 on unconditionally for all USB disks ... is that
>> > safe?  As in have you tested this with older USB thumb drives?
>>
>> Actually it only turns it on for large capacity drives, as said in the
>> comment.  sdp->force_read_16 only matters for >2TB drives:
>
> If you follow the discussion, we'll need to turn it on for some drives
> regardless of size.
>
>> >> +  /* Many large capacity USB drives/controllers require the use of read(16). */
>> >> +  force_read16 = (sdkp->capacity > 0xffffffffULL && sdp->force_read_16);
>> >> +
>> >>    if (host_dif == SD_DIF_TYPE2_PROTECTION) {
>> >>            SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
>> >>
>> >> @@ -833,7 +836,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>> >>            SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
>> >>            SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
>> >>            SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
>> >> -  } else if (block > 0xffffffff) {
>> >> +  } else if (block > 0xffffffff || force_read16) {
>>
>> so the better name would be never_use_10_for_rw_on_large_disks.  For
>> some definitions of better. :)
>
> Hm.
>
>> Any reason not to do this always on >2TB drives, which basically means
>> changing this:
>>
>> -     } else if (block > 0xffffffff) {
>> +     } else if (sdkp->capacity > 0xffffffff) {
>>
>> and nothing else?
>
> Because of the coming T10 mandate in SBC-4 deprecating everything other
> than the 16 byte commands.
>

It seems like the deprecations in the coming SBC-4 spec (still in
draft state, yes?) are a
separate issue best handled with a separate patch.  My goal with this
patch is to allow Linux
to work with some (most/all?) large capacity drives in external USB
enclosures. The one line
solution proposed by Paolo accomplishes this goal while minimizing
code changes that
might need to be revised/undone later when the deprecation of
read/write(<16) is being
handled.

--
- Jason J. Herne (hernejj@gmail.com)

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Paolo Bonzini]

Il 12/11/2012 16:10, James Bottomley ha scritto:
>> Actually it only turns it on for large capacity drives, as said in the
>> comment.  sdp->force_read_16 only matters for >2TB drives: 
> 
> If you follow the discussion, we'll need to turn it on for some drives
> regardless of size.

Even if the two reasons to use r/w(16) commands were setting the same
flag, it would be handled in a separate patch; it doesn't really make
sense to complicate the code now when a one-liner does it.

The proposed change is not part of the Oct 31st draft available on
t10.org, for what we know the discussion could end up in nothing.

>> > Any reason not to do this always on >2TB drives, which basically means
>> > changing this:
>> > 
>> > -	} else if (block > 0xffffffff) {
>> > +	} else if (sdkp->capacity > 0xffffffff) {
>> > 
>> > and nothing else?
> Because of the coming T10 mandate in SBC-4 deprecating everything other
> than the 16 byte commands.

And would this change make the upcoming patch for SBC-4 support longer
or harder to review?

Paolo

Re: [PATCH] USB enclosures seem to require read(16) with >2TB drives

[Alan Stern]

On Mon, 12 Nov 2012, Paolo Bonzini wrote:

> Il 12/11/2012 16:10, James Bottomley ha scritto:
> >> Actually it only turns it on for large capacity drives, as said in the
> >> comment.  sdp->force_read_16 only matters for >2TB drives: 
> > 
> > If you follow the discussion, we'll need to turn it on for some drives
> > regardless of size.
> 
> Even if the two reasons to use r/w(16) commands were setting the same
> flag, it would be handled in a separate patch; it doesn't really make
> sense to complicate the code now when a one-liner does it.
> 
> The proposed change is not part of the Oct 31st draft available on
> t10.org, for what we know the discussion could end up in nothing.

There's a simple way to do what everybody wants.  Add a "use_16_for_rw"  
flag with the understanding that it overrides use_10_for_rw, and set
this flag in sd_read_capacity() if the actual capacity is >= 2^32
blocks (as opposed to 2^41 bytes).  Similarly, clear the flag if the
actual capacity is smaller -- a device with removable media might 
require this, in theory.

Then the test in sd_prep_fn() becomes

	if (sdp->use_16_for_rw) {

rather than

	if (block > 0xffffffff) {

which on 32-bit architectures is a simpler test.  If T10 decides to
deprecate the 10-byte commands then a second patch can set the new flag
(and avoid clearing it) under the appropriate circumstances.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html