Re: [PATCH v10 09/10] drm/xe/pci: Introduce PCIe Function Level Reset

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing hardware quiescence before PCI reset leads to potential system panics.
--

--- Patch [9]: [PATCH v10 09/10] drm/xe/pci: Introduce PCIe Function Level Reset ---
commit f3c8299b36fc1f6ea55ecfbcd781c310e60e62ab
Author: Raag Jadav <[email protected]>

drm/xe/pci: Introduce PCIe Function Level Reset

This commit introduces PCIe Function Level Reset (FLR) support which 
re-initializes hardware state without the need for reloading the driver 
from userspace.

> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/xe_pci_error.c
> index 48466d726eae6..81c4e55fae8f0 100644
> --- a/drivers/gpu/drm/xe/xe_pci_error.c
> +++ b/drivers/gpu/drm/xe/xe_pci_error.c

> [ ... ]

> @@ -142,9 +189,90 @@ static void xe_pci_error_resume(struct pci_dev *pdev)
> +static void xe_pci_reset_prepare(struct pci_dev *pdev)
> +{
> +	struct xe_device *xe = pdev_to_xe_device(pdev);
> +	int err;
> +
> +	err = xe_pci_reset_skip(xe);
> +	if (err) {
> +		xe_err(xe, "PCIe FLR not supported\n");
> +		goto wedge;
> +	}
> +
> +	err = xe_device_wedged(xe);
> +	if (err)
> +		xe_err(xe, "PCIe FLR failed, device in unexpected state\n");
> +
> +wedge:
> +	/* Wedge the device to prevent userspace access but don't send the event yet */
> +	xe_device_wedged_get(xe);
> +	if (err)
> +		return;

[Severity: High]
Since xe_pci_reset_prepare() is a void callback, the PCI core will proceed
with the physical reset unconditionally even if we return early here. 

If xe_pci_reset_skip(xe) returns true, we jump to the wedge label with err
evaluating to true. This causes the function to return immediately after
xe_device_wedged_get(xe).

Does this skip the hardware quiescence (xe_device_suspend) while the PCI
core still performs the physical reset? 

If a user triggers a sysfs reset on a device that skips FLR, could this
leave VRAM mapped and DMA active during the reset, resulting in PCIe
Unsupported Requests and fatal AER faults that panic the system?

> +
> +	/*
> +	 * The hardware could be in corrupted state and access unreliable, but we try to
> +	 * update data structures and cleanup any pending work to avoid side effects during
> +	 * PCIe FLR. This will be similar to system suspend flow but without eviction.
> +	 */
> +	err = xe_device_suspend(xe, true);
> +	if (err) {
> +		xe_err(xe, "Failed to prepare for PCIe FLR\n");
> +		return;
> +	}
> +
> +	xe->flr_prepared = true;
> +	xe_info(xe, "Prepared for PCIe FLR\n");
> +}

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