| Message ID | 5127441.yGvM1JjtLk@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | PM: runtime: clk: Fix clk_pm_runtime_get() error path | expand |
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
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 >
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
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.
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)