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: <67274gnjp4qy4h3bcawey2edmjiuufdbm262q2qxgcc76dwlic@hdjxqczr54nt>
Date: Sat, 6 Sep 2025 20:23:52 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	David Box <david.e.box@...ux.intel.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 03:55:42PM GMT, 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.
> 

Why? I thought your concern was with the callers of this API. Since that is
taken care, do you have any other concerns?

> 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.
> 
> > And the other API, pci_enable_link_state() doesn't have a caller for now,
> > but will be used by the 'atheros' WLAN drivers in the subsequent commits.
> > 
> > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> 
> This tag sound like I'm endorsing this approach which is not the case. I'd 
> prefer separate functions for each use case, setting aspm_default and 
> another for the enable state.
> 

Sorry, I misunderstood then. I'll drop this tag.

- Mani

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