Re: [PATCH v2] drm/xe: Add wa_14028434957 to Graphics Version 35.11

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: early MSI workqueues wedge device before cleanup handlers are registered
--

commit 64d5d3713cb5cf15e2cdb1f9711919646d7c8e35
Author: Varun Gupta <varun.gupta@intel.com>
drm/xe: Add wa_14028434957 to Graphics Version 35.11

This patch prevents the SNIB AER latch from swallowing back-to-back 
uncorrectable non-fatal errors by intercepting the MSI and reusing the 
existing GFSP error containment logic. Because GFSP mailbox queries can 
sleep, handling is deferred to a workqueue to ensure hardirq safety, 
wedging the device if a reset is required.

> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 5f2abc9485ff5..bde707a3fb641 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
[ ... ]
> @@ -173,6 +178,29 @@ static void csc_hw_error_work(struct work_struct *work)
>  	xe_survivability_mode_runtime_enable(xe);
>  }
>  
> +static void nonfatal_msi_work(struct work_struct *work)
> +{
> +	struct xe_drm_ras *ras = container_of(work, typeof(*ras), nonfatal_msi_work);
> +	struct xe_device *xe = container_of(ras, typeof(*xe), ras);
> +
> +	guard(xe_pm_runtime)(xe);
> +	enum xe_ras_recovery_action action = xe_ras_process_errors(xe);
> +
> +	/*
> +	 * TODO: Request an SBR instead of wedging, once VR driver
> +	 * quiescing support is in place.
> +	 */
> +	if (action != XE_RAS_RECOVERY_ACTION_RECOVERED)
> +		xe_device_declare_wedged(xe);

[Severity: Medium]
Can this workqueue race with driver probe?

Looking at xe_irq_install(), it calls xe_hw_error_init() which processes 
pre-existing hardware errors via process_hw_errors(). This immediately queues 
nonfatal_msi_work if non-fatal errors are already pending.

If the work executes concurrently with xe_device_probe(), it will call 
xe_device_declare_wedged(xe) which acquires a PM runtime reference 
(xe_pm_runtime_get_noresume) and sends a wedged uevent. 

However, xe_device_wedged_fini isn't registered until the very end of probe, 
and drm_dev_register() hasn't finished yet. 

If probe fails in between, could the PM runtime reference leak? Is it safe 
to send a wedged uevent on an unregistered DRM device?

> +}
> +
[ ... ]
> @@ -436,6 +464,15 @@ static void hw_error_source_handler(struct xe_tile *tile, const enum hardware_er
>  	 * here for those platforms.
>  	 */
>  	if (xe->info.has_sysctrl) {
> +		/*
> +		 * Defer non-fatal MSI handling to process context because the GFSP
> +		 * query can sleep.
> +		 */
> +		if (hw_err == HARDWARE_ERROR_NONFATAL && XE_DEVICE_WA(xe, 14028434957)) {
> +			schedule_work(&xe->ras.nonfatal_msi_work);

[Severity: Medium]
Could this execute too early during initialization?

As noted above, if a non-fatal error is present during module load, this 
schedules the work early in the initialization sequence before cleanup 
handlers are fully established in xe_device_probe().

> +			return;
> +		}
> +
>  		drm_err_ratelimited(&xe->drm, HW_ERR "Invalid error routing\n");
>  		return;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009075224.1316885-2-varun.gupta@intel.com?part=1