Hi Bjorn, Thanks for the patch :-)
On Thu Oct 23, 2025 at 8:25 PM CEST, Bjorn Helgaas wrote: > On Thu, Oct 23, 2025 at 01:06:26PM -0500, Bjorn Helgaas wrote: >> From: Bjorn Helgaas <[email protected]> >> >> f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree >> platforms") enabled Clock Power Management and L1 PM Substates, but those >> features depend on CLKREQ# and possibly other device-specific >> configuration. We don't know whether CLKREQ# is supported, so we shouldn't >> blindly enable Clock PM and L1 PM Substates. >> >> Enable only ASPM L0s and L1, and only when both ends of the link advertise >> support for them. >> >> Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for >> devicetree platforms") >> Reported-by: Christian Zigotzky <[email protected]> >> Link: >> https://lore.kernel.org/r/[email protected]/ >> Reported-by: FUKAUMI Naoki <[email protected]> >> Closes: >> https://lore.kernel.org/r/[email protected]/ >> Reported-by: Herve Codina <[email protected]> >> Link: https://lore.kernel.org/r/[email protected]/ >> Reported-by: Diederik de Haas <[email protected]> >> Link: https://lore.kernel.org/r/[email protected]/ >> Signed-off-by: Bjorn Helgaas <[email protected]> >> Tested-by: FUKAUMI Naoki <[email protected]> > > Provisionally applied to pci/for-linus, hoping to make v6.18-rc3. > > Happy to add any testing reports or amend as needed. My build with your patch (on top of 6.18-rc2) just finished, so I installed it and rebooted into it. Happy to report that everything seems to be working fine and I can't find any errors, warnings or other messages with 'nvme' in dmesg that indicate sth could be wrong. IOW: it does indeed fix the issue I reported earlier. So feel free to add my: Tested-by: Diederik de Haas <[email protected]> Cheers, Diederik >> --- >> I intend this for v6.18-rc3. >> >> I think it will fix the issues reported by Diederik and FUKAUMI Naoki (both >> on Rockchip). I hope it will fix Christian's report on powerpc, but don't >> have confirmation. I think the performance regression Herve reported is >> related, but this patch doesn't seem to fix it. >> >> FUKAUMI Naoki's successful testing report: >> https://lore.kernel.org/r/[email protected]/ >> --- >> drivers/pci/pcie/aspm.c | 34 +++++++++------------------------- >> 1 file changed, 9 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index 7cc8281e7011..79b965158473 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -243,8 +243,7 @@ struct pcie_link_state { >> /* Clock PM state */ >> u32 clkpm_capable:1; /* Clock PM capable? */ >> u32 clkpm_enabled:1; /* Current Clock PM state */ >> - u32 clkpm_default:1; /* Default Clock PM state by BIOS or >> - override */ >> + u32 clkpm_default:1; /* Default Clock PM state by BIOS */ >> u32 clkpm_disable:1; /* Clock PM disabled */ >> }; >> >> @@ -376,18 +375,6 @@ static void pcie_set_clkpm(struct pcie_link_state >> *link, int enable) >> pcie_set_clkpm_nocheck(link, enable); >> } >> >> -static void pcie_clkpm_override_default_link_state(struct pcie_link_state >> *link, >> - int enabled) >> -{ >> - struct pci_dev *pdev = link->downstream; >> - >> - /* For devicetree platforms, enable ClockPM by default */ >> - if (of_have_populated_dt() && !enabled) { >> - link->clkpm_default = 1; >> - pci_info(pdev, "ASPM: DT platform, enabling ClockPM\n"); >> - } >> -} >> - >> static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist) >> { >> int capable = 1, enabled = 1; >> @@ -410,7 +397,6 @@ static void pcie_clkpm_cap_init(struct pcie_link_state >> *link, int blacklist) >> } >> link->clkpm_enabled = enabled; >> link->clkpm_default = enabled; >> - pcie_clkpm_override_default_link_state(link, enabled); >> link->clkpm_capable = capable; >> link->clkpm_disable = blacklist ? 1 : 0; >> } >> @@ -811,19 +797,17 @@ static void >> pcie_aspm_override_default_link_state(struct pcie_link_state *link) >> struct pci_dev *pdev = link->downstream; >> u32 override; >> >> - /* For devicetree platforms, enable all ASPM states by default */ >> + /* For devicetree platforms, enable L0s and L1 by default */ >> if (of_have_populated_dt()) { >> - link->aspm_default = PCIE_LINK_STATE_ASPM_ALL; >> + if (link->aspm_support & PCIE_LINK_STATE_L0S) >> + link->aspm_default |= PCIE_LINK_STATE_L0S; >> + if (link->aspm_support & PCIE_LINK_STATE_L1) >> + link->aspm_default |= PCIE_LINK_STATE_L1; >> override = link->aspm_default & ~link->aspm_enabled; >> if (override) >> - pci_info(pdev, "ASPM: DT platform, >> enabling%s%s%s%s%s%s%s\n", >> - FLAG(override, L0S_UP, " L0s-up"), >> - FLAG(override, L0S_DW, " L0s-dw"), >> - FLAG(override, L1, " L1"), >> - FLAG(override, L1_1, " ASPM-L1.1"), >> - FLAG(override, L1_2, " ASPM-L1.2"), >> - FLAG(override, L1_1_PCIPM, " PCI-PM-L1.1"), >> - FLAG(override, L1_2_PCIPM, " PCI-PM-L1.2")); >> + pci_info(pdev, "ASPM: default states%s%s\n", >> + FLAG(override, L0S, " L0s"), >> + FLAG(override, L1, " L1")); >> } >> } >> >> -- >> 2.43.0 >>
