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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250716205601.GA2555277@bhelgaas>
Date: Wed, 16 Jul 2025 15:56:01 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: manivannan.sadhasivam@....qualcomm.com,
	"Rafael J. Wysocki" <rafael@...nel.org>
Cc: Jeff Johnson <jjohnson@...nel.org>,
	Manivannan Sadhasivam <mani@...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 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'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.

>  	return __pci_enable_link_state(pdev, state, false);
>  }