Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path
@ 2020-05-21 17:08 ` Rafael J. Wysocki
  2020-05-22  5:19   ` Marek Szyprowski
  0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2020-05-21 17:08 UTC (permalink / raw)
  To: Linux PM
  Cc: LKML, Marek Szyprowski, Ulf Hansson, Krzysztof Kozlowski,
	Michael Turquette

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)




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path
  2020-05-21 17:08 ` [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path Rafael J. Wysocki
@ 2020-05-22  5:19   ` Marek Szyprowski
  2020-05-22 18:39     ` Rafael J. Wysocki
  0 siblings, 1 reply; 5+ messages in thread
From: Marek Szyprowski @ 2020-05-22  5:19 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux PM
  Cc: LKML, Ulf Hansson, Krzysztof Kozlowski, Michael Turquette,
	Bartlomiej Zolnierkiewicz

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path
  2020-05-22  5:19   ` Marek Szyprowski
@ 2020-05-22 18:39     ` Rafael J. Wysocki
  2020-05-25  9:30       ` Ulf Hansson
  0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2020-05-22 18:39 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Rafael J. Wysocki, Linux PM, LKML, Ulf Hansson,
	Krzysztof Kozlowski, Michael Turquette,
	Bartlomiej Zolnierkiewicz

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path
  2020-05-22 18:39     ` Rafael J. Wysocki
@ 2020-05-25  9:30       ` Ulf Hansson
  2020-05-26  8:52         ` Rafael J. Wysocki
  0 siblings, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2020-05-25  9:30 UTC (permalink / raw)
  To: Rafael J. Wysocki, Marek Szyprowski
  Cc: Rafael J. Wysocki, Linux PM, LKML, Krzysztof Kozlowski,
	Michael Turquette, Bartlomiej Zolnierkiewicz

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path
  2020-05-25  9:30       ` Ulf Hansson
@ 2020-05-26  8:52         ` Rafael J. Wysocki
  0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:52 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rafael J. Wysocki, Marek Szyprowski, Rafael J. Wysocki, Linux PM,
	LKML, Krzysztof Kozlowski, Michael Turquette,
	Bartlomiej Zolnierkiewicz

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200521170817eucas1p13d9477a0a5d13d2df876134cf41131d8@eucas1p1.samsung.com>
2020-05-21 17:08 ` [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path Rafael J. Wysocki
2020-05-22  5:19   ` Marek Szyprowski
2020-05-22 18:39     ` Rafael J. Wysocki
2020-05-25  9:30       ` Ulf Hansson
2020-05-26  8:52         ` Rafael J. Wysocki

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git