[v1] PCI/ASPM: Fix pci_enable_link_state*() APIs behavior

[PATCH 2/6] PCI/ASPM: Transition the device to D0 (if required) inside pci_enable_link_state_locked() API

Posted by Manivannan Sadhasivam via B4 Relay 2 months, 3 weeks ago

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.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.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c |  5 -----
 drivers/pci/controller/vmd.c           |  5 -----
 drivers/pci/pcie/aspm.c                | 22 ++++++++++++++++++----
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f856550bcfa1ce09962ba9c086d117de05..204f87607c0bc1ce31299aa5a5763b564ddeda29 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1018,11 +1018,6 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 
 static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
 {
-	/*
-	 * Downstream devices need to be in D0 state before enabling PCI PM
-	 * substates.
-	 */
-	pci_set_power_state_locked(pdev, PCI_D0);
 	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
 
 	return 0;
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 8df064b62a2ff3e22dd8507a66783ca6c6a8b777..cf11036dd20cbae5d403739b226452ce17c4cb7f 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -765,11 +765,6 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	pci_info(pdev, "VMD: Default LTR value set by driver\n");
 
 out_state_change:
-	/*
-	 * Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
-	 * PCIe r6.0, sec 5.5.4.
-	 */
-	pci_set_power_state_locked(pdev, PCI_D0);
 	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
 	return 0;
 }
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ec63880057942cef9ffbf3f67dcd87ee3d2df17d..c56553de953c158cf9e8bf54c6b358a9a81a2691 100644
--- a/drivers/pci/pcie/aspm.c
+++ 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);
+
 	return __pci_enable_link_state(pdev, state, false);
 }
 EXPORT_SYMBOL(pci_enable_link_state);
@@ -1494,8 +1501,8 @@ EXPORT_SYMBOL(pci_enable_link_state);
  * 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.
  *
  * Context: Caller holds pci_bus_sem read lock.
  *
@@ -1505,6 +1512,13 @@ int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
 {
 	lockdep_assert_held_read(&pci_bus_sem);
 
+	/*
+	 * 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);
+
 	return __pci_enable_link_state(pdev, state, true);
 }
 EXPORT_SYMBOL(pci_enable_link_state_locked);

-- 
2.45.2

Re: [PATCH 2/6] PCI/ASPM: Transition the device to D0 (if required) inside pci_enable_link_state_locked() API

Posted by Bjorn Helgaas 2 months, 3 weeks ago

On Wed, Jul 16, 2025 at 06:26:21PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.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);
>  }

Re: [PATCH 2/6] PCI/ASPM: Transition the device to D0 (if required) inside pci_enable_link_state_locked() API

Posted by Manivannan Sadhasivam 2 months, 3 weeks ago

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@oss.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

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

[PATCH 1/6] PCI/ASPM: Fix the behavior of pci_enable_link_state*() APIs
[PATCH 2/6] PCI/ASPM: Transition the device to D0 (if required) inside pci_enable_link_state_locked() API
[PATCH 3/6] PCI/ASPM: Improve the kernel-doc for pci_disable_link_state*() APIs
[PATCH 4/6] wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states
[PATCH 5/6] wifi: ath11k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states
[PATCH 6/6] wifi: ath10k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states