Re: [RfC PATCH] xhci: fix usb3 streams

2013-09-02 Thread 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


Re: [RfC PATCH] xhci: fix usb3 streams

2013-08-30 Thread Sarah Sharp
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.

One nit-picky comment below, but everything else looks good.

On Mon, Aug 26, 2013 at 02:31:10PM +0200, Gerd Hoffmann wrote:
 +
 +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;
 +}
 +
 @@ -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, 1);

Could you use true instead of 1 here?

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

2013-08-26 Thread 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 kra...@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..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_update_stream_ring(cur_ring, 1);
if (ret) {
xhci_ring_free(xhci, cur_ring);
stream_info-stream_rings[cur_stream] = NULL;