On Tue, Mar 02, 2010 at 06:28:45PM +0800, Libin Yang wrote:
> >From d598ff50c8908bd62c1871ca9bfe68d952e0a949 Mon Sep 17 00:00:00 2001
> From: Andiry Xu <andiry.xu@xxxxxxx>
> Date: Fri, 26 Feb 2010 17:34:47 +0800
> Subject: [PATCH 1/7] xHCI: Introduce urb_priv structure
>
> Introduce urb_priv data structure to allow one URB contains multiple TDs.
> This structure is necessary for isoc transfers, because xHCI can only
> transfer one isoc TD in a interval, and one isoc URB needs to be split
> into multiple isoc TDs.
>
> Some codes are borrowed from OHCI driver.
Hi Libin and Andiry,
Can you add more description to your commit message? There are some
semantics about when urb->hcpriv is NULL that I don't quite understand.
It looks like you're trying to make urb->hcpriv == NULL mean that the
URB has been given back to the USB core. Is that true?
There's a lot of conditional code based on this comparison, so your
assumptions about behavior needs to be clear. I think the extra NULL
pointer checking complicates things quite a bit, and it makes the
cancellation case much harder to think about.
For instance, you only give back the URB in xhci_giveback_urb_in_irq()
if urb->hcpriv != NULL. But how can xhci_giveback_urb_in_irq() be
called if urb->hcpriv is NULL? That would mean there's a TD dangling on
the endpoint's cancellation list when its URB has already been given
back (which I've tried very hard to prevent), or the URB was somehow
given back and its TD was left on the normal endpoint ring TD list.
I would much rather get a null pointer deference oops that reveals that
assumptions about behavior are incorrect than try to debug behavioral
errors that are masked by extra null pointer checking.
Sarah Sharp
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> Signed-off-by: Crane Cai <crane.cai@xxxxxxx>
> Signed-off-by: Libin Yang <libin.yang@xxxxxxx>
> ---
> drivers/usb/host/xhci-hcd.c | 58 ++++++++++++++++++++++++++++++---
> drivers/usb/host/xhci-mem.c | 17 ++++++++++
> drivers/usb/host/xhci-ring.c | 71 +++++++++++++++++++++++++-----------------
> drivers/usb/host/xhci.h | 10 ++++++
> 4 files changed, 121 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hcd.c b/drivers/usb/host/xhci-hcd.c
> index 4cb69e0..36b7fa1 100644
> --- a/drivers/usb/host/xhci-hcd.c
> +++ b/drivers/usb/host/xhci-hcd.c
> @@ -681,7 +681,8 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
> unsigned long flags;
> int ret = 0;
> unsigned int slot_id, ep_index;
> -
> + struct urb_priv *urb_priv;
> + int size, i;
>
> if (!urb || xhci_check_args(hcd, urb->dev, urb->ep, true, __func__) <= 0)
> return -EINVAL;
> @@ -701,6 +702,37 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
> ret = -ESHUTDOWN;
> goto exit;
> }
> +
> + if (usb_endpoint_xfer_isoc(&urb->ep->desc))
> + size = urb->number_of_packets;
> + else
> + size = 1;
> +
> + urb_priv = kzalloc(sizeof(struct urb_priv) +
> + size * sizeof(struct xhci_td *), mem_flags);
Don't you allocate one struct xhci_td pointer too many here? The struct
urb_priv already contains one xhci_td pointer (urb_priv->td[0]), which
is enough for the non-isochronous endpoints. I think the line should
be:
(size - 1) * sizeof(struct xhci_td *), mem_flags);
> + if (!urb_priv)
> + return -ENOMEM;
> +
> + for (i = 0; i < size; i++) {
> + urb_priv->td[i] = kzalloc(sizeof(struct xhci_td), mem_flags);
> + if (!urb_priv->td[i]) {
> + urb_priv->length = i;
> + urb_free_priv(xhci, urb_priv);
> + return -ENOMEM;
> + }
> + }
> +
> + spin_lock_irqsave(&xhci->lock, flags);
> + ret = usb_hcd_link_urb_to_ep(hcd, urb);
> + spin_unlock_irqrestore(&xhci->lock, flags);
I think this breaks the cancellation case. The urb should be linked and
the TDs should be added to the endpoint list as one atomic operation
under xhci->lock. If you move this code into prepare_transfer()
instead, you'll be protected by the lock, and I think your code will
look cleaner. You can only link the URB if the td_index passed to
prepare_transfer() is 0.
> +
> + if (unlikely(ret)) {
> + urb_free_priv(xhci, urb_priv);
> + goto exit;
> + }
> + urb_priv->length = size;
> + urb->hcpriv = urb_priv;
> +
> if (usb_endpoint_xfer_control(&urb->ep->desc)) {
> /* Check to see if the max packet size for the default control
> * endpoint changed during FS device enumeration
> @@ -741,6 +773,7 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
> exit:
> return ret;
> dying:
> + urb_free_priv(xhci, urb_priv);
> xhci_dbg(xhci, "Ep 0x%x: URB %p submitted for "
> "non-responsive xHCI host.\n",
> urb->ep->desc.bEndpointAddress, urb);
> @@ -782,9 +815,10 @@ dying:
> int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> {
> unsigned long flags;
> - int ret;
> + int ret, i;
> u32 temp;
> struct xhci_hcd *xhci;
> + struct urb_priv *urb_priv;
> struct xhci_td *td;
> unsigned int ep_index;
> struct xhci_ring *ep_ring;
> @@ -799,12 +833,15 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> temp = xhci_readl(xhci, &xhci->op_regs->status);
> if (temp == 0xffffffff) {
> xhci_dbg(xhci, "HW died, freeing TD.\n");
> - td = (struct xhci_td *) urb->hcpriv;
> + urb_priv = urb->hcpriv;
>
> usb_hcd_unlink_urb_from_ep(hcd, urb);
> spin_unlock_irqrestore(&xhci->lock, flags);
> usb_hcd_giveback_urb(xhci_to_hcd(xhci), urb, -ESHUTDOWN);
> - kfree(td);
> + if (urb_priv) {
> + urb_free_priv(xhci, urb_priv);
> + urb->hcpriv = NULL;
> + }
> return ret;
> }
> if (xhci->xhc_state & XHCI_STATE_DYING) {
> @@ -827,9 +864,18 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> ep_ring = ep->ring;
> xhci_dbg(xhci, "Endpoint ring:\n");
> xhci_debug_ring(xhci, ep_ring);
> - td = (struct xhci_td *) urb->hcpriv;
>
> - list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
> + urb_priv = urb->hcpriv;
> +
> + if (urb_priv) {
> + for (i = 0; i < urb_priv->length; i++) {
> + td = urb_priv->td[i];
> + if (td)
> + list_add_tail(&td->cancelled_td_list,
> + &ep->cancelled_td_list);
> + }
> + }
> +
> /* Queue a stop endpoint command, but only if this is
> * the first cancellation to be handled.
> */
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 8045bc6..a645c3a 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -881,6 +881,23 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
> return command;
> }
>
> +void urb_free_priv(struct xhci_hcd *xhci, struct urb_priv *urb_priv)
> +{
> + int last = urb_priv->length - 1;
> +
Why not make this function return if the passed in urb_priv is NULL?
That would make your clean up code simpler in all the other functions.
> + if (last >= 0) {
> + int i;
> + struct xhci_td *td;
> +
> + for (i = 0; i <= last; i++) {
> + td = urb_priv->td[i];
> + kfree(td);
Just use kfree(urb_priv->td[i]) and then you can get rid of the curly
braces and the extra variable.
> + }
> + }
> +
> + kfree(urb_priv);
> +}
> +
> void xhci_free_command(struct xhci_hcd *xhci,
> struct xhci_command *command)
> {
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 6ba841b..79041b9 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -492,16 +492,18 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
> struct xhci_td *cur_td, int status, char *adjective)
> {
> struct usb_hcd *hcd = xhci_to_hcd(xhci);
> + struct urb *urb;
>
> - cur_td->urb->hcpriv = NULL;
> - usb_hcd_unlink_urb_from_ep(hcd, cur_td->urb);
> - xhci_dbg(xhci, "Giveback %s URB %p\n", adjective, cur_td->urb);
> + urb = cur_td->urb;
> + if (urb->hcpriv) {
> + usb_hcd_unlink_urb_from_ep(hcd, urb);
> + xhci_dbg(xhci, "Giveback %s URB %p\n", adjective, urb);
>
> - spin_unlock(&xhci->lock);
> - usb_hcd_giveback_urb(hcd, cur_td->urb, status);
> - kfree(cur_td);
> - spin_lock(&xhci->lock);
> - xhci_dbg(xhci, "%s URB given back\n", adjective);
> + spin_unlock(&xhci->lock);
> + usb_hcd_giveback_urb(hcd, urb, status);
> + spin_lock(&xhci->lock);
> + xhci_dbg(xhci, "%s URB given back\n", adjective);
> + }
> }
>
> /*
> @@ -1472,9 +1474,9 @@ td_cleanup:
> if (usb_endpoint_xfer_control(&urb->ep->desc) ||
> (trb_comp_code != COMP_STALL &&
> trb_comp_code != COMP_BABBLE)) {
> - kfree(td);
> + urb_free_priv(xhci, urb->hcpriv);
> + urb->hcpriv = NULL;
> }
> - urb->hcpriv = NULL;
> }
> cleanup:
> inc_deq(xhci, xhci->event_ring, true);
> @@ -1628,34 +1630,32 @@ static int prepare_transfer(struct xhci_hcd *xhci,
> unsigned int ep_index,
> unsigned int num_trbs,
> struct urb *urb,
> - struct xhci_td **td,
> + unsigned int td_index,
> gfp_t mem_flags)
> {
> int ret;
> + struct urb_priv *urb_priv;
> + struct xhci_td *td;
> struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
> ret = prepare_ring(xhci, xdev->eps[ep_index].ring,
> ep_ctx->ep_info & EP_STATE_MASK,
> num_trbs, mem_flags);
> if (ret)
> return ret;
> - *td = kzalloc(sizeof(struct xhci_td), mem_flags);
> - if (!*td)
> - return -ENOMEM;
> - INIT_LIST_HEAD(&(*td)->td_list);
> - INIT_LIST_HEAD(&(*td)->cancelled_td_list);
>
> - ret = usb_hcd_link_urb_to_ep(xhci_to_hcd(xhci), urb);
> - if (unlikely(ret)) {
> - kfree(*td);
> - return ret;
> - }
> + urb_priv = urb->hcpriv;
> + td = urb_priv->td[td_index];
> +
> + INIT_LIST_HEAD(&td->td_list);
> + INIT_LIST_HEAD(&td->cancelled_td_list);
>
> - (*td)->urb = urb;
> - urb->hcpriv = (void *) (*td);
> + td->urb = urb;
> /* Add this TD to the tail of the endpoint ring's TD list */
> - list_add_tail(&(*td)->td_list, &xdev->eps[ep_index].ring->td_list);
> - (*td)->start_seg = xdev->eps[ep_index].ring->enq_seg;
> - (*td)->first_trb = xdev->eps[ep_index].ring->enqueue;
> + list_add_tail(&td->td_list, &xdev->eps[ep_index].ring->td_list);
> + td->start_seg = xdev->eps[ep_index].ring->enq_seg;
> + td->first_trb = xdev->eps[ep_index].ring->enqueue;
> +
> + urb_priv->td[td_index] = td;
>
> return 0;
> }
> @@ -1794,6 +1794,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> {
> struct xhci_ring *ep_ring;
> unsigned int num_trbs;
> + struct urb_priv *urb_priv;
> struct xhci_td *td;
> struct scatterlist *sg;
> int num_sgs;
> @@ -1809,9 +1810,13 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> num_sgs = urb->num_sgs;
>
> trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id],
> - ep_index, num_trbs, urb, &td, mem_flags);
> + ep_index, num_trbs, urb, 0, mem_flags);
> if (trb_buff_len < 0)
> return trb_buff_len;
> +
> + urb_priv = urb->hcpriv;
> + td = urb_priv->td[0];
> +
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> * until we've finished creating all the other TRBs. The ring's cycle
> @@ -1927,6 +1932,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> struct urb *urb, int slot_id, unsigned int ep_index)
> {
> struct xhci_ring *ep_ring;
> + struct urb_priv *urb_priv;
> struct xhci_td *td;
> int num_trbs;
> struct xhci_generic_trb *start_trb;
> @@ -1968,10 +1974,13 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> num_trbs);
>
> ret = prepare_transfer(xhci, xhci->devs[slot_id], ep_index,
> - num_trbs, urb, &td, mem_flags);
> + num_trbs, urb, 0, mem_flags);
> if (ret < 0)
> return ret;
>
> + urb_priv = urb->hcpriv;
> + td = urb_priv->td[0];
> +
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> * until we've finished creating all the other TRBs. The ring's cycle
> @@ -2052,6 +2061,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> struct xhci_generic_trb *start_trb;
> int start_cycle;
> u32 field, length_field;
> + struct urb_priv *urb_priv;
> struct xhci_td *td;
>
> ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
> @@ -2076,10 +2086,13 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> if (urb->transfer_buffer_length > 0)
> num_trbs++;
> ret = prepare_transfer(xhci, xhci->devs[slot_id], ep_index, num_trbs,
> - urb, &td, mem_flags);
> + urb, 0, mem_flags);
> if (ret < 0)
> return ret;
>
> + urb_priv = urb->hcpriv;
> + td = urb_priv->td[0];
> +
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> * until we've finished creating all the other TRBs. The ring's cycle
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index e5eb09b..6d4b21c 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -858,6 +858,9 @@ struct xhci_event_cmd {
> /* Control transfer TRB specific fields */
> #define TRB_DIR_IN (1<<16)
>
> +/* Isoc TRB specific fields */
> +#define TRB_SIA (1<<31)
> +
> struct xhci_generic_trb {
> u32 field[4];
> };
> @@ -1015,6 +1018,12 @@ struct xhci_scratchpad {
> dma_addr_t *sp_dma_buffers;
> };
>
> +struct urb_priv {
> + u16 length;
> + int td_cnt;
> + struct xhci_td *td[0];
> +};
> +
> /*
> * Each segment table entry is 4*32bits long. 1K seems like an ok size:
> * (1K bytes * 8bytes/bit) / (4*32 bits) = 64 segment entries in the table,
> @@ -1241,6 +1250,7 @@ void xhci_free_or_cache_endpoint_ring(struct xhci_hcd *xhci,
> struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
> bool allocate_in_ctx, bool allocate_completion,
> gfp_t mem_flags);
> +void urb_free_priv(struct xhci_hcd *xhci, struct urb_priv *urb_priv);
> void xhci_free_command(struct xhci_hcd *xhci,
> struct xhci_command *command);
>
> --
> 1.6.0.4
>
>
>
>
> --
> 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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
