Messages in this thread | | | From | Ulf Hansson <> | Date | Mon, 25 May 2020 11:30:48 +0200 | Subject | Re: [PATCH] PM: runtime: clk: Fix clk_pm_runtime_get() error path |
| |
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
| |