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, avoiding
a potential PM usage counter leak.
Acked-by: Rui Miguel Silva <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 025fdc488bd6..1dc680d94a46 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
mipi_csis_clear_counters(state);
- ret = pm_runtime_get_sync(&state->pdev->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(&state->pdev->dev);
+ ret = pm_runtime_resume_and_get(&state->pdev->dev);
+ if (ret < 0)
return ret;
- }
+
ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
if (ret < 0 && ret != -ENOIOCTLCMD)
goto done;
--
2.30.2
On Wed, 5 May 2021 11:41:52 +0200
Mauro Carvalho Chehab <[email protected]> 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, avoiding
> a potential PM usage counter leak.
>
> Acked-by: Rui Miguel Silva <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Not a fix as far as I can see, just a cleanup - so perhaps not this set?
Jonathan
> ---
> drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 025fdc488bd6..1dc680d94a46 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>
> mipi_csis_clear_counters(state);
>
> - ret = pm_runtime_get_sync(&state->pdev->dev);
> - if (ret < 0) {
> - pm_runtime_put_noidle(&state->pdev->dev);
> + ret = pm_runtime_resume_and_get(&state->pdev->dev);
> + if (ret < 0)
> return ret;
> - }
> +
> ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> if (ret < 0 && ret != -ENOIOCTLCMD)
> goto done;
Em Wed, 5 May 2021 12:06:52 +0100
Jonathan Cameron <[email protected]> escreveu:
> On Wed, 5 May 2021 11:41:52 +0200
> Mauro Carvalho Chehab <[email protected]> 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, avoiding
> > a potential PM usage counter leak.
> >
> > Acked-by: Rui Miguel Silva <[email protected]>
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> Not a fix as far as I can see, just a cleanup - so perhaps not this set?
Yeah, will move it to the non-error patch pile and change the comments.
>
> Jonathan
>
>
> > ---
> > drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 025fdc488bd6..1dc680d94a46 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
> >
> > mipi_csis_clear_counters(state);
> >
> > - ret = pm_runtime_get_sync(&state->pdev->dev);
> > - if (ret < 0) {
> > - pm_runtime_put_noidle(&state->pdev->dev);
> > + ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > + if (ret < 0)
> > return ret;
> > - }
> > +
> > ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> > if (ret < 0 && ret != -ENOIOCTLCMD)
> > goto done;
>
Thanks,
Mauro
Hi,
On Wed May 5, 2021 at 12:06 PM WEST, Jonathan Cameron wrote:
> On Wed, 5 May 2021 11:41:52 +0200
> Mauro Carvalho Chehab <[email protected]> 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, avoiding
> > a potential PM usage counter leak.
> >
> > Acked-by: Rui Miguel Silva <[email protected]>
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> Not a fix as far as I can see, just a cleanup - so perhaps not this set?
yes, the original changelog of this patch, that I acked, made it
clear it was a cleanup:
"
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.
Use the new API, in order to cleanup the error check logic.
"
This one above is new, but I saw Mauro is going change it.
------
Cheers,
Rui
>
> Jonathan
>
>
> > ---
> > drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 025fdc488bd6..1dc680d94a46 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
> >
> > mipi_csis_clear_counters(state);
> >
> > - ret = pm_runtime_get_sync(&state->pdev->dev);
> > - if (ret < 0) {
> > - pm_runtime_put_noidle(&state->pdev->dev);
> > + ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > + if (ret < 0)
> > return ret;
> > - }
> > +
> > ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> > if (ret < 0 && ret != -ENOIOCTLCMD)
> > goto done;
Em Wed, 05 May 2021 14:56:40 +0100
"Rui Miguel Silva" <[email protected]> escreveu:
> Hi,
> On Wed May 5, 2021 at 12:06 PM WEST, Jonathan Cameron wrote:
>
> > On Wed, 5 May 2021 11:41:52 +0200
> > Mauro Carvalho Chehab <[email protected]> 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, avoiding
> > > a potential PM usage counter leak.
> > >
> > > Acked-by: Rui Miguel Silva <[email protected]>
> > > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> >
> > Not a fix as far as I can see, just a cleanup - so perhaps not this set?
>
> yes, the original changelog of this patch, that I acked, made it
> clear it was a cleanup:
>
> "
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
> deal with usage counter")
> added pm_runtime_resume_and_get() in order to automatically handle
> dev->power.usage_count decrement on errors.
>
> Use the new API, in order to cleanup the error check logic.
> "
>
> This one above is new, but I saw Mauro is going change it.
Yes, I'll change the subject/description to the
"use pm_runtime_resume_and_get()" one on this patch, as there's
no issue to be fixed here, just a cleanup ;-)
Sorry for the mess. I did lots of rebase on ~80 patch series
over the last couple of days, based on the reviews (and my own
internal reviews)...
See, the current patchset has ~80 patches with ~30% contained
fixes. It shows that writing a balanced PM runtime code is not
so trivial ;-)
Thanks,
Mauro