One small thing I noticed is that this changes what the xHCI driver does
with urb->status. I think that the host driver isn't supposed to
directly set urb->status. I think the host driver is only supposed to pass the
status to usb_hcd_giveback_urb().
The problem is that drivers aren't supposed to look at urb->status
until their completion function is called, but I think some of them
still might. So if the URB generates multiple events, the driver might
see the change of status for the first event and deallocate the URB
before the xHCI host has a chance to deal with the second event. I
think it's best to let the USB core handle setting urb->status.
I did a quick grep for "urb->status", and none of the major hosts (EHCI,
UHCI, OHCI) directly set urb->status. There are a few embedded ones
(fhci, imx21, isp1362, isp1760, & oxu210hp), but I have no idea if
they're doing the correct thing.
Plus, I'm not sure if finish_td() should unconditionally set
urb->status. I would have to think hard about the stopped endpoint,
short packet, and stall cases. The problem is we can get multiple
events for an URB, and I'm not sure if the xHCI driver should overwrite
urb->status for each of those events. In some cases, taking the last
status could be OK, but I'm not sure if it's true of all cases.
It's a pretty easy fix for this though. Just pass the address of the
status variable in handle_tx_event() to finish_td(), and let it
overwrite it if necessary. Then you don't have to set urb->status in
finish_td().
I think this issues effects other refactoring patches and the
isochronous patches.
On Fri, Jun 25, 2010 at 04:48:54PM +0800, Andiry Xu wrote:
> >From 5eed654ae1f0304ef2513a63a25f7849338fc8af Mon Sep 17 00:00:00 2001
> From: Andiry Xu <andiry.xu@xxxxxxx>
> Date: Thu, 24 Jun 2010 14:15:16 +0800
> Subject: [PATCH 1/9] xHCI: handle_tx_event() refactor: finish_td
>
> This patch moves the td universal processing part in handle_tx_event()
> into a seperate function finish_td().
>
> If finish_td() returns 1, it indicates the urb can be given back.
>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 190 +++++++++++++++++++++++++-----------------
> 1 files changed, 115 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index a2064a0..c73bfce 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1213,6 +1213,105 @@ int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code)
> }
>
> /*
> + * Finish the td procession, remove the td from td list;
> + * Return 1 if the urb can be given back.
> + */
> +static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
> + union xhci_trb *event_trb, struct xhci_transfer_event *event,
> + struct xhci_virt_ep *ep, int status, bool skip)
So this becomes:
+static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
+ union xhci_trb *event_trb, struct xhci_transfer_event *event,
+ struct xhci_virt_ep *ep, int *status, bool skip)
> +{
> + struct xhci_virt_device *xdev;
> + struct xhci_ring *ep_ring;
> + unsigned int slot_id;
> + int ep_index;
> + struct urb *urb = NULL;
> + struct xhci_ep_ctx *ep_ctx;
> + int ret = 0;
> + u32 trb_comp_code;
> +
> + slot_id = TRB_TO_SLOT_ID(event->flags);
> + xdev = xhci->devs[slot_id];
> + ep_index = TRB_TO_EP_ID(event->flags) - 1;
> + ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer);
> + ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
> + trb_comp_code = GET_COMP_CODE(event->transfer_len);
> +
> + if (skip)
> + goto td_cleanup;
> +
> + if (trb_comp_code == COMP_STOP_INVAL ||
> + trb_comp_code == COMP_STOP) {
> + /* The Endpoint Stop Command completion will take care of any
> + * stopped TDs. A stopped TD may be restarted, so don't update
> + * the ring dequeue pointer or take this TD off any lists yet.
> + */
> + ep->stopped_td = td;
> + ep->stopped_trb = event_trb;
> + return 0;
> + } else {
> + if (trb_comp_code == COMP_STALL) {
> + /* The transfer is completed from the driver's
> + * perspective, but we need to issue a set dequeue
> + * command for this stalled endpoint to move the dequeue
> + * pointer past the TD. We can't do that here because
> + * the halt condition must be cleared first. Let the
> + * USB class driver clear the stall later.
> + */
> + ep->stopped_td = td;
> + ep->stopped_trb = event_trb;
> + ep->stopped_stream = ep_ring->stream_id;
> + } else if (xhci_requires_manual_halt_cleanup(xhci,
> + ep_ctx, trb_comp_code)) {
> + /* Other types of errors halt the endpoint, but the
> + * class driver doesn't call usb_reset_endpoint() unless
> + * the error is -EPIPE. Clear the halted status in the
> + * xHCI hardware manually.
> + */
> + xhci_cleanup_halted_endpoint(xhci,
> + slot_id, ep_index, ep_ring->stream_id,
> + td, event_trb);
> + } else {
> + /* Update ring dequeue pointer */
> + while (ep_ring->dequeue != td->last_trb)
> + inc_deq(xhci, ep_ring, false);
> + inc_deq(xhci, ep_ring, false);
> + }
> +
> +td_cleanup:
> + /* Clean up the endpoint's TD list */
> + urb = td->urb;
> +
> + /* Do one last check of the actual transfer length.
> + * If the host controller said we transferred more data than
> + * the buffer length, urb->actual_length will be a very big
> + * number (since it's unsigned). Play it safe and say we didn't
> + * transfer anything.
> + */
> + if (urb->actual_length > urb->transfer_buffer_length) {
> + xhci_warn(xhci, "URB transfer length is wrong, "
> + "xHC issue? req. len = %u, "
> + "act. len = %u\n",
> + urb->transfer_buffer_length,
> + urb->actual_length);
> + urb->actual_length = 0;
> + if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> + status = -EREMOTEIO;
> + else
> + status = 0;
These three lines become:
+ *status = -EREMOTEIO;
+ else
+ *status = 0;
> + }
> + list_del(&td->td_list);
> + /* Was this TD slated to be cancelled but completed anyway? */
> + if (!list_empty(&td->cancelled_td_list))
> + list_del(&td->cancelled_td_list);
> +
> + urb->status = status;
The above line gets removed.
> + ret = 1;
> + }
> +
> + return ret;
> +}
> +
> +/*
> * If this function returns an error condition, it means it got a Transfer
> * event with a corrupted Slot ID, Endpoint ID, or TRB DMA address.
> * At this point, the host controller is probably hosed and should be reset.
> @@ -1233,6 +1332,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> int status = -EINPROGRESS;
> struct xhci_ep_ctx *ep_ctx;
> u32 trb_comp_code;
> + int ret = 0;
>
> xhci_dbg(xhci, "In %s\n", __func__);
> slot_id = TRB_TO_SLOT_ID(event->flags);
> @@ -1263,7 +1363,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> xhci_dbg(xhci, "Event TRB with TRB type ID %u\n",
> (unsigned int) (event->flags & TRB_TYPE_BITMASK)>>10);
> xhci_print_trb_offsets(xhci, (union xhci_trb *) event);
> - urb = NULL;
> goto cleanup;
> }
> xhci_dbg(xhci, "%s - getting list entry\n", __func__);
> @@ -1334,7 +1433,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> break;
> }
> xhci_warn(xhci, "ERROR Unknown event condition, HC probably busted\n");
> - urb = NULL;
> goto cleanup;
> }
> /* Now update the urb's actual_length and give back to the core */
> @@ -1382,7 +1480,10 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>
> xhci_cleanup_halted_endpoint(xhci,
> slot_id, ep_index, 0, td, event_trb);
> - goto td_cleanup;
> +
> + ret = finish_td(xhci, td, event_trb, event, ep,
> + status, true);
This line becomes:
+ &status, true);
> + goto cleanup;
> }
> /*
> * Did we transfer any data, despite the errors that might have
> @@ -1411,7 +1512,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> td->urb->transfer_buffer_length -
> TRB_LEN(event->transfer_len);
> xhci_dbg(xhci, "Waiting for status stage event\n");
> - urb = NULL;
> goto cleanup;
> }
> }
> @@ -1513,68 +1613,16 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> TRB_LEN(event->transfer_len);
> }
> }
> - if (trb_comp_code == COMP_STOP_INVAL ||
> - trb_comp_code == COMP_STOP) {
> - /* The Endpoint Stop Command completion will take care of any
> - * stopped TDs. A stopped TD may be restarted, so don't update
> - * the ring dequeue pointer or take this TD off any lists yet.
> - */
> - ep->stopped_td = td;
> - ep->stopped_trb = event_trb;
> - } else {
> - if (trb_comp_code == COMP_STALL) {
> - /* The transfer is completed from the driver's
> - * perspective, but we need to issue a set dequeue
> - * command for this stalled endpoint to move the dequeue
> - * pointer past the TD. We can't do that here because
> - * the halt condition must be cleared first. Let the
> - * USB class driver clear the stall later.
> - */
> - ep->stopped_td = td;
> - ep->stopped_trb = event_trb;
> - ep->stopped_stream = ep_ring->stream_id;
> - } else if (xhci_requires_manual_halt_cleanup(xhci,
> - ep_ctx, trb_comp_code)) {
> - /* Other types of errors halt the endpoint, but the
> - * class driver doesn't call usb_reset_endpoint() unless
> - * the error is -EPIPE. Clear the halted status in the
> - * xHCI hardware manually.
> - */
> - xhci_cleanup_halted_endpoint(xhci,
> - slot_id, ep_index, ep_ring->stream_id, td, event_trb);
> - } else {
> - /* Update ring dequeue pointer */
> - while (ep_ring->dequeue != td->last_trb)
> - inc_deq(xhci, ep_ring, false);
> - inc_deq(xhci, ep_ring, false);
> - }
>
> -td_cleanup:
> - /* Clean up the endpoint's TD list */
> - urb = td->urb;
> - /* Do one last check of the actual transfer length.
> - * If the host controller said we transferred more data than
> - * the buffer length, urb->actual_length will be a very big
> - * number (since it's unsigned). Play it safe and say we didn't
> - * transfer anything.
> - */
> - if (urb->actual_length > urb->transfer_buffer_length) {
> - xhci_warn(xhci, "URB transfer length is wrong, "
> - "xHC issue? req. len = %u, "
> - "act. len = %u\n",
> - urb->transfer_buffer_length,
> - urb->actual_length);
> - urb->actual_length = 0;
> - if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> - status = -EREMOTEIO;
> - else
> - status = 0;
> - }
> - list_del(&td->td_list);
> - /* Was this TD slated to be cancelled but completed anyway? */
> - if (!list_empty(&td->cancelled_td_list))
> - list_del(&td->cancelled_td_list);
> + ret = finish_td(xhci, td, event_trb, event, ep, status, false);
This line becomes:
+ ret = finish_td(xhci, td, event_trb, event, ep, &status, false);
>
> +cleanup:
> + inc_deq(xhci, xhci->event_ring, true);
> + xhci_set_hc_event_deq(xhci);
> +
> + /* FIXME for multi-TD URBs (who have buffers bigger than 64MB) */
> + if (ret) {
> + urb = td->urb;
> /* Leave the TD around for the reset endpoint function to use
> * (but only if it's not a control endpoint, since we already
> * queued the Set TR dequeue pointer command for stalled
> @@ -1582,22 +1630,14 @@ td_cleanup:
> */
> if (usb_endpoint_xfer_control(&urb->ep->desc) ||
> (trb_comp_code != COMP_STALL &&
> - trb_comp_code != COMP_BABBLE)) {
> + trb_comp_code != COMP_BABBLE))
> kfree(td);
> - }
> - urb->hcpriv = NULL;
> - }
> -cleanup:
> - inc_deq(xhci, xhci->event_ring, true);
> - xhci_set_hc_event_deq(xhci);
>
> - /* FIXME for multi-TD URBs (who have buffers bigger than 64MB) */
> - if (urb) {
> usb_hcd_unlink_urb_from_ep(xhci_to_hcd(xhci), urb);
> xhci_dbg(xhci, "Giveback URB %p, len = %d, status = %d\n",
> - urb, urb->actual_length, status);
> + urb, urb->actual_length, urb->status);
> spin_unlock(&xhci->lock);
> - usb_hcd_giveback_urb(xhci_to_hcd(xhci), urb, status);
> + usb_hcd_giveback_urb(xhci_to_hcd(xhci), urb, urb->status);
> spin_lock(&xhci->lock);
> }
> return 0;
> --
> 1.7.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
