[PATCH v2] ohci-pci: add qemu quirk

[Gerd Hoffmann]

On a loaded virtualization host (dozen guests booting at the same time)
it may happen that the ohci controller emulation doesn't manage to do
timely frame processing, with the result that the io watchdog fires and
considers the controller being dead, even though it's only the emulation
being unusual slow due to the load peak.

So, add a quirk for qemu and don't use the watchdog in case we figure we
are running on emulated ohci.  The virtual ohci controller masquerades
as apple ohci controller, but we can identify it by subsystem id.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/host/ohci-hcd.c |  3 ++-
 drivers/usb/host/ohci-pci.c | 16 
 drivers/usb/host/ohci.h |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index b6daf2e..5f81a2b 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -231,7 +231,8 @@ static int ohci_urb_enqueue (
 
/* Start up the I/O watchdog timer, if it's not running */
if (!timer_pending(&ohci->io_watchdog) &&
-   list_empty(&ohci->eds_in_use)) {
+   list_empty(&ohci->eds_in_use) &&
+   !(ohci->flags & OHCI_QUIRK_QEMU)) {
ohci->prev_frame_no = ohci_frame_no(ohci);
mod_timer(&ohci->io_watchdog,
jiffies + IO_WATCHDOG_DELAY);
diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
index bb15096..a84aebe 100644
--- a/drivers/usb/host/ohci-pci.c
+++ b/drivers/usb/host/ohci-pci.c
@@ -164,6 +164,15 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd)
return 0;
 }
 
+static int ohci_quirk_qemu(struct usb_hcd *hcd)
+{
+   struct ohci_hcd *ohci = hcd_to_ohci(hcd);
+
+   ohci->flags |= OHCI_QUIRK_QEMU;
+   ohci_dbg(ohci, "enabled qemu quirk\n");
+   return 0;
+}
+
 /* List of quirks for OHCI */
 static const struct pci_device_id ohci_pci_quirks[] = {
{
@@ -214,6 +223,13 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd)
PCI_DEVICE(PCI_VENDOR_ID_ATI, 0x4399),
.driver_data = (unsigned long)ohci_quirk_amd700,
},
+   {
+   .vendor = PCI_VENDOR_ID_APPLE,
+   .device = 0x003f,
+   .subvendor  = PCI_SUBVENDOR_ID_REDHAT_QUMRANET,
+   .subdevice  = PCI_SUBDEVICE_ID_QEMU,
+   .driver_data= (unsigned long)ohci_quirk_qemu,
+   },
 
/* FIXME for some of the early AMD 760 southbridges, OHCI
 * won't work at all.  blacklist them.
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index 37f1725..a51b189 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -418,6 +418,7 @@ struct ohci_hcd {
 #defineOHCI_QUIRK_AMD_PLL  0x200   /* AMD PLL 
quirk*/
 #defineOHCI_QUIRK_AMD_PREFETCH 0x400   /* pre-fetch 
for ISO transfer */
 #defineOHCI_QUIRK_GLOBAL_SUSPEND   0x800   /* must suspend 
ports */
+#defineOHCI_QUIRK_QEMU 0x1000  /* relax timing 
expectations */
 
// there are also chip quirks/bugs in init logic
 
-- 
1.8.3.1

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

[PATCH] ohci-pci: add qemu quirk

[Gerd Hoffmann]

On a loaded virtualization host (dozen guests booting at the same time)
it may happen that the ohci controller emulation doesn't manage to do
timely frame processing, with the result that the io watchdog fires and
considers the controller being dead, even though it's only the emulation
being unusual slow due to the load peak.

So, add a quirk for qemu and don't use the watchdog in case we figure we
are running on emulated ohci.  The virtual ohci controller masquerades
as apple ohci controller, but we can identify it by subsystem id.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/host/ohci-hcd.c |  3 ++-
 drivers/usb/host/ohci-pci.c | 16 
 drivers/usb/host/ohci.h |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index b6daf2e..b75ae28 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -231,7 +231,8 @@ static int ohci_urb_enqueue (
 
/* Start up the I/O watchdog timer, if it's not running */
if (!timer_pending(&ohci->io_watchdog) &&
-   list_empty(&ohci->eds_in_use)) {
+   list_empty(&ohci->eds_in_use) &&
+   !(ohci->flags & OHCI_QUIRK_QEMU)) {
ohci->prev_frame_no = ohci_frame_no(ohci);
mod_timer(&ohci->io_watchdog,
jiffies + IO_WATCHDOG_DELAY);
diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
index bb15096..a84aebe 100644
--- a/drivers/usb/host/ohci-pci.c
+++ b/drivers/usb/host/ohci-pci.c
@@ -164,6 +164,15 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd)
return 0;
 }
 
+static int ohci_quirk_qemu(struct usb_hcd *hcd)
+{
+   struct ohci_hcd *ohci = hcd_to_ohci(hcd);
+
+   ohci->flags |= OHCI_QUIRK_QEMU;
+   ohci_dbg(ohci, "enabled qemu quirk\n");
+   return 0;
+}
+
 /* List of quirks for OHCI */
 static const struct pci_device_id ohci_pci_quirks[] = {
{
@@ -214,6 +223,13 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd)
PCI_DEVICE(PCI_VENDOR_ID_ATI, 0x4399),
.driver_data = (unsigned long)ohci_quirk_amd700,
},
+   {
+   .vendor = PCI_VENDOR_ID_APPLE,
+   .device = 0x003f,
+   .subvendor  = PCI_SUBVENDOR_ID_REDHAT_QUMRANET,
+   .subdevice  = PCI_SUBDEVICE_ID_QEMU,
+   .driver_data= (unsigned long)ohci_quirk_qemu,
+   },
 
/* FIXME for some of the early AMD 760 southbridges, OHCI
 * won't work at all.  blacklist them.
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index 37f1725..a51b189 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -418,6 +418,7 @@ struct ohci_hcd {
 #defineOHCI_QUIRK_AMD_PLL  0x200   /* AMD PLL 
quirk*/
 #defineOHCI_QUIRK_AMD_PREFETCH 0x400   /* pre-fetch 
for ISO transfer */
 #defineOHCI_QUIRK_GLOBAL_SUSPEND   0x800   /* must suspend 
ports */
+#defineOHCI_QUIRK_QEMU 0x1000  /* relax timing 
expectations */
 
// there are also chip quirks/bugs in init logic
 
-- 
1.8.3.1

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

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

[Gerd Hoffmann]

On Do, 2016-07-14 at 14:26 +0200, Hans de Goede wrote:
> Oliver Neukum is taking over uas maintainership from me and
> Gerd Hoffmann.
> 
> Cc: Oliver Neukum 
> Cc: Gerd Hoffmann 
> Signed-off-by: Hans de Goede 

Acked-by: Gerd Hoffmann 

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

Re: [PATCH 32/68] uas: Use proper packet size when submitting reponse urbs

[Gerd Hoffmann]

On Mo, 2013-11-18 at 09:25 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/18/2013 08:05 AM, Gerd Hoffmann wrote:
> > On Fr, 2013-11-15 at 16:06 +0100, Hans de Goede wrote:
> >> task management commands expect a response_iu rather then a sense_iu,
> >> and
> >> these have different sizes. Make the urb we submit to get the reply
> >> the right
> >> size.
> >
> > I think that doesn't work for uas in usb2 (no streams) mode.  status
> > urbs are not linked to requests (via tag / stream id) in that case, so
> > you can't know in advance which status urb gets filles with a sense_ui
> > and which with a response_ui by the device.
> 
> Oh, good point. You're completely right, so we always need to submit the
> bigger size which is the sense urb size. I'll drop this patch from my
> set then, and send a v3. Before I do that, do you have any other remarks ?

Didn't spot anything else.

cheers,
  Gerd


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

Re: [PATCH 32/68] uas: Use proper packet size when submitting reponse urbs

[Gerd Hoffmann]

On Fr, 2013-11-15 at 16:06 +0100, Hans de Goede wrote:
> task management commands expect a response_iu rather then a sense_iu,
> and
> these have different sizes. Make the urb we submit to get the reply
> the right
> size.

I think that doesn't work for uas in usb2 (no streams) mode.  status
urbs are not linked to requests (via tag / stream id) in that case, so
you can't know in advance which status urb gets filles with a sense_ui
and which with a response_ui by the device.

cheers,
  Gerd


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

[PATCH v2] xhci: fix usb3 streams

[Gerd Hoffmann]

Gerd, Hans, any objections to this updated patch?  The warning is fixed
with it.

The patch probably still needs to address the case where the ring
expansion fails because we can't insert the new segments into the radix
tree.  The patch should probably allocate the segments, attempt to add
them to the radix tree, and fail without modifying the link TRBs of the
ring.  I'd have to look more deeply into the code to see what exactly
should be done there.

I would like that issue fixed before I merge these patches, so if you
want to take a stab at fixing it, please do.

Sarah Sharp

8<>8

xhci maintains a radix tree for each stream endpoint because it must
be able to map a trb address to the stream ring.  Each ring segment
must be added to the ring for this to work.  Currently xhci sticks
only the first segment of each stream ring into the radix tree.

Result is that things work initially, but as soon as the first segment
is full xhci can't map the trb address from the completion event to the
stream ring any more -> BOOM.  You'll find this message in the logs:

  ERROR Transfer event for disabled endpoint or incorrect stream ring

This patch adds a helper function to update the radix tree, and a
function to remove ring segments from the tree.  Both functions loop
over the segment list and handles all segments instead of just the
first.

[Note: Sarah changed this patch to add radix_tree_maybe_preload() and
radix_tree_preload_end() calls around the radix tree insert, since we
can now insert entries in interrupt context.  There are now two helper
functions to make the code cleaner, and those functions are moved to
make them static.]

Signed-off-by: Gerd Hoffmann 
Signed-off-by: Hans de Goede 
Signed-off-by: Sarah Sharp 
---
 drivers/usb/host/xhci-mem.c | 132 +---
 drivers/usb/host/xhci.h |   1 +
 2 files changed, 90 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 83bcd13..8b1ba5b 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -149,14 +149,95 @@ static void xhci_link_rings(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
}
 }
 
+/*
+ * We need a radix tree for mapping physical addresses of TRBs to which stream
+ * ID they belong to.  We need to do this because the host controller won't 
tell
+ * us which stream ring the TRB came from.  We could store the stream ID in an
+ * event data TRB, but that doesn't help us for the cancellation case, since 
the
+ * endpoint may stop before it reaches that event data TRB.
+ *
+ * The radix tree maps the upper portion of the TRB DMA address to a ring
+ * segment that has the same upper portion of DMA addresses.  For example, say 
I
+ * have segments of size 1KB, that are always 64-byte aligned.  A segment may
+ * start at 0x10c91000 and end at 0x10c913f0.  If I use the upper 10 bits, the
+ * key to the stream ID is 0x43244.  I can use the DMA address of the TRB to
+ * pass the radix tree a key to get the right stream ID:
+ *
+ * 0x10c90fff >> 10 = 0x43243
+ * 0x10c912c0 >> 10 = 0x43244
+ * 0x10c91400 >> 10 = 0x43245
+ *
+ * Obviously, only those TRBs with DMA addresses that are within the segment
+ * will make the radix tree return the stream ID for that ring.
+ *
+ * Caveats for the radix tree:
+ *
+ * The radix tree uses an unsigned long as a key pair.  On 32-bit systems, an
+ * unsigned long will be 32-bits; on a 64-bit system an unsigned long will be
+ * 64-bits.  Since we only request 32-bit DMA addresses, we can use that as the
+ * key on 32-bit or 64-bit systems (it would also be fine if we asked for 
64-bit
+ * PCI DMA addresses on a 64-bit system).  There might be a problem on 32-bit
+ * extended systems (where the DMA address can be bigger than 32-bits),
+ * if we allow the PCI dma mask to be bigger than 32-bits.  So don't do that.
+ */
+static int xhci_update_stream_mapping(struct xhci_ring *ring, gfp_t mem_flags)
+{
+   struct xhci_segment *seg;
+   unsigned long key;
+   int ret;
+
+   if (WARN_ON_ONCE(ring->trb_address_map == NULL))
+   return 0;
+
+   seg = ring->first_seg;
+   do {
+   key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+   /* Skip any segments that were already added. */
+   if (radix_tree_lookup(ring->trb_address_map, key))
+   continue;
+
+   ret = radix_tree_maybe_preload(mem_flags);
+   if (ret)
+   return ret;
+   ret = radix_tree_insert(ring->trb_address_map,
+   key, ring);
+   radix_tree_preload_end();
+   if (ret)
+   return ret;
+   seg = seg->next;
+   } while (seg != ring->first_seg)

Re: [PATCH v3 3/6] uas: make work list per-device

[Gerd Hoffmann]

On Di, 2013-09-17 at 13:30 -0700, Christoph Hellwig wrote:
> On Fri, Sep 13, 2013 at 01:27:12PM +0200, Gerd Hoffmann wrote:
> > Simplifies locking, we'll protect the list with the device spin lock.
> > Also plugs races which can happen when two devices operate on the
> > global list.
> > 
> > While being at it rename the list head from "list" to "work", preparing
> > for the addition of a second list.
> 
> Why do you even the list?

The list was already there when I took over maintainance ...

> What would a ordered per-device workqueue not
> provide?

Had no reason to look into replacing the list with something else so
far.  Why do you suggest using a workqueue instead?

Note that the list is not used in a typical request workflow.  Only in
case queuing an usb urb failed the request is linked into the list and
the worker will try to submit the usb urb again.

cheers,
  Gerd


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

[PATCH v3 2/6] uas: properly reinitialize in uas_eh_bus_reset_handler

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d966b59..fc08ee9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -85,6 +85,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
 static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_configure_endpoints(struct uas_dev_info *devinfo);
+static void uas_free_streams(struct uas_dev_info *devinfo);
 
 static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_work_lock);
@@ -800,7 +802,10 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+   uas_free_streams(devinfo);
err = usb_reset_device(udev);
+   if (!err)
+   uas_configure_endpoints(devinfo);
devinfo->resetting = 0;
 
if (err) {
-- 
1.8.3.1

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

[PATCH v3 4/6] uas: add dead request list

[Gerd Hoffmann]

This patch adds a new list where all requests which are canceled are
added to, so we don't loose them.  Then, after killing all inflight
urbs on bus reset (and disconnect) we'll walk over the list and clean
them up.

Without this we can end up with aborted requests lingering around in
case of status pipe transfer errors.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 50 +++
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3cf5a5f..f049038 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -53,6 +53,7 @@ struct uas_dev_info {
spinlock_t lock;
struct work_struct work;
struct list_head work_list;
+   struct list_head dead_list;
 };
 
 enum {
@@ -80,6 +81,7 @@ struct uas_cmd_info {
struct urb *data_in_urb;
struct urb *data_out_urb;
struct list_head work;
+   struct list_head dead;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -89,6 +91,7 @@ static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
 static void uas_configure_endpoints(struct uas_dev_info *devinfo);
 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_unlink_data_urbs(struct uas_dev_info *devinfo,
 struct uas_cmd_info *cmdinfo)
@@ -150,16 +153,12 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
  SCp);
+   uas_log_cmd_state(cmnd, __func__);
+   WARN_ON(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
cmdinfo->state &= ~IS_IN_WORK_LIST;
-   if (devinfo->resetting) {
-   /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
-   cmdinfo->state &= ~COMMAND_INFLIGHT;
-   }
-   uas_try_complete(cmnd, __func__);
list_del(&cmdinfo->work);
+   list_add_tail(&cmdinfo->dead, &devinfo->dead_list);
}
spin_unlock_irqrestore(&devinfo->lock, flags);
 }
@@ -176,6 +175,28 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
schedule_work(&devinfo->work);
 }
 
