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 <[email protected]> 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/[email protected]?part=4
