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
