Hi Sarah,
I have some questions about it. You know I don't know xhci driver well.
Please correct me if I was wrong.
On Sat, 2011-06-18 at 01:12 +0800, Sarah Sharp wrote:
> 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:
I saw the xhci_urb_enqueue() (call prepare_ring()) has checked the
ep_state to prevent to add TD to ep_ring for DISABLED EP.
>
> 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.
>
The section 4.7 of the xHCI spec says that the Endpoint Not Enable Error
Event TRB with TRB Pointer, TRB Transfer Length, Event Data(ED) fields
set to '0'. It means that this Event is not interrelated with any TDs
directly. So we don't know the factual situation of the ep_ring, i.e.
which URB of the TD can be used to return -EPROTO? Can it be the first
TD in the ep_ring->td_list or all TDs in the ep_ring->td_list even they
hasn't be transfered?
Thanks,
Alex
--
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
