Re: [PATCH 69/78] media: rcar-vin: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sat, 24 Apr 2021 11:12:31 +0200
Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> escreveu:

> Hi Mauro,
> 
> On Sat, Apr 24, 2021 at 8:46 AM Mauro Carvalho Chehab
> <mchehab+huawei@xxxxxxxxxx> 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@xxxxxxxxxx>  
> 
> 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