Johannes:
The more I study the UHCI driver source, the more questions and other
observations come up. I hope you won't mind taking the time to respond.
First, I mentioned in an email yesterday that UHCI doesn't support
urb->timeout. That was wrong, it does. So does the hc_simple driver.
But OHCI and EHCI don't, and in any case it seems like a good idea to
centralize that support in the hcd glue layer rather than spreading it
among all the HCDs.
UHCI does not initialize urb->error_count to 0 anywhere. Again,
that probably ought to be done in usb_submit_urb(), anyway.
What happens when an isochronous URB is submitted with descriptor
intervals that wrap all the way around the frame list (that is, if
uhci_insert_td_frame_list() is called with framenum > current-frame +
1024)? It looks like the TDs get placed on the list anyway and then
processed at the wrong time.
There are a few places in uhci-hcd.c and uhci-hcd.h that talk
about low speed and high speed. I assume that is really supposed to be
low speed and _full_ speed. It might be worth changing them.
I really don't understand what's going on in uhci_fsbr_timeout().
What's it trying to accomplish? And in the line
if (tmp != head && (count % DEPTH_INTERVAL) == (DEPTH_INTERVAL - 1))
should that == be != ?
The uhci_frame_list structure contains both hardware and software
portions. The hardware parts have to be allocated in DMA-consistent
memory but the software portions don't. Why are they all grouped
together?
uhci_submit_isochronous() skips over frame descriptors that have
length 0; it doesn't set up 0-length TDs. Is that appropriate? By
contrast, uhci_result_isochronous() doesn't skip over 0-length frame
descriptors, so the descriptors and the TDs will get out of sync.
There's a nasty race in uhci_delete_queued_urb(). If the URB
being deleted isn't at the start of its queue and the HC finishes the
previous URB at about the same time as the function is called, then the
link pointer from the last TD in the previous URB will be copied into the
element pointer of the previous URB's QH. This might happen before the
code manages to overwrite the TD's link pointer, but the code completely
ignores that possibility.
I don't understand why uhci_control_retrigger_status() is so
complicated. Why does it have to allocate a new QH instead of re-using
the old one? There's a comment about avoiding pointer overwriting
problems -- what problems? Why not simply store the DMA address of the
status TD in the QH's element pointer? Of course this would require
testing to see if the status TD had already been executed, but that
wouldn't be hard. Doing this would also simplify uhci_remove_qh(), since
it would only get called immediately after uhci_delete_queued_urb().
Why are urb_remove_list and complete_list separate lists? There
doesn't seem to be any reason for putting URBs on the urb_remove_list,
since all that happens is that uhci_remove_pending_qhs() moves them onto
the complete_list anyway.
There's another race, this one against the hardware, in the code
that adds items to qh_remove_list. The whole reason for having this list
is because the items on it musn't be processed until after a
Start-Of-Frame. But on an SMP system, it's entirely possible that an item
could be added after the EOF interrupt had arrived and before uhci_irq()
had started processing the list. The only way to protect against this
problem is to record the current frame number at the time an entry is
added to the list, and only process the list entries with frame numbers
different from the current one at interrupt time.
There is still the problem of freeing TDs too soon (that is,
before the next SOF. How does this sound as a solution: Have
uhci_finish_urb() call uhci_destroy_urb_priv() only for URBs with no QH
(i.e., isochronous). For all others it would merely sever the links
between the urb and the urbp, and the rest could be handled by having
uhci_free_pending_qhs() call uhci_destroy_urb_priv() for the QH's urbp.
I'll work out a patch that does that and try it.
I hope this all makes sense...
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
