On Sat, Jun 17, 2017 at 9:32 AM, Tomasz Figa <tf...@chromium.org> wrote:
> On Sat, Jun 17, 2017 at 9:00 AM, Zhi, Yong <yong....@intel.com> wrote:
>>> On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong....@intel.com> 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
