David:
A few things about your suspend/resume changes are unclear or seem wrong.
No big surprise. Call it alpha level code ... it works in the configs I've tested, but that's hardly enough. Plus this seems to need things the PM framework doesn't yet facilitate.
I'm still not happy with the notion that we can't rely on that PM framework to handle (or avoid) the suspend-children-first types of logic, except during whole-system suspend. Selective suspend needs to work sanely; not everything has as much energy to waste as current PC architectures do.
Instead of checking for USB_STATE_SUSPENDED in hub_irq() when
resubmitting the interrupt status URB, the URB should be unlinked during hub_suspend().
It would be a good place to use kill_urb(); but since that hasn't merged yet, I stuck to a simple approach. Doing that in hub_suspend() would sure make the root hub timers happier! The delayed work, blinking LEDS, should also get shut down there.
In usb_resume_device(), when resuming a root hub, I think you shouldn't call hub_resume() but instead call finish_resume(). After all, the hub driver may have been unbound from the root hub!
Is it otherwise safe to unbind hubs from the hub driver? That issue is worth looking at on its own. For now I'm assuming the answer is no.
That change would interact with the policy for using remote wakeup too ... in this case badly, since finish_resume() would disable it. It should probably be renamed ... finish_port_resume() is more like how I'd been thinking of its role, and that's the kind of context it's always called from.
There's no apparent mechanism for an HCD to indicate that the root
hub has received a remote wakeup request while in global suspend mode. How is this meant to be handled?
In global suspend mode, I seem to recall the only wakeup request is port connect/disconnect ... device remote wakeup is only turned on when a port is placed into selective suspend, and I recall you being keen that one distinction of global suspend is that individual ports were NOT selectively suspended.
I think all the HCDs can just get a port change detect IRQ somehow. That's what OHCI and EHCI should be doing now anyway. For things like keyboards waking hosts from sleep, there's also PME# on PCI. Non-PCI systems have their own solutions for restarting clocks and otherwise bringing the relevant system components back to life.
Your patch leaves two suspend/resume entries in struct hc_driver: one for suspending the entire controller and one for suspending just the
root hub. It's not clear under what more the first has to do besides
calling usb_suspend_device() for the root hub, which will then call the
second. Maybe for a simple PCI controller like UHCI it doesn't have to do
anything else?
Given that hcd-pci does the rest, that's likely true for UHCI. But my brief reading there didn't turn up info on how UHCI does things like "wakeup from D3" (or even learn that it can_wakeup), so it might need more.
I suspect that the hc_driver should only have the root hub entry. The rest of suspend/resume is more like bus glue, or even cpu-specific glue. Consider OMAP: PCI isn't normally a factor, and different chip versions may turn off the clocks using different registers. For now, this will affect OHCI the most ... all the OHCI bus glue will be able to use the same root hub suspend/resume logic.
Thanks for the comments!
- Dave
-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
