Alan Stern wrote:
> On Wed, 20 Jul 2011, Arvid Brodin wrote:
>
>> When a low or full speed urb in progress is unlinked (or some other error
>> occurs), the buffer in the transaction translator (part of the hub) might end
>> up in an inconsistent state. This can make all further low and full speed
>> transactions fail, unless the buffer is cleared.
>>
>> The bug can be seen when running the usbtest unlink tests as "set altsetting
>> to 0 failed, -110", and gets fixed by this patch.
>>
>> Signed-off-by: Arvid Brodin <arvid.brodin@xxxxxxxx>
>> ---
>> drivers/usb/host/isp1760-hcd.c | 83 +++++++++++++++++++++++++++++++++++----
>> 1 files changed, 74 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
>> index 840beda..2ab8fb2 100644
>> --- a/drivers/usb/host/isp1760-hcd.c
>> +++ b/drivers/usb/host/isp1760-hcd.c
>> @@ -114,6 +114,7 @@ struct isp1760_qh {
>> u32 toggle;
>> u32 ping;
>> int slot;
>> + int tt_buffer_dirty; /* See USB2.0 spec section 11.17.5 */
>> };
>>
>> struct urb_listitem {
>> @@ -928,6 +929,10 @@ static void enqueue_qtds(struct usb_hcd *hcd, struct isp1760_qh *qh)
>> return;
>> }
>>
>> + /* Make sure this endpoint's TT buffer is clean before queueing ptds */
>> + if (qh->tt_buffer_dirty)
>> + return;
>> +
>> if (usb_pipeint(list_entry(qh->qtd_list.next, struct isp1760_qtd,
>> qtd_list)->urb->pipe)) {
>> ptd_offset = INT_PTD_OFFSET;
>> @@ -1281,6 +1286,15 @@ static irqreturn_t isp1760_irq(struct usb_hcd *hcd)
>>
>> case PTD_STATE_URB_RETIRE:
>> qtd->status = QTD_RETIRE;
>> + if ((qtd->urb->dev->speed != USB_SPEED_HIGH) &&
>> + (qtd->urb->status != -EPIPE) &&
>> + (qtd->urb->status != -EREMOTEIO)) {
>
> You also want to avoid this if urb->status == 0. Do you know from
> PTD_STATE_URB_RETIRE that the status must be nonzero?
Correct; if we're here then some kind of transaction error was detected by the
hardware and urb->status has been set to some nonzero value by
check_{atl,int}_transfer() above.
>> + qh->tt_buffer_dirty = 1;
>> + if (usb_hub_clear_tt_buffer(qtd->urb))
>> + /* Clear failed; let's hope things work
>> + anyway */
>> + qh->tt_buffer_dirty = 0;
>> + }
>> qtd = NULL;
>> qh->toggle = 0;
>> qh->ping = 0;
>> @@ -1573,6 +1587,41 @@ static void kill_transfer(struct usb_hcd *hcd, struct urb *urb,
>> priv->active_ptds--;
>> }
>>
>> +/*
>> + * 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.)
>> + }
>> +}
>> +
>> static int isp1760_urb_dequeue(struct usb_hcd *hcd, struct urb *urb,
>> int status)
>> {
>> @@ -1595,9 +1644,8 @@ static int isp1760_urb_dequeue(struct usb_hcd *hcd, struct urb *urb,
>>
>> list_for_each_entry(qtd, &qh->qtd_list, qtd_list)
>> if (qtd->urb == urb) {
>> - if (qtd->status == QTD_XFER_STARTED)
>> - kill_transfer(hcd, urb, qh);
>> - qtd->status = QTD_RETIRE;
>> + dequeue_urb_from_qtd(hcd, qh, qtd);
>> + break;
>> }
>>
>> urb->status = status;
>> @@ -1622,12 +1670,11 @@ static void isp1760_endpoint_disable(struct usb_hcd *hcd,
>> if (!qh)
>> goto out;
>>
>> - list_for_each_entry(qtd, &qh->qtd_list, qtd_list) {
>> - if (qtd->status == QTD_XFER_STARTED)
>> - kill_transfer(hcd, qtd->urb, qh);
>> - qtd->status = QTD_RETIRE;
>> - qtd->urb->status = -ECONNRESET;
>> - }
>> + 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?
--
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
