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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] guc_ct_send_recv() still checks xe_device_wedged() instead of xe_device_io_blocked() when returning cancellation errors, causing state inconsistency.

Pre-existing issues:
- [High] Directly calling probe during AER slot reset bypasses driver core error handling, leading to double-remove and crashes if it fails.
--

commit 1cbbd9c3fc1b9bad858f6195cfff20e523edc2ca
Author: Arvind Yadav <arvind.yadav@intel.com>

drm/xe: Separate AER reset state from device wedging

This commit modifies the PCI error recovery path (AER) to use a temporary
in_reset state instead of the permanent wedged flag to block driver I/O.
It introduces the xe_device_io_blocked() helper to check both conditions,
replacing xe_device_wedged() in multiple places where temporary blocks
should also apply.

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 5c4733da385c5..3c3fe4928fa20 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;
>  	}

[ ... ]

[Severity: Medium]
In guc_ct_send_recv(), the code still checks the old xe_device_wedged() state
when returning cancellation errors:

	if (g2h_fence.fail) {
		if (g2h_fence.cancel) {
			xe_gt_dbg(gt, "H2G request %#x canceled!\n", action[0]);
			ret = xe_device_wedged(ct_to_xe(ct)) ? -ENOTRECOVERABLE : -ECANCELED;
			goto unlock;
		}

Does this leave a state inconsistency during a PCI AER reset? If an in-flight
H2G request is cancelled because of a temporary IO block, xe_device_wedged()
will evaluate to false. This means the caller receives -ECANCELED instead of
-ENOTRECOVERABLE. Should this check be updated to use xe_device_io_blocked()
as well?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/xe_pci_error.c
> index 79ce0c671549a..d82256d8721f3 100644
> --- a/drivers/gpu/drm/xe/xe_pci_error.c
> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
[ ... ]
> @@ -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);
>  

[Severity: High]
This is a pre-existing issue, but does manually calling probe here bypass
driver core error handling?

If pdev->driver->probe(pdev, ent) fails during xe_pci_error_slot_reset(), it
returns PCI_ERS_RESULT_DISCONNECT. The PCI AER core will then unbind the
driver and call pdev->driver->remove(pdev). 

Since the failing probe likely already called xe_device_remove(xe) on its
error path, wouldn't this lead to a double remove and potential crashes?

> @@ -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;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827101801.1247654-1-arvind.yadav@intel.com?part=2