| 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: <DDPYVBSYR01V.1OSGKL2X8LT82@cknow-tech.com>
Date: Thu, 23 Oct 2025 21:59:41 +0200
From: "Diederik de Haas" <diederik@...ow-tech.com>
To: "Bjorn Helgaas" <helgaas@...nel.org>, <linux-pci@...r.kernel.org>
Cc: "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>, "Dragan Simic" <dsimic@...jaro.org>,
<linuxppc-dev@...ts.ozlabs.org>, <linux-rockchip@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, "Bjorn Helgaas" <bhelgaas@...gle.com>
Subject: Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree
platforms
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 <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>
>
> 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 <diederik@...ow-tech.com>
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/4B275FBD7B747BE6+a3e5b367-9710-4b67-9d66-3ea34fc30866@radxa.com/
>> ---
>> 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
>>
Powered by blists - more mailing lists