On Thu, Jul 24, 2003, Alan Stern <[EMAIL PROTECTED]> wrote: > On Wed, 23 Jul 2003, Johannes Erdfelt wrote: > > > > 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? > > It's documented in include/linux/usb.h -- it is the number of iso. > descriptors that reported errors. I don't know when it was added, but it > must have been a while back. UHCI _increments_ it properly for each > error; it just doesn't _initialize_ the field to begin with.
My brain wasn't functioning well. error_count has been in since 2.4. It's just as broken in uhci.o. This should probably go into the core since every HCD needs it to be set to 0. > > > 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. > > Here's an example. Suppose an iso. URB is submitted that has 5 > descriptors and an interval of 300. Let's say the driver schedules the > first descriptor for frame #100. Then looping through the descriptors, > uhci_insert_td_frame_list() will be called 5 times and will store: > > td[0] in frame 100, > td[1] in frame 400, > td[2] in frame 700, > td[3] in frame 1000, and > td[4] in frame (1300 % 1024) = 276. > > So the HC will transfer td[0] during frame 100, then td[4] during frame > 276, then td[1] during frame 400, and so on. td[4] will be transferred at > the wrong time; it shouldn't be sent until frame 276 _of the next loop_. I see. I think we should consider this a bogus request. Maybe we should return an error if we see a driver submit an URB like this. If the calling driver wants to do this, submit it as 2 seperate requests at the correct time. > > > 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. > > Okay, that makes sense. > > > When we determine that the URB is idle, as an optimization, we try to > > set the depth first flag for a group of TDs. > > Does this really help? Once the URB starts moving again, this will make > it finish up a little quicker, but not very much (unless FSBR has been > turned off completely). Besides, if an URB has already been blocked for > so long, does it matter if it then takes a little extra time to finish up? It won't matter much if FSBR is not turned off. I don't know how well it works. I do know that uhci.o (and presumably uhci-hcd.o) is faster than usb-uhci.o. However, it could be because of other optimizations. It should help, but I won't argue if there is a reason to remove it :) > > > 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. > > Do you mean that uhci_submit_isochronous() isn't supposed to skip length-0 > descriptors or do you mean that uhci_result_isochronous() is? That uhci_result_isochronous() isn't skipping length-0. It should be. Or perhaps we shouldn't allow length-0 descriptors? I think there's probably a good use for them, so we should make sure they are supported correctly. > > > 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? > > You are. Here's another example that will make it clearer. Let's suppose > that 3 URBs (a, b, and c) are queued for the same endpoint. To keep > things simple, assume each is a bulk transfer with only 1 TD. So > initially, the schedule looks like this: > > QHa: Link -> skel_term_qh, Element -> TDa > TDa: Link -> QHb > QHb: Link -> skel_term_qh, Element -> TDb > TDb: Link -> QHc > QHc: Link -> skel_term_qh, Element -> TDc > TDc: Link -> null > > Now suppose the HC completes TDa just before a driver unlinks URB b. When > TDa is completed, its Link is copied into QHa's Element pointer. So now > the schedule looks like: > > QHa: Link -> skel_term_qh, Element -> QHb > TDa: Link -> QHb > ... > > The unlink request comes in, and uhci_unlink_generic() calls > uhci_delete_queued_urb(). Since URB b is queued, the routine sets purbp > to URB a and pltd to TDa. Since nurbp points to URB c, the code sets > pltd->link to point to QHc -- that's how it attempts to unlink URB b. > Now the schedule looks like this: > > QHa: Link -> skel_term_qh, Element -> QHb > TDa: Link -> QHc > ... > > As you can see, URB b has not been properly removed from the schedule; the > HC will access it immediately after reading QHa. > > This race is very difficult to solve correctly without shutting down the > entire queue. I see the race now. I think I ignored it in the past because we would unlink QHa very shortly since it was complete. Of course, that doesn't matter because the HC has gone and started processing QHb already. I think we probably need to shut down the queue and remove it. I'll take a look at the spec and see if we can do something clever to make sure there aren't any races here. > > > 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. > > I don't agree. Primarily, what do you mean by "the HCD owns the QH"? Do > you mean that the HC is going to read the QH at various unpredictable > times? That shouldn't matter; there are plenty of places where the code > modifies QH link pointers under the same circumstances, so why should an > element pointer be any different? Because the element pointer is R/W by the HC. > Or do you mean that the HC is going to _write_ the QH element pointer at > various unpredictable times? That's not true in this case. Since the > element pointer points at a TD that got a short read and is hence marked > inactive, the HC won't update the element pointer any more. No, you're right. I think I was being a bit too cautious at the time. > > > 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. > > Darn it -- I was aware of that and then forgot it. Trying to juggle too > many concepts at once. Obviously, you can't give back an URB until the HC > is finished using the transfer buffer. > > So okay, it just means that urb_remove_list is subject to the same race > as qh_remove_list. Yes. Ok, so we need to add the frame the URB or QH was deleted as well. 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
