[PATCH] uas: Add a new NO_REPORT_LUNS quirk

[PATCH] uas: Add a new NO_REPORT_LUNS quirk

[Hans de Goede]

Add a new NO_REPORT_LUNS quirk and set it for Seagate drives with
an usb-id of: 0bc2:331a, as these will fail to respond to a
REPORT_LUNS command.

Cc: stable@vger.kernel.org
Reported-and-tested-by: David Webb <djw@noc.ac.uk>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c         | 14 +++++++++++++-
 drivers/usb/storage/unusual_uas.h |  7 +++++++
 drivers/usb/storage/usb.c         |  5 ++++-
 include/linux/usb_usual.h         |  2 ++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index c68e724147..5e15ed03 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -2,7 +2,7 @@
  * USB Attached SCSI
  * Note that this is not the same as the USB Mass Storage driver
  *
- * Copyright Hans de Goede <hdegoede@redhat.com> for Red Hat, Inc. 2013 - 2014
+ * Copyright Hans de Goede <hdegoede@redhat.com> for Red Hat, Inc. 2013 - 2016
  * Copyright Matthew Wilcox for Intel Corp, 2010
  * Copyright Sarah Sharp for Intel Corp, 2010
  *
@@ -781,6 +781,17 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 	return SUCCESS;
 }
 
+static int uas_target_alloc(struct scsi_target *starget)
+{
+	struct uas_dev_info *devinfo = (struct uas_dev_info *)
+			dev_to_shost(starget->dev.parent)->hostdata;
+
+	if (devinfo->flags & US_FL_NO_REPORT_LUNS)
+		starget->no_report_luns = 1;
+
+	return 0;
+}
+
 static int uas_slave_alloc(struct scsi_device *sdev)
 {
 	struct uas_dev_info *devinfo =
@@ -831,6 +842,7 @@ static struct scsi_host_template uas_host_template = {
 	.module = THIS_MODULE,
 	.name = "uas",
 	.queuecommand = uas_queuecommand,
+	.target_alloc = uas_target_alloc,
 	.slave_alloc = uas_slave_alloc,
 	.slave_configure = uas_slave_configure,
 	.eh_abort_handler = uas_eh_abort_handler,
diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
index ccc113e..53341a7 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -64,6 +64,13 @@ UNUSUAL_DEV(0x0bc2, 0x3312, 0x0000, 0x9999,
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
 		US_FL_NO_ATA_1X),
 
+/* Reported-by: David Webb <djw@noc.ac.uk> */
+UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999,
+		"Seagate",
+		"Expansion Desk",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_NO_REPORT_LUNS),
+
 /* Reported-by: Hans de Goede <hdegoede@redhat.com> */
 UNUSUAL_DEV(0x0bc2, 0x3320, 0x0000, 0x9999,
 		"Seagate",
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 43576ed..9de988a 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -482,7 +482,7 @@ void usb_stor_adjust_quirks(struct usb_device *udev, unsigned long *fflags)
 			US_FL_NO_READ_DISC_INFO | US_FL_NO_READ_CAPACITY_16 |
 			US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
 			US_FL_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
-			US_FL_MAX_SECTORS_240);
+			US_FL_MAX_SECTORS_240 | US_FL_NO_REPORT_LUNS);
 
 	p = quirks;
 	while (*p) {
@@ -532,6 +532,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, unsigned long *fflags)
 		case 'i':
 			f |= US_FL_IGNORE_DEVICE;
 			break;
+		case 'j':
+			f |= US_FL_NO_REPORT_LUNS;
+			break;
 		case 'l':
 			f |= US_FL_NOT_LOCKABLE;
 			break;
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index 7f5f78b..245f57d 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -79,6 +79,8 @@
 		/* Cannot handle MI_REPORT_SUPPORTED_OPERATION_CODES */	\
 	US_FLAG(MAX_SECTORS_240,	0x08000000)		\
 		/* Sets max_sectors to 240 */			\
+	US_FLAG(NO_REPORT_LUNS,	0x10000000)			\
+		/* Cannot handle REPORT_LUNS */			\
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.7.3

Re: [PATCH] uas: Add a new NO_REPORT_LUNS quirk

[Alan Stern]

