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

[sashiko-bot@kernel.org]

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.

> +	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.

[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?

[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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906172604.2215987-14-badal.nilawar@intel.com?part=2