Re: [PATCH v3 2/2] drm/xe: Poll GT for C6 before D3

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The system PM suspend abort path incorrectly uses runtime PM resume (`xe_pm_runtime_resume`), leaving the device hardware uninitialized, leaking PCI D3Cold configuration, and violating power management context separation rules.
--

commit 30b6702ddd1d3cfed0a5d047a525e3df63d91571
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

drm/xe: Poll GT for C6 before D3

Check if GTs are in C6 before transitioning to D3. PM subsystem can
retry if this is not the case. This ensures some component is not
accessing the GT when D3 state is forced. This check can be added
to the runtime idle check since it helps to check for pending
forcewakes as well before we poll for GT C6.

Cc: Badal Nilawar <badal.nilawar@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 278cfd390364c..e663cddcb9946 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -1326,7 +1326,7 @@ static int xe_pci_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct xe_device *xe = pdev_to_xe_device(pdev);
> -	int err;
> +	int err, ret;
>  
>  	if (xe_survivability_mode_is_boot_enabled(xe))
>  		return -EBUSY;
> @@ -1342,6 +1342,17 @@ static int xe_pci_suspend(struct device *dev)
>  	 */
>  	d3cold_toggle(pdev, D3COLD_ENABLE);
>  
> +	ret = xe_pm_check_runtime_idle(xe);
> +	if (ret) {
> +		err = xe_pm_runtime_resume(xe);

[Severity: High]
Does this error path leave the hardware in a broken, partially-suspended
state?

When xe_pm_check_runtime_idle() fails and aborts the system suspend, it calls
xe_pm_runtime_resume() to recover. However, xe_pm_runtime_resume() uses the
runtime PM flag xe->d3cold.allowed to conditionally skip critical
reinitialization steps:

drivers/gpu/drm/xe/xe_pm.c:xe_pm_runtime_resume() {
    ...
	if (xe->d3cold.allowed) {
		for_each_gt(gt, xe, id)
			xe_gt_idle_disable_c6(gt);
    ...
		err = xe_bo_restore_early(xe);
    ...
}

Because the device was fully suspended via xe_pm_suspend() earlier in
xe_pci_suspend(), bypassing these steps if xe->d3cold.allowed is false leaves
memory unrestored and the GT incorrectly suspended. Should system PM recovery
instead use system PM resume functions that do not rely on runtime PM flags?

> +		if (err) {
> +			drm_err(&xe->drm, "Resume failed after suspend was canceled");
> +			return err;
> +		}
> +
> +		return ret;

[Severity: High]
Could this exit path be leaving the device with incorrect D3Cold
configuration? 

The suspend function calls d3cold_toggle(pdev, D3COLD_ENABLE) just before the
idle check, but returns here without calling d3cold_toggle(pdev, D3COLD_DISABLE)
to revert the toggle when the suspend is aborted.

> +	}
> +
>  	pci_save_state(pdev);
>  	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, PCI_D3cold);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910215756.2393137-1-vinay.belgaumkar@intel.com?part=2