On Sunday 23 January 2005 9:18 am, Alan Stern wrote: > David: > > As part of the ongoing changes to the hcd glue layer, I'd like to discuss > some more the hcd->state field. I'm still uncomfortable about the way it > gets used: > > It's not clearly owned either by usbcore or by the HCD. > > It's not protected by a semaphore or spinlock.
I think I've said for some time that it needs work. Though there don't seem to be any clearly defined problems with it just now ... we may be working towards having some, which means we'd know what to change. Having layers share ownership of state living on the border isn't necessarily a problem. Though I have thought that maybe having a per-HCD spinlock in that state might help sort some things out. > HCDs may have their own uses for it, but here's a list of all the ways it > gets used in the core: Though without the "why". Which, without re-reading the current code, I'll summarize as avoiding the need to make every HCD make such tests. (One test in usbcore instead of one in each HCD, where the HCDs have historically acted inconsistently...) There are still a few cases where the HCDs do need to track the state; but especially for a root hub operating in polled-by-usbcore mode, some parts can be almost entirely offloaded to usbcore. > 1. Tested during URB submission for a device on the bus. > > 2. Tested during root-hub control URB submission. > > 3. Tested during root-hub status polling. > > 4. Tested during IRQ handling. > > 5. Tested in get_frame_number. > > 6. Used incorrectly for root-hub power-level checking during > HC suspend and resume in hcd-pci.c. > > 7. Used for warning about unlink while the HC isn't running. > > 8. Used for warning about endpoint-disable while the HC isn't > running. I'll trust your list for now. Though "incorrectly" (in #6) isn't clear ... and remember, about four different subsystems need to interact there, and the Linux PM framework itself has some basic conceptual problems that USB must dodge. > The different tests look for different things, like HC running, HC > suspended, HC stopped, HC died... Or suspending, or resuming. We've been working our way from a framework (2.4) where HCDs do whatever they like and the other software (usbcore, drivers) had to hope it was safe ... towards one where the HCDs do what's necessary to make things safe. Right now we're closer to the latter than we were, but we're not there yet. Remote wakeup, not to mention system sleep state transitions, are makes trouble. We have BIOS interactioon issues during HC startup. And HCDs don't get cleaned up when they die; they stay forever in limbo, and have no afterlife. > I think it would be a lot better to make the conditions being tested more > explicit and not have usbcore worry about the overall state of the HC, or > at least worry about it as little as possible. So for example, we might > create some bitfields in the usb_hcd struct: > > unsigned okay_to_submit_urbs:1; > unsigned okay_to_use_root_hub:1; > unsigned okay_to_handle_irq:1; I'd rather see them as ok_to_XXX() functions, maybe wrapping atomic bitops, for what it's worth, but suspect you may be right about the basic need to factor at least some of those issues out. There might still be state that's not easily modeled as predicates though ... for example, the fact that it's safe _right now_ to submit urbs doesn't mean the HC won't die before usbcore hands the URB to the HCD. > Maybe a couple of others too. They could be set by the HCDs, although > synchronizing testing with setting would still be problematic. Testing > for whether an HC or root hub is suspended can be done through the struct > device power setting; it doesn't need an extra flag. Atomic bitops have test-and-set primitives; a wrapper-of-bitop approach might address that issue. That PM framework stuff is still borked, even in struct device; I can't see a way to use it without surgery to the PM code. > The warnings can be removed. They were all added because they reflected real -- and nasty! -- problems. Until the problems go away, the warnings still help. For example, we still get folk with IRQ setup problems. And I'd much rather have someone find a message "controller is probably using the wrong IRQ" and solve that issue first, than go back to getting lots of "USB is borken, help!!!" messages which, after investigation turn out to be "controller used wrong IRQ, get the ACPI guys to fixit". > HCDs should be able to handle unlinks while > the HC isn't running -- or else we should guarantee that such unlinks > never happen. I don't follow you here. They should handle them, and I think both OHCI and EHCI already do so. We can't guarantee that no such unlinks happen. Simple scenario: cardbus physical eject, while syncing to the USB device on that card. > We can avoid the need to disable endpoints while an HC is > stopped by delaying altsetting changes for a device until the device is > resumed (there's already a comment about that in the hub driver). Again I don't follow. A "stopped" state doesn't exist, do you mean "suspended"? And I don't know what comment you mean. - Dave ------------------------------------------------------- This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting Tool for open source databases. Create drag-&-drop reports. Save time by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc. Download a FREE copy at http://www.intelliview.com/go/osdn_nl _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