On Thu, 31 Mar 2016, Hans de Goede wrote:

> Add a new NO_REPORT_LUNS quirk and set it for Seagate drives with
> an usb-id of: 0bc2:331a, as these will fail to respond to a
> REPORT_LUNS command.

> @@ -532,6 +532,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, unsigned long *fflags)
>  		case 'i':
>  			f |= US_FL_IGNORE_DEVICE;
>  			break;
> +		case 'j':
> +			f |= US_FL_NO_REPORT_LUNS;
> +			break;
>  		case 'l':
>  			f |= US_FL_NOT_LOCKABLE;
>  			break;

You forgot to document this new module parameter flag in 
Documentation/kernel-parameters.txt.

Alan Stern

Re: [PATCH] uas: Add a new NO_REPORT_LUNS quirk

[James Bottomley]

On Thu, 2016-03-31 at 14:22 +0200, Hans de Goede wrote:
> Add a new NO_REPORT_LUNS quirk and set it for Seagate drives with
> an usb-id of: 0bc2:331a, as these will fail to respond to a
> REPORT_LUNS command.

Actually, if we're sending them a report luns command, they must be
reporting in at SCSI-3 SPC or higher.  Should we be quirking them down
to SCSI-2 instead because it reduces the risk of running into something
else they're not doing from the SPC command set?

James

Re: [PATCH] uas: Add a new NO_REPORT_LUNS quirk

[Hans de Goede]

Hi,

On 31-03-16 16:48, James Bottomley wrote:
> On Thu, 2016-03-31 at 14:22 +0200, Hans de Goede wrote:
>> Add a new NO_REPORT_LUNS quirk and set it for Seagate drives with
>> an usb-id of: 0bc2:331a, as these will fail to respond to a
>> REPORT_LUNS command.
>
> Actually, if we're sending them a report luns command, they must be
> reporting in at SCSI-3 SPC or higher.  Should we be quirking them down
> to SCSI-2 instead because it reduces the risk of running into something
> else they're not doing from the SPC command set?

These are fairly new devices, so they should really be scsi3, but the
usb <-> sata bridge (presumably) used does not seem to like report_luns.

Note that usb-storage simple sets no_report_luns conditionally for all
usb-storage devices. The scsi people have repeatedly asked me to not
do this kinda blanket blacklisting for uas devices, because they hope
that uas will allow them to more or less do proper scsi over usb, so
we end up with blacklisting specific commands every now and then to
get devices to work.

Regards,

Hans

Re: [PATCH] uas: Add a new NO_REPORT_LUNS quirk

[James Bottomley]

On Thu, 2016-03-31 at 17:03 +0200, Hans de Goede wrote:
> Hi,
> 
> On 31-03-16 16:48, James Bottomley wrote:
> > On Thu, 2016-03-31 at 14:22 +0200, Hans de Goede wrote:
> > > Add a new NO_REPORT_LUNS quirk and set it for Seagate drives with
> > > an usb-id of: 0bc2:331a, as these will fail to respond to a
> > > REPORT_LUNS command.
> > 
> > Actually, if we're sending them a report luns command, they must be
> > reporting in at SCSI-3 SPC or higher.  Should we be quirking them 
> > down to SCSI-2 instead because it reduces the risk of running into
> > something else they're not doing from the SPC command set?
> 
> These are fairly new devices, so they should really be scsi3, but the
> usb <-> sata bridge (presumably) used does not seem to like
> report_luns.

That's what I'm questioning: REPORT LUNS is one of the big SCSI-3
changes, if they don't support that, it really looks like someone
picked up an old engine and just fuzzed the inquiry data to return SCSI
-3.  In which case we should put it back to SCSI-2 where it belongs.

Also, if it's USB<->SCSI bridge, that isn't really UAS, is it?

> Note that usb-storage simple sets no_report_luns conditionally for 
> all usb-storage devices. The scsi people have repeatedly asked me to 
> not do this kinda blanket blacklisting for uas devices, because they 
> hope that uas will allow them to more or less do proper scsi over 
> usb, so we end up with blacklisting specific commands every now and 
> then to get devices to work.

