USB storage devices and SAT

USB storage devices and SAT

[Douglas Gilbert]

USB storage devices that support SAT (the T10 SCSI to ATA
translation standard) are beginning to appear.

SAT enables tools like smartmontools to access SMART data
on a ATA disk in a USB enclosure. We have run into
a problem. It seems that the usb storage subsystem is wedded
to the idea of sense data that is no longer than 18 bytes. **
That doesn't play well with SAT which uses descriptor format
sense data that is made up of an 8 byte header plus zero or
more descriptors. SAT uses a 14 byte "ATA return" (sense)
descriptor to yield the ATA registers. That means the sense
data length is 22 bytes when an ATA return descriptor is
required.

Alan Stern has already noted to another smartmontools developer
that such a change is likely to break some USB storage devices.
Perhaps the maximum sense buffer size could be optionally
specified per usb storage device. Alternatively the usb mass
storage logic could make some dynamic decisions itself.
For example if the (disk) device responds successfully to either
SCSI ATA PASS_THROUGH (12 or 16 byte) command then it will
be capable of (at least) 22 byte sense data. A T10 vendor
identification field in a standard INQUIRY response of
"ATA     " is another indication of a device that supports
SAT.


** the 18 byte sense data limit comes from SCSI-2 (circa 1992).
That was upped to 252 bytes around 10 years ago. Some SCSI disk
vendors have been using the extra bytes (above 18) for some
time. Then in SPC-3 (standard in 2005) descriptor format sense
data was introduced and SAT uses it. There are other uses: any
SCSI (virtual) disk that needs more than 32 bits to represent all
its LBAs should use descriptor format for MEDIUM ERRORs. The
reason is that MEDIUM ERRORs should include the LBA of the first
failure and the "info" field in the older "fixed" sense format
is only 4 bytes.

Doug Gilbert

Re: USB storage devices and SAT

[James Bottomley]

On Mon, 2008-08-04 at 03:10 +0200, Douglas Gilbert wrote:
> USB storage devices that support SAT (the T10 SCSI to ATA
> translation standard) are beginning to appear.

Um, confused.  The USB devices that were ATA under the covers already
have to do SAT since the USB storage transport mandates SCSI.

You mean they're now supporting ATA-12/16 pass through?  And will this
become standard for non-ATA USB storage (like flash)?

James

Re: [usb-storage] USB storage devices and SAT

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Sun, Aug 03, 2008 at 08:33:13PM -0500, James Bottomley wrote:
> On Mon, 2008-08-04 at 03:10 +0200, Douglas Gilbert wrote:
> > USB storage devices that support SAT (the T10 SCSI to ATA
> > translation standard) are beginning to appear.
> 
> Um, confused.  The USB devices that were ATA under the covers already
> have to do SAT since the USB storage transport mandates SCSI.

The enclosures today translate a small subset set of SCSI commands to
ATA/ATAPI commands.

SAT would allow arbitrary ATA commands to be sent to target devices.  In
theory.  I've only seen a few products that support ATA passthrough, and
they all did it in a vendor-proprietary manner.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I could always suspend a few hundred accounts and watch what happens.
					-- Tanya
User Friendly, 7/31/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [usb-storage] USB storage devices and SAT

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Mon, Aug 04, 2008 at 03:10:04AM +0200, Douglas Gilbert wrote:
> Alan Stern has already noted to another smartmontools developer
> that such a change is likely to break some USB storage devices.
> Perhaps the maximum sense buffer size could be optionally
> specified per usb storage device. Alternatively the usb mass
> storage logic could make some dynamic decisions itself.

To clarify: A great many devices choke (fatally) if asked for sense data
other than 18 bytes.  Since the first TEST_UNIT_READY will likely require
sense data, almost every device sees REQUEST_SENSE.

Personally, I hate having to make dynamic decisions in usb-storage.  The
more we try to do there, the more likely we are to get it wrong.

If you've got an app that is sending a command, and you KNOW that command
should produce >18 bytes of sense data, then there should be a way to
specify to the SCSI core (and thus get passed to usb-storage) that sense
data of >18 bytes should be requested.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I'm seen in many forms.  Now open your mouth.  It's caffeine time.
					-- Cola Man to Greg
User Friendly, 10/28/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [usb-storage] USB storage devices and SAT

[Boaz Harrosh]

