Alan Stern wrote:
> 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.
I don't think it is sufficient to call usb_hub_clear_tt_buffer() in
kill_transfer(). I believe split transfers consists of (at least?) two packets:
start split (SS) and complete split (CS). kill_transfer() is called if one of
these transfers are in progress during an unlink. However, the unlink may also
happen when SS has completed and CS has not yet started, in which case
kill_transfer() will not be called, but my guess is the TT buffer might need
clearing then as well?
>>>> + 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,
Oops, didn't notice that. :)
> so you can't return an
> error code. If you want, you could put a WARN_ON in case the queue
> isn't empty.
Ok.
--
Arvid Brodin
Enea Services Stockholm AB
--
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
