Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210422164606.68231-1-krzysztof.kozlowski@canonical.com>
 <CAJZ5v0iUQBfrTtVmfrrDixZnnr1_THgaM1+mFu4TRT+OOYb2mw@mail.gmail.com> <a56096c4-0b64-ca16-8d5c-d086e96a436d@canonical.com>
In-Reply-To: <a56096c4-0b64-ca16-8d5c-d086e96a436d@canonical.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 26 Apr 2021 13:45:47 +0200
Message-ID: <CAJZ5v0h2_4qjDqTS4Vm21e6CafqHZcjqj5p0Z8DWaAwUkkmtbA@mail.gmail.com>
Subject: Re: [PATCH] PM: runtime: document common mistake with pm_runtime_get_sync()
To:     Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.