Re: [PATCH v10 08/10] drm/xe: Introduce temporary device wedging

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Sleeping in atomic context due to `mutex_lock()` in `xe_device_declare_wedged()`.
- [High] Unprotected state modification and double-increment of `wedged.ref` in `prepare_device_for_reset()`.
- [High] Temporary wedged state from PCI error recovery leaks PM reference and triggers teardown assert if recovery aborts.
--

--- Patch [8]: [PATCH v10 08/10] drm/xe: Introduce temporary device wedging ---
commit 102094142d418b8de53397ac8e1fbe1c7ca46ae9
Author: Raag Jadav <[email protected]>

drm/xe: Introduce temporary device wedging

This commit converts the boolean wedged flag to a reference count to allow
temporary wedged states. It also introduces a perm flag for permanent
wedging and a mutex to protect the wedged state transitions.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1476,36 +1526,40 @@ void xe_device_declare_wedged(struct xe_device *xe)
>  		return;
>  	}
>  
> -	if (!atomic_xchg(&xe->wedged.flag, 1)) {
> -		xe->needs_flr_on_fini = true;
> -		xe_pm_runtime_get_noresume(xe);
> +	mutex_lock(&xe->wedged.lock);

[Severity: Critical]
Can this cause a sleeping in atomic context panic?

xe_device_declare_wedged() can be called from hard IRQ contexts, for
example:

xe_mert_irq_handler()
  mert_handle_cat_error()
    xe_device_declare_wedged()

Since mutex_lock() can sleep, taking it here from an interrupt handler
would lead to a scheduling while atomic regression.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/xe_pci_error.c
> --- a/drivers/gpu/drm/xe/xe_pci_error.c
> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
> @@ -25,8 +25,10 @@ static void prepare_device_for_reset(struct pci_dev *pdev)
>  	 * xe_device_wedged_fini() releases runtime pm if wedged flag is set, so acquire a runtime
>  	 * pm reference to avoid underflow.
>  	 */
> -	if (!atomic_xchg(&xe->wedged.flag, 1))
> -		xe_pm_runtime_get_noresume(xe);
> +	xe_device_wedged_get(xe);
> +
> +	/* TODO: We shouldn't be doing such ugly hacks, reuse FLR helpers and drop this. */
> +	xe->wedged.perm = true;

[Severity: High]
Could this leak a reference and cause an assertion failure during device
teardown if the device is already wedged?

Unlike the old atomic_xchg code which checked if the device was already
wedged, xe_device_wedged_get() increments the reference count unconditionally.
If the device is already wedged from a previous event, the reference count
will be incremented twice.

During teardown, xe_device_wedged_fini() only drops a single reference
when wedged.perm is true:

drivers/gpu/drm/xe/xe_device.c:xe_device_wedged_fini() {
    if (xe->wedged.perm)
        xe_device_wedged_put(xe);

    xe_assert(xe, !xe_device_wedged(xe));
}

Will the second reference be left behind and trip the xe_assert() check?
Also, should the state modification here be protected by xe->wedged.lock?

[ ... ]
> @@ -126,7 +128,7 @@ static pci_ers_result_t xe_pci_error_slot_reset(struct pci_dev *pdev)
>  	xe = pdev_to_xe_device(pdev);
>  
>  	/* Wedge the device to prevent I/O operations till the resume callback */
> -	atomic_set(&xe->wedged.flag, 1);
> +	xe_device_wedged_get(xe);

[Severity: High]
If the PCI error recovery aborts or fails here, and the device is subsequently
unbound, will this leak the runtime PM reference?

This code acquires a temporary wedged reference but does not set
xe->wedged.perm to true. If the expected xe_pci_error_resume() is never
called due to a recovery failure, xe_device_wedged_fini() will see
xe->wedged.perm as false and skip dropping the reference, which would
ultimately trigger the xe_assert(xe, !xe_device_wedged(xe)) on teardown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8