Matthew Dharm wrote:
> On Mon, Aug 04, 2008 at 03:10:04AM +0200, Douglas Gilbert wrote:
>> Alan Stern has already noted to another smartmontools developer
>> that such a change is likely to break some USB storage devices.
>> Perhaps the maximum sense buffer size could be optionally
>> specified per usb storage device. Alternatively the usb mass
>> storage logic could make some dynamic decisions itself.
> 
> To clarify: A great many devices choke (fatally) if asked for sense data
> other than 18 bytes.  Since the first TEST_UNIT_READY will likely require
> sense data, almost every device sees REQUEST_SENSE.
> 
> Personally, I hate having to make dynamic decisions in usb-storage.  The
> more we try to do there, the more likely we are to get it wrong.
> 
> If you've got an app that is sending a command, and you KNOW that command
> should produce >18 bytes of sense data, then there should be a way to
> specify to the SCSI core (and thus get passed to usb-storage) that sense
> data of >18 bytes should be requested.
> 

What?!! The number 18 comes totally from USB land at:
drivers/usb/storage/transport.c:601 via call to scsi_eh_prep_cmnd
Otherwise the entire kernel including scsi-ml and all user applications
request 96 bytes of sense, always.

So there is nothing the SCSI core or application can do about it. 
It is USB fix time I'm afraid.

> Matt
> 

The SAT layer will have to override transport.c/usb_stor_invoke_transport
somehow and make do for bigger REQUEST_SENSE. Or a sense_size param per host,
or per command.

Boaz

Re: [usb-storage] USB storage devices and SAT

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

On Mon, Aug 04, 2008 at 11:31:58AM +0300, Boaz Harrosh wrote:
> Matthew Dharm wrote:
> > If you've got an app that is sending a command, and you KNOW that command
> > should produce >18 bytes of sense data, then there should be a way to
> > specify to the SCSI core (and thus get passed to usb-storage) that sense
> > data of >18 bytes should be requested.
> 
> What?!! The number 18 comes totally from USB land at:
> drivers/usb/storage/transport.c:601 via call to scsi_eh_prep_cmnd
> Otherwise the entire kernel including scsi-ml and all user applications
> request 96 bytes of sense, always.

Well, yes.  I think the value 96 must be (relatively) new... I think the
value 18 originally came about by copying it from somewhere else, since
it's not passed as a parameter from the scsi-ml and needed to be coded
directly into usb-storage to support auto-sense.

Perhaps you mis-interpreted my use of "should"?  I mean "we should add a
way" (not "a way should already exist").

Supporting auto-sense was a requirement since there are transport/protocol
combinations where it's not really optional.  And, once issued
REQUEST_SENSE will clear the sense data, so it we needed to keep the data
to put into the auto-sense buffer at those times when sense data needed to
be passed to the scsi-ml.

> So there is nothing the SCSI core or application can do about it. 
> It is USB fix time I'm afraid.

Right now, there is nothing that SCSI core or an app can do about it.  What
I'm suggesting is that there be a parameter (in the scsi_host structure,
perhaps?) that allows the core or an app to control this.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I'm a pink gumdrop! How can anything be worse?!!
					-- Erwin
User Friendly, 10/4/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: USB storage devices and SAT

[Alan Stern]

On Mon, 4 Aug 2008, Douglas Gilbert wrote:

> USB storage devices that support SAT (the T10 SCSI to ATA
> translation standard) are beginning to appear.
> 
> SAT enables tools like smartmontools to access SMART data
> on a ATA disk in a USB enclosure. We have run into
> a problem. It seems that the usb storage subsystem is wedded
> to the idea of sense data that is no longer than 18 bytes. **
> That doesn't play well with SAT which uses descriptor format
> sense data that is made up of an 8 byte header plus zero or
> more descriptors. SAT uses a 14 byte "ATA return" (sense)
> descriptor to yield the ATA registers. That means the sense
> data length is 22 bytes when an ATA return descriptor is
> required.
> 
> Alan Stern has already noted to another smartmontools developer
> that such a change is likely to break some USB storage devices.
> Perhaps the maximum sense buffer size could be optionally
> specified per usb storage device. Alternatively the usb mass
> storage logic could make some dynamic decisions itself.
> For example if the (disk) device responds successfully to either
> SCSI ATA PASS_THROUGH (12 or 16 byte) command then it will
> be capable of (at least) 22 byte sense data. A T10 vendor
> identification field in a standard INQUIRY response of
> "ATA     " is another indication of a device that supports
> SAT.

If either of these techniques could be used for dynamically detecting
when a device can reliably support 22-byte REQUEST SENSE, they would be
okay.

