On Wed, Jul 23, 2003, Alan Stern <[EMAIL PROTECTED]> wrote: > 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.
Nope. I'm actually excited that someone else has taken the time to look over my code in this detail. > 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. Yeah, I have no problem with that. > UHCI does not initialize urb->error_count to 0 anywhere. Again, > that probably ought to be done in usb_submit_urb(), anyway. I must have missed when urb->error_count was added. What is it for? Why didn't the patch that added it implement something for UHCI? > 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. I don't quite understand what you mean? It looks like it handles wraps correctly. > 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. Agreed. > 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 != ? uhci_fsbr_timeout() is called if we turned on FSBR for the transfer, but it hasn't made any progress in the transfer. This is so we don't keep FSBR for URBs that are sitting around getting NAK'd constantly. When we determine that the URB is idle, as an optimization, we try to set the depth first flag for a group of TDs. That being said, you're right, the check is inverted. > 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? Simplicity. Why allocate two structures and manage the pointers when there is little benefit? > 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. Nope, you're right. They will get out of sync. I think this was an oversight when I merged in some code from usb-uhci a long time ago. > 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. If the previous URB is finished, does it matter if QH->element is set to an already completed TD? Or am I misunderstanding the problem? > 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(). Because the HCD owns the QH still. We can't modify QH->element in that case. We would have to grab ownership of the QH by unlinking it and waiting, before we can modify QH->element safely. Allocating a new QH is just easier. > 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. Because complete_list is called ASAP, but urb_remove_list must wait until the next frame. > 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. I think you're right. We could alternatively do something similar to what OHCI does and use an even and odd frame list. > 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. Sounds good to me. > I hope this all makes sense... And I hope my responses make sense :) JE ------------------------------------------------------- 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
