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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning -EAGAIN from xe_pci_runtime_suspend() without rolling back the successful xe_pm_runtime_suspend() call leaves the hardware permanently suspended while the PM core believes the device is active.
- [High] wait_for_gt_c6_state() dereferences a NULL idle_status pointer when executed on SR-IOV Virtual Functions.
--

commit 5290095cf4ef101ed42731b369d55f98f9bbeaa1
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

drm/xe: Poll GT for C6 before D3

This commit adds a check before transitioning to D3 to ensure GTs are in the
C6 state. If the GT is not in C6, it returns -EAGAIN to allow the PM
subsystem to retry, preventing component access during forced D3 state.

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
[ ... ]
> @@ -1377,10 +1378,25 @@ static int xe_pci_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int wait_for_gt_c6_state(struct xe_gt *gt,
> +				int timeout_ms)
> +{
> +	struct xe_guc_pc *pc = &gt->uc.guc.pc;
> +	enum xe_gt_idle_state state;
> +
> +	return poll_timeout_us(state = gt->gtidle.idle_status(pc),

[Severity: High]
Is it possible for this to dereference a NULL pointer for SR-IOV Virtual
Functions?

If IS_SRIOV_VF(xe) is true, initialization is skipped in xe_gt_idle_init(),
leaving the gtidle->idle_status function pointer NULL. The
xe_assert(xe, !IS_SRIOV_VF(xe)) in xe_pci_runtime_suspend() only emits a
warning in debug builds (or acts as a no-op in production builds) and does
not halt execution, allowing a VF to reach this unconditional dereference
in wait_for_gt_c6_state().

> +			       state == GT_IDLE_C6,
> +			       20,
> +			       timeout_ms * USEC_PER_MSEC,
> +			       false);
> +}
> +
>  static int xe_pci_runtime_suspend(struct device *dev)
>  {
[ ... ]
> @@ -1396,6 +1412,14 @@ static int xe_pci_runtime_suspend(struct device *dev)
>  	if (err)
>  		return err;
>  
> +	/* Poll and retry if we don't enter C6 on any GT */
> +	for_each_gt(gt, xe, id) {
> +		if (wait_for_gt_c6_state(gt, 200)) {
> +			xe_gt_info(gt, "GT is not in C6\n");
> +			return -EAGAIN;

[Severity: High]
Could returning -EAGAIN here leave the hardware permanently suspended while
the PM core believes the device is active?

The driver successfully suspends multiple hardware components (display, GTs,
IRQs) earlier in xe_pci_runtime_suspend() via xe_pm_runtime_suspend().
However, returning -EAGAIN here aborts the PM core transition, leaving the
device status as RPM_ACTIVE.

The PM core will not invoke the resume callback, and subsequent driver
operations will attempt to access powered-down hardware. Should this call
xe_pm_runtime_resume() to roll back the successful suspend operations before
returning?

> +		}
> +	}
> +
>  	pci_save_state(pdev);

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