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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 2 Mar 2013 17:02:13 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Felipe Balbi <balbi@...com>
cc:	Vivek Gautam <gautam.vivek@...sung.com>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-omap@...r.kernel.org>, <linux-samsung-soc@...r.kernel.org>,
	<gregkh@...uxfoundation.org>, <sarah.a.sharp@...ux.intel.com>,
	<kgene.kim@...sung.com>, <kishon@...com>,
	Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

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.

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.

> 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.

> 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.

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.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/