On Sat, Jun 17, 2017 at 9:32 AM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> On Sat, Jun 17, 2017 at 9:00 AM, Zhi, Yong <yong.zhi@xxxxxxxxx> wrote:
>>> On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong.zhi@xxxxxxxxx> wrote:
>>> > + /* Set Power */
>>> > + r = pm_runtime_get_sync(dev);
>>> > + if (r < 0) {
>>> > + dev_err(dev, "failed to set imgu power\n");
>>>
>>> > + pm_runtime_put(dev);
>>>
>>> I'm not sure it's a right thing to do.
>>> How did you test runtime PM counters in this case?
>>>
>>> > + return r;
>>> > + }
>> Actually I have not tested the error case, what the right way to do in your opinion? there is no checking of this function return in lot of the driver code, or simply returning the error code, I also saw examples to call either pm_runtime_put() or pm_runtime_put_noidle() in this case.
>
> Instead of speculating, if we inspect pm_runtime_get_sync() [1], we
> can see that it always causes the runtime PM counter to increment, but
> it never decrements it, even in case of error. So to keep things
> balanced, you need to call pm_runtime_put() in error path.
>
> It shouldn't matter if it's pm_runtime_put() or
> pm_runtime_put_noidle(), because of runtime PM semantics, which are
> explicitly specified [2] that after an error, no hardware state change
> is attempted until the state is explicitly reset by the driver with
> either pm_runtime_set_active() or pm_runtime_set_suspended().
>
> So, as far as I didn't miss some even more obscure bits of the runtime
> PM framework, current code is fine.
Indeed. Thanks for explanation. PM runtime is hard :-)
Previously I didn't meet (and actually never used) check for returning
code of pm_runtime_get*().
> [1] http://elixir.free-electrons.com/linux/v4.11.6/source/include/linux/pm_runtime.h#L235
> and the main part:
> http://elixir.free-electrons.com/linux/v4.11.6/source/drivers/base/power/runtime.c#L1027
>
> [2] http://elixir.free-electrons.com/linux/v4.11.6/source/Documentation/power/runtime_pm.txt#L128
--
With Best Regards,
Andy Shevchenko
