Re: [PATCH 02/13] drm/xe: Separate AER reset state from device wedging

["Yadav, Arvind" <arvind.yadav@intel.com>]

On 10-09-2026 15:37, Thomas Hellström wrote:
> On Thu, 2026-08-27 at 15:47 +0530, Arvind Yadav wrote:
>> PCI error recovery currently uses xe->wedged.flag to block driver
>> access. This mixes a temporary AER reset with a permanent device
>> wedge.
>>
>> If the device wedges during AER recovery, the wedge is not seen as
>> the
>> first transition. The AER resume callback may then clear the flag and
>> make the permanently wedged device appear usable again.
>>
>> Keep the old device blocked while slot reset removes it, and block
>> the
>> new device until the AER resume callback.
>>
>> The old AER path took a runtime PM reference to balance
>> xe_device_wedged_fini(), which drops one when wedged.flag is set. AER
>> no
>> longer sets that flag, so keeping the Xe-owned reference would leak
>> it.
>> pcie_do_recovery() holds a PCI-core runtime PM reference across the
>> error_detected, slot_reset and resume callbacks.
>>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c            |  2 +-
>>   drivers/gpu/drm/xe/xe_device.c        |  4 ++--
>>   drivers/gpu/drm/xe/xe_device.h        | 12 ++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_ct.c        |  4 ++--
>>   drivers/gpu/drm/xe/xe_guc_pc.c        | 10 +++++-----
>>   drivers/gpu/drm/xe/xe_guc_rc.c        |  4 ++--
>>   drivers/gpu/drm/xe/xe_guc_submit.c    |  8 ++++++--
>>   drivers/gpu/drm/xe/xe_guc_tlb_inval.c |  8 +++++++-
>>   drivers/gpu/drm/xe/xe_pci_error.c     | 22 +++++++++++-----------
>>   drivers/gpu/drm/xe/xe_sriov_pf.c      |  2 +-
>>   10 files changed, 49 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index dde309821237..b86cd6030ed6 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -2094,7 +2094,7 @@ static vm_fault_t xe_bo_cpu_fault(struct
>> vm_fault *vmf)
>>   	int err = 0;
>>   	int idx;
>>   
>> -	if (xe_device_wedged(xe) || !drm_dev_enter(&xe->drm, &idx))
>> +	if (xe_device_io_blocked(xe) || !drm_dev_enter(&xe->drm,
>> &idx))
>>   		return ttm_bo_vm_dummy_page(vmf, vmf->vma-
>>> vm_page_prot);
>>   
>>   	ret = xe_bo_cpu_fault_fastpath(vmf, xe, bo, needs_rpm);
>> diff --git a/drivers/gpu/drm/xe/xe_device.c
>> b/drivers/gpu/drm/xe/xe_device.c
>> index 74d566693dfd..a92e90acdf0d 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -225,7 +225,7 @@ static long xe_drm_ioctl(struct file *file,
>> unsigned int cmd, unsigned long arg)
>>   	struct xe_device *xe = to_xe_device(file_priv->minor->dev);
>>   	long ret;
>>   
>> -	if (xe_device_wedged(xe))
>> +	if (xe_device_io_blocked(xe))
>>   		return -ECANCELED;
>>   
>>   	ACQUIRE(xe_pm_runtime_ioctl, pm)(xe);
>> @@ -243,7 +243,7 @@ static long xe_drm_compat_ioctl(struct file
>> *file, unsigned int cmd, unsigned lo
>>   	struct xe_device *xe = to_xe_device(file_priv->minor->dev);
>>   	long ret;
>>   
>> -	if (xe_device_wedged(xe))
>> +	if (xe_device_io_blocked(xe))
>>   		return -ECANCELED;
>>   
>>   	ACQUIRE(xe_pm_runtime_ioctl, pm)(xe);
>> diff --git a/drivers/gpu/drm/xe/xe_device.h
>> b/drivers/gpu/drm/xe/xe_device.h
>> index 6c4cfaebc44a..a3f876c60d76 100644
>> --- a/drivers/gpu/drm/xe/xe_device.h
>> +++ b/drivers/gpu/drm/xe/xe_device.h
>> @@ -212,6 +212,18 @@ static inline bool xe_device_wedged(struct
>> xe_device *xe)
>>   	return atomic_read(&xe->wedged.flag);
>>   }
>>   
>> +/*
>> + * Return true when device access must be blocked either permanently
>> because
>> + * the device is wedged or temporarily while PCI error recovery is
>> running.
>> + *
>> + * Do not use this helper for one-way wedged-device decisions such
>> as DMA
>> + * isolation, IRQ resume suppression or recovery-method reporting.
>> + */
>> +static inline bool xe_device_io_blocked(struct xe_device *xe)
>> +{
>> +	return xe_device_wedged(xe) || xe_device_is_in_reset(xe);
>> +}
>> +
>>   #ifdef CONFIG_DRM_XE_DEBUG_PAGE_SIZE
>>   static inline bool xe_debug_page_size_supported(struct xe_device
>> *xe)
>>   {
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c
>> b/drivers/gpu/drm/xe/xe_guc_ct.c
>> index 5c4733da385c..3c3fe4928fa2 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>> @@ -1062,7 +1062,7 @@ static int __guc_ct_send_locked(struct
>> xe_guc_ct *ct, const u32 *action,
>>   	xe_gt_assert(gt, g2h_len || !num_g2h);
>>   	lockdep_assert_held(&ct->lock);
>>   
>> -	if (xe_device_wedged(ct_to_xe(ct))) {
>> +	if (xe_device_io_blocked(ct_to_xe(ct))) {
>>   		ret = -ENOTRECOVERABLE;
>>   		goto out;
>>   	}
>> @@ -1813,7 +1813,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32
>> *msg, bool fast_path)
>>   	xe_gt_assert(gt, xe_guc_ct_initialized(ct));
>>   	lockdep_assert_held(&ct->fast_lock);
>>   
>> -	if (xe_device_wedged(xe))
>> +	if (xe_device_io_blocked(xe))
>>   		return -ENOTRECOVERABLE;
>>   
>>   	if (ct->state == XE_GUC_CT_STATE_DISABLED)
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 097b075bd89a..9fe397296dc4 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -188,7 +188,7 @@ static int pc_action_reset(struct xe_guc_pc *pc)
>>   	int ret;
>>   
>>   	ret = xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0);
>> -	if (ret && !(xe_device_wedged(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>> +	if (ret && !(xe_device_io_blocked(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>>   		xe_gt_err(pc_to_gt(pc), "GuC PC reset failed:
>> %pe\n",
>>   			  ERR_PTR(ret));
>>   
>> @@ -212,7 +212,7 @@ static int pc_action_query_task_state(struct
>> xe_guc_pc *pc)
>>   
>>   	/* Blocking here to ensure the results are ready before
>> reading them */
>>   	ret = xe_guc_ct_send_block(ct, action, ARRAY_SIZE(action));
>> -	if (ret && !(xe_device_wedged(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>> +	if (ret && !(xe_device_io_blocked(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>>   		xe_gt_err(pc_to_gt(pc), "GuC PC query task state
>> failed: %pe\n",
>>   			  ERR_PTR(ret));
>>   
>> @@ -235,7 +235,7 @@ static int pc_action_set_param(struct xe_guc_pc
>> *pc, u8 id, u32 value)
>>   		return -EAGAIN;
>>   
>>   	ret = xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0);
>> -	if (ret && !(xe_device_wedged(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>> +	if (ret && !(xe_device_io_blocked(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>>   		xe_gt_err(pc_to_gt(pc), "GuC PC set param[%u]=%u
>> failed: %pe\n",
>>   			  id, value, ERR_PTR(ret));
>>   
>> @@ -257,7 +257,7 @@ static int pc_action_unset_param(struct xe_guc_pc
>> *pc, u8 id)
>>   		return -EAGAIN;
>>   
>>   	ret = xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0);
>> -	if (ret && !(xe_device_wedged(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>> +	if (ret && !(xe_device_io_blocked(pc_to_xe(pc)) && ret == -
>> ECANCELED))
>>   		xe_gt_err(pc_to_gt(pc), "GuC PC unset param failed:
>> %pe",
>>   			  ERR_PTR(ret));
>>   
>> @@ -1357,7 +1357,7 @@ static void xe_guc_pc_fini_hw(void *arg)
>>   	struct xe_guc_pc *pc = arg;
>>   	struct xe_device *xe = pc_to_xe(pc);
>>   
>> -	if (xe_device_wedged(xe))
>> +	if (xe_device_io_blocked(xe))
>>   		return;
>>   
>>   	xe_guc_pc_stop(pc);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_rc.c
>> b/drivers/gpu/drm/xe/xe_guc_rc.c
>> index 99fa127b261f..eb5ec443f7ee 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_rc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_rc.c
>> @@ -40,7 +40,7 @@ static int guc_action_setup_gucrc(struct xe_guc
>> *guc, u32 control)
>>   	int ret;
>>   
>>   	ret = xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
>> 0, 0);
>> -	if (ret && !(xe_device_wedged(guc_to_xe(guc)) && ret == -
>> ECANCELED))
>> +	if (ret && !(xe_device_io_blocked(guc_to_xe(guc)) && ret ==
>> -ECANCELED))
>>   		xe_gt_err(guc_to_gt(guc),
>>   			  "GuC RC setup %s(%u) failed (%pe)\n",
>>   			   control == GUCRC_HOST_CONTROL ?
>> "HOST_CONTROL" :
>> @@ -73,7 +73,7 @@ static void xe_guc_rc_fini_hw(void *arg)
>>   	struct xe_device *xe = guc_to_xe(guc);
>>   	struct xe_gt *gt = guc_to_gt(guc);
>>   
>> -	if (xe_device_wedged(xe))
>> +	if (xe_device_io_blocked(xe))
>>   		return;
>>   
>>   	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
>> b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 99d8c807ff05..a307af458cf8 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -2452,7 +2452,7 @@ static int
>> guc_exec_queue_wait_suspend_done(struct xe_exec_queue *q, bool blocki
>>   						       WAIT_COND, HZ
>> * 5);
>>   	}
>>   
>> -	if (!blocking && vf_recovery(guc) && !xe_device_wedged(xe))
>> +	if (!blocking && vf_recovery(guc) &&
>> !xe_device_io_blocked(xe))
>>   		return -EAGAIN;
>>   
>>   	if (!ret)
>> @@ -2694,7 +2694,11 @@ int xe_guc_submit_reset_prepare(struct xe_guc
>> *guc)
>>   
>>   void xe_guc_submit_reset_wait(struct xe_guc *guc)
>>   {
>> -	wait_event(guc->ct.wq, xe_device_wedged(guc_to_xe(guc)) ||
>> +	/*
>> +	 * AER sets in_reset before declaring the GT wedged, which
>> wakes this
>> +	 * waitqueue.
>> +	 */
>> +	wait_event(guc->ct.wq, xe_device_io_blocked(guc_to_xe(guc))
>> ||
>>   		   !xe_guc_read_stopped(guc));
>>   }
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
>> b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
>> index 046d0655122f..646e13671cd9 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
>> @@ -34,6 +34,9 @@ static int send_tlb_inval(struct xe_guc *guc, const
>> u32 *action, int len)
>>   
>>   	xe_gt_assert(gt, action[1]);	/* Seqno */
>>   
>> +	if (xe_device_io_blocked(guc_to_xe(guc)))
>> +		return -ECANCELED;
>> +
>>   	xe_gt_stats_incr(gt, XE_GT_STATS_ID_TLB_INVAL, 1);
>>   	return xe_guc_ct_send(&guc->ct, action, len,
>>   			      G2H_LEN_DW_TLB_INVALIDATE, 1);
>> @@ -69,6 +72,9 @@ static int send_tlb_inval_ggtt(struct xe_tlb_inval
>> *tlb_inval, u32 seqno)
>>   	 * signals waiters.
>>   	 */
>>   
>> +	if (xe_device_io_blocked(xe))
>> +		return -ECANCELED;
>> +
>>   	if (xe_guc_ct_enabled(&guc->ct) && guc-
>>> submission_state.enabled) {
>>   		u32 action[] = {
>>   			XE_GUC_ACTION_TLB_INVALIDATION,
>> @@ -77,7 +83,7 @@ static int send_tlb_inval_ggtt(struct xe_tlb_inval
>> *tlb_inval, u32 seqno)
>>   		};
>>   
>>   		return send_tlb_inval(guc, action,
>> ARRAY_SIZE(action));
>> -	} else if (xe_device_uc_enabled(xe) &&
>> !xe_device_wedged(xe)) {
>> +	} else if (xe_device_uc_enabled(xe)) {
>>   		struct xe_mmio *mmio = &gt->mmio;
>>   
>>   		if (IS_SRIOV_VF(xe))
>> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c
>> b/drivers/gpu/drm/xe/xe_pci_error.c
>> index 79ce0c671549..d82256d8721f 100644
>> --- a/drivers/gpu/drm/xe/xe_pci_error.c
>> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
>> @@ -9,7 +9,6 @@
>>   #include "xe_gt.h"
>>   #include "xe_log.h"
>>   #include "xe_pci.h"
>> -#include "xe_pm.h"
>>   #include "xe_printk.h"
>>   #include "xe_ras.h"
>>   #include "xe_survivability_mode.h"
>> @@ -20,14 +19,15 @@ static void prepare_device_for_reset(struct
>> pci_dev *pdev)
>>   	struct xe_gt *gt;
>>   	u8 id;
>>   
>> +
>>   	/*
>> -	 * Wedge the device to prevent userspace access but do not
>> send the uevent.
>> -	 * xe_device_wedged_fini() releases runtime pm if wedged
>> flag is set, so acquire a runtime
>> -	 * pm reference to avoid underflow.
>> +	 * Block device access while PCI error recovery is in
>> progress.
>> +	 *
>> +	 * The old runtime PM reference balanced
>> xe_device_wedged_fini() while
>> +	 * AER set wedged.flag. AER no longer sets that flag, and
>> +	 * pcie_do_recovery() holds its own runtime PM reference
>> across the
>> +	 * recovery callbacks.
> This is an in-code comment describing what this patch is doing. A
> future code reader has no idea what "The old runtime PM reference" is.
> Please keep comments involving the old pre-patch code in the commit
> message.


Thanks for the review.

Agreed. I will keep the old runtime PM details in the commit message. 
and the comment will only describe the current behavior.


Thanks,
Arvind

>
>
>>   	 */
>> -	if (!atomic_xchg(&xe->wedged.flag, 1))
>> -		xe_pm_runtime_get_noresume(xe);
>> -
>>   	xe_device_set_in_reset(xe);
>>   
>>   	for_each_gt(gt, xe, id)
>> @@ -116,7 +116,6 @@ static pci_ers_result_t
>> xe_pci_error_slot_reset(struct pci_dev *pdev)
>>   	 * TODO: optimize by re-initializing only the hardware state
>> and re-creating
>>   	 * kernel BOs.
>>   	 */
>> -	xe_device_clear_in_reset(xe);
>>   	pdev->driver->remove(pdev);
>>   	devres_release_group(&pdev->dev, xe->devres_group);
>>   
>> @@ -125,8 +124,8 @@ 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);
>> +	/* Block the new instance until the resume callback. */
>> +	xe_device_set_in_reset(xe);
>>   
>>   	return PCI_ERS_RESULT_RECOVERED;
>>   }
>> @@ -137,7 +136,8 @@ static void xe_pci_error_resume(struct pci_dev
>> *pdev)
>>   
>>   	xe_info(xe, "PCI error: resume\n");
>>   
>> -	atomic_set(&xe->wedged.flag, 0);
>> +	/* Resume I/O operations. */
>> +	xe_device_clear_in_reset(xe);
>>   }
>>   
>>   const struct pci_error_handlers xe_pci_error_handlers = {
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_pf.c
>> b/drivers/gpu/drm/xe/xe_sriov_pf.c
>> index 33bd754d138f..568b7ed7c380 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_pf.c
>> +++ b/drivers/gpu/drm/xe/xe_sriov_pf.c
>> @@ -157,7 +157,7 @@ int xe_sriov_pf_wait_ready(struct xe_device *xe)
>>   	unsigned int id;
>>   	int err;
>>   
>> -	if (xe_device_wedged(xe))
>> +	if (xe_device_io_blocked(xe))
>>   		return -ECANCELED;
>>   
>>   	for_each_gt(gt, xe, id) {
>
> /Thomas