[PATCH 1/5] xhci: fix usb3 streams

[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 <kraxel@redhat.com>
---
 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++) {
 		cur_ring = stream_info->stream_rings[cur_stream];
 		if (cur_ring) {
-			addr = cur_ring->first_seg->dma;
-			radix_tree_delete(&stream_info->trb_address_map,
-					addr >> TRB_SEGMENT_SHIFT);
 			xhci_ring_free(xhci, cur_ring);
 			stream_info->stream_rings[cur_stream] = NULL;
 		}
@@ -764,7 +793,6 @@ void xhci_free_stream_info(struct xhci_hcd *xhci,
 {
 	int cur_stream;
 	struct xhci_ring *cur_ring;
-	dma_addr_t addr;
 
 	if (!stream_info)
 		return;
@@ -773,9 +801,6 @@ void xhci_free_stream_info(struct xhci_hcd *xhci,
 			cur_stream++) {
 		cur_ring = stream_info->stream_rings[cur_stream];
 		if (cur_ring) {
-			addr = cur_ring->first_seg->dma;
-			radix_tree_delete(&stream_info->trb_address_map,
-					addr >> TRB_SEGMENT_SHIFT);
 			xhci_ring_free(xhci, cur_ring);
 			stream_info->stream_rings[cur_stream] = NULL;
 		}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c338741..b525cfc 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1334,6 +1334,7 @@ struct xhci_ring {
 	unsigned int		num_trbs_free_temp;
 	enum xhci_ring_type	type;
 	bool			last_td_was_short;
+	struct radix_tree_root	*trb_address_map;
 };
 
 struct xhci_erst_entry {
@@ -1702,6 +1703,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci, struct xhci_virt_device *virt_dev,
 void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring);
 int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
 				unsigned int num_trbs, gfp_t flags);
+int xhci_update_stream_ring(struct xhci_ring *ring, bool insert);
 void xhci_free_or_cache_endpoint_ring(struct xhci_hcd *xhci,
 		struct xhci_virt_device *virt_dev,
 		unsigned int ep_index);
-- 
1.8.3.1

Re: [PATCH 1/5] xhci: fix usb3 streams

[Joe Perches]

On Mon, 2013-09-02 at 13:25 +0200, Gerd Hoffmann wrote:
> 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.
[]
> 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.
[]
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
[]
> @@ -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);

BUG_ON is really not nice.  Maybe:

	if (WARN_ON_ONCE(ring->trb_address_map == NULL))
		return 0;

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

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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

[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 <kraxel@redhat.com>
---
 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_lck(struct scsi_cmnd *cmnd,
 			spin_unlock_irqrestore(&devinfo->lock, flags);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
-		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);
 	}
 
@@ -764,10 +764,10 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 	spin_lock_irqsave(&devinfo->lock, flags);
 	cmdinfo->state |= COMMAND_ABORTED;
 	if (cmdinfo->state & IS_IN_WORK_LIST) {
-		spin_lock(&uas_work_lock);
-		list_del(&cmdinfo->list);
+		spin_lock(&uas_lists_lock);
+		list_del(&cmdinfo->work);
 		cmdinfo->state &= ~IS_IN_WORK_LIST;
-		spin_unlock(&uas_work_lock);
+		spin_unlock(&uas_lists_lock);
 	}
 	if (cmdinfo->state & COMMAND_INFLIGHT) {
 		spin_unlock_irqrestore(&devinfo->lock, flags);
-- 
1.8.3.1

[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 <kraxel@redhat.com>
---
 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 << 16;
+		spin_lock_irq(&uas_lists_lock);
+		list_del(&cmdinfo->dead);
+		spin_unlock_irq(&uas_lists_lock);
 	}
 	cmnd->scsi_done(cmnd);
 	return 0;
@@ -307,7 +348,12 @@ 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;
 	}
@@ -762,7 +808,11 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 
 	uas_log_cmd_state(cmnd, __func__);
 	spin_lock_irqsave(&devinfo->lock, flags);
+	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);
 	if (cmdinfo->state & IS_IN_WORK_LIST) {
 		spin_lock(&uas_lists_lock);
 		list_del(&cmdinfo->work);
@@ -797,11 +847,13 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
 	struct usb_device *udev = devinfo->udev;
 	int err;
 
+	shost_printk(KERN_INFO, sdev->host, "%s start\n", __func__);
 	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);
+	uas_zap_dead(devinfo);
 	uas_free_streams(devinfo);
 	err = usb_reset_device(udev);
 	if (!err) {
@@ -1055,6 +1107,7 @@ static void uas_disconnect(struct usb_interface *intf)
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
 	usb_kill_anchored_urbs(&devinfo->data_urbs);
+	uas_zap_dead(devinfo);
 	scsi_remove_host(shost);
 	uas_free_streams(devinfo);
 	kfree(devinfo);
-- 
1.8.3.1

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

[Sarah Sharp]

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.

Does this code currently handle the case where a device refuses to
respond to a SCSI request, and the upper layers attempt to cancel that
request?  Or does it simply ensure that SCSI bus reset works?

I'm not comfortable with removing CONFIG_BROKEN until the upper layers
can cancel one outstanding request.

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.

Sarah Sharp

On Mon, Sep 02, 2013 at 01:25:28PM +0200, Gerd Hoffmann wrote:
> 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 <kraxel@redhat.com>
> ---
>  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 << 16;
> +		spin_lock_irq(&uas_lists_lock);
> +		list_del(&cmdinfo->dead);
> +		spin_unlock_irq(&uas_lists_lock);
>  	}
>  	cmnd->scsi_done(cmnd);
>  	return 0;
> @@ -307,7 +348,12 @@ 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;
>  	}
> @@ -762,7 +808,11 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
>  
>  	uas_log_cmd_state(cmnd, __func__);
>  	spin_lock_irqsave(&devinfo->lock, flags);
> +	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);
>  	if (cmdinfo->state & IS_IN_WORK_LIST) {
>  		spin_lock(&uas_lists_lock);
>  		list_del(&cmdinfo->work);
> @@ -797,11 +847,13 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
>  	struct usb_device *udev = devinfo->udev;
>  	int err;
>  
> +	shost_printk(KERN_INFO, sdev->host, "%s start\n", __func__);
>  	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);
> +	uas_zap_dead(devinfo);
>  	uas_free_streams(devinfo);
>  	err = usb_reset_device(udev);
>  	if (!err) {
> @@ -1055,6 +1107,7 @@ static void uas_disconnect(struct usb_interface *intf)
>  	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
>  	usb_kill_anchored_urbs(&devinfo->sense_urbs);
>  	usb_kill_anchored_urbs(&devinfo->data_urbs);
> +	uas_zap_dead(devinfo);
>  	scsi_remove_host(shost);
>  	uas_free_streams(devinfo);
>  	kfree(devinfo);
> -- 
> 1.8.3.1
> 

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

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

[Oliver Neukum]

On Mon, 2013-09-02 at 13:25 +0200, Gerd Hoffmann wrote:
> +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);

