linux-kernel - Re: [PATCH 2/6] PCI/ASPM: Transition the device to D0 (if required) inside pci_enable_link_state

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 for Android: free password hash cracker in your pocket

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

Message-ID: <7o23kvvetjcohrucjhgkonrkkunykybce7q6tlfycbb6pafg4y@lpxvbepupemp>
Date: Thu, 17 Jul 2025 13:06:05 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Jeff Johnson <jjohnson@...nel.org>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Nirmal Patel <nirmal.patel@...ux.intel.com>, Jonathan Derrick <jonathan.derrick@...ux.dev>, 
	linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org, ath12k@...ts.infradead.org, 
	ath11k@...ts.infradead.org, ath10k@...ts.infradead.org, ilpo.jarvinen@...ux.intel.com, 
	linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Subject: Re: [PATCH 2/6] PCI/ASPM: Transition the device to D0 (if required)
 inside pci_enable_link_state_locked() API

On Wed, Jul 16, 2025 at 03:56:01PM GMT, Bjorn Helgaas wrote:
> On Wed, Jul 16, 2025 at 06:26:21PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > 
> > Both of the current callers of the pci_enable_link_state_locked() API
> > transition the device to D0 before calling. This aligns with the PCIe spec
> > r6.0, sec 5.5.4:
> > 
> > "If setting either or both of the enable bits for PCI-PM L1 PM Substates,
> > both ports must be configured as described in this section while in D0."
> > 
> > But it looks redundant to let the callers transition the device to D0. So
> > move the logic inside the API and perform D0 transition only if the PCI-PM
> > L1 Substates are getting enabled.
> 
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1474,13 +1474,20 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> >   * Note that if the BIOS didn't grant ASPM control to the OS, this does
> >   * nothing because we can't touch the LNKCTL register.
> >   *
> > - * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> > - * PCIe r6.0, sec 5.5.4.
> > + * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
> > + * are getting enabled.
> >   *
> >   * Return: 0 on success, a negative errno otherwise.
> >   */
> >  int pci_enable_link_state(struct pci_dev *pdev, int state)
> >  {
> > +	/*
> > +	 * Ensure the device is in D0 before enabling PCI-PM L1 PM Substates, per
> > +	 * PCIe r6.0, sec 5.5.4.
> > +	 */
> > +	if (FIELD_GET(PCIE_LINK_STATE_L1_SS_PCIPM, state))
> > +		pci_set_power_state(pdev, PCI_D0);
> 
> This is really just a move, not new code, but this niggles at me a
> little bit because my impression is that pci_set_power_state() doesn't
> guarantee that the device *stays* in the given state.
> 
> Rafael, is there a get/put interface we should be wrapping this with
> instead?
> 

I don't quite understand this statement. A device cannot transition itself to
any D-states without host software intervention. So only host software should
intiate the transition. So are you saying that this API could be used by other
entities to change the device state? So you want to use some lock to prevent
callers from racing aganist each other?

I believe the current users of this API doesn't use any locks and just go by the
fact that the device stays in the give state. It does look racy, but seems to be
working fine so far. Obviously, the client driver need to ensure that it doesn't
create any race within itself. But the race could exist between the PCI core and
the driver theoretically.

> I'm also not sure it's worth the FIELD_GET().  This should be a
> low-frequency operation and making the power state dependent on the
> exact "state" makes more paths to worry about.
> 

Are you worrying about the usage of FIELD_GET() to check the ASPM state or the
existence of the check itself?

- Mani

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