linux-kernel - Re: [PATCH v2 2/8] PCI/ASPM: Fix the behavior of pci_enable_link

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: <rci6ku374tvbco2leey3ccyjf4r6bfa6mdksasn2c26v2a5ydc@xqbccsco3x5o>
Date: Sat, 6 Sep 2025 21:07:41 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: David Box <david.e.box@...ux.intel.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Nirmal Patel <nirmal.patel@...ux.intel.com>, 
	Jonathan Derrick <jonathan.derrick@...ux.dev>, Jeff Johnson <jjohnson@...nel.org>, linux-pci@...r.kernel.org, 
	LKML <linux-kernel@...r.kernel.org>, linux-arm-msm@...r.kernel.org, linux-wireless@...r.kernel.org, 
	ath12k@...ts.infradead.org, ath11k@...ts.infradead.org, ath10k@...ts.infradead.org, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v2 2/8] PCI/ASPM: Fix the behavior of
 pci_enable_link_state*() APIs

On Tue, Aug 26, 2025 at 02:24:05PM GMT, David Box wrote:
> On Tue, Aug 26, 2025 at 03:55:42PM +0300, Ilpo Järvinen wrote:
> > +David
> > 
> > On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote:
> > 
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > > 
> > > pci_enable_link_state() and pci_enable_link_state_locked() APIs are
> > > supposed to be symmectric with pci_disable_link_state() and
> > > pci_disable_link_state_locked() APIs.
> > > 
> > > But unfortunately, they are not symmetric. This behavior was mentioned in
> > > the kernel-doc of these APIs:
> > > 
> > > " Clear and set the default device link state..."
> > > 
> > > and
> > > 
> > > "Also note that this does not enable states disabled by
> > > pci_disable_link_state()"
> > > 
> > > These APIs won't enable all the states specified by the 'state' parameter,
> > > but only enable the ones not previously disabled by the
> > > pci_disable_link_state*() APIs. But this behavior doesn't align with the
> > > naming of these APIs, as they give the impression that these APIs will
> > > enable all the specified states.
> > > 
> > > To resolve this ambiguity, allow these APIs to enable the specified states,
> > > regardeless of whether they were previously disabled or not. This is
> > > accomplished by clearing the previously disabled states from the
> > > 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also,
> > > reword the kernel-doc to reflect this behavior.
> > > 
> > > The current callers of pci_enable_link_state_locked() APIs (vmd and
> > > pcie-qcom) did not disable the ASPM states before calling this API. So it
> > > is evident that they do not depend on the previous behavior of this API and
> > > intend to enable all the specified states.
> > 
> > While it might be "safe" in the sense that ->aspm_disable is not set by 
> > anything, I'm still not sure if overloading this function for two 
> > different use cases is a good idea.
> > 
> > I'd like to hear David's opinion on this as he grasps the ->aspm_default 
> > vs ->aspm_disable thing much better than I do.
> 
> The concern I see is that this would override the init-time blacklist which is
> set in pcie_aspm_sanity_check() and only consulted during initialization.
> __pci_disable_link_state() doesn't do this. It ORs in bits to aspm_disable.  By
> contrast, this change would clear bits from aspm_disable in the enable path,
> which allows ASPM to be enabled on links that pcie_aspm_sanity_check()
> determined should be disabled.
> 
> But I noticed the sysfs path, aspm_attr_store_common(), already permits this
> override. That may be unintentional though since the comment in
> pcie_aspm_sanity_check() implies the blacklist can only be overridden with
> pcie_aspm=force. At minimum, that needs to be clarified.
> 

Thanks for pointing out the blacklist devices issue. I have no concerns with
pcie-qcom as we are going to drop the pci_enable_link_state_locked() API anyway
from it. But I'm not sure about VMD as one may still connect pre 1.1 device to
it and observe issues.

So I'll create a separate API for this new behavior and use it with ath drivers
only since they know what kind of devices they are dealing with and since they
were changing the LNKCTL manually, there shouldn't be any issue.

- Mani

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