| Message ID | 6fb49df1ba7c80c2e6c51fb95322e025243ce36c.1619621413.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Address some issues with PM runtime at media subsystem | expand |
On Wed, 28 Apr 2021 16:51:32 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > The pm_runtime_get_sync() internally increments the > dev->power.usage_count without decrementing it, even on errors. > Replace it by the new pm_runtime_resume_and_get(), introduced by: > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") > in order to properly decrement the usage counter and avoid memory > leaks. > > Also, right now, the driver is ignoring any troubles when > trying to do PM resume. So, add the proper error handling > for the code. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Refactor to move the error handling block to one place perhaps? Also, I thin more cleanup is needed int he > --- > drivers/media/platform/rcar_fdp1.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c > index 01c1fbb97bf6..c32d237af618 100644 > --- a/drivers/media/platform/rcar_fdp1.c > +++ b/drivers/media/platform/rcar_fdp1.c > @@ -2140,7 +2140,13 @@ static int fdp1_open(struct file *file) > } > > /* Perform any power management required */ > - pm_runtime_get_sync(fdp1->dev); > + ret = pm_runtime_resume_and_get(fdp1->dev); > + if (ret < 0) { > + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx); Next few lines repeated enough times that I'd suggest just pulling error handling out to it's own path. > + v4l2_ctrl_handler_free(&ctx->hdl); > + kfree(ctx); > + goto done; > + } > > v4l2_fh_add(&ctx->fh); > > @@ -2351,7 +2357,9 @@ static int fdp1_probe(struct platform_device *pdev) > > /* Power up the cells to read HW */ > pm_runtime_enable(&pdev->dev); > - pm_runtime_get_sync(fdp1->dev); > + ret = pm_runtime_resume_and_get(fdp1->dev); From a balance point of view, do you want to all pm_runtime_disable() if this happens and also I'd guess you need to unregister the device as done in other error paths in probe The lack of balance between ordering in probe and remove before your patch bothers me but I don't know the code well enough to tell if there are any actual bugs there. > + if (ret < 0) > + return ret; > > hw_version = fdp1_read(fdp1, FD1_IP_INTDATA); > switch (hw_version) {
diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c index 01c1fbb97bf6..c32d237af618 100644 --- a/drivers/media/platform/rcar_fdp1.c +++ b/drivers/media/platform/rcar_fdp1.c @@ -2140,7 +2140,13 @@ static int fdp1_open(struct file *file) } /* Perform any power management required */ - pm_runtime_get_sync(fdp1->dev); + ret = pm_runtime_resume_and_get(fdp1->dev); + if (ret < 0) { + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx); + v4l2_ctrl_handler_free(&ctx->hdl); + kfree(ctx); + goto done; + } v4l2_fh_add(&ctx->fh); @@ -2351,7 +2357,9 @@ static int fdp1_probe(struct platform_device *pdev) /* Power up the cells to read HW */ pm_runtime_enable(&pdev->dev); - pm_runtime_get_sync(fdp1->dev); + ret = pm_runtime_resume_and_get(fdp1->dev); + if (ret < 0) + return ret; hw_version = fdp1_read(fdp1, FD1_IP_INTDATA); switch (hw_version) {
The pm_runtime_get_sync() internally increments the dev->power.usage_count without decrementing it, even on errors. Replace it by the new pm_runtime_resume_and_get(), introduced by: commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") in order to properly decrement the usage counter and avoid memory leaks. Also, right now, the driver is ignoring any troubles when trying to do PM resume. So, add the proper error handling for the code. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/platform/rcar_fdp1.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)