I tend to agree with Matt that it would be best to have the higher
layers tell usb-storage exactly how much sense data they want to get.
The problem is that these higher layers would not know about the 
18-byte restriction on many earlier USB devices, so usb-storage would 
probably end up needing to make its own dynamic decisions anyway.

Alan Stern

Re: [usb-storage] USB storage devices and SAT

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On Mon, Aug 04, 2008 at 11:51:57AM -0400, Alan Stern wrote:
> I tend to agree with Matt that it would be best to have the higher
> layers tell usb-storage exactly how much sense data they want to get.
> The problem is that these higher layers would not know about the 
> 18-byte restriction on many earlier USB devices, so usb-storage would 
> probably end up needing to make its own dynamic decisions anyway.

Why not handle this like other fields in the struct scsi_device?  Let the
core initialize it with a value, then override it in the slave_configure()
routine via unusual_devs.h flag OR via userspace command to the SCSI core?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Sir, for the hundreth time, we do NOT carry 600-round boxes of belt-fed 
suction darts!
					-- Salesperson to Greg
User Friendly, 12/30/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [usb-storage] USB storage devices and SAT

[Boaz Harrosh]

Matthew Dharm wrote:
> On Mon, Aug 04, 2008 at 11:51:57AM -0400, Alan Stern wrote:
>> I tend to agree with Matt that it would be best to have the higher
>> layers tell usb-storage exactly how much sense data they want to get.
>> The problem is that these higher layers would not know about the 
>> 18-byte restriction on many earlier USB devices, so usb-storage would 
>> probably end up needing to make its own dynamic decisions anyway.
> 
> Why not handle this like other fields in the struct scsi_device?  Let the
> core initialize it with a value, then override it in the slave_configure()
> routine via unusual_devs.h flag OR via userspace command to the SCSI core?
> 
> Matt
> 

There was a correct and simple patch proposed that fixes this problem
the right way:
http://marc.info/?l=linux-usb&m=121762869915609&w=2

Doug could you please test this patch to see if it fixes your device?

scsi-core already gives drivers complete control on what sense_size to
fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
for slave_configure(), default value, and all that loop.

Matthieu castet <castet.matthieu@free.fr> wrote ...
> we have got report from 2 users that scsi sat (to do ata passthrough)  was not fully \
> working with some usb bridge (maxtor and western digital). Everything works except \
> the sense result was incomplete [1].
> 
> 
> After some investigation, they need a sense size different of 18 (at least 22). [2]
> 
> Because some devices can crash if the sense size is different of 18, in order to not \
> break these devices a flag that is set either by unusual entries, either for spc3 or \
> latter devices is used.
> 
> 
> Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>

Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>

