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 <[email protected]> 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/[email protected]?part=5
