aacraid code passes GFP_DMA32 to kmalloc this will not work

[Hans de Goede]

Hi All,

Since I made the same mistake myself I've done a quick grep for
GFP_DMA32 in the kernel and drivers/scsi/aacraid/commctrl.c
came up as a result of this grep, it does:

p = kmalloc(sg_count[i], GFP_KERNEL|GFP_DMA32);

But kmalloc always returns memory from the normal memory-zone,
if you need memory from a specific memory-zone like the
DMA32 zone, you must use the dma allocation functions (which
from a quick glance at the code seems appropriate here) or
directly call alloc_page or __get_free_page.

Regards,

Hans

Re: [PATCH] uas: Unblock scsi-requests on failure to alloc streams in post_reset

[Hans de Goede]

Hi,

On 10-01-18 16:23, Oliver Neukum wrote:

Am Mittwoch, den 10.01.2018, 08:13 +0100 schrieb Hans de Goede:

If we return 1 from our post_reset handler, then our disconnect handler
will be called immediately afterwards. Since pre_reset blocks all scsi
requests our disconnect handler will then hang in the scsi_remove_host
call.


Hi Hans,

it seems to me that the diagnosis is spot on. But why do we
keep different code paths at all in this case? I do not see
the point of not reporting the reset to the SCSI subsystem,
even if we are not operational afterwards.
So how about something like this?


Sure, works for me :)

Regards,

Hans




 From 4d1e26154bc5d09913bfba34d7adc39cce98d20a Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneu...@suse.com>
Date: Wed, 10 Jan 2018 16:16:03 +0100
Subject: [PATCH] usb: uas: unconditionally bring back host after reset

Quoting Hans:

If we return 1 from our post_reset handler, then our disconnect handler
will be called immediately afterwards. Since pre_reset blocks all scsi
requests our disconnect handler will then hang in the scsi_remove_host
call.

This is esp. bad because our disconnect handler hanging for ever also
stops the USB subsys from enumerating any new USB devices, causes
commands
like lsusb to hang, etc.

In practice this happens when unplugging some uas devices because the
hub
code may see the device as needing a warm-reset and calls
usb_reset_device
before seeing the disconnect. In this case uas_configure_endpoints
fails
with -ENODEV. We do not want to print an error for this, so this commit
also silences the shost_printk for -ENODEV.

ENDQUOTE

However, if we do that we better drop any unconditional execution
and report to the SCSI subsystem that we have undergone a reset
but we are not operational now.

Signed-off-by: Oliver Neukum <oneu...@suse.com>
Reported-by: Hans de Goede <hdego...@redhat.com>
---
  Makefile  | 2 +-
  drivers/usb/storage/uas.c | 7 +++
  2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 5d04c40ee40a..3b1b9695177a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -1076,20 +1076,19 @@ static int uas_post_reset(struct usb_interface
*intf)
    return 0;
  
  	err = uas_configure_endpoints(devinfo);

-   if (err) {
+   if (err && err != ENODEV)
    shost_printk(KERN_ERR, shost,
     "%s: alloc streams error %d after reset",
     __func__, err);
-   return 1;
-   }
  
+	/* we must unblock the host in every case lest we deadlock */

    spin_lock_irqsave(shost->host_lock, flags);
    scsi_report_bus_reset(shost, 0);
    spin_unlock_irqrestore(shost->host_lock, flags);
  
  	scsi_unblock_requests(shost);
  
-	return 0;

+   return err ? 1 : 0;
  }
  
  static int uas_suspend(struct usb_interface *intf, pm_message_t

message)
--
2.13.6

[PATCH] uas: Unblock scsi-requests on failure to alloc streams in post_reset

[Hans de Goede]

If we return 1 from our post_reset handler, then our disconnect handler
will be called immediately afterwards. Since pre_reset blocks all scsi
requests our disconnect handler will then hang in the scsi_remove_host
call.

This is esp. bad because our disconnect handler hanging for ever also
stops the USB subsys from enumerating any new USB devices, causes commands
like lsusb to hang, etc.

In practice this happens when unplugging some uas devices because the hub
code may see the device as needing a warm-reset and calls usb_reset_device
before seeing the disconnect. In this case uas_configure_endpoints fails
with -ENODEV. We do not want to print an error for this, so this commit
also silences the shost_printk for -ENODEV.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1531966
Cc: sta...@vger.kernel.org
Fixes: 8d51444cdd06 ("uas: Not being able to alloc streams ... is an error")
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 5d04c40ee40a..5471422aa1ab 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -1077,9 +1077,13 @@ static int uas_post_reset(struct usb_interface *intf)
 
err = uas_configure_endpoints(devinfo);
if (err) {
-   shost_printk(KERN_ERR, shost,
-"%s: alloc streams error %d after reset",
-__func__, err);
+   if (err != -ENODEV) {
+   shost_printk(KERN_ERR, shost,
+"%s: alloc streams error %d after reset",
+__func__, err);
+   }
+   /* So that scsi_remove_host in uas_disconnect does not hang */
+   scsi_unblock_requests(shost);
return 1;
}
 
-- 
2.14.3

[PATCH] uas: Remove US_FL_NO_ATA_1X unusual device entries for Seagate devices

[Hans de Goede]