> 
> 
> 
> [1]
> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5613
> 
> [2]
> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721
> Index: linux-2.6/drivers/usb/storage/scsiglue.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:24.000000000 +0200
> +++ linux-2.6/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:37.000000000 +0200
> @@ -170,6 +170,10 @@
>  		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
>  			sdev->guess_capacity = 1;
>  
> +		/* assume SPC3 or latter device support sense size different of 18 */
> +		if (sdev->scsi_level > SCSI_SPC_2)
> +			us->fflags |= US_FL_SANE_SENSE;
> +
>  		/* Some devices report a SCSI revision level above 2 but are
>  		 * unable to handle the REPORT LUNS command (for which
>  		 * support is mandatory at level 3).  Since we already have
> Index: linux-2.6/drivers/usb/storage/transport.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/storage/transport.c	2008-08-02 00:07:24.000000000 \
>                 +0200
> +++ linux-2.6/drivers/usb/storage/transport.c	2008-08-02 00:07:37.000000000 +0200
> @@ -595,10 +595,15 @@
>  	if (need_auto_sense) {
>  		int temp_result;
>  		struct scsi_eh_save ses;
> +		int sense_size = US_SENSE_SIZE;
> +
> +		/* device support and need bigger sense buffer */
> +		if (us->fflags & US_FL_SANE_SENSE)
> +			sense_size = ~0;
>  
>  		US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
>  
> -		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, US_SENSE_SIZE);
> +		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
>  
>  		/* FIXME: we must do the protocol translation here */
>  		if (us->subclass == US_SC_RBC || us->subclass == US_SC_SCSI ||
> Index: linux-2.6/drivers/usb/storage/unusual_devs.h
> ===================================================================
> --- linux-2.6.orig/drivers/usb/storage/unusual_devs.h	2008-08-02 00:07:24.000000000 \
>                 +0200
> +++ linux-2.6/drivers/usb/storage/unusual_devs.h	2008-08-02 00:07:37.000000000 +0200
> @@ -1791,6 +1791,18 @@
>  		US_SC_DEVICE, US_PR_DEVICE, NULL,
>  		US_FL_CAPACITY_HEURISTICS),
>  
> +UNUSUAL_DEV(  0x0d49, 0x7310, 0x0000, 0x9999,
> +		"Maxtor",
> +		"usb sata bridge",
> +		US_SC_DEVICE, US_PR_DEVICE, NULL,
> +		US_FL_SANE_SENSE ),
> +
> +UNUSUAL_DEV(  0x1058, 0x0704, 0x0000, 0x9999,
> +		"Western Digital",
> +		"External HDD",
> +		US_SC_DEVICE, US_PR_DEVICE, NULL,
> +		US_FL_SANE_SENSE ),
> +
>  /* Control/Bulk transport for all SubClass values */
>  USUAL_DEV(US_SC_RBC, US_PR_CB, USB_US_TYPE_STOR),
>  USUAL_DEV(US_SC_8020, US_PR_CB, USB_US_TYPE_STOR),
> Index: linux-2.6/include/linux/usb_usual.h
> ===================================================================
> --- linux-2.6.orig/include/linux/usb_usual.h	2008-08-02 00:07:24.000000000 +0200
> +++ linux-2.6/include/linux/usb_usual.h	2008-08-02 00:07:37.000000000 +0200
> @@ -52,7 +52,9 @@
>  	US_FLAG(MAX_SECTORS_MIN,0x00002000)			\
>  		/* Sets max_sectors to arch min */		\
>  	US_FLAG(BULK_IGNORE_TAG,0x00004000)			\
> -		/* Ignore tag mismatch in bulk operations */
> +		/* Ignore tag mismatch in bulk operations */	\
> +	US_FLAG(SANE_SENSE,0x00008000)			\
> +		/* Need a sense size different of 18 for some cmd (SAT) */
>  
>  
>  #define US_FLAG(name, value)	US_FL_##name = value ,
> 
> 

I recommend this patch. It does exactly what's needed with minimum risk
it is even maybe over protective, with the white list. Perhaps it should
be turned to a black list instead. The old broken devices been an extincting
exception.

Boaz

Re: [usb-storage] USB storage devices and SAT

[Alan Stern]

On Tue, 5 Aug 2008, Boaz Harrosh wrote:

> There was a correct and simple patch proposed that fixes this problem
> the right way:
> http://marc.info/?l=linux-usb&m=121762869915609&w=2
> 
> Doug could you please test this patch to see if it fixes your device?
> 
> scsi-core already gives drivers complete control on what sense_size to
> fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
> for slave_configure(), default value, and all that loop.

> > http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721
> > Index: linux-2.6/drivers/usb/storage/scsiglue.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:24.000000000 +0200
> > +++ linux-2.6/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:37.000000000 +0200
> > @@ -170,6 +170,10 @@
> >  		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
> >  			sdev->guess_capacity = 1;
> >  
> > +		/* assume SPC3 or latter device support sense size different of 18 */
> > +		if (sdev->scsi_level > SCSI_SPC_2)
> > +			us->fflags |= US_FL_SANE_SENSE;
> > +
> >  		/* Some devices report a SCSI revision level above 2 but are
> >  		 * unable to handle the REPORT LUNS command (for which
> >  		 * support is mandatory at level 3).  Since we already have
> > Index: linux-2.6/drivers/usb/storage/transport.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/usb/storage/transport.c	2008-08-02 00:07:24.000000000 \
> >                 +0200
> > +++ linux-2.6/drivers/usb/storage/transport.c	2008-08-02 00:07:37.000000000 +0200
> > @@ -595,10 +595,15 @@
> >  	if (need_auto_sense) {
> >  		int temp_result;
> >  		struct scsi_eh_save ses;
> > +		int sense_size = US_SENSE_SIZE;
> > +
> > +		/* device support and need bigger sense buffer */
> > +		if (us->fflags & US_FL_SANE_SENSE)
> > +			sense_size = ~0;

This looks highly suspicious at best.  Shouldn't sense_size be set to a
real value, like 22 or 255?


> I recommend this patch. It does exactly what's needed with minimum risk
> it is even maybe over protective, with the white list. Perhaps it should
> be turned to a black list instead. The old broken devices been an extincting
> exception.

Changing it to a blacklist would be bad -- in fact it would be a 
regression, because all the deficient devices which used to work would 
stop working until they were identified and added to the blacklist.

Apart from the one nit above, I agree that this patch looks sensible.

