[02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
diff mbox series

Message ID 793a5806a63b6313606fd1c344b9eec41e61a440.1620207353.git.mchehab+huawei@kernel.org
State New, archived
Headers show
Series
  • Fix some PM runtime issues at the media subsystem
Related show

Commit Message

Mauro Carvalho Chehab May 5, 2021, 9:41 a.m. UTC
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 <rmfrfs@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron May 5, 2021, 11:06 a.m. UTC | #1
On Wed, 5 May 2021 11:41:52 +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, avoiding
> a potential PM usage counter leak.
> 
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

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;
Mauro Carvalho Chehab May 5, 2021, 11:17 a.m. UTC | #2
Em Wed, 5 May 2021 12:06:52 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Wed, 5 May 2021 11:41:52 +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, avoiding
> > a potential PM usage counter leak.
> > 
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> 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
Rui Miguel Silva May 5, 2021, 1:56 p.m. UTC | #3
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 <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, avoiding
> > a potential PM usage counter leak.
> > 
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
> 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;
Mauro Carvalho Chehab May 5, 2021, 2:23 p.m. UTC | #4
Em Wed, 05 May 2021 14:56:40 +0100
"Rui Miguel Silva" <rmfrfs@gmail.com> 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 <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, avoiding
> > > a potential PM usage counter leak.
> > > 
> > > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> >
> > 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

Patch
diff mbox series

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;