| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <da79f38f-fdb9-0101-67cc-144ef8d6e1d1@manjaro.org>
Date: Thu, 23 Oct 2025 20:27:25 +0200
From: "Dragan Simic" <dsimic@...jaro.org>
To: "Bjorn Helgaas" <helgaas@...nel.org>
Cc: linux-pci@...r.kernel.org, "Manivannan Sadhasivam" <manivannan.sadhasivam@....qualcomm.com>, "Christian Zigotzky" <chzigotzky@...osoft.de>, "FUKAUMI Naoki" <naoki@...xa.com>, "Herve Codina" <herve.codina@...tlin.com>, "Diederik de Haas" <diederik@...ow-tech.com>, linuxppc-dev@...ts.ozlabs.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, "Bjorn Helgaas" <bhelgaas@...gle.com>, "Shawn Lin" <shawn.lin@...k-chips.com>
Subject: Re: [PATCH] PCI/ASPM: Enable only L0s and L1
for devicetree platforms
Hello Bjorn,
On Thursday, October 23, 2025 20:06 CEST, Bjorn Helgaas <helgaas@...nel.org> wrote:
> From: Bjorn Helgaas <bhelgaas@...gle.com>
>
> 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 <chzigotzky@...osoft.de>
> Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045-a1b04908051a@xenosoft.de/
> Reported-by: FUKAUMI Naoki <naoki@...xa.com>
> Closes: https://lore.kernel.org/r/22594781424C5C98+22cb5d61-19b1-4353-9818-3bb2b311da0b@radxa.com/
> Reported-by: Herve Codina <herve.codina@...tlin.com>
> Link: https://lore.kernel.org/r/20251015101304.3ec03e6b@bootlin.com/
> Reported-by: Diederik de Haas <diederik@...ow-tech.com>
> Link: https://lore.kernel.org/r/DDJXHRIRGTW9.GYC2ULZ5WQAL@cknow-tech.com/
> Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
> Tested-by: FUKAUMI Naoki <naoki@...xa.com>
> ---
> 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/4B275FBD7B747BE6+a3e5b367-9710-4b67-9d66-3ea34fc30866@radxa.com/
I'm more than happy with the way ASPM patches for DT platforms and
Rockchip SoCs in particular are unfolding! Admittedly, we've had
a rough start with the blanket enabling of ASPM, which followed the
theory, but the theory often differs from practice, so the combined
state of this and associated patches from Shawn should be fine.
Thank you very much for all the effort that included quite a lot
of back and forth, and please feel free to include
Acked-by: Dragan Simic <dsimic@...jaro.org>
> ---
> 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"));
> }
> }
Powered by blists - more mailing lists