[69/78] media: rcar-vin: use pm_runtime_resume_and_get()
diff mbox series

Message ID dc7f4bc45cd6be79d19d1a4027fb6f35dfb45a03.1619191723.git.mchehab+huawei@kernel.org
State New, archived
Headers show
Series
  • media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
Related show

Commit Message

Mauro Carvalho Chehab April 24, 2021, 6:45 a.m. UTC
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.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 2 +-
 drivers/media/platform/rcar-vin/rcar-dma.c  | 6 ++----
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++----
 3 files changed, 5 insertions(+), 9 deletions(-)

Comments

Niklas Söderlund April 24, 2021, 8:33 a.m. UTC | #1
Hi Mauro,

Thanks for your work.

On 2021-04-24 08:45:19 +0200, Mauro Carvalho Chehab wrote:
> 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.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 2 +-
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 6 ++----
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++----
>  3 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index e06cd512aba2..85574765a11a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -408,7 +408,7 @@ static void rcsi2_enter_standby(struct rcar_csi2 *priv)
>  
>  static void rcsi2_exit_standby(struct rcar_csi2 *priv)
>  {
> -	pm_runtime_get_sync(priv->dev);
> +	pm_runtime_resume_and_get(priv->dev);
>  	reset_control_deassert(priv->rstc);
>  }
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f30dafbdf61c..f5f722ab1d4e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1458,11 +1458,9 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>  	u32 vnmc;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(vin->dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(vin->dev);
> +	ret = pm_runtime_resume_and_get(vin->dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	/* Make register writes take effect immediately. */
>  	vnmc = rvin_read(vin, VNMC_REG);
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 457a65bf6b66..b1e9f86caa5c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -870,11 +870,9 @@ static int rvin_open(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(vin->dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(vin->dev);
> +	ret = pm_runtime_resume_and_get(vin->dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	ret = mutex_lock_interruptible(&vin->lock);
>  	if (ret)
> -- 
> 2.30.2
>
Geert Uytterhoeven April 24, 2021, 9:12 a.m. UTC | #2
Hi Mauro,

On Sat, Apr 24, 2021 at 8:46 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
> 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.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Thanks for your patch!

> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -408,7 +408,7 @@ static void rcsi2_enter_standby(struct rcar_csi2 *priv)
>
>  static void rcsi2_exit_standby(struct rcar_csi2 *priv)
>  {
> -       pm_runtime_get_sync(priv->dev);
> +       pm_runtime_resume_and_get(priv->dev);

I believe this part is incorrect: on failure[*], the refcount will now
be decremented, and in a subsequent call to rcsi2_enter_standby(), the
refcount will be decremented again due to the call to pm_runtime_put().

[*] On e.g. R-Car SoCs, this can never fail.  This is the reason why
    many R-Car (and SuperH) drivers never check the result of
    pm_runtime_get_sync().  So the refcount "imbalances" were actually
    introduced by the various "clean up" patches to add return value
    checking, which now need another round of fixing...

>         reset_control_deassert(priv->rstc);
>  }

> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1458,11 +1458,9 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>         u32 vnmc;
>         int ret;
>
> -       ret = pm_runtime_get_sync(vin->dev);
> -       if (ret < 0) {
> -               pm_runtime_put_noidle(vin->dev);
> +       ret = pm_runtime_resume_and_get(vin->dev);
> +       if (ret < 0)
>                 return ret;
> -       }

This change (and the change below) is correct, as the logic before/after
is equivalent.

Gr{oetje,eeting}s,

                        Geert
Mauro Carvalho Chehab April 26, 2021, 1:33 p.m. UTC | #3
Em Sat, 24 Apr 2021 11:12:31 +0200
Geert Uytterhoeven <geert@linux-m68k.org> escreveu:

> Hi Mauro,
> 
> On Sat, Apr 24, 2021 at 8:46 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> > 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.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> Thanks for your patch!
> 
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -408,7 +408,7 @@ static void rcsi2_enter_standby(struct rcar_csi2 *priv)
> >
> >  static void rcsi2_exit_standby(struct rcar_csi2 *priv)
> >  {
> > -       pm_runtime_get_sync(priv->dev);
> > +       pm_runtime_resume_and_get(priv->dev);  
> 
> I believe this part is incorrect: on failure[*], the refcount will now
> be decremented, and in a subsequent call to rcsi2_enter_standby(), the
> refcount will be decremented again due to the call to pm_runtime_put().

I see your point.

> [*] On e.g. R-Car SoCs, this can never fail.  This is the reason why
>     many R-Car (and SuperH) drivers never check the result of
>     pm_runtime_get_sync().  So the refcount "imbalances" were actually
>     introduced by the various "clean up" patches to add return value
>     checking, which now need another round of fixing...

It sounds dangerous to assume that. I'm not a PM expert, but, at least
looking at drivers/base/power/runtime.c, there are two situations where the
core itself will return an error, even if the PM callback never return
errors[1], and more could be added in the future:

        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

[1] see rpm_resume() function

IMO, the right thing here would be to check it at resume time,
and to handle it properly.

> 
> >         reset_control_deassert(priv->rstc);
> >  }  
> 
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -1458,11 +1458,9 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >         u32 vnmc;
> >         int ret;
> >
> > -       ret = pm_runtime_get_sync(vin->dev);
> > -       if (ret < 0) {
> > -               pm_runtime_put_noidle(vin->dev);
> > +       ret = pm_runtime_resume_and_get(vin->dev);
> > +       if (ret < 0)
> >                 return ret;
> > -       }  
> 
> This change (and the change below) is correct, as the logic before/after
> is equivalent.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 



Thanks,
Mauro

Patch
diff mbox series

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index e06cd512aba2..85574765a11a 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -408,7 +408,7 @@  static void rcsi2_enter_standby(struct rcar_csi2 *priv)
 
 static void rcsi2_exit_standby(struct rcar_csi2 *priv)
 {
-	pm_runtime_get_sync(priv->dev);
+	pm_runtime_resume_and_get(priv->dev);
 	reset_control_deassert(priv->rstc);
 }
 
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f30dafbdf61c..f5f722ab1d4e 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1458,11 +1458,9 @@  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 	u32 vnmc;
 	int ret;
 
-	ret = pm_runtime_get_sync(vin->dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(vin->dev);
+	ret = pm_runtime_resume_and_get(vin->dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	/* Make register writes take effect immediately. */
 	vnmc = rvin_read(vin, VNMC_REG);
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 457a65bf6b66..b1e9f86caa5c 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -870,11 +870,9 @@  static int rvin_open(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	ret = pm_runtime_get_sync(vin->dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(vin->dev);
+	ret = pm_runtime_resume_and_get(vin->dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = mutex_lock_interruptible(&vin->lock);
 	if (ret)