Hi Jan,
I'm testing out some changes to the xHCI USB host controller driver
(which uses a radix tree when a UAS device is attached to the host), and
I noticed the following warning:
Oct 11 14:42:08 xanatos kernel: [18165.819014] usb 2-2: new SuperSpeed USB device number 2 using xhci_hcd
Oct 11 14:42:08 xanatos kernel: [18165.836264] usb 2-2: New USB device found, idVendor=174c, idProduct=55aa
Oct 11 14:42:08 xanatos kernel: [18165.836271] usb 2-2: New USB device strings: Mfr=2, Product=3, SerialNumber=1
Oct 11 14:42:08 xanatos kernel: [18165.836275] usb 2-2: Product: Plugable USB3-SATA-UASP1
Oct 11 14:42:08 xanatos kernel: [18165.836279] usb 2-2: Manufacturer: ASM1053E
Oct 11 14:42:08 xanatos kernel: [18165.836291] usb 2-2: SerialNumber: 123456789045
Oct 11 14:42:08 xanatos kernel: [18165.847661] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/8759
Oct 11 14:42:08 xanatos kernel: [18165.847667] caller is radix_tree_node_alloc+0x5c/0xa0
Oct 11 14:42:08 xanatos kernel: [18165.847670] CPU: 1 PID: 8759 Comm: modprobe Not tainted 3.12.0-rc4+ #94
Oct 11 14:42:08 xanatos kernel: [18165.847671] Hardware name: LENOVO 2325AP7/2325AP7, BIOS G2ET82WW (2.02 ) 09/11/2012
Oct 11 14:42:08 xanatos kernel: [18165.847673] ffff88010c1d3fd8 ffff88010c1d38e8 ffffffff816081b9 0000000000000007
Oct 11 14:42:08 xanatos kernel: [18165.847677] 0000000000000001 ffff88010c1d3918 ffffffff812e3ffc ffff88010c26c988
Oct 11 14:42:08 xanatos kernel: [18165.847679] 0000000000000020 00000000000240b1 0000000000000003 ffff88010c1d3938
Oct 11 14:42:08 xanatos kernel: [18165.847682] Call Trace:
Oct 11 14:42:08 xanatos kernel: [18165.847686] [<ffffffff816081b9>] dump_stack+0x4f/0x84
Oct 11 14:42:08 xanatos kernel: [18165.847689] [<ffffffff812e3ffc>] debug_smp_processor_id+0xdc/0x100
Oct 11 14:42:08 xanatos kernel: [18165.847692] [<ffffffff812d47fc>] radix_tree_node_alloc+0x5c/0xa0
Oct 11 14:42:08 xanatos kernel: [18165.847695] [<ffffffff812d48d5>] radix_tree_insert+0x95/0x260
Oct 11 14:42:08 xanatos kernel: [18165.847702] [<ffffffffa008446f>] xhci_update_stream_ring+0x8f/0xc0 [xhci_hcd]
Oct 11 14:42:08 xanatos kernel: [18165.847708] [<ffffffffa00860f0>] xhci_alloc_stream_info+0x190/0x410 [xhci_hcd]
Oct 11 14:42:08 xanatos kernel: [18165.847713] [<ffffffffa00802ef>] xhci_alloc_streams+0x36f/0x750 [xhci_hcd]
Oct 11 14:42:08 xanatos kernel: [18165.847716] [<ffffffff8108678f>] ? ttwu_stat+0xef/0x160
Oct 11 14:42:08 xanatos kernel: [18165.847720] [<ffffffff810c8e93>] ? is_module_address+0x33/0x60
Oct 11 14:42:08 xanatos kernel: [18165.847729] [<ffffffffa0036e35>] usb_alloc_streams+0x95/0xb0 [usbcore]
Oct 11 14:42:08 xanatos kernel: [18165.847732] [<ffffffffa01eb874>] uas_configure_endpoints+0x154/0x210 [uas]
Oct 11 14:42:08 xanatos kernel: [18165.847735] [<ffffffffa01ec367>] uas_probe+0x2d7/0x390 [uas]
Oct 11 14:42:08 xanatos kernel: [18165.847743] [<ffffffffa0040053>] usb_probe_interface+0x1c3/0x2f0 [usbcore]
Oct 11 14:42:08 xanatos kernel: [18165.847746] [<ffffffff813ba421>] driver_probe_device+0x91/0x3c0
Oct 11 14:42:08 xanatos kernel: [18165.847749] [<ffffffff813ba7fb>] __driver_attach+0xab/0xb0
Oct 11 14:42:08 xanatos kernel: [18165.847751] [<ffffffff813ba750>] ? driver_probe_device+0x3c0/0x3c0
Oct 11 14:42:08 xanatos kernel: [18165.847753] [<ffffffff813b835e>] bus_for_each_dev+0x5e/0x90
Oct 11 14:42:08 xanatos kernel: [18165.847756] [<ffffffff813b9e6e>] driver_attach+0x1e/0x20
Oct 11 14:42:08 xanatos kernel: [18165.847758] [<ffffffff813b988f>] bus_add_driver+0x10f/0x2d0
Oct 11 14:42:08 xanatos kernel: [18165.847760] [<ffffffff813baf24>] driver_register+0x64/0xf0
Oct 11 14:42:08 xanatos kernel: [18165.847767] [<ffffffffa003ec34>] usb_register_driver+0xc4/0x180 [usbcore]
Oct 11 14:42:08 xanatos kernel: [18165.847770] [<ffffffffa01f0000>] ? 0xffffffffa01effff
Oct 11 14:42:08 xanatos kernel: [18165.847773] [<ffffffffa01f001e>] uas_driver_init+0x1e/0x20 [uas]
Oct 11 14:42:08 xanatos kernel: [18165.847775] [<ffffffff8100210a>] do_one_initcall+0xda/0x180
Oct 11 14:42:08 xanatos kernel: [18165.847778] [<ffffffff81080313>] ? __blocking_notifier_call_chain+0x63/0x80
Oct 11 14:42:08 xanatos kernel: [18165.847782] [<ffffffff810c7e42>] load_module+0x14b2/0x1ad0
Oct 11 14:42:08 xanatos kernel: [18165.847784] [<ffffffff810c4680>] ? show_initstate+0x50/0x50
Oct 11 14:42:08 xanatos kernel: [18165.847788] [<ffffffff810c8532>] SyS_init_module+0xd2/0x120
Oct 11 14:42:08 xanatos kernel: [18165.847791] [<ffffffff816183a9>] system_call_fastpath+0x16/0x1b
This looks possibly related to commit
5e4c0d974139a98741b829b27cf38dc8f9284490 "lib/radix-tree.c: make
radix_tree_node_alloc() work correctly within interrupt". I'll revert
the commit, and see if the warning disappears. In the meantime, can you
look into fixing this?
Thanks,
Sarah Sharp
Hello Sarah,
On Fri 11-10-13 15:13:15, Sarah Sharp wrote:
> I'm testing out some changes to the xHCI USB host controller driver
> (which uses a radix tree when a UAS device is attached to the host), and
> I noticed the following warning:
>
> Oct 11 14:42:08 xanatos kernel: [18165.819014] usb 2-2: new SuperSpeed USB device number 2 using xhci_hcd
> Oct 11 14:42:08 xanatos kernel: [18165.836264] usb 2-2: New USB device found, idVendor=174c, idProduct=55aa
> Oct 11 14:42:08 xanatos kernel: [18165.836271] usb 2-2: New USB device strings: Mfr=2, Product=3, SerialNumber=1
> Oct 11 14:42:08 xanatos kernel: [18165.836275] usb 2-2: Product: Plugable USB3-SATA-UASP1
> Oct 11 14:42:08 xanatos kernel: [18165.836279] usb 2-2: Manufacturer: ASM1053E
> Oct 11 14:42:08 xanatos kernel: [18165.836291] usb 2-2: SerialNumber: 123456789045
> Oct 11 14:42:08 xanatos kernel: [18165.847661] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/8759
> Oct 11 14:42:08 xanatos kernel: [18165.847667] caller is radix_tree_node_alloc+0x5c/0xa0
Well, this warning seems to come from the __get_cpu_var() function. I don't
see how my commit could have caused what you observe. It seems you are
calling radix_tree_insert() for a radix tree which has atomic gfp mask set
and you don't do radix_tree_preload() / radix_tree_preload_end() around
that? That was always problematic and could lead to the above warning.
I'm not sure in which contexts xhci_update_stream_ring() can be called. But
if you are guaranteed non-atomic context, then using radix_tree_preload()
with a more relaxed gfp mask is good (lowers pressure on atomic allocations).
If the context depends on the caller, things are more complex. Generally,
you can use radix_tree_maybe_preload() but you have to set gfp mask
argument according to the context. The function then figures out whether
it's worth it to do a preload or not (but it always does preempt_disable()
which will silence the warning).
Honza
> Oct 11 14:42:08 xanatos kernel: [18165.847670] CPU: 1 PID: 8759 Comm: modprobe Not tainted 3.12.0-rc4+ #94
> Oct 11 14:42:08 xanatos kernel: [18165.847671] Hardware name: LENOVO 2325AP7/2325AP7, BIOS G2ET82WW (2.02 ) 09/11/2012
> Oct 11 14:42:08 xanatos kernel: [18165.847673] ffff88010c1d3fd8 ffff88010c1d38e8 ffffffff816081b9 0000000000000007
> Oct 11 14:42:08 xanatos kernel: [18165.847677] 0000000000000001 ffff88010c1d3918 ffffffff812e3ffc ffff88010c26c988
> Oct 11 14:42:08 xanatos kernel: [18165.847679] 0000000000000020 00000000000240b1 0000000000000003 ffff88010c1d3938
> Oct 11 14:42:08 xanatos kernel: [18165.847682] Call Trace:
> Oct 11 14:42:08 xanatos kernel: [18165.847686] [<ffffffff816081b9>] dump_stack+0x4f/0x84
> Oct 11 14:42:08 xanatos kernel: [18165.847689] [<ffffffff812e3ffc>] debug_smp_processor_id+0xdc/0x100
> Oct 11 14:42:08 xanatos kernel: [18165.847692] [<ffffffff812d47fc>] radix_tree_node_alloc+0x5c/0xa0
> Oct 11 14:42:08 xanatos kernel: [18165.847695] [<ffffffff812d48d5>] radix_tree_insert+0x95/0x260
> Oct 11 14:42:08 xanatos kernel: [18165.847702] [<ffffffffa008446f>] xhci_update_stream_ring+0x8f/0xc0 [xhci_hcd]
> Oct 11 14:42:08 xanatos kernel: [18165.847708] [<ffffffffa00860f0>] xhci_alloc_stream_info+0x190/0x410 [xhci_hcd]
> Oct 11 14:42:08 xanatos kernel: [18165.847713] [<ffffffffa00802ef>] xhci_alloc_streams+0x36f/0x750 [xhci_hcd]
> Oct 11 14:42:08 xanatos kernel: [18165.847716] [<ffffffff8108678f>] ? ttwu_stat+0xef/0x160
> Oct 11 14:42:08 xanatos kernel: [18165.847720] [<ffffffff810c8e93>] ? is_module_address+0x33/0x60
> Oct 11 14:42:08 xanatos kernel: [18165.847729] [<ffffffffa0036e35>] usb_alloc_streams+0x95/0xb0 [usbcore]
> Oct 11 14:42:08 xanatos kernel: [18165.847732] [<ffffffffa01eb874>] uas_configure_endpoints+0x154/0x210 [uas]
> Oct 11 14:42:08 xanatos kernel: [18165.847735] [<ffffffffa01ec367>] uas_probe+0x2d7/0x390 [uas]
> Oct 11 14:42:08 xanatos kernel: [18165.847743] [<ffffffffa0040053>] usb_probe_interface+0x1c3/0x2f0 [usbcore]
> Oct 11 14:42:08 xanatos kernel: [18165.847746] [<ffffffff813ba421>] driver_probe_device+0x91/0x3c0
> Oct 11 14:42:08 xanatos kernel: [18165.847749] [<ffffffff813ba7fb>] __driver_attach+0xab/0xb0
> Oct 11 14:42:08 xanatos kernel: [18165.847751] [<ffffffff813ba750>] ? driver_probe_device+0x3c0/0x3c0
> Oct 11 14:42:08 xanatos kernel: [18165.847753] [<ffffffff813b835e>] bus_for_each_dev+0x5e/0x90
> Oct 11 14:42:08 xanatos kernel: [18165.847756] [<ffffffff813b9e6e>] driver_attach+0x1e/0x20
> Oct 11 14:42:08 xanatos kernel: [18165.847758] [<ffffffff813b988f>] bus_add_driver+0x10f/0x2d0
> Oct 11 14:42:08 xanatos kernel: [18165.847760] [<ffffffff813baf24>] driver_register+0x64/0xf0
> Oct 11 14:42:08 xanatos kernel: [18165.847767] [<ffffffffa003ec34>] usb_register_driver+0xc4/0x180 [usbcore]
> Oct 11 14:42:08 xanatos kernel: [18165.847770] [<ffffffffa01f0000>] ? 0xffffffffa01effff
> Oct 11 14:42:08 xanatos kernel: [18165.847773] [<ffffffffa01f001e>] uas_driver_init+0x1e/0x20 [uas]
> Oct 11 14:42:08 xanatos kernel: [18165.847775] [<ffffffff8100210a>] do_one_initcall+0xda/0x180
> Oct 11 14:42:08 xanatos kernel: [18165.847778] [<ffffffff81080313>] ? __blocking_notifier_call_chain+0x63/0x80
> Oct 11 14:42:08 xanatos kernel: [18165.847782] [<ffffffff810c7e42>] load_module+0x14b2/0x1ad0
> Oct 11 14:42:08 xanatos kernel: [18165.847784] [<ffffffff810c4680>] ? show_initstate+0x50/0x50
> Oct 11 14:42:08 xanatos kernel: [18165.847788] [<ffffffff810c8532>] SyS_init_module+0xd2/0x120
> Oct 11 14:42:08 xanatos kernel: [18165.847791] [<ffffffff816183a9>] system_call_fastpath+0x16/0x1b
>
> This looks possibly related to commit
> 5e4c0d974139a98741b829b27cf38dc8f9284490 "lib/radix-tree.c: make
> radix_tree_node_alloc() work correctly within interrupt". I'll revert
> the commit, and see if the warning disappears. In the meantime, can you
> look into fixing this?
>
> Thanks,
> Sarah Sharp
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Oct 14, 2013 at 02:30:34PM +0200, Jan Kara wrote:
> Hello Sarah,
>
> On Fri 11-10-13 15:13:15, Sarah Sharp wrote:
> > I'm testing out some changes to the xHCI USB host controller driver
> > (which uses a radix tree when a UAS device is attached to the host), and
> > I noticed the following warning:
> >
> > Oct 11 14:42:08 xanatos kernel: [18165.819014] usb 2-2: new SuperSpeed USB device number 2 using xhci_hcd
> > Oct 11 14:42:08 xanatos kernel: [18165.836264] usb 2-2: New USB device found, idVendor=174c, idProduct=55aa
> > Oct 11 14:42:08 xanatos kernel: [18165.836271] usb 2-2: New USB device strings: Mfr=2, Product=3, SerialNumber=1
> > Oct 11 14:42:08 xanatos kernel: [18165.836275] usb 2-2: Product: Plugable USB3-SATA-UASP1
> > Oct 11 14:42:08 xanatos kernel: [18165.836279] usb 2-2: Manufacturer: ASM1053E
> > Oct 11 14:42:08 xanatos kernel: [18165.836291] usb 2-2: SerialNumber: 123456789045
> > Oct 11 14:42:08 xanatos kernel: [18165.847661] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/8759
> > Oct 11 14:42:08 xanatos kernel: [18165.847667] caller is radix_tree_node_alloc+0x5c/0xa0
> Well, this warning seems to come from the __get_cpu_var() function. I don't
> see how my commit could have caused what you observe. It seems you are
> calling radix_tree_insert() for a radix tree which has atomic gfp mask set
> and you don't do radix_tree_preload() / radix_tree_preload_end() around
> that? That was always problematic and could lead to the above warning.
I see. Ok, we'll need to fix that. The code went into the kernel years
ago, but wasn't really tested until now.
Do we only need to call radix_tree_preload() and
radix_tree_preload_end() only around the radix_tree_insert()? Or will
we need it around radix_tree_delete() as well?
> I'm not sure in which contexts xhci_update_stream_ring() can be called. But
> if you are guaranteed non-atomic context, then using radix_tree_preload()
> with a more relaxed gfp mask is good (lowers pressure on atomic allocations).
>
> If the context depends on the caller, things are more complex. Generally,
> you can use radix_tree_maybe_preload() but you have to set gfp mask
> argument according to the context. The function then figures out whether
> it's worth it to do a preload or not (but it always does preempt_disable()
> which will silence the warning).
There are a couple ways xhci_update_stream_ring() could be called:
- xhci_alloc_stream_info can be called while the bandwidth mutex is
held, so that must be in process context
- xhci_ring_free is called when the command to disable a slot
completes, in interrupt context.
- xhci_ring_expansion during URB submission, which can happen in
interrupt context.
So it looks like the context depends on the caller, and we'll have to
call radix_tree_maybe_preload().
Sarah Sharp
On Mon 14-10-13 11:17:21, Sarah Sharp wrote:
> On Mon, Oct 14, 2013 at 02:30:34PM +0200, Jan Kara wrote:
> > Hello Sarah,
> >
> > On Fri 11-10-13 15:13:15, Sarah Sharp wrote:
> > > I'm testing out some changes to the xHCI USB host controller driver
> > > (which uses a radix tree when a UAS device is attached to the host), and
> > > I noticed the following warning:
> > >
> > > Oct 11 14:42:08 xanatos kernel: [18165.819014] usb 2-2: new SuperSpeed USB device number 2 using xhci_hcd
> > > Oct 11 14:42:08 xanatos kernel: [18165.836264] usb 2-2: New USB device found, idVendor=174c, idProduct=55aa
> > > Oct 11 14:42:08 xanatos kernel: [18165.836271] usb 2-2: New USB device strings: Mfr=2, Product=3, SerialNumber=1
> > > Oct 11 14:42:08 xanatos kernel: [18165.836275] usb 2-2: Product: Plugable USB3-SATA-UASP1
> > > Oct 11 14:42:08 xanatos kernel: [18165.836279] usb 2-2: Manufacturer: ASM1053E
> > > Oct 11 14:42:08 xanatos kernel: [18165.836291] usb 2-2: SerialNumber: 123456789045
> > > Oct 11 14:42:08 xanatos kernel: [18165.847661] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/8759
> > > Oct 11 14:42:08 xanatos kernel: [18165.847667] caller is radix_tree_node_alloc+0x5c/0xa0
> > Well, this warning seems to come from the __get_cpu_var() function. I don't
> > see how my commit could have caused what you observe. It seems you are
> > calling radix_tree_insert() for a radix tree which has atomic gfp mask set
> > and you don't do radix_tree_preload() / radix_tree_preload_end() around
> > that? That was always problematic and could lead to the above warning.
>
> I see. Ok, we'll need to fix that. The code went into the kernel years
> ago, but wasn't really tested until now.
>
> Do we only need to call radix_tree_preload() and
> radix_tree_preload_end() only around the radix_tree_insert()? Or will
> we need it around radix_tree_delete() as well?
Only around insert. The reason is radix_tree_insert() may need to
allocate new radix tree nodes. radix_tree_delete() doesn't need to allocate
anything.
> > I'm not sure in which contexts xhci_update_stream_ring() can be called. But
> > if you are guaranteed non-atomic context, then using radix_tree_preload()
> > with a more relaxed gfp mask is good (lowers pressure on atomic allocations).
> >
> > If the context depends on the caller, things are more complex. Generally,
> > you can use radix_tree_maybe_preload() but you have to set gfp mask
> > argument according to the context. The function then figures out whether
> > it's worth it to do a preload or not (but it always does preempt_disable()
> > which will silence the warning).
>
> There are a couple ways xhci_update_stream_ring() could be called:
>
> - xhci_alloc_stream_info can be called while the bandwidth mutex is
> held, so that must be in process context
>
> - xhci_ring_free is called when the command to disable a slot
> completes, in interrupt context.
>
> - xhci_ring_expansion during URB submission, which can happen in
> interrupt context.
>
> So it looks like the context depends on the caller, and we'll have to
> call radix_tree_maybe_preload().
Yes, you need radix_tree_maybe_preload() in that case. Usually we handle
similar situations (e.g. in block/blk-ioc.c) by passing gfp mask from the
caller which knows the context.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
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 <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
Signed-off-by: Sarah Sharp <[email protected]>
---
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)
+{
+ struct xhci_segment *seg;
+ unsigned long key;
+
+ if (WARN_ON_ONCE(ring->trb_address_map == NULL))
+ return;
+
+ seg = ring->first_seg;
+ do {
+ key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+ if (radix_tree_lookup(ring->trb_address_map, key))
+ radix_tree_delete(ring->trb_address_map, key);
+ seg = seg->next;
+ } while (seg != ring->first_seg);
+}
+
/* XXX: Do we need the hcd structure in all these functions? */
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_remove_stream_mapping(ring);
xhci_free_segments_for_ring(xhci, ring->first_seg);
+ }
kfree(ring);
}
@@ -353,6 +434,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
"ring expansion succeed, now has %d segments",
ring->num_segs);
+ if (ring->type == TYPE_STREAM) {
+ ret = xhci_update_stream_mapping(ring, flags);
+ WARN_ON(ret); /* FIXME */
+ }
+
return 0;
}
@@ -509,36 +595,6 @@ struct xhci_ring *xhci_stream_id_to_ring(
* The number of stream contexts in the stream context array may be bigger than
* the number of streams the driver wants to use. This is because the number of
* stream context array entries must be a power of two.
- *
- * 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.
*/
struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
@@ -547,7 +603,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;
@@ -602,6 +657,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) |
@@ -611,10 +667,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_mapping(cur_ring, mem_flags);
if (ret) {
xhci_ring_free(xhci, cur_ring);
stream_info->stream_rings[cur_stream] = NULL;
@@ -634,9 +687,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;
}
@@ -697,7 +747,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;
@@ -706,9 +755,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 289fbfb..737dcc1 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 {
--
1.8.3.3
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 <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> Signed-off-by: Sarah Sharp <[email protected]>
> ---
> 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)
> +{
> + struct xhci_segment *seg;
> + unsigned long key;
> +
> + if (WARN_ON_ONCE(ring->trb_address_map == NULL))
> + return;
> +
> + seg = ring->first_seg;
> + do {
> + key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
> + if (radix_tree_lookup(ring->trb_address_map, key))
> + radix_tree_delete(ring->trb_address_map, key);
> + seg = seg->next;
> + } while (seg != ring->first_seg);
> +}
> +
> /* XXX: Do we need the hcd structure in all these functions? */
> 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_remove_stream_mapping(ring);
> xhci_free_segments_for_ring(xhci, ring->first_seg);
> + }
>
> kfree(ring);
> }
> @@ -353,6 +434,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
> "ring expansion succeed, now has %d segments",
> ring->num_segs);
>
> + if (ring->type == TYPE_STREAM) {
> + ret = xhci_update_stream_mapping(ring, flags);
> + WARN_ON(ret); /* FIXME */
> + }
> +
> return 0;
> }
>
> @@ -509,36 +595,6 @@ struct xhci_ring *xhci_stream_id_to_ring(
> * The number of stream contexts in the stream context array may be bigger than
> * the number of streams the driver wants to use. This is because the number of
> * stream context array entries must be a power of two.
> - *
> - * 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.
> */
> struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
> unsigned int num_stream_ctxs,
> @@ -547,7 +603,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;
>
> @@ -602,6 +657,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) |
> @@ -611,10 +667,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_mapping(cur_ring, mem_flags);
> if (ret) {
> xhci_ring_free(xhci, cur_ring);
> stream_info->stream_rings[cur_stream] = NULL;
> @@ -634,9 +687,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;
> }
> @@ -697,7 +747,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;
> @@ -706,9 +755,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 289fbfb..737dcc1 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 {
>
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
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
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
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 <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> Signed-off-by: Sarah Sharp <[email protected]>
> ---
> 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)
> +{
> + struct xhci_segment *seg;
> + unsigned long key;
> +
> + if (WARN_ON_ONCE(ring->trb_address_map == NULL))
> + return;
> +
> + seg = ring->first_seg;
> + do {
> + key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
> + if (radix_tree_lookup(ring->trb_address_map, key))
> + radix_tree_delete(ring->trb_address_map, key);
> + seg = seg->next;
> + } while (seg != ring->first_seg);
> +}
> +
> /* XXX: Do we need the hcd structure in all these functions? */
> 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_remove_stream_mapping(ring);
> xhci_free_segments_for_ring(xhci, ring->first_seg);
> + }
>
> kfree(ring);
> }
> @@ -353,6 +434,11 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
> "ring expansion succeed, now has %d segments",
> ring->num_segs);
>
> + if (ring->type == TYPE_STREAM) {
> + ret = xhci_update_stream_mapping(ring, flags);
> + WARN_ON(ret); /* FIXME */
> + }
> +
> return 0;
> }
>
> @@ -509,36 +595,6 @@ struct xhci_ring *xhci_stream_id_to_ring(
> * The number of stream contexts in the stream context array may be bigger than
> * the number of streams the driver wants to use. This is because the number of
> * stream context array entries must be a power of two.
> - *
> - * 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.
> */
> struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
> unsigned int num_stream_ctxs,
> @@ -547,7 +603,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;
>
> @@ -602,6 +657,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) |
> @@ -611,10 +667,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_mapping(cur_ring, mem_flags);
> if (ret) {
> xhci_ring_free(xhci, cur_ring);
> stream_info->stream_rings[cur_stream] = NULL;
> @@ -634,9 +687,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;
> }
> @@ -697,7 +747,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;
> @@ -706,9 +755,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 289fbfb..737dcc1 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 {
> --
> 1.8.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
If we're expanding a stream ring, we want to make sure we can add those
ring segments to the radix tree that maps segments to ring pointers.
Try the radix tree insert after the new ring segments have been allocated
(the last segment in the new ring chunk will point to the first newly
allocated segment), but before the new ring segments are linked into the
old ring.
If insert fails on any one segment, remove each segment from the radix
tree, deallocate the new segments, and return. Otherwise, link the new
segments into the tree.
Signed-off-by: Sarah Sharp <[email protected]>
---
Something like this. It's ugly, but it compiles. I haven't tested it.
Hans, can you review and test this?
Sarah Sharp
drivers/usb/host/xhci-mem.c | 106 +++++++++++++++++++++++++++++++++-----------
1 file changed, 80 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index a455c56..6ce8d31 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -180,53 +180,98 @@ static void xhci_link_rings(struct xhci_hcd *xhci, struct xhci_ring *ring,
* 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)
+static int xhci_insert_segment_mapping(struct radix_tree_root *trb_address_map,
+ struct xhci_ring *ring,
+ struct xhci_segment *seg,
+ gfp_t mem_flags)
{
- struct xhci_segment *seg;
unsigned long key;
int ret;
- if (WARN_ON_ONCE(ring->trb_address_map == NULL))
+ key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+ /* Skip any segments that were already added. */
+ if (radix_tree_lookup(trb_address_map, key))
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(trb_address_map,
+ key, ring);
+ radix_tree_preload_end();
+ return ret;
+}
- 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();
+static void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
+ struct xhci_segment *seg)
+{
+ unsigned long key;
+
+ key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
+ if (radix_tree_lookup(trb_address_map, key))
+ radix_tree_delete(trb_address_map, key);
+}
+
+static int xhci_update_stream_segment_mapping(
+ struct radix_tree_root *trb_address_map,
+ struct xhci_ring *ring,
+ struct xhci_segment *first_seg,
+ struct xhci_segment *last_seg,
+ gfp_t mem_flags)
+{
+ struct xhci_segment *seg;
+ struct xhci_segment *failed_seg;
+ int ret;
+
+ if (WARN_ON_ONCE(trb_address_map == NULL))
+ return 0;
+
+ seg = first_seg;
+ do {
+ ret = xhci_insert_segment_mapping(trb_address_map,
+ ring, seg, mem_flags);
if (ret)
- return ret;
+ goto remove_streams;
+ if (seg == last_seg)
+ return 0;
seg = seg->next;
- } while (seg != ring->first_seg);
+ } while (seg != first_seg);
return 0;
+
+remove_streams:
+ failed_seg = seg;
+ seg = first_seg;
+ do {
+ xhci_remove_segment_mapping(trb_address_map, seg);
+ if (seg == failed_seg)
+ return ret;
+ seg = seg->next;
+ } while (seg != first_seg);
+
+ return ret;
}
static void xhci_remove_stream_mapping(struct xhci_ring *ring)
{
struct xhci_segment *seg;
- unsigned long key;
if (WARN_ON_ONCE(ring->trb_address_map == NULL))
return;
seg = ring->first_seg;
do {
- key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
- if (radix_tree_lookup(ring->trb_address_map, key))
- radix_tree_delete(ring->trb_address_map, key);
+ xhci_remove_segment_mapping(ring->trb_address_map, seg);
seg = seg->next;
} while (seg != ring->first_seg);
}
+static int xhci_update_stream_mapping(struct xhci_ring *ring, gfp_t mem_flags)
+{
+ return xhci_update_stream_segment_mapping(ring->trb_address_map, ring,
+ ring->first_seg, ring->last_seg, mem_flags);
+}
+
/* XXX: Do we need the hcd structure in all these functions? */
void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring)
{
@@ -429,16 +474,25 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
if (ret)
return -ENOMEM;
+ ret = xhci_update_stream_segment_mapping(ring->trb_address_map, ring,
+ first, last, flags);
+ if (ret) {
+ struct xhci_segment *next;
+ do {
+ next = first->next;
+ xhci_segment_free(xhci, first);
+ if (first == last)
+ break;
+ first = next;
+ } while (true);
+ return ret;
+ }
+
xhci_link_rings(xhci, ring, first, last, num_segs);
xhci_dbg_trace(xhci, trace_xhci_dbg_ring_expansion,
"ring expansion succeed, now has %d segments",
ring->num_segs);
- if (ring->type == TYPE_STREAM) {
- ret = xhci_update_stream_mapping(ring, flags);
- WARN_ON(ret); /* FIXME */
- }
-
return 0;
}
--
1.8.3.3
Hi,
On 10/17/2013 09:44 PM, Sarah Sharp wrote:
> If we're expanding a stream ring, we want to make sure we can add those
> ring segments to the radix tree that maps segments to ring pointers.
> Try the radix tree insert after the new ring segments have been allocated
> (the last segment in the new ring chunk will point to the first newly
> allocated segment), but before the new ring segments are linked into the
> old ring.
>
> If insert fails on any one segment, remove each segment from the radix
> tree, deallocate the new segments, and return. Otherwise, link the new
> segments into the tree.
>
> Signed-off-by: Sarah Sharp <[email protected]>
> ---
>
> Something like this.
Thanks for working on this.
> It's ugly, but it compiles. I haven't tested it.
> Hans, can you review and test this?
Reviewed, I've one small nitpick, see inline comments, other then that it looks
good, and I don't find it all that ugly :)
I've also run various tests and it seems to work as advertised (I've not
managed to trigger the error path though AFAIK).
Acked-by: Hans de Goede <[email protected]>
>
> Sarah Sharp
>
> drivers/usb/host/xhci-mem.c | 106 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 80 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index a455c56..6ce8d31 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -180,53 +180,98 @@ static void xhci_link_rings(struct xhci_hcd *xhci, struct xhci_ring *ring,
> * 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)
> +static int xhci_insert_segment_mapping(struct radix_tree_root *trb_address_map,
> + struct xhci_ring *ring,
> + struct xhci_segment *seg,
> + gfp_t mem_flags)
> {
> - struct xhci_segment *seg;
> unsigned long key;
> int ret;
>
> - if (WARN_ON_ONCE(ring->trb_address_map == NULL))
> + key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
> + /* Skip any segments that were already added. */
> + if (radix_tree_lookup(trb_address_map, key))
> 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(trb_address_map,
> + key, ring);
> + radix_tree_preload_end();
> + return ret;
> +}
>
> - 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();
> +static void xhci_remove_segment_mapping(struct radix_tree_root *trb_address_map,
> + struct xhci_segment *seg)
> +{
> + unsigned long key;
> +
> + key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
> + if (radix_tree_lookup(trb_address_map, key))
> + radix_tree_delete(trb_address_map, key);
> +}
> +
> +static int xhci_update_stream_segment_mapping(
> + struct radix_tree_root *trb_address_map,
> + struct xhci_ring *ring,
> + struct xhci_segment *first_seg,
> + struct xhci_segment *last_seg,
> + gfp_t mem_flags)
> +{
> + struct xhci_segment *seg;
> + struct xhci_segment *failed_seg;
> + int ret;
> +
> + if (WARN_ON_ONCE(trb_address_map == NULL))
> + return 0;
> +
> + seg = first_seg;
> + do {
> + ret = xhci_insert_segment_mapping(trb_address_map,
> + ring, seg, mem_flags);
> if (ret)
> - return ret;
> + goto remove_streams;
> + if (seg == last_seg)
> + return 0;
> seg = seg->next;
> - } while (seg != ring->first_seg);
> + } while (seg != first_seg);
The while here tests for looping round, but that should never
happen, IMHO using do {} while (true); here would be more clear,
and also consistent with how the tear-down is done in the
error case in xhci_ring_expansion.
>
> return 0;
> +
> +remove_streams:
> + failed_seg = seg;
> + seg = first_seg;
> + do {
> + xhci_remove_segment_mapping(trb_address_map, seg);
> + if (seg == failed_seg)
> + return ret;
> + seg = seg->next;
> + } while (seg != first_seg);
And I would also prefer "} while (true};" here for the same reasons.
> +
> + return ret;
> }
>
> static void xhci_remove_stream_mapping(struct xhci_ring *ring)
> {
> struct xhci_segment *seg;
> - unsigned long key;
>
> if (WARN_ON_ONCE(ring->trb_address_map == NULL))
> return;
>
> seg = ring->first_seg;
> do {
> - key = (unsigned long)(seg->dma >> TRB_SEGMENT_SHIFT);
> - if (radix_tree_lookup(ring->trb_address_map, key))
> - radix_tree_delete(ring->trb_address_map, key);
> + xhci_remove_segment_mapping(ring->trb_address_map, seg);
> seg = seg->next;
> } while (seg != ring->first_seg);
> }
>
> +static int xhci_update_stream_mapping(struct xhci_ring *ring, gfp_t mem_flags)
> +{
> + return xhci_update_stream_segment_mapping(ring->trb_address_map, ring,
> + ring->first_seg, ring->last_seg, mem_flags);
> +}
> +
> /* XXX: Do we need the hcd structure in all these functions? */
> void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring)
> {
> @@ -429,16 +474,25 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
> if (ret)
> return -ENOMEM;
>
> + ret = xhci_update_stream_segment_mapping(ring->trb_address_map, ring,
> + first, last, flags);
> + if (ret) {
> + struct xhci_segment *next;
> + do {
> + next = first->next;
> + xhci_segment_free(xhci, first);
> + if (first == last)
> + break;
> + first = next;
> + } while (true);
> + return ret;
> + }
> +
> xhci_link_rings(xhci, ring, first, last, num_segs);
> xhci_dbg_trace(xhci, trace_xhci_dbg_ring_expansion,
> "ring expansion succeed, now has %d segments",
> ring->num_segs);
>
> - if (ring->type == TYPE_STREAM) {
> - ret = xhci_update_stream_mapping(ring, flags);
> - WARN_ON(ret); /* FIXME */
> - }
> -
> return 0;
> }
>
>
Regards,
Hans