Hi, On Sat, Mar 02, 2013 at 05:02:13PM -0500, Alan Stern wrote: > On Sat, 2 Mar 2013, Felipe Balbi wrote: > > > > > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev) > > > > struct usb_hcd *hcd = platform_get_drvdata(dev); > > > > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > > > > > > > + if (!pm_runtime_suspended(&dev->dev)) > > > > + pm_runtime_put(&dev->dev); > > > > + pm_runtime_disable(&dev->dev); > > > > + > > > > usb_remove_hcd(xhci->shared_hcd); > > > > usb_put_hcd(xhci->shared_hcd); > > > > > > This is very strange. Why have a pm_runtime_put with no balancing > > > pm_runtime_get? > > > > this is good point and, in fact, a doubt I have myself. How are we > > supposed to check if device is suspended ? In case it _is_ suspended we > > might not be able to read device's registers due to clocks possibly > > being gated. > > That's really a separate question. It has a simple answer, though: If > you want to access a device's registers, call pm_runtime_get_sync() > beforehand and pm_runtime_put() (or _put_sync()) afterward. Then it > won't matter if the device was suspended originally. that's alright, but how do you want me to check if my device is enabled or not before pm_runtime_enable() ? > If you actually do want to tell whether or not a device is suspended > and nothing more, call pm_runtime_status_suspended(). Of course, this > is racy -- the power state might change right after you make the call. right. > > Also, considering that some drivers are used in multiple platforms and > > those might behave differently when it comes to clock handling, how do > > we do that ? Should we require drivers to explicitly clk_get(); > > clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ? > > I don't know much about clock handling. In general, the > pm_runtime_set_active() and pm_runtime_enable() parts should be done by > the subsystem, not the driver, whenever possible. good to know :-) Though I disagree with calling pm_runtime_enable() at the subsystem level. This means we can add pm_runtime_set_active() > > While that's doable, I don't see how that'd be doable for OMAP since > > they want to hide clock handling from drivers. > > > > Any tips ? > > Whichever piece of code is responsible for associating a clock with a > device should also be responsible for making sure that neither is > suspended when the driver's probe routine runs. I'm not sure how > different platforms do this; using a PM domain could be one answer. that's alright, but it generates a different set of problems. That same piece of code which associates a device with its clock, doesn't really know if the driver is even available which means we could be enabling clocks for no reason and just wasting precious battery juice ;-) > All this is somewhat off the point of my original comment, however. > Drivers must be sure to balance their pm_runtime_get() and _put() > calls. Having an unbalanced _put() in the remove routine is almost > certainly a mistake -- especially if it is conditional on the device's > power state, because a device can remain unsuspended even after the > driver does a pm_runtime_put(). For example, this will happen if the > user wrote "on" to /sys/.../power/control. indeed... Makes sense. I'll consider mailing linux-pm for the rest of the discussion, cheers. -- balbi
Attachment:
signature.asc
Description: Digital signature
