Hi Alex,
I thought at first that this patch was correct, but looking at it
further, I think this patch will leak URBs.
On Wed, Jun 08, 2011 at 06:25:50PM +0800, Alex He wrote:
> xHCI 1.0: Endpoint Not Enabled Error
>
> To handle the Endpoint Not Enabled Error described in section 4.7 of the xHCI
> spec v1.0. Add the limit condition to avoid ringing the doorbell of the dis-
> abled slot and do the WARN for the Endpoint Not Enabled Error.
>
> Signed-off-by: Alex He <alex.he@xxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 800f417..49c8bfe 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -314,6 +314,16 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
> __le32 __iomem *db_addr = &xhci->dba->doorbell[slot_id];
> struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
> unsigned int ep_state = ep->ep_state;
> + struct xhci_virt_device *dev = xhci->devs[slot_id];
> + struct xhci_slot_ctx *slot_ctx;
> + unsigned int slot_state;
> +
> + slot_ctx = xhci_get_slot_ctx(xhci, dev->out_ctx);
> + slot_state = GET_SLOT_STATE(slot_ctx->dev_state);
> +
> + /* Don't ring the doorbell of the disabled slot */
> + if (slot_state == SLOT_STATE_DISABLED)
> + return;
>
> /* Don't ring the doorbell for this endpoint if there are pending
> * cancellations because we don't want to interrupt processing.
> @@ -1995,6 +2005,10 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> case COMP_BUFF_OVER:
> xhci_warn(xhci, "WARN: buffer overrun event on endpoint\n");
> break;
> + case COMP_EBADEP:
> + xhci_warn(xhci, "WARN: Doorbell rung for disabled slot "
> + "or endpoint\n");
> + goto cleanup;
What happens if we've placed a TD on a disabled endpoint ring and rung
the doorbell? If you go directly down to cleanup, ret will still be
zero, and the TD will remain on the TD list:
cleanup:
/*
* Do not update event ring dequeue pointer if ep->skip is set.
* Will roll back to continue process missed tds.
*/
if (trb_comp_code == COMP_MISSED_INT || !ep->skip) {
inc_deq(xhci, xhci->event_ring, true);
}
if (ret) {
urb = td->urb;
urb_priv = urb->hcpriv;
/* 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 control endpoints).
*/
if (usb_endpoint_xfer_control(&urb->ep->desc) ||
(trb_comp_code != COMP_STALL &&
trb_comp_code != COMP_BABBLE))
xhci_urb_free_priv(xhci, urb_priv);
usb_hcd_unlink_urb_from_ep(bus_to_hcd(urb->dev->bus), urb);
if ((urb->actual_length != urb->transfer_buffer_length &&
(urb->transfer_flags &
URB_SHORT_NOT_OK)) ||
status != 0)
xhci_dbg(xhci, "Giveback URB %p, len = %d, "
"expected = %x, status = %d\n",
urb, urb->actual_length,
urb->transfer_buffer_length,
status);
spin_unlock(&xhci->lock);
/* EHCI, UHCI, and OHCI always unconditionally set the
* urb->status of an isochronous endpoint to 0.
*/
if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
status = 0;
usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, status);
spin_lock(&xhci->lock);
}
The ring code will get stuck if you leave the TD on the td_list, and the
driver will eventually ask you to cancel the URB, and then we get into
further issues with the cancellation code and disabled endpoints. I
think you need to allow the code to try to find the TD and its URB, so
that it can be given back to the driver with an error return code.
Possibly -EPROTO? We don't want to return -ENODEV, because the device
might still be there, just not that particular endpoint.
I think to fix this, you need to add handling for the error code to the
various process_*_td functions that sets the status code (or the frame
status code for isochronous endpoints) to -EPROTO.
Sarah Sharp
--
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
