On Tue, Sep 29, 2026 at 2:29 AM Roman <[email protected]> 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 <[email protected]>: > > > > On Fri, Sep 4, 2026 at 5:58 AM Roman Demidov <[email protected]> > > 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 <[email protected]> > > > --- > > > 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 > > >
