| Message ID | 20210422164606.68231-1-krzysztof.kozlowski@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM: runtime: document common mistake with pm_runtime_get_sync() | expand |
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 >
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
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
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.
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) {
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(-)