Re: [PATCH v3] drm/xe/pci_error: Allow debugging PCI errors

[sashiko-bot@kernel.org]

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

New issues:
- [High] The device is unconditionally wedged and disconnected for fully recoverable, non-fatal errors when XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET is set.
- [Medium] The patch fails to prevent bus resets for fatal PCI errors, contradicting the commit message.

Pre-existing issues:
- [Medium] Missing idempotency check before wedging execution queues leads to reference leaks.
--

--- Patch [1]: [PATCH v3] drm/xe/pci_error: Allow debugging PCI errors ---
commit 6e197279f5660d6825fd4f15d304684b9f2531ca
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/pci_error: Allow debugging PCI errors

Update PCI error handling path to disconnect the device when
XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET is set. This prevents bus
reset and preserves the device state so that it is available to
the user for debugging.

> Update PCI error handling path to disconnect the device when
> XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET is set. This prevents bus
> reset and preserves the device state so that it is available to
> the user for debugging.

[Severity: Medium]
Will fatal PCIe errors still trigger a reset and destroy the hardware state?

In xe_pci_error_detected() for pci_channel_io_frozen, it unconditionally 
returns PCI_ERS_RESULT_NEED_RESET:

drivers/gpu/drm/xe/xe_pci_error.c:xe_pci_error_detected() {
...
	case pci_channel_io_frozen:
		prepare_device_for_reset(pdev);
		return PCI_ERS_RESULT_NEED_RESET;
...
}

Since this path bypasses xe_pci_error_mmio_enabled() entirely, does the new 
debug feature fail to prevent bus resets for fatal errors?

> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/xe_pci_error.c
> index e41af2ac7f23b..98aa26275ad66 100644
> --- a/drivers/gpu/drm/xe/xe_pci_error.c
> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
> @@ -83,6 +83,12 @@ static pci_ers_result_t xe_pci_error_mmio_enabled(struct pci_dev *pdev)
>  	xe_info(xe, "PCI error: MMIO enabled\n");
>  	action = xe_ras_process_errors(xe);
>  
> +	/* User wants to debug the error, prevent reset */
> +	if (xe->wedged.mode == XE_WEDGED_MODE_UPON_ANY_HANG_NO_RESET) {
> +		xe_device_declare_wedged(xe);

[Severity: Medium]
This is a pre-existing issue, but does calling xe_device_declare_wedged()
multiple times leak references?

xe_device_declare_wedged() invokes xe_gt_declare_wedged() unconditionally
outside its atomic block:

drivers/gpu/drm/xe/xe_device.c:xe_device_declare_wedged() {
...
	for_each_gt(gt, xe, id)
		xe_gt_declare_wedged(gt);
...
}

This calls down to xe_guc_submit_wedge():

drivers/gpu/drm/xe/xe_guc_submit.c:xe_guc_submit_wedge() {
...
		xa_for_each(&guc->submission_state.exec_queue_lookup, index, q)
			if (xe_exec_queue_get_unless_zero(q))
				set_exec_queue_wedged(q);
...
}

This iterates over exec_queue_lookup and unconditionally takes an extra
reference on each queue every time it's called, without checking if the queue
was already wedged. Since guc_submit_fini() drops only one reference per
wedged queue, does this leak exec_queue objects if xe_device_declare_wedged()
is called sequentially?

> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}

[Severity: High]
Does this unconditionally wedge the device and force a PCI disconnect even
for fully recoverable errors?

If a correctable error occurs, xe_ras_process_errors() will return 
XE_RAS_RECOVERY_ACTION_RECOVERED.

Because we check xe->wedged.mode here without verifying if the action returned
by xe_ras_process_errors() actually required a reset, will we ignore the fact
that no reset was going to happen and unnecessarily break device availability
for benign hardware events?

> +
>  	return ras_action_to_pci_result(pdev, action);
>  }

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