> There are in fact two distinct but related problems here. One is the
> problem you mention, in which usbcore doesn't call the root-hub-suspend
> methods if CONFIG_USB_SUSPEND isn't set.
Or in fact, ANY hub suspend methods... to be fixed at some point. :)
> 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 ...".
> 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.
> > 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.
> 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. :)
> If we just got rid of CONFIG_USB_SUSPEND entirely, this wouldn't be an
> issue... :-)
That's coming eventually...
> 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!!
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.
- Dave
-------------------------------------------------------
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
