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:	Tue, 10 Nov 2015 10:30:51 -0800
From:	Eduardo Valentin <edubezval@...il.com>
To:	Ulf Hansson <ulf.hansson@...aro.org>
Cc:	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>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync()

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

BR,

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