Since commit 7fee72d5e8f1 ("uas: Always apply US_FL_NO_ATA_1X quirk to
Seagate devices"), the US_FL_NO_ATA_1X is always set for Seagate devices,
so the per device unusual_uas.h entries for Seagate devices can be
removed.

Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/unusual_uas.h | 63 ---
 1 file changed, 63 deletions(-)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index d520374a824e..bf457b0f6a1f 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -38,20 +38,6 @@ UNUSUAL_DEV(0x0984, 0x0301, 0x0128, 0x0128,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_IGNORE_UAS),
 
-/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
-UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
-   "Seagate",
-   "Expansion Desk",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
-UNUSUAL_DEV(0x0bc2, 0x3312, 0x, 0x,
-   "Seagate",
-   "Expansion Desk",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
 /* Reported-by: David Webb <d...@noc.ac.uk> */
 UNUSUAL_DEV(0x0bc2, 0x331a, 0x, 0x,
"Seagate",
@@ -59,55 +45,6 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_REPORT_LUNS),
 
-/* Reported-by: Hans de Goede <hdego...@redhat.com> */
-UNUSUAL_DEV(0x0bc2, 0x3320, 0x, 0x,
-   "Seagate",
-   "Expansion Desk",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* Reported-by: Bogdan Mihalcea <bogdan.mihal...@infim.ro> */
-UNUSUAL_DEV(0x0bc2, 0xa003, 0x, 0x,
-   "Seagate",
-   "Backup Plus",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* Reported-by: Marcin Zajączkowski <msz...@wp.pl> */
-UNUSUAL_DEV(0x0bc2, 0xa013, 0x, 0x,
-   "Seagate",
-       "Backup Plus",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* Reported-by: Hans de Goede <hdego...@redhat.com> */
-UNUSUAL_DEV(0x0bc2, 0xa0a4, 0x, 0x,
-   "Seagate",
-   "Backup Plus Desk",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
-UNUSUAL_DEV(0x0bc2, 0xab20, 0x, 0x,
-   "Seagate",
-   "Backup+ BK",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
-UNUSUAL_DEV(0x0bc2, 0xab21, 0x, 0x,
-   "Seagate",
-   "Backup+ BK",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
-/* Reported-by: G. Richard Bellamy <rbell...@pteradigm.com> */
-UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
-   "Seagate",
-   "BUP Fast HDD",
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_NO_ATA_1X),
-
 /* Reported-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com> */
 UNUSUAL_DEV(0x13fd, 0x3940, 0x, 0x,
"Initio Corporation",
-- 
2.14.3

Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Hi,

On 15-11-17 10:06, Oliver Neukum wrote:

Am Dienstag, den 14.11.2017, 18:44 +0100 schrieb Hans de Goede:


Greg, please do no merge the 2 recent uas seagate quirks I send
then. I will submit a patch with the new approach right away.


Hi,

I am afraid in that case we will need a way to override a
quirk in the other direction, that is, to not apply it.


We already have that usb_stor_adjust_quirks when a match to the
device's vend:prod ids is found in usb_storage.quirks,
usb_stor_adjust_quirks will clear all quirks it allows to be set
through the usb_storage.quirks module option.

Regards,

Hans

[PATCH] uas: Always apply US_FL_NO_ATA_1X quirk to Seagate devices

[Hans de Goede]

We've been adding this as a quirk on a per device basis hoping that
newer disk enclosures would do better, but that has not happened,
so simply apply this quirk to all Seagate devices.

Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas-detect.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index a155cd02bce2..ecc83c405a8b 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -111,6 +111,10 @@ static int uas_use_uas_driver(struct usb_interface *intf,
}
}
 
+   /* All Seagate disk enclosures have broken ATA pass-through support */
+   if (le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2)
+   flags |= US_FL_NO_ATA_1X;
+
usb_stor_adjust_quirks(udev, );
 
if (flags & US_FL_IGNORE_UAS) {
-- 
2.14.3

Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Hi,

On 11/14/2017 04:25 PM, Alan Stern wrote:

On Tue, 14 Nov 2017, Hans de Goede wrote:


Hi,

On 14-11-17 15:00, Hans de Goede wrote:

Just like all previous UAS capable Seagate disk enclosures, this
one needs a US_FL_NO_ATA_1X quirk.

Cc: sta...@vger.kernel.org # 3.16
Reported-by: Wido <wido...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>



So far we've been adding quirks for Seagate drives on a one by one
basis (I started this myself) hoping that one day they will fix
their ATA_1X pass-through support. But that does not seem to
be happening.

Maybe we need to do something like this instead ?   :

--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -111,6 +111,10 @@ static int uas_use_uas_driver(struct usb_interface *intf,
  }
  }

+ /* All Seagate disk enclosures have broken ATA pass-through support */
+ if (le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2)
+ flags |= US_FL_NO_ATA_1X;
+
  usb_stor_adjust_quirks(udev, );

  if (flags & US_FL_IGNORE_UAS) {


Then we can remove a whole lot of quirks and we avoid future
churn when new Seagate device ids show up.


That is a reasonable approach.  For what it's worth, usb-storage has
had similar code for many years, affecting devices from Nokia, Nikon,
Pentax, and Motorola.



Ok.

Greg, please do no merge the 2 recent uas seagate quirks I send
then. I will submit a patch with the new approach right away.

Once the patch with the new approach is merged I will
submit a patch to remove all the seagate quirks.

Regards,

Hans

Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Hi,

On 14-11-17 15:00, Hans de Goede wrote:

Just like all previous UAS capable Seagate disk enclosures, this
one needs a US_FL_NO_ATA_1X quirk.

Cc: sta...@vger.kernel.org # 3.16
Reported-by: Wido <wido...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>



So far we've been adding quirks for Seagate drives on a one by one
basis (I started this myself) hoping that one day they will fix
their ATA_1X pass-through support. But that does not seem to
be happening.

Maybe we need to do something like this instead ?   :

--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -111,6 +111,10 @@ static int uas_use_uas_driver(struct usb_interface *intf,
}
}

+ /* All Seagate disk enclosures have broken ATA pass-through support */
+ if (le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2)
+ flags |= US_FL_NO_ATA_1X;
+
usb_stor_adjust_quirks(udev, );

if (flags & US_FL_IGNORE_UAS) {


Then we can remove a whole lot of quirks and we avoid future
churn when new Seagate device ids show up.

Regards,

Hans




---
  drivers/usb/storage/unusual_uas.h | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 2fe4fd336446..a1ddcbfb7323 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -114,6 +114,13 @@ UNUSUAL_DEV(0x0bc2, 0xab21, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
  
+/* Reported-by: Wido <wido...@gmail.com> */

+UNUSUAL_DEV(0x0bc2, 0xab24, 0x, 0x,
+   "Seagate",
+   "BUP Slim BK",
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
  /* Reported-by: G. Richard Bellamy <rbell...@pteradigm.com> */
  UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
"Seagate",

[PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Just like all previous UAS capable Seagate disk enclosures, this
one needs a US_FL_NO_ATA_1X quirk.

Cc: sta...@vger.kernel.org # 3.16
Reported-by: Wido <wido...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 2fe4fd336446..a1ddcbfb7323 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -114,6 +114,13 @@ UNUSUAL_DEV(0x0bc2, 0xab21, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Wido <wido...@gmail.com> */
+UNUSUAL_DEV(0x0bc2, 0xab24, 0x, 0x,
+   "Seagate",
+   "BUP Slim BK",
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* Reported-by: G. Richard Bellamy <rbell...@pteradigm.com> */
 UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
"Seagate",
-- 
2.14.3

Re: [PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Hi,

On 13-11-17 07:14, Jérôme Carretero wrote:

On Mon, 13 Nov 2017 07:01:30 +0300
Andrey Astafyev <1...@246060.ru> wrote:


13.11.2017 00:42, Jérôme Carretero пишет:

Nov 12 16:20:59 Bidule kernel: sd 22:0:0:0: [sdaa] tag#2
uas_eh_abort_handler 0 uas-tag 3 inflight: CMD OUT
[...]
Do you see such things?
  

Hi, I've seen dmesg output like this so I've added my device to
quirks list like any other Seagate USB drive.


Hi Andrey, Hans,


For my devices, adding US_FL_NO_ATA_1X to unusual_uas.h didn't
change anything, and while adding US_FL_IGNORE_UAS (using
quirks=0bc2:ab34:u,0bc2:ab38:u) there are still device resets,
but they cause shorter hangs in system activity (~1 second when
UAS was more like ~20).


The errors you are seeing are write errors. If you're seeing these
errors with both the usb-storage and uas drivers then there likely
is something wrong with your setup / hardware.

Does the drive in question use an external power-supply or is it
USB bus-powered? If it is the latter then that is likely the problem.

Anyways things I would check and try to swap are both the cable
used, the power-supply used (if any), the USB-port used as well
as trying the disk on a completely different computer.

I've the feeling something is busted with your hardware, it
could be the disk itself. Did you mention that this was the first
release of a new higher capacity ? Those often have some kinks
which are worked out in later revisions.

Regards,

Hans

[PATCH] uas: Add US_FL_NO_ATA_1X quirk for one more Seagate device

[Hans de Goede]

Just like all previous UAS capable Seagate disk enclosures, this
one needs a US_FL_NO_ATA_1X quirk.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Andrey Astafyev <1...@246060.ru>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index cde115359793..2fe4fd336446 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -121,6 +121,13 @@ UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Andrey Astafyev <d...@246060.ru> */
+UNUSUAL_DEV(0x0bc2, 0xab30, 0x, 0x,
+   "Seagate",
+   "BUP BK",
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* Reported-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com> */
 UNUSUAL_DEV(0x13fd, 0x3940, 0x, 0x,
"Initio Corporation",
-- 
2.14.3

Re: Race to power off harming SATA SSDs

[Hans de Goede]

Hi,

On 08-05-17 11:06, Ricard Wanderlof wrote:


On Mon, 8 May 2017, David Woodhouse wrote:


On Mon, 8 May 2017, David Woodhouse wrote:

Our empirical testing trumps your "can never happen" theory :)


I'm sure it does. But what is the explanation then? Has anyone analyzed
what is going on using an oscilloscope to verify relationship between
erase command and supply voltage drop?


Not that I'm aware of. Once we have reached the "it does happen and we
have to cope" there was not a lot of point in working out *why* it
happened.

In fact, the only examples I *personally* remember were on NOR flash,
which takes longer to erase. So it's vaguely possible that it doesn't
happen on NAND. But really, it's not something we should be depending
on and the software mechanisms have to remain in place.


My point is really that say that the problem is in fact not that the erase
is cut short due to the power fail, but that the software issues a second
command before the first erase command has completed, for instance, or
some other situation. Then we'd have a concrete situation which we can
resolve (i.e., fix the bug), rather than assuming that it's the hardware's
fault and implement various software workarounds.


You're forgetting that the SSD itself (this thread is about SSDs) also has
a major software component which is doing housekeeping all the time, so even
if the main CPU gets reset the SSD's controller may still happily be erasing
blocks.

Regards,

Hans

Re: JMS56x not working reliably with uas driver

[Hans de Goede]

Hi,

On 21-12-16 13:07, George Cherian wrote:



On 12/21/2016 05:12 PM, Oliver Neukum wrote:

On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote:

Hi Oliver,

I was working with this JMicron device and using the uas driver.
I am seeing the following 2 issues.

1) On connect I see the following messages.


Thanks. Do you want to submit it to Greg?
The patch is fine.

Yes please!!!




2) On disconnect I am seeing the following issue

   scsi host4: uas_post_reset: alloc streams error -19 after reset
   sd 4:0:0:0: [sdb] Synchronizing SCSI cache

This is more fatal because after these messages the USB port becomes
unusable. Even an lsusb invocation hangs for ever.


Ouch. That points to a logic error. We should not reset if
a device is gone.
Could you send dmesg of such a case?

here is the dmesg!!
[  203.475382] usb 4-1.3: new SuperSpeed USB device number 3 using xhci_hcd
[  203.496172] usb 4-1.3: New USB device found, idVendor=152d, idProduct=9561
[  203.503037] usb 4-1.3: New USB device strings: Mfr=1, Product=2, 
SerialNumber=5
[  203.510352] usb 4-1.3: Product: JMS56x Series
[  203.514698] usb 4-1.3: Manufacturer: JMicron
[  203.518966] usb 4-1.3: SerialNumber: 
[  203.594383] usbcore: registered new interface driver usb-storage
[  203.612425] scsi host4: uas
[  203.615418] usbcore: registered new interface driver uas
[  203.620979] scsi 4:0:0:0: Direct-Access ST4000NM 0033-9ZM170  0001 PQ: 0 
ANSI: 6
[  203.630240] sd 4:0:0:0: Attached scsi generic sg1 type 0
[  203.630382] sd 4:0:0:0: [sdb] 7814037168 512-byte logical blocks: (4.00 
TB/3.63 TiB)
[  203.631338] sd 4:0:0:0: [sdb] Write Protect is off
[  203.631342] sd 4:0:0:0: [sdb] Mode Sense: 67 00 10 08
[  203.631734] sd 4:0:0:0: [sdb] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[  203.631899] xhci_hcd :00:11.0: ERROR Transfer event for disabled 
endpoint or incorrect stream ring
[  203.631904] xhci_hcd :00:11.0: @001f610a1c10   
1b00 03078001 state 14 ep_info 9403
[  203.631906] xhci_hcd :00:11.0: No epring
[  203.674546]  sdb: sdb1
[  203.676639] sd 4:0:0:0: [sdb] Attached SCSI disk
[  213.222913] scsi host4: uas_post_reset: alloc streams error -19 after reset
[  213.230548] sd 4:0:0:0: [sdb] Synchronizing SCSI cache

Above is the dmesg without the unusual_uas patch applied.
Do you need me to enable any specific dev_dbg and then the dmesg output?


Can you get us a dmesg with the unusual_uas patch applied? Usually once things 
go foobar because
of issue-ing a command which the device does not understand, more things 
typically come down
as both the device and the host may be in an undefined state then.

Regards,

Hans
--
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

Re: JMS56x not working reliably with uas driver

[Hans de Goede]

Hi,

On 21-12-16 12:42, Oliver Neukum wrote:

On Wed, 2016-12-21 at 17:09 +0530, George Cherian wrote:

Hi Oliver,

I was working with this JMicron device and using the uas driver.
I am seeing the following 2 issues.

1) On connect I see the following messages.


Thanks. Do you want to submit it to Greg?
The patch is fine.


2) On disconnect I am seeing the following issue

  scsi host4: uas_post_reset: alloc streams error -19 after reset
  sd 4:0:0:0: [sdb] Synchronizing SCSI cache

This is more fatal because after these messages the USB port becomes
unusable. Even an lsusb invocation hangs for ever.


Ouch. That points to a logic error. We should not reset if
a device is gone.


Ah good point, so instead of just seeing a disconnect, something
is triggering a reset, and then we try to alloc stream on the
disconnected usb port and that apparently makes the xhci controller
quite unhappy.

Regards,

Hans
--
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

Re: JMS56x not working reliably with uas driver

[Hans de Goede]

Hi,

On 21-12-16 12:39, George Cherian wrote:

Hi Oliver,

I was working with this JMicron device and using the uas driver.
I am seeing the following 2 issues.

1) On connect I see the following messages.
xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect 
stream ring
 This was eliminated using the following scissor patch.

-8<
[PATCH] usb: storage: unusual_uas: Add JMicron JMS56x to unusual device

This device gives the following error on detection.
xhci_hcd :00:11.0: ERROR Transfer event for disabled endpoint or incorrect 
stream ring

The same error is not seen when it is added to unusual_device
list with US_FL_NO_REPORT_OPCODES passed.

Signed-off-by: George Cherian <george.cher...@cavium.com>
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index cbea9f3..d292299 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -142,6 +142,13 @@ UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
 USB_SC_DEVICE, USB_PR_DEVICE, NULL,
 US_FL_BROKEN_FUA | US_FL_NO_REPORT_OPCODES),

+/* Reported-by George Cherian <george.cher...@cavium.com> */
+UNUSUAL_DEV(0x152d, 0x9561, 0x, 0x,
+"JMicron",
+"JMS56x",
+USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+US_FL_NO_REPORT_OPCODES),
+
 /* Reported-by: Hans de Goede <hdego...@redhat.com> */
 UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x,
 "VIA",
->8

2) On disconnect I am seeing the following issue

 scsi host4: uas_post_reset: alloc streams error -19 after reset
 sd 4:0:0:0: [sdb] Synchronizing SCSI cache

This is more fatal because after these messages the USB port becomes unusable. 
Even an lsusb invocation hangs for ever.

Also please note that the device works fine with usb-storage driver.
I am attaching the usbmon capture of disconnect using uas and usb-storage 
driver.

Any help in this regard is highly appreciated.


Are you still seeen this second problem with the first patch applied ? Is this 
after
an actual disconnect, or after the kernel seeing a disconnect without the device
being actually disconnected.

This second problem sounds like it is an issue with your xhci controller. Can 
you
try this on another motherboard (with another xhci controller) ?

Regards,

Hans
--
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

[PATCH] usb: MAINTAINERS: Oliver Neukum is the new uas maintainer

[Hans de Goede]

Oliver Neukum is taking over uas maintainership from me and
Gerd Hoffmann.

Cc: Oliver Neukum <oneu...@suse.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 MAINTAINERS | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5c728d1..9c220f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11801,8 +11801,7 @@ S:  Maintained
 F: drivers/net/wireless/ath/ar5523/
 
 USB ATTACHED SCSI
-M: Hans de Goede <hdego...@redhat.com>
-M: Gerd Hoffmann <kra...@redhat.com>
+M: Oliver Neukum <oneu...@suse.com>
 L: linux-...@vger.kernel.org
 L: linux-scsi@vger.kernel.org
 S: Maintained
-- 
2.7.4

--
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

Re: [PATCH resend] USB: uas: Fix slave queue_depth not being set

[Hans de Goede]

Hi,

On 13-06-16 14:05, Oliver Neukum wrote:

On Tue, 2016-05-31 at 09:18 +0200, Hans de Goede wrote:

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
removed the scsi_change_queue_depth() call from uas_slave_configure()
assuming that the slave would inherit the host's queue_depth, which
that commit sets to the same value.

This is incorrect, without the scsi_change_queue_depth() call the slave's
queue_depth defaults to 1, introducing a performance regression.


Hans, may I ask what become of this patch? I don't see it in the queue.


It is here:

https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-linus=593224ea77b1ca842f45cf76f4deeef44dfbacd1

Which is part of:

https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/log/?h=usb-linus

Regards,

Hans
--
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

[PATCH resend 0/1] USB: uas: Fix slave queue_depth not being set

[Hans de Goede]

Hi Greg,

There was some discussion about this patch during its initial posting,
but the concensus seems to be that this patch, which is in essence a
partial revert of the patch causing the problem, is the best way to fix
this.

So I'm resending this to make sure it does not fall through the cracks,
given that this fixes a (performance) regression.

Regards,

Hans
--
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

[PATCH resend] USB: uas: Fix slave queue_depth not being set

[Hans de Goede]

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
removed the scsi_change_queue_depth() call from uas_slave_configure()
assuming that the slave would inherit the host's queue_depth, which
that commit sets to the same value.

This is incorrect, without the scsi_change_queue_depth() call the slave's
queue_depth defaults to 1, introducing a performance regression.

This commit restores the call, fixing the performance regression.

Cc: sta...@vger.kernel.org
Fixes: 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
Reported-by: Tom Yan <tom.t...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 16bc679..ecc7d4b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -835,6 +835,7 @@ static int uas_slave_configure(struct scsi_device *sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;
 
+   scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
return 0;
 }
 
-- 
2.7.4

--
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

Re: [PATCH] USB: uas: Fix slave queue_depth not being set

[Hans de Goede]

Hi,

On 24-05-16 14:44, James Bottomley wrote:

On Tue, 2016-05-24 at 08:53 +0200, Hans de Goede wrote:

Hi,

On 23-05-16 19:36, James Bottomley wrote:

On Mon, 2016-05-23 at 13:49 +0200, Hans de Goede wrote:

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host
level")
removed the scsi_change_queue_depth() call from
uas_slave_configure() assuming that the slave would inherit the
host's queue_depth, which that commit sets to the same value.

This is incorrect, without the scsi_change_queue_depth() call the
slave's queue_depth defaults to 1, introducing a performance
regression.

This commit restores the call, fixing the performance regression.

Cc: sta...@vger.kernel.org
Fixes: 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host
level")
Reported-by: Tom Yan <tom.t...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/storage/uas.c
b/drivers/usb/storage/uas.c
index 16bc679..ecc7d4b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -835,6 +835,7 @@ static int uas_slave_configure(struct
scsi_device
*sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;

+   scsi_change_queue_depth(sdev, devinfo->qdepth - 2);


Are you sure about this?  For spinning rust, experiments imply that
the optimal queue depth per device is somewhere between 2 and 4.
 Obviously that's not true for SSDs, so it depends on your use
case.  Plus, for ATA NCQ devices (which I believe most UAS is
bridged to) you have a maximum NCQ depth of 31.


So this value is the same as host.can_queue, and is what uas has
always used, basically this says it is ok to queue as much as the
bridge can handle. We've seen a few rare multi-lun devices, but
typically almost all uas devices have one lun, what I really want to
do here is give a maximum and let say the sd driver lower that if it
is sub-optimal.


If that's what you actually want, you should be setting sdev
->max_queue_depth and .track_queue_depth = 1 in the template.


Hmm, I've been looking into this, but that does not seem right.

max_queue_depth is never set by drivers, it is set to sdev->queue_depth
in scsi_scan.c: scsi_add_lun() after calling the host drivers'
slave_configure callback. So it seems that the right way to set
max_queue_depth is to actually set queue_depth, or iow restore the
call to scsi_change_queue_depth() as the patch we're discussing does.

As for track_queue_depth = 1 that seems to be only set by some drivers
under drivers/scsi and is never set by any drivers under drivers/ata,
and we're almost exclusively dealing with sata disks in uas. It seems
that track_queue_depth = 1 is mostly used for iscsi and fibre channel
iow enterprise class storage stuff, so looking at existing drivers
usage of this flag using it does not seem a good idea.

Anyways this patch is a (partial) revert of a previous bug-fix patch
(which has also gone to stable) so for now I would really like to
move forward with this patch and get it upstream and in stable
to fix the performance regressions people are seeing caused by
me wrongly dropping the scsi_change_queue_depth() call.

Then if we want to tweak the queuing further we can do that
on top of this fix, and put that in next and let it get some testing
first.

So are you ok with moving this patch forward ?

Regards,

Hans




You might also need to add calls to scsi_track_queue_full() but only if
the devices aren't responding QUEUE_FULL correctly.

James


Also notice that uas is used a lot with ssd-s, that is mostly what
I want to optimize for, but it is definitely also used with spinning
rust.

And yes almost all uas devices are bridged sata devices (this may
change in the near future though, with ssd-s specifically designed
for usb-3 attachment, although sofar these all seem to use an
embbeded sata bridge), so from this pov an upper limit of 31 makes
sense, I guess, but I've not seen any bridges which actually do more
then 32 streams anyways.

Still this is a bug-fix patch, essentially a partial revert, to
address performance regressions, so lets get this out as is and take
our time to come up with some tweaks (if necessary) for the say 4.8.


There's a good reason why you don't want a queue deeper than you
can handle: it tends to interfere with writeback because you build
up a lot of pending I/O in the queue which can't be issued (it's
very similar to why bufferbloat is a problem in networks).  In
theory, as long as your devices return the correct indicator
(QUEUE_FULL status), we'll handle most of this in the mid-layer by
plugging the block queue, but given what I've seen from UAS
devices, that's less than probable.


So any smart ideas how to be nicer to spinning rust, without
negatively impacting ssd-s? As said if I've to choice I think we
should chose optimizing ssd-s, as that is where uas is used a lot
(although u

Re: [uas/scsi] untagged commands?

[Hans de Goede]

Hi,

On 24-05-16 10:18, Tom Yan wrote:

In this commit:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/storage/uas.c?id=198de51dbc3454d95b015ca0a055b673f85f01bb

There is the following comment:

1 tag is reserved for untagged commands


So my question is, what exactly are "untagged commands"?


https://en.wikipedia.org/wiki/Tagged_Command_Queuing

Regards,

Hans
--
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

Re: [PATCH v2 1/1] uas: remove can_queue set in host template

[Hans de Goede]

Hi,

On 23-05-16 21:28, tom.t...@gmail.com wrote:

From: Tom Yan <tom.t...@gmail.com>

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level") made
qdepth limit set in host template (`.can_queue = MAX_CMNDS`) redundant.
Removing it to avoid confusion.

Signed-off-by: Tom Yan <tom.t...@gmail.com>


I agree removing this is good:

Acked-by: Hans de Goede <hdego...@redhat.com>

Regards,

Hans




diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4d49fce..e03c490 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -848,7 +848,6 @@ static struct scsi_host_template uas_host_template = {
.slave_configure = uas_slave_configure,
.eh_abort_handler = uas_eh_abort_handler,
.eh_bus_reset_handler = uas_eh_bus_reset_handler,
-   .can_queue = MAX_CMNDS,
.this_id = -1,
.sg_tablesize = SG_NONE,
.skip_settle_delay = 1,


--
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

Re: [PATCH] USB: uas: Fix slave queue_depth not being set

[Hans de Goede]

Hi,

On 23-05-16 19:36, James Bottomley wrote:

On Mon, 2016-05-23 at 13:49 +0200, Hans de Goede wrote:

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
removed the scsi_change_queue_depth() call from uas_slave_configure()
assuming that the slave would inherit the host's queue_depth, which
that commit sets to the same value.

This is incorrect, without the scsi_change_queue_depth() call the
slave's queue_depth defaults to 1, introducing a performance
regression.

This commit restores the call, fixing the performance regression.

Cc: sta...@vger.kernel.org
Fixes: 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
Reported-by: Tom Yan <tom.t...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 16bc679..ecc7d4b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -835,6 +835,7 @@ static int uas_slave_configure(struct scsi_device
*sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;

+   scsi_change_queue_depth(sdev, devinfo->qdepth - 2);


Are you sure about this?  For spinning rust, experiments imply that the
optimal queue depth per device is somewhere between 2 and 4.  Obviously
that's not true for SSDs, so it depends on your use case.  Plus, for
ATA NCQ devices (which I believe most UAS is bridged to) you have a
maximum NCQ depth of 31.


So this value is the same as host.can_queue, and is what uas has always
used, basically this says it is ok to queue as much as the bridge can
handle. We've seen a few rare multi-lun devices, but typically almost
all uas devices have one lun, what I really want to do here is give a
maximum and let say the sd driver lower that if it is sub-optimal.

Also notice that uas is used a lot with ssd-s, that is mostly what
I want to optimize for, but it is definitely also used with
spinning rust.

And yes almost all uas devices are bridged sata devices (this may
change in the near future though, with ssd-s specifically designed
for usb-3 attachment, although sofar these all seem to use an
embbeded sata bridge), so from this pov an upper limit of 31 makes sense,
I guess, but I've not seen any bridges which actually do more then 32
streams anyways.

Still this is a bug-fix patch, essentially a partial revert, to address
performance regressions, so lets get this out as is and take our time to
come up with some tweaks (if necessary) for the say 4.8.


There's a good reason why you don't want a queue deeper than you can
handle: it tends to interfere with writeback because you build up a lot
of pending I/O in the queue which can't be issued (it's very similar to
why bufferbloat is a problem in networks).  In theory, as long as your
devices return the correct indicator (QUEUE_FULL status), we'll handle
most of this in the mid-layer by plugging the block queue, but given
what I've seen from UAS devices, that's less than probable.


So any smart ideas how to be nicer to spinning rust, without negatively
impacting ssd-s? As said if I've to choice I think we should chose
optimizing ssd-s, as that is where uas is used a lot (although usb-attached
harddisks are switching over to it too).

Note I just checked the 1TB sata/ahci harddisk in my workstation and it is using
a queue_depth of 31 too, so this really does seem like a mid-layer problem
to me.

Regards,

Hans
--
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

[PATCH] USB: uas: Fix slave queue_depth not being set

[Hans de Goede]

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
removed the scsi_change_queue_depth() call from uas_slave_configure()
assuming that the slave would inherit the host's queue_depth, which
that commit sets to the same value.

This is incorrect, without the scsi_change_queue_depth() call the slave's
queue_depth defaults to 1, introducing a performance regression.

This commit restores the call, fixing the performance regression.

Cc: sta...@vger.kernel.org
Fixes: 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level")
Reported-by: Tom Yan <tom.t...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 16bc679..ecc7d4b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -835,6 +835,7 @@ static int uas_slave_configure(struct scsi_device *sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;
 
+   scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
return 0;
 }
 
-- 
2.7.4

--
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

Re: [regression] uas no longer queues commands since v4.5.2

[Hans de Goede]

Hi,

On 22-05-16 12:39, Tom Yan wrote:

With commit 198de51dbc3454d95b015ca0a055b673f85f01bb, uas no longer
set `queue_depth` with scsi_change_queue_depth(), so now `queue_depth`
of UAS drives are 1. Even though `can_queue` is set to
`devinfo->qdepth - 2`, but apparently that does not help, since I can
see performance impact.

Suppose limiting `can_queue` is the right way to fix this bug
(https://bugzilla.redhat.com/show_bug.cgi?id=1315013), do we really
need to eliminate scsi_change_queue_depth() at the same time though?


No, I thought it was no longer needed, assuming the slave would
inherit the host's value, but I was mistaken, the slave will default
to a queue_depth of 1 if not specified.

I'll send out a patch fixing this right away.

Regards,

Hans
--
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

Re: uas: order 7 page allocation failure in init_tag_map()

[Hans de Goede]

Hi,

On 23-04-16 23:10, Johannes Stezenbach wrote:

Hi,

I bought a new backup disk which turned out to be UAS capable,
but when I plugged it in I got an order 7 page allocation failure.
My hunch is that the .can_queue = 65536 in drivers/usb/storage/uas.c
is much too large.  Maybe 256 would be a pratical value that matches
the capabilities of existing hardware?


This is already fixed upstream (and being backported to the stable series):

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/storage/uas.c?id=55ff8cfbc4e12a7d2187df523938cc671fbebdd1

Regards,

Hans





[1859683.261465] usb 4-2: new SuperSpeed USB device number 8 using xhci_hcd
[1859683.281986] scsi host18: uas
[1859683.282003] kworker/0:2: page allocation failure: order:7, mode:0x208c020
[1859683.282008] CPU: 0 PID: 6888 Comm: kworker/0:2 Not tainted 4.4.6 #1
[1859683.282011] Hardware name: System manufacturer System Product 
Name/P8H77-V, BIOS 1905 10/27/2014
[1859683.282017] Workqueue: usb_hub_wq hub_event
[1859683.282021]  0286 d38f5999 8800751674d0 
813527de
[1859683.282026]   0208c020 880075167570 
81157c56
[1859683.282031]  880075167580 880075167508 81f43840 
00f438b8
[1859683.282036] Call Trace:
[1859683.282045]  [] dump_stack+0x85/0xbe
[1859683.282050]  [] warn_alloc_failed+0x12c/0x156
[1859683.282055]  [] __alloc_pages_nodemask+0x73a/0x8f1
[1859683.282060]  [] ? dev_vprintk_emit+0x1cb/0x1f1
[1859683.282065]  [] alloc_kmem_pages+0x22/0x8a
[1859683.282069]  [] kmalloc_order+0x18/0x46
[1859683.282072]  [] kmalloc_order_trace+0x21/0xe9
[1859683.282077]  [] __kmalloc+0x38/0x22f
[1859683.282081]  [] ? __blk_queue_init_tags+0x2f/0x73
[1859683.282085]  [] init_tag_map+0x54/0xa3
[1859683.282088]  [] __blk_queue_init_tags+0x45/0x73
[1859683.282092]  [] blk_init_tags+0x14/0x16
[1859683.282096]  [] scsi_add_host_with_dma+0xc8/0x2a0
[1859683.282102]  [] uas_probe+0x3aa/0x420 [uas]
[1859683.282107]  [] usb_probe_interface+0x1a6/0x22d
[1859683.282112]  [] driver_probe_device+0x173/0x3a6
[1859683.282116]  [] __device_attach_driver+0x71/0x78
[1859683.282120]  [] ? driver_allows_async_probing+0x31/0x31
[1859683.282124]  [] bus_for_each_drv+0x8a/0xad
[1859683.282128]  [] __device_attach+0xba/0x14f
[1859683.282132]  [] device_initial_probe+0x13/0x15
[1859683.282136]  [] bus_probe_device+0x33/0x9e
[1859683.282140]  [] device_add+0x2e4/0x56e
[1859683.282144]  [] usb_set_configuration+0x689/0x6d9
[1859683.282148]  [] ? debug_smp_processor_id+0x17/0x19
[1859683.282152]  [] generic_probe+0x43/0x73
[1859683.282156]  [] usb_probe_device+0x53/0x66
[1859683.282159]  [] driver_probe_device+0x173/0x3a6
[1859683.282163]  [] __device_attach_driver+0x71/0x78
[1859683.282167]  [] ? driver_allows_async_probing+0x31/0x31
[1859683.282171]  [] bus_for_each_drv+0x8a/0xad
[1859683.282175]  [] __device_attach+0xba/0x14f
[1859683.282179]  [] device_initial_probe+0x13/0x15
[1859683.282183]  [] bus_probe_device+0x33/0x9e
[1859683.282186]  [] device_add+0x2e4/0x56e
[1859683.282191]  [] usb_new_device+0x241/0x38a
[1859683.282194]  [] hub_event+0xcb9/0x10f2
[1859683.282201]  [] process_one_work+0x27f/0x4d7
[1859683.282206]  [] ? put_lock_stats.isra.9+0xe/0x20
[1859683.282209]  [] worker_thread+0x273/0x35b
[1859683.282214]  [] ? rescuer_thread+0x2a7/0x2a7
[1859683.282217]  [] kthread+0xff/0x107
[1859683.28]  [] ? kthread_create_on_node+0x1ea/0x1ea
[1859683.282228]  [] ret_from_fork+0x3f/0x70
[1859683.282231]  [] ? kthread_create_on_node+0x1ea/0x1ea
[1859683.282234] Mem-Info:
[1859683.282241] active_anon:21278 inactive_anon:69854 isolated_anon:0
   active_file:212300 inactive_file:194346 isolated_file:0
   unevictable:2018 dirty:87 writeback:0 unstable:0
   slab_reclaimable:127644 slab_unreclaimable:12137
   mapped:11526 shmem:13394 pagetables:5007 bounce:0
   free:270678 free_pcp:1027 free_cma:0
[1859683.282252] DMA free:14412kB min:32kB low:40kB high:48kB active_anon:180kB 
inactive_anon:468kB active_file:268kB inactive_file:92kB unevictable:0kB 
isolated(anon):0kB isolated(file):0kB present:15984kB managed:15900kB 
mlocked:0kB dirty:4kB writeback:0kB mapped:172kB shmem:328kB 
slab_reclaimable:208kB slab_unreclaimable:92kB kernel_stack:0kB pagetables:56kB 
unstable:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB 
writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[1859683.282255] lowmem_reserve[]: 0 3162 3597 3597
[1859683.282267] DMA32 free:904468kB min:6728kB low:8408kB high:10092kB 
active_anon:66188kB inactive_anon:237164kB active_file:803244kB 
inactive_file:704168kB unevictable:7024kB isolated(anon):0kB isolated(file):0kB 
present:3334492kB managed:3243208kB mlocked:7024kB dirty:280kB writeback:0kB 
mapped:37116kB shmem:40212kB slab_reclaimable:435236kB 
slab_unreclaimable:37848kB kernel_stack:3968kB pagetables:16696kB unstable:0kB 
bounce:0kB 

[PATCH v2 2/2] USB: 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: sta...@vger.kernel.org
Reported-and-tested-by: David Webb <d...@noc.ac.uk>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
Changes in v2:
-Document new j quirk in Documentation/kernel-parameters.txt
---
 Documentation/kernel-parameters.txt |  2 ++
 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 ++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index ecc74fa..0b3de80 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -4077,6 +4077,8 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
sector if the number is odd);
i = IGNORE_DEVICE (don't bind to this
device);
+   j = NO_REPORT_LUNS (don't use report luns
+   command, uas only);
l = NOT_LOCKABLE (don't try to lock and
unlock ejectable media);
m = MAX_SECTORS_64 (don't transfer more
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b1ec749..16bc679 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 <hdego...@redhat.com> for Red Hat, Inc. 2013 - 2014
+ * Copyright Hans de Goede <hdego...@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, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: David Webb <d...@noc.ac.uk> */
+UNUSUAL_DEV(0x0bc2, 0x331a, 0x, 0x,
+   "Seagate",
+   "Expansion Desk",
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_REPORT_LUNS),
+
 /* Reported-by: Hans de Goede <hdego...@redhat.com> */
 UNUSUAL_DEV(0x0bc2, 0x3320, 0x, 0x,
"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

[PATCH v2 1/2] USB: uas: Limit qdepth at the scsi-host level

[Hans de Goede]

Commit 64d513ac31bd ("scsi: use host wide tags by default") causes
the SCSI core to queue more commands then we can handle on devices with
multiple LUNs, limit the queue depth at the scsi-host level instead of
per slave to fix this.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1315013
Cc: sta...@vger.kernel.org # 4.4.x and 4.5.x
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
Changes in v2:
-Fix some spelling issues in the commit message and in a comment
---
 drivers/usb/storage/uas.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 13e4cc3..b1ec749 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -824,7 +824,6 @@ static int uas_slave_configure(struct scsi_device *sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;
 
-   scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
return 0;
 }
 
@@ -956,6 +955,12 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
if (result)
goto set_alt0;
 
+   /*
+* 1 tag is reserved for untagged commands +
+* 1 tag to avoid off by one errors in some bridge firmwares
+*/
+   shost->can_queue = devinfo->qdepth - 2;
+
usb_set_intfdata(intf, shost);
result = scsi_add_host(shost, >dev);
if (result)
-- 
2.7.3

--
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

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
--
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

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
--
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

[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: sta...@vger.kernel.org
Reported-and-tested-by: David Webb <d...@noc.ac.uk>
Signed-off-by: Hans de Goede <hdego...@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 <hdego...@redhat.com> for Red Hat, Inc. 2013 - 2014
+ * Copyright Hans de Goede <hdego...@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, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: David Webb <d...@noc.ac.uk> */
+UNUSUAL_DEV(0x0bc2, 0x331a, 0x, 0x,
+   "Seagate",
+   "Expansion Desk",
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_REPORT_LUNS),
+
 /* Reported-by: Hans de Goede <hdego...@redhat.com> */
 UNUSUAL_DEV(0x0bc2, 0x3320, 0x, 0x,
"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,0x0800) \
/* Sets max_sectors to 240 */   \
+   US_FLAG(NO_REPORT_LUNS, 0x1000) \
+   /* Cannot handle REPORT_LUNS */ \
 
 #define US_FLAG(name, value)   US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.7.3

--
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

[PATCH] uas: Limit qdepth at the scsi-host level

[Hans de Goede]

Commit 64d513ac31bd ("scsi: use host wide tags by default") causes
the scsi-core to queue more cmnds then we can handle on devices with
multiple LUNs, limit the qdepth at the scsi-host level instead of
per slave to fix this.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1315013
Cc: sta...@vger.kernel.org # 4.4.x and 4.5.x
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index c90a7e4..b5cb7ab 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -800,7 +800,6 @@ static int uas_slave_configure(struct scsi_device *sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;
 
-   scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
return 0;
 }
 
@@ -932,6 +931,12 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
if (result)
goto set_alt0;
 
+   /*
+* 1 tag is reserved for untagged commands +
+* 1 tag to avoid of by one errors in some bridge firmwares
+*/
+   shost->can_queue = devinfo->qdepth - 2;
+
usb_set_intfdata(intf, shost);
result = scsi_add_host(shost, >dev);
if (result)
-- 
2.7.3

--
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

Re: "scsi: use host wide tags by default" causes uas regression

[Hans de Goede]

Hi,

On 09-03-16 19:26, Christoph Hellwig wrote:

On Wed, Mar 09, 2016 at 06:58:48PM +0100, Hans de Goede wrote:

So my question is how do I tell the scsi layer to not submit more then X 
commands with
scsi_init_shared_tag_map() gone ?


By setting ->can_queue in the Scsi_Host structure to the maximum number
of command you want outstanding for this host.


Can I lower this after calling scsi_host_alloc(), or do I need to make
my template "dynamic" ?

Regards,

Hans

--
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

"scsi: use host wide tags by default" causes uas regression

[Hans de Goede]

Hi Christoph,

I've received a bug report that uas is causing a 2-disk enclosure to
lookup with 4.4 and later:

https://bugzilla.redhat.com/show_bug.cgi?id=1315013

Looking at the dmesg this stands out:

Mar 09 01:55:21 larry kernel: sd 2:0:0:1: [sdc] tag#31 uas_eh_abort_handler 0 
uas-tag 32 inflight: CMD OUT
Mar 09 01:55:21 larry kernel: sd 2:0:0:1: [sdc] tag#31 CDB: Write(10) 2a 00 e8 
10 f4 00 00 04 00 00

Specifically the uas-tag 32, technically that is correct, but we have been 
avoiding
actually submitting commands with that tag because bulk-streams are numbered 1-x
and I was afraid that actually using the full-range would trigger off-by-one
errors in devices and it looks I was right.

Before your patch uas was doing:

@@ -929,10 +928,6 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
if (result)
goto set_alt0;

-   result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 2);
-   if (result)
-   goto free_streams;
-
usb_set_intfdata(intf, shost);
result = scsi_add_host(shost, >dev);
if (result)

With devinfo->qdepth being 32 in this case, so the end-result would be passing
30 to scsi_init_shared_tag_map, reserving one tag for untagged commands and one
tag for the off-by-one thingie.

We also have:

static int uas_slave_configure(struct scsi_device *sdev)
{
...
scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
...
}

But that only helps with single lun enclosures, so I guess that once we fix the 
issue that
line can actually go.

So my question is how do I tell the scsi layer to not submit more then X 
commands with
scsi_init_shared_tag_map() gone ?

Regards,

Hans
--
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

[PATCH] uas: Reduce can_queue to MAX_CMNDS

[Hans de Goede]

The uas driver can never queue more then MAX_CMNDS (- 1) tags and tags
are shared between luns, so there is no need to claim that we can_queue
some random large number.

Not claiming that we can_queue 65536 commands, fixes the uas driver
failing to initialize while allocating the tag map with a "Page allocation
failure (order 7)" error on systems which have been running for a while
and thus have fragmented memory.

Cc: sta...@vger.kernel.org
Reported-and-tested-by: Yves-Alexis Perez <cor...@corsac.net>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 drivers/usb/storage/uas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 9ff9404..c90a7e4 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -812,7 +812,7 @@ static struct scsi_host_template uas_host_template = {
.slave_configure = uas_slave_configure,
.eh_abort_handler = uas_eh_abort_handler,
.eh_bus_reset_handler = uas_eh_bus_reset_handler,
-   .can_queue = 65536, /* Is there a limit on the _host_ ? */
+   .can_queue = MAX_CMNDS,
.this_id = -1,
.sg_tablesize = SG_NONE,
.skip_settle_delay = 1,
-- 
2.7.2

--
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

Re: Page allocation failure (order 7) in UAS code

[Hans de Goede]

Hi,

On 04-03-16 08:13, Yves-Alexis Perez wrote:

On mar., 2016-03-01 at 11:49 +0100, Hans de Goede wrote:

Hi,

On 01-03-16 10:42, Yves-Alexis Perez wrote:


Hi,

[sorry if this is not the right point for reporting bugs, I took the email
addresses from MAINTAINERS but please point me to the correct place if
needed]

I have an external USB drive (Samsung M3), which apparently uses the UAS
code.
Starting with 4.4 (from Debian sid, I could retry with vanilla if needed),
I
can't mount the drive anymore after a while (few hours/days uptime). Just
plugging the disk, I get page allocation failure in kernel logs:

Can you try building a kernel with the following line in
drivers/usb/storage/uas.c :

  .can_queue = 65536, /* Is there a limit on the _host_ ? */

(around line 815) Replaced with

  .can_queue = MAX_CMNDS,

That should help as MAX_CMNDS is 256, so claiming that we can queue more
is not helpful, and that likely is what is causing this quite high order
alloc.


After a few days, it seems that it does work fine, although I can't say
anything about sides effects.


Thanks for testing, there shouldn't be any side-effects, I'll turn this into
a proper patch, add a:

Reported-and-tested-by: Yves-Alexis Perez <cor...@corsac.net>

line to the comit msg and submit this upstream.

Regards,

Hans
--
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

Re: Page allocation failure (order 7) in UAS code

[Hans de Goede]

Hi,

On 01-03-16 10:42, Yves-Alexis Perez wrote:

Hi,

[sorry if this is not the right point for reporting bugs, I took the email
addresses from MAINTAINERS but please point me to the correct place if needed]

I have an external USB drive (Samsung M3), which apparently uses the UAS code.
Starting with 4.4 (from Debian sid, I could retry with vanilla if needed), I
can't mount the drive anymore after a while (few hours/days uptime). Just
plugging the disk, I get page allocation failure in kernel logs:


Can you try building a kernel with the following line in 
drivers/usb/storage/uas.c :

.can_queue = 65536, /* Is there a limit on the _host_ ? */

(around line 815) Replaced with

.can_queue = MAX_CMNDS,

That should help as MAX_CMNDS is 256, so claiming that we can queue more
is not helpful, and that likely is what is causing this quite high order alloc.

Regards,

Hans
--
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

[PATCH v2 2/3] uas: Add US_FL_MAX_SECTORS_240 flag

[Hans de Goede]

The usb-storage driver sets max_sectors = 240 in its scsi-host template,
for uas we do not want to do that for all devices, but testing has shown
that some devices need it.

This commit adds a US_FL_MAX_SECTORS_240 flag for such devices, and
implements support for it in uas.c, while at it it also adds support
for US_FL_MAX_SECTORS_64 to uas.c.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
Changes in v2:
-Document new 'g' flag in kernel-parameters.txt
-Add Cc stable@vger
---
 Documentation/kernel-parameters.txt |  2 ++
 drivers/usb/storage/uas.c   | 10 +-
 drivers/usb/storage/usb.c   |  6 +-
 include/linux/usb_usual.h   |  2 ++
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 274252f..4ddf5b5 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3774,6 +3774,8 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
READ_CAPACITY_16 command);
f = NO_REPORT_OPCODES (don't use report opcodes
command, uas only);
+   g = MAX_SECTORS_240 (don't transfer more than
+   240 sectors at a time, uas only);
h = CAPACITY_HEURISTICS (decrease the
reported device capacity by one
sector if the number is odd);
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index c6109c1..6d3122a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -759,7 +759,10 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 
 static int uas_slave_alloc(struct scsi_device *sdev)
 {
-   sdev-hostdata = (void *)sdev-host-hostdata;
+   struct uas_dev_info *devinfo =
+   (struct uas_dev_info *)sdev-host-hostdata;
+
+   sdev-hostdata = devinfo;
 
/* USB has unusual DMA-alignment requirements: Although the
 * starting address of each scatter-gather element doesn't matter,
@@ -778,6 +781,11 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 */
blk_queue_update_dma_alignment(sdev-request_queue, (512 - 1));
 
+   if (devinfo-flags  US_FL_MAX_SECTORS_64)
+   blk_queue_max_hw_sectors(sdev-request_queue, 64);
+   else if (devinfo-flags  US_FL_MAX_SECTORS_240)
+   blk_queue_max_hw_sectors(sdev-request_queue, 240);
+
return 0;
 }
 
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index db6f6b5..6c10c88 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -479,7 +479,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
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_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
+   US_FL_MAX_SECTORS_240);
 
p = quirks;
while (*p) {
@@ -520,6 +521,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
case 'f':
f |= US_FL_NO_REPORT_OPCODES;
break;
+   case 'g':
+   f |= US_FL_MAX_SECTORS_240;
+   break;
case 'h':
f |= US_FL_CAPACITY_HEURISTICS;
break;
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index a7f2604..7f5f78b 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -77,6 +77,8 @@
/* Cannot handle ATA_12 or ATA_16 CDBs */   \
US_FLAG(NO_REPORT_OPCODES,  0x0400) \
/* Cannot handle MI_REPORT_SUPPORTED_OPERATION_CODES */ \
+   US_FLAG(MAX_SECTORS_240,0x0800) \
+   /* Sets max_sectors to 240 */   \
 
 #define US_FLAG(name, value)   US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.3.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://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] uas: Set max_sectors_240 quirk for ASM1053 devices

[Hans de Goede]

Testing has shown that ASM1053 devices do not work properly with transfers
larger than 240 sectors, so set max_sectors to 240 on these.

Cc: sta...@vger.kernel.org # 3.16
Reported-by: Steve Bangert sbang...@frontier.com
Signed-off-by: Hans de Goede hdego...@redhat.com
Tested-by: Steve Bangert sbang...@frontier.com
---
Changes in v2:
-Add Cc stable@vger
-Add Tested-by tag
---
 drivers/usb/storage/uas-detect.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index 63ae161..f58caa9 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -74,7 +74,7 @@ static int uas_use_uas_driver(struct usb_interface *intf,
 * this writing the following versions exist:
 * ASM1051 - no uas support version
 * ASM1051 - with broken (*) uas support
-* ASM1053 - with working uas support
+* ASM1053 - with working uas support, but problems with large xfers
 * ASM1153 - with working uas support
 *
 * Devices with these chips re-use a number of device-ids over the
@@ -104,6 +104,9 @@ static int uas_use_uas_driver(struct usb_interface *intf,
} else if (usb_ss_max_streams(eps[1]-ss_ep_comp) == 32) {
/* Possibly an ASM1051, disable uas */
flags |= US_FL_IGNORE_UAS;
+   } else {
+   /* ASM1053, these have issues with large transfers */
+   flags |= US_FL_MAX_SECTORS_240;
}
}
 
-- 
2.3.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://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] uas: Allow uas_use_uas_driver to return usb-storage flags

[Hans de Goede]

uas_use_uas_driver may set some US_FL_foo flags during detection, currently
these are stored in a local variable and then throw away, but these may be
of interest to the caller, so add an extra parameter to (optionally) return
the detected flags, and use this in the uas driver.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
Changes in v2:
-Add Cc stable@vger
---
 drivers/usb/storage/uas-detect.h | 6 +-
 drivers/usb/storage/uas.c| 6 +++---
 drivers/usb/storage/usb.c| 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index 9893d69..63ae161 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -51,7 +51,8 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
 }
 
 static int uas_use_uas_driver(struct usb_interface *intf,
- const struct usb_device_id *id)
+ const struct usb_device_id *id,
+ unsigned long *flags_ret)
 {
struct usb_host_endpoint *eps[4] = { };
struct usb_device *udev = interface_to_usbdev(intf);
@@ -132,5 +133,8 @@ static int uas_use_uas_driver(struct usb_interface *intf,
return 0;
}
 
+   if (flags_ret)
+   *flags_ret = flags;
+
return 1;
 }
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6cdabdc..c6109c1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -887,8 +887,9 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
struct Scsi_Host *shost = NULL;
struct uas_dev_info *devinfo;
struct usb_device *udev = interface_to_usbdev(intf);
+   unsigned long dev_flags;
 
-   if (!uas_use_uas_driver(intf, id))
+   if (!uas_use_uas_driver(intf, id, dev_flags))
return -ENODEV;
 
if (uas_switch_interface(udev, intf))
@@ -910,8 +911,7 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo-udev = udev;
devinfo-resetting = 0;
devinfo-shutdown = 0;
-   devinfo-flags = id-driver_info;
-   usb_stor_adjust_quirks(udev, devinfo-flags);
+   devinfo-flags = dev_flags;
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 5600c33..db6f6b5 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1080,7 +1080,7 @@ static int storage_probe(struct usb_interface *intf,
 
/* If uas is enabled and this device can do uas then ignore it. */
 #if IS_ENABLED(CONFIG_USB_UAS)
-   if (uas_use_uas_driver(intf, id))
+   if (uas_use_uas_driver(intf, id, NULL))
return -ENXIO;
 #endif
 
-- 
2.3.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://vger.kernel.org/majordomo-info.html

[PATCH 1/3] uas: Allow uas_use_uas_driver to return usb-storage flags

[Hans de Goede]

uas_use_uas_driver may set some US_FL_foo flags during detection, currently
these are stored in a local variable and then throw away, but these may be
of interest to the caller, so add an extra parameter to (optionally) return
the detected flags, and use this in the uas driver.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas-detect.h | 6 +-
 drivers/usb/storage/uas.c| 6 +++---
 drivers/usb/storage/usb.c| 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index 9893d69..63ae161 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -51,7 +51,8 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
 }
 
 static int uas_use_uas_driver(struct usb_interface *intf,
- const struct usb_device_id *id)
+ const struct usb_device_id *id,
+ unsigned long *flags_ret)
 {
struct usb_host_endpoint *eps[4] = { };
struct usb_device *udev = interface_to_usbdev(intf);
@@ -132,5 +133,8 @@ static int uas_use_uas_driver(struct usb_interface *intf,
return 0;
}
 
+   if (flags_ret)
+   *flags_ret = flags;
+
return 1;
 }
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6cdabdc..c6109c1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -887,8 +887,9 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
struct Scsi_Host *shost = NULL;
struct uas_dev_info *devinfo;
struct usb_device *udev = interface_to_usbdev(intf);
+   unsigned long dev_flags;
 
-   if (!uas_use_uas_driver(intf, id))
+   if (!uas_use_uas_driver(intf, id, dev_flags))
return -ENODEV;
 
if (uas_switch_interface(udev, intf))
@@ -910,8 +911,7 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo-udev = udev;
devinfo-resetting = 0;
devinfo-shutdown = 0;
-   devinfo-flags = id-driver_info;
-   usb_stor_adjust_quirks(udev, devinfo-flags);
+   devinfo-flags = dev_flags;
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 5600c33..db6f6b5 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1080,7 +1080,7 @@ static int storage_probe(struct usb_interface *intf,
 
/* If uas is enabled and this device can do uas then ignore it. */
 #if IS_ENABLED(CONFIG_USB_UAS)
-   if (uas_use_uas_driver(intf, id))
+   if (uas_use_uas_driver(intf, id, NULL))
return -ENXIO;
 #endif
 
-- 
2.3.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://vger.kernel.org/majordomo-info.html

[PATCH 3/3] uas: Set max_sectors_240 quirk for ASM1053 devices

[Hans de Goede]

Testing has shown that ASM1053 devices do not work properly with transfers
larger than 240 sectors, so set max_sectors to 240 on these.

Reported-by: Steve Bangert sbang...@frontier.com
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas-detect.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index 63ae161..f58caa9 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -74,7 +74,7 @@ static int uas_use_uas_driver(struct usb_interface *intf,
 * this writing the following versions exist:
 * ASM1051 - no uas support version
 * ASM1051 - with broken (*) uas support
-* ASM1053 - with working uas support
+* ASM1053 - with working uas support, but problems with large xfers
 * ASM1153 - with working uas support
 *
 * Devices with these chips re-use a number of device-ids over the
@@ -104,6 +104,9 @@ static int uas_use_uas_driver(struct usb_interface *intf,
} else if (usb_ss_max_streams(eps[1]-ss_ep_comp) == 32) {
/* Possibly an ASM1051, disable uas */
flags |= US_FL_IGNORE_UAS;
+   } else {
+   /* ASM1053, these have issues with large transfers */
+   flags |= US_FL_MAX_SECTORS_240;
}
}
 
-- 
2.3.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://vger.kernel.org/majordomo-info.html

[PATCH 2/3] uas: Add US_FL_MAX_SECTORS_240 flag

[Hans de Goede]

The usb-storage driver sets max_sectors = 240 in its scsi-host template, for
uas we do not want to do that for all devices, but testing has shown that
some devices need it.

This commit adds a US_FL_MAX_SECTORS_240 flag for such devices, and implements
support for it in uas.c, while at it it also adds support for
US_FL_MAX_SECTORS_64 to uas.c.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 10 +-
 drivers/usb/storage/usb.c |  6 +-
 include/linux/usb_usual.h |  2 ++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index c6109c1..6d3122a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -759,7 +759,10 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 
 static int uas_slave_alloc(struct scsi_device *sdev)
 {
-   sdev-hostdata = (void *)sdev-host-hostdata;
+   struct uas_dev_info *devinfo =
+   (struct uas_dev_info *)sdev-host-hostdata;
+
+   sdev-hostdata = devinfo;
 
/* USB has unusual DMA-alignment requirements: Although the
 * starting address of each scatter-gather element doesn't matter,
@@ -778,6 +781,11 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 */
blk_queue_update_dma_alignment(sdev-request_queue, (512 - 1));
 
+   if (devinfo-flags  US_FL_MAX_SECTORS_64)
+   blk_queue_max_hw_sectors(sdev-request_queue, 64);
+   else if (devinfo-flags  US_FL_MAX_SECTORS_240)
+   blk_queue_max_hw_sectors(sdev-request_queue, 240);
+
return 0;
 }
 
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index db6f6b5..6c10c88 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -479,7 +479,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
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_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
+   US_FL_MAX_SECTORS_240);
 
p = quirks;
while (*p) {
@@ -520,6 +521,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
case 'f':
f |= US_FL_NO_REPORT_OPCODES;
break;
+   case 'g':
+   f |= US_FL_MAX_SECTORS_240;
+   break;
case 'h':
f |= US_FL_CAPACITY_HEURISTICS;
break;
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index a7f2604..7f5f78b 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -77,6 +77,8 @@
/* Cannot handle ATA_12 or ATA_16 CDBs */   \
US_FLAG(NO_REPORT_OPCODES,  0x0400) \
/* Cannot handle MI_REPORT_SUPPORTED_OPERATION_CODES */ \
+   US_FLAG(MAX_SECTORS_240,0x0800) \
+   /* Sets max_sectors to 240 */   \
 
 #define US_FLAG(name, value)   US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.3.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://vger.kernel.org/majordomo-info.html

[PATCH] uas: Add US_FL_NO_ATA_1X for Initio Corporation controllers / devices

[Hans de Goede]

A new uas compatible controller has shown up in some people's devices from
the manufacturer Initio Corporation, this controller needs the US_FL_NO_ATA_1X
quirk to work properly with uas, so add it to the uas quirks table.

Reported-and-tested-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
Cc: Benjamin Tissoires benjamin.tissoi...@redhat.com
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 8257042..c85ea53 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -113,6 +113,13 @@ UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Benjamin Tissoires benjamin.tissoi...@redhat.com */
+UNUSUAL_DEV(0x13fd, 0x3940, 0x, 0x,
+   Initio Corporation,
+   ,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* Reported-by: Tom Arild Naess tana...@gmail.com */
 UNUSUAL_DEV(0x152d, 0x0539, 0x, 0x,
JMicron,
-- 
2.3.1

--
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

[PATCH] uas: Add US_FL_NO_REPORT_OPCODES for JMicron JMS539

[Hans de Goede]

Like the JMicron JMS567 enclosures with the JMS539 choke on report-opcodes,
so avoid it.

Tested-and-reported-by: Tom Arild Naess tana...@gmail.com
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index dbc00e5..8257042 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -113,6 +113,13 @@ UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Tom Arild Naess tana...@gmail.com */
+UNUSUAL_DEV(0x152d, 0x0539, 0x, 0x,
+   JMicron,
+   JMS539,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_REPORT_OPCODES),
+
 /* Reported-by: Claudio Bizzarri claudio.bizza...@gmail.com */
 UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
JMicron,
-- 
2.3.0

--
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

Re: [PATCH v4 10/11] ata: ahci_platform: adjust module reference for scsi host

[Hans de Goede]

Hi,

On 18-01-15 16:06, Akinobu Mita wrote:

The ahci platform drivers depend on libahci_platform module.  The
module reference of these scsi host is initialized to libahci_platform's
one.  Because these drivers use ahci_platform_init_host() which is
defined in libahci_platform module and calls scsi_host_alloc()
internally.  So these drivers can be unloaded even if the scsi device
is being accessed.

This fixes it by converting into ahci_platform_init_host() macro so that
these drivers can pass their correct module reference through
__ata_host_alloc_pinfo().

Signed-off-by: Akinobu Mita akinobu.m...@gmail.com
Cc: Hans de Goede hdego...@redhat.com
Cc: Tejun Heo t...@kernel.org
Cc: Christoph Hellwig h...@lst.de
Cc: James E.J. Bottomley jbottom...@parallels.com
Cc: linux-...@vger.kernel.org
Cc: linux-scsi@vger.kernel.org


Looks good to me:

Acked-by: Hans de Goede hdego...@redhat.com

Regards,

Hans


---
  drivers/ata/libahci_platform.c | 14 --
  include/linux/ahci_platform.h  |  9 ++---
  2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 0b03f90..ea75416 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -395,10 +395,11 @@ err_out:
  EXPORT_SYMBOL_GPL(ahci_platform_get_resources);

  /**
- * ahci_platform_init_host - Bring up an ahci-platform host
+ * __ahci_platform_init_host - Bring up an ahci-platform host
   * @pdev: platform device pointer for the host
   * @hpriv: ahci-host private data for the host
   * @pi_template: template for the ata_port_info to use
+ * @owner: module which will be the owner of the ahci-platform host
   *
   * This function does all the usual steps needed to bring up an
   * ahci-platform host, note any necessary resources (ie clks, phys, etc.)
@@ -407,9 +408,10 @@ EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
   * RETURNS:
   * 0 on success otherwise a negative error code
   */
-int ahci_platform_init_host(struct platform_device *pdev,
-   struct ahci_host_priv *hpriv,
-   const struct ata_port_info *pi_template)
+int __ahci_platform_init_host(struct platform_device *pdev,
+ struct ahci_host_priv *hpriv,
+ const struct ata_port_info *pi_template,
+ struct module *owner)
  {
struct device *dev = pdev-dev;
struct ata_port_info pi = *pi_template;
@@ -443,7 +445,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
 */
n_ports = max(ahci_nr_ports(hpriv-cap), fls(hpriv-port_map));

-   host = ata_host_alloc_pinfo(dev, ppi, n_ports);
+   host = __ata_host_alloc_pinfo(dev, ppi, n_ports, owner);
if (!host)
return -ENOMEM;

@@ -495,7 +497,7 @@ int ahci_platform_init_host(struct platform_device *pdev,

return ahci_host_activate(host, irq, ahci_platform_sht);
  }
-EXPORT_SYMBOL_GPL(ahci_platform_init_host);
+EXPORT_SYMBOL_GPL(__ahci_platform_init_host);

  static void ahci_host_stop(struct ata_host *host)
  {
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 642d6ae..0dcac84 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -28,9 +28,12 @@ int ahci_platform_enable_resources(struct ahci_host_priv 
*hpriv);
  void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
  struct ahci_host_priv *ahci_platform_get_resources(
struct platform_device *pdev);
-int ahci_platform_init_host(struct platform_device *pdev,
-   struct ahci_host_priv *hpriv,
-   const struct ata_port_info *pi_template);
+int __ahci_platform_init_host(struct platform_device *pdev,
+ struct ahci_host_priv *hpriv,
+ const struct ata_port_info *pi_template,
+ struct module *owner);
+#define ahci_platform_init_host(pdev, hpriv, pi_template) \
+   __ahci_platform_init_host(pdev, hpriv, pi_template, THIS_MODULE)

  int ahci_platform_suspend_host(struct device *dev);
  int ahci_platform_resume_host(struct device *dev);


--
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

[PATCH] uas: Add no-report-opcodes quirk for Simpletech devices with id 4971:8017

[Hans de Goede]

Like some other uas devices these devices hang when a report-opcodes scsi
command is send to them.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1124119
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 9ec4561..da3d98c 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -130,3 +130,10 @@ UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x,
External HDD,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_IGNORE_UAS),
+
+/* Reported-by: Richard Henderson r...@redhat.com */
+UNUSUAL_DEV(0x4971, 0x8017, 0x, 0x,
+   SimpleTech,
+   External HDD,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_REPORT_OPCODES),
-- 
2.1.0

--
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

[PATCH 2/2] uas: Add US_FL_NO_ATA_1X for 2 more Seagate disk enclosures

[Hans de Goede]

Just like all previous UAS capable Seagate disk enclosures, these need the
US_FL_NO_ATA_1X to not crash when udev probes them.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index f8492c1..9ec4561 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -75,6 +75,13 @@ UNUSUAL_DEV(0x0bc2, 0xa013, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Hans de Goede hdego...@redhat.com */
+UNUSUAL_DEV(0x0bc2, 0xa0a4, 0x, 0x,
+   Seagate,
+   Backup Plus Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* https://bbs.archlinux.org/viewtopic.php?id=183190 */
 UNUSUAL_DEV(0x0bc2, 0xab20, 0x, 0x,
Seagate,
@@ -89,6 +96,13 @@ UNUSUAL_DEV(0x0bc2, 0xab21, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: G. Richard Bellamy rbell...@pteradigm.com */
+UNUSUAL_DEV(0x0bc2, 0xab2a, 0x, 0x,
+   Seagate,
+   BUP Fast HDD,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* Reported-by: Claudio Bizzarri claudio.bizza...@gmail.com */
 UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
JMicron,
-- 
2.1.0

--
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

[PATCH 1/2] uas: Do not blacklist ASM1153 disk enclosures

[Hans de Goede]

Our detection logic to avoid doing UAS on ASM1051 bridge chips causes problems
with newer ASM1153 disk enclosures in 2 ways:

1) Some ASM1153 disk enclosures re-use the ASM1051 device-id of 5106, which
   we assume is always an ASM1051, so remove the quirk for 5106, and instead
   use the same detection logic as we already use for device-id 55aa, which is
   used for all of ASM1051, ASM1053 and ASM1153 devices sigh.

2) Our detection logic to differentiate between ASM1051 and ASM1053 sees
   ASM1153 devices as ASM1051 because they have 32 streams like ASM1051 devs.
   Luckily the ASM1153 descriptors are not 100% identical, unlike the previous
   models the ASM1153 has bMaxPower == 0, so use that to differentiate it.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas-detect.h  | 33 -
 drivers/usb/storage/unusual_uas.h |  8 
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index 8a6f371..9893d69 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -69,16 +69,39 @@ static int uas_use_uas_driver(struct usb_interface *intf,
return 0;
 
/*
-* ASM1051 and older ASM1053 devices have the same usb-id, and UAS is
-* broken on the ASM1051, use the number of streams to differentiate.
-* New ASM1053-s also support 32 streams, but have a different prod-id.
+* ASMedia has a number of usb3 to sata bridge chips, at the time of
+* this writing the following versions exist:
+* ASM1051 - no uas support version
+* ASM1051 - with broken (*) uas support
+* ASM1053 - with working uas support
+* ASM1153 - with working uas support
+*
+* Devices with these chips re-use a number of device-ids over the
+* entire line, so the device-id is useless to determine if we're
+* dealing with an ASM1051 (which we want to avoid).
+*
+* The ASM1153 can be identified by config.MaxPower == 0,
+* where as the ASM105x models have config.MaxPower == 36.
+*
+* Differentiating between the ASM1053 and ASM1051 is trickier, when
+* connected over USB-3 we can look at the number of streams supported,
+* ASM1051 supports 32 streams, where as early ASM1053 versions support
+* 16 streams, newer ASM1053-s also support 32 streams, but have a
+* different prod-id.
+*
+* (*) ASM1051 chips do work with UAS with some disks (with the
+* US_FL_NO_REPORT_OPCODES quirk), but are broken with other disks
 */
if (le16_to_cpu(udev-descriptor.idVendor) == 0x174c 
-   le16_to_cpu(udev-descriptor.idProduct) == 0x55aa) {
-   if (udev-speed  USB_SPEED_SUPER) {
+   (le16_to_cpu(udev-descriptor.idProduct) == 0x5106 ||
+le16_to_cpu(udev-descriptor.idProduct) == 0x55aa)) {
+   if (udev-actconfig-desc.bMaxPower == 0) {
+   /* ASM1153, do nothing */
+   } else if (udev-speed  USB_SPEED_SUPER) {
/* No streams info, assume ASM1051 */
flags |= US_FL_IGNORE_UAS;
} else if (usb_ss_max_streams(eps[1]-ss_ep_comp) == 32) {
+   /* Possibly an ASM1051, disable uas */
flags |= US_FL_IGNORE_UAS;
}
}
diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 2f0a3d3..f8492c1 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -96,14 +96,6 @@ UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_REPORT_OPCODES),
 
-/* Most ASM1051 based devices have issues with uas, blacklist them all */
-/* Reported-by: Hans de Goede hdego...@redhat.com */
-UNUSUAL_DEV(0x174c, 0x5106, 0x, 0x,
-   ASMedia,
-   ASM1051,
-   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
-   US_FL_IGNORE_UAS),
-
 /* Reported-by: Hans de Goede hdego...@redhat.com */
 UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x,
VIA,
-- 
2.1.0

--
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

Re: UAS crash with Apricorn USB3 SATA bridge

[Hans de Goede]

Hi,

On 11-12-14 09:37, Darrick J. Wong wrote:

On Wed, Dec 10, 2014 at 05:41:54PM -0800, Darrick J. Wong wrote:

On Wed, Dec 10, 2014 at 02:29:29AM -0800, Darrick J. Wong wrote:

On Wed, Dec 10, 2014 at 02:15:14AM -0800, Darrick J. Wong wrote:

On Wed, Dec 10, 2014 at 01:04:58AM -0800, Darrick J. Wong wrote:

On Wed, Dec 10, 2014 at 09:19:04AM +0100, Hans de Goede wrote:

Hi,

On 09-12-14 20:31, Darrick J. Wong wrote:

Hi,

I have an Apricorn USB 3 disk dongle thing that claims to support UAS.
However, the kernel crashes when I plug it in[1].


Yes there are some known issues with uas error handling which are fixed
in 3.18, can you try with a 3.18 kernel please ?


The crash pic was from 3.18.0, blk_mq disabled.  I'll work on getting a fuller
dmesg output.  Looking at the code, it looks like we end up in
queue_bulk_sg_tx() with a sg list that is shorter than num_sgs, so we fall off
the end.


Well, there are (at least) two issues going on here.  The first is that the
SCSI layer passes us zero-length READ10 commands, which is causing this crash.
Zero length means the sglist is empty, so the usb host has nothing to map, and
hence urb-num_mapped_sgs == 0 and the loop goes boom.  I don't know what it
means to send a bulk URB with no buffers, so...

...then I took a tour of how SCSI LLDDs deal with zero-length read/write
commands.  mpt2sas attaches a junk sg and pushes the command out.  libata
detects zero-length READ/WRITE SCSI commands and completes the scsi command
without ever touching hardware.  I wasn't able to get any of my parallel SCSI
disks to boot, so I could not try that.

The other problem is when I plug in a different disk (same mfg/model), READ
CAPACITY 16 intermittently returns the string USBSUSBSUSBS, which of course
is garbage.  The kernel then tries to use these values; fortunately, it rejects
a sector size of 1431519827 (USBS) and sets the size to zero.


It turns out that this dongle will return USBSUSBSUSB to just about
*any* command, such as READ10.  In fact, that's the root cause of the
crash.  The partition code issues a 4k read to the disk (looking for
partition tables).  The dongle returns USBSUSBSUSB (13 bytes) which
causes the bio to be advanced by 13 bytes because the URB's
actual_length is stuffed into the SCSI resid(ual length) field.  The
block layer code now wants to read 4083 bytes starting at byte 13,
which, results in 3584 bytes being read ... to somewhere.  This leaves
499 bytes in the bio, which is rounded down to 0 sectors, and thus we
crash on a zero-length READ10 when we try to read the remaining piece
and there's no sg to land the data.  Worse yet, if you somehow patch
all *that* up, now the reader sees USBSUSBSUSB when the bio completes.

Let's disable UAS on this thing entirely.  (Well, you /could/ hack it
to detect USBSUSBSUSB and fail the SCSI command entirely, but... meh.)

Though we should shortcut a zero-length read to avoid crashing the
kernel, since sg_raw can issue such commands.

Patches soon,


Thanks for all the time you've spend on this!

Disabling uas on this bridge sounds like the right thing to do to me.

A patch to not crash on 0 byte scsi data commands would indeed be welcome too.

Regards,

Hans







--D


So, I can code up a couple of patches -- one to teach UAS how to deal with zero
length read and writes; and a second patch to set US_FL_IGNORE_UAS on Apricorn
bridges.  I tried setting US_FL_NO_READ_CAPACITY_16, but for whatever reason
sd.c was still trying RC16.

--D



(Alas it's now 1am here, so I'm going to bed. :/ )


Eh, nuts to sleeping.  dmesg produces this:

[  231.128074] usbcore: registered new interface driver usb-storage
[  231.133822] usbcore: registered new interface driver uas
[  252.121353] usb 2-4: new SuperSpeed USB device number 2 using xhci_hcd
[  252.136927] scsi host6: uas
[  252.141679] scsi 6:0:0:0: Direct-Access Apricorn  0128 
PQ: 0 ANSI: 6
[  252.145433] sd 6:0:0:0: Attached scsi generic sg2 type 0
[  252.145525] sd 6:0:0:0: [sdc] 312581808 512-byte logical blocks: (160 GB/149 
GiB)
[  252.145527] sd 6:0:0:0: [sdc] 4096-byte physical blocks
[  252.145891] sd 6:0:0:0: [sdc] Write Protect is off
[  252.145973] sd 6:0:0:0: [sdc] No Caching mode page found
[  252.145975] sd 6:0:0:0: [sdc] Assuming drive cache: write through


Huh.  4096-byte physical blocks??  That drive is /not/ a 4k sector drive.
Here's what the kernel said when I plugged in the other (Plugable brand) UAS
bridge[1]:

[   32.466870] usb 2-4: new SuperSpeed USB device number 2 using xhci_hcd
[   32.498996] usbcore: registered new interface driver usb-storage
[   37.660963] scsi host6: uas
[   37.661193] usbcore: registered new interface driver uas
[   37.661292] queue_bulk_sg_tx: num=1 sg=880447764500 addr=45af41000 len=0 
pagelink=ea00116bd042
[   37.661550] queue_bulk_sg_tx: num=1 sg=8804483fb600 addr=45af41000 len=0 
pagelink=ea00116bd042
[   37.661744] scsi 6:0:0:0: Direct-Access Plugable USB3-SATA-UASP1  0

Re: [PATCH] uas: disable UAS on Apricorn SATA dongles

[Hans de Goede]

Adding Greg and stable to the cc

On 11-12-14 20:01, Darrick J. Wong wrote:

The Apricorn SATA dongle will occasionally return USBSUSBSUSB in
response to SCSI commands when running in UAS mode.  Therefore,
disable UAS mode on this dongle.

Signed-off-by: Darrick J. Wong darrick.w...@oracle.com


Looks good:

Acked-by: Hans de Goede hdego...@redhat.com

Greg, can you add this to your tree please ?

Regards,

Hans



---
  drivers/usb/storage/unusual_uas.h |   10 ++
  1 file changed, 10 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 18a283d..3530cb0 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,6 +40,16 @@
   * and don't forget to CC: the USB development list 
linux-...@vger.kernel.org
   */

+/*
+ * Apricorn USB3 dongle sometimes returns USBSUSBSUSBS in response to SCSI
+ * commands in UAS mode.  Observed with the 1.28 firmware; are there others?
+ */
+UNUSUAL_DEV(0x0984, 0x0301, 0x0128, 0x0128,
+   Apricorn,
+   ,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_IGNORE_UAS),
+
  /* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
  UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
Seagate,


--
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

Re: UAS crash with Apricorn USB3 SATA bridge

[Hans de Goede]

Hi,

On 09-12-14 20:31, Darrick J. Wong wrote:

Hi,

I have an Apricorn USB 3 disk dongle thing that claims to support UAS.
However, the kernel crashes when I plug it in[1].


Yes there are some known issues with uas error handling which are fixed
in 3.18, can you try with a 3.18 kernel please ?

Note that the device will likely still not work, but it should no
longer crash things. When running 3.18 please collect the output of
dmesg after plugging in the drive and send that to me, then we'll see
if we can get it to work from there.

Regards,

Hans
--
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

[PATCH] uas: Add US_FL_NO_REPORT_OPCODES for JMicron JMS566 with usb-id 0bc2:a013

[Hans de Goede]

Like the JMicron JMS567 enclosures with the JMS566 choke on report-opcodes,
so avoid it.

Tested-and-reported-by: Takeo Nakayama javh...@gmx.com
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 2918376..2f0a3d3 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -111,6 +111,13 @@ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Takeo Nakayama javh...@gmx.com */
+UNUSUAL_DEV(0x357d, 0x7788, 0x, 0x,
+   JMicron,
+   JMS566,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_REPORT_OPCODES),
+
 /* Reported-by: Hans de Goede hdego...@redhat.com */
 UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x,
Hitachi,
-- 
2.1.0

--
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

[PATCH 2/2] uas: Add US_FL_NO_ATA_1X for Seagate devices with usb-id 0bc2:a013

[Hans de Goede]

This is yet another Seagate device which needs the US_FL_NO_ATA_1X quirk

Reported-by: Marcin Zajączkowski msz...@wp.pl
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 18a283d..2918376 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -68,6 +68,13 @@ UNUSUAL_DEV(0x0bc2, 0xa003, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Marcin Zajączkowski msz...@wp.pl */
+UNUSUAL_DEV(0x0bc2, 0xa013, 0x, 0x,
+   Seagate,
+   Backup Plus,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* https://bbs.archlinux.org/viewtopic.php?id=183190 */
 UNUSUAL_DEV(0x0bc2, 0xab20, 0x, 0x,
Seagate,
-- 
2.1.0

--
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

[PATCH] uas: Add no-uas quirk for Hitachi usb-3 enclosures 4971:1012

[Hans de Goede]

These disks have a broken uas implementation, the tag field of the status
iu-s is not set properly, so we need to fall-back to usb-storage for these.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 2fefaf9..18a283d 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -103,3 +103,10 @@ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x,
VL711,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
+
+/* Reported-by: Hans de Goede hdego...@redhat.com */
+UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x,
+   Hitachi,
+   External HDD,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_IGNORE_UAS),
-- 
2.1.0

--
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

[PATCH] uas: Add US_FL_NO_ATA_1X quirk for 1 more Seagate model

[Hans de Goede]

These drives hang when receiving ATA12 commands, so set the US_FL_NO_ATA_1X
quirk to filter these out.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 44ab3289..2fefaf9 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -75,6 +75,13 @@ UNUSUAL_DEV(0x0bc2, 0xab20, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
+UNUSUAL_DEV(0x0bc2, 0xab21, 0x, 0x,
+   Seagate,
+   Backup+ BK,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* Reported-by: Claudio Bizzarri claudio.bizza...@gmail.com */
 UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
JMicron,
-- 
2.1.0

--
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

[PATCH] uas: Add US_FL_NO_ATA_1X quirk for 2 more Seagate models

[Hans de Goede]

These drives hang when receiving ATA12 commands, so set the US_FL_NO_ATA_1X
quirk to filter these out.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index a801850..44ab3289 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -54,6 +54,20 @@ UNUSUAL_DEV(0x0bc2, 0x3312, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* Reported-by: Hans de Goede hdego...@redhat.com */
+UNUSUAL_DEV(0x0bc2, 0x3320, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
+/* Reported-by: Bogdan Mihalcea bogdan.mihal...@infim.ro */
+UNUSUAL_DEV(0x0bc2, 0xa003, 0x, 0x,
+   Seagate,
+   Backup Plus,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* https://bbs.archlinux.org/viewtopic.php?id=183190 */
 UNUSUAL_DEV(0x0bc2, 0xab20, 0x, 0x,
Seagate,
-- 
2.1.0

--
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

[PATCH v3] uas: Make uas work with blk-mq

[Hans de Goede]

With uas over usb-3 the tags inside the uas iu-s must match the usb-3 stream
ids, and those go from 1 - qdepth.

Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we would
only get cmnd-request-tag from 0 - (qdepth - 1), and we used those as
uas-tags / stream-ids.

With blk-mq however we are guaranteed to never get more then qdepth commands
queued at the same time, but the cmnd-request-tag values may be much larger,
which breaks uas.

This commit fixes this by generating uas tags in the 1 - qdepth range ourselves
instead of using cmnd-request-tag.

While touching all involved code anyways also rename the uas_cmd_info stream
field to uas_tag, because when using uas over usb-2 streams are not used.

Cc: Christoph Hellwig h...@infradead.org
Reported-by: Douglas Gilbert dgilb...@interlog.com
Signed-off-by: Hans de Goede hdego...@redhat.com
Reviewed-by: Christoph Hellwig h...@lst.de

--
Changes in v2:
-Remove .disable_blk_mq = true from uas_host_template
Changes in v3:
-Rebased on top of Linus' current master branch
---
 drivers/usb/storage/uas.c | 64 +--
 1 file changed, 23 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 89b2434..004ebc1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -66,7 +66,7 @@ enum {
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
-   unsigned int stream;
+   unsigned int uas_tag;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -173,30 +173,15 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd 
*cmnd)
cmnd-result = sense_iu-status;
 }
 
-/*
- * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
- * which go from 1 - nr_streams. And we use 1 for untagged commands.
- */
-static int uas_get_tag(struct scsi_cmnd *cmnd)
-{
-   int tag;
-
-   if (blk_rq_tagged(cmnd-request))
-   tag = cmnd-request-tag + 2;
-   else
-   tag = 1;
-
-   return tag;
-}
-
 static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
  int status)
 {
struct uas_cmd_info *ci = (void *)cmnd-SCp;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
 
scmd_printk(KERN_INFO, cmnd,
-   %s %d tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
-   prefix, status, uas_get_tag(cmnd),
+   %s %d uas-tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
+   prefix, status, cmdinfo-uas_tag,
(ci-state  SUBMIT_STATUS_URB) ?  s-st  : ,
(ci-state  ALLOC_DATA_IN_URB) ?  a-in  : ,
(ci-state  SUBMIT_DATA_IN_URB)?  s-in  : ,
@@ -242,7 +227,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
  DATA_OUT_URB_INFLIGHT |
  COMMAND_ABORTED))
return -EBUSY;
-   devinfo-cmnd[uas_get_tag(cmnd) - 1] = NULL;
+   devinfo-cmnd[cmdinfo-uas_tag - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd-scsi_done(cmnd);
return 0;
@@ -289,7 +274,7 @@ static void uas_stat_cmplt(struct urb *urb)
idx = be16_to_cpup(iu-tag) - 1;
if (idx = MAX_CMNDS || !devinfo-cmnd[idx]) {
dev_err(urb-dev-dev,
-   stat urb: no pending cmd for tag %d\n, idx + 1);
+   stat urb: no pending cmd for uas-tag %d\n, idx + 1);
goto out;
}
 
@@ -427,7 +412,8 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto out;
usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb-length,
  uas_data_cmplt, cmnd);
-   urb-stream_id = cmdinfo-stream;
+   if (devinfo-use_streams)
+   urb-stream_id = cmdinfo-uas_tag;
urb-num_sgs = udev-bus-sg_tablesize ? sdb-table.nents : 0;
urb-sg = sdb-table.sgl;
  out:
@@ -451,7 +437,8 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 
usb_fill_bulk_urb(urb, udev, devinfo-status_pipe, iu, sizeof(*iu),
  uas_stat_cmplt, cmnd-device-host);
-   urb-stream_id = cmdinfo-stream;
+   if (devinfo-use_streams)
+   urb-stream_id = cmdinfo-uas_tag;
urb-transfer_flags |= URB_FREE_BUFFER;
  out:
return urb;
@@ -465,6 +452,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 {
struct usb_device *udev = devinfo-udev;
struct scsi_device *sdev = cmnd-device;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct command_iu *iu;
int len;
@@ -481,7 +469,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto

[PATCH] uas: Add NO_ATA_1X for VIA VL711 devices

[Hans de Goede]

Just like some Seagate enclosures, these devices do not seem to grok ata
pass through commands.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 8511b54..a801850 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -75,3 +75,10 @@ UNUSUAL_DEV(0x174c, 0x5106, 0x, 0x,
ASM1051,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_IGNORE_UAS),
+
+/* Reported-by: Hans de Goede hdego...@redhat.com */
+UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x,
+   VIA,
+   VL711,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
-- 
2.1.0

--
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

Re: [PATCH v2 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/04/2014 04:46 PM, James Bottomley wrote:
 On Sat, 2014-10-04 at 10:53 +0200, Hans de Goede wrote:
 Hi,

 On 10/03/2014 11:47 PM, Greg Kroah-Hartman wrote:
 On Fri, Oct 03, 2014 at 12:08:57PM +0200, Hans de Goede wrote:
 With uas over usb-3 the tags inside the uas iu-s must match the usb-3 
 stream
 ids, and those go from 1 - qdepth.

 Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we 
 would
 only get cmnd-request-tag from 0 - (qdepth - 1), and we used those as
 uas-tags / stream-ids.

 With blk-mq however we are guaranteed to never get more then qdepth 
 commands
 queued at the same time, but the cmnd-request-tag values may be much 
 larger,
 which breaks uas.

 This commit fixes this by generating uas tags in the 1 - qdepth range 
 ourselves
 instead of using cmnd-request-tag.

 While touching all involved code anyways also rename the uas_cmd_info 
 stream
 field to uas_tag, because when using uas over usb-2 streams are not used.

 Cc: Christoph Hellwig h...@infradead.org
 Reported-by: Douglas Gilbert dgilb...@interlog.com
 Signed-off-by: Hans de Goede hdego...@redhat.com
 Reviewed-by: Christoph Hellwig h...@lst.de

 This doesn't apply to my usb-next branch of usb.git, care to refresh it
 and resend?

 I did this v2 to resolve a conflict with a last minute fix for 3.17 which
 James Bottomley pushed out yesterday. So if this does not apply that likely
 is because that fix is not (yet) in next. See:

 https://git.kernel.org/cgit/linux/kernel/git/jejb/scsi.git/log/?h=fixes

 And specifically:

 https://git.kernel.org/cgit/linux/kernel/git/jejb/scsi.git/commit/?h=fixesid=2c2d831c81ec75a7b0d8e28caa8e3d9c1fe546f9

 I'm not sure how to proceed here, maybe the best thing is to cherry pick
 that fix into usb-next (should disappear on merge), and then apply
 this one on top ?
 
 Depends:  The fixes tree is based on -rc1, so you could just use that as
 the base for the topic branch uas goes in.  That's the usual way.  Or we
 could just do it via the SCSI tree, which will have the necessary base.
 
 Which would you prefer?

This patch sits on top of a lot of other uas fixes which are already
in usb-next, so this needs to go in through usb-next.

Regards,

Hans
--
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

Re: [PATCH v2 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/05/2014 11:10 AM, Christoph Hellwig wrote:
 On Sun, Oct 05, 2014 at 11:06:00AM +0200, Hans de Goede wrote:
 This patch sits on top of a lot of other uas fixes which are already
 in usb-next, so this needs to go in through usb-next.
 
 What is the actual conflict? If it's just your revert of the
 disable_blk_mq flag

Yes, the conclict is just my revert of the disable_blk_mq flag

 you might want to skip that for this pull request
 and send it on after both scsi and usb trees make it to Linus.

Since the patch with the conclict actually makes the flag unnecessary,
to me this belongs in the same patch.

 If there's more conflict around the host template you could cherry pick
 my patch for your tree.

Yes, Greg cherry picking the patch in question into his tree to resolv
the conflict would be my preferred solution to this, but ultimately
that is Greg's call.

Regards,

Hans
--
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

Re: [PATCH v2 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/03/2014 11:47 PM, Greg Kroah-Hartman wrote:
 On Fri, Oct 03, 2014 at 12:08:57PM +0200, Hans de Goede wrote:
 With uas over usb-3 the tags inside the uas iu-s must match the usb-3 stream
 ids, and those go from 1 - qdepth.

 Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we 
 would
 only get cmnd-request-tag from 0 - (qdepth - 1), and we used those as
 uas-tags / stream-ids.

 With blk-mq however we are guaranteed to never get more then qdepth commands
 queued at the same time, but the cmnd-request-tag values may be much 
 larger,
 which breaks uas.

 This commit fixes this by generating uas tags in the 1 - qdepth range 
 ourselves
 instead of using cmnd-request-tag.

 While touching all involved code anyways also rename the uas_cmd_info stream
 field to uas_tag, because when using uas over usb-2 streams are not used.

 Cc: Christoph Hellwig h...@infradead.org
 Reported-by: Douglas Gilbert dgilb...@interlog.com
 Signed-off-by: Hans de Goede hdego...@redhat.com
 Reviewed-by: Christoph Hellwig h...@lst.de
 
 This doesn't apply to my usb-next branch of usb.git, care to refresh it
 and resend?

I did this v2 to resolve a conflict with a last minute fix for 3.17 which
James Bottomley pushed out yesterday. So if this does not apply that likely
is because that fix is not (yet) in next. See:

https://git.kernel.org/cgit/linux/kernel/git/jejb/scsi.git/log/?h=fixes

And specifically:

https://git.kernel.org/cgit/linux/kernel/git/jejb/scsi.git/commit/?h=fixesid=2c2d831c81ec75a7b0d8e28caa8e3d9c1fe546f9

I'm not sure how to proceed here, maybe the best thing is to cherry pick
that fix into usb-next (should disappear on merge), and then apply
this one on top ?

Regards,

Hans
--
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

Re: [PATCH 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/03/2014 10:47 AM, Christoph Hellwig wrote:
 On Thu, Oct 02, 2014 at 09:25:50AM -0400, James Bottomley wrote:
 OK, so 3.17 should be on sunday giving three days or so to sort this
 out.  If you're happy with blk-mq being disabled so the bug never
 triggers unless the user activates it, doing nothing is my preferred
 outcome ... but that also means no need to backport to stable.  

 If you want a it fixed in 3.17, then we need a patch from you now.
 Ultimately you have to judge the necessity for us, since it's your
 driver.
 
 scsi-mq might be new and not default, but I wouldn't really consider it
 experimental in the sense that people should expect crashes if they
 attach a usb device.
 
 Please consider applying the patch below for 3.17, as there is a cxgb
 patch pending anyway:

For the record, and since I said before that I'm fine with leaving this as
is. As Christoph believes it is important to get this fixed for 3.17, lets
try to get this fix in.

So my Acked-by which is included below still stands.

Regards,

Hans



 
 ---
 From 8a42ea1369701960a821b7fa9a3da5c68e7e4366 Mon Sep 17 00:00:00 2001
 From: Christoph Hellwig h...@lst.de
 Date: Fri, 3 Oct 2014 10:45:10 +0200
 Subject: uas: disable use of blk-mq I/O path
 MIME-Version: 1.0
 Content-Type: text/plain; charset=UTF-8
 Content-Transfer-Encoding: 8bit
 
 The uas driver uses the block layer tag for USB3 stream IDs.  With
 blk-mq we can get larger tag numbers that the queue depth, which breaks
 this assumption.  A fix is under way for 3.18, but sits on top of
 large changes so can't easily be backported.   Set the disable_blk_mq
 path so that a uas device can't easily crash the system when using
 blk-mq for SCSI.
 
 Signed-off-by: Christoph Hellwig h...@lst.de
 Acked-by: Hans de Goede hdego...@redhat.com
 ---
  drivers/usb/storage/uas.c | 7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
 index 3f42785..9bfa725 100644
 --- a/drivers/usb/storage/uas.c
 +++ b/drivers/usb/storage/uas.c
 @@ -970,6 +970,13 @@ static struct scsi_host_template uas_host_template = {
   .cmd_per_lun = 1,   /* until we override it */
   .skip_settle_delay = 1,
   .ordered_tag = 1,
 +
 + /*
 +  * The uas drivers expects tags not to be bigger than the maximum
 +  * per-device queue depth, which is not true with the blk-mq tag
 +  * allocator.
 +  */
 + .disable_blk_mq = true,
  };
  
  #define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
 
--
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

[PATCH v2 1/2] uas: Reduce number of function arguments for uas_alloc_foo functions

[Hans de Goede]

The stream_id and pipe are already present in uas_cmd_info resp uas_dev_info,
so there is no need to pass a copy along.

Signed-off-by: Hans de Goede hdego...@redhat.com
Reviewed-by: Christoph Hellwig h...@lst.de
---
 drivers/usb/storage/uas.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f62ef61..89b2434 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -412,20 +412,22 @@ static void uas_cmd_cmplt(struct urb *urb)
 }
 
 static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
- unsigned int pipe, u16 stream_id,
  struct scsi_cmnd *cmnd,
  enum dma_data_direction dir)
 {
struct usb_device *udev = devinfo-udev;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct scsi_data_buffer *sdb = (dir == DMA_FROM_DEVICE)
? scsi_in(cmnd) : scsi_out(cmnd);
+   unsigned int pipe = (dir == DMA_FROM_DEVICE)
+   ? devinfo-data_in_pipe : devinfo-data_out_pipe;
 
if (!urb)
goto out;
usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb-length,
  uas_data_cmplt, cmnd);
-   urb-stream_id = stream_id;
+   urb-stream_id = cmdinfo-stream;
urb-num_sgs = udev-bus-sg_tablesize ? sdb-table.nents : 0;
urb-sg = sdb-table.sgl;
  out:
@@ -433,9 +435,10 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 }
 
 static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
-  struct Scsi_Host *shost, u16 stream_id)
+  struct scsi_cmnd *cmnd)
 {
struct usb_device *udev = devinfo-udev;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct sense_iu *iu;
 
@@ -447,8 +450,8 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto free;
 
usb_fill_bulk_urb(urb, udev, devinfo-status_pipe, iu, sizeof(*iu),
-   uas_stat_cmplt, shost);
-   urb-stream_id = stream_id;
+ uas_stat_cmplt, cmnd-device-host);
+   urb-stream_id = cmdinfo-stream;
urb-transfer_flags |= URB_FREE_BUFFER;
  out:
return urb;
@@ -500,15 +503,13 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
  * daft to me.
  */
 
-static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd,
-   gfp_t gfp, unsigned int stream)
+static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp)
 {
-   struct Scsi_Host *shost = cmnd-device-host;
-   struct uas_dev_info *devinfo = (struct uas_dev_info *)shost-hostdata;
+   struct uas_dev_info *devinfo = cmnd-device-hostdata;
struct urb *urb;
int err;
 
-   urb = uas_alloc_sense_urb(devinfo, gfp, shost, stream);
+   urb = uas_alloc_sense_urb(devinfo, gfp, cmnd);
if (!urb)
return NULL;
usb_anchor_urb(urb, devinfo-sense_urbs);
@@ -531,7 +532,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
lockdep_assert_held(devinfo-lock);
if (cmdinfo-state  SUBMIT_STATUS_URB) {
-   urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo-stream);
+   urb = uas_submit_sense_urb(cmnd, gfp);
if (!urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo-state = ~SUBMIT_STATUS_URB;
@@ -539,8 +540,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
if (cmdinfo-state  ALLOC_DATA_IN_URB) {
cmdinfo-data_in_urb = uas_alloc_data_urb(devinfo, gfp,
-   devinfo-data_in_pipe, cmdinfo-stream,
-   cmnd, DMA_FROM_DEVICE);
+   cmnd, DMA_FROM_DEVICE);
if (!cmdinfo-data_in_urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo-state = ~ALLOC_DATA_IN_URB;
@@ -560,8 +560,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
if (cmdinfo-state  ALLOC_DATA_OUT_URB) {
cmdinfo-data_out_urb = uas_alloc_data_urb(devinfo, gfp,
-   devinfo-data_out_pipe, cmdinfo-stream,
-   cmnd, DMA_TO_DEVICE);
+   cmnd, DMA_TO_DEVICE);
if (!cmdinfo-data_out_urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo-state = ~ALLOC_DATA_OUT_URB;
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux

[PATCH v2 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

With uas over usb-3 the tags inside the uas iu-s must match the usb-3 stream
ids, and those go from 1 - qdepth.

Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we would
only get cmnd-request-tag from 0 - (qdepth - 1), and we used those as
uas-tags / stream-ids.

With blk-mq however we are guaranteed to never get more then qdepth commands
queued at the same time, but the cmnd-request-tag values may be much larger,
which breaks uas.

This commit fixes this by generating uas tags in the 1 - qdepth range ourselves
instead of using cmnd-request-tag.

While touching all involved code anyways also rename the uas_cmd_info stream
field to uas_tag, because when using uas over usb-2 streams are not used.

Cc: Christoph Hellwig h...@infradead.org
Reported-by: Douglas Gilbert dgilb...@interlog.com
Signed-off-by: Hans de Goede hdego...@redhat.com
Reviewed-by: Christoph Hellwig h...@lst.de

--
Changes in v2:
-Remove .disable_blk_mq = true from uas_host_template
---
 drivers/usb/storage/uas.c | 64 +--
 1 file changed, 23 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 89b2434..004ebc1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -66,7 +66,7 @@ enum {
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
-   unsigned int stream;
+   unsigned int uas_tag;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -173,30 +173,15 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd 
*cmnd)
cmnd-result = sense_iu-status;
 }
 
-/*
- * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
- * which go from 1 - nr_streams. And we use 1 for untagged commands.
- */
-static int uas_get_tag(struct scsi_cmnd *cmnd)
-{
-   int tag;
-
-   if (blk_rq_tagged(cmnd-request))
-   tag = cmnd-request-tag + 2;
-   else
-   tag = 1;
-
-   return tag;
-}
-
 static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
  int status)
 {
struct uas_cmd_info *ci = (void *)cmnd-SCp;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
 
scmd_printk(KERN_INFO, cmnd,
-   %s %d tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
-   prefix, status, uas_get_tag(cmnd),
+   %s %d uas-tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
+   prefix, status, cmdinfo-uas_tag,
(ci-state  SUBMIT_STATUS_URB) ?  s-st  : ,
(ci-state  ALLOC_DATA_IN_URB) ?  a-in  : ,
(ci-state  SUBMIT_DATA_IN_URB)?  s-in  : ,
@@ -242,7 +227,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
  DATA_OUT_URB_INFLIGHT |
  COMMAND_ABORTED))
return -EBUSY;
-   devinfo-cmnd[uas_get_tag(cmnd) - 1] = NULL;
+   devinfo-cmnd[cmdinfo-uas_tag - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd-scsi_done(cmnd);
return 0;
@@ -289,7 +274,7 @@ static void uas_stat_cmplt(struct urb *urb)
idx = be16_to_cpup(iu-tag) - 1;
if (idx = MAX_CMNDS || !devinfo-cmnd[idx]) {
dev_err(urb-dev-dev,
-   stat urb: no pending cmd for tag %d\n, idx + 1);
+   stat urb: no pending cmd for uas-tag %d\n, idx + 1);
goto out;
}
 
@@ -427,7 +412,8 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto out;
usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb-length,
  uas_data_cmplt, cmnd);
-   urb-stream_id = cmdinfo-stream;
+   if (devinfo-use_streams)
+   urb-stream_id = cmdinfo-uas_tag;
urb-num_sgs = udev-bus-sg_tablesize ? sdb-table.nents : 0;
urb-sg = sdb-table.sgl;
  out:
@@ -451,7 +437,8 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 
usb_fill_bulk_urb(urb, udev, devinfo-status_pipe, iu, sizeof(*iu),
  uas_stat_cmplt, cmnd-device-host);
-   urb-stream_id = cmdinfo-stream;
+   if (devinfo-use_streams)
+   urb-stream_id = cmdinfo-uas_tag;
urb-transfer_flags |= URB_FREE_BUFFER;
  out:
return urb;
@@ -465,6 +452,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 {
struct usb_device *udev = devinfo-udev;
struct scsi_device *sdev = cmnd-device;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct command_iu *iu;
int len;
@@ -481,7 +469,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto free;
 
iu-iu_id = IU_ID_COMMAND;
-   iu-tag

[PATCH 1/2] uas: Reduce number of function arguments for uas_alloc_foo functions

[Hans de Goede]

The stream_id and pipe are already present in uas_cmd_info resp uas_dev_info,
so there is no need to pass a copy along.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b27fe21..d1dbe88 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -412,20 +412,22 @@ static void uas_cmd_cmplt(struct urb *urb)
 }
 
 static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
- unsigned int pipe, u16 stream_id,
  struct scsi_cmnd *cmnd,
  enum dma_data_direction dir)
 {
struct usb_device *udev = devinfo-udev;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct scsi_data_buffer *sdb = (dir == DMA_FROM_DEVICE)
? scsi_in(cmnd) : scsi_out(cmnd);
+   unsigned int pipe = (dir == DMA_FROM_DEVICE)
+   ? devinfo-data_in_pipe : devinfo-data_out_pipe;
 
if (!urb)
goto out;
usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb-length,
  uas_data_cmplt, cmnd);
-   urb-stream_id = stream_id;
+   urb-stream_id = cmdinfo-stream;
urb-num_sgs = udev-bus-sg_tablesize ? sdb-table.nents : 0;
urb-sg = sdb-table.sgl;
  out:
@@ -433,9 +435,10 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 }
 
 static struct urb *uas_alloc_sense_urb(struct uas_dev_info *devinfo, gfp_t gfp,
-  struct Scsi_Host *shost, u16 stream_id)
+  struct scsi_cmnd *cmnd)
 {
struct usb_device *udev = devinfo-udev;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct sense_iu *iu;
 
@@ -447,8 +450,8 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto free;
 
usb_fill_bulk_urb(urb, udev, devinfo-status_pipe, iu, sizeof(*iu),
-   uas_stat_cmplt, shost);
-   urb-stream_id = stream_id;
+ uas_stat_cmplt, cmnd-device-host);
+   urb-stream_id = cmdinfo-stream;
urb-transfer_flags |= URB_FREE_BUFFER;
  out:
return urb;
@@ -500,15 +503,13 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
  * daft to me.
  */
 
-static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd,
-   gfp_t gfp, unsigned int stream)
+static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp)
 {
-   struct Scsi_Host *shost = cmnd-device-host;
-   struct uas_dev_info *devinfo = (struct uas_dev_info *)shost-hostdata;
+   struct uas_dev_info *devinfo = cmnd-device-hostdata;
struct urb *urb;
int err;
 
-   urb = uas_alloc_sense_urb(devinfo, gfp, shost, stream);
+   urb = uas_alloc_sense_urb(devinfo, gfp, cmnd);
if (!urb)
return NULL;
usb_anchor_urb(urb, devinfo-sense_urbs);
@@ -531,7 +532,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
lockdep_assert_held(devinfo-lock);
if (cmdinfo-state  SUBMIT_STATUS_URB) {
-   urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo-stream);
+   urb = uas_submit_sense_urb(cmnd, gfp);
if (!urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo-state = ~SUBMIT_STATUS_URB;
@@ -539,8 +540,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
if (cmdinfo-state  ALLOC_DATA_IN_URB) {
cmdinfo-data_in_urb = uas_alloc_data_urb(devinfo, gfp,
-   devinfo-data_in_pipe, cmdinfo-stream,
-   cmnd, DMA_FROM_DEVICE);
+   cmnd, DMA_FROM_DEVICE);
if (!cmdinfo-data_in_urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo-state = ~ALLOC_DATA_IN_URB;
@@ -560,8 +560,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
if (cmdinfo-state  ALLOC_DATA_OUT_URB) {
cmdinfo-data_out_urb = uas_alloc_data_urb(devinfo, gfp,
-   devinfo-data_out_pipe, cmdinfo-stream,
-   cmnd, DMA_TO_DEVICE);
+   cmnd, DMA_TO_DEVICE);
if (!cmdinfo-data_out_urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo-state = ~ALLOC_DATA_OUT_URB;
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord

[PATCH 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

With uas over usb-3 the tags inside the uas iu-s must match the usb-3 stream
ids, and those go from 1 - qdepth.

Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we would
only get cmnd-request-tag from 0 - (qdepth - 1), and we used those as
uas-tags / stream-ids.

With blk-mq however we are guaranteed to never get more then qdepth commands
queued at the same time, but the cmnd-request-tag values may be much larger,
which breaks uas.

This commit fixes this by generating uas tags in the 1 - qdepth range ourselves
instead of using cmnd-request-tag.

While touching all involved code anyways also rename the uas_cmd_info stream
field to uas_tag, because when using uas over usb-2 streams are not used.

Cc: Christoph Hellwig h...@infradead.org
Reported-by: Douglas Gilbert dgilb...@interlog.com
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 57 +++
 1 file changed, 23 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d1dbe88..004ebc1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -66,7 +66,7 @@ enum {
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
-   unsigned int stream;
+   unsigned int uas_tag;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -173,30 +173,15 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd 
*cmnd)
cmnd-result = sense_iu-status;
 }
 
-/*
- * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
- * which go from 1 - nr_streams. And we use 1 for untagged commands.
- */
-static int uas_get_tag(struct scsi_cmnd *cmnd)
-{
-   int tag;
-
-   if (blk_rq_tagged(cmnd-request))
-   tag = cmnd-request-tag + 2;
-   else
-   tag = 1;
-
-   return tag;
-}
-
 static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
  int status)
 {
struct uas_cmd_info *ci = (void *)cmnd-SCp;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
 
scmd_printk(KERN_INFO, cmnd,
-   %s %d tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
-   prefix, status, uas_get_tag(cmnd),
+   %s %d uas-tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
+   prefix, status, cmdinfo-uas_tag,
(ci-state  SUBMIT_STATUS_URB) ?  s-st  : ,
(ci-state  ALLOC_DATA_IN_URB) ?  a-in  : ,
(ci-state  SUBMIT_DATA_IN_URB)?  s-in  : ,
@@ -242,7 +227,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
  DATA_OUT_URB_INFLIGHT |
  COMMAND_ABORTED))
return -EBUSY;
-   devinfo-cmnd[uas_get_tag(cmnd) - 1] = NULL;
+   devinfo-cmnd[cmdinfo-uas_tag - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd-scsi_done(cmnd);
return 0;
@@ -289,7 +274,7 @@ static void uas_stat_cmplt(struct urb *urb)
idx = be16_to_cpup(iu-tag) - 1;
if (idx = MAX_CMNDS || !devinfo-cmnd[idx]) {
dev_err(urb-dev-dev,
-   stat urb: no pending cmd for tag %d\n, idx + 1);
+   stat urb: no pending cmd for uas-tag %d\n, idx + 1);
goto out;
}
 
@@ -427,7 +412,8 @@ static struct urb *uas_alloc_data_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto out;
usb_fill_bulk_urb(urb, udev, pipe, NULL, sdb-length,
  uas_data_cmplt, cmnd);
-   urb-stream_id = cmdinfo-stream;
+   if (devinfo-use_streams)
+   urb-stream_id = cmdinfo-uas_tag;
urb-num_sgs = udev-bus-sg_tablesize ? sdb-table.nents : 0;
urb-sg = sdb-table.sgl;
  out:
@@ -451,7 +437,8 @@ static struct urb *uas_alloc_sense_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 
usb_fill_bulk_urb(urb, udev, devinfo-status_pipe, iu, sizeof(*iu),
  uas_stat_cmplt, cmnd-device-host);
-   urb-stream_id = cmdinfo-stream;
+   if (devinfo-use_streams)
+   urb-stream_id = cmdinfo-uas_tag;
urb-transfer_flags |= URB_FREE_BUFFER;
  out:
return urb;
@@ -465,6 +452,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
 {
struct usb_device *udev = devinfo-udev;
struct scsi_device *sdev = cmnd-device;
+   struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct urb *urb = usb_alloc_urb(0, gfp);
struct command_iu *iu;
int len;
@@ -481,7 +469,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info 
*devinfo, gfp_t gfp,
goto free;
 
iu-iu_id = IU_ID_COMMAND;
-   iu-tag = cpu_to_be16(uas_get_tag(cmnd));
+   iu-tag = cpu_to_be16(cmdinfo-uas_tag);
iu-prio_attr = UAS_SIMPLE_TAG

Re: [PATCH 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/02/2014 01:26 PM, Christoph Hellwig wrote:
 Looks fine to me (and actually removes code..)
 
 Reviewed-by: Christoph Hellwig h...@lst.de
 
 It's fairly late in 3.17 to get something like this in, but would you
 consider Ccing it to stable so we can get it into the first 3.17 stable
 release?

This patch sits on top of a whole bunch of uas cleanups / fixes which
are going into 3.18, and which are to big of a change to backport.

Regards,

Hans
--
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

Re: [PATCH 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/02/2014 03:06 PM, Christoph Hellwig wrote:
 On Thu, Oct 02, 2014 at 02:35:10PM +0200, Hans de Goede wrote:
 This patch sits on top of a whole bunch of uas cleanups / fixes which
 are going into 3.18, and which are to big of a change to backport.
 
 Can add a patch to set disable_blk_mq for uas on 3.17 then?

Yes, although that likely would need to go through stable, without
going into 3.18 .

Greg, is this ok with you and how do I get a patch in stable which
is not in upstream (because upstream will get a better fix) ?

Regards,

Hans
--
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

Re: [PATCH 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/02/2014 03:12 PM, Christoph Hellwig wrote:
 On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote:
 Yes, although that likely would need to go through stable, without
 going into 3.18 .

 Greg, is this ok with you and how do I get a patch in stable which
 is not in upstream (because upstream will get a better fix) ?
 
 This is a trivial one liner and 3.17 isn't released yet, so can you just
 send it to Linus now?

I don't think it is that urgent, given that block-mq is disabled by default
in 3.17 AFAIK. But feel free to send such a patch directly to Linus with my:

Acked-by: Hans de Goede hdego...@redhat.com

Added, if that gets in, I guess I then need to do a v2 of the patch
from $subject, reverting it.

Regards,

Hans
--
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

Re: [PATCH 2/2] uas: Make uas work with blk-mq

[Hans de Goede]

Hi,

On 10/02/2014 03:25 PM, James Bottomley wrote:
 On Thu, 2014-10-02 at 15:18 +0200, Hans de Goede wrote:
 Hi,

 On 10/02/2014 03:12 PM, Christoph Hellwig wrote:
 On Thu, Oct 02, 2014 at 03:08:40PM +0200, Hans de Goede wrote:
 Yes, although that likely would need to go through stable, without
 going into 3.18 .

 Greg, is this ok with you and how do I get a patch in stable which
 is not in upstream (because upstream will get a better fix) ?

 This is a trivial one liner and 3.17 isn't released yet, so can you just
 send it to Linus now?

 I don't think it is that urgent, given that block-mq is disabled by default
 in 3.17 AFAIK. But feel free to send such a patch directly to Linus with my:

 Acked-by: Hans de Goede hdego...@redhat.com

 Added, if that gets in, I guess I then need to do a v2 of the patch
 from $subject, reverting it.
 
 OK, so 3.17 should be on sunday giving three days or so to sort this
 out.  If you're happy with blk-mq being disabled so the bug never
 triggers unless the user activates it, doing nothing is my preferred
 outcome ... but that also means no need to backport to stable.  
 
 If you want a it fixed in 3.17, then we need a patch from you now.
 Ultimately you have to judge the necessity for us, since it's your
 driver.

Fair enough. Given that this only triggers if the user enables an experimental
option, and then mixes that with uas usage, my preferred way to deal with this
is also doing nothing.

Regards,

Hans
--
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

uas breakage when using scsi_mod.blk_mq=Y

[Hans de Goede]

Hi Christoph,

Douglas Gilbert (in the CC), has been testing uas with
scsi_mod.blk_mq=Y and this fails. When it fails the following
messages appear in dmesg:

kernel: scsi host8: uas
kernel: blk-mq: reduced tag depth to 10240
mtp-probe: checking bus 2, device 3: 
/sys/devices/pci:00/:00:14.0/usb2/2-1
mtp-probe: bus: 2, device: 3 was not an MTP device
kernel: scsi 8:0:0:0: Direct-Access INTEL SS DSA2M080G2GC 2CV1 PQ: 0 
ANSI: 6
kernel: sd 8:0:0:0: [sdb] 156301484 512-byte logical blocks: (80.0 GB/74.5 GiB)
kernel: sd 8:0:0:0: Attached scsi generic sg1 type 0
kernel: sd 8:0:0:0: [sdb] Write Protect is off
kernel: sd 8:0:0:0: [sdb] Mode Sense: 31 00 00 00
kernel: sd 8:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't 
support DPO or FUA
kernel: sdb: sdb1 sdb2
kernel: sd 8:0:0:0: [sdb] Attached SCSI disk
kernel: xhci_hcd :00:14.0: WARN: Slot ID 8, ep index 14 has stream IDs 1 to 
32 allocated, but stream ID 33 is requested.
kernel: sd 8:0:0:0: [sdb] sense submit err -22 tag 33 inflight: s-st a-in s-in 
a-cmd s-cmd
kernel: sd 8:0:0:0: [sdb] CDB:
kernel: Read(10): 28 00 00 00 01 6c 00 00 04 00

The problematic part here, which I believe is caused by scsi_mod.blk_mq=Y,
is the tag number 33. uas.c does the following in slave_configure:

scsi_activate_tcq(sdev, devinfo-qdepth - 2);

Where qdepth is 32, so 30 gets passed in. uas.c stranslates scsi tags
to uas stream ids, which means it adds 2 (stream ids start at 1 not 0,
and 1 is reserved for untagged commands).

So the tag 33 above, means that the scsi subsys has called uas.c with
a tagged command with a tag of 31, which should not happen when using
scsi_activate_tcq(sdev, 30).

So should the uas.c code do something different with blk-mq to tell
it to only use tags 0-29, or is this a blk-mq bug ?

Regards,

Hans
--
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

Re: uas breakage when using scsi_mod.blk_mq=Y

[Hans de Goede]

Hi,

On 10/01/2014 02:45 PM, Christoph Hellwig wrote:
 On Wed, Oct 01, 2014 at 10:17:41AM +0200, Hans de Goede wrote:
 The problematic part here, which I believe is caused by scsi_mod.blk_mq=Y,
 is the tag number 33. uas.c does the following in slave_configure:

  scsi_activate_tcq(sdev, devinfo-qdepth - 2);

 Where qdepth is 32, so 30 gets passed in. uas.c stranslates scsi tags
 to uas stream ids, which means it adds 2 (stream ids start at 1 not 0,
 and 1 is reserved for untagged commands).

 So the tag 33 above, means that the scsi subsys has called uas.c with
 a tagged command with a tag of 31, which should not happen when using
 scsi_activate_tcq(sdev, 30).

 So should the uas.c code do something different with blk-mq to tell
 it to only use tags 0-29, or is this a blk-mq bug ?
 
 This is a mismatch between the (undocumented) existing behavior and
 what blk-mq implements.  In the old code unless you use host-shared
 maps you would never see a tag number greater than the queue depth
 when using block layer tags.  With blk-mq we also use host-wide maps,
 so you can easily see tag numbers bigger than the queue depth.
 
 So far it seems uas is the only driver with this expectation.
 Given how it maps tags to a non-scsi concept it might be better to
 just use a separate bitmaps for the streams inside uas than reusing
 the tags.

So let me see if I understand this correctly, blk-mq will never queue
more then qdepth commands, but it will use higher tag numbers ?

If that is the case fixing this should be easy, uas already has
an array to map stream-ids to scsi cmnds so that when it gets a status
urb, it can find the scsi cmnd which belongs to that status urb. We
can just search for a free slot in that array.

 Is this mapping an implementation detail of the Linux uas
 driver or part of the spec?

The mapping is a Linux uas implementation detail, the spec is
silent on how to map things.

Regards,

Hans
--
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

[PATCH fix for 3.17] uas: Add another ASM1051 usb-id to the uas blacklist

[Hans de Goede]

As most ASM1051 based devices, this one has unfixable issues with uas too.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 94fb09f..8511b54 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -67,3 +67,11 @@ UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
JMS567,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_REPORT_OPCODES),
+
+/* Most ASM1051 based devices have issues with uas, blacklist them all */
+/* Reported-by: Hans de Goede hdego...@redhat.com */
+UNUSUAL_DEV(0x174c, 0x5106, 0x, 0x,
+   ASMedia,
+   ASM1051,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_IGNORE_UAS),
-- 
2.1.0

--
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

[PATCH fix for 3.17] uas: Add US_FL_NO_ATA_1X quirk for Seagate (0bc2:ab20) drives

[Hans de Goede]

https://bbs.archlinux.org/viewtopic.php?pid=1457492

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/unusual_uas.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 3e62437..94fb09f 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -54,6 +54,13 @@ UNUSUAL_DEV(0x0bc2, 0x3312, 0x, 0x,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
 
+/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
+UNUSUAL_DEV(0x0bc2, 0xab20, 0x, 0x,
+   Seagate,
+   Backup+ BK,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
 /* Reported-by: Claudio Bizzarri claudio.bizza...@gmail.com */
 UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
JMicron,
-- 
2.1.0

--
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

[PATCH fix for 3.17 0/1] uas: Add no-report-opcodes quirk

[Hans de Goede]

Hi Greg, et al,

Another day, another uas quirk. Although you may not see it that way, this
is
actually a good thing :) With the new uas error handling code (which users
are
testing through a stand-alone version of the uas driver), people are now
actually sending me logs with useful error messages instead of complaining
uas crashes their system.

These error logs allow me to figure out the right quirks for various models
(which unfortunately are necessary), and get more uas disk enclosures to
work
properly in uas mode.

Regards,

Hans
--
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

[PATCH fix for 3.17] uas: Add no-report-opcodes quirk

[Hans de Goede]

Besides the ASM1051 (*) needing sdev-no_report_opcodes = 1, it turns out that
the JMicron JMS567 also needs it to work properly with uas (usb-storage always
sets it). Since some of the scsi devs were not to keen on the idea to
outrightly set sdev-no_report_opcodes = 1 for all uas devices, so add a quirk
for this, and set it for the JMS567.

*) Which has become a non-issue since we've completely blacklisted uas on
the ASM1051 for other reasons

Cc: sta...@vger.kernel.org
Reported-and-tested-by: Claudio Bizzarri claudio.bizza...@gmail.com
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 Documentation/kernel-parameters.txt | 2 ++
 drivers/usb/storage/uas.c   | 4 
 drivers/usb/storage/unusual_uas.h   | 7 +++
 drivers/usb/storage/usb.c   | 5 -
 include/linux/usb_usual.h   | 2 ++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index dd4fe98..1edd5fd 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3522,6 +3522,8 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
READ_DISC_INFO command);
e = NO_READ_CAPACITY_16 (don't use
READ_CAPACITY_16 command);
+   f = NO_REPORT_OPCODES (don't use report opcodes
+   command, uas only);
h = CAPACITY_HEURISTICS (decrease the
reported device capacity by one
sector if the number is odd);
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 75d2ccd..2713693 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -961,6 +961,10 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 static int uas_slave_configure(struct scsi_device *sdev)
 {
struct uas_dev_info *devinfo = sdev-hostdata;
+
+   if (devinfo-flags  US_FL_NO_REPORT_OPCODES)
+   sdev-no_report_opcodes = 1;
+
scsi_set_tag_type(sdev, MSG_ORDERED_TAG);
scsi_activate_tcq(sdev, devinfo-qdepth - 2);
return 0;
diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 3ff2dd4..3e62437 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -53,3 +53,10 @@ UNUSUAL_DEV(0x0bc2, 0x3312, 0x, 0x,
Expansion Desk,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_ATA_1X),
+
+/* Reported-by: Claudio Bizzarri claudio.bizza...@gmail.com */
+UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x,
+   JMicron,
+   JMS567,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_REPORT_OPCODES),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index b9d1b93..f60e7d4 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -479,7 +479,7 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
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_ATA_1X | US_FL_NO_REPORT_OPCODES);
 
p = quirks;
while (*p) {
@@ -517,6 +517,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
case 'e':
f |= US_FL_NO_READ_CAPACITY_16;
break;
+   case 'f':
+   f |= US_FL_NO_REPORT_OPCODES;
+   break;
case 'h':
f |= US_FL_CAPACITY_HEURISTICS;
break;
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index d271f88..a7f2604 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -75,6 +75,8 @@
/* Cannot handle FUA in WRITE or READ CDBs */   \
US_FLAG(NO_ATA_1X,  0x0200) \
/* Cannot handle ATA_12 or ATA_16 CDBs */   \
+   US_FLAG(NO_REPORT_OPCODES,  0x0400) \
+   /* Cannot handle MI_REPORT_SUPPORTED_OPERATION_CODES */ \
 
 #define US_FLAG(name, value)   US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.1.0

--
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

[PATCH fix for 3.17 v4] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.

https://bugzilla.kernel.org/show_bug.cgi?id=79511
https://bbs.archlinux.org/viewtopic.php?id=183190

While at it also add missing documentation for the u value for usb-storage
quirks.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com

--
Changes in v2: Add documentation for new t and u usb-storage.quirks flags
Changes in v3: Fix typo in documentation
Changes in v4: Also apply the quirk to (0bc2:3312)
---
 Documentation/kernel-parameters.txt |  3 +++
 drivers/usb/storage/uas.c   | 13 +
 drivers/usb/storage/unusual_uas.h   | 23 +--
 drivers/usb/storage/usb.c   |  6 +-
 include/linux/usb_usual.h   |  2 ++
 5 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 5ae8608..ec6b25a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
bogus residue values);
s = SINGLE_LUN (the device has only one
Logical Unit);
+   t = NO_ATA_1X (don't allow ATA(12) and ATA(16)
+   commands, uas only);
+   u = IGNORE_UAS (do not try to use uas);
w = NO_WP_DETECT (don't test whether the
medium is write-protected).
Example: quirks=0419:aaf5:rl,0421:0433:rc
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..75d2ccd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -28,6 +28,7 @@
 #include scsi/scsi_tcq.h
 
 #include uas-detect.h
+#include scsiglue.h
 
 /*
  * The r00-r01c specs define this version of the SENSE IU data structure.
@@ -49,6 +50,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
+   unsigned long flags;
int qdepth, resetting;
struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -714,6 +716,15 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
BUILD_BUG_ON(sizeof(struct uas_cmd_info)  sizeof(struct scsi_pointer));
 
+   if ((devinfo-flags  US_FL_NO_ATA_1X) 
+   (cmnd-cmnd[0] == ATA_12 || cmnd-cmnd[0] == ATA_16)) {
+   memcpy(cmnd-sense_buffer, usb_stor_sense_invalidCDB,
+  sizeof(usb_stor_sense_invalidCDB));
+   cmnd-result = SAM_STAT_CHECK_CONDITION;
+   cmnd-scsi_done(cmnd);
+   return 0;
+   }
+
spin_lock_irqsave(devinfo-lock, flags);
 
if (devinfo-resetting) {
@@ -1080,6 +1091,8 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo-resetting = 0;
devinfo-running_task = 0;
devinfo-shutdown = 0;
+   devinfo-flags = id-driver_info;
+   usb_stor_adjust_quirks(udev, devinfo-flags);
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 724..3ff2dd4 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,13 +40,16 @@
  * and don't forget to CC: the USB development list linux-...@vger.kernel.org
  */
 
-/*
- * This is an example entry for the US_FL_IGNORE_UAS flag. Once we have an
- * actual entry using US_FL_IGNORE_UAS this entry should be removed.
- *
- * UNUSUAL_DEV(  0xabcd, 0x1234, 0x0100, 0x0100,
- * Example,
- * Storage with broken UAS,
- * USB_SC_DEVICE, USB_PR_DEVICE, NULL,
- * US_FL_IGNORE_UAS),
- */
+/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
+UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
+/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
+UNUSUAL_DEV(0x0bc2, 0x3312, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index cedb292..b9d1b93 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -478,7 +478,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_CAPACITY_OK | US_FL_IGNORE_RESIDUE

[PATCH fix for 3.17 v4] uas: Add a quirk for rejecting ATA_12 and ATA_16

[Hans de Goede]

Hi Greg,

Sorry for the high number of iterations on this one. After the documentation
issues pointed out during review, I've just received a report from a user
that (as I already suspected) another seagate disk enclosure also benifits
from this quirk. So this version sets the quirk for 2 different seagate
models.

Regards,

Hans
--
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

Re: [PATCH fix for 3.17] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

Hi,

On 09/15/2014 10:42 AM, David Laight wrote:
 From: Alan Stern
 ...
 p = quirks;
 while (*p) {
 @@ -543,6 +544,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
 unsigned long *fflags)
 case 's':
 f |= US_FL_SINGLE_LUN;
 break;
 +   case 't':
 +   f |= US_FL_NO_ATA_1X;
 +   break;
 case 'u':
 f |= US_FL_IGNORE_UAS;
 break;

 You must not add an aditional value for a module parameter without
 documenting it in Documentation/kernel-parameters.txt.
 
 How can this work as a 'module parameter'?
 I might want to use two different usb-scsi devices that have different
 requirements.

The usb-storage.quirks format includes a usb prod:vend if pair, so unless
you've 2 identical devices, you can specify which quirks to apply to
which device.

Regards,

Hans
--
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

[PATCH fix for 3.17 v5] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.

https://bugzilla.kernel.org/show_bug.cgi?id=79511
https://bbs.archlinux.org/viewtopic.php?id=183190

While at it also add missing documentation for the u value for usb-storage
quirks.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com

--
Changes in v2: Add documentation for new t and u usb-storage.quirks flags
Changes in v3: Fix typo in documentation
Changes in v4: Also apply the quirk to (0bc2:3312)
Changes in v5: Rebased on 3.17-rc5, drop u documentation, already upstream
---
 Documentation/kernel-parameters.txt |  2 ++
 drivers/usb/storage/uas.c   | 13 +
 drivers/usb/storage/unusual_uas.h   | 23 +--
 drivers/usb/storage/usb.c   |  6 +-
 include/linux/usb_usual.h   |  2 ++
 5 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 10d51c2..dd4fe98 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3541,6 +3541,8 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
bogus residue values);
s = SINGLE_LUN (the device has only one
Logical Unit);
+   t = NO_ATA_1X (don't allow ATA(12) and ATA(16)
+   commands, uas only);
u = IGNORE_UAS (don't bind to the uas driver);
w = NO_WP_DETECT (don't test whether the
medium is write-protected).
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..75d2ccd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -28,6 +28,7 @@
 #include scsi/scsi_tcq.h
 
 #include uas-detect.h
+#include scsiglue.h
 
 /*
  * The r00-r01c specs define this version of the SENSE IU data structure.
@@ -49,6 +50,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
+   unsigned long flags;
int qdepth, resetting;
struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -714,6 +716,15 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
BUILD_BUG_ON(sizeof(struct uas_cmd_info)  sizeof(struct scsi_pointer));
 
+   if ((devinfo-flags  US_FL_NO_ATA_1X) 
+   (cmnd-cmnd[0] == ATA_12 || cmnd-cmnd[0] == ATA_16)) {
+   memcpy(cmnd-sense_buffer, usb_stor_sense_invalidCDB,
+  sizeof(usb_stor_sense_invalidCDB));
+   cmnd-result = SAM_STAT_CHECK_CONDITION;
+   cmnd-scsi_done(cmnd);
+   return 0;
+   }
+
spin_lock_irqsave(devinfo-lock, flags);
 
if (devinfo-resetting) {
@@ -1080,6 +1091,8 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo-resetting = 0;
devinfo-running_task = 0;
devinfo-shutdown = 0;
+   devinfo-flags = id-driver_info;
+   usb_stor_adjust_quirks(udev, devinfo-flags);
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 724..3ff2dd4 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,13 +40,16 @@
  * and don't forget to CC: the USB development list linux-...@vger.kernel.org
  */
 
-/*
- * This is an example entry for the US_FL_IGNORE_UAS flag. Once we have an
- * actual entry using US_FL_IGNORE_UAS this entry should be removed.
- *
- * UNUSUAL_DEV(  0xabcd, 0x1234, 0x0100, 0x0100,
- * Example,
- * Storage with broken UAS,
- * USB_SC_DEVICE, USB_PR_DEVICE, NULL,
- * US_FL_IGNORE_UAS),
- */
+/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
+UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
+
+/* https://bbs.archlinux.org/viewtopic.php?id=183190 */
+UNUSUAL_DEV(0x0bc2, 0x3312, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index cedb292..b9d1b93 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -478,7 +478,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_CAPACITY_OK | US_FL_IGNORE_RESIDUE

Re: [PATCH fix for 3.17 v5] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

Hi,

On 09/15/2014 04:08 PM, Greg Kroah-Hartman wrote:
 On Mon, Sep 15, 2014 at 04:04:12PM +0200, Hans de Goede wrote:
 And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
 seems to hang upon receiving an ATA_12 or ATA_16 command.

 https://bugzilla.kernel.org/show_bug.cgi?id=79511
 https://bbs.archlinux.org/viewtopic.php?id=183190

 While at it also add missing documentation for the u value for usb-storage
 quirks.

 Cc: sta...@vger.kernel.org # 3.16
 Signed-off-by: Hans de Goede hdego...@redhat.com

 --
 Changes in v2: Add documentation for new t and u usb-storage.quirks flags
 Changes in v3: Fix typo in documentation
 Changes in v4: Also apply the quirk to (0bc2:3312)
 Changes in v5: Rebased on 3.17-rc5, drop u documentation, already upstream
 
 So I should wait another day for v6?  :)

Hehe, no I certainly hope not.

Chances are I there will eventually be more seagate models needing the quirk,
but I've none in the pipeline atm, and we can always add those with an
incremental patch.

So if this is not too late for 3.17 and you can pick up v5 that would be great.

Thanks,

Hans
--
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

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Hans de Goede]

Hi,

On 9/12/14 7:00 PM, Christoph Hellwig wrote:
 Please try the fix below, looks like the commit broke TCQ for all drivers
 using block-level tagging.

 ---
  From 865a19b760d2786fe37d3b5c151a4ecea4c0e95e Mon Sep 17 00:00:00 2001
 From: Christoph Hellwig h...@lst.de
 Date: Fri, 12 Sep 2014 16:00:19 -0700
 Subject: scsi: fix regression that accidentally disabled block-based tcq

 The scsi blk-mq support accidentally flipped a conditional, which lead to
 never enabling block based tcq when using the legacy request path.

 Fixes: d285203cf647d7c9 scsi: add support for a blk-mq based I/O path.
 Reported-by: Hans de Goede hdego...@redhat.com
 Signed-off-by: Christoph Hellwig h...@lst.de
 ---
   include/scsi/scsi_tcq.h | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
 index cdcc90b..e645835 100644
 --- a/include/scsi/scsi_tcq.h
 +++ b/include/scsi/scsi_tcq.h
 @@ -68,7 +68,7 @@ static inline void scsi_activate_tcq(struct scsi_device 
 *sdev, int depth)
   return;
   
   if (!shost_use_blk_mq(sdev-host) 
 - blk_queue_tagged(sdev-request_queue))
 + !blk_queue_tagged(sdev-request_queue))
   blk_queue_init_tags(sdev-request_queue, depth,
   sdev-host-bqt);
   

Reviewed-by: Hans de Goede hdego...@redhat.com

Regards,

Hans
--
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

Re: [PATCH v2 10/24] uas: zap_pending: data urbs should have completed at this time

[Hans de Goede]

Hi,

On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
 Hello.
 
 On 9/13/2014 1:26 PM, Hans de Goede wrote:
 
 The data urbs are all killed before calling zap_pending, and their completion
 handler should have cleared their inflight flag.
 
 Do not 0 the data inflight flags, and add a check for try_complete 
 succeeding,
 as it should always succeed when called from zap_pending.
 
 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
   drivers/usb/storage/uas.c | 10 +-
   1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
 index 08edb6b..85bbc1d 100644
 --- a/drivers/usb/storage/uas.c
 +++ b/drivers/usb/storage/uas.c
 @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info 
 *devinfo, int result)
   struct uas_cmd_info *cmdinfo;
   struct uas_cmd_info *temp;
   unsigned long flags;
 +int err;
 
Er, I don't see why this variable is necessary.
 
 [...]
 @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info 
 *devinfo, int result)
   struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
 SCp);
   uas_log_cmd_state(cmnd, __func__);
 -/* all urbs are killed, clear inflight bits */
 -cmdinfo-state = ~(COMMAND_INFLIGHT |
 -DATA_IN_URB_INFLIGHT |
 -DATA_OUT_URB_INFLIGHT);
 +/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
 +cmdinfo-state = ~COMMAND_INFLIGHT;
   cmnd-result = result  16;
 -uas_try_complete(cmnd, __func__);
 +err = uas_try_complete(cmnd, __func__);
 +WARN_ON(err != 0);
 
Why not:
 
 WARN_ON(uas_try_complete(cmnd, __func__));
 

This was discussed already during v1 of this patch-set, WARN_ON may
not have a side-effect, as it may be defined as an empty macro.

Regards,

Hans
--
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

Re: [PATCH fix for 3.17 v2] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

Hi,

On 09/13/2014 10:27 PM, Sergei Shtylyov wrote:
 Hello.
 
 On 9/13/2014 10:21 PM, Thomas Backlund wrote:
 
 And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
 seems to hang upon receiving an ATA_12 or ATA_16 command.
 
 https://bugzilla.kernel.org/show_bug.cgi?id=79511
 
 While at it also add missing documentation for the u value for usb-storage
 quirks.
 
 Cc: sta...@vger.kernel.org # 3.16
 Signed-off-by: Hans de Goede hdego...@redhat.com
 
 -- 
 Changes in v2: Add documentation for new t and u usb-storage.quirks flags
 ---
   Documentation/kernel-parameters.txt |  3 +++
   drivers/usb/storage/uas.c   | 13 +
   drivers/usb/storage/unusual_uas.h   | 16 ++--
   drivers/usb/storage/usb.c   |  6 +-
   include/linux/usb_usual.h   |  2 ++
   5 files changed, 29 insertions(+), 11 deletions(-)
 
 diff --git a/Documentation/kernel-parameters.txt
 b/Documentation/kernel-parameters.txt
 index 5ae8608..7c32053 100644
 --- a/Documentation/kernel-parameters.txt
 +++ b/Documentation/kernel-parameters.txt
 @@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be
 entirely omitted.
   bogus residue values);
   s = SINGLE_LUN (the device has only one
   Logical Unit);
 +t = NO_ATA_1X (don't allow ATA12 and ATA12
 +commands, uas only);
 
 I guess you meant ATA12 and *ATA16*
 
... or even ATA(12) and ATA(16). As far as I remember SCSI, it has CDB 
 length in parens for the command names.

Right, ugh. v3 coming up ...

Thanks,

Hans
--
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

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Hans de Goede]

Hi,

On 09/13/2014 07:50 PM, Christoph Hellwig wrote:
 On Sat, Sep 13, 2014 at 12:28:41PM +0200, Hans de Goede wrote:
 Yes this one does the trick and fixes things. Note the git tree I used for
 testing also had your previous fix to split up the blk_tcq union in 2
 separate struct members. Let me know if you want me to re-test without that
 fix.
 
 I'm pretty sure that isn't needed, so double testing it indeed doesn't
 would be helpful.

I can confirm that things still work fine with the union left in place.

  I'd like to add your Tested-by: tag after that, too.

Ack:

Tested-by: Hans de Goede hdego...@redhat.com

Regards,

Hans
--
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

[PATCH fix for 3.17 v3] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.

https://bugzilla.kernel.org/show_bug.cgi?id=79511

While at it also add missing documentation for the u value for usb-storage
quirks.

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com

--
Changes in v2: Add documentation for new t and u usb-storage.quirks flags
Changes in v3: Fix typo in documentation
---
 Documentation/kernel-parameters.txt |  3 +++
 drivers/usb/storage/uas.c   | 13 +
 drivers/usb/storage/unusual_uas.h   | 16 ++--
 drivers/usb/storage/usb.c   |  6 +-
 include/linux/usb_usual.h   |  2 ++
 5 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 5ae8608..ec6b25a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
bogus residue values);
s = SINGLE_LUN (the device has only one
Logical Unit);
+   t = NO_ATA_1X (don't allow ATA(12) and ATA(16)
+   commands, uas only);
+   u = IGNORE_UAS (do not try to use uas);
w = NO_WP_DETECT (don't test whether the
medium is write-protected).
Example: quirks=0419:aaf5:rl,0421:0433:rc
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..75d2ccd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -28,6 +28,7 @@
 #include scsi/scsi_tcq.h
 
 #include uas-detect.h
+#include scsiglue.h
 
 /*
  * The r00-r01c specs define this version of the SENSE IU data structure.
@@ -49,6 +50,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
+   unsigned long flags;
int qdepth, resetting;
struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -714,6 +716,15 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
BUILD_BUG_ON(sizeof(struct uas_cmd_info)  sizeof(struct scsi_pointer));
 
+   if ((devinfo-flags  US_FL_NO_ATA_1X) 
+   (cmnd-cmnd[0] == ATA_12 || cmnd-cmnd[0] == ATA_16)) {
+   memcpy(cmnd-sense_buffer, usb_stor_sense_invalidCDB,
+  sizeof(usb_stor_sense_invalidCDB));
+   cmnd-result = SAM_STAT_CHECK_CONDITION;
+   cmnd-scsi_done(cmnd);
+   return 0;
+   }
+
spin_lock_irqsave(devinfo-lock, flags);
 
if (devinfo-resetting) {
@@ -1080,6 +1091,8 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo-resetting = 0;
devinfo-running_task = 0;
devinfo-shutdown = 0;
+   devinfo-flags = id-driver_info;
+   usb_stor_adjust_quirks(udev, devinfo-flags);
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 724..52f7012 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,13 +40,9 @@
  * and don't forget to CC: the USB development list linux-...@vger.kernel.org
  */
 
-/*
- * This is an example entry for the US_FL_IGNORE_UAS flag. Once we have an
- * actual entry using US_FL_IGNORE_UAS this entry should be removed.
- *
- * UNUSUAL_DEV(  0xabcd, 0x1234, 0x0100, 0x0100,
- * Example,
- * Storage with broken UAS,
- * USB_SC_DEVICE, USB_PR_DEVICE, NULL,
- * US_FL_IGNORE_UAS),
- */
+/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
+UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index cedb292..b9d1b93 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -478,7 +478,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_CAPACITY_OK | US_FL_IGNORE_RESIDUE |
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
US_FL_NO_READ_DISC_INFO | US_FL_NO_READ_CAPACITY_16 |
-   US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE);
+   US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
+   US_FL_NO_ATA_1X);
 
p = quirks;
while (*p) {
@@ -543,6 +544,9

Re: [PATCH v2 10/24] uas: zap_pending: data urbs should have completed at this time

[Hans de Goede]

Hi,

On 09/14/2014 12:29 PM, James Bottomley wrote:
 On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
 Hi,

 On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
 Hello.

 On 9/13/2014 1:26 PM, Hans de Goede wrote:

 The data urbs are all killed before calling zap_pending, and their 
 completion
 handler should have cleared their inflight flag.

 Do not 0 the data inflight flags, and add a check for try_complete 
 succeeding,
 as it should always succeed when called from zap_pending.

 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
   drivers/usb/storage/uas.c | 10 +-
   1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
 index 08edb6b..85bbc1d 100644
 --- a/drivers/usb/storage/uas.c
 +++ b/drivers/usb/storage/uas.c
 @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info 
 *devinfo, int result)
   struct uas_cmd_info *cmdinfo;
   struct uas_cmd_info *temp;
   unsigned long flags;
 +int err;

Er, I don't see why this variable is necessary.

 [...]
 @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info 
 *devinfo, int result)
   struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
 SCp);
   uas_log_cmd_state(cmnd, __func__);
 -/* all urbs are killed, clear inflight bits */
 -cmdinfo-state = ~(COMMAND_INFLIGHT |
 -DATA_IN_URB_INFLIGHT |
 -DATA_OUT_URB_INFLIGHT);
 +/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
 +cmdinfo-state = ~COMMAND_INFLIGHT;
   cmnd-result = result  16;
 -uas_try_complete(cmnd, __func__);
 +err = uas_try_complete(cmnd, __func__);
 +WARN_ON(err != 0);

Why not:

 WARN_ON(uas_try_complete(cmnd, __func__));


 This was discussed already during v1 of this patch-set, WARN_ON may
 not have a side-effect, as it may be defined as an empty macro.
 
 Must have missed the discussion, but whoever said that loses all their
 review points.  We're very careful to make sure that even in the case
 where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
 side effects are still accounted for.  This is the canonical definition
 of WARN_ON in the compiled out case:
 
 #define WARN_ON(condition) ({ \
   int __ret_warn_on = !!(condition);  \
   unlikely(__ret_warn_on);\
 })
 
 So the compiler will eliminate the statement only if there are no side
 effects.

Ah that is good to know. Still I would like to stick with the new version
(which adds the err), as I believe that that code is more readable.

AFAIK in general the kernel coding style is to favor:

err = func();
if (err)

over:

if (func())

And this is sorta the same.

Regards,

Hans
--
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

[PATCH fix for 3.17] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

[Hans de Goede]

And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.

https://bugzilla.kernel.org/show_bug.cgi?id=79511

Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 13 +
 drivers/usb/storage/unusual_uas.h | 16 ++--
 drivers/usb/storage/usb.c |  6 +-
 include/linux/usb_usual.h |  2 ++
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..75d2ccd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -28,6 +28,7 @@
 #include scsi/scsi_tcq.h
 
 #include uas-detect.h
+#include scsiglue.h
 
 /*
  * The r00-r01c specs define this version of the SENSE IU data structure.
@@ -49,6 +50,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
+   unsigned long flags;
int qdepth, resetting;
struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -714,6 +716,15 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
BUILD_BUG_ON(sizeof(struct uas_cmd_info)  sizeof(struct scsi_pointer));
 
+   if ((devinfo-flags  US_FL_NO_ATA_1X) 
+   (cmnd-cmnd[0] == ATA_12 || cmnd-cmnd[0] == ATA_16)) {
+   memcpy(cmnd-sense_buffer, usb_stor_sense_invalidCDB,
+  sizeof(usb_stor_sense_invalidCDB));
+   cmnd-result = SAM_STAT_CHECK_CONDITION;
+   cmnd-scsi_done(cmnd);
+   return 0;
+   }
+
spin_lock_irqsave(devinfo-lock, flags);
 
if (devinfo-resetting) {
@@ -1080,6 +1091,8 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo-resetting = 0;
devinfo-running_task = 0;
devinfo-shutdown = 0;
+   devinfo-flags = id-driver_info;
+   usb_stor_adjust_quirks(udev, devinfo-flags);
init_usb_anchor(devinfo-cmd_urbs);
init_usb_anchor(devinfo-sense_urbs);
init_usb_anchor(devinfo-data_urbs);
diff --git a/drivers/usb/storage/unusual_uas.h 
b/drivers/usb/storage/unusual_uas.h
index 724..52f7012 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,13 +40,9 @@
  * and don't forget to CC: the USB development list linux-...@vger.kernel.org
  */
 
-/*
- * This is an example entry for the US_FL_IGNORE_UAS flag. Once we have an
- * actual entry using US_FL_IGNORE_UAS this entry should be removed.
- *
- * UNUSUAL_DEV(  0xabcd, 0x1234, 0x0100, 0x0100,
- * Example,
- * Storage with broken UAS,
- * USB_SC_DEVICE, USB_PR_DEVICE, NULL,
- * US_FL_IGNORE_UAS),
- */
+/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
+UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
+   Seagate,
+   Expansion Desk,
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_NO_ATA_1X),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index cedb292..b9d1b93 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -478,7 +478,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_CAPACITY_OK | US_FL_IGNORE_RESIDUE |
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
US_FL_NO_READ_DISC_INFO | US_FL_NO_READ_CAPACITY_16 |
-   US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE);
+   US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
+   US_FL_NO_ATA_1X);
 
p = quirks;
while (*p) {
@@ -543,6 +544,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
case 's':
f |= US_FL_SINGLE_LUN;
break;
+   case 't':
+   f |= US_FL_NO_ATA_1X;
+   break;
case 'u':
f |= US_FL_IGNORE_UAS;
break;
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index 9b7de1b..d271f88 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -73,6 +73,8 @@
/* Device advertises UAS but it is broken */\
US_FLAG(BROKEN_FUA, 0x0100) \
/* Cannot handle FUA in WRITE or READ CDBs */   \
+   US_FLAG(NO_ATA_1X,  0x0200) \
+   /* Cannot handle ATA_12 or ATA_16 CDBs */   \
 
 #define US_FLAG(name, value)   US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.1.0

--
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

[PATCH fix for 3.17 0/1] uas: Add a quirk for rejecting ATA_12 and ATA_16

[Hans de Goede]

Hi Greg,

As the subject indicates, if possible I would like to still get this into
3.17.

For now this impacts only a single usb-id, but looking at
https://bbs.archlinux.org/viewtopic.php?id=183190

It seems more seagate uas capable enclosures are having issues, I'm waiting
for feedback from their users. I hope those can be fixed by adding the same
quirk for there usb-ids.

Regards,

Hans
--
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

[PATCH v2 01/24] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Hans de Goede]

From: Sanjeev Sharma sanjeev_sha...@mentor.com

On some architecture spin_is_locked() always return false in
uniprocessor configuration and therefore it would be advise to replace
with lockdep_assert_held().

Signed-off-by: Sanjeev Sharma sanjeev_sha...@mentor.com
Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 75d2ccd..8c7d4a2 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -156,7 +156,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
 
uas_log_cmd_state(cmnd, caller);
-   WARN_ON_ONCE(!spin_is_locked(devinfo-lock));
+   lockdep_assert_held(devinfo-lock);
WARN_ON_ONCE(cmdinfo-state  COMMAND_ABORTED);
cmdinfo-state |= COMMAND_ABORTED;
cmdinfo-state = ~IS_IN_WORK_LIST;
@@ -183,7 +183,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
struct uas_dev_info *devinfo = cmnd-device-hostdata;
 
-   WARN_ON_ONCE(!spin_is_locked(devinfo-lock));
+   lockdep_assert_held(devinfo-lock);
cmdinfo-state |= IS_IN_WORK_LIST;
schedule_work(devinfo-work);
 }
@@ -285,7 +285,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
struct uas_cmd_info *cmdinfo = (void *)cmnd-SCp;
struct uas_dev_info *devinfo = (void *)cmnd-device-hostdata;
 
-   WARN_ON_ONCE(!spin_is_locked(devinfo-lock));
+   lockdep_assert_held(devinfo-lock);
if (cmdinfo-state  (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
  DATA_OUT_URB_INFLIGHT |
@@ -624,7 +624,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct urb *urb;
int err;
 
-   WARN_ON_ONCE(!spin_is_locked(devinfo-lock));
+   lockdep_assert_held(devinfo-lock);
if (cmdinfo-state  SUBMIT_STATUS_URB) {
urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo-stream);
if (!urb)
-- 
2.1.0

--
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

[PATCH v2 00/24] uas: rewrite error handling for robustness + misc cleanups

[Hans de Goede]

Hi Greg, et al,

Here is v2 of my uas error handling rewrite + cleanups series.

Note this has been rebased on top of the

[PATCH fix for 3.17] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands

patch which I've just send, so if that does not make 3.17, you should still
apply that one first to avoid conflicts.

Changes since v1:
-Rebased on top of:
 uas: Add a quirk for rejecting ATA_12 and ATA_16 commands
-uas: zap_pending: data urbs should have completed at this time:
 Modified WARN_ON check to not have side effects
-Added 3 new patches:
 uas: Cleanup uas_log_cmd_state usage
 uas: Log error codes when logging errors
 uas: Add response iu handling

Regards,

Hans
--
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

[PATCH v2 23/24] uas: Log error codes when logging errors

[Hans de Goede]

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 0b3d3a5..1dcaeee 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -750,7 +750,8 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_unlock_device(udev);
 
if (err) {
-   shost_printk(KERN_INFO, sdev-host, %s FAILED\n, __func__);
+   shost_printk(KERN_INFO, sdev-host, %s FAILED err %d\n,
+__func__, err);
return FAILED;
}
 
@@ -1020,13 +1021,16 @@ static int uas_post_reset(struct usb_interface *intf)
struct Scsi_Host *shost = usb_get_intfdata(intf);
struct uas_dev_info *devinfo = (struct uas_dev_info *)shost-hostdata;
unsigned long flags;
+   int err;
 
if (devinfo-shutdown)
return 0;
 
-   if (uas_configure_endpoints(devinfo) != 0) {
+   err = uas_configure_endpoints(devinfo);
+   if (err) {
shost_printk(KERN_ERR, shost,
-%s: alloc streams error after reset, __func__);
+%s: alloc streams error %d after reset,
+__func__, err);
return 1;
}
 
@@ -1062,10 +1066,13 @@ static int uas_reset_resume(struct usb_interface *intf)
struct Scsi_Host *shost = usb_get_intfdata(intf);
struct uas_dev_info *devinfo = (struct uas_dev_info *)shost-hostdata;
unsigned long flags;
+   int err;
 
-   if (uas_configure_endpoints(devinfo) != 0) {
+   err = uas_configure_endpoints(devinfo);
+   if (err) {
shost_printk(KERN_ERR, shost,
-%s: alloc streams error after reset, __func__);
+%s: alloc streams error %d after reset,
+__func__, err);
return -EIO;
}
 
-- 
2.1.0

--
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

[PATCH v2 19/24] uas: Drop COMMAND_COMPLETED flag

[Hans de Goede]

It was only used to sanity check against completing the same cmnd twice,
but that is the case we're likely operating on free-ed memory, and doing
sanity checks on free-ed memory is not really helpful.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 652b97b..c6f7870 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -74,9 +74,8 @@ enum {
COMMAND_INFLIGHT= (1  8),
DATA_IN_URB_INFLIGHT= (1  9),
DATA_OUT_URB_INFLIGHT   = (1  10),
-   COMMAND_COMPLETED   = (1  11),
-   COMMAND_ABORTED = (1  12),
-   IS_IN_WORK_LIST = (1  13),
+   COMMAND_ABORTED = (1  11),
+   IS_IN_WORK_LIST = (1  12),
 };
 
 /* Overrides scsi_pointer */
@@ -232,7 +231,7 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
struct uas_cmd_info *ci = (void *)cmnd-SCp;
 
scmd_printk(KERN_INFO, cmnd,
-   %s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ,
+   %s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
caller, uas_get_tag(cmnd),
(ci-state  SUBMIT_STATUS_URB) ?  s-st  : ,
(ci-state  ALLOC_DATA_IN_URB) ?  a-in  : ,
@@ -244,7 +243,6 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
(ci-state  COMMAND_INFLIGHT)  ?  CMD   : ,
(ci-state  DATA_IN_URB_INFLIGHT)  ?  IN: ,
(ci-state  DATA_OUT_URB_INFLIGHT) ?  OUT   : ,
-   (ci-state  COMMAND_COMPLETED) ?  done  : ,
(ci-state  COMMAND_ABORTED)   ?  abort : ,
(ci-state  IS_IN_WORK_LIST)   ?  work  : );
scsi_print_command(cmnd);
@@ -280,8 +278,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
  DATA_OUT_URB_INFLIGHT |
  COMMAND_ABORTED))
return -EBUSY;
-   WARN_ON_ONCE(cmdinfo-state  COMMAND_COMPLETED);
-   cmdinfo-state |= COMMAND_COMPLETED;
devinfo-cmnd[uas_get_tag(cmnd) - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd-scsi_done(cmnd);
-- 
2.1.0

--
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

[PATCH v2 10/24] uas: zap_pending: data urbs should have completed at this time

[Hans de Goede]

The data urbs are all killed before calling zap_pending, and their completion
handler should have cleared their inflight flag.

Do not 0 the data inflight flags, and add a check for try_complete succeeding,
as it should always succeed when called from zap_pending.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08edb6b..85bbc1d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, 
int result)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
unsigned long flags;
+   int err;
 
spin_lock_irqsave(devinfo-lock, flags);
list_for_each_entry_safe(cmdinfo, temp, devinfo-dead_list, list) {
@@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, 
int result)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
  SCp);
uas_log_cmd_state(cmnd, __func__);
-   /* all urbs are killed, clear inflight bits */
-   cmdinfo-state = ~(COMMAND_INFLIGHT |
-   DATA_IN_URB_INFLIGHT |
-   DATA_OUT_URB_INFLIGHT);
+   /* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
+   cmdinfo-state = ~COMMAND_INFLIGHT;
cmnd-result = result  16;
-   uas_try_complete(cmnd, __func__);
+   err = uas_try_complete(cmnd, __func__);
+   WARN_ON(err != 0);
}
spin_unlock_irqrestore(devinfo-lock, flags);
 }
-- 
2.1.0

--
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

[PATCH v2 20/24] uas: Remove support for old sense ui as used in pre-production hardware

[Hans de Goede]

I've access to a number of different uas devices now, and none of them use
old style sense urbs. The only case where these code-paths trigger is with
the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes
status iu-s when it does not have any sense data, but uses new style
sense iu-s regardless, as can be seen for scsi cmnds where there is sense
data.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 47 +--
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index c6f7870..cbd4312 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -32,20 +32,6 @@
 
 #define MAX_CMNDS 256
 
-/*
- * The r00-r01c specs define this version of the SENSE IU data structure.
- * It's still in use by several different firmware releases.
- */
-struct sense_iu_old {
-   __u8 iu_id;
-   __u8 rsvd1;
-   __be16 tag;
-   __be16 len;
-   __u8 status;
-   __u8 service_response;
-   __u8 sense[SCSI_SENSE_BUFFERSIZE];
-};
-
 struct uas_dev_info {
struct usb_interface *intf;
struct usb_device *udev;
@@ -56,7 +42,6 @@ struct uas_dev_info {
int qdepth, resetting;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
unsigned use_streams:1;
-   unsigned uas_sense_old:1;
unsigned shutdown:1;
struct scsi_cmnd *cmnd[MAX_CMNDS];
spinlock_t lock;
@@ -187,29 +172,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd 
*cmnd)
cmnd-result = sense_iu-status;
 }
 
-static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
-{
-   struct sense_iu_old *sense_iu = urb-transfer_buffer;
-   struct scsi_device *sdev = cmnd-device;
-
-   if (urb-actual_length  8) {
-   unsigned len = be16_to_cpup(sense_iu-len) - 2;
-   if (len + 8 != urb-actual_length) {
-   int newlen = min(len + 8, urb-actual_length) - 8;
-   if (newlen  0)
-   newlen = 0;
-   sdev_printk(KERN_INFO, sdev, %s: urb length %d 
-   disagrees with IU sense data length %d, 
-   using %d bytes of sense data\n, __func__,
-   urb-actual_length, len, newlen);
-   len = newlen;
-   }
-   memcpy(cmnd-sense_buffer, sense_iu-sense, len);
-   }
-
-   cmnd-result = sense_iu-status;
-}
-
 /*
  * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
  * which go from 1 - nr_streams. And we use 1 for untagged commands.
@@ -339,12 +301,7 @@ static void uas_stat_cmplt(struct urb *urb)
 
switch (iu-iu_id) {
case IU_ID_STATUS:
-   if (urb-actual_length  16)
-   devinfo-uas_sense_old = 1;
-   if (devinfo-uas_sense_old)
-   uas_sense_old(urb, cmnd);
-   else
-   uas_sense(urb, cmnd);
+   uas_sense(urb, cmnd);
if (cmnd-result != 0) {
/* cancel data transfers on error */
data_in_urb = usb_get_urb(cmdinfo-data_in_urb);
@@ -900,8 +857,6 @@ static int uas_configure_endpoints(struct uas_dev_info 
*devinfo)
struct usb_device *udev = devinfo-udev;
int r;
 
-   devinfo-uas_sense_old = 0;
-
r = uas_find_endpoints(devinfo-intf-cur_altsetting, eps);
if (r)
return r;
-- 
2.1.0

--
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

[PATCH v2 22/24] uas: Cleanup uas_log_cmd_state usage

[Hans de Goede]

Instead of doing:

uas_log_cmd_state(cmnd, __func__)
scmd_printk(KERN_ERR, cmnd, error doing foo %d\n, err)

On error, resulting in 2 log calls for a single error, make uas_log_cmd_state
take a status code, and change calls like the above to:

uas_log_cmd_state(cmnd, error doing foo, err)

Also change various sanity checks (which should never trigger) from:
scmd_printk(KERN_ERR, cmnd, sanity foo failed\n) to calling the new
uas_log_cmd_state(), so that when they do trigger we get more info.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 52 +--
 1 file changed, 19 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index a77c5e0..0b3d3a5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,7 +78,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
 static void uas_free_streams(struct uas_dev_info *devinfo);
-static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
+   int status);
 
 static void uas_do_work(struct work_struct *work)
 {
@@ -139,7 +140,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, 
int result)
 
cmnd = devinfo-cmnd[i];
cmdinfo = (void *)cmnd-SCp;
-   uas_log_cmd_state(cmnd, __func__);
+   uas_log_cmd_state(cmnd, __func__, 0);
/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
cmdinfo-state = ~COMMAND_INFLIGHT;
cmnd-result = result  16;
@@ -188,13 +189,14 @@ static int uas_get_tag(struct scsi_cmnd *cmnd)
return tag;
 }
 
-static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *prefix,
+ int status)
 {
struct uas_cmd_info *ci = (void *)cmnd-SCp;
 
scmd_printk(KERN_INFO, cmnd,
-   %s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
-   caller, uas_get_tag(cmnd),
+   %s %d tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ,
+   prefix, status, uas_get_tag(cmnd),
(ci-state  SUBMIT_STATUS_URB) ?  s-st  : ,
(ci-state  ALLOC_DATA_IN_URB) ?  a-in  : ,
(ci-state  SUBMIT_DATA_IN_URB)?  s-in  : ,
@@ -295,7 +297,7 @@ static void uas_stat_cmplt(struct urb *urb)
cmdinfo = (void *)cmnd-SCp;
 
if (!(cmdinfo-state  COMMAND_INFLIGHT)) {
-   scmd_printk(KERN_ERR, cmnd, unexpected status cmplt\n);
+   uas_log_cmd_state(cmnd, unexpected status cmplt, 0);
goto out;
}
 
@@ -313,7 +315,7 @@ static void uas_stat_cmplt(struct urb *urb)
case IU_ID_READ_READY:
if (!cmdinfo-data_in_urb ||
(cmdinfo-state  DATA_IN_URB_INFLIGHT)) {
-   scmd_printk(KERN_ERR, cmnd, unexpected read rdy\n);
+   uas_log_cmd_state(cmnd, unexpected read rdy, 0);
break;
}
uas_xfer_data(urb, cmnd, SUBMIT_DATA_IN_URB);
@@ -321,14 +323,13 @@ static void uas_stat_cmplt(struct urb *urb)
case IU_ID_WRITE_READY:
if (!cmdinfo-data_out_urb ||
(cmdinfo-state  DATA_OUT_URB_INFLIGHT)) {
-   scmd_printk(KERN_ERR, cmnd, unexpected write rdy\n);
+   uas_log_cmd_state(cmnd, unexpected write rdy, 0);
break;
}
uas_xfer_data(urb, cmnd, SUBMIT_DATA_OUT_URB);
break;
default:
-   scmd_printk(KERN_ERR, cmnd,
-   Bogus IU (%d) received on status pipe\n, iu-iu_id);
+   uas_log_cmd_state(cmnd, bogus IU, iu-iu_id);
}
 out:
usb_free_urb(urb);
@@ -374,17 +375,13 @@ static void uas_data_cmplt(struct urb *urb)
 
/* Data urbs should not complete before the cmd urb is submitted */
if (cmdinfo-state  SUBMIT_CMD_URB) {
-   scmd_printk(KERN_ERR, cmnd, unexpected data cmplt\n);
+   uas_log_cmd_state(cmnd, unexpected data cmplt, 0);
goto out;
}
 
if (urb-status) {
-   if (urb-status != -ENOENT  urb-status != -ECONNRESET) {
-   uas_log_cmd_state(cmnd, __func__);
-   scmd_printk(KERN_ERR, cmnd,
-   data cmplt err %d stream %d\n,
-   urb-status, urb-stream_id);
-   }
+   if (urb-status != -ENOENT  urb-status != -ECONNRESET)
+   uas_log_cmd_state

[PATCH v2 15/24] uas: pre_reset and suspend: Fix a few races

[Hans de Goede]

The purpose of uas_pre_reset is to:

1) Stop any new commands from being submitted while an externally triggered
   usb-device-reset is running
2) Wait for any pending commands to finish before allowing the usb-device-reset
   to continue

