Re: [PATCH 3/3] drm/xe/xe_hw_error: Handle non-fatal errors routed via direct IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `hw_error_work` accesses system controller MMIO registers without holding an active runtime PM reference, risking MMIO faults if the device is suspended.
- [Medium] `hw_error_work` unconditionally overwrites the device wedge method with `DRM_WEDGE_RECOVERY_BUS_RESET`, clobbering previously set recovery methods and sending duplicate wedge events.
--

commit 7bcb91bb7afbf8e7efbb4e0783cf8098239c131e
Author: Riana Tauro <riana.tauro@intel.com>

drm/xe/xe_hw_error: Handle non-fatal errors routed via direct IRQ
    
This commit modifies xe_hw_error to process, log, and recover non-fatal errors
routed via direct IRQ. For errors that cannot be recovered locally, the device
is wedged using a bus-reset as the recovery action.

> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 5a57b7eaebc3b..a895436f2ce7e 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c

[ ... ]

>  static void hw_error_work(struct work_struct *work)
>  {
>  	struct xe_tile *tile = container_of(work, typeof(*tile), hw_error_work);
>  	struct xe_device *xe = tile_to_xe(tile);
>  
> +	if (xe->info.has_sysctrl) {
> +		int ret;
> +
> +		ret = xe_ras_process_errors(xe);

[Severity: High]
Does this code access system controller MMIO registers without holding an
active runtime PM reference?

When hw_error_source_handler() schedules hw_error_work() asynchronously, it
doesn't take a runtime PM reference. Once this work executes, it calls
xe_ras_process_errors() which eventually calls xe_sysctrl_send_command().

While xe_sysctrl_send_command() uses guard(xe_pm_runtime_noresume)(xe), this
only increments the usage count and asserts a reference is held; it does not
actually wake a suspended device. If the device transitions to runtime suspend
between the hardware error IRQ firing and the workqueue executing, the
subsequent MMIO reads in sysctrl_read_frame() will access powered-down
hardware, potentially causing PCI target aborts, 0xFFFFFFFF reads, or system
hangs.

> +		/* For any non-fatal errors that do not return recovered, declare wedged */
> +		if (ret) {
> +			xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_BUS_RESET);
> +			xe_device_declare_wedged(xe);

[Severity: Medium]
Can this unconditionally overwrite the device wedge method, clobbering
previously set recovery methods and sending duplicate wedge events?

During xe_ras_process_errors(), functions like handle_soc_internal_errors()
may call xe_survivability_mode_runtime_enable(xe). That function sets the
wedge method to DRM_WEDGE_RECOVERY_VENDOR and declares the device wedged,
before returning XE_RAS_RECOVERY_ACTION_DISCONNECT:

drivers/gpu/drm/xe/xe_ras.c:handle_soc_internal_errors() {
	...
	xe_survivability_mode_runtime_enable(xe);
	return XE_RAS_RECOVERY_ACTION_DISCONNECT;
}

Here, ret (the returned action) is treated as a boolean. By unconditionally
calling xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_BUS_RESET) and
xe_device_declare_wedged(xe) again, the device is presented to userspace with
an incorrect recovery method (BUS_RESET) instead of the true requirement
(VENDOR), and duplicate uevents are emitted.

> +		}
> +
> +		return;
> +	}
> +
>  	xe_survivability_mode_runtime_enable(xe);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830065603.419055-5-riana.tauro@intel.com?part=3