+static void uas_zap_dead(struct uas_dev_info *devinfo)
+{
+   struct uas_cmd_info *cmdinfo;
+   struct uas_cmd_info *temp;
+   unsigned long flags;
+
+   spin_lock_irqsave(&devinfo->lock, flags);
+   list_for_each_entry_safe(cmdinfo, temp, &devinfo->dead_list, dead) {
+   struct scsi_pointer *scp = (void *)cmdinfo;
+   struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
+ SCp);
+   uas_log_cmd_state(cmnd, __func__);
+   WARN_ON(!(cmdinfo->state & COMMAND_ABORTED));
+   /* all urbs are killed, clear inflight bits */
+   cmdinfo->state &= ~(COMMAND_INFLIGHT |
+   DATA_IN_URB_INFLIGHT |
+   DATA_OUT_URB_INFLIGHT);
+   uas_try_complete(cmnd, __func__);
+   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
 static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 {
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -263,6 +284,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
if (cmdinfo->state & COMMAND_ABORTED) {
scmd_printk(KERN_INFO, cmnd, "abort completed\n");
cmnd->result = DID_ABORT << 16;
+   list_del(&cmdinfo->dead);
}
cmnd->scsi_done(cmnd);
return 0;
@@ -292,7 +314,13 @@ static void uas_stat_cmplt(struct urb *urb)
u16 tag;
 
if (urb->status) {
-   dev_err(&urb->dev->dev, "URB BAD STATUS %d\n", urb->status);
+   if (urb->status == -ENOENT) {
+   dev_err(&urb->dev->dev, "stat urb: killed, stream %d\n",
+   urb->stream_id);
+   } else {
+   dev_err(&urb->dev->dev, "stat urb: status %d\n",
+   urb->status);
+   }
usb_free_urb(urb);
return;
}
@@ -743,7 +771,9 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 
  

[PATCH v3 3/6] uas: make work list per-device

[Gerd Hoffmann]

Simplifies locking, we'll protect the list with the device spin lock.
Also plugs races which can happen when two devices operate on the
global list.

While being at it rename the list head from "list" to "work", preparing
for the addition of a second list.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 106 +++---
 1 file changed, 44 insertions(+), 62 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index fc08ee9..3cf5a5f 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -51,6 +51,8 @@ struct uas_dev_info {
unsigned uas_sense_old:1;
struct scsi_cmnd *cmnd;
spinlock_t lock;
+   struct work_struct work;
+   struct list_head work_list;
 };
 
 enum {
@@ -77,7 +79,7 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
-   struct list_head list;
+   struct list_head work;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -88,10 +90,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller);
 static void uas_configure_endpoints(struct uas_dev_info *devinfo);
 static void uas_free_streams(struct uas_dev_info *devinfo);
 
-static DECLARE_WORK(uas_work, uas_do_work);
-static DEFINE_SPINLOCK(uas_work_lock);
-static LIST_HEAD(uas_work_list);
-
 static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
 struct uas_cmd_info *cmdinfo)
 {
@@ -118,75 +116,66 @@ static void uas_unlink_data_urbs(struct uas_dev_info 
*devinfo,
 
 static void uas_do_work(struct work_struct *work)
 {
+   struct uas_dev_info *devinfo =
+   container_of(work, struct uas_dev_info, work);
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
-   struct list_head list;
unsigned long flags;
int err;
 
-   spin_lock_irq(&uas_work_lock);
-   list_replace_init(&uas_work_list, &list);
-   spin_unlock_irq(&uas_work_lock);
-
-   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   spin_lock_irqsave(&devinfo->lock, flags);
+   list_for_each_entry_safe(cmdinfo, temp, &devinfo->work_list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
-   struct scsi_cmnd *cmnd = container_of(scp,
-   struct scsi_cmnd, SCp);
-   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
-   spin_lock_irqsave(&devinfo->lock, flags);
+   struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
+ SCp);
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
-   if (!err)
+   if (!err) {
cmdinfo->state &= ~IS_IN_WORK_LIST;
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   if (err) {
-   list_del(&cmdinfo->list);
-   spin_lock_irq(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
-   spin_unlock_irq(&uas_work_lock);
-   schedule_work(&uas_work);
+   list_del(&cmdinfo->work);
+   } else {
+   schedule_work(&devinfo->work);
}
}
+   spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
 static void uas_abort_work(struct uas_dev_info *devinfo)
 {
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
-   struct list_head list;
unsigned long flags;
 
-   spin_lock_irq(&uas_work_lock);
-   list_replace_init(&uas_work_list, &list);
-   spin_unlock_irq(&uas_work_lock);
-
spin_lock_irqsave(&devinfo->lock, flags);
-   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   list_for_each_entry_safe(cmdinfo, temp, &devinfo->work_list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
-   struct scsi_cmnd *cmnd = container_of(scp,
-   struct scsi_cmnd, SCp);
-   struct uas_dev_info *di = (void *)cmnd->device->hostdata;
-
-   if (di == devinfo) {
-   cmdinfo->state |= COMMAND_ABORTED;
-   cmdinfo->state &= ~IS_IN_WORK_LIST;
-   if (devinfo->resetting) {
-   /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
-   cmdinfo->state &= ~COMMAND_INFLIGHT;
-   

[PATCH v3 5/6] uas: replace BUG_ON() + WARN_ON() with WARN_ON_ONCE()

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f049038..046eedf 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -154,7 +154,7 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
  SCp);
uas_log_cmd_state(cmnd, __func__);
-   WARN_ON(cmdinfo->state & COMMAND_ABORTED);
+   WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
cmdinfo->state &= ~IS_IN_WORK_LIST;
list_del(&cmdinfo->work);
@@ -169,7 +169,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(!spin_is_locked(&devinfo->lock));
+   WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
list_add_tail(&cmdinfo->work, &devinfo->work_list);
cmdinfo->state |= IS_IN_WORK_LIST;
schedule_work(&devinfo->work);
@@ -187,7 +187,7 @@ static void uas_zap_dead(struct uas_dev_info *devinfo)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
  SCp);
uas_log_cmd_state(cmnd, __func__);
-   WARN_ON(!(cmdinfo->state & COMMAND_ABORTED));
+   WARN_ON_ONCE(!(cmdinfo->state & COMMAND_ABORTED));
/* all urbs are killed, clear inflight bits */
cmdinfo->state &= ~(COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
@@ -271,13 +271,13 @@ 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(!spin_is_locked(&devinfo->lock));
+   WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
  DATA_OUT_URB_INFLIGHT |
  UNLINK_DATA_URBS))
return -EBUSY;
-   BUG_ON(cmdinfo->state & COMMAND_COMPLETED);
+   WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
usb_free_urb(cmdinfo->data_in_urb);
usb_free_urb(cmdinfo->data_out_urb);
@@ -398,8 +398,9 @@ static void uas_data_cmplt(struct urb *urb)
sdb = scsi_out(cmnd);
cmdinfo->state &= ~DATA_OUT_URB_INFLIGHT;
}
-   BUG_ON(sdb == NULL);
-   if (urb->status) {
+   if (sdb == NULL) {
+   WARN_ON_ONCE(1);
+   } else if (urb->status) {
/* error: no data transfered */
sdb->resid = sdb->length;
} else {
@@ -573,7 +574,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
int err;
 
-   WARN_ON(!spin_is_locked(&devinfo->lock));
+   WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & SUBMIT_STATUS_URB) {
err = uas_submit_sense_urb(cmnd->device->host, gfp,
   cmdinfo->stream);
@@ -771,7 +772,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 
uas_log_cmd_state(cmnd, __func__);
spin_lock_irqsave(&devinfo->lock, flags);
-   WARN_ON(cmdinfo->state & COMMAND_ABORTED);
+   WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
list_add_tail(&cmdinfo->dead, &devinfo->dead_list);
if (cmdinfo->state & IS_IN_WORK_LIST) {
-- 
1.8.3.1

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

[PATCH v3 6/6] uas: remove BROKEN

[Gerd Hoffmann]

xhci streams support is fixed, unblock usb attached scsi.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
index 8470e1b..4761a28 100644
--- a/drivers/usb/storage/Kconfig
+++ b/drivers/usb/storage/Kconfig
@@ -202,7 +202,7 @@ config USB_STORAGE_ENE_UB6250
 
 config USB_UAS
tristate "USB Attached SCSI"
-   depends on SCSI && BROKEN
+   depends on SCSI
help
  The USB Attached SCSI protocol is supported by some USB
  storage devices.  It permits higher performance by supporting
-- 
1.8.3.1

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

[PATCH v3 0/6] make uas fly

[Gerd Hoffmann]

  Hi,

This patch series enables uas support in the linux kernel.  First patch
fixes usb3 streams support in xhci, patches 2-5 improve uas error
handling and patch 6 removes BROKEN from the uas kernel config.

New in v3: Fix the race pointed out by Oliver, on both existing code and
the patch.  Added patch which removes the BUG_ON()s

cheers,
  Gerd

Gerd Hoffmann (6):
  xhci: fix usb3 streams
  uas: properly reinitialize in uas_eh_bus_reset_handler
  uas: make work list per-device
  uas: add dead request list
  uas: replace BUG_ON() + WARN_ON() with WARN_ON_ONCE()
  uas: remove BROKEN

 drivers/usb/host/xhci-mem.c |  53 +++
 drivers/usb/host/xhci.h |   2 +
 drivers/usb/storage/Kconfig |   2 +-
 drivers/usb/storage/uas.c   | 158 +---
 4 files changed, 133 insertions(+), 82 deletions(-)

-- 
1.8.3.1

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

[PATCH v3 1/6] xhci: fix usb3 streams

[Gerd Hoffmann]

xhci maintains a radix tree for each stream endpoint because it must
be able to map a trb address to the stream ring.  Each ring segment
must be added to the ring for this to work.  Currently xhci sticks
only the first segment of each stream ring into the radix tree.

Result is that things work initially, but as soon as the first segment
is full xhci can't map the trb address from the completion event to the
stream ring any more -> BOOM.  You'll find this message in the logs:

  ERROR Transfer event for disabled endpoint or incorrect stream ring

This patch adds a helper function to update the radix tree.  It can
both insert and remove ring segments.  It loops over the segment list
and handles all segments instead of just the first.  It is called
whenever an update is needed:  When allocating a ring, when expanding
a ring and when releasing a ring.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/host/xhci-mem.c | 53 ++---
 drivers/usb/host/xhci.h |  2 ++
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6f8c2fd..de8b006 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -154,8 +154,11 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct 
xhci_ring *ring)
if (!ring)
return;
 
-   if (ring->first_seg)
+   if (ring->first_seg) {
+   if (ring->type == TYPE_STREAM)
+   xhci_update_stream_ring(ring, false);
xhci_free_segments_for_ring(xhci, ring->first_seg);
+   }
 
kfree(ring);
 }
@@ -351,6 +354,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
xhci_dbg(xhci, "ring expansion succeed, now has %d segments\n",
ring->num_segs);
 
+   if (ring->type == TYPE_STREAM) {
+   ret = xhci_update_stream_ring(ring, true);
+   WARN_ON(ret); /* FIXME */
+   }
+
return 0;
 }
 
@@ -601,6 +609,35 @@ static int xhci_test_radix_tree(struct xhci_hcd *xhci,
  * extended systems (where the DMA address can be bigger than 32-bits),
  * if we allow the PCI dma mask to be bigger than 32-bits.  So don't do that.
  */
+
+int xhci_update_stream_ring(struct xhci_ring *ring, bool insert)
+{
+   struct xhci_segment *seg;
+   unsigned long key;
+   bool present;
+   int ret;
+
+   if (WARN_ON_ONCE(ring->trb_address_map == NULL))
+   return 0;
+
+   seg = ring->first_seg;
+   do {
+   key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+   present = radix_tree_lookup(ring->trb_address_map, key) != NULL;
+   if (!present && insert) {
+   ret = radix_tree_insert(ring->trb_address_map,
+   key, ring);
+   if (ret)
+   return ret;
+   }
+   if (present && !insert)
+   radix_tree_delete(ring->trb_address_map, key);
+   seg = seg->next;
+   } while (seg != ring->first_seg);
+
+   return 0;
+}
+
 struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
unsigned int num_streams, gfp_t mem_flags)
@@ -608,7 +645,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
struct xhci_stream_info *stream_info;
u32 cur_stream;
struct xhci_ring *cur_ring;
-   unsigned long key;
u64 addr;
int ret;
 
@@ -663,6 +699,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
if (!cur_ring)
goto cleanup_rings;
cur_ring->stream_id = cur_stream;
+   cur_ring->trb_address_map = &stream_info->trb_address_map;
/* Set deq ptr, cycle bit, and stream context type */
addr = cur_ring->first_seg->dma |
SCT_FOR_CTX(SCT_PRI_TR) |
@@ -672,10 +709,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n",
cur_stream, (unsigned long long) addr);
 
-   key = (unsigned long)
-   (cur_ring->first_seg->dma >> TRB_SEGMENT_SHIFT);
-   ret = radix_tree_insert(&stream_info->trb_address_map,
-   key, cur_ring);
+   ret = xhci_update_stream_ring(cur_ring, true);
if (ret) {
xhci_ring_free(xhci, cur_ring);
stream_info->stream_rings[cur_stream] = NULL;
@@ -702,9 +736,6 @@ cleanup_rings:
for (cur_strea

Re: [PATCH 4/5] uas: add dead request list

[Gerd Hoffmann]

On Di, 2013-09-03 at 10:39 -0700, Sarah Sharp wrote:
> Don't you need to send an ABORT TASK message to the device to cancel the
> outstanding request for that stream ID?  I don't see that in this code.
> I see lots of URB cancellation code, but nothing to remove the request
> from the device-side queue.

It is there.  uas_eh_abort_handler() cancels a single request.  There is
also uas_eh_device_reset_handler() which will try a LOGICAL UNIT RESET.

Those might not work though, depending on the failure mode.  If your
usb3 streams stop working you can't cancel scsi requests that way.

> Or does it simply ensure that SCSI bus reset works?

The scsi layer invokes the uas_eh_bus_reset_handler() as last resort,
when everything else fails.  So, yes, there we'll have the sledge hammer
approach to recover: cancel all usb urbs, abort all requests, full usb
device reset + re-initialization.  But we hardly have another chance
when the less invasive methods to cancel a requests didn't work ...

> Plus, as Joe mentioned, this code is full of BUG_ON(), which is not
> friendly to users, and doesn't increase my confidence that the driver is
> ready to have CONFIG_BROKEN removed.

Huh?  Why you are thinking BUG_ON() is a indicator for bad code quality?
I'm using BUG_ON() like assert() in userspace, i.e. they are extra
sanity checks which should never ever trigger.

I can switch them to less disruptive WARN_ON() if that is the preferred
way these says.

cheers,
  Gerd


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

[PATCH 4/5] uas: add dead request list

[Gerd Hoffmann]

This patch adds a new list where all requests which are canceled are
added to, so we don't loose them.  Then, after killing all inflight
urbs on bus reset (and disconnect) we'll walk over the list and clean
them up.

Without this we can end up with aborted requests lingering around in
case of status pipe transfer errors.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 70 +--
 1 file changed, 62 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index db09bda..2b3ca29 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,6 +78,7 @@ struct uas_cmd_info {
struct urb *data_in_urb;
struct urb *data_out_urb;
struct list_head work;
+   struct list_head dead;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -87,10 +88,12 @@ static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
 static void uas_configure_endpoints(struct uas_dev_info *devinfo);
 static void uas_free_streams(struct uas_dev_info *devinfo);
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
 
 static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_lists_lock);
 static LIST_HEAD(uas_work_list);
+static LIST_HEAD(uas_dead_list);
 
 static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
 struct uas_cmd_info *cmdinfo)
@@ -167,15 +170,13 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct uas_dev_info *di = (void *)cmnd->device->hostdata;
 
if (di == devinfo) {
+   uas_log_cmd_state(cmnd, __func__);
+   BUG_ON(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->dead, &uas_dead_list);
+   spin_unlock_irq(&uas_lists_lock);
cmdinfo->state &= ~IS_IN_WORK_LIST;
-   if (devinfo->resetting) {
-   /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
-   cmdinfo->state &= ~COMMAND_INFLIGHT;
-   }
-   uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
list_del(&cmdinfo->work);
@@ -187,6 +188,43 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
+static void uas_zap_dead(struct uas_dev_info *devinfo)
+{
+   struct uas_cmd_info *cmdinfo;
+   struct uas_cmd_info *temp;
+   struct list_head list;
+   unsigned long flags;
+
+   spin_lock_irq(&uas_lists_lock);
+   list_replace_init(&uas_dead_list, &list);
+   spin_unlock_irq(&uas_lists_lock);
+
+   spin_lock_irqsave(&devinfo->lock, flags);
+   list_for_each_entry_safe(cmdinfo, temp, &list, dead) {
+   struct scsi_pointer *scp = (void *)cmdinfo;
+   struct scsi_cmnd *cmnd = container_of(scp,
+   struct scsi_cmnd, SCp);
+   struct uas_dev_info *di = (void *)cmnd->device->hostdata;
+
+   if (di == devinfo) {
+   uas_log_cmd_state(cmnd, __func__);
+   BUG_ON(!(cmdinfo->state & COMMAND_ABORTED));
+   /* all urbs are killed, clear inflight bits */
+   cmdinfo->state &= ~(COMMAND_INFLIGHT |
+   DATA_IN_URB_INFLIGHT |
+   DATA_OUT_URB_INFLIGHT);
+   uas_try_complete(cmnd, __func__);
+   } else {
+   /* not our uas device, relink into list */
+   list_del(&cmdinfo->dead);
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->dead, &uas_dead_list);
+   spin_unlock_irq(&uas_lists_lock);
+   }
+   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
 static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 {
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -274,6 +312,9 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
if (cmdinfo->state & COMMAND_ABORTED) {
scmd_printk(KERN_INFO, cmnd, "abort completed\n");
cmnd->result = DID_ABORT <&l

[PATCH 2/5] uas: properly reinitialize in uas_eh_bus_reset_handler

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d966b59..fc08ee9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -85,6 +85,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
 static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_configure_endpoints(struct uas_dev_info *devinfo);
+static void uas_free_streams(struct uas_dev_info *devinfo);
 
 static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_work_lock);
@@ -800,7 +802,10 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+   uas_free_streams(devinfo);
err = usb_reset_device(udev);
+   if (!err)
+   uas_configure_endpoints(devinfo);
devinfo->resetting = 0;
 
if (err) {
-- 
1.8.3.1

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

[PATCH 5/5] uas: remove BROKEN

[Gerd Hoffmann]

xhci streams support is fixed, unblock usb attached scsi.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
index 8470e1b..4761a28 100644
--- a/drivers/usb/storage/Kconfig
+++ b/drivers/usb/storage/Kconfig
@@ -202,7 +202,7 @@ config USB_STORAGE_ENE_UB6250
 
 config USB_UAS
tristate "USB Attached SCSI"
-   depends on SCSI && BROKEN
+   depends on SCSI
help
  The USB Attached SCSI protocol is supported by some USB
  storage devices.  It permits higher performance by supporting
-- 
1.8.3.1

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

[PATCH 3/5] uas: rename work list lock + list field

[Gerd Hoffmann]

This patch prepares for the addition of another list and renames the
work list lock and the list_head field in struct uas_cmd_info.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 50 +++
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index fc08ee9..db09bda 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -77,7 +77,7 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
-   struct list_head list;
+   struct list_head work;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -89,7 +89,7 @@ static void uas_configure_endpoints(struct uas_dev_info 
*devinfo);
 static void uas_free_streams(struct uas_dev_info *devinfo);
 
 static DECLARE_WORK(uas_work, uas_do_work);
-static DEFINE_SPINLOCK(uas_work_lock);
+static DEFINE_SPINLOCK(uas_lists_lock);
 static LIST_HEAD(uas_work_list);
 
 static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
@@ -124,11 +124,11 @@ static void uas_do_work(struct work_struct *work)
unsigned long flags;
int err;
 
-   spin_lock_irq(&uas_work_lock);
+   spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
-   spin_unlock_irq(&uas_work_lock);
+   spin_unlock_irq(&uas_lists_lock);
 
-   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -139,10 +139,10 @@ static void uas_do_work(struct work_struct *work)
cmdinfo->state &= ~IS_IN_WORK_LIST;
spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
-   list_del(&cmdinfo->list);
-   spin_lock_irq(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
-   spin_unlock_irq(&uas_work_lock);
+   list_del(&cmdinfo->work);
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->work, &uas_work_list);
+   spin_unlock_irq(&uas_lists_lock);
schedule_work(&uas_work);
}
}
@@ -155,12 +155,12 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct list_head list;
unsigned long flags;
 
-   spin_lock_irq(&uas_work_lock);
+   spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
-   spin_unlock_irq(&uas_work_lock);
+   spin_unlock_irq(&uas_lists_lock);
 
spin_lock_irqsave(&devinfo->lock, flags);
-   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -178,10 +178,10 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
-   list_del(&cmdinfo->list);
-   spin_lock_irq(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
-   spin_unlock_irq(&uas_work_lock);
+   list_del(&cmdinfo->work);
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->work, &uas_work_list);
+   spin_unlock_irq(&uas_lists_lock);
}
}
spin_unlock_irqrestore(&devinfo->lock, flags);
@@ -288,10 +288,10 @@ static void uas_xfer_data(struct urb *urb, struct 
scsi_cmnd *cmnd,
cmdinfo->state |= direction | SUBMIT_STATUS_URB;
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
if (err) {
-   spin_lock(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
+   spin_lock(&uas_lists_lock);
+   list_add_tail(&cmdinfo->work, &uas_work_list);
cmdinfo->state |= IS_IN_WORK_LIST;
-   spin_unlock(&uas_work_lock);
+   spin_unlock(&uas_lists_lock);
schedule_work(&uas_work);
}
 }
@@ -694,10 +694,10 @@ static int uas_queuecommand_l

[PATCH 1/5] xhci: fix usb3 streams

[Gerd Hoffmann]

xhci maintains a radix tree for each stream endpoint because it must
be able to map a trb address to the stream ring.  Each ring segment
must be added to the ring for this to work.  Currently xhci sticks
only the first segment of each stream ring into the radix tree.

Result is that things work initially, but as soon as the first segment
is full xhci can't map the trb address from the completion event to the
stream ring any more -> BOOM.  You'll find this message in the logs:

  ERROR Transfer event for disabled endpoint or incorrect stream ring

This patch adds a helper function to update the radix tree.  It can
both insert and remove ring segments.  It loops over the segment list
and handles all segments instead of just the first.  It is called
whenever an update is needed:  When allocating a ring, when expanding
a ring and when releasing a ring.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/host/xhci-mem.c | 53 ++---
 drivers/usb/host/xhci.h |  2 ++
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6f8c2fd..de8b006 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -154,8 +154,11 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct 
xhci_ring *ring)
if (!ring)
return;
 
-   if (ring->first_seg)
+   if (ring->first_seg) {
+   if (ring->type == TYPE_STREAM)
+   xhci_update_stream_ring(ring, false);
xhci_free_segments_for_ring(xhci, ring->first_seg);
+   }
 
kfree(ring);
 }
@@ -351,6 +354,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
xhci_dbg(xhci, "ring expansion succeed, now has %d segments\n",
ring->num_segs);
 
+   if (ring->type == TYPE_STREAM) {
+   ret = xhci_update_stream_ring(ring, true);
+   WARN_ON(ret); /* FIXME */
+   }
+
return 0;
 }
 
@@ -601,6 +609,35 @@ static int xhci_test_radix_tree(struct xhci_hcd *xhci,
  * extended systems (where the DMA address can be bigger than 32-bits),
  * if we allow the PCI dma mask to be bigger than 32-bits.  So don't do that.
  */
+
+int xhci_update_stream_ring(struct xhci_ring *ring, bool insert)
+{
+   struct xhci_segment *seg;
+   unsigned long key;
+   bool present;
+   int ret;
+
+   if (WARN_ON_ONCE(ring->trb_address_map == NULL))
+   return 0;
+
+   seg = ring->first_seg;
+   do {
+   key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+   present = radix_tree_lookup(ring->trb_address_map, key) != NULL;
+   if (!present && insert) {
+   ret = radix_tree_insert(ring->trb_address_map,
+   key, ring);
+   if (ret)
+   return ret;
+   }
+   if (present && !insert)
+   radix_tree_delete(ring->trb_address_map, key);
+   seg = seg->next;
+   } while (seg != ring->first_seg);
+
+   return 0;
+}
+
 struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
unsigned int num_streams, gfp_t mem_flags)
@@ -608,7 +645,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
struct xhci_stream_info *stream_info;
u32 cur_stream;
struct xhci_ring *cur_ring;
-   unsigned long key;
u64 addr;
int ret;
 
@@ -663,6 +699,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
if (!cur_ring)
goto cleanup_rings;
cur_ring->stream_id = cur_stream;
+   cur_ring->trb_address_map = &stream_info->trb_address_map;
/* Set deq ptr, cycle bit, and stream context type */
addr = cur_ring->first_seg->dma |
SCT_FOR_CTX(SCT_PRI_TR) |
@@ -672,10 +709,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n",
cur_stream, (unsigned long long) addr);
 
-   key = (unsigned long)
-   (cur_ring->first_seg->dma >> TRB_SEGMENT_SHIFT);
-   ret = radix_tree_insert(&stream_info->trb_address_map,
-   key, cur_ring);
+   ret = xhci_update_stream_ring(cur_ring, true);
if (ret) {
xhci_ring_free(xhci, cur_ring);
stream_info->stream_rings[cur_stream] = NULL;
@@ -702,9 +736,6 @@ cleanup_rings:
for (cur_strea

[PATCH v2 0/5] make uas fly

[Gerd Hoffmann]

  Hi,

This patch series enables uas support in the linux kernel.  First patch
fixes usb3 streams support in xhci, patches 2-4 improve uas error
handling and patch 5 removes BROKEN from the uas kernel config.

v2 changes:
 - use WARN_ON_ONCE() instead of BUG_ON()
 - make checkpatch happy (codestyle fixes).

cheers,
  Gerd

Gerd Hoffmann (5):
  xhci: fix usb3 streams
  uas: properly reinitialize in uas_eh_bus_reset_handler
  uas: rename work list lock + list field
  uas: add dead request list
  uas: remove BROKEN

 drivers/usb/host/xhci-mem.c |  53 ++-
 drivers/usb/host/xhci.h |   2 +
 drivers/usb/storage/Kconfig |   2 +-
 drivers/usb/storage/uas.c   | 123 
 4 files changed, 134 insertions(+), 46 deletions(-)

-- 
1.8.3.1

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

Re: [RfC PATCH] xhci: fix usb3 streams

[Gerd Hoffmann]

On Fr, 2013-08-30 at 09:48 -0700, Sarah Sharp wrote:
> Hi Gerd,
> 
> Thanks for catching this!  I have a UAS device now, but won't have time
> to test it for a week or so.
> 
> Can you please Cc me on patches to the xHCI driver in the future?
> Otherwise it gets lost in the other linux-usb mailing list traffic.

Hooked up scripts/get_maintainer.pl in my git config now, that should
take care of this in the future.

> > +   ret = xhci_update_stream_ring(cur_ring, 1);
> 
> Could you use true instead of 1 here?

Fixed here and in the other two places, resent with some uas patches on
top.

cheers,
  Gerd


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

[PATCH 3/5] uas: rename work list lock + list field

[Gerd Hoffmann]

This patch prepares for the addition of another list and renames the
work list lock and the list_head field in struct uas_cmd_info.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 50 +++
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f89202f..a63972a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -77,7 +77,7 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
-   struct list_head list;
+   struct list_head work;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -89,7 +89,7 @@ static void uas_configure_endpoints(struct uas_dev_info 
*devinfo);
 static void uas_free_streams(struct uas_dev_info *devinfo);
 
 static DECLARE_WORK(uas_work, uas_do_work);
-static DEFINE_SPINLOCK(uas_work_lock);
+static DEFINE_SPINLOCK(uas_lists_lock);
 static LIST_HEAD(uas_work_list);
 
 static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
@@ -124,11 +124,11 @@ static void uas_do_work(struct work_struct *work)
unsigned long flags;
int err;
 
-   spin_lock_irq(&uas_work_lock);
+   spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
-   spin_unlock_irq(&uas_work_lock);
+   spin_unlock_irq(&uas_lists_lock);
 
-   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -139,10 +139,10 @@ static void uas_do_work(struct work_struct *work)
cmdinfo->state &= ~IS_IN_WORK_LIST;
spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
-   list_del(&cmdinfo->list);
-   spin_lock_irq(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
-   spin_unlock_irq(&uas_work_lock);
+   list_del(&cmdinfo->work);
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->work, &uas_work_list);
+   spin_unlock_irq(&uas_lists_lock);
schedule_work(&uas_work);
}
}
@@ -155,12 +155,12 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct list_head list;
unsigned long flags;
 
-   spin_lock_irq(&uas_work_lock);
+   spin_lock_irq(&uas_lists_lock);
list_replace_init(&uas_work_list, &list);
-   spin_unlock_irq(&uas_work_lock);
+   spin_unlock_irq(&uas_lists_lock);
 
spin_lock_irqsave(&devinfo->lock, flags);
-   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   list_for_each_entry_safe(cmdinfo, temp, &list, work) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
@@ -178,10 +178,10 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
-   list_del(&cmdinfo->list);
-   spin_lock_irq(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
-   spin_unlock_irq(&uas_work_lock);
+   list_del(&cmdinfo->work);
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->work, &uas_work_list);
+   spin_unlock_irq(&uas_lists_lock);
}
}
spin_unlock_irqrestore(&devinfo->lock, flags);
@@ -288,10 +288,10 @@ static void uas_xfer_data(struct urb *urb, struct 
scsi_cmnd *cmnd,
cmdinfo->state |= direction | SUBMIT_STATUS_URB;
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
if (err) {
-   spin_lock(&uas_work_lock);
-   list_add_tail(&cmdinfo->list, &uas_work_list);
+   spin_lock(&uas_lists_lock);
+   list_add_tail(&cmdinfo->work, &uas_work_list);
cmdinfo->state |= IS_IN_WORK_LIST;
-   spin_unlock(&uas_work_lock);
+   spin_unlock(&uas_lists_lock);
schedule_work(&uas_work);
}
 }
