Re: [PATCH] drm/msm/a6xx: check pm_runtime_resume_and_get() during resume

[Rob Clark]

On Tue, Sep 29, 2026 at 2:29 AM Roman <roman.demidov.nn@xxxxxxxxx> wrote:
>
> Hi Rob,
>
> Thanks. I did consider guard/cleanup here.
>
> A plain guard(mutex)(&a6xx_gpu->gmu.lock) would remove the manual
> unlock/err_unlock, but it would also keep gmu.lock held until the
> function returns, i.e. across msm_devfreq_resume() and
> a6xx_llc_activate(). The current code drops the lock before those calls,
> and I didn't want to change that as part of this fix.
>
> scoped_guard() can preserve the original lock scope, but the error paths
> still need to unwind the PM refs and clear the OPP in the right order.
> Doing that with labels inside the scoped block and a success label
> outside ends up more convoluted than the current linear unwind. We'd
> still have goto, just with extra nesting/state.
>
> I also considered PM_RUNTIME_ACQUIRE(). The auto-cleanup for
> pm_runtime_put() is nice, but it doesn't remove the goto-based unwind
> for the clk_bulk_prepare_enable() failures. If the GPU clock enable or
> GMU clock enable fails, we still need to unwind the previously enabled
> clocks, clear the OPP, and drop the mutex. So we'd still need a goto.
>
> That said, if you'd prefer I switch the two pm_runtime_resume_and_get()
> calls to PM_RUNTIME_ACQUIRE() and keep the explicit clock/OPP unwind, I
> can send a v2. Or I can respin with guard(mutex) if holding the lock
> across those calls is acceptable.

Ahh, right, holding the lock across devfreq call would be bad.  So
disregard my suggestion, I think it turns into a bit more of a
refactor than what I had in mind.

BR,
-R

> BR,
> Roman
>
> сб, 12 сент. 2026 г. в 04:55, Rob Clark <rob.clark@xxxxxxxxxxxxxxxx>:
> >
> > On Fri, Sep 4, 2026 at 5:58 AM Roman Demidov <roman.demidov.nn@xxxxxxxxx> wrote:
> > >
> > > The return values of pm_runtime_resume_and_get() calls in a6xx_pm_resume()
> > > are not checked, which can lead to hardware access on suspended devices
> > > and PM reference underflows.
> > >
> > > Fix this by checking the return value of each pm_runtime_resume_and_get()
> > > call and properly unwinding the previously acquired resources on failure.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > >
> > > Fixes: 5a903a44a984 ("drm/msm/a6xx: Introduce GMU wrapper support")
> > > Signed-off-by: Roman Demidov <roman.demidov.nn@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 38 +++++++++++++++------------
> > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > index f9de9329dee3..0cf205ea744b 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > @@ -2175,44 +2175,48 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
> > >         opp = dev_pm_opp_find_freq_ceil(&gpu->pdev->dev, &freq);
> > >         if (IS_ERR(opp)) {
> > >                 ret = PTR_ERR(opp);
> > > -               goto err_set_opp;
> > > +               goto err_unlock;
> > >         }
> > >         dev_pm_opp_put(opp);
> > >
> > >         /* Set the core clock and bus bw, having VDD scaling in mind */
> > >         dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
> > >
> > > -       pm_runtime_resume_and_get(gmu->dev);
> > > -       pm_runtime_resume_and_get(gmu->gxpd);
> > > +       ret = pm_runtime_resume_and_get(gmu->dev);
> > > +       if (ret < 0)
> > > +               goto err_opp_clear;
> > > +       ret = pm_runtime_resume_and_get(gmu->gxpd);
> > > +       if (ret < 0)
> > > +               goto err_put_dev;
> > >
> > >         ret = clk_bulk_prepare_enable(gpu->nr_clocks, gpu->grp_clks);
> > >         if (ret)
> > > -               goto err_bulk_clk;
> > > +               goto err_put_gxpd;
> > >
> > >         ret = clk_bulk_prepare_enable(gmu->nr_clocks, gmu->clocks);
> > >         if (ret) {
> > >                 clk_bulk_disable_unprepare(gpu->nr_clocks, gpu->grp_clks);
> > > -               goto err_bulk_clk;
> > > +               goto err_put_gxpd;
> > >         }
> > >
> > >         if (adreno_is_a619_holi(adreno_gpu))
> > >                 a6xx_sptprac_enable(gmu);
> > >
> > > -       /* If anything goes south, tear the GPU down piece by piece.. */
> > > -       if (ret) {
> > > -err_bulk_clk:
> > > -               pm_runtime_put(gmu->gxpd);
> > > -               pm_runtime_put(gmu->dev);
> > > -               dev_pm_opp_set_opp(&gpu->pdev->dev, NULL);
> > > -       }
> > > -err_set_opp:
> > >         mutex_unlock(&a6xx_gpu->gmu.lock);
> > > +       msm_devfreq_resume(gpu);
> > > +       a6xx_llc_activate(a6xx_gpu);
> > >
> > > -       if (!ret) {
> > > -               msm_devfreq_resume(gpu);
> > > -               a6xx_llc_activate(a6xx_gpu);
> > > -       }
> > > +       return 0;
> > >
> > > +       /* If anything goes south, tear the GPU down piece by piece.. */
> > > +err_put_gxpd:
> > > +       pm_runtime_put(gmu->gxpd);
> > > +err_put_dev:
> > > +       pm_runtime_put(gmu->dev);
> > > +err_opp_clear:
> > > +       dev_pm_opp_set_opp(&gpu->pdev->dev, NULL);
> > > +err_unlock:
> > > +       mutex_unlock(&a6xx_gpu->gmu.lock);
> >
> > this does at least look a bit less terrifying than what came before...
> > but maybe
> >
> >    guard(mutex)(&a6xx_gpu->gmu.lock);
> >
> > to simplify the locking part of this.  And I think at least some of
> > the runpm stuff could also be handled w/ guard/cleanup stuff?
> >
> > BR,
> > -R
> >
> > >         return ret;
> > >  }
> > >
> > > --
> > > 2.53.0
> > >