[PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path

[PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

clk_pm_runtime_get() assumes that the PM-runtime usage counter will
be dropped by pm_runtime_get_sync() on errors, which is not the case,
so PM-runtime references to devices acquired by the former are leaked
on errors returned by the latter.

Fix this by modifying clk_pm_runtime_get() to drop the reference if
pm_runtime_get_sync() returns an error.

Fixes: 9a34b45397e5 clk: Add support for runtime PM
Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/clk/clk.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/clk/clk.c
===================================================================
--- linux-pm.orig/drivers/clk/clk.c
+++ linux-pm/drivers/clk/clk.c
@@ -114,7 +114,11 @@ static int clk_pm_runtime_get(struct clk
 		return 0;
 
 	ret = pm_runtime_get_sync(core->dev);
-	return ret < 0 ? ret : 0;
+	if (ret < 0) {
+		pm_runtime_put_noidle(core->dev);
+		return ret;
+	}
+	return 0;
 }
 
 static void clk_pm_runtime_put(struct clk_core *core)

Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path

[Marek Szyprowski]

Hi Rafael,

On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> clk_pm_runtime_get() assumes that the PM-runtime usage counter will
> be dropped by pm_runtime_get_sync() on errors, which is not the case,
> so PM-runtime references to devices acquired by the former are leaked
> on errors returned by the latter.
>
> Fix this by modifying clk_pm_runtime_get() to drop the reference if
> pm_runtime_get_sync() returns an error.
>
> Fixes: 9a34b45397e5 clk: Add support for runtime PM
> Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Frankly, I would rather fix the runtime_get_sync() instead of fixing the 
return path everywhere in the kernel. The current behavior of the 
pm_runtime_get_sync() is completely counter-intuitive then. I bet that 
in the 99% of the places where it is being called assume that no special 
fixup is needed in case of failure. This is one of the most common 
runtime PM related function and it is really a common pattern in the 
drivers to call:

pm_runtime_get_sync()

do something with the hardware

pm_runtime_put()

Do you really want to fix the error paths of the all such calls?


> ---
>   drivers/clk/clk.c |    6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/clk/clk.c
> ===================================================================
> --- linux-pm.orig/drivers/clk/clk.c
> +++ linux-pm/drivers/clk/clk.c
> @@ -114,7 +114,11 @@ static int clk_pm_runtime_get(struct clk
>   		return 0;
>   
>   	ret = pm_runtime_get_sync(core->dev);
> -	return ret < 0 ? ret : 0;
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(core->dev);
> +		return ret;
> +	}
> +	return 0;
>   }
>   
>   static void clk_pm_runtime_put(struct clk_core *core)
>
>
>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path

[Rafael J. Wysocki]

On Fri, May 22, 2020 at 7:19 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rafael,
>
> On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > clk_pm_runtime_get() assumes that the PM-runtime usage counter will
> > be dropped by pm_runtime_get_sync() on errors, which is not the case,
> > so PM-runtime references to devices acquired by the former are leaked
> > on errors returned by the latter.
> >
> > Fix this by modifying clk_pm_runtime_get() to drop the reference if
> > pm_runtime_get_sync() returns an error.
> >
> > Fixes: 9a34b45397e5 clk: Add support for runtime PM
> > Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Frankly, I would rather fix the runtime_get_sync() instead of fixing the
> return path everywhere in the kernel. The current behavior of the
> pm_runtime_get_sync() is completely counter-intuitive then. I bet that
> in the 99% of the places where it is being called assume that no special
> fixup is needed in case of failure. This is one of the most common
> runtime PM related function and it is really a common pattern in the
> drivers to call:
>
> pm_runtime_get_sync()
>
> do something with the hardware
>
> pm_runtime_put()
>
> Do you really want to fix the error paths of the all such calls?

No, I don't, and that's why I'm proposing this patch.

The caller that does what you said above is OK now and if the behavior
of pm_runtime_get_sync() changed, that caller would need to be
updated.

OTOH, a caller that fails to drop the reference on an error returned
by pm_runtime_get_sync() is buggy (and has ever been so).

I'd rather update the buggy callers than the ones that are OK.

Thanks!

>
>
> > ---
> >   drivers/clk/clk.c |    6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/clk/clk.c
> > ===================================================================
> > --- linux-pm.orig/drivers/clk/clk.c
> > +++ linux-pm/drivers/clk/clk.c
> > @@ -114,7 +114,11 @@ static int clk_pm_runtime_get(struct clk
> >               return 0;
> >
> >       ret = pm_runtime_get_sync(core->dev);
> > -     return ret < 0 ? ret : 0;
> > +     if (ret < 0) {
> > +             pm_runtime_put_noidle(core->dev);
> > +             return ret;
> > +     }
> > +     return 0;
> >   }
> >
> >   static void clk_pm_runtime_put(struct clk_core *core)
> >
> >
> >
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path

[Ulf Hansson]

On Fri, 22 May 2020 at 20:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, May 22, 2020 at 7:19 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> >
> > Hi Rafael,
> >
> > On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > clk_pm_runtime_get() assumes that the PM-runtime usage counter will
> > > be dropped by pm_runtime_get_sync() on errors, which is not the case,
> > > so PM-runtime references to devices acquired by the former are leaked
> > > on errors returned by the latter.
> > >
> > > Fix this by modifying clk_pm_runtime_get() to drop the reference if
> > > pm_runtime_get_sync() returns an error.
> > >
> > > Fixes: 9a34b45397e5 clk: Add support for runtime PM
> > > Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Frankly, I would rather fix the runtime_get_sync() instead of fixing the
> > return path everywhere in the kernel. The current behavior of the
> > pm_runtime_get_sync() is completely counter-intuitive then. I bet that
> > in the 99% of the places where it is being called assume that no special
> > fixup is needed in case of failure. This is one of the most common
> > runtime PM related function and it is really a common pattern in the
> > drivers to call:
> >
> > pm_runtime_get_sync()
> >
> > do something with the hardware
> >
> > pm_runtime_put()
> >
> > Do you really want to fix the error paths of the all such calls?
>
> No, I don't, and that's why I'm proposing this patch.
>
> The caller that does what you said above is OK now and if the behavior
> of pm_runtime_get_sync() changed, that caller would need to be
> updated.
>
> OTOH, a caller that fails to drop the reference on an error returned
> by pm_runtime_get_sync() is buggy (and has ever been so).
>
> I'd rather update the buggy callers than the ones that are OK.

I agree.

In hindsight we should have dropped the usage count in
pm_runtime_get_sync(), when it fails. However, that's too late,
especially since there are many cases having no error handling at all
- and in those cases, that would mean the subsequent call to
pm_runtime_put() can mess up the usage count (if pm_runtime_get_sync()
failed and has already dropped the count).

So, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

[...]

Kind regards
Uffe

Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path

[Rafael J. Wysocki]

On Mon, May 25, 2020 at 11:31 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 22 May 2020 at 20:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, May 22, 2020 at 7:19 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > clk_pm_runtime_get() assumes that the PM-runtime usage counter will
> > > > be dropped by pm_runtime_get_sync() on errors, which is not the case,
> > > > so PM-runtime references to devices acquired by the former are leaked
> > > > on errors returned by the latter.
> > > >
> > > > Fix this by modifying clk_pm_runtime_get() to drop the reference if
> > > > pm_runtime_get_sync() returns an error.
> > > >
> > > > Fixes: 9a34b45397e5 clk: Add support for runtime PM
> > > > Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Frankly, I would rather fix the runtime_get_sync() instead of fixing the
> > > return path everywhere in the kernel. The current behavior of the
> > > pm_runtime_get_sync() is completely counter-intuitive then. I bet that
> > > in the 99% of the places where it is being called assume that no special
> > > fixup is needed in case of failure. This is one of the most common
> > > runtime PM related function and it is really a common pattern in the
> > > drivers to call:
> > >
> > > pm_runtime_get_sync()
> > >
> > > do something with the hardware
> > >
> > > pm_runtime_put()
> > >
> > > Do you really want to fix the error paths of the all such calls?
> >
> > No, I don't, and that's why I'm proposing this patch.
> >
> > The caller that does what you said above is OK now and if the behavior
> > of pm_runtime_get_sync() changed, that caller would need to be
> > updated.
> >
> > OTOH, a caller that fails to drop the reference on an error returned
> > by pm_runtime_get_sync() is buggy (and has ever been so).
> >
> > I'd rather update the buggy callers than the ones that are OK.
>
> I agree.
>
> In hindsight we should have dropped the usage count in
> pm_runtime_get_sync(), when it fails. However, that's too late,
> especially since there are many cases having no error handling at all
> - and in those cases, that would mean the subsequent call to
> pm_runtime_put() can mess up the usage count (if pm_runtime_get_sync()
> failed and has already dropped the count).
>
> So, feel free to add:
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks!

Given the lack of other comments, I'm applying this patch as 5.8 material.