Re: [PATCH v4 3/5] drm/xe: Make xe_device_declare_wedged() IRQ safe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: data race on xe->wedged.method due to wedged_work concurrent execution
--

commit 1b950e6e1cdfbf78ff8ea7b58f37f030e514ddf2
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe: Make xe_device_declare_wedged() IRQ safe

Currently, xe_device_declare_wedged() implementation wedges the GTs and
sends wedged uevent to userspace. This is problematic for usecases which
require declaring the device as wedged in IRQ context, as it allocates
memory for event buffer and acquires mutexes deep into it's call path.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 1c3281c8d2863..f4536583e57ce 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1392,6 +1400,26 @@ void xe_device_set_wedged_method(struct xe_device *xe, unsigned long method)
>  	xe->wedged.method = method;
>  }

[Severity: Medium]
Could this cause a data race on xe->wedged.method?

Since xe_device_set_wedged_method() can be called from multiple RAS error
handlers in a loop while processing hardware errors, it might write to
xe->wedged.method while the newly added asynchronous worker is actively
reading or modifying it.

> +static void wedged_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, wedged.work);
> +
> +	/*
> +	 * XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET is intended for debugging
> +	 * hangs, so wedge the device with 'none' recovery method and have
> +	 * it available to the user for debugging.
> +	 */
> +	if (xe->wedged.mode == XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET)
> +		xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_NONE);
> +	/* If no wedge recovery method is set, use default */
> +	else if (!xe->wedged.method)
> +		xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_REBIND |
> +					    DRM_WEDGE_RECOVERY_BUS_RESET);
> +
> +	/* Notify userspace of wedged device */
> +	drm_dev_wedged_event(&xe->drm, xe->wedged.method, NULL);
> +}

[Severity: Medium]
Is it possible for the wedged method to be corrupted here?

If xe_ras_process_errors() processes hardware errors, the first error might
set xe->wedged.method and call xe_device_declare_wedged() which schedules
wedged.work.

If the loop continues and a second error sets xe->wedged.method again, this
sequential write could race with this worker concurrently reading and
modifying xe->wedged.method on another CPU. This could lead to a corrupted
or incorrect recovery method being passed to drm_dev_wedged_event().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010090718.68308-1-raag.jadav@intel.com?part=3