linux-kernel - Re: [PATCH 69/78] media: rcar-vin: use pm_runtime_resume_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210426153311.150f5b4f@coco.lan>
Date:   Mon, 26 Apr 2021 15:33:11 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Linuxarm <linuxarm@...wei.com>, mauro.chehab@...wei.com,
        Niklas Söderlund 
        <niklas.soderlund@...natech.se>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 69/78] media: rcar-vin: use pm_runtime_resume_and_get()

Em Sat, 24 Apr 2021 11:12:31 +0200
Geert Uytterhoeven <geert@...ux-m68k.org> escreveu:

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