lkml.org 
[lkml]   [2021]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 69/78] media: rcar-vin: use pm_runtime_resume_and_get()
    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

    --
    Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

    In personal conversations with technical people, I call myself a hacker. But
    when I'm talking to journalists I just say "programmer" or something like that.
    -- Linus Torvalds

    \
     
     \ /
      Last update: 2021-04-24 11:14    [W:4.054 / U:0.272 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site