Alan Stern

Re: [usb-storage] USB storage devices and SAT

[Boaz Harrosh]

Alan Stern wrote:
> On Tue, 5 Aug 2008, Boaz Harrosh wrote:
> 
>> There was a correct and simple patch proposed that fixes this problem
>> the right way:
>> http://marc.info/?l=linux-usb&m=121762869915609&w=2
>>
>> Doug could you please test this patch to see if it fixes your device?
>>
>> scsi-core already gives drivers complete control on what sense_size to
>> fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
>> for slave_configure(), default value, and all that loop.
> 
>>> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721
>>> Index: linux-2.6/drivers/usb/storage/scsiglue.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:24.000000000 +0200
>>> +++ linux-2.6/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:37.000000000 +0200
>>> @@ -170,6 +170,10 @@
>>>  		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
>>>  			sdev->guess_capacity = 1;
>>>  
>>> +		/* assume SPC3 or latter device support sense size different of 18 */
>>> +		if (sdev->scsi_level > SCSI_SPC_2)
>>> +			us->fflags |= US_FL_SANE_SENSE;
>>> +
>>>  		/* Some devices report a SCSI revision level above 2 but are
>>>  		 * unable to handle the REPORT LUNS command (for which
>>>  		 * support is mandatory at level 3).  Since we already have
>>> Index: linux-2.6/drivers/usb/storage/transport.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/usb/storage/transport.c	2008-08-02 00:07:24.000000000 \
>>>                 +0200
>>> +++ linux-2.6/drivers/usb/storage/transport.c	2008-08-02 00:07:37.000000000 +0200
>>> @@ -595,10 +595,15 @@
>>>  	if (need_auto_sense) {
>>>  		int temp_result;
>>>  		struct scsi_eh_save ses;
>>> +		int sense_size = US_SENSE_SIZE;
>>> +
>>> +		/* device support and need bigger sense buffer */
>>> +		if (us->fflags & US_FL_SANE_SENSE)
>>> +			sense_size = ~0;
> 
> This looks highly suspicious at best.  Shouldn't sense_size be set to a
> real value, like 22 or 255?
> 

No this is the regular way to signal from a driver that we support
whatever the scsi-ml or application decided. So ~0 means, effectively,
scsi's default which is 96. But can change in future with no changes
to drivers.
(BTW ~0 is used everywhere in Kernel but here)

Don't just hard code what ever you want. the maximum sense_size mandated
by scsi standard is 260. The current midlayer uses 96 every where. If
the driver has specific needs, like 18 with broken devices, do so but please
don't hard code to arbitrary value.

> 
>> I recommend this patch. It does exactly what's needed with minimum risk
>> it is even maybe over protective, with the white list. Perhaps it should
>> be turned to a black list instead. The old broken devices been an extincting
>> exception.
> 
> Changing it to a blacklist would be bad -- in fact it would be a 
> regression, because all the deficient devices which used to work would 
> stop working until they were identified and added to the blacklist.
> 
> Apart from the one nit above, I agree that this patch looks sensible.
> 
> Alan Stern
> 

Thanks
Boaz

Re: [usb-storage] USB storage devices and SAT

[Douglas Gilbert]

Alan Stern wrote:
> On Tue, 5 Aug 2008, Boaz Harrosh wrote:
> 
>> There was a correct and simple patch proposed that fixes this problem
>> the right way:
>> http://marc.info/?l=linux-usb&m=121762869915609&w=2
>>
>> Doug could you please test this patch to see if it fixes your device?
>>
>> scsi-core already gives drivers complete control on what sense_size to
>> fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
>> for slave_configure(), default value, and all that loop.
> 
>>> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721
>>> Index: linux-2.6/drivers/usb/storage/scsiglue.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:24.000000000 +0200
>>> +++ linux-2.6/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:37.000000000 +0200
>>> @@ -170,6 +170,10 @@
>>>  		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
>>>  			sdev->guess_capacity = 1;
>>>  
>>> +		/* assume SPC3 or latter device support sense size different of 18 */
>>> +		if (sdev->scsi_level > SCSI_SPC_2)
>>> +			us->fflags |= US_FL_SANE_SENSE;
>>> +
>>>  		/* Some devices report a SCSI revision level above 2 but are
>>>  		 * unable to handle the REPORT LUNS command (for which
>>>  		 * support is mandatory at level 3).  Since we already have
>>> Index: linux-2.6/drivers/usb/storage/transport.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/usb/storage/transport.c	2008-08-02 00:07:24.000000000 \
>>>                 +0200
>>> +++ linux-2.6/drivers/usb/storage/transport.c	2008-08-02 00:07:37.000000000 +0200
>>> @@ -595,10 +595,15 @@
>>>  	if (need_auto_sense) {
>>>  		int temp_result;
>>>  		struct scsi_eh_save ses;
>>> +		int sense_size = US_SENSE_SIZE;
>>> +
>>> +		/* device support and need bigger sense buffer */
>>> +		if (us->fflags & US_FL_SANE_SENSE)
>>> +			sense_size = ~0;
> 
> This looks highly suspicious at best.  Shouldn't sense_size be set to a
> real value, like 22 or 255?

