Re: [PATCH v10 4/4] drm/xe: Handle PUNIT errors by requesting cold-reset recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected modification and TOCTOU race on `xe->wedged.method` when declaring a wedge state.
--

--- Patch [4]: [PATCH v10 4/4] drm/xe: Handle PUNIT errors by requesting cold-reset recovery ---
commit ee614ec29f76518d52ca16432516d10ff5da4e57
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe: Handle PUNIT errors by requesting cold-reset recovery

When PUNIT (power management unit) errors are detected that persist across
warm resets, mark the device as wedged with DRM_WEDGE_RECOVERY_COLD_RESET
and notify userspace that a complete device power cycle is required to
restore normal operation.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 3d39bb95e94d6..cc2a0e59dedf5 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -194,6 +194,12 @@ static void ras_usp_aer_init(struct xe_device *xe)
>  	dev_dbg(&usp->dev, "Uncorrectable Internal Errors downgraded and unmasked\n");
>  }
>  
> +static void punit_error_handler(struct xe_device *xe)
> +{
> +	xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_COLD_RESET);
> +	xe_device_declare_wedged(xe);
> +}
> +

[Severity: High]
This is a pre-existing issue, but is there a race condition between setting
the wedge method here and other threads declaring the device wedged?

If a PUNIT error causes concurrent GPU hangs, the GT hang recovery worker
(Thread B) might call xe_device_declare_wedged() concurrently with the RAS
interrupt handler (Thread A) calling punit_error_handler().

Thread B could read xe->wedged.method before Thread A writes it:

Thread B in xe_device_declare_wedged():
    if (!xe->wedged.method) {
        // Thread B evaluates this as true

Thread A in punit_error_handler():
    xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_COLD_RESET);

Thread B in xe_device_declare_wedged():
        xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_REBIND |
                                        DRM_WEDGE_RECOVERY_BUS_RESET);
    }

Since the read-then-write of xe->wedged.method is lockless, Thread B can
overwrite the DRM_WEDGE_RECOVERY_COLD_RESET requested by Thread A with the
default method. 

Could this cause the device to fail recovery by missing the required cold
reset, and does xe->wedged.method need synchronization?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713083939.251872-6-mallesh.koujalagi@intel.com?part=4