Re: [PATCH v2] xhci: fix usb3 streams
The more I look at this patch, the more I hate it for the failure cases
it doesn't cover.
What happens if the radix_tree_insert fails in the middle of adding a
set of ring segments? We leave those segments that were inserted in the
radix tree, which is a problem, since we could allocate those segments
out of the DMA pool later for a different stream ID.
That's OK for the initial stream ring allocation, since the xhci_ring
itself will get freed. It's not ok for ring expansion through, since
the xhci_ring remains in tact, and we simply fail the URB submission.
I'm working on a patch to fix this, but may not get it done today.
Sarah Sharp
On Mon, Oct 14, 2013 at 06:54:24PM -0700, Gerd Hoffmann wrote:
> 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;
> +
Re: [PATCH v2] xhci: fix usb3 streams
On Wed, 16 Oct 2013, Sarah Sharp 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. > > There may be a simpler approach to this problem. > > > > When using a new ring segment, keep the first TRB entry in reserve. > > Don't put a normal TRB in there, instead leave it as a no-op entry > > containing a pointer to the stream ring. (Make the prior Link TRB > > point to the second entry in the new segment instead of the first.) > > > > Then you won't have to add to or remove anything from the radix tree. > > I don't understand how this would help. Are you advocating a different > way of mapping TRB DMA addresses to stream rings that would allow us to > ditch the radix tree all together? > > Ok, so with your solution, we have a virtual stream ring pointer as the > first TRB of the segment. We get an event with the DMA address of a TRB > in one of many stream rings on an endpoint. From that, I think we can > infer the DMA address of the first TRB on the segment, due to the > alignment requirements and ring size. > > And then what do we do with that? We don't have the virtual address of > that first TRB, so the xHCI driver can't read the ring pointer from it. > I'm confused as to what the next steps would be to solve this. My mistake; I misunderstood the original description of the problem. I didn't realize that "map a trb address" referred to the TRB's DMA address. BTW, ohci-hcd faces the same problem (of mapping DMA addresses to kernel addresses). It solves the problem with a hash table rather than a radix tree. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xhci: fix usb3 streams
On Tue, Oct 15, 2013 at 10:53:57AM -0400, Alan Stern wrote: > On Mon, 14 Oct 2013, Gerd Hoffmann wrote: > > > 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 > > Sarah, how did you manage to send an email with the "From:" line set to > Gerd's name and address? I sent it using git format-patch and mutt, and accidentally left Gerd's >From line intact. Looking at the headers, it seems like the default Intel exchange servers simply passed the email through. Header forging, what fun! > > 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. > > There may be a simpler approach to this problem. > > When using a new ring segment, keep the first TRB entry in reserve. > Don't put a normal TRB in there, instead leave it as a no-op entry > containing a pointer to the stream ring. (Make the prior Link TRB > point to the second entry in the new segment instead of the first.) > > Then you won't have to add to or remove anything from the radix tree. I don't understand how this would help. Are you advocating a different way of mapping TRB DMA addresses to stream rings that would allow us to ditch the radix tree all together? Ok, so with your solution, we have a virtual stream ring pointer as the first TRB of the segment. We get an event with the DMA address of a TRB in one of many stream rings on an endpoint. From that, I think we can infer the DMA address of the first TRB on the segment, due to the alignment requirements and ring size. And then what do we do with that? We don't have the virtual address of that first TRB, so the xHCI driver can't read the ring pointer from it. I'm confused as to what the next steps would be to solve this. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xhci: fix usb3 streams
On Mon, 14 Oct 2013, Gerd Hoffmann wrote: > 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 Sarah, how did you manage to send an email with the "From:" line set to Gerd's name and address? > 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. There may be a simpler approach to this problem. When using a new ring segment, keep the first TRB entry in reserve. Don't put a normal TRB in there, instead leave it as a no-op entry containing a pointer to the stream ring. (Make the prior Link TRB point to the second entry in the new segment instead of the first.) Then you won't have to add to or remove anything from the radix tree. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xhci: fix usb3 streams
Hi Sarah,
On 10/15/2013 03:54 AM, Gerd Hoffmann wrote:
Gerd, Hans, any objections to this updated patch? The warning is fixed
with it.
No objections, looks good to me.
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
Regards,
Hans
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 !
[PATCH v2] xhci: fix usb3 streams
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);
+
+ return 0;
+}
+
+static void xhci_remove_stream_mapping(struct xhci_ring *ring)
+{
+
