On Thu, 21 Feb 2008, Misha Zhilin wrote:
> Alan,
> I've got a bit more time now, so let me give you more detailed
> explanation of what's going on.
>
> First of all we are talking about large URBs. These are URBs that
> transfers more than 20KB of data. In other words, they require more than
> one qtd.
>
> So, how it used to be up until 2.6.24:
> Short transfer was detected inside qh_completions. EREMOTEIO status was
> set to urb_status. By means of qtd_copy_status function and urb_status
> field, the EREMOTEIO status was carried through all of qtd belonging to
> the URB. As EREMOTEIO is a "error" status, all subsequent qtd of the
> same URB even with QTD_STS_ACTIVE (!!!) were unlinked.
That's the key point: -EREMOTEIO is an "error" status. But 0 is an
unusual status also; the "normal" status is -EINPROGRESS.
> And ehci_urb_done
> was eventually called (calling in turn usb_hcd_giveback_urb).
>
> After your patch:
> Short transfer was detected in qh_completions. EREMOTEIO "error" status
> was set to last_status. Then it was almost immediately replaced by
> "normal" EINPROGRESS status.
Yes. However the "if ((token & QTD_STS_HALT) != 0)" test should
succeed, causing "stopped" to be set to 1.
> All subsequent qtd of the same URB have
> QTD_STS_ACTIVE flag set... forever.
That shouldn't matter. Since "stopped" is set and last_status isn't
-EINPROGRESS, the QTD shouldn't be skipped over; it should be removed
from the list.
> So they stay stuck in the list and
> ehci_urb_done is newer called. All the modifications you did in
> usb_hcd_giveback_urb won't help to get it called.
It still ought to be called. Perhaps the problem is here:
/* clean up any state from previous QTD ...*/
if (last) {
if (likely (last->urb != urb)) {
ehci_urb_done(ehci, last->urb, last_status);
count++;
}
ehci_qtd_free (ehci, last);
last = NULL;
last_status = -EINPROGRESS;
}
In the case where (last->urb == urb) we are handling one of these
still-active QTDs following a bad one. In this case we shouldn't set
last_status back to -EINPROGRESS. So perhaps we need to move that
statement into the "if" block:
if (last) {
if (likely (last->urb != urb)) {
ehci_urb_done(ehci, last->urb, last_status);
count++;
last_status = -EINPROGRESS;
}
ehci_qtd_free (ehci, last);
last = NULL;
}
Does that suffice to fix your problem?
> You probably didn't notice it as it's quite unique situation. I've
> noticed it because we have hardware that doing large URB transfers and
> as part of normal operations we use short reads. Our driver worked fined
> through 2.4.x and 2.6.x up until EREMOTEIO "optimization" in
> qh_completions.
That could indeed explain why I didn't see the problem in my testing.
Alan Stern
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
