On Fri, 25 Jul 2003, Duncan Sands wrote:
> My patch also doesn't need to add a list header to each TD: you can just
> reuse the list entry. I added the new list head to keep things symmetric
> with QHs.
In the end, I find it distasteful to separate the freeing of the TDs and
the QHs. They should be freed at the same time, because you can't be sure
that the TDs are unlinked from the schedule until the QH is. Of course,
isochronous URBs don't have QHs and so must be handled differently.
The other complication is in uhci_control_retrigger_status(), where the QH
and _some_ of the TDs are freed, and another QH is allocated to manage the
remaining TD. But Johannes is convinced now that this approach is
unnecessary, and when than routine is cleaned up the complication will go
away.
> I guess you preferred to keep the TDs as part of the urbp (rather than
> making them part of the QH) because isochronous transfers have no QH?
> Fair enough. However, wouldn't a cleaner approach be to do the same for
> QHs as you do for TDs? I mean: just have a list of urbp's to be removed
> (rather than a list of QHs), and when the urbp goes down destroy the TDs
> and any QH? That way you can remove a list_head (remove_list) from the
> QH as well :)
Yes, that would be better. The urb_remove_list could subsume the duties
of the qh_remove_list. A tricky aspect would be that when processing the
urb_remove_list (carried out by uhci_remove_pending_qhs(), which _really_
needs to be renamed), it would be necessary to distinguish URBs that are
already on the complete_list (because they finished normally) from those
that aren't (because they were unlinked).
> Why not define uhci_get_urbp and uhci_put_urbp methods?
> Especially uhci_put_urbp, which would call uhci_destroy_urb_priv
> when the urbp goes down. At the moment you do this by hand
> each time.
That's not a bad idea. Since I was only doing this in a couple of places,
it didn't occur to me to formalize things with get_ and put_ functions.
But it makes sense.
> > @@ -943,6 +940,7 @@
> >
> > urbp->qh = uhci_alloc_qh(uhci, urb->dev);
> > if (!urbp->qh) {
> > + atomic_dec(&urbp->refcount);
>
> So no destroy_urb_priv here...
>
> > err("unable to allocate new QH for control retrigger");
> > return -ENOMEM;
> > }
That's the special case in uhci_control_retrigger_status() I mentioned
above.
> > + if (urbp && atomic_dec_and_test(&urbp->refcount)) {
> > + qh->urbp = NULL;
> > + urbp->qh = NULL;
> > + spin_unlock_irqrestore(&uhci->qh_remove_list_lock, flags);
> > + uhci_destroy_urb_priv(uhci, urbp);
>
> Why release the spinlock before calling this?
Because uchi_destroy_urb_priv() calls uhci_remove_td(), which acquires the
frame_list_lock. According to the locking rules spelled out at the end of
uhci-hcd.h, frame_list_lock and qh_remove_list_lock may not be held
simultaneously.
It's ironic that this code path is only used for non-iso. TDs, which don't
need uhci_remove_td(). But releasing the spinlock seemed easier than
trying to avoid the unnecessary function call. On further thought, I
guess the test
if (td->frame == -1 && list_empty(&td->fl_list))
which appears at the start of uhci_remove_td() could be duplicated in
uhci_destroy_urb_priv() and used to avoid making the unnecessary call.
Then the spinlock would not need to be released.
> By the way, looks like much of the list-handling in uhci-hcd could be done
> more cleanly using the macros from list.h.
Agreed. But that's just a cleanup, not a fix. It can be done some other
time.
On another subject, related to the problem you originally reported...
I've been experiencing difficulties with one of the tests in usbtest when
using a UHCI controller. The same thing happened on two different
systems. I can provide more details if you want, but the basic idea is
that the test queues several control URBs to endpoint 0. The URBs are
standard control requests, like get device descriptor or get config
descriptor. Individually the URBs work fine, but when they are queued the
second URB always fails with a STALL.
With an OHCI controller, running the test on the same USB device works
perfectly. With UHCI, the second URB stalls. That happens even when I
switch the order in which the URBs are queued. I can't figure out what's
going wrong. Any ideas?
Alan Stern
-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