@@ -694,10 +694,10 @@ static int uas_queuecommand_l

[PATCH 1/5] xhci: fix usb3 streams

[Gerd Hoffmann]

xhci maintains a radix tree for each stream endpoint because it must
be able to map a trb address to the stream ring.  Each ring segment
must be added to the ring for this to work.  Currently xhci sticks
only the first segment of each stream ring into the radix tree.

Result is that things work initially, but as soon as the first segment
is full xhci can't map the trb address from the completion event to the
stream ring any more -> BOOM.  You'll find this message in the logs:

  ERROR Transfer event for disabled endpoint or incorrect stream ring

This patch adds a helper function to update the radix tree.  It can
both insert and remove ring segments.  It loops over the segment list
and handles all segments instead of just the first.  It is called
whenever an update is needed:  When allocating a ring, when expanding
a ring and when releasing a ring.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/host/xhci-mem.c | 51 +
 drivers/usb/host/xhci.h |  2 ++
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6f8c2fd..c7fd88f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -154,8 +154,11 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct 
xhci_ring *ring)
if (!ring)
return;
 
-   if (ring->first_seg)
+   if (ring->first_seg) {
+   if (ring->type == TYPE_STREAM)
+   xhci_update_stream_ring(ring, false);
xhci_free_segments_for_ring(xhci, ring->first_seg);
+   }
 
kfree(ring);
 }
