[lvc-project] [PATCH] drm/msm/a6xx: check pm_runtime_resume_and_get() during resume

[Roman]

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.

BR,
Roman

сб, 12 сент. 2026 г. в 04:55, Rob Clark <rob.clark at oss.qualcomm.com>:
>
> On Fri, Sep 4, 2026 at 5:58 AM Roman Demidov <roman.demidov.nn at gmail.com> 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 at gmail.com>
> > ---
> >  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
> >