Re: [PATCH v5 08/10] drm/xe/pm: clear PME HPD flag on runtime suspend error handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing rollback of suspended GTs and evicted BOs in PM suspend error paths.
--

commit 05e729a00eec5c74e7d63026c111b70a9b537da1
Author: Vinod Govindapillai <vinod.govindapillai@intel.com>

drm/xe/pm: clear PME HPD flag on runtime suspend error handler

This commit explicitly clears the PME HPD flag in the error path of
xe_pm_runtime_suspend(). The flag is normally set during early runtime
suspend and cleared during early runtime resume, but was previously left
uncleared if an error occurred in the suspend sequence.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index c79f55b66a419..5e7263316683a 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -663,6 +663,13 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>  	return 0;
>  
>  out_resume:
> +	/*
> +	 * We need to explicitly clear the PME HPD flag in this error handler.
> +	 * Normally this is set during early runtime suspend and cleared during
> +	 * early runtime resume call.
> +	 */
> +	xe_display_clear_pme_hpd(xe);
> +
>  	xe_display_pm_runtime_resume(xe);
>  	xe_pxp_pm_resume(xe->pxp);

[Severity: High]
This isn't a bug introduced by this patch, but on this error path, do we
leave previously suspended GTs in a suspended state?

If xe_gt_suspend() or xe_gt_runtime_suspend() fails during the for_each_gt()
iteration in xe_pm_runtime_suspend(), we jump to out_resume. It appears we
do not resume the GTs that were already successfully suspended in earlier
loop iterations, nor do we restore the BOs that were evicted before the loop:

    for_each_gt(gt, xe, id) {
        err = xe->d3cold.allowed ? xe_gt_suspend(gt) : xe_gt_runtime_suspend(gt);
        if (err)
            goto out_resume;
    }

A similar pattern seems to exist in the system suspend path in xe_pm_suspend():

    for_each_gt(gt, xe, id) {
        err = xe_gt_suspend(gt);
        if (err)
            goto err_display;
    }

Can this leave the device in a corrupted state where some components are
suspended while the PM core considers the device active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911080448.778316-1-vinod.govindapillai@intel.com?part=8