From: Rafael J. Wysocki <[email protected]>
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+ <[email protected]> # 4.15+
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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)
Hi Rafael,
On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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+ <[email protected]> # 4.15+
> Signed-off-by: Rafael J. Wysocki <[email protected]>
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
On Fri, May 22, 2020 at 7:19 AM Marek Szyprowski
<[email protected]> wrote:
>
> Hi Rafael,
>
> On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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+ <[email protected]> # 4.15+
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> 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
>
On Fri, 22 May 2020 at 20:39, Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, May 22, 2020 at 7:19 AM Marek Szyprowski
> <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > 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+ <[email protected]> # 4.15+
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
[...]
Kind regards
Uffe
On Mon, May 25, 2020 at 11:31 AM Ulf Hansson <[email protected]> wrote:
>
> On Fri, 22 May 2020 at 20:39, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Fri, May 22, 2020 at 7:19 AM Marek Szyprowski
> > <[email protected]> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On 21.05.2020 19:08, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > 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+ <[email protected]> # 4.15+
> > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > >
> > > 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 <[email protected]>
Thanks!
Given the lack of other comments, I'm applying this patch as 5.8 material.