On Thursday 05 May 2005 8:16 am, Alan Stern wrote: > On Wed, 4 May 2005, David Brownell wrote: > > > ohci-hub.c:ohci_hub_suspend(): > > > > > > if (status == 0) > > > hcd->state = HC_STATE_SUSPENDED; > > > > I think I remember why that's there. It pairs the earlier line > > in the same function to set hcd->state to QUIESCING. And the reason > > for that is because hcd->state is the only hook we have for, erm, > > quiescing the traffic going into the HCD. > ... > > We have no other way to throttle down traffic just now... > > Do we really need to throttle down traffic from within usbcore?
Sure looked like it last time I checked this issue out in detail. > Won't it > be enough to rely on the HCDs to reject submissions when they are unable > to accept them? (Plus the fact that thanks to all the cleanups made in > the last couple of years, hardly any submissions are made at inappropriate > times.) Thing is there can be a window of around 10 msec there, unless this is the autosuspend case. During those 10 msec, something needs to prevent other code from using that device (e.g. on SMP). For now, that something is hcd->state. Maybe it can be improved, but I don't have timte to do it. > > I think a better way of looking at this is that the two notions > > may not yet been fully teased apart. > > If you really want to do this from within usbcore, consider restricting > hcd->state to only three possible values: > > Okay to submit and unlink (running normally); > Okay to unlink but not to submit (quiescing); > Not okay to unlink or submit (any other state). That is, two bits to control submit and unlink capabilities, rather than the various other ones that now compose hcd->state? Sounds plausible. > No part of usbcore outside of hcd-pci.c should care about anything else. > And while we're at it, make it clear who owns hcd->state -- either the HCD > or usbcore does, and the other shouldn't write to it at all. > > Any additional information needed by hcd-pci.c or the HCDs (such as > whether IRQs are enabled) can be stored in a separate new field. Sounds plausible. As I recall, the reason usbcore touches hcd->state is primarily since the HCDs couldn't previously be relied on to do it right in all the relevant cases. > > However, it seems I've been interpreting hcd->state as representing > > the parts of "root hub suspended" that are a superset of what the > > root_hub->state == USB_STATE_SUSPENDED means. And leaving the other > > parts to bus-specific logic. > > > > > > One complication is that there's _also_ a need to represent the > > state of the HC itself. Including cases like: > > <snip> > > > There's also the struct device dev->power.power_state field, which > > has always been problematic (vergin on useless) since the PM core > > doesn't manage it coherently. It's never really been usable as > > anything other than a boolean ... not powerful enough to distinguish > > cases like [4] in current kernels, either. > > Apart from the PCI bus glue, all of that stuff is private to the HCDs. > It shouldn't be folded in along with a field used elsewhere in usbcore. Most of that "superset" is the root hub timer, and after your timer updates get merged (post-2.6.12) it becomes more reasonable for those bits to be managed that way. Re the [1]..[4] cases, I'd have to look up the snipped bits to see if I agree with applying that argument. > And even the PCI glue info should be kept separate from things used only > by HCDs. Right now hcd->state mixes all these different categories > together in a confusing and error-prone way. It's called "evolution in action". ;) > > Those two "host controller" suspend/resume calls are effectively > > there only for the PCI based controllers, and nothing they currently > > do seems HC-specific. (Other than the PMAC hooks, which seem to > > reflect deficiencies in the PCI suspend/resume framework... that > > stuff should be handled using generic PCI platform hooks.) I'd > > be happy to see those hooks vanish. > > You mean the suspend/resume routines in hcd-pci.c? Once a suitable > generic PCI power driver is available they shouldn't be needed. We're nearly there already, in fact. - Dave ------------------------------------------------------- This SF.Net email is sponsored by Oracle Space Sweepstakes Want to be the first software developer in space? Enter now for the Oracle Space Sweepstakes! http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
