From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/xe: Suppress Surprise Link Down on non-hotplug device Date: Mon, 25 May 2026 21:57:10 +1000 Message-ID: In-Reply-To: <20260520113351.171119-12-mallesh.koujalagi@intel.com> References: <20260520113351.171119-7-mallesh.koujalagi@intel.com> <20260520113351.171119-12-mallesh.koujalagi@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The rationale is sound: a cold reset drops the PCIe link, and on non-hotplu= g slots this triggers a spurious Surprise Link Down AER event that defeats = the recovery. Masking SLD on the USP before requesting the cold reset preve= nts this. **Concern: SLD mask is never restored** ```c pci_read_config_dword(usp, aer_cap + PCI_ERR_UNCOR_MASK, &aer_uncorr_mask); aer_uncorr_mask |=3D PCI_ERR_UNC_SURPDN; pci_write_config_dword(usp, aer_cap + PCI_ERR_UNCOR_MASK, aer_uncorr_mask); ``` If the cold reset succeeds, hardware presumably reinitializes the AER regis= ters, so the mask is implicitly cleared. But if the cold reset is never per= formed (userspace ignores the uevent) or fails, the SLD mask remains perman= ently applied on the USP. This means legitimate surprise link-down events w= ould be silently masked for the remaining uptime of the system. Consider do= cumenting this trade-off explicitly, or adding a comment explaining why res= toration isn't needed. **`pcie_slot_is_hotplug_capable()` =E2=80=94 correct but worth a comment** ```c static bool pcie_slot_is_hotplug_capable(struct pci_dev *usp) { struct pci_dev *root_port =3D pci_upstream_bridge(usp); ``` The function name says "slot" but it takes the USP and walks up to the root= port. This is correct (the slot the USP is plugged into is owned by the ro= ot port), but the naming could confuse readers. A brief comment like `/* Th= e hotplug slot is on usp's parent (root port) */` would help. **Hotplug capability check:** ```c return (sltcap & (PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_PCP)) =3D=3D (PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_PCP); ``` Checking both Hot-Plug Capable and Power Controller Present is correct =E2= =80=94 both must be present for the slot to support power cycling that woul= d make a surprise link-down expected. --- Generated by Claude Code Patch Reviewer