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

["Laguna, Lukasz" <lukasz.laguna@intel.com>]

On 8/21/2026 13:37, sashiko-bot@kernel.org wrote:
> 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()`.

Already discussed, can be addressed in a follow-up series.

> - [High] Unprotected state modification and double-increment of `wedged.ref` in `prepare_device_for_reset()`.

Valid. It should be enough to just take the lock and check 
xe->wedged.perm before taking the reference.

> - [High] Temporary wedged state from PCI error recovery leaks PM reference and triggers teardown assert if recovery aborts.

It seems to be a false report. Reference is taken just before 
RESULT_RECOVERED.

Lukasz

> --
>
> --- Patch [8]: [PATCH v10 08/10] drm/xe: Introduce temporary device wedging ---
> commit 102094142d418b8de53397ac8e1fbe1c7ca46ae9
> Author: Raag Jadav <raag.jadav@intel.com>
>
> 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.
>