Re: [PATCH v3 02/12] drm/xe/cper: Retrieve the error counter record for CPER reporting

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 06-09-2026 22:53, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] Fetching error counters from the generic logging emission path introduces a might_sleep() restriction into generic logging macros.
> - [High] xe_emit_hardware_error_cper reads MMIO registers via sysctrl command without ensuring the device is awake, leading to potential system crashes or PCIe AER errors if called when the device is in D3cold.
> - [Medium] The patch introduces redundant, synchronous sysctrl mailbox commands to fetch counter values that the driver either already possesses or fetches again immediately.
>
> Pre-existing issues:
> - [High] Preexisting logic bug causes xe_emit_hardware_error_cper to immediately abort for hardware SIGIDs, making the newly added code unreachable.
> --
>
> commit ea598ea85d76d6f40d20fe1e15edf6a22c8e705c
> Author: Badal Nilawar <badal.nilawar@intel.com>
>
> drm/xe/cper: Retrieve the error counter record for CPER reporting
>
> Retrieve the error counter record required to build a CPER record when
> one is not provided by the caller.
>
>> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
>> index e8017e3ee3a09..f04a91223a43c 100644
>> --- a/drivers/gpu/drm/xe/xe_cper.c
>> +++ b/drivers/gpu/drm/xe/xe_cper.c
> [ ... ]
>> @@ -26,6 +28,8 @@ void xe_emit_hardware_error_cper(struct pci_dev *pdev, int cper_sev, enum xe_sig
>>   				 struct xe_ras_get_counter_response *response)
>>   {
>>   	struct xe_device *xe = pdev_to_xe_device(pdev);
>> +	struct xe_ras_get_counter_response local_resp = {};
>> +	struct xe_ras_get_counter_response *counter_response = response;
>>   
>>   	if (!xe)
>>   		return;
>> @@ -33,5 +37,16 @@ void xe_emit_hardware_error_cper(struct pci_dev *pdev, int cper_sev, enum xe_sig
>>   	if ((int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START)
>>   		return;
> [Severity: High]
> This isn't a bug introduced by this patch, but does this condition in
> xe_emit_hardware_error_cper() prevent the newly added code from executing?
>
> Since hardware SIGIDs are strictly greater than or equal to
> INTEL_SIGID_GPU_XE_HARDWARE_START, checking if sigid is greater than or
> equal to this constant seems to cause an immediate return for all hardware
> errors, silently disabling the CPER record emission.
Will fix this.
>
>> +	if (!counter || !xe_ras_counter_is_valid(xe, counter))
>> +		return;
>> +
>> +	if (!counter_response) {
>> +		counter_response = &local_resp;
>> +		if (xe_ras_get_counter_response(xe, counter, counter_response)) {
> [Severity: High]
> Does fetching error counters from the generic logging emission path
> introduce a sleep restriction into logging macros?
>
> If a generic macro like xe_log_comp_fatal() is called from an atomic context,
> this synchronous fetch via xe_ras_get_counter_response() will invoke
> xe_sysctrl_send_command(). The underlying sysctrl command asserts might_sleep()
> and waits on a mutex, which could cause a bug or deadlock if triggered from an
> interrupt handler or spinlock.

xe_log_comp_fatal() and related logging macros can reach 
xe_ras_get_counter_response(), which sends a sysctrl command and may sleep.

Only hardware SIGIDs associated with a valid xe_ras_error_class take 
this path. The callers are xe_ras_process_errors() handlers and 
xe_ras_counter_threshold_crossed().

These paths already issue sysctrl commands and therefore execute in a 
context where sleeping is allowed.

Will add might_sleep() to enforce the sleep-able-context requirement.

>
> [Severity: High]
> Also, does this hardware access ensure the device is runtime-resumed?
>
> The sysctrl command path uses guard(xe_pm_runtime_noresume)(xe), which
> increments the usage counter but avoids waking a suspended device. If this
> logging path executes while the device is in D3cold, will the subsequent
> MMIO reads hit powered-off hardware and trigger unhandled PCIe AER errors?
Callers are expected to hold outer runtime pm reference. Will add 
xe_device_assert_mem_access().
>
> [Severity: Medium]
> Finally, does this force a new mailbox command to fetch data that the caller
> might already possess?
>
> For example, in xe_ras_process_errors(), the driver already executes a
> mailbox command that returns the counter values. However, it passes only a
> pointer to the generic logging function, forcing
> xe_emit_hardware_error_cper() to issue an entirely new, redundant synchronous
> mailbox command to fetch the same value. Can this be optimized to reuse the
> existing data and avoid extra latency during error handling?

There is no redundant mailbox command. Call inside xe_ras_process_errors 
fetch the aggregate counter for netlink event whereas 
xe_emit_hardware_error_cper() fetches detail error counter required for 
CPER, which also contains info queue data.

Thanks,
Badal

>
>> +			xe_err(xe, "[RAS]: CPER: failed to get counter, skipping record\n");
>> +			return;
>> +		}
>> +	}
>> +
>>   	/* TODO */
>>   }