Well, we were hoping that with UAS the USB device creators would
actually learn what a standard was when it bit them, yes.  The fact
that Seagate can release a SCSI-3 UAS device that doesn't do REPORT
LUNS kind of dashes that hope.

James

Re: [PATCH] uas: Add a new NO_REPORT_LUNS quirk

[Hans de Goede]

Hi,

On 31-03-16 17:11, James Bottomley wrote:
> On Thu, 2016-03-31 at 17:03 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 31-03-16 16:48, James Bottomley wrote:
>>> On Thu, 2016-03-31 at 14:22 +0200, Hans de Goede wrote:
>>>> Add a new NO_REPORT_LUNS quirk and set it for Seagate drives with
>>>> an usb-id of: 0bc2:331a, as these will fail to respond to a
>>>> REPORT_LUNS command.
>>>
>>> Actually, if we're sending them a report luns command, they must be
>>> reporting in at SCSI-3 SPC or higher.  Should we be quirking them
>>> down to SCSI-2 instead because it reduces the risk of running into
>>> something else they're not doing from the SPC command set?
>>
>> These are fairly new devices, so they should really be scsi3, but the
>> usb <-> sata bridge (presumably) used does not seem to like
>> report_luns.
>
> That's what I'm questioning: REPORT LUNS is one of the big SCSI-3
> changes, if they don't support that, it really looks like someone
> picked up an old engine and just fuzzed the inquiry data to return SCSI
> -3.  In which case we should put it back to SCSI-2 where it belongs.

Actually it does support REPORT LUNS, some of the time. When you first
boot the computer with uas blacklisted for this device, so initialize
it once with usb-storage, and then reboot with out the blacklist
(and without removing power to the drive) uas will work with REPORT LUNS
bit cold-booting directly into uas mode and then doing a REPORT LUNS
upsets the drive / disk enclosure (this has all been observed by
David Webb, I do not own such a drive).

> Also, if it's USB<->SCSI bridge, that isn't really UAS, is it?

I assume you mean that a USB<->sata bridge is not really a SCSI device
but more of a scsi emulating device. I'm not going to argue that, but
all devices talking the uas protocol I've seen sofar are USB<->sata
bridges.

>> Note that usb-storage simple sets no_report_luns conditionally for
>> all usb-storage devices. The scsi people have repeatedly asked me to
>> not do this kinda blanket blacklisting for uas devices, because they
>> hope that uas will allow them to more or less do proper scsi over
>> usb, so we end up with blacklisting specific commands every now and
>> then to get devices to work.
>
> Well, we were hoping that with UAS the USB device creators would
> actually learn what a standard was when it bit them, yes.  The fact
> that Seagate can release a SCSI-3 UAS device that doesn't do REPORT
> LUNS kind of dashes that hope.

See above it does sortof do REPORT LUNS just not reliable (and thus
not usable). Also for some reason Seagate seems to be particularly
bad in their uas implementation, we have a ton of quirks for Seagate
uas devices in drivers/usb/storage/unusual_uas.h, so far all of them
stop responding until reset after ATA_12 or ATA_16 CDBs so we filter
those out. This REPORT LUNS issue is new.

Regards,

Hans

Re: [PATCH] uas: Add a new NO_REPORT_LUNS quirk

[David Webb]

Hi,

> Actually it does support REPORT LUNS, some of the time. When you first
> boot the computer with uas blacklisted for this device, so initialize
> it once with usb-storage, and then reboot with out the blacklist
> (and without removing power to the drive) uas will work with REPORT LUNS
> bit cold-booting directly into uas mode and then doing a REPORT LUNS
> upsets the drive / disk enclosure (this has all been observed by
> David Webb, I do not own such a drive).

Just to confirm what Hans has reported.  After power has been removed the 
Seagate Expansion usb disk always produces faults unless it has been 
blacklisted in some way.  Once the disk is working the computer can be powered 
off and restarted without the blacklist and, as long as its power has not been 
removed, the disk can be reconnected many times without any error.

With Hans's changes the disk mounts correctly with the uas module every time.

My guess is that one of the interface registers is not or cannot be 
initialized correctly.  If after a failure I try unplugging and reinserting 
the usb connector many times it has sometimes connected correctly - which to 
me means that some random bit eventually has the right value. 

Regards,

David Webb.