On Thu, 21 Jul 2011, Arvid Brodin wrote:
> >> +/*
> >> + * Retire the qtds beginning at 'qtd' and belonging all to the same urb, killing
> >> + * any active transfer belonging to the urb in the process.
> >> + */
> >> +static void dequeue_urb_from_qtd(struct usb_hcd *hcd, struct isp1760_qh *qh,
> >> + struct isp1760_qtd *qtd)
> >> +{
> >> + struct urb *urb;
> >> + int urb_was_running;
> >> +
> >> + urb = qtd->urb;
> >> + urb_was_running = 0;
> >> + list_for_each_entry_from(qtd, &qh->qtd_list, qtd_list) {
> >> + if (qtd->urb != urb)
> >> + break;
> >> +
> >> + if (qtd->status >= QTD_XFER_STARTED)
> >> + urb_was_running = 1;
> >> + if (last_qtd_of_urb(qtd, qh) &&
> >> + (qtd->status >= QTD_XFER_COMPLETE))
> >> + urb_was_running = 0;
> >> +
> >> + if (qtd->status == QTD_XFER_STARTED)
> >> + kill_transfer(hcd, urb, qh);
> >> + qtd->status = QTD_RETIRE;
> >> + }
> >> +
> >> + if ((urb->dev->speed != USB_SPEED_HIGH) && urb_was_running) {
> >> + qh->tt_buffer_dirty = 1;
> >> + if (usb_hub_clear_tt_buffer(urb))
> >> + /* Clear failed; let's hope things work anyway */
> >> + qh->tt_buffer_dirty = 0;
> >
> > Do you need to do this here? Won't it automatically happen anyway in
> > the isp1760_irq() code added above?
>
> kill_transfer() reclaims the transfer slot from the hardware, which means we
> will never get an interrupt for this packet; isp1760_irq() will not get
> executed. (More details about this in earlier discussions with Sebastian
> Siewior, may 17 I think.)
I see. Then maybe usb_hub_clear_tt_buffer() belongs in
kill_transfer()? But if this is the only place that calls
kill_transfer() then it's okay to put it here instead.
> >> + list_for_each_entry(qtd, &qh->qtd_list, qtd_list)
> >> + if (qtd->status != QTD_RETIRE) {
> >> + dequeue_urb_from_qtd(hcd, qh, qtd);
> >> + qtd->urb->status = -ECONNRESET;
> >> + }
> >
> > This code shouldn't be needed. usbcore guarantees that the endpoint
> > queue is empty when endpoint_disable is called.
>
> Ok. That seems to be the subject of an additional patch (which will have to wait until
> I get back from vacation, middle of august). If I remove this, I think I want to
> add a check that the queue really is empty. Would -EBUSY be a sane error to return if
> the queue is not empty?
The endpoint_disable routine has to return void, so you can't return an
error code. If you want, you could put a WARN_ON in case the queue
isn't empty.
Alan Stern
--
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
