Re: [PATCH v8 5/6] drm/xe: Suppress Surprise Link Down on device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Masking Surprise Link Down on the Upstream Switch Port violates PCIe AER specifications.
- [High] Directly mutating AER registers without verifying OS native AER ownership.
- [High] Permanent masking of PCI_ERR_UNC_SURPDN with no restoration mechanism.
- [High] Blind PCIe hierarchy traversal modifies unrelated upstream bridges.
- [Medium] Data race due to unprotected read-modify-write on a shared AER mask register.
--

commit 7a2dc1d13a9370ceb4dd62a9d15aa26d2212058a
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
drm/xe: Suppress Surprise Link Down on device

This commit attempts to prevent fatal AER events during a recovery-induced
cold reset. It masks the Surprise Link Down (SLD) bit in the upstream
port's AER Uncorrectable Error Mask register before triggering the reset.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 93a56a0269f1e..15c2fa0d323a6 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -209,8 +209,57 @@ static enum xe_ras_recovery_action handle_core_compute_errors(struct xe_ras_erro
>  	return XE_RAS_RECOVERY_ACTION_RECOVERED;
>  }
>  
> +static struct pci_dev *find_usp_dev(struct pci_dev *pdev)
> +{
> +	struct pci_dev *vsp;
> +
> +	/*
> +	 * Device Hierarchy:
> +	 *
> +	 * Upstream Switch Port (USP) --> Virtual Switch Port (VSP) --> SGunit (GPU endpoint)
> +	 */
> +	vsp = pci_upstream_bridge(pdev);
> +	if (!vsp)
> +		return NULL;
> +
> +	return pci_upstream_bridge(vsp);

[Severity: High]
Does this blind traversal safely handle diverse PCIe topologies? If the GPU
is connected to a standard motherboard switch, passed through a hypervisor, or
connected directly to a Root Port, might this return an unrelated bridge
device owned by the host system? Should this verify the Vendor ID or Device
ID of the USP before proceeding?

> +}
> +
> +#ifdef CONFIG_PCIEAER
> +static void pcie_suppress_surprise_link_down(struct pci_dev *usp)
> +{
> +	u32 aer_uncorr_mask;
> +	u16 aer_cap;
> +
> +	aer_cap = usp->aer_cap;
> +	if (!aer_cap) {
> +		dev_dbg(&usp->dev,
> +			"AER capability not present\n");
> +		return;
> +	}
> +
> +	pci_read_config_dword(usp, aer_cap + PCI_ERR_UNCOR_MASK, &aer_uncorr_mask);

[Severity: Medium]
Since the Upstream Switch Port is a shared bridge managed by the pcieport
driver, could this lockless read-modify-write sequence race with the AER
driver handling other hotplug events, or with other endpoints on the same
switch experiencing concurrent errors?

> +	aer_uncorr_mask |= PCI_ERR_UNC_SURPDN;

[Severity: High]
Is there a mechanism to unmask this bit after the cold reset completes? If
the Surprise Link Down bit is set here but never cleared during driver
probe, resume, or a post-reset callback, will it permanently disable AER
reporting for genuine link failures on this port?

> +	pci_write_config_dword(usp, aer_cap + PCI_ERR_UNCOR_MASK, aer_uncorr_mask);

[Severity: High]
According to the PCIe Base Specification, Surprise Down errors are exclusively
detected and reported by Downstream Ports. On Upstream Ports, the Surprise
Down Error Status and Mask bits are reserved and hardwired to 0. Does masking
PCI_ERR_UNC_SURPDN on the Upstream Switch Port actually prevent the downstream
port (the VSP or Root Port) that detects the link drop from generating the
fatal AER event?

[Severity: High]
Is it safe to mutate the AER mask registers directly without verifying OS
native AER ownership? On platforms where firmware retains control of AER
(such as ACPI APEI Firmware First setups), could mutating these registers
directly from the OS conflict with platform management firmware? Is a check
like pcie_aer_is_native(usp) needed here?

> +	dev_dbg(&usp->dev, "Surprise Link Down masked for cold reset\n");
> +}
> +#endif /* CONFIG_PCIEAER */

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612080722.26726-8-mallesh.koujalagi@intel.com?part=5