PM: runtime: clk: Fix clk_pm_runtime_get() error path
diff mbox series

Message ID 5127441.yGvM1JjtLk@kreacher
State Accepted
Commit 64c7d7ea22d86cacb65d0c097cc447bc0e6d8abd
Headers show
Series
  • PM: runtime: clk: Fix clk_pm_runtime_get() error path
Related show

Commit Message

Rafael J. Wysocki May 21, 2020, 5:08 p.m. UTC
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(-)

Comments

Marek Szyprowski May 22, 2020, 5:19 a.m. UTC | #1
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
Rafael J. Wysocki May 22, 2020, 6:39 p.m. UTC | #2
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
>
Ulf Hansson May 25, 2020, 9:30 a.m. UTC | #3
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
Rafael J. Wysocki May 26, 2020, 8:52 a.m. UTC | #4
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.

Patch
diff mbox series

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)