Re: [PATCH v2] xhci: fix usb3 streams

[Sarah Sharp]

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 <kraxel@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> ---
>  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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/