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?
> + 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?
> + }
> +}
> +
> 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.
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