The purpose of uas_suspend is to:
2) Wait for any pending commands to finish before suspending

This commit fixes races in both paths:

1) For 1) we use scsi_block_requests, but the scsi midlayer calls queuecommand
   without holding any locks, so a queuecommand may already past the midlayer
   scsi_block_requests checks when we call it, add a check to uas_queuecommand
   to fix this

2) For 2) we were waiting for all sense-urbs to complete, there are 2 problems
   with this approach:
a) data-urbs may complete after the sense urb, so we need to check for those
   too
b) if a sense-urb completes with a iu id of READ/WRITE_READY a command is not
   yet done. We submit a new sense-urb immediately in this case, but that
   submit may fail (in which case it will get retried by uas_do_work), if this
   happens the sense_urbs anchor may become empty while the cmnd is not yet
   done

Also unblock requests on timeout, to avoid things getting stuck in that case.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 61 ++-
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index fca8775..f4fe816 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -667,6 +667,10 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
BUILD_BUG_ON(sizeof(struct uas_cmd_info)  sizeof(struct scsi_pointer));
 
+   /* Re-check scsi_block_requests now that we've the host-lock */
+   if (cmnd-device-host-host_self_blocked)
+   return SCSI_MLQUEUE_DEVICE_BUSY;
+
if ((devinfo-flags  US_FL_NO_ATA_1X) 
(cmnd-cmnd[0] == ATA_12 || cmnd-cmnd[0] == ATA_16)) {
memcpy(cmnd-sense_buffer, usb_stor_sense_invalidCDB,
@@ -1005,6 +1009,54 @@ set_alt0:
return result;
 }
 
+static int uas_cmnd_list_empty(struct uas_dev_info *devinfo)
+{
+   unsigned long flags;
+   int i, r = 1;
+
+   spin_lock_irqsave(devinfo-lock, flags);
+
+   for (i = 0; i  devinfo-qdepth; i++) {
+   if (devinfo-cmnd[i]) {
+   r = 0; /* Not empty */
+   break;
+   }
+   }
+
+   spin_unlock_irqrestore(devinfo-lock, flags);
+
+   return r;
+}
+
+/*
+ * Wait for any pending cmnds to complete, on usb-2 sense_urbs may temporarily
+ * get empty while there still is more work to do due to sense-urbs completing
+ * with a READ/WRITE_READY iu code, so keep waiting until the list gets empty.
+ */
+static int uas_wait_for_pending_cmnds(struct uas_dev_info *devinfo)
+{
+   unsigned long start_time;
+   int r;
+
+   start_time = jiffies;
+   do {
+   flush_work(devinfo-work);
+
+   r = usb_wait_anchor_empty_timeout(devinfo-sense_urbs, 5000);
+   if (r == 0)
+   return -ETIME;
+
+   r = usb_wait_anchor_empty_timeout(devinfo-data_urbs, 500);
+   if (r == 0)
+   return -ETIME;
+
+   if (time_after(jiffies, start_time + 5 * HZ))
+   return -ETIME;
+   } while (!uas_cmnd_list_empty(devinfo));
+
+   return 0;
+}
+
 static int uas_pre_reset(struct usb_interface *intf)
 {
struct Scsi_Host *shost = usb_get_intfdata(intf);
@@ -1019,10 +1071,9 @@ static int uas_pre_reset(struct usb_interface *intf)
scsi_block_requests(shost);
spin_unlock_irqrestore(shost-host_lock, flags);
 
-   /* Wait for any pending requests to complete */
-   flush_work(devinfo-work);
-   if (usb_wait_anchor_empty_timeout(devinfo-sense_urbs, 5000) == 0) {
+   if (uas_wait_for_pending_cmnds(devinfo) != 0) {
shost_printk(KERN_ERR, shost, %s: timed out\n, __func__);
+   scsi_unblock_requests(shost);
return 1;
}
 
@@ -1060,9 +,7 @@ static int uas_suspend(struct usb_interface *intf, 
pm_message_t message)
struct Scsi_Host *shost = usb_get_intfdata(intf);
struct uas_dev_info *devinfo = (struct uas_dev_info *)shost-hostdata;
 
-   /* Wait for any pending requests to complete */
-   flush_work(devinfo-work);
-   if (usb_wait_anchor_empty_timeout(devinfo-sense_urbs, 5000) == 0) {
+   if (uas_wait_for_pending_cmnds(devinfo) != 0) {
shost_printk(KERN_ERR, shost, %s: timed out\n, __func__);
return -ETIME;
}
-- 
2.1.0

--
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

[PATCH v2 21/24] uas: Remove protype hardware usb interface info

[Hans de Goede]

We've removed all hack from the driver for pre-production hardware.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index cbd4312..a77c5e0 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -830,8 +830,6 @@ static struct usb_device_id uas_usb_ids[] = {
 #  include unusual_uas.h
{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_BULK) 
},
{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_UAS) },
-   /* 0xaa is a prototype device I happen to have access to */
-   { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, 0xaa) },
{ }
 };
 MODULE_DEVICE_TABLE(usb, uas_usb_ids);
-- 
2.1.0

--
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

[PATCH v2 17/24] uas: Do not log urb status error on cancellation

[Hans de Goede]

Check for both type of cancellation codes for sense and data urbs.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 drivers/usb/storage/uas.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 187e7bf..90afe4a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -318,10 +318,7 @@ static void uas_stat_cmplt(struct urb *urb)
goto out;
 
if (urb-status) {
-   if (urb-status == -ENOENT) {
-   dev_err(urb-dev-dev, stat urb: killed, stream %d\n,
-   urb-stream_id);
-   } else {
+   if (urb-status != -ENOENT  urb-status != -ECONNRESET) {
dev_err(urb-dev-dev, stat urb: status %d\n,
urb-status);
}
@@ -428,7 +425,7 @@ static void uas_data_cmplt(struct urb *urb)
}
 
if (urb-status) {
-   if (urb-status != -ECONNRESET) {
+   if (urb-status != -ENOENT  urb-status != -ECONNRESET) {
uas_log_cmd_state(cmnd, __func__);
scmd_printk(KERN_ERR, cmnd,
data cmplt err %d stream %d\n,
-- 
2.1.0

--
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