This looks like a window for a race.

> +       spin_lock_irqsave(&devinfo->lock, flags);
> +       list_for_each_entry_safe(cmdinfo, temp, &list, dead) {
> +  

What happens if list entries are on the private list, when
the function is called for another device? It looks to me like
the di==devinfo test could put them back on the list although
they would need to be canceled.

	Regards
		Oliver

[PATCH 5/5] uas: remove BROKEN

[Gerd Hoffmann]

xhci streams support is fixed, unblock usb attached scsi.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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

[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 <kraxel@redhat.com>
---
 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_stream = 1; cur_stream < num_streams; cur_stream++) {
 		cur_ring = stream_info->stream_rings[cur_stream];
 		if (cur_ring) {
-			addr = cur_ring->first_seg->dma;
-			radix_tree_delete(&stream_info->trb_address_map,
-					addr >> TRB_SEGMENT_SHIFT);
 			xhci_ring_free(xhci, cur_ring);
 			stream_info->stream_rings[cur_stream] = NULL;
 		}
@@ -764,7 +795,6 @@ void xhci_free_stream_info(struct xhci_hcd *xhci,
 {
 	int cur_stream;
 	struct xhci_ring *cur_ring;
-	dma_addr_t addr;
 
 	if (!stream_info)
 		return;
@@ -773,9 +803,6 @@ void xhci_free_stream_info(struct xhci_hcd *xhci,
 			cur_stream++) {
 		cur_ring = stream_info->stream_rings[cur_stream];
 		if (cur_ring) {
-			addr = cur_ring->first_seg->dma;
-			radix_tree_delete(&stream_info->trb_address_map,
-					addr >> TRB_SEGMENT_SHIFT);
 			xhci_ring_free(xhci, cur_ring);
 			stream_info->stream_rings[cur_stream] = NULL;
 		}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c338741..b525cfc 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1334,6 +1334,7 @@ struct xhci_ring {
 	unsigned int		num_trbs_free_temp;
 	enum xhci_ring_type	type;
 	bool			last_td_was_short;
+	struct radix_tree_root	*trb_address_map;
 };
 
 struct xhci_erst_entry {
@@ -1702,6 +1703,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci, struct xhci_virt_device *virt_dev,
 void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring);
 int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
 				unsigned int num_trbs, gfp_t flags);
+int xhci_update_stream_ring(struct xhci_ring *ring, bool insert);
 void xhci_free_or_cache_endpoint_ring(struct xhci_hcd *xhci,
 		struct xhci_virt_device *virt_dev,
 		unsigned int ep_index);
-- 
1.8.3.1