On Thu, 15 Sep 2005, David Brownell wrote:
> > But I was talking about a
> > different problem. Even when CONFIG_USB_SUSPEND is set, your patch will
> > prevent usbcore from suspending root hubs during FREEZE events.
>
> Actually it _does_ suspend them ... in the same way it does without
> that CONFIG option. That is, as part of suspending the controller
> itself. Though (as you pointed out later) because those #ifdefs, the
> HCDs don't expect that path when USB_SUSPEND is enabled; we don't
> want that to stick around either.
>
> At any rate, that'd only break an EXPERIMENTAL feature, one that's
> clearly labeled as "... may not work as expected ...".
True. See below.
> > The patch should have said something more like this:
> >
> > + udev = to_usb_device(dev);
> > + if (message.event == PM_EVENT_FREEZE && udev->parent) {
> > + dev->power.power_state = message;
> > + return 0;
> > + }
> > + return usb_suspend_device (udev);
>
> The operative change being testing udev->parent. I think I prefer
> the solution of having the hub suspend code call the root hub suspend
> code, to make layering a bit more clear.
Do you mean having the HCD suspend code call the root hub suspend code?
Either way could work. I prefer to do it this way, though. (1) It would
mean we don't lie to the PM core for a brief while, saying that the root
hub is in FREEZE when it isn't. (2) The logic is located in this one
spot, instead of being copied (error-prone!) among a bunch of different
HCDs.
Or perhaps you were referring to the change I suggested to the
non-USB_SUSPEND version of usb_suspend_device? That won't help with this
problem, since without the change above that code wouldn't get called.
> > > This seems related to two OHCI regressions I noticed, by the way.
> > >
> > > In one case, resume-via-reset seems broken without CONFIG_USB_SUSPEND;
> > > the root hub timer doesn't restart correctly. (It tries to resubmit
> > > the URB while the hub is suspended, fails, then dies.) In the other,
> > > wakeup is broken since khubd never notices USB_PORT_STAT_C_SUSPEND;
> > > as if the root hub timer stopped (new behavior?) and didn't restart.
> >
> > It looks like the first problem is a logical error in the USB resume code:
> > Device states are set back to USB_STATE_CONFIGURED _after_ the devices are
> > resumed. Naturally, URBs can complete as soon as the physical resume is
> > done, and completion handlers may try to resubmit -- but they will fail
> > until the state is set back. Clearly the state needs to be changed
> > _before_ the resume is done.
>
> Sounds right, but I'll look at that again. There were (are!!) some
> funky interactions with root hubs; they've been getting less funky
> over time, and are soon (I hope!) to become even less funky.
Well, it was close but not quite right. I looked at the ohci_hub_suspend
and ohci_hub_resume routines; the problem seems to be that they set
hcd->state in the wrong place. For instance, the resume routine doesn't
set it back to HC_STATE_RUNNING until just before returning, but
port-change interrupts (which will trigger the completion and resubmission
of the status URB) can start up many milliseconds earlier.
In fact, it seems a little odd that the root-hub routines should affect
hcd->state. In the UHCI driver, only the HC routines change it. But of
course, that's a matter of personal programming style.
> > I don't understand the second problem. Why would you expect wakeup to
> > work when CONFIG_USB_SUSPEND isn't set? Why should it have to work at
> > all in that case?
>
> The second case assumed USB_SUSPEND, otherwise there'd be no wakeup!
> Sorry for the confusion. :)
Maybe fixing the first problem will cure the second problem too.
> > Maybe a better fix for this problem would be to make the non-USB_SUSPEND
> > version of usb_suspend_device more than just an empty placeholder.
>
> It's not empty ... it has a "return 0"! And a comment!!
By George, you're right about the "return 0"! But I don't see any
comments in the routine, although there is a comment on the "#else" line.
> Which is part of why your small patch above bothers me: it's NOP unless
> the USB_SUSPEND stuff is present. So the two paths would still act
> different, and HCDs would still need special casing.
>
>
> > If the
> > device is a root hub, do perform the hcd->hub_suspend call. And likewise
> > for usb_resume_device, of course. Then all that special indirect upstream
> > stuff could be removed from the HCDs.
>
> We do want to get rid of that special casing in the HCDs, yes. The sooner
> the better; it was expedient at the time (when hub_suspend was young, just
> last year!, and few HCDs supported it) but now it's just confusing.
If we make the second change (non-USB_SUSPEND version of
usb_suspend_device) then the first change (usb_generic_suspend) will no
longer be a NOP. Making _both_ changes will not only allow removal of the
special casing from the HCDs, it also means that the oddball
parallel-execution paths are localized to one spot, clearly marked by
#ifdef. Furthermore it puts the decision about how to handle a FREEZE
where it logically belongs, at the interface between usbcore and the PM
core. Above that point we have all the pm_state weirdness, while below it
there's nothing but USB suspend or resume.
Alan Stern
-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