@@ -351,6 +354,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
xhci_dbg(xhci, "ring expansion succeed, now has %d segments\n",
ring->num_segs);
 
+   if (ring->type == TYPE_STREAM) {
+   ret = xhci_update_stream_ring(ring, true);
+   WARN_ON(ret); /* FIXME */
+   }
+
return 0;
 }
 
@@ -601,6 +609,33 @@ static int xhci_test_radix_tree(struct xhci_hcd *xhci,
  * extended systems (where the DMA address can be bigger than 32-bits),
  * if we allow the PCI dma mask to be bigger than 32-bits.  So don't do that.
  */
+
+int xhci_update_stream_ring(struct xhci_ring *ring, bool insert)
+{
+   struct xhci_segment *seg;
+   unsigned long key;
+   bool present;
+   int ret;
+
+   BUG_ON(ring->trb_address_map == NULL);
+   seg = ring->first_seg;
+   do {
+   key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+   present = radix_tree_lookup(ring->trb_address_map, key) != NULL;
+   if (!present && insert) {
+   ret = radix_tree_insert(ring->trb_address_map, key, 
ring);
+   if (ret)
+   return ret;
+   }
+   if (present && !insert) {
+   radix_tree_delete(ring->trb_address_map, key);
+   }
+   seg = seg->next;
+   } while (seg != ring->first_seg);
+
+   return 0;
+}
+
 struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
unsigned int num_streams, gfp_t mem_flags)
@@ -608,7 +643,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
struct xhci_stream_info *stream_info;
u32 cur_stream;
struct xhci_ring *cur_ring;
-   unsigned long key;
u64 addr;
int ret;
 
@@ -663,6 +697,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
if (!cur_ring)
goto cleanup_rings;
cur_ring->stream_id = cur_stream;
+   cur_ring->trb_address_map = &stream_info->trb_address_map;
/* Set deq ptr, cycle bit, and stream context type */
addr = cur_ring->first_seg->dma |
SCT_FOR_CTX(SCT_PRI_TR) |
@@ -672,10 +707,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n",
cur_stream, (unsigned long long) addr);
 
-   key = (unsigned long)
-   (cur_ring->first_seg->dma >> TRB_SEGMENT_SHIFT);
-   ret = radix_tree_insert(&stream_info->trb_address_map,
-   key, cur_ring);
+   ret = xhci_update_stream_ring(cur_ring, true);
if (ret) {
xhci_ring_free(xhci, cur_ring);
stream_info->stream_rings[cur_stream] = NULL;
@@ -702,9 +734,6 @@ cleanup_rings:
for (cur_stream = 1; cur_stream < num_streams; cur_stream++) {
   

[PATCH 4/5] uas: add dead request list

[Gerd Hoffmann]

This patch adds a new list where all requests which are canceled are
added to, so we don't loose them.  Then, after killing all inflight
urbs on bus reset (and disconnect) we'll walk over the list and clean
them up.

Without this we can end up with aborted requests lingering around in
case of status pipe transfer errors.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 69 +--
 1 file changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index a63972a..9dfb8f9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -78,6 +78,7 @@ struct uas_cmd_info {
struct urb *data_in_urb;
struct urb *data_out_urb;
struct list_head work;
+   struct list_head dead;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -87,10 +88,12 @@ static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
 static void uas_configure_endpoints(struct uas_dev_info *devinfo);
 static void uas_free_streams(struct uas_dev_info *devinfo);
+static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
 
 static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_lists_lock);
 static LIST_HEAD(uas_work_list);
+static LIST_HEAD(uas_dead_list);
 
 static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
 struct uas_cmd_info *cmdinfo)
@@ -167,15 +170,13 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
struct uas_dev_info *di = (void *)cmnd->device->hostdata;
 
if (di == devinfo) {
+   uas_log_cmd_state(cmnd, __func__);
+   BUG_ON(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->dead, &uas_dead_list);
+   spin_unlock_irq(&uas_lists_lock);
cmdinfo->state &= ~IS_IN_WORK_LIST;
-   if (devinfo->resetting) {
-   /* uas_stat_cmplt() will not do that
-* when a device reset is in
-* progress */
-   cmdinfo->state &= ~COMMAND_INFLIGHT;
-   }
-   uas_try_complete(cmnd, __func__);
} else {
/* not our uas device, relink into list */
list_del(&cmdinfo->work);
@@ -187,6 +188,43 @@ static void uas_abort_work(struct uas_dev_info *devinfo)
spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
+static void uas_zap_dead(struct uas_dev_info *devinfo)
+{
+   struct uas_cmd_info *cmdinfo;
+   struct uas_cmd_info *temp;
+   struct list_head list;
+   unsigned long flags;
+
+   spin_lock_irq(&uas_lists_lock);
+   list_replace_init(&uas_dead_list, &list);
+   spin_unlock_irq(&uas_lists_lock);
+
+   spin_lock_irqsave(&devinfo->lock, flags);
+   list_for_each_entry_safe(cmdinfo, temp, &list, dead) {
+   struct scsi_pointer *scp = (void *)cmdinfo;
+   struct scsi_cmnd *cmnd = container_of(scp,
+   struct scsi_cmnd, SCp);
+   struct uas_dev_info *di = (void *)cmnd->device->hostdata;
+
+   if (di == devinfo) {
+   uas_log_cmd_state(cmnd, __func__);
+   BUG_ON(!(cmdinfo->state & COMMAND_ABORTED));
+   /* all urbs are killed, clear inflight bits */
+   cmdinfo->state &= ~(COMMAND_INFLIGHT |
+   DATA_IN_URB_INFLIGHT |
+   DATA_OUT_URB_INFLIGHT);
+   uas_try_complete(cmnd, __func__);
+   } else {
+   /* not our uas device, relink into list */
+   list_del(&cmdinfo->dead);
+   spin_lock_irq(&uas_lists_lock);
+   list_add_tail(&cmdinfo->dead, &uas_dead_list);
+   spin_unlock_irq(&uas_lists_lock);
+   }
+   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
 static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 {
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -274,6 +312,9 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
if (cmdinfo->state & COMMAND_ABORTED) {
scmd_printk(KERN_INFO, cmnd, "abort completed\n");
cmnd->result = DID_ABORT <&l

[PATCH 5/5] uas: remove BROKEN

[Gerd Hoffmann]

xhci streams support is fixed, unblock usb attached scsi.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
index 8470e1b..4761a28 100644
--- a/drivers/usb/storage/Kconfig
+++ b/drivers/usb/storage/Kconfig
@@ -202,7 +202,7 @@ config USB_STORAGE_ENE_UB6250
 
 config USB_UAS
tristate "USB Attached SCSI"
-   depends on SCSI && BROKEN
+   depends on SCSI
help
  The USB Attached SCSI protocol is supported by some USB
  storage devices.  It permits higher performance by supporting
-- 
1.8.3.1

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

[PATCH 2/5] uas: properly reinitialize in uas_eh_bus_reset_handler

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index d966b59..f89202f 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -85,6 +85,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
 static void uas_do_work(struct work_struct *work);
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
+static void uas_configure_endpoints(struct uas_dev_info *devinfo);
+static void uas_free_streams(struct uas_dev_info *devinfo);
 
 static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_work_lock);
@@ -800,7 +802,11 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+   uas_free_streams(devinfo);
err = usb_reset_device(udev);
+   if (!err) {
+   uas_configure_endpoints(devinfo);
+   }
devinfo->resetting = 0;
 
if (err) {
-- 
1.8.3.1

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

[PATCH 0/5] make uas fly

[Gerd Hoffmann]

  Hi,

This patch series enables uas support in the linux kernel.  First patch
fixes usb3 streams support in xhci, patches 2-4 improve uas error
handling and patch 5 removes BROKEN from the uas kernel config.

cheers,
  Gerd

Gerd Hoffmann (5):
  xhci: fix usb3 streams
  uas: properly reinitialize in uas_eh_bus_reset_handler
  uas: rename work list lock + list field
  uas: add dead request list
  uas: remove BROKEN

 drivers/usb/host/xhci-mem.c |  51 +-
 drivers/usb/host/xhci.h |   2 +
 drivers/usb/storage/Kconfig |   2 +-
 drivers/usb/storage/uas.c   | 123 
 4 files changed, 132 insertions(+), 46 deletions(-)

-- 
1.8.3.1

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

[RfC PATCH] xhci: fix usb3 streams

[Gerd Hoffmann]

xhci maintains a radix tree for each stream endpoint because it must
be able to map a trb address to the stream ring.  Each ring segment
must be added to the ring for this to work.  Currently xhci sticks
only the first segment of each stream ring into the radix tree.

Result is that things work initially, but as soon as the first segment
is full xhci can't map the trb address from the completion event to the
stream ring any more -> BOOM.  You'll find this message in the logs:

  ERROR Transfer event for disabled endpoint or incorrect stream ring

This patch adds a helper function to update the radix tree.  It can
both insert and remove ring segments.  It loops over the segment list
and handles all segments instead of just the first.  It is called
whenever an update is needed:  When allocating a ring, when expanding
a ring and when releasing a ring.

It's RfC because (a) I wanna stress test this and the uas driver a bit
more before submitting for real and (b) because there is still a FIXME
to fix.  Reviews are welcome nevertheless.  Also sending to make people
aware I'm looking into this atm.

Latest bits are available from:
  git://git.kraxel.org/linux uas

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/host/xhci-mem.c | 51 +
 drivers/usb/host/xhci.h |  2 ++
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6f8c2fd..44cdb1d 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -154,8 +154,11 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct 
xhci_ring *ring)
if (!ring)
return;
 
-   if (ring->first_seg)
+   if (ring->first_seg) {
+   if (ring->type == TYPE_STREAM)
+   xhci_update_stream_ring(ring, 0);
xhci_free_segments_for_ring(xhci, ring->first_seg);
+   }
 
kfree(ring);
 }
@@ -351,6 +354,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
xhci_dbg(xhci, "ring expansion succeed, now has %d segments\n",
ring->num_segs);
 
+   if (ring->type == TYPE_STREAM) {
+   ret = xhci_update_stream_ring(ring, 1);
+   WARN_ON(ret); /* FIXME */
+   }
+
return 0;
 }
 
@@ -601,6 +609,33 @@ static int xhci_test_radix_tree(struct xhci_hcd *xhci,
  * extended systems (where the DMA address can be bigger than 32-bits),
  * if we allow the PCI dma mask to be bigger than 32-bits.  So don't do that.
  */
+
+int xhci_update_stream_ring(struct xhci_ring *ring, bool insert)
+{
+   struct xhci_segment *seg;
+   unsigned long key;
+   bool present;
+   int ret;
+
+   BUG_ON(ring->trb_address_map == NULL);
+   seg = ring->first_seg;
+   do {
+   key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+   present = radix_tree_lookup(ring->trb_address_map, key) != NULL;
+   if (!present && insert) {
+   ret = radix_tree_insert(ring->trb_address_map, key, 
ring);
+   if (ret)
+   return ret;
+   }
+   if (present && !insert) {
+   radix_tree_delete(ring->trb_address_map, key);
+   }
+   seg = seg->next;
+   } while (seg != ring->first_seg);
+
+   return 0;
+}
+
 struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
unsigned int num_streams, gfp_t mem_flags)
@@ -608,7 +643,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
struct xhci_stream_info *stream_info;
u32 cur_stream;
struct xhci_ring *cur_ring;
-   unsigned long key;
u64 addr;
int ret;
 
@@ -663,6 +697,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
if (!cur_ring)
goto cleanup_rings;
cur_ring->stream_id = cur_stream;
+   cur_ring->trb_address_map = &stream_info->trb_address_map;
/* Set deq ptr, cycle bit, and stream context type */
addr = cur_ring->first_seg->dma |
SCT_FOR_CTX(SCT_PRI_TR) |
@@ -672,10 +707,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct 
xhci_hcd *xhci,
xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n",
cur_stream, (unsigned long long) addr);
 
-   key = (unsigned long)
-   (cur_ring->first_seg->dma >> TRB_SEGMENT_SHIFT);
-   ret = radix_tree_insert(&stream_info->trb_address_map,
-   key, cur_ring);
+   ret = xhci_u

Re: USB2.0 disk format failure in windows guest

[Gerd Hoffmann]

  Hi,

> Fixing this will require qemu to copy the beginning and ending parts of
> these non-aligned qTDs into separate bounce buffers so that the URB
> length can be divisible by 512.

Worth trying:  http://www.kraxel.org/cgit/qemu/log/?h=usb.80

It puts the qemu usb passthrough code upside down.  All xfers will go
through a bounce buffer, requests are submitted via libusbx.  That
should fix it.  Of course there is the risk of regressions in other
areas as it is all new code.  Also make sure you have libusbx-devel
installed, otherwise qemu will fallback to the old code which uses usbfs
ioctls directly.

cheers,
  Gerd

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

Re: Fwd: PROBLEM: Permanent kernel panic in USB hub driver - 3.5.0-22

[Gerd Hoffmann]

  Hi,

>> However, I think the root cause is the same as
>> in previous cores, so it is still would be worth to analyze them.
>> Here is a new picture of the panic:
>> https://dl.dropbox.com/u/8276110/3.7.4%20panic.jpg
> 
> Do you have the UAS driver compiled in?  I see some functions that could
> only be called after the UAS driver allocates a streams context (i.e.
> xhci_stream_id_to_ring).  It doesn't seem to be related to the Set
> Address timeout crash that was your previous issue.

The crash could be a uas driver bug, please pick up fixes from
http://www.kraxel.org/cgit/linux/log/?h=uas (greg's usb-next tree will
do too).

That will not make the uas driver work due to the radix tree bug, but
the uas driver's error handling should be solid enougth that xhci driver
bugs don't make uas crash the box.

cheers,
  Gerd

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

Re: xhci streams bug

[Gerd Hoffmann]

On 01/31/13 15:34, Gerd Hoffmann wrote:
> [ all still in qemu, will cross-checking on real hardware ]

Done now.  Getting the same behavior with the TI demo board on a nec
xhci controller (express card).

cheers,
  Gerd

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

Re: xhci streams bug

[Gerd Hoffmann]

  Hi,

> I think it's because xhci doesn't manage the trb_address_map radix tree
> correctly.  I can only find a single radix_tree_insert() call in the
> code, and that one is for the initial segment.  But nobody seems to
> update the radix tree when linking the next segment ...

There seems to be a bit more fishy, a device reset doesn't bring the
device back online.

[  117.169453] scsi3 : uas
[  117.171072] usbcore: registered new interface driver uas
[  117.175060] scsi 3:0:0:0: Direct-Access QEMU QEMU HARDDISK
 1.3. PQ: 0 ANSI: 5
[  117.195589] sd 3:0:0:0: Attached scsi generic sg1 type 0
[  117.206834] sd 3:0:0:0: [sdb] 2097152 512-byte logical blocks: (1.07
GB/1.00 GiB)
[  117.223331] sd 3:0:0:0: [sdb] Write Protect is off
[  117.236356] sd 3:0:0:0: [sdb] Write cache: enabled, read cache:
enabled, doesn't support DPO or FUA
[  117.251144]  sdb: sdb1
[  117.266808] sd 3:0:0:0: [sdb] Attached SCSI disk

All fine so far.

[  117.324571] xhci_hcd :00:0f.0: ERROR Transfer event for disabled
endpoint or incorrect stream ring
[  117.325543] xhci_hcd :00:0f.0: @3c348550 3c8a8800
 0d60 01058000

Hitting stream ring link bug, status pipe stops working.

[  177.760380] sd 3:0:0:0: [sdb] uas_eh_abort_handler 88003c8ef600
tag 0, inflight: CMD
[  180.769264] scsi host3: uas_eh_task_mgmt: ABORT TASK timed out
[  180.778724] sd 3:0:0:0: [sdb] uas_eh_abort_handler 8800350f6d00
tag 1, inflight: CMD
[  183.780182] scsi host3: uas_eh_task_mgmt: ABORT TASK timed out
[  183.790096] sd 3:0:0:0: uas_eh_abort_handler 88002e859100 tag 2,
inflight: CMD
[  186.796318] scsi host3: uas_eh_task_mgmt: ABORT TASK timed out
[  186.799973] sd 3:0:0:0: uas_eh_device_reset_handler
[  189.805352] scsi host3: uas_eh_task_mgmt: LOGICAL UNIT RESET timed out

scsi / uas tries to recover via task management, which fails.
Probably due to the status pipe being hosed.
Could also be a bug though in qemu's tmf code though.

[  189.815757] usb 7-3: URB BAD STATUS -2
[  189.819638] usb 7-3: URB BAD STATUS -2
[  189.822628] usb 7-3: URB BAD STATUS -2
[  189.826979] usb 7-3: URB BAD STATUS -2
[  189.829789] usb 7-3: URB BAD STATUS -2
[  189.830699] usb 7-3: URB BAD STATUS -2

uas canceled inflight urbs here via usb_kill_anchored_urbs()

[  189.936982] usb 7-3: reset SuperSpeed USB device number 2 using xhci_hcd
[  189.956674] usb 7-3: Parent hub missing LPM exit latency info.  Power
management will be impacted.
[  189.958721] xhci_hcd :00:0f.0: xHCI xhci_drop_endpoint called
with disabled ep 88003d043700
[  189.964337] xhci_hcd :00:0f.0: xHCI xhci_drop_endpoint called
with disabled ep 88003d043740
[  189.968832] xhci_hcd :00:0f.0: xHCI xhci_drop_endpoint called
with disabled ep 88003d043780
[  189.970601] xhci_hcd :00:0f.0: xHCI xhci_drop_endpoint called
with disabled ep 
88003d0437c0

uas resets device.

[  189.974161] scsi host3: uas_eh_bus_reset_handler success

uas thinks we are fine again ...

[  189.978617] scsi host3: sense urb submission failure

... but we are not, sense pipe still broken.

[  199.979340] sd 3:0:0:0: uas_eh_abort_handler 88002e859100 tag 2,
inflight: s-st a-cmd s-cmd
[  199.991331] sd 3:0:0:0: abort completed
[  199.995088] sd 3:0:0:0: Device offlined - not ready after error recovery

scsi layer decides to take the device offline as the request (test
unit ready probably) didn't work.

[  199.999020] sd 3:0:0:0: Device offlined - not ready after error recovery
[  200.001558] sd 3:0:0:0: Device offlined - not ready after error recovery
[  200.003413] sd 3:0:0:0: [sdb] Unhandled error code
[  200.006862] sd 3:0:0:0: [sdb]
[  200.007532] Result: hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK
[  200.011701] sd 3:0:0:0: [sdb] CDB:
[  200.013864] Read(10): 28 00 00 00 08 00 00 00 08 00
[  200.015128] end_request: I/O error, dev sdb, sector 2048
[  200.016244] Buffer I/O error on device sdb1, logical block 0
[  200.017412] sd 3:0:0:0: [sdb] Unhandled error code
[  200.018384] sd 3:0:0:0: [sdb]
[  200.019023] Result: hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK
[  200.020161] sd 3:0:0:0: [sdb] CDB:
[  200.020846] Read(10): 28 00 00 00 01 68 00 00 08 00
[  200.023422] end_request: I/O error, dev sdb, sector 360
[  200.024636] Buffer I/O error on device sdb, logical block 45

scsi layer finally throws an I/O error.

But, hey, at least the machine is still fine, uas didn't crash in the
process ...

[ all still in qemu, will cross-checking on real hardware ]

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

xhci streams bug

[Gerd Hoffmann]

  Hi,

Started hacking streams support into qemu, trapped into this one:

[  218.807129] xhci_hcd :00:0f.0: ERROR Transfer event for disabled
endpoint or incorrect stream ring
[  218.808087] xhci_hcd :00:0f.0: @3c32d560 38342000
 0100 01078001

Triggers after xhci emulation stepping over the first link trb for a
stream ring.

I think it's because xhci doesn't manage the trb_address_map radix tree
correctly.  I can only find a single radix_tree_insert() call in the
code, and that one is for the initial segment.  But nobody seems to
update the radix tree when linking the next segment ...

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

Re: UAS questions

[Gerd Hoffmann]

On 01/27/13 13:54, Vlad Silman wrote:
> Hello,
> 
> I have a few questions regarding Linux UAS host-side and device-side drivers.
> I've seen that Linux UAS host driver supports the task management
> commands as defined by T10 UAS spec, such as ABORT TASK and LOGICAL
> UNIT RESET.
> I'm trying to work with Thermaltake BlacX dock that doesn't support
> these commands and it doesn't work with Linux UAS host driver.

uas should work nevertheless.  Under normal circumstances the device
management functions should not be used.  And even if they don't work,
at some point in error handling the scsi layer will invoke the bus_reset
handler which doesn't try to use task management functions any more and
just resets the device to recover ...

Be sure to run the latest bits (greg's usb tree or
http://www.kraxel.org/cgit/linux/log/?h=uas), there have been uas fixes
recently which are not merged upstream yet.

Might also be bugs in the xhci stream handling are tripping you up.

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

Re: [PATCH 2/2] usb-uas: update MAINTAINERS entry

[Gerd Hoffmann]

  Hi,

>> Problem is that uas is pretty much the only device using streams,
>> so uas will be the one who triggers any stream bugs in xhci.
>> I have no idea how solid xhci streams support is at the moment.
> 
> The xHCI streams support isn't well tested, because the UAS devices I
> had were so buggy that I couldn't fully test them, and the UAS driver
> didn't (doesn't still?) properly support cancellation or device reset.

There have been a number of improvements fixes there, it should do alot
better now.

> Also, I do know there are a couple of streams work-arounds that need to
> be created for the Intel Panther Point xHCI host controller, so I would
> suggest testing with an NEC/Renesas xHCI host instead.  I will get
> around to implementing them, but other bugs have taken a higher
> priority.

Hmm.  I have a nec xhci for that, but streams not working on Intel
pretty much blocks removing the BROKEN tag from uas :(

cheers,
  Gerd

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

Re: [PATCH 2/2] usb-uas: update MAINTAINERS entry

[Gerd Hoffmann]

  Hi,

>> Sarah, is there some way to avoid using streams?  The UAS specs seems to
>> imply using streams is mandatory when connected to a USB-3 port, is that
>> correct?  Is there some way to force usb3 devices into usb2 mode even
>> when plugged into a usb3 port?  I'd like to have a no_streams module
>> option if possible ...
> 
> Well, I think we want to use streams, that's the whole advantage of UAS
> over the old spec. 

Sure, but being able to turn them off for trouble-shooting purposes
would still be useful IMO.

> I wasn't aware that the bugs were in the xhci
> driver, I thought they were in the uas driver, but I could be totally
> wrong.

Oh, uas had bugs too, pretty serious ones included, no question.

> Oh, and any hints on what device on the market today actually follows
> the UAS spec so I can buy one for testing?

/me asked the same a while ago, here is the reply



I would suggest getting a TI UAS evaluation board.  They seem to be the
most stable UAS device out there:

http://www.ti.com/tool/tusb9261demo

I have one of their boards from a year or so ago, but I suspect there's
a new revision by now.  I got the sample from Kevin Main ,
and I suspect he might give you one for free as well.

Another option might be to use the Linux UAS gadget stack with a OMAP5
board with the Synopsis Designware 3 USB 3.0 device controller.  You
could talk with Sebastian Andrzej Siewior , since
he has been doing a lot of work on the UAS gadget driver lately.



cheers,
  Gerd

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

Re: [PATCH 2/2] usb-uas: update MAINTAINERS entry

[Gerd Hoffmann]

  Hi,

>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8ae709e..c5b37de 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7911,9 +7911,10 @@ F:drivers/net/wireless/ath/ar5523/
>>  USB ATTACHED SCSI
>>  M:  Matthew Wilcox 
>>  M:  Sarah Sharp 
>> +M:  Gerd Hoffmann 
> 
> Should Matthew be removed from this?

Dunno, Sarah?

> Also, any word on when I can remove the CONFIG_BROKEN marking on this
> driver?

With the patches in -next uas itself should be reasonable solid.

Problem is that uas is pretty much the only device using streams,
so uas will be the one who triggers any stream bugs in xhci.
I have no idea how solid xhci streams support is at the moment.

Sarah, is there some way to avoid using streams?  The UAS specs seems to
imply using streams is mandatory when connected to a USB-3 port, is that
correct?  Is there some way to force usb3 devices into usb2 mode even
when plugged into a usb3 port?  I'd like to have a no_streams module
option if possible ...

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

[PATCH v2] usb-uas: set max_lun and max_channel

[Gerd Hoffmann]

256 luns is what the sam-4 address method 0 can handle and what
the qemu uas emulation supports.  So pick that for now.

[ v2: unlike the other two max_* fields max_channel isn't max+1 ]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 29c5dab..2d40a11 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -991,6 +991,8 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
 
shost->max_cmd_len = 16 + 252;
shost->max_id = 1;
+   shost->max_lun = 256;
+   shost->max_channel = 0;
shost->sg_tablesize = udev->bus->sg_tablesize;
 
devinfo->intf = intf;
-- 
1.7.9.7

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

[PATCH 2/2] usb-uas: update MAINTAINERS entry

[Gerd Hoffmann]

Cc: Sarah Sharp 
Signed-off-by: Gerd Hoffmann 
---
 MAINTAINERS |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8ae709e..c5b37de 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7911,9 +7911,10 @@ F:   drivers/net/wireless/ath/ar5523/
 USB ATTACHED SCSI
 M: Matthew Wilcox 
 M: Sarah Sharp 
+M: Gerd Hoffmann 
 L: linux-usb@vger.kernel.org
 L: linux-s...@vger.kernel.org
-S: Supported
+S: Maintained
 F: drivers/usb/storage/uas.c
 
 USB CDC ETHERNET DRIVER
-- 
1.7.9.7

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

[PATCH 1/2] usb-uas: set max_lun and max_channel

[Gerd Hoffmann]

256 luns is what the sam-4 address method 0 can handle and what
the qemu uas emulation supports.  So pick that for now.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 29c5dab..0b72257 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -991,6 +991,8 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
 
shost->max_cmd_len = 16 + 252;
shost->max_id = 1;
+   shost->max_lun = 256;
+   shost->max_channel = 1;
shost->sg_tablesize = udev->bus->sg_tablesize;
 
devinfo->intf = intf;
-- 
1.7.9.7

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

[PATCH v2 0/6] uas: error handling fixes

[Gerd Hoffmann]

  Hi,

New version of the uas error handling patch series.
See also https://bugzilla.kernel.org/show_bug.cgi?id=51031

Gained two new commits to also handle bus reset and
disconnect better.

Also available from
  git://git.kraxel.org/linux uas

[ git tree includes an additional debug commit ]

cheers,
  Gerd

Gerd Hoffmann (6):
  uas: new function to cancel data urbs
  uas: add UNLINK_DATA_URBS flag
  uas: add IS_IN_WORK_LIST flag
  uas: improve abort handler
  uas: improve device reset
  uas: fail any request submitted while resetting the device.

 drivers/usb/storage/uas.c |  122 +++--
 1 files changed, 106 insertions(+), 16 deletions(-)

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

[PATCH 3/6] uas: add IS_IN_WORK_LIST flag

[Gerd Hoffmann]

Keep track whenever the request is linked into the work list or not.
Needed for request abort.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   10 --
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index a972e53..05f1f2b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -67,6 +67,7 @@ enum {
COMMAND_COMPLETED   = (1 << 11),
COMMAND_ABORTED = (1 << 12),
UNLINK_DATA_URBS= (1 << 13),
+   IS_IN_WORK_LIST = (1 << 14),
 };
 
 /* Overrides scsi_pointer */
@@ -131,6 +132,8 @@ static void uas_do_work(struct work_struct *work)
struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
spin_lock_irqsave(&devinfo->lock, flags);
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
+   if (!err)
+   cmdinfo->state &= ~IS_IN_WORK_LIST;
spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
list_del(&cmdinfo->list);
@@ -193,7 +196,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -207,7 +210,8 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
(ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT"   : "",
(ci->state & COMMAND_COMPLETED) ? " done"  : "",
(ci->state & COMMAND_ABORTED)   ? " abort" : "",
-   (ci->state & UNLINK_DATA_URBS)  ? " unlink": "");
+   (ci->state & UNLINK_DATA_URBS)  ? " unlink": "",
+   (ci->state & IS_IN_WORK_LIST)   ? " work"  : "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -244,6 +248,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd 
*cmnd,
if (err) {
spin_lock(&uas_work_lock);
list_add_tail(&cmdinfo->list, &uas_work_list);
+   cmdinfo->state |= IS_IN_WORK_LIST;
spin_unlock(&uas_work_lock);
schedule_work(&uas_work);
}
@@ -643,6 +648,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
}
spin_lock(&uas_work_lock);
list_add_tail(&cmdinfo->list, &uas_work_list);
+   cmdinfo->state |= IS_IN_WORK_LIST;
spin_unlock(&uas_work_lock);
schedule_work(&uas_work);
}
-- 
1.7.1

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

[PATCH 1/6] uas: new function to cancel data urbs

[Gerd Hoffmann]

Add uas_unlink_data_urbs function to cancel in-flight data urbs.
Moves existing code into a separate function.

[ v2: also drop the locking, just call usb_unlink_urb no matter what,
  which is safe because the usb core guarantees the completion
  callback is called only once ]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   22 --
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 98b98ee..8b58e5e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -87,6 +87,15 @@ static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_work_lock);
 static LIST_HEAD(uas_work_list);
 
