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

2012-11-12 Thread 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/
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@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

2012-11-12 Thread James Bottomley 
On Fri, 2012-11-09 at 16:33 +, 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 majord...@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

2012-11-12 Thread 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


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@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

2012-11-12 Thread 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  0xULL  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  0x) {
 +} else if (block  0x || 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  0x) {
+   } else if (sdkp-capacity  0x) {

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 majord...@vger.kernel.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 majord...@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

2012-11-12 Thread Jason J. Herne 
On Mon, Nov 12, 2012 at 9:31 AM, Paolo Bonzini pbonz...@redhat.com 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  0x) {
 +   } else if (sdkp-capacity  0x) {

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

2012-11-12 Thread 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  0xULL  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  0x) {
  +  } else if (block  0x || 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  0x) {
 + } else if (sdkp-capacity  0x) {
 
 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 majord...@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

2012-11-12 Thread Jason J. Herne 
On Mon, Nov 12, 2012 at 10:10 AM, James Bottomley
james.bottom...@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  0xULL  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  0x) {
  +  } else if (block  0x || 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  0x) {
 + } else if (sdkp-capacity  0x) {

 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 (hern...@gmail.com)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@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

2012-11-12 Thread 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  0x) {
  +   } else if (sdkp-capacity  0x) {
 
  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


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@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

2012-11-12 Thread 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  0x) {
  +  } else if (sdkp-capacity  0x) {
  
  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
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@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

2012-11-12 Thread 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  0x) {

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

2012-11-11 Thread 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-11-09 Thread Jason J. Herne 
From: Jason J. Herne hern...@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 hern...@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  0xULL  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  0x) {
+   } else if (block  0x || 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-usb in
the body of a message to majord...@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

2012-11-09 Thread 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-ow...@vger.kernel.org 
[mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Jason J. Herne
Sent: Friday, November 09, 2012 9:08 AM
To: linux-s...@vger.kernel.org
Cc: linux-usb@vger.kernel.org; st...@rowland.harvard.edu; Jason J. Herne
Subject: [PATCH] USB enclosures seem to require read(16) with 2TB drives

From: Jason J. Herne hern...@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 hern...@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  0xULL  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  0x) {
+   } else if (block  0x || 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 majord...@vger.kernel.org
More majordomo info at  http