The "correct" maximum value (SPC-3 and draft SPC-4) is 252.
Since SPC-3 the recommended maximum length for the basic
SCSI commands that have a 1 byte allocation length field was
altered from 255 to 252. This is to be a little friendlier
to transports that move data in 4 byte units across their
transport. Guessing a bit here but SATA, SAS and FCP fall
into that group of transports.

At some stage the allocation field length of an INQUIRY
command was changed from 1 to 2 bytes. So to pick up
VPD pages on older devices an INQUIRY's maximum allocation
length of 252 may be prudent. For example, choosing a value
of 260 (0x104) may give a surprising result if the device
only expects a 1 byte allocation length field.

>> I recommend this patch. It does exactly what's needed with minimum risk
>> it is even maybe over protective, with the white list. Perhaps it should
>> be turned to a black list instead. The old broken devices been an extincting
>> exception.
> 
> Changing it to a blacklist would be bad -- in fact it would be a 
> regression, because all the deficient devices which used to work would 
> stop working until they were identified and added to the blacklist.
> 
> Apart from the one nit above, I agree that this patch looks sensible.

Boaz,
I didn't test the above patch (as I don't have the external
USB devices that need it) but a gentleman who did, tells me
that he used a very similar patch and it solved his problem.

Doug Gilbert

Re: [usb-storage] USB storage devices and SAT

[Boaz Harrosh]

Douglas Gilbert wrote:
> Alan Stern wrote:
>> On Tue, 5 Aug 2008, Boaz Harrosh wrote:
>>
>>> There was a correct and simple patch proposed that fixes this problem
>>> the right way:
>>> http://marc.info/?l=linux-usb&m=121762869915609&w=2
>>>
>>> Doug could you please test this patch to see if it fixes your device?
>>>
>>> scsi-core already gives drivers complete control on what sense_size to
>>> fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
>>> for slave_configure(), default value, and all that loop.
>>>> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721
>>>> Index: linux-2.6/drivers/usb/storage/scsiglue.c
<snip>
>> This looks highly suspicious at best.  Shouldn't sense_size be set to a
>> real value, like 22 or 255?
> 
> The "correct" maximum value (SPC-3 and draft SPC-4) is 252.
> Since SPC-3 the recommended maximum length for the basic
> SCSI commands that have a 1 byte allocation length field was
> altered from 255 to 252. This is to be a little friendlier
> to transports that move data in 4 byte units across their
> transport. Guessing a bit here but SATA, SAS and FCP fall
> into that group of transports.

252 + 8 bytes header for REQUEST_SENSE command. So total
260 buffer size. (Last I look)

> At some stage the allocation field length of an INQUIRY
> command was changed from 1 to 2 bytes. So to pick up
> VPD pages on older devices an INQUIRY's maximum allocation
> length of 252 may be prudent. For example, choosing a value
> of 260 (0x104) may give a surprising result if the device
> only expects a 1 byte allocation length field.
> 

INQUIRY and VPD pages are different. From what I remember Vendor
VPD pages where always be16 length field.

You and the scsi code keep talking about "1 byte allocation length field". 
But the standard talks about be16 values. For me they are not the same.
 
>>> I recommend this patch. It does exactly what's needed with minimum risk
>>> it is even maybe over protective, with the white list. Perhaps it should
>>> be turned to a black list instead. The old broken devices been an extincting
>>> exception.
>> Changing it to a blacklist would be bad -- in fact it would be a 
>> regression, because all the deficient devices which used to work would 
>> stop working until they were identified and added to the blacklist.
>>
>> Apart from the one nit above, I agree that this patch looks sensible.
> 
> Boaz,
> I didn't test the above patch (as I don't have the external
> USB devices that need it) but a gentleman who did, tells me
> that he used a very similar patch and it solved his problem.
> 

Do you know if he used the white-list or if his device returned
SPC-3 compliance. The reason I ask is because all the devices I have
here from usb sticks to external boxes and converters all work with
96 bytes sense but all report spc-2