+static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
+struct uas_cmd_info *cmdinfo)
+{
+   if (cmdinfo->data_in_urb)
+   usb_unlink_urb(cmdinfo->data_in_urb);
+   if (cmdinfo->data_out_urb)
+   usb_unlink_urb(cmdinfo->data_out_urb);
+}
+
 static void uas_do_work(struct work_struct *work)
 {
struct uas_cmd_info *cmdinfo;
@@ -274,16 +283,9 @@ static void uas_stat_cmplt(struct urb *urb)
uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   usb_unlink_urb(cmdinfo->data_in_urb);
-   spin_lock_irqsave(&devinfo->lock, flags);
-   }
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT) {
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   usb_unlink_urb(cmdinfo->data_out_urb);
-   spin_lock_irqsave(&devinfo->lock, flags);
-   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   uas_unlink_data_urbs(devinfo, cmdinfo);
+   spin_lock_irqsave(&devinfo->lock, flags);
}
cmdinfo->state &= ~COMMAND_INFLIGHT;
uas_try_complete(cmnd, __func__);
-- 
1.7.1

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

[PATCH 2/6] uas: add UNLINK_DATA_URBS flag

[Gerd Hoffmann]

uas_unlink_data_urbs uses this to make sure the the scsi command is
not released while looking at it.  This will be needed when we start
calling uas_unlink_data_urbs in the request cancel code paths.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   24 +---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 8b58e5e..a972e53 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -66,6 +66,7 @@ enum {
DATA_OUT_URB_INFLIGHT   = (1 << 10),
COMMAND_COMPLETED   = (1 << 11),
COMMAND_ABORTED = (1 << 12),
+   UNLINK_DATA_URBS= (1 << 13),
 };
 
 /* Overrides scsi_pointer */
