Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

[Felipe Balbi]

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