Re: [PATCH] xhci: Remove segments from radix tree on failed insert.

From: Hans de Goede
Date: Thu Oct 17 2013 - 17:12:36 EST

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 <sarah.a.sharp@xxxxxxxxxxxxxxx>
---

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 <hdegoede@xxxxxxxxxx>


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/