@@ -90,10 +91,25 @@ static LIST_HEAD(uas_work_list);
 static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
 struct uas_cmd_info *cmdinfo)
 {
+   unsigned long flags;
+
+   /*
+* The UNLINK_DATA_URBS flag makes sure uas_try_complete
+* (called by urb completion) doesn't release cmdinfo
+* underneath us.
+*/
+   spin_lock_irqsave(&devinfo->lock, flags);
+   cmdinfo->state |= UNLINK_DATA_URBS;
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+
if (cmdinfo->data_in_urb)
usb_unlink_urb(cmdinfo->data_in_urb);
if (cmdinfo->data_out_urb)
usb_unlink_urb(cmdinfo->data_out_urb);
+
+   spin_lock_irqsave(&devinfo->lock, flags);
+   cmdinfo->state &= ~UNLINK_DATA_URBS;
+   spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
 static void uas_do_work(struct work_struct *work)
@@ -177,7 +193,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -190,7 +206,8 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
(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 & COMMAND_ABORTED)   ? " abort" : "",
+   (ci->state & UNLINK_DATA_URBS)  ? " unlink": "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -201,7 +218,8 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
WARN_ON(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
- DATA_OUT_URB_INFLIGHT))
+ DATA_OUT_URB_INFLIGHT |
+ UNLINK_DATA_URBS))
return -EBUSY;
BUG_ON(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
-- 
1.7.1

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

[PATCH 6/6] uas: fail any request submitted while resetting the device.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 547f96a..ebb9972 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -644,6 +644,12 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
 
+   if (devinfo->resetting) {
+   cmnd->result = DID_ERROR << 16;
+   cmnd->scsi_done(cmnd);
+   return 0;
+   }
+
spin_lock_irqsave(&devinfo->lock, flags);
if (devinfo->cmnd) {
spin_unlock_irqrestore(&devinfo->lock, flags);
-- 
1.7.1

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

[PATCH 5/6] uas: improve device reset

[Gerd Hoffmann]

Add new function to unlink and abort requests from the work
list, call it on bus reset and disconnect where we kill all
in-flight urbs.  Also reorder calls in disconnect to first
cancel transfers, then remove the scsi hba.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   45 -
 1 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 5416f2a..547f96a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -84,6 +84,7 @@ struct uas_cmd_info {
 static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo, gfp_t gfp);
 static void uas_do_work(struct work_struct *work);
+static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller);
 
 static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_work_lock);
@@ -145,6 +146,45 @@ static void uas_do_work(struct work_struct *work)
}
 }
 
+static void uas_abort_work(struct uas_dev_info *devinfo)
+{
+   struct uas_cmd_info *cmdinfo;
+   struct uas_cmd_info *temp;
+   struct list_head list;
+   unsigned long flags;
+
+   spin_lock_irq(&uas_work_lock);
+   list_replace_init(&uas_work_list, &list);
+   spin_unlock_irq(&uas_work_lock);
+
+   spin_lock_irqsave(&devinfo->lock, flags);
+   list_for_each_entry_safe(cmdinfo, temp, &list, list) {
+   struct scsi_pointer *scp = (void *)cmdinfo;
+   struct scsi_cmnd *cmnd = container_of(scp,
+   struct scsi_cmnd, SCp);
+   struct uas_dev_info *di = (void *)cmnd->device->hostdata;
+
+   if (di == devinfo) {
+   cmdinfo->state |= COMMAND_ABORTED;
+   cmdinfo->state &= ~IS_IN_WORK_LIST;
+   if (devinfo->resetting) {
+   /* uas_stat_cmplt() will not do that
+* when a device reset is in
+* progress */
+   cmdinfo->state &= ~COMMAND_INFLIGHT;
+   }
+   uas_try_complete(cmnd, __func__);
+   } else {
+   /* not our uas device, relink into list */
+   list_del(&cmdinfo->list);
+   spin_lock_irq(&uas_work_lock);
+   list_add_tail(&cmdinfo->list, &uas_work_list);
+   spin_unlock_irq(&uas_work_lock);
+   }
+   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
 static void uas_sense(struct urb *urb, struct scsi_cmnd *cmnd)
 {
struct sense_iu *sense_iu = urb->transfer_buffer;
@@ -750,6 +790,7 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
int err;
 
devinfo->resetting = 1;
+   uas_abort_work(devinfo);
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
@@ -995,10 +1036,12 @@ static void uas_disconnect(struct usb_interface *intf)
struct Scsi_Host *shost = usb_get_intfdata(intf);
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
-   scsi_remove_host(shost);
+   devinfo->resetting = 1;
+   uas_abort_work(devinfo);
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
+   scsi_remove_host(shost);
uas_free_streams(devinfo);
kfree(devinfo);
 }
-- 
1.7.1

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

[PATCH 4/6] uas: improve abort handler

[Gerd Hoffmann]

Two changes.  First we check whenever the request is linked in the work
list and if so take it out.  Second check whenever the command is
actually in flight before asking the device to cancel it via task
management, and in case it isn't just zap the data urbs and finish it.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   19 +--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 05f1f2b..5416f2a 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -715,8 +715,23 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
uas_log_cmd_state(cmnd, __func__);
spin_lock_irqsave(&devinfo->lock, flags);
cmdinfo->state |= COMMAND_ABORTED;
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
+   if (cmdinfo->state & IS_IN_WORK_LIST) {
+   spin_lock(&uas_work_lock);
+   list_del(&cmdinfo->list);
+   cmdinfo->state &= ~IS_IN_WORK_LIST;
+   spin_unlock(&uas_work_lock);
+   }
+   if (cmdinfo->state & COMMAND_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
+   } else {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   uas_unlink_data_urbs(devinfo, cmdinfo);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   uas_try_complete(cmnd, __func__);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   ret = SUCCESS;
+   }
return ret;
 }
 
-- 
1.7.1

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

Re: [PATCH 1/4] uas: new function to cancel data urbs

[Gerd Hoffmann]

 +static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
 +   struct uas_cmd_info *cmdinfo)
 +{
 +  unsigned long flags;
 +
 +  spin_lock_irqsave(&devinfo->lock, flags);
 +  if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
 +  spin_unlock_irqrestore(&devinfo->lock, flags);
>>
>> urb_unlink_urb may call the completion callback which in turn grabs the
>> lock to update cmdinfo->state, so we must drop it to avoid deadlocks.
> 
> But what is the point of taking it at all if the result of the check may be 
> reversed
> when you act upon it?

Good question.  I'm doing all cmdinfo->state access under lock to avoid
races, but I can see how this is kida pointless here.  Guess I can just
call urb_unlink_urb no matter what as the usb core guarantees the
completion handler is called only once.

Is it safe to call urb_unlink_urb twice on the same urb?  Or must I take
care to not do that?

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

Re: [PATCH 4/4] uas: improve abort handler

[Gerd Hoffmann]

On 11/29/12 14:29, Oliver Neukum wrote:
> On Thursday 29 November 2012 14:06:15 Gerd Hoffmann wrote:
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index dd23b61..5f498db 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -717,8 +717,22 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
>> uas_log_cmd_state(cmnd, __func__);
>> spin_lock_irqsave(&devinfo->lock, flags);
>> cmdinfo->state |= COMMAND_ABORTED;
>> -   spin_unlock_irqrestore(&devinfo->lock, flags);
>> -   ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
>> +   if (cmdinfo->state & IS_IN_WORK_LIST) {
>> +   spin_lock_irq(&uas_work_lock);
> 
> a) it makes no sense to take the _irq version while you hold an _irqsave

Will fix.

> b) are you sure this sequence of locks is safe deadlockwise?

Yes.  No other lock is acquired anywhere while holding uas_work_lock.

cheers,
  Gerd

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

Re: [PATCH 1/4] uas: new function to cancel data urbs

[Gerd Hoffmann]

On 11/29/12 14:20, Oliver Neukum wrote:
> On Thursday 29 November 2012 14:06:12 Gerd Hoffmann wrote:
>> Add uas_unlink_data_urbs function to cancel in-flight data urbs.
>> Moves existing code into a separate function.
>>
>> Signed-off-by: Gerd Hoffmann 
>> ---
>>  drivers/usb/storage/uas.c |   32 ++--
>>  1 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index 98b98ee..c348afa 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -87,6 +87,25 @@ static DECLARE_WORK(uas_work, uas_do_work);
>>  static DEFINE_SPINLOCK(uas_work_lock);
>>  static LIST_HEAD(uas_work_list);
>>  
>> +static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
>> + struct uas_cmd_info *cmdinfo)
>> +{
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(&devinfo->lock, flags);
>> +if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
>> +spin_unlock_irqrestore(&devinfo->lock, flags);

urb_unlink_urb may call the completion callback which in turn grabs the
lock to update cmdinfo->state, so we must drop it to avoid deadlocks.

cheers,
  Gerd

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

[PATCH 0/4] uas: error handling fixes

[Gerd Hoffmann]

  Hi,

Trying to address https://bugzilla.kernel.org/show_bug.cgi?id=51031

cheers,
  Gerd

Gerd Hoffmann (4):
  uas: new function to cancel data urbs
  uas: add UNLINK_DATA_URBS flag
  uas: add IS_IN_WORK_LIST flag
  uas: improve abort handler

 drivers/usb/storage/uas.c |   72 +++-
 1 files changed, 57 insertions(+), 15 deletions(-)

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

[PATCH 3/4] uas: add IS_IN_WORK_LIST flag

[Gerd Hoffmann]

Keep track whenever the request is linked into the work list or not.
Needed for request abort.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   10 --
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1ebe974..dd23b61 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -67,6 +67,7 @@ enum {
COMMAND_COMPLETED   = (1 << 11),
COMMAND_ABORTED = (1 << 12),
UNLINK_DATA_URBS= (1 << 13),
+   IS_IN_WORK_LIST = (1 << 14),
 };
 
 /* Overrides scsi_pointer */
@@ -133,6 +134,8 @@ static void uas_do_work(struct work_struct *work)
struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
spin_lock_irqsave(&devinfo->lock, flags);
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
+   if (!err)
+   cmdinfo->state &= ~IS_IN_WORK_LIST;
spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
list_del(&cmdinfo->list);
@@ -195,7 +198,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -209,7 +212,8 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
(ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT"   : "",
(ci->state & COMMAND_COMPLETED) ? " done"  : "",
(ci->state & COMMAND_ABORTED)   ? " abort" : "",
-   (ci->state & UNLINK_DATA_URBS)  ? " unlink": "");
+   (ci->state & UNLINK_DATA_URBS)  ? " unlink": "",
+   (ci->state & IS_IN_WORK_LIST)   ? " work"  : "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -246,6 +250,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd 
*cmnd,
if (err) {
spin_lock(&uas_work_lock);
list_add_tail(&cmdinfo->list, &uas_work_list);
+   cmdinfo->state |= IS_IN_WORK_LIST;
spin_unlock(&uas_work_lock);
schedule_work(&uas_work);
}
@@ -645,6 +650,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
}
spin_lock(&uas_work_lock);
list_add_tail(&cmdinfo->list, &uas_work_list);
+   cmdinfo->state |= IS_IN_WORK_LIST;
spin_unlock(&uas_work_lock);
schedule_work(&uas_work);
}
-- 
1.7.1

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

[PATCH 4/4] uas: improve abort handler

[Gerd Hoffmann]

Two changes.  First we check whenever the request is linked in the work
list and if so take it out.  Second check whenever the command is
actually in flight before asking the device to cancel it via task
management, and in case it isn't just zap the data urbs and finish it.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   18 --
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index dd23b61..5f498db 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -717,8 +717,22 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
uas_log_cmd_state(cmnd, __func__);
spin_lock_irqsave(&devinfo->lock, flags);
cmdinfo->state |= COMMAND_ABORTED;
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
+   if (cmdinfo->state & IS_IN_WORK_LIST) {
+   spin_lock_irq(&uas_work_lock);
+   list_del(&cmdinfo->list);
+   cmdinfo->state &= ~IS_IN_WORK_LIST;
+   spin_unlock_irq(&uas_work_lock);
+   }
+   if (cmdinfo->state & COMMAND_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
+   } else {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   uas_unlink_data_urbs(devinfo, cmdinfo);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   uas_try_complete(cmnd, __func__);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   }
return ret;
 }
 
-- 
1.7.1

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

[PATCH 2/4] uas: add UNLINK_DATA_URBS flag

[Gerd Hoffmann]

uas_unlink_data_urbs uses this to make sure the the scsi command is
not released while looking at it.  This will be needed when we start
calling uas_unlink_data_urbs in the request cancel code paths.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   16 +---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index c348afa..1ebe974 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -66,6 +66,7 @@ enum {
DATA_OUT_URB_INFLIGHT   = (1 << 10),
COMMAND_COMPLETED   = (1 << 11),
COMMAND_ABORTED = (1 << 12),
+   UNLINK_DATA_URBS= (1 << 13),
 };
 
 /* Overrides scsi_pointer */
@@ -92,7 +93,13 @@ static void uas_unlink_data_urbs(struct uas_dev_info 
*devinfo,
 {
unsigned long flags;
 
+   /*
+* The UNLINK_DATA_URBS flag makes sure uas_try_complete
+* (called by urb completion) doesn't release cmdinfo
+* underneath us.
+*/
spin_lock_irqsave(&devinfo->lock, flags);
+   cmdinfo->state |= UNLINK_DATA_URBS;
if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_in_urb);
@@ -103,6 +110,7 @@ static void uas_unlink_data_urbs(struct uas_dev_info 
*devinfo,
usb_unlink_urb(cmdinfo->data_out_urb);
spin_lock_irqsave(&devinfo->lock, flags);
}
+   cmdinfo->state &= ~UNLINK_DATA_URBS;
spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
@@ -187,7 +195,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -200,7 +208,8 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
(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 & COMMAND_ABORTED)   ? " abort" : "",
+   (ci->state & UNLINK_DATA_URBS)  ? " unlink": "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -211,7 +220,8 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
WARN_ON(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
- DATA_OUT_URB_INFLIGHT))
+ DATA_OUT_URB_INFLIGHT |
+ UNLINK_DATA_URBS))
return -EBUSY;
BUG_ON(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
-- 
1.7.1

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

[PATCH 1/4] uas: new function to cancel data urbs

[Gerd Hoffmann]

Add uas_unlink_data_urbs function to cancel in-flight data urbs.
Moves existing code into a separate function.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   32 ++--
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 98b98ee..c348afa 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -87,6 +87,25 @@ static DECLARE_WORK(uas_work, uas_do_work);
 static DEFINE_SPINLOCK(uas_work_lock);
 static LIST_HEAD(uas_work_list);
 
+static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
+struct uas_cmd_info *cmdinfo)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(&devinfo->lock, flags);
+   if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   usb_unlink_urb(cmdinfo->data_in_urb);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
+   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   usb_unlink_urb(cmdinfo->data_out_urb);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+}
+
 static void uas_do_work(struct work_struct *work)
 {
struct uas_cmd_info *cmdinfo;
@@ -274,16 +293,9 @@ static void uas_stat_cmplt(struct urb *urb)
uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   usb_unlink_urb(cmdinfo->data_in_urb);
-   spin_lock_irqsave(&devinfo->lock, flags);
-   }
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT) {
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   usb_unlink_urb(cmdinfo->data_out_urb);
-   spin_lock_irqsave(&devinfo->lock, flags);
-   }
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   uas_unlink_data_urbs(devinfo, cmdinfo);
+   spin_lock_irqsave(&devinfo->lock, flags);
}
cmdinfo->state &= ~COMMAND_INFLIGHT;
uas_try_complete(cmnd, __func__);
-- 
1.7.1

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

Re: xhci Portsc register issue

[Gerd Hoffmann]

  Hi,

> But I'm not sure, since I've never attempted to memory map PCI registers
> from userspace.  I was under the impression that you just can't do that.

