>>>>>While I agree some of the locking can be made simpler, it can't be >>>>>reduced to 1 lock. >>>> >>>>Could you elaborate? ... >>>> >>>>I think that if you treat "one spinlock per HC" as a >>>>technical goal, you'll find it's very possible. >>> >>>Take a look at the discussions a year (or so?) ago about the recursive >>>spinlocks and the then later with the switch to using the complete list. >>> >>>The short of it: We need to grab usb_list_lock to find which URB >>>finished, which means we hold a lock while complete() is called, and the >>>URB completion routine can submit a new URB = deadlock. >> >>Of course, if you took a different strategy that wasn't >>focussed on an URB list (like both UHCIs do), that issue >>needn't come up. For example, scanning active QHs for TDs >>that have completed (successfully or with errors), you know >>the urb associated with each TD. > > 1) Not all transfers have QH's (Interrupt, Isochronous)
Hmm, I thought that only ISO TDs were allowed to live outside a QH. Regardless, the point is actually that scanning the schedule for TDs (normally done through QHs) lets you get to those URBs directly; you don't need another list of URBs. This is an area where the EHCI and UHCI hardware data structures are _very_ similar. Both have periodic schedule tables that have ISO TDs followed by interrupt transfers (QHs), and an async schedule (control/bulk) arranged in a ring. And the hardware does similar updates for both: TDs get marked, QHs get updated. Differences include the fact that EHCI lets each part of the schedule be turned off independently, the UHCI "ring" is a driver policy issue (for FSBR) which can be disabled, and in UHCI those async transfers hang out at the end of the periodic schedule. None of those differences is enough to make UHCI drivers need a list of URBs (and corresponding added locks) ... EHCI needs very similar handling, but does fine with just one lock per HC. > 2) When we submit a new URB, we have to add that new QH to the list > > The problem here is no matter which piece of identifying information we > use, we need to put it into a list and *that's* the problem. Right, but how many lists? It's already living in one list that's locked, and which moreover is sorted in a useful order (the schedule), so there's no need to have those requests also live in parallel data structures with a different locking policy. >>During completion callbacks there are some invariants you >>can rely on, like that URB not changing (not that it should >>matter, all its TDs finished!) and that no URB could queue >>in front of the TDs you already scanned. > > But that requires dropping the lock and that means *anything* can happen > to the list. If you save the position you're currently on and acquire > the lock and continue after the callback, you may find the URB you're > working on disappeared already, either because of the callback or > another CPU. No, not "anything". The normal case is URB getting unlinked _at the same time as_ the callback. Only interrupt transfers don't work that way (and I hope to see that change later in 2.5, it'll simplify quite a bit of stuff at several layers in the USB stack). If you work by scanning the schedule rather than a list of URBs, there's even less variability: the HC updates every QH so TDs that have finished don't show up there, and marks non-QH TDs. Scan the list of TDs that _were_ linked to the schedule, issuing completions as needed, then stop when you get to a TD the HC is still working on. The only lock needed is a "schedule lock" to keep other threads from tweaking the schedule while you scan it; drop that during the completion callback. In the rare cases that the part of the schedule you're looking at got updated when you get the lock back, recovery is simple. > Take a look at uhci_interrupt in usb-uhci.c for their processing loop > and tell me that's something you would prefer over having 2 locks :) I was saying that _both_ UHCI HCDs should be simpler. Both have too many locks, their queueing code (the hairiest part of any HCD, I expect you'll agree!) is too complex ... those factors are related. Take a look at ehci_irq() in ehci-hcd.c (and the schedule scanners it arranges to be called later) and then explain why it should convert to use more locks ... :) - Dave ------------------------------------------------------- This sf.net email is sponsored by:ThinkGeek No, I will not fix your computer. http://thinkgeek.com/sf _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
