linux-kernel - Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm

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]

Message-ID: <20140131164534.GL20736@saruman.home>
Date:	Fri, 31 Jan 2014 10:45:34 -0600
From:	Felipe Balbi <balbi@...com>
To:	Santosh Shilimkar <santosh.shilimkar@...com>
CC:	<balbi@...com>, Grygorii Strashko <grygorii.strashko@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<ivan.khoronzhuk@...com>, <linux-usb@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm

On Fri, Jan 31, 2014 at 10:50:40AM -0500, Santosh Shilimkar wrote:
> On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote:
> > On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote:
> >> On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:
> >>> Hi,
> >>>
> >>> On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:
> >>>> The Keystone PM management layer has been implemented using PM bus for
> >>>> power management clocks. As result, most of Keystone drivers don't need
> >>>> to manage clocks directly. They just need to enable runtime PM and use it
> >>>> to handle their PM state and clocks.
> >>>>
> >>>> Hence, remove clock management code and switch to use runtime PM.
> >>>>
> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
> >>>
> >>> quite a few weeks back I sent a series enabling runtime pm for all glue
> >>> layers. I'll use that version instead, sorry.
> >>>
> >> That should be fine but you need to drop clk_*() related code
> >> from that patch. I assume you will send refresh version of it then.
> > 
> > why ? it makes no difference if you enable twice and disable twice.
> > 
> Sure but why do you want to have the clock node handling code in drivers
> if it is not needed. Isn't that better ?

It might, but then way that I wanted to see it is so that I can make
assumptions about the device state. From a driver perspective, what I
would love to see is the ability to assume that when my probe gets
called the device is already enabled. So if you can make sure that
clk_enable() happens before my probe and that you call
pm_runtime_set_active() before my probe too, then I can more than
hapilly remove clk_* calls from the driver ;-)

either that or maintain the driver like so:

probe()
{
	...

	clk_get(dev, "fck");
	clk_prepare(clk);
	clk_enable(clk);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	pm_runtime_get_sync(dev);
	...
}

runtime_suspend()
{
	clk_disable(dev);
}

runtime_resume()
{
	clk_enable(dev);
}

note that because of pm_runtime_set_active() that first
pm_runtime_get_sync() in probe() will simply increase the reference
counter without calling my ->runtime_resume() callback, which is exactly
what we want, as that would completely avoid situations of bad context
being restored because of that initial pm_runtime_get_sync().

Then, we can even make pm_runtime completely async easily, because
clk_prepare() was called only on probe() (or before it, for that
matter).

Bottomline is, if you can guarantee me that clk_get(), clk_prepare(),
clk_enable() and pm_runtime_set_active() will be called properly before
my probe, i'll be more than happy to comply with your request above as
that will greatly simplify my driver.

Just make, also, that if this clock is shared between dwc3-keystone
wrapper and dwc3 core, you clk_get() on both driver's probe.

cheers

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)