You can mmap /sys/bus/pci/devices(${device}/resource0.  Just hacked up a
tool which dumps the capability registers this way:

http://www.kraxel.org/cgit/usb-tools/tree/usb-print-caps.c

cheers,
  Gerd

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

[PATCH 2/2] uas: fix gcc warning

[Gerd Hoffmann]

Streamline control flow so it is easier for gcc to follow which paths
can be taken and which can't.

Fixes "warning: 'cmdinfo' may be used uninitialized in this function"

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   18 --
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4218701..98b98ee 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -249,16 +249,18 @@ static void uas_stat_cmplt(struct urb *urb)
cmnd = devinfo->cmnd;
else
cmnd = scsi_host_find_tag(shost, tag - 1);
+
if (!cmnd) {
-   if (iu->iu_id != IU_ID_RESPONSE) {
-   usb_free_urb(urb);
-   spin_unlock_irqrestore(&devinfo->lock, flags);
-   return;
+   if (iu->iu_id == IU_ID_RESPONSE) {
+   /* store results for uas_eh_task_mgmt() */
+   memcpy(&devinfo->response, iu, 
sizeof(devinfo->response));
}
-   } else {
-   cmdinfo = (void *)&cmnd->SCp;
+   usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
+   return;
}
 
+   cmdinfo = (void *)&cmnd->SCp;
switch (iu->iu_id) {
case IU_ID_STATUS:
if (devinfo->cmnd == cmnd)
@@ -292,10 +294,6 @@ static void uas_stat_cmplt(struct urb *urb)
case IU_ID_WRITE_READY:
uas_xfer_data(urb, cmnd, SUBMIT_DATA_OUT_URB);
break;
-   case IU_ID_RESPONSE:
-   /* store results for uas_eh_task_mgmt() */
-   memcpy(&devinfo->response, iu, sizeof(devinfo->response));
-   break;
default:
scmd_printk(KERN_ERR, cmnd,
"Bogus IU (%d) received on status pipe\n", iu->iu_id);
-- 
1.7.1

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

[PATCH 1/2] uas: fix locking

[Gerd Hoffmann]

Forgot to unlock in the uas_eh_task_mgmt error paths.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1578909..4218701 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -649,12 +649,14 @@ static int uas_eh_task_mgmt(struct scsi_cmnd *cmnd,
shost_printk(KERN_INFO, shost,
 "%s: %s: submit sense urb failed\n",
 __func__, fname);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
return FAILED;
}
if (uas_submit_task_urb(cmnd, GFP_ATOMIC, function, tag)) {
shost_printk(KERN_INFO, shost,
 "%s: %s: submit task mgmt urb failed\n",
 __func__, fname);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
return FAILED;
}
spin_unlock_irqrestore(&devinfo->lock, flags);
-- 
1.7.1

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

Re: [usb:usb-next 66/70] drivers/usb/storage/uas.c:638:12: sparse: context imbalance in 'uas_eh_task_mgmt' - wrong count at exit

[Gerd Hoffmann]

On 09/26/12 05:06, Fengguang Wu wrote:
> Hi Gerd,
> 
> FYI, there are new sparse warnings show up in
> 
> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-next
> head:   655db7980596f0ad4f15f8f4c51beb3e705762de
> commit: e064852072c47b69f62325c6b7fa4a58332655bd [66/70] USB: uas: add locking
> 
> + drivers/usb/storage/uas.c:638:12: sparse: context imbalance in 
> 'uas_eh_task_mgmt' - wrong count at exit

Ok, that one is real, we don't unlock in the error paths, I'll send a
incremental fix in a moment.

>   drivers/usb/storage/uas.c: In function 'uas_stat_cmplt':
>   drivers/usb/storage/uas.c:280:15: warning: 'cmdinfo' may be used 
> uninitialized in this function [-Wmaybe-uninitialized]

Wrong, can't happen, although that one is hard to see for gcc.  I'll see
what I can do about it.

cheers,
  Gerd

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

Re: USB tree closed for 3.7

[Gerd Hoffmann]

On 09/25/12 15:45, Greg KH wrote:
> Hi all,
> 
> If you haven't sent me any pending patches for 3.7, it's a bit too late
> as I've now closed the usb-next tree for any new stuff for 3.7, unless
> it's bug fixes.
> 
> If you have sent me stuff and I've missed it, please let me know as I
> think my queue is now empty.

[ sent to the list only, guess I better Cc you in the future ]

http://marc.info/?l=linux-usb&m=134856283907957&w=2

cheers,
  Gerd

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

[PATCH v2 5/5] uas: add locking

[Gerd Hoffmann]

Add spinlock to protect uas data structures.

[ v2: s/GFP_NOIO/GFP_ATOMIC/, better don't sleep when holding a spinlock ]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   51 ++--
 1 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index df1d72e..1578909 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -50,6 +50,7 @@ struct uas_dev_info {
unsigned use_streams:1;
unsigned uas_sense_old:1;
struct scsi_cmnd *cmnd;
+   spinlock_t lock;
 };
 
 enum {
@@ -91,6 +92,7 @@ static void uas_do_work(struct work_struct *work)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
struct list_head list;
+   unsigned long flags;
int err;
 
spin_lock_irq(&uas_work_lock);
@@ -101,7 +103,10 @@ static void uas_do_work(struct work_struct *work)
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
-   err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO);
+   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+   spin_lock_irqsave(&devinfo->lock, flags);
+   err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
list_del(&cmdinfo->list);
spin_lock_irq(&uas_work_lock);
@@ -182,7 +187,9 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
 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(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
  DATA_OUT_URB_INFLIGHT))
@@ -222,6 +229,7 @@ static void uas_stat_cmplt(struct urb *urb)
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
struct scsi_cmnd *cmnd;
struct uas_cmd_info *cmdinfo;
+   unsigned long flags;
u16 tag;
 
if (urb->status) {
@@ -235,6 +243,7 @@ static void uas_stat_cmplt(struct urb *urb)
return;
}
 
+   spin_lock_irqsave(&devinfo->lock, flags);
tag = be16_to_cpup(&iu->tag) - 1;
if (tag == 0)
cmnd = devinfo->cmnd;
@@ -243,6 +252,7 @@ static void uas_stat_cmplt(struct urb *urb)
if (!cmnd) {
if (iu->iu_id != IU_ID_RESPONSE) {
usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
return;
}
} else {
@@ -262,10 +272,16 @@ static void uas_stat_cmplt(struct urb *urb)
uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+   if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_in_urb);
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
+   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_out_urb);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
}
cmdinfo->state &= ~COMMAND_INFLIGHT;
uas_try_complete(cmnd, __func__);
@@ -285,14 +301,18 @@ static void uas_stat_cmplt(struct urb *urb)
"Bogus IU (%d) received on status pipe\n", iu->iu_id);
}
usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
 static void uas_data_cmplt(struct urb *urb)
 {
struct scsi_cmnd *cmnd = urb->context;
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
struct scsi_data_buffer *sdb = NULL;
+   unsigned long flags;
 
+   spin_lock_irqsave(&devinfo->lock, flags);
if (cmdinfo->data_in_urb

[PATCH v2 4/5] uas: fix abort

[Gerd Hoffmann]

Properly report aborted commands.
Also don't access cmdinfo after kicking task management,
it may not be valid any more once it returns.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   11 ---
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 42976ec..df1d72e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -191,6 +191,10 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
cmdinfo->state |= COMMAND_COMPLETED;
usb_free_urb(cmdinfo->data_in_urb);
usb_free_urb(cmdinfo->data_out_urb);
+   if (cmdinfo->state & COMMAND_ABORTED) {
+   scmd_printk(KERN_INFO, cmnd, "abort completed\n");
+   cmnd->result = DID_ABORT << 16;
+   }
cmnd->scsi_done(cmnd);
return 0;
 }
@@ -303,9 +307,6 @@ static void uas_data_cmplt(struct urb *urb)
} else {
sdb->resid = sdb->length - urb->actual_length;
}
-   if (cmdinfo->state & COMMAND_ABORTED) {
-   return;
-   }
uas_try_complete(cmnd, __func__);
 }
 
@@ -654,10 +655,6 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
uas_log_cmd_state(cmnd, __func__);
cmdinfo->state |= COMMAND_ABORTED;
ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
-   usb_kill_urb(cmdinfo->data_in_urb);
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
-   usb_kill_urb(cmdinfo->data_out_urb);
return ret;
 }
 
-- 
1.7.1

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

[PATCH v2 1/5] uas: keep track of command urbs

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   11 ++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 638cd64..ab66365 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -41,6 +41,7 @@ struct sense_iu_old {
 struct uas_dev_info {
struct usb_interface *intf;
struct usb_device *udev;
+   struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
int qdepth, resetting;
@@ -431,6 +432,7 @@ static int uas_submit_task_urb(struct scsi_cmnd *cmnd, 
gfp_t gfp,
err = usb_submit_urb(urb, gfp);
if (err)
goto err;
+   usb_anchor_urb(urb, &devinfo->cmd_urbs);
 
return 0;
 
@@ -521,18 +523,22 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
if (cmdinfo->state & ALLOC_CMD_URB) {
cmdinfo->cmd_urb = uas_alloc_cmd_urb(devinfo, gfp, cmnd,
-   cmdinfo->stream);
+cmdinfo->stream);
if (!cmdinfo->cmd_urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo->state &= ~ALLOC_CMD_URB;
}
 
if (cmdinfo->state & SUBMIT_CMD_URB) {
+   usb_get_urb(cmdinfo->cmd_urb);
if (usb_submit_urb(cmdinfo->cmd_urb, gfp)) {
scmd_printk(KERN_INFO, cmnd,
"cmd urb submission failure\n");
return SCSI_MLQUEUE_DEVICE_BUSY;
}
+   usb_anchor_urb(cmdinfo->cmd_urb, &devinfo->cmd_urbs);
+   usb_put_urb(cmdinfo->cmd_urb);
+   cmdinfo->cmd_urb = NULL;
cmdinfo->state &= ~SUBMIT_CMD_URB;
cmdinfo->state |= COMMAND_INFLIGHT;
}
@@ -670,6 +676,7 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
int err;
 
devinfo->resetting = 1;
+   usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
err = usb_reset_device(udev);
@@ -868,6 +875,7 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo->intf = intf;
devinfo->udev = udev;
devinfo->resetting = 0;
+   init_usb_anchor(&devinfo->cmd_urbs);
init_usb_anchor(&devinfo->sense_urbs);
init_usb_anchor(&devinfo->data_urbs);
uas_configure_endpoints(devinfo);
@@ -913,6 +921,7 @@ static void uas_disconnect(struct usb_interface *intf)
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
scsi_remove_host(shost);
+   usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
uas_free_streams(devinfo);
-- 
1.7.1

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

[PATCH v2 3/5] uas: remove aborted field, replace with status bit.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1d326c5..42976ec 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -64,13 +64,13 @@ enum {
DATA_IN_URB_INFLIGHT= (1 << 9),
DATA_OUT_URB_INFLIGHT   = (1 << 10),
COMMAND_COMPLETED   = (1 << 11),
+   COMMAND_ABORTED = (1 << 12),
 };
 
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
unsigned int stream;
-   unsigned int aborted;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -163,7 +163,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -175,7 +175,8 @@ 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_COMPLETED) ? " done"  : "",
+   (ci->state & COMMAND_ABORTED)   ? " abort" : "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -302,7 +303,7 @@ static void uas_data_cmplt(struct urb *urb)
} else {
sdb->resid = sdb->length - urb->actual_length;
}
-   if (cmdinfo->aborted) {
+   if (cmdinfo->state & COMMAND_ABORTED) {
return;
}
uas_try_complete(cmnd, __func__);
@@ -570,7 +571,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
cmdinfo->state = SUBMIT_STATUS_URB |
ALLOC_CMD_URB | SUBMIT_CMD_URB;
-   cmdinfo->aborted = 0;
 
switch (cmnd->sc_data_direction) {
case DMA_FROM_DEVICE:
@@ -652,7 +652,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
int ret;
 
uas_log_cmd_state(cmnd, __func__);
-   cmdinfo->aborted = 1;
+   cmdinfo->state |= COMMAND_ABORTED;
ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
usb_kill_urb(cmdinfo->data_in_urb);
-- 
1.7.1

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

[PATCH v2 2/5] uas: fix task management

[Gerd Hoffmann]

Allocate one tag for task management functions and
use it properly.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ab66365..1d326c5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -611,7 +611,7 @@ static int uas_eh_task_mgmt(struct scsi_cmnd *cmnd,
 {
struct Scsi_Host *shost = cmnd->device->host;
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
-   u16 tag = ; /* FIXME */
+   u16 tag = devinfo->qdepth - 1;
 
memset(&devinfo->response, 0, sizeof(devinfo->response));
if (uas_submit_sense_urb(shost, GFP_NOIO, tag)) {
@@ -701,7 +701,7 @@ static int uas_slave_configure(struct scsi_device *sdev)
 {
struct uas_dev_info *devinfo = sdev->hostdata;
scsi_set_tag_type(sdev, MSG_ORDERED_TAG);
-   scsi_activate_tcq(sdev, devinfo->qdepth - 2);
+   scsi_activate_tcq(sdev, devinfo->qdepth - 3);
return 0;
 }
 
@@ -880,7 +880,7 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
init_usb_anchor(&devinfo->data_urbs);
uas_configure_endpoints(devinfo);
 
-   result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 2);
+   result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 3);
if (result)
goto free;
 
-- 
1.7.1

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

[PATCH v2 0/5] uas: bug fixes

[Gerd Hoffmann]

  Hi,

Resending whole uas bug fix patch series.  Patches #3 + #5 got updated,
addressing review comments.  The fixed patches have been on the list
already, but not yet the whole series as updated v2.

cheers,
  Gerd

The following changes since commit 56d27adcb536b7430d5f8a6240df8ad261eb00bd:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile 
(2012-09-24 16:17:17 -0700)

are available in the git repository at:

  git://git.kraxel.org/linux uas

Gerd Hoffmann (5):
  uas: keep track of command urbs
  uas: fix task management
  uas: remove aborted field, replace with status bit.
  uas: fix abort
  uas: add locking

 drivers/usb/storage/uas.c |   89 +
 1 files changed, 66 insertions(+), 23 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] uas: remove aborted field, replace with status bit.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1d326c5..42976ec 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -64,13 +64,13 @@ enum {
DATA_IN_URB_INFLIGHT= (1 << 9),
DATA_OUT_URB_INFLIGHT   = (1 << 10),
COMMAND_COMPLETED   = (1 << 11),
+   COMMAND_ABORTED = (1 << 12),
 };
 
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
unsigned int stream;
-   unsigned int aborted;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -163,7 +163,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -175,7 +175,8 @@ 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_COMPLETED) ? " done"  : "",
+   (ci->state & COMMAND_ABORTED)   ? " abort" : "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -302,7 +303,7 @@ static void uas_data_cmplt(struct urb *urb)
} else {
sdb->resid = sdb->length - urb->actual_length;
}
-   if (cmdinfo->aborted) {
+   if (cmdinfo->state & COMMAND_ABORTED) {
return;
}
uas_try_complete(cmnd, __func__);
@@ -570,7 +571,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
cmdinfo->state = SUBMIT_STATUS_URB |
ALLOC_CMD_URB | SUBMIT_CMD_URB;
-   cmdinfo->aborted = 0;
 
switch (cmnd->sc_data_direction) {
case DMA_FROM_DEVICE:
@@ -652,7 +652,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
int ret;
 
uas_log_cmd_state(cmnd, __func__);
-   cmdinfo->aborted = 1;
+   cmdinfo->state |= COMMAND_ABORTED;
ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
usb_kill_urb(cmdinfo->data_in_urb);
-- 
1.7.1

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

[PATCH v2] uas: remove aborted field, replace with status bit.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1d326c5..42976ec 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -64,13 +64,13 @@ enum {
DATA_IN_URB_INFLIGHT= (1 << 9),
DATA_OUT_URB_INFLIGHT   = (1 << 10),
COMMAND_COMPLETED   = (1 << 11),
+   COMMAND_ABORTED = (1 << 12),
 };
 
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
unsigned int stream;
-   unsigned int aborted;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -163,7 +163,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -175,7 +175,8 @@ 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_COMPLETED) ? " done"  : "",
+   (ci->state & COMMAND_ABORTED)   ? " abort" : "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -302,7 +303,7 @@ static void uas_data_cmplt(struct urb *urb)
} else {
sdb->resid = sdb->length - urb->actual_length;
}
-   if (cmdinfo->aborted) {
+   if (cmdinfo->state & COMMAND_ABORTED) {
return;
}
uas_try_complete(cmnd, __func__);
@@ -570,7 +571,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
cmdinfo->state = SUBMIT_STATUS_URB |
ALLOC_CMD_URB | SUBMIT_CMD_URB;
-   cmdinfo->aborted = 0;
 
switch (cmnd->sc_data_direction) {
case DMA_FROM_DEVICE:
@@ -652,7 +652,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
int ret;
 
uas_log_cmd_state(cmnd, __func__);
-   cmdinfo->aborted = 1;
+   cmdinfo->state |= COMMAND_ABORTED;
ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
usb_kill_urb(cmdinfo->data_in_urb);
-- 
1.7.1

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

[PATCH v2 5/5] uas: add locking

[Gerd Hoffmann]

Add spinlock to protect uas data structures.

[ v2: s/GFP_NOIO/GFP_ATOMIC/, better don't sleep when holding a spinlock ]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   51 ++--
 1 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index df1d72e..1578909 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -50,6 +50,7 @@ struct uas_dev_info {
unsigned use_streams:1;
unsigned uas_sense_old:1;
struct scsi_cmnd *cmnd;
+   spinlock_t lock;
 };
 
 enum {
@@ -91,6 +92,7 @@ static void uas_do_work(struct work_struct *work)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
struct list_head list;
+   unsigned long flags;
int err;
 
spin_lock_irq(&uas_work_lock);
@@ -101,7 +103,10 @@ static void uas_do_work(struct work_struct *work)
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
-   err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO);
+   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+   spin_lock_irqsave(&devinfo->lock, flags);
+   err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
list_del(&cmdinfo->list);
spin_lock_irq(&uas_work_lock);
@@ -182,7 +187,9 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
 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(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
  DATA_OUT_URB_INFLIGHT))
@@ -222,6 +229,7 @@ static void uas_stat_cmplt(struct urb *urb)
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
struct scsi_cmnd *cmnd;
struct uas_cmd_info *cmdinfo;
+   unsigned long flags;
u16 tag;
 
if (urb->status) {
@@ -235,6 +243,7 @@ static void uas_stat_cmplt(struct urb *urb)
return;
}
 
+   spin_lock_irqsave(&devinfo->lock, flags);
tag = be16_to_cpup(&iu->tag) - 1;
if (tag == 0)
cmnd = devinfo->cmnd;
@@ -243,6 +252,7 @@ static void uas_stat_cmplt(struct urb *urb)
if (!cmnd) {
if (iu->iu_id != IU_ID_RESPONSE) {
usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
return;
}
} else {
@@ -262,10 +272,16 @@ static void uas_stat_cmplt(struct urb *urb)
uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+   if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_in_urb);
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
+   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_out_urb);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
}
cmdinfo->state &= ~COMMAND_INFLIGHT;
uas_try_complete(cmnd, __func__);
@@ -285,14 +301,18 @@ static void uas_stat_cmplt(struct urb *urb)
"Bogus IU (%d) received on status pipe\n", iu->iu_id);
}
usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
 static void uas_data_cmplt(struct urb *urb)
 {
struct scsi_cmnd *cmnd = urb->context;
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
struct scsi_data_buffer *sdb = NULL;
+   unsigned long flags;
 
+   spin_lock_irqsave(&devinfo->lock, flags);
if (cmdinfo->data_in_urb

[PATCH 4/5] uas: fix abort

[Gerd Hoffmann]

Properly report aborted commands.
Also don't access cmdinfo after kicking task management,
it may not be valid any more once it returns.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   11 ---
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 42976ec..df1d72e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -191,6 +191,10 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const 
char *caller)
cmdinfo->state |= COMMAND_COMPLETED;
usb_free_urb(cmdinfo->data_in_urb);
usb_free_urb(cmdinfo->data_out_urb);
+   if (cmdinfo->state & COMMAND_ABORTED) {
+   scmd_printk(KERN_INFO, cmnd, "abort completed\n");
+   cmnd->result = DID_ABORT << 16;
+   }
cmnd->scsi_done(cmnd);
return 0;
 }