> Doug Gilbert
> 

Thanks
Boaz

Re: [usb-storage] USB storage devices and SAT

[Boaz Harrosh]

Boaz Harrosh wrote:
> Douglas Gilbert wrote:
>> The "correct" maximum value (SPC-3 and draft SPC-4) is 252.
>> Since SPC-3 the recommended maximum length for the basic
>> SCSI commands that have a 1 byte allocation length field was
>> altered from 255 to 252. This is to be a little friendlier
>> to transports that move data in 4 byte units across their
>> transport. Guessing a bit here but SATA, SAS and FCP fall
>> into that group of transports.
> 
> 252 + 8 bytes header for REQUEST_SENSE command. So total
> 260 buffer size. (Last I look)
> 

OK you are right 252 allocation length max, specified at
REQUEST_SENSE CDB. The sense_buffer structure itself has 
252 + 8 restriction which got me confused. In OSD we
have large sense payload that can get truncated by these
values.

Boaz

Re: [usb-storage] USB storage devices and SAT

[matthieu castet]

Hi,

Boaz Harrosh wrote:
> Douglas Gilbert wrote:
> 
> Do you know if he used the white-list or if his device returned
> SPC-3 compliance. The reason I ask is because all the devices I have
> here from usb sticks to external boxes and converters all work with
> 96 bytes sense but all report spc-2
>
He used the white-list. At the moment I don't know if some device report 
at spc-3, but it could be a easy way to white-list them.


Matthieu

Re: [usb-storage] USB storage devices and SAT

[matthieu castet]

Hi,

Boaz Harrosh wrote:
> 
> There was a correct and simple patch proposed that fixes this problem
> the right way:
> http://marc.info/?l=linux-usb&m=121762869915609&w=2
> 
> Doug could you please test this patch to see if it fixes your device?
> 
> scsi-core already gives drivers complete control on what sense_size to
> fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
> for slave_configure(), default value, and all that loop.
> 
> Matthieu castet <castet.matthieu@free.fr> wrote ...
>> we have got report from 2 users that scsi sat (to do ata passthrough)  was not fully \
>> working with some usb bridge (maxtor and western digital). Everything works except \
>> the sense result was incomplete [1].
>>
>>
>> After some investigation, they need a sense size different of 18 (at least 22). [2]
>>
>> Because some devices can crash if the sense size is different of 18, in order to not \
>> break these devices a flag that is set either by unusual entries, either for spc3 or \
>> latter devices is used.
>>
>>
>> Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
> 
> Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>
> 
What the status of that.

Should I try to resubmit that patch on linux-usb ?


