On Sun, 23 Jan 2005, David Brownell wrote:
> 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.
We are (or I am, anyway). Maybe not clearly defined problems, but
definite psychological problems in grokking how it should work.
> 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.
A spinlock might not be necessary. Or maybe one private to the HCD, not
available to usbcore.
> > 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.
Right. The code would be clearer if these common tests could be done more
explicitly, rather than relying on some preset relationship between
hcd->state and the set of allowed actions. What's legal for one HCD might
be illegal for another in the same state.
> > 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.
We'll discuss that one some other time...
> > 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.
Attack of the Zombie HCDs!!! Good thing we have usbtest to exorcize
them :-)
Yes, all these interactions do pose considerable problems. By making
explicit the things that usbcore tests for, I hope the issues will become
clearer and simpler.
> I'd rather see them as ok_to_XXX() functions, maybe wrapping atomic bitops,
That's fine with me.
> 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.
Exactly -- these things need to be thought out more fully. In this
particular example, we might agree that the test done in usbcore is merely
an optimization and the HCD is still responsible for rejecting submissions
it's unable to handle. Or alternatively an HCD could _always_ accept
submissions, even after the HC has died (knowing that they will be
unlinked in the near future).
> 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.
Hmm. In any case an HCD would probably maintain its own "suspended"
flag, properly synchronized.
> > 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".
Yes, you're right, sorry. I had forgotten the purpose of those WARN_ON
lines in hcd_unlink_urb and hcd_endpoint_disable. I guess the real
question is: Can the offending condition be given in terms that don't
rely on hcd->state? For instance, maybe it would be enough to know
whether the root hub is suspended.
(And can things be done in such a way that they will still work properly
in the absence of CONFIG_USB_SUSPEND?)
> > 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.
By "HC is stopped" I mean that it is not running or quiescing, i.e., it is
reset, suspended, or dead. The comment is in finish_port_resume:
/* FIXME maybe force to alt 0 */
Terse, yes, but _you_ wrote it... :-)
Alan Stern
-------------------------------------------------------
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
