[PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()

[PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()

[Krzysztof Kozlowski]

pm_runtime_get_sync(), contradictory to intuition, does not drop the
runtime PM usage counter on errors which lead to several wrong usages in
drivers (missing the put).  pm_runtime_resume_and_get() was added as a
better implementation so document the preference of using it, hoping it
will stop bad patterns.

Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 Documentation/power/runtime_pm.rst | 4 +++-
 include/linux/pm_runtime.h         | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
index 18ae21bf7f92..478f08942811 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -378,7 +378,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
 
   `int pm_runtime_get_sync(struct device *dev);`
     - increment the device's usage counter, run pm_runtime_resume(dev) and
-      return its result
+      return its result;
+      be aware that it does not drop the device's usage counter on errors so
+      usage of pm_runtime_resume_and_get(dev) usually results in cleaner code
 
   `int pm_runtime_get_if_in_use(struct device *dev);`
     - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 6c08a085367b..0dfd23d2cfc3 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -380,6 +380,9 @@ static inline int pm_runtime_get(struct device *dev)
  * The possible return values of this function are the same as for
  * pm_runtime_resume() and the runtime PM usage counter of @dev remains
  * incremented in all cases, even if it returns an error code.
+ * Lack of decrementing the runtime PM usage counter on errors is a common
+ * mistake, so consider using pm_runtime_resume_and_get() instead for a cleaner
+ * code.
  */
 static inline int pm_runtime_get_sync(struct device *dev)
 {
-- 
2.25.1

Re: [PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()

[Rafael J. Wysocki]

On Thu, Apr 22, 2021 at 6:46 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> pm_runtime_get_sync(), contradictory to intuition, does not drop the
> runtime PM usage counter on errors which lead to several wrong usages in
> drivers (missing the put).  pm_runtime_resume_and_get() was added as a
> better implementation so document the preference of using it, hoping it
> will stop bad patterns.
>
> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  Documentation/power/runtime_pm.rst | 4 +++-
>  include/linux/pm_runtime.h         | 3 +++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> index 18ae21bf7f92..478f08942811 100644
> --- a/Documentation/power/runtime_pm.rst
> +++ b/Documentation/power/runtime_pm.rst
> @@ -378,7 +378,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
>
>    `int pm_runtime_get_sync(struct device *dev);`
>      - increment the device's usage counter, run pm_runtime_resume(dev) and
> -      return its result
> +      return its result;
> +      be aware that it does not drop the device's usage counter on errors so
> +      usage of pm_runtime_resume_and_get(dev) usually results in cleaner code

Whether or not it results in cleaner code depends on what that code does.

If the code is

pm_runtime_get_sync(dev);

<Do something that will fail if the device is in a low-power state,
but there is no way to handle the failure gracefully anyway>

pm_runtime_put(dev);

then having to use pm_runtime_resume_and_get() instead of the
pm_runtime_get_sync() would be a nuisance.

However, if the code wants to check the return value, that is:

error = pm_runtime_resume_and_get(dev);
if (error)
        return error;

<Do something that will crash and burn the system if the device is in
a low-power state>

pm_runtime_put(dev);

then obviously pm_runtime_resume_and_get() should be your choice.

The rule of thumb seems to be whether or not the return value is going
to be used.

>    `int pm_runtime_get_if_in_use(struct device *dev);`
>      - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 6c08a085367b..0dfd23d2cfc3 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -380,6 +380,9 @@ static inline int pm_runtime_get(struct device *dev)
>   * The possible return values of this function are the same as for
>   * pm_runtime_resume() and the runtime PM usage counter of @dev remains
>   * incremented in all cases, even if it returns an error code.
> + * Lack of decrementing the runtime PM usage counter on errors is a common
> + * mistake, so consider using pm_runtime_resume_and_get() instead for a cleaner
> + * code.
>   */
>  static inline int pm_runtime_get_sync(struct device *dev)
>  {
> --
> 2.25.1
>

Re: [PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()

[Krzysztof Kozlowski]

On 23/04/2021 16:08, Rafael J. Wysocki wrote:
> On Thu, Apr 22, 2021 at 6:46 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> pm_runtime_get_sync(), contradictory to intuition, does not drop the
>> runtime PM usage counter on errors which lead to several wrong usages in
>> drivers (missing the put).  pm_runtime_resume_and_get() was added as a
>> better implementation so document the preference of using it, hoping it
>> will stop bad patterns.
>>
>> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>> ---
>>  Documentation/power/runtime_pm.rst | 4 +++-
>>  include/linux/pm_runtime.h         | 3 +++
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
>> index 18ae21bf7f92..478f08942811 100644
>> --- a/Documentation/power/runtime_pm.rst
>> +++ b/Documentation/power/runtime_pm.rst
>> @@ -378,7 +378,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
>>
>>    `int pm_runtime_get_sync(struct device *dev);`
>>      - increment the device's usage counter, run pm_runtime_resume(dev) and
>> -      return its result
>> +      return its result;
>> +      be aware that it does not drop the device's usage counter on errors so
>> +      usage of pm_runtime_resume_and_get(dev) usually results in cleaner code
> 
> Whether or not it results in cleaner code depends on what that code does.
> 
> If the code is
> 
> pm_runtime_get_sync(dev);
> 
> <Do something that will fail if the device is in a low-power state,
> but there is no way to handle the failure gracefully anyway>
> 
> pm_runtime_put(dev);
> 
> then having to use pm_runtime_resume_and_get() instead of the
> pm_runtime_get_sync() would be a nuisance.
> 
> However, if the code wants to check the return value, that is:
> 
> error = pm_runtime_resume_and_get(dev);
> if (error)
>         return error;
> 
> <Do something that will crash and burn the system if the device is in
> a low-power state>
> 
> pm_runtime_put(dev);
> 
> then obviously pm_runtime_resume_and_get() should be your choice.
> 
> The rule of thumb seems to be whether or not the return value is going
> to be used.

Yes, you're right. What I wanted to point is that there is a pattern of
missing put when using pm_runtime_get_sync() all over the kernel. It's
quite common mistake because the interface is non-intuitive.

Therefore I find worth to warn users of the API: usually, for simple
cases, one should use the pm_runtime_resume_and_get(). This only a hint,
matching common cases, but not every case. I am not claiming that one is
better than other, just that old interface mislead in the past.

Maybe you wish to rephrase the comment to:
"be aware that it does not drop the device's usage counter on errors so
check if pm_runtime_resume_and_get(dev) would result in a cleaner code"


Best regards,
Krzysztof

Re: [PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

Hi!

> However, if the code wants to check the return value, that is:
> 
> error = pm_runtime_resume_and_get(dev);
> if (error)
>         return error;

Well, we mostly expect people to check error values, and the "continue
on error" case seems to be pretty unusual and mostly result of
oversight.

Quite large percentage of -stable patches is fixes after people got
confused with unusual interface...

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()

[Rafael J. Wysocki]

On Fri, Apr 23, 2021 at 5:03 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 23/04/2021 16:08, Rafael J. Wysocki wrote:
> > On Thu, Apr 22, 2021 at 6:46 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> pm_runtime_get_sync(), contradictory to intuition, does not drop the
> >> runtime PM usage counter on errors which lead to several wrong usages in
> >> drivers (missing the put).  pm_runtime_resume_and_get() was added as a
> >> better implementation so document the preference of using it, hoping it
> >> will stop bad patterns.
> >>
> >> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> >> ---
> >>  Documentation/power/runtime_pm.rst | 4 +++-
> >>  include/linux/pm_runtime.h         | 3 +++
> >>  2 files changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> >> index 18ae21bf7f92..478f08942811 100644
> >> --- a/Documentation/power/runtime_pm.rst
> >> +++ b/Documentation/power/runtime_pm.rst
> >> @@ -378,7 +378,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
> >>
> >>    `int pm_runtime_get_sync(struct device *dev);`
> >>      - increment the device's usage counter, run pm_runtime_resume(dev) and
> >> -      return its result
> >> +      return its result;
> >> +      be aware that it does not drop the device's usage counter on errors so
> >> +      usage of pm_runtime_resume_and_get(dev) usually results in cleaner code
> >
> > Whether or not it results in cleaner code depends on what that code does.
> >
> > If the code is
> >
> > pm_runtime_get_sync(dev);
> >
> > <Do something that will fail if the device is in a low-power state,
> > but there is no way to handle the failure gracefully anyway>
> >
> > pm_runtime_put(dev);
> >
> > then having to use pm_runtime_resume_and_get() instead of the
> > pm_runtime_get_sync() would be a nuisance.
> >
> > However, if the code wants to check the return value, that is:
> >
> > error = pm_runtime_resume_and_get(dev);
> > if (error)
> >         return error;
> >
> > <Do something that will crash and burn the system if the device is in
> > a low-power state>
> >
> > pm_runtime_put(dev);
> >
> > then obviously pm_runtime_resume_and_get() should be your choice.
> >
> > The rule of thumb seems to be whether or not the return value is going
> > to be used.
>
> Yes, you're right. What I wanted to point is that there is a pattern of
> missing put when using pm_runtime_get_sync() all over the kernel. It's
> quite common mistake because the interface is non-intuitive.
>
> Therefore I find worth to warn users of the API: usually, for simple
> cases, one should use the pm_runtime_resume_and_get(). This only a hint,
> matching common cases, but not every case. I am not claiming that one is
> better than other, just that old interface mislead in the past.
>
> Maybe you wish to rephrase the comment to:
> "be aware that it does not drop the device's usage counter on errors so
> check if pm_runtime_resume_and_get(dev) would result in a cleaner code"

I would say "so consider using pm_runtime_resume_and_get() instead of
it, especially if its return value is checked by the caller, as this
is likely to result in cleaner code."

IMO that should be sufficient to turn the reader's attention to the replacement.