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

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 9/8/2026 6:53 PM, sashiko-bot@kernel.org wrote:
> 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().
yes, will add a check.
>
>> +			       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?

Will call xe_pm_runtime_resume() if GT is not in C6.

Thanks,

Vinay.

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