>>
>>
>> [1]
>> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5613
>>
>> [2]
>> http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721
>> Index: linux-2.6/drivers/usb/storage/scsiglue.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:24.000000000 +0200
>> +++ linux-2.6/drivers/usb/storage/scsiglue.c	2008-08-02 00:07:37.000000000 +0200
>> @@ -170,6 +170,10 @@
>>  		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
>>  			sdev->guess_capacity = 1;
>>  
>> +		/* assume SPC3 or latter device support sense size different of 18 */
>> +		if (sdev->scsi_level > SCSI_SPC_2)
>> +			us->fflags |= US_FL_SANE_SENSE;
>> +
>>  		/* Some devices report a SCSI revision level above 2 but are
>>  		 * unable to handle the REPORT LUNS command (for which
>>  		 * support is mandatory at level 3).  Since we already have
>> Index: linux-2.6/drivers/usb/storage/transport.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/usb/storage/transport.c	2008-08-02 00:07:24.000000000 \
>>                 +0200
>> +++ linux-2.6/drivers/usb/storage/transport.c	2008-08-02 00:07:37.000000000 +0200
>> @@ -595,10 +595,15 @@
>>  	if (need_auto_sense) {
>>  		int temp_result;
>>  		struct scsi_eh_save ses;
>> +		int sense_size = US_SENSE_SIZE;
>> +
>> +		/* device support and need bigger sense buffer */
>> +		if (us->fflags & US_FL_SANE_SENSE)
>> +			sense_size = ~0;
>>  
>>  		US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
>>  
>> -		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, US_SENSE_SIZE);
>> +		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
>>  
>>  		/* FIXME: we must do the protocol translation here */
>>  		if (us->subclass == US_SC_RBC || us->subclass == US_SC_SCSI ||
>> Index: linux-2.6/drivers/usb/storage/unusual_devs.h
>> ===================================================================
>> --- linux-2.6.orig/drivers/usb/storage/unusual_devs.h	2008-08-02 00:07:24.000000000 \
>>                 +0200
>> +++ linux-2.6/drivers/usb/storage/unusual_devs.h	2008-08-02 00:07:37.000000000 +0200
>> @@ -1791,6 +1791,18 @@
>>  		US_SC_DEVICE, US_PR_DEVICE, NULL,
>>  		US_FL_CAPACITY_HEURISTICS),
>>  
>> +UNUSUAL_DEV(  0x0d49, 0x7310, 0x0000, 0x9999,
>> +		"Maxtor",
>> +		"usb sata bridge",
>> +		US_SC_DEVICE, US_PR_DEVICE, NULL,
>> +		US_FL_SANE_SENSE ),
>> +
>> +UNUSUAL_DEV(  0x1058, 0x0704, 0x0000, 0x9999,
>> +		"Western Digital",
>> +		"External HDD",
>> +		US_SC_DEVICE, US_PR_DEVICE, NULL,
>> +		US_FL_SANE_SENSE ),
>> +
>>  /* Control/Bulk transport for all SubClass values */
>>  USUAL_DEV(US_SC_RBC, US_PR_CB, USB_US_TYPE_STOR),
>>  USUAL_DEV(US_SC_8020, US_PR_CB, USB_US_TYPE_STOR),
>> Index: linux-2.6/include/linux/usb_usual.h
>> ===================================================================
>> --- linux-2.6.orig/include/linux/usb_usual.h	2008-08-02 00:07:24.000000000 +0200
>> +++ linux-2.6/include/linux/usb_usual.h	2008-08-02 00:07:37.000000000 +0200
>> @@ -52,7 +52,9 @@
>>  	US_FLAG(MAX_SECTORS_MIN,0x00002000)			\
>>  		/* Sets max_sectors to arch min */		\
>>  	US_FLAG(BULK_IGNORE_TAG,0x00004000)			\
>> -		/* Ignore tag mismatch in bulk operations */
>> +		/* Ignore tag mismatch in bulk operations */	\
>> +	US_FLAG(SANE_SENSE,0x00008000)			\
>> +		/* Need a sense size different of 18 for some cmd (SAT) */
>>  
>>  
>>  #define US_FLAG(name, value)	US_FL_##name = value ,
>>
>>
> 
> I recommend this patch. It does exactly what's needed with minimum risk
> it is even maybe over protective, with the white list. Perhaps it should
> be turned to a black list instead. The old broken devices been an extincting
> exception.
> 
> Boaz
> 
> 
> 
> 

Re: [usb-storage] USB storage devices and SAT

[Boaz Harrosh]

matthieu castet wrote:
> Hi,
> 
> Boaz Harrosh wrote:
>> There was a correct and simple patch proposed that fixes this problem
>> the right way:
>> http://marc.info/?l=linux-usb&m=121762869915609&w=2
>>
>> Doug could you please test this patch to see if it fixes your device?
>>
>> scsi-core already gives drivers complete control on what sense_size to
>> fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need
>> for slave_configure(), default value, and all that loop.
>>
>> Matthieu castet <castet.matthieu@free.fr> wrote ...
>>> we have got report from 2 users that scsi sat (to do ata passthrough)  was not fully \
>>> working with some usb bridge (maxtor and western digital). Everything works except \
>>> the sense result was incomplete [1].
>>>
>>>
>>> After some investigation, they need a sense size different of 18 (at least 22). [2]
>>>
>>> Because some devices can crash if the sense size is different of 18, in order to not \
>>> break these devices a flag that is set either by unusual entries, either for spc3 or \
>>> latter devices is used.
>>>
>>>
>>> Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
>> Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>
>>
> What the status of that.
> 
> Should I try to resubmit that patch on linux-usb ?
> 

I see the To: is me. But I'm not a USB guy at all.

Like you said, I too think the best is to freshen up the 
patch for latest USB tree and send it to the USB mailing
list.

Cheers
Boaz

Re: [usb-storage] USB storage devices and SAT

[castet.matthieu]

Hi,

Matthew Dharm wrote:
> Personally, I hate having to make dynamic decisions in usb-storage.  The
> more we try to do there, the more likely we are to get it wrong.
>
Did you see the patch I proposed at
http://marc.info/?l=linux-usb&m=121762869915609&w=2 ?

The idea is to keep a list of whitelist of device that support SAT.
The problem is we should add each new devices to this list (or hope vendor will
report SPC3 device).


Matthieu