Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1266368

Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync()

Show all headers | View raw

On 10 November 2015 at 09:18, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Morimoto-san, Ulf,
>
> On Tue, Nov 10, 2015 at 3:12 AM, Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.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@renesas.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.

>>
>>         return 0;
>
> While I can confirm this fixes the issue, I think this is a bug in the PM
> core, and thus your patch is merely a workaround.
>
> Morimoto-san: I assume this is a recent regression. Have you tried to bisect?
>
> With a bit more debugging info, this is the difference between the failing
> and the "fixed" cases:
>
>  unbind:
>
> +rcar_thermal e61f0000.thermal: pm_clk_suspend()
> +renesas-cpg-mssr e6150000.clock-controller: MSTP 522/thermal OFF
>  rcar_thermal e61f0000.thermal: removing from PM domain clock-controller
>  pm_genpd_remove_device: Remove e61f0000.thermal from clock-controller
> -renesas-cpg-mssr e6150000.clock-controller: MSTP 522/thermal OFF
>
>  bind:
>
>  rcar_thermal e61f0000.thermal: adding to PM domain clock-controller
>  __pm_genpd_add_device: Add e61f0000.thermal to clock-controller
>  rcar_thermal e61f0000.thermal: Clock thermal con_id (null) managed by
> runtime PM.
> -rcar_thermal e61f0000.thermal: thermal sensor was broken
> +rcar_thermal e61f0000.thermal: pm_clk_resume()
> +renesas-cpg-mssr e6150000.clock-controller: MSTP 522/thermal ON
>  rcar_thermal e61f0000.thermal: 1 sensor probed
>
> In the failing case, pm_clk_suspend() is not called, and turning off the
> module clock is thus delayed until removal of the device from the clock
> domain.
> But as pm_clk_suspend() wasn't called, the device isn't correctly resumed on
> rebind, and the module clock is never re-enabled, leading to a failure.
>
> Ulf, what do you think?

I totally agree on your analyse.

The problem is that the runtime PM status of the device isn't
correctly updated at ->remove(). The effect is that the the
pm_runtime_get_sync() in ->probe() at re-bind will *not* trigger the
->runtime_resume() callbacks to be invoked, as the runtime PM core
believes the device is already runtime resumed.

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/2] thermal: rcar_thermal: tidyup rebind issue Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> - 2015-11-10 03:20 +0100
  Re: [PATCH 0/2] thermal: rcar_thermal: tidyup rebind issue Nguyen Viet Dung <nv-dung@jinso.co.jp> - 2015-11-10 04:30 +0100
  Re: [PATCH 0/2] thermal: rcar_thermal: tidyup rebind issue Nguyen Viet Dung <nv-dung@jinso.co.jp> - 2015-11-10 06:20 +0100
  Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Geert Uytterhoeven <geert@linux-m68k.org> - 2015-11-10 09:20 +0100
    Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Ulf Hansson <ulf.hansson@linaro.org> - 2015-11-10 11:00 +0100
      Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Geert Uytterhoeven <geert@linux-m68k.org> - 2015-11-10 11:20 +0100
        Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Ulf Hansson <ulf.hansson@linaro.org> - 2015-11-10 14:10 +0100
          Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Eduardo Valentin <edubezval@gmail.com> - 2015-11-10 19:40 +0100
            Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-11-11 00:50 +0100
          Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-11-11 00:30 +0100
            Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Ulf Hansson <ulf.hansson@linaro.org> - 2015-11-11 12:10 +0100
              Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-11-12 01:40 +0100
                Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Ulf Hansson <ulf.hansson@linaro.org> - 2015-11-12 09:10 +0100
                Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Eduardo Valentin <edubezval@gmail.com> - 2015-11-12 19:50 +0100
                Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Ulf Hansson <ulf.hansson@linaro.org> - 2015-11-13 16:10 +0100
  Re: [PATCH 1/2] thermal: rcar_thermal: remove redundant operation Ulf Hansson <ulf.hansson@linaro.org> - 2015-11-13 16:00 +0100

csiph-web