@@ -303,9 +307,6 @@ static void uas_data_cmplt(struct urb *urb)
} else {
sdb->resid = sdb->length - urb->actual_length;
}
-   if (cmdinfo->state & COMMAND_ABORTED) {
-   return;
-   }
uas_try_complete(cmnd, __func__);
 }
 
@@ -654,10 +655,6 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
uas_log_cmd_state(cmnd, __func__);
cmdinfo->state |= COMMAND_ABORTED;
ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
-   usb_kill_urb(cmdinfo->data_in_urb);
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
-   usb_kill_urb(cmdinfo->data_out_urb);
return ret;
 }
 
-- 
1.7.1

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

[PATCH 5/5] uas: add locking

[Gerd Hoffmann]

Add spinlock to protect uas data structures.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   45 +
 1 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index df1d72e..cb5c9e3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -50,6 +50,7 @@ struct uas_dev_info {
unsigned use_streams:1;
unsigned uas_sense_old:1;
struct scsi_cmnd *cmnd;
+   spinlock_t lock;
 };
 
 enum {
@@ -91,6 +92,7 @@ static void uas_do_work(struct work_struct *work)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
struct list_head list;
+   unsigned long flags;
int err;
 
spin_lock_irq(&uas_work_lock);
@@ -101,7 +103,10 @@ static void uas_do_work(struct work_struct *work)
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp,
struct scsi_cmnd, SCp);
+   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+   spin_lock_irqsave(&devinfo->lock, flags);
err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_NOIO);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
if (err) {
list_del(&cmdinfo->list);
spin_lock_irq(&uas_work_lock);
@@ -182,7 +187,9 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const 
char *caller)
 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(!spin_is_locked(&devinfo->lock));
if (cmdinfo->state & (COMMAND_INFLIGHT |
  DATA_IN_URB_INFLIGHT |
  DATA_OUT_URB_INFLIGHT))
@@ -222,6 +229,7 @@ static void uas_stat_cmplt(struct urb *urb)
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
struct scsi_cmnd *cmnd;
struct uas_cmd_info *cmdinfo;
+   unsigned long flags;
u16 tag;
 
if (urb->status) {
@@ -235,6 +243,7 @@ static void uas_stat_cmplt(struct urb *urb)
return;
}
 
+   spin_lock_irqsave(&devinfo->lock, flags);
tag = be16_to_cpup(&iu->tag) - 1;
if (tag == 0)
cmnd = devinfo->cmnd;
@@ -243,6 +252,7 @@ static void uas_stat_cmplt(struct urb *urb)
if (!cmnd) {
if (iu->iu_id != IU_ID_RESPONSE) {
usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
return;
}
} else {
@@ -262,10 +272,16 @@ static void uas_stat_cmplt(struct urb *urb)
uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
-   if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+   if (cmdinfo->state & DATA_IN_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_in_urb);
-   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
+   if (cmdinfo->state & DATA_OUT_URB_INFLIGHT) {
+   spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlink_urb(cmdinfo->data_out_urb);
+   spin_lock_irqsave(&devinfo->lock, flags);
+   }
}
cmdinfo->state &= ~COMMAND_INFLIGHT;
uas_try_complete(cmnd, __func__);
@@ -285,14 +301,18 @@ static void uas_stat_cmplt(struct urb *urb)
"Bogus IU (%d) received on status pipe\n", iu->iu_id);
}
usb_free_urb(urb);
+   spin_unlock_irqrestore(&devinfo->lock, flags);
 }
 
 static void uas_data_cmplt(struct urb *urb)
 {
struct scsi_cmnd *cmnd = urb->context;
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+   struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
struct scsi_data_buffer *sdb = NULL;
+   unsigned long flags;
 
+   spin_lock_irqsave(&devinfo->lock, flags);
if (cmdinfo->data_in_urb == urb) {
sdb = scsi_in(cmnd);
cmdinfo->state &= ~DATA_IN_URB_INFLIGHT;
@@ -308,6 +328,7 @@ static void uas_data_cmplt(struct urb *urb)
sdb->

[PATCH 0/5] uas: a bunch of bugfixes.

[Gerd Hoffmann]

  Hi,

Bugfix series for uas.  First fixing a bunch of little issues in
the scsi error handling.  Then I finally figured the reason we end
up in the scsi error handles in the first place is data corruption
due to lack of locking.  /me wonders why I didn't notice earlier,
seems my kernel coding skills suffered from hacking on qemu too
much.

cheers,
  Gerd

Gerd Hoffmann (5):
  uas: keep track of command urbs
  uas: fix task management
  uas: remove aborted field, replace with status bit.
  uas: fix abort
  uas: add locking

 drivers/usb/storage/uas.c |   83 ++---
 1 files changed, 63 insertions(+), 20 deletions(-)

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

[PATCH 2/5] uas: fix task management

[Gerd Hoffmann]

Allocate one tag for task management functions and
use it properly.

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ab66365..1d326c5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -611,7 +611,7 @@ static int uas_eh_task_mgmt(struct scsi_cmnd *cmnd,
 {
struct Scsi_Host *shost = cmnd->device->host;
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
-   u16 tag = ; /* FIXME */
+   u16 tag = devinfo->qdepth - 1;
 
memset(&devinfo->response, 0, sizeof(devinfo->response));
if (uas_submit_sense_urb(shost, GFP_NOIO, tag)) {
@@ -701,7 +701,7 @@ static int uas_slave_configure(struct scsi_device *sdev)
 {
struct uas_dev_info *devinfo = sdev->hostdata;
scsi_set_tag_type(sdev, MSG_ORDERED_TAG);
-   scsi_activate_tcq(sdev, devinfo->qdepth - 2);
+   scsi_activate_tcq(sdev, devinfo->qdepth - 3);
return 0;
 }
 
@@ -880,7 +880,7 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
init_usb_anchor(&devinfo->data_urbs);
uas_configure_endpoints(devinfo);
 
-   result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 2);
+   result = scsi_init_shared_tag_map(shost, devinfo->qdepth - 3);
if (result)
goto free;
 
-- 
1.7.1

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

[PATCH 1/5] uas: keep track of command urbs

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/usb/storage/uas.c |   11 ++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 638cd64..ab66365 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -41,6 +41,7 @@ struct sense_iu_old {
 struct uas_dev_info {
struct usb_interface *intf;
struct usb_device *udev;
+   struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
int qdepth, resetting;
@@ -431,6 +432,7 @@ static int uas_submit_task_urb(struct scsi_cmnd *cmnd, 
gfp_t gfp,
err = usb_submit_urb(urb, gfp);
if (err)
goto err;
+   usb_anchor_urb(urb, &devinfo->cmd_urbs);
 
return 0;
 
@@ -521,18 +523,22 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 
if (cmdinfo->state & ALLOC_CMD_URB) {
cmdinfo->cmd_urb = uas_alloc_cmd_urb(devinfo, gfp, cmnd,
-   cmdinfo->stream);
+cmdinfo->stream);
if (!cmdinfo->cmd_urb)
return SCSI_MLQUEUE_DEVICE_BUSY;
cmdinfo->state &= ~ALLOC_CMD_URB;
}
 
if (cmdinfo->state & SUBMIT_CMD_URB) {
+   usb_get_urb(cmdinfo->cmd_urb);
if (usb_submit_urb(cmdinfo->cmd_urb, gfp)) {
scmd_printk(KERN_INFO, cmnd,
"cmd urb submission failure\n");
return SCSI_MLQUEUE_DEVICE_BUSY;
}
+   usb_anchor_urb(cmdinfo->cmd_urb, &devinfo->cmd_urbs);
+   usb_put_urb(cmdinfo->cmd_urb);
+   cmdinfo->cmd_urb = NULL;
cmdinfo->state &= ~SUBMIT_CMD_URB;
cmdinfo->state |= COMMAND_INFLIGHT;
}
@@ -670,6 +676,7 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
int err;
 
devinfo->resetting = 1;
+   usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
err = usb_reset_device(udev);
@@ -868,6 +875,7 @@ static int uas_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
devinfo->intf = intf;
devinfo->udev = udev;
devinfo->resetting = 0;
+   init_usb_anchor(&devinfo->cmd_urbs);
init_usb_anchor(&devinfo->sense_urbs);
init_usb_anchor(&devinfo->data_urbs);
uas_configure_endpoints(devinfo);
@@ -913,6 +921,7 @@ static void uas_disconnect(struct usb_interface *intf)
struct uas_dev_info *devinfo = (void *)shost->hostdata[0];
 
scsi_remove_host(shost);
+   usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
uas_free_streams(devinfo);
-- 
1.7.1

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

[PATCH 3/5] uas: remove aborted field, replace with status bit.

[Gerd Hoffmann]

---
 drivers/usb/storage/uas.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1d326c5..42976ec 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -64,13 +64,13 @@ enum {
DATA_IN_URB_INFLIGHT= (1 << 9),
DATA_OUT_URB_INFLIGHT   = (1 << 10),
COMMAND_COMPLETED   = (1 << 11),
+   COMMAND_ABORTED = (1 << 12),
 };
 
 /* Overrides scsi_pointer */
 struct uas_cmd_info {
unsigned int state;
unsigned int stream;
-   unsigned int aborted;
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
@@ -163,7 +163,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 %p tag %d, inflight:"
-   "%s%s%s%s%s%s%s%s%s%s%s\n",
+   "%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, cmnd->request->tag,
(ci->state & SUBMIT_STATUS_URB) ? " s-st"  : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in"  : "",
@@ -175,7 +175,8 @@ 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_COMPLETED) ? " done"  : "",
+   (ci->state & COMMAND_ABORTED)   ? " abort" : "");
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -302,7 +303,7 @@ static void uas_data_cmplt(struct urb *urb)
} else {
sdb->resid = sdb->length - urb->actual_length;
}
-   if (cmdinfo->aborted) {
+   if (cmdinfo->state & COMMAND_ABORTED) {
return;
}
uas_try_complete(cmnd, __func__);
@@ -570,7 +571,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 
cmdinfo->state = SUBMIT_STATUS_URB |
ALLOC_CMD_URB | SUBMIT_CMD_URB;
-   cmdinfo->aborted = 0;
 
switch (cmnd->sc_data_direction) {
case DMA_FROM_DEVICE:
@@ -652,7 +652,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
int ret;
 
uas_log_cmd_state(cmnd, __func__);
-   cmdinfo->aborted = 1;
+   cmdinfo->state |= COMMAND_ABORTED;
ret = uas_eh_task_mgmt(cmnd, "ABORT TASK", TMF_ABORT_TASK);
if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
usb_kill_urb(cmdinfo->data_in_urb);
-- 
1.7.1

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

Re: disconnect and uas_work_list

[Gerd Hoffmann]

On 08/17/12 15:01, Sebastian Andrzej Siewior wrote:
> On Fri, Aug 17, 2012 at 02:13:40PM +0200, Oliver Neukum wrote:
>>> I just noticed some of my patches here got reverted. Do you have any 
>>> hardware
>>> or did you just stumble over it? I have just my target UAS gadget…
>>
>> No hardware. Do you have suggestions where to get hardware?
> 
> No. I've been asking around in stores but nothing. I've seen a lot different
> USB 3.0 sticks or hard disks but according to their descriptors they all been
> doing BOT. Sarah & friends had some early Intel thingy afaik. I have my own
> that is tcm_usb_gadget with dummy_hcd or any UDC you find.
> Gerd posted patches as well so maybe he knows where to get real hardware.

I don't know, /me has a single 3.0 stick which does BOT too.

I'm testing uas in a virtual machine, qemu 1.2 (to be released soon)
features uas emulation support.  Only usb 2.0 for now, qemu usb
emulation can't handle streams and other 3.0 stuff.  Yet.

cheers,
  Gerd

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