linux-kernel - Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put

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:	Wed, 11 Nov 2015 01:11:33 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Eduardo Valentin <edubezval@...il.com>
Cc:	Ulf Hansson <ulf.hansson@...aro.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
	Zhang Rui <rui.zhang@...el.com>,
	Linux-SH <linux-sh@...r.kernel.org>,
	Linux-Kernel <linux-kernel@...r.kernel.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Cao Minh Hiep <cm-hiep@...so.co.jp>,
	Dung：人ソ <nv-dung@...so.co.jp>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync()

On Tuesday, November 10, 2015 10:30:51 AM Eduardo Valentin wrote:
> Hi,
> 
> On Tue, Nov 10, 2015 at 02:00:38PM +0100, Ulf Hansson wrote:
> > +Rafael, Alan
> > 
> > On 10 November 2015 at 11:10, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > Hi Ulf,
> > >
> > > On Tue, Nov 10, 2015 at 10:57 AM, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > >> On 10 November 2015 at 09:18, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > >>> On Tue, Nov 10, 2015 at 3:12 AM, Kuninori Morimoto
> > >>> <kuninori.morimoto.gx@...esas.com> wrote:
> > >>>> From: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
> > >>>>
> > >>>> It is using pm_runtime_get_sync() on probe(). Let's use
> > >>>> pm_runtime_put_sync() instead of pm_runtime_put(). Otherwise thermal
> > >>>> sensor doesn't work after unbind/re-bind
> > >>>>
> > >>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
> > >>>> ---
> > >>>>  drivers/thermal/rcar_thermal.c | 2 +-
> > >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> > >>>> index 13d01ed..f7cf2d7 100644
> > >>>> --- a/drivers/thermal/rcar_thermal.c
> > >>>> +++ b/drivers/thermal/rcar_thermal.c
> > >>>> @@ -373,7 +373,7 @@ static int rcar_thermal_remove(struct platform_device *pdev)
> > >>>>                 thermal_zone_device_unregister(priv->zone);
> > >>>>         }
> > >>>>
> > >>>> -       pm_runtime_put(dev);
> > >>>> +       pm_runtime_put_sync(dev);
> > >>>>         pm_runtime_disable(dev);
> > >>
> > >> For the reasons explained by Geert, this is to me also a "workaround".
> > >>
> > >> I would replace pm_runtime_put() and pm_runtime_disable() with a call
> > >> to pm_runtime_force_suspend().
> > >>
> > >> In that way, you will make sure you device get runtime suspended
> > >> (clock domain will gate the clock). Additionally, the runtime PM
> > >> status will properly reflect the status of the device.
> > >
> > > That still sounds like a workaround to me, which we have to apply to all
> > > drivers relying on Runtime PM?
> > 
> > Definitely not all drivers, but those that runs pm_runtime_get_sync()
> > during ->probe() and expects the ->runtime_resume() callback to always
> > be invoked because of that. I guess we need to check upon which
> > drivers that may suffer from this.

Generally, calling pm_runtime_get_sync() in ->probe() and expecting the
driver's ->runtime_resume() to be always be invoked is a mistake.  I know
nothing about any guarantees that this will always happen.

If you want your ->runtime_resume() to be invoked no matter what, you really
need to figure out what the current state of things is, change it to your
expectations with runtime PM disabled and enable runtime PM after that. 

Still, that also needs to be done with care as the bus type/PM domain may be
affected by it.

> > 
> > I wouldn't be surprised if at least a subset of those cases we find,
> > are poorly designed from PM point of view and won't even probe
> > successfully unless CONFIG_PM is set. Whatever that means...
> 
> 
> Yeah, if it is the case this is a bug in runtime pm core, I would prefer
> this to be properly fixed, and not only this driver benefits of it.
> 
> Rafael? Any thoughts?

First off, it's not a bug in the runtime PM core, as that code is agnostic
to what should or should not happen to devices during ->probe, ->remove etc.

Second, as I said above (and elsewhere), the driver is just a piece of the
puzzle in many cases.

Thanks,
Rafael

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