| Message ID | 20260930144650.3701516-3-stefan.roese@mailbox.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Fix Renesas uPD720201 hang after the RCB Link Control write | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] pci/aspm: cached saved state not updated after clearing ASPMC -- commit d667f086616b3d0999815c8db89e04e4d40aea30 Author: Stefan Roese <stefan.roese@mailbox.org> PCI/ASPM: Clear ASPM Control on links without common ASPM support This commit ensures ASPM Control is cleared for all functions on a link that lacks common ASPM support, processing downstream components first. This prevents devices that enable ASPM by default from keeping it enabled when connected to a port that does not support it. > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index f8e64971c6c3d..13d7a6d3f6ad3 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c [ ... ] > @@ -947,11 +959,17 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) > > /* > * If ASPM not supported, don't mess with the clocks and link, > - * bail out now. > + * bail out now. A device may still come out of reset with ASPM > + * Control set, which the rest of this code never touches for > + * such a link. Clear it, downstream component first. > */ > if (!(parent->aspm_l0s_support && child->aspm_l0s_support) && > - !(parent->aspm_l1_support && child->aspm_l1_support)) > + !(parent->aspm_l1_support && child->aspm_l1_support)) { > + list_for_each_entry(fn, &linkbus->devices, bus_list) > + pcie_aspm_clear_aspmc(fn); > + pcie_aspm_clear_aspmc(parent); [Severity: High] Does this bypass updating the parent's cached saved state? By clearing the ASPM Control in hardware but returning immediately without updating the cache (e.g., via pci_update_aspm_saved_state()), it appears the parent bridge's saved_config_space will retain the stale ASPMC enabled value. If a non-ASPM-capable device is hot-added, and the system undergoes an AER or DPC reset before any power management event forces a state save, could pci_restore_state() use this stale cache to erroneously re-enable ASPM on the parent port? This might lead to completion timeouts or link hangs. > return; > + }
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index f8e64971c6c3..13d7a6d3f6ad 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -931,6 +931,18 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link) } } +static void pcie_aspm_clear_aspmc(struct pci_dev *pdev) +{ + u16 lnkctl; + + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &lnkctl); + if (!(lnkctl & PCI_EXP_LNKCTL_ASPMC)) + return; + + pci_info(pdev, "ASPM: link supports no ASPM, clearing ASPM Control\n"); + pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC); +} + static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) { struct pci_dev *child = link->downstream, *parent = link->pdev; @@ -947,11 +959,17 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) /* * If ASPM not supported, don't mess with the clocks and link, - * bail out now. + * bail out now. A device may still come out of reset with ASPM + * Control set, which the rest of this code never touches for + * such a link. Clear it, downstream component first. */ if (!(parent->aspm_l0s_support && child->aspm_l0s_support) && - !(parent->aspm_l1_support && child->aspm_l1_support)) + !(parent->aspm_l1_support && child->aspm_l1_support)) { + list_for_each_entry(fn, &linkbus->devices, bus_list) + pcie_aspm_clear_aspmc(fn); + pcie_aspm_clear_aspmc(parent); return; + } /* Configure common clock before checking latencies */ pcie_aspm_configure_common_clock(link);
pcie_aspm_cap_init() returns early when the two ends of a link share no ASPM state. It then never touches Link Control, so ASPM Control keeps whatever the device came out of reset with. A device that enables ASPM by default, against a port that cannot do it, keeps ASPM enabled, which the spec does not allow. The Renesas uPD720201 xHCI (1912:0014) is such a device: it resets with LnkCtl 0x0003 (L0s and L1 enabled) and sits behind the CPM Root Port of AMD Versal, which supports no ASPM. It only works because the chip clears ASPM Control itself during its firmware download, and it stops doing so once the host has written Link Control. Clear ASPM Control on every function of such a link, downstream component first, and only where it is set. Before (lspci -vvv -s 01:00.0): LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes, LnkDisable- CommClk- After: pci 0000:01:00.0: ASPM: link supports no ASPM, clearing ASPM Control LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk- Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> --- drivers/pci/pcie/aspm.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)