linux-kernel - Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2wv7662f23seqtifn4zkud6xlecfhpeso4rn72syn6q2ts6sm5@cllw6gyv2xwx>
Date: Tue, 21 Oct 2025 18:05:07 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	Christian Zigotzky <chzigotzky@...osoft.de>, FUKAUMI Naoki <naoki@...xa.com>, linux-rockchip@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH] PCI/ASPM: Enable only L0s and L1 for devicetree platforms

On Tue, Oct 21, 2025 at 09:31:32AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 20, 2025 at 05:12:07PM -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 Substates, but that
> > caused regressions because these features depend on CLKREQ#, and not all
> > devices and form factors support it.
> 
> I believe we haven't concluded that CLKREQ# is the cluprit here. It is probably
> the best bet, but there could be the device specific issues as well.
> 
> > 
> > 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/
> 
> Closes?
> 
> > Reported-by: FUKAUMI Naoki <naoki@...xa.com>
> > Link: https://lore.kernel.org/r/22594781424C5C98+22cb5d61-19b1-4353-9818-3bb2b311da0b@radxa.com/
> 
> Same here.
> 
> > ---
> > 
> > Mani, not sure what you think we should do here.  Here's a stab at it as a
> > strawman and in case anybody can test it.
> > 
> 
> Thanks Bjorn! I had a similar change slated to be sent post Diwali, but you beat
> me to it.
> 
> > Not sure about the message log message.  Maybe OK for testing, but might be
> > overly verbose ultimately.
> > 
> 
> Let's have it for sometime, so we have some clue when someone reports issue with
> ASPM.
> 
> > ---
> >  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..dbc74cc85bcb 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;
> 
> Not sure if it is worth setting these states conditionally. Link state
> enablement code should make use of the cached ASPM cap in 'link->aspm_capable'.
> 

I see the point now. Without the check, we falsely claim that the ASPM states
are getting enabled. But we cannot be sure until the register write to LNKCTL
happens, which will depend on many factors (own device capability,
upstream/downstream port capability,...)

To avoid ambiguity, can we reword the log to something like,

	"ASPM: Overridding default states %s%s\n"

- Mani

-- 
மணிவண்ணன் சதாசிவம்