Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

On Fri, 11 Jul 2025, Krishna Chaitanya Chundru wrote:
> On 7/11/2025 2:51 PM, Ilpo Järvinen wrote:
> > On Fri, 11 Jul 2025, Manivannan Sadhasivam wrote:
> > > On Wed, Jul 09, 2025 at 06:01:22PM GMT, Krishna Chaitanya Chundru wrote:
> > > > On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:

> > > > > 1) Renames pci_enable_link_state() to pci_set_default_link_state()
> > > > > 
> > > > > 1b) If pci_enable_link_state() is still needed after that, a new
> > > > > function
> > > > > is added to symmetrically pair with pci_disable_link_state().
> > > > > 
> > > > > or alternatively,
> > > > > 
> > > > > 2) Changelog justifies very clearly why this change is okay with the
> > > > > existing callers. (And obviously the function comment should be
> > > > > altered to
> > > > > match the functionality in that case too).
> > > > > 
> > > > > If approach 2 is chosen, it should be very carefully reviewed when it
> > > > > comes to the callers.
> > > > > 
> > > > I am in favor of approach 2 which you suggested, but lets wait for other
> > > > reviewers feedback on this. Based up on the response i will make
> > > > necessary changes in v5.
> > > > 
> > > 
> > > I would go for (1). It is always going to be a problem to change a legacy
> > > API
> > > like this. We might end up causing regressions. So it is safe to rename to
> > > reflect the purpose and try to come up with a new API that does what you
> > > want.
> > > If callers want to migrate to the new API, they can also do it in the
> > > future.
> > 
> > That's my recommendation as well.
> I will take this as learning, we will go with approach (1) as both of
> you are having valid points.
> 
> llpo,
> In the previous patch you said you have some patches on can you send me
> those or shall I proceed with patches from myside.

Sure, attached 3 patches.

Those were based on 6.11 and I've booted them along with my ASPM rework 
series (refactoring custom ASPM code in drivers to use the ASPM driver 
instaed). I quickly checked aspm.c fixes after that point and nothing 
seemed to require adaptations AFAICT.

-- 
 i.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-PCI-ASPM-Rename-pci_enable_link_state-to-pci_set_def.patch, Size: 7342 bytes --]

From 2885d4d3764e964b15adda01f66c516ce5ff2079 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 11 Jul 2025 16:28:24 +0300
Subject: [PATCH 1/3] PCI/ASPM: Rename pci_enable_link_state() to
 pci_set_default_link_state()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

pci_enable_link_state() and pci_disable_link_state() are not paired
symmetrically despite their names suggesting otherwise.
pci_enable_link_state() tweaks link state when the "default" policy is
in use rather than exactly "enabling" some link states. Obviously, when
the default policy is in use and the default link state is changed,
some link states may get enabled but that is a secondary effect.

Thus, rename pci_enable_link_state() to pci_set_default_link_state() to
better match what it does. The rename also frees
pci_enable_link_state() name so that a function that pairs
symmetrically with pci_disable_link_state() can be added later.

There's also pci_enable_link_state_locked() variant which is similarly
renamed to pci_set_default_link_state_locked().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c |  2 +-
 drivers/pci/controller/vmd.c           |  2 +-
 drivers/pci/pcie/aspm.c                | 29 +++++++++++++-------------
 include/linux/pci.h                    |  8 +++----
 4 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f85655..37ef3ba0c20b 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1023,7 +1023,7 @@ static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
 	 * substates.
 	 */
 	pci_set_power_state_locked(pdev, PCI_D0);
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+	pci_set_default_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 8df064b62a2f..f45e9e39c415 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -770,7 +770,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 * 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);
+	pci_set_default_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 29fcb0689a91..33e9885c0210 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1432,7 +1432,7 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
-static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
+static int __pci_set_default_link_state(struct pci_dev *pdev, int state, bool locked)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
@@ -1465,8 +1465,8 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 }
 
 /**
- * pci_enable_link_state - Clear and set the default device link state so that
- * the link may be allowed to enter the specified states. Note that if the
+ * pci_set_default_link_state - Clear and set the default device link state so
+ * that the link may be allowed to enter the specified states. Note that if the
  * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
  * touch the LNKCTL register. Also note that this does not enable states
  * disabled by pci_disable_link_state(). Return 0 or a negative errno.
@@ -1477,18 +1477,19 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
  */
-int pci_enable_link_state(struct pci_dev *pdev, int state)
+int pci_set_default_link_state(struct pci_dev *pdev, int state)
 {
-	return __pci_enable_link_state(pdev, state, false);
+	return __pci_set_default_link_state(pdev, state, false);
 }
-EXPORT_SYMBOL(pci_enable_link_state);
+EXPORT_SYMBOL(pci_set_default_link_state);
 
 /**
- * pci_enable_link_state_locked - Clear and set the default device link state
- * so that the link may be allowed to enter the specified states. Note that if
- * the BIOS didn't grant ASPM control to the OS, this does nothing because we
- * can't touch the LNKCTL register. Also note that this does not enable states
- * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ * pci_set_default_link_state_locked - Clear and set the default device link
+ * state so that the link may be allowed to enter the specified states. Note
+ * that if the BIOS didn't grant ASPM control to the OS, this does nothing
+ * because we can't touch the LNKCTL register. Also note that this does not
+ * enable states disabled by pci_disable_link_state(). Return 0 or a negative
+ * errno.
  *
  * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
  * PCIe r6.0, sec 5.5.4.
@@ -1498,13 +1499,13 @@ EXPORT_SYMBOL(pci_enable_link_state);
  *
  * Context: Caller holds pci_bus_sem read lock.
  */
-int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
+int pci_set_default_link_state_locked(struct pci_dev *pdev, int state)
 {
 	lockdep_assert_held_read(&pci_bus_sem);
 
-	return __pci_enable_link_state(pdev, state, true);
+	return __pci_set_default_link_state(pdev, state, true);
 }
-EXPORT_SYMBOL(pci_enable_link_state_locked);
+EXPORT_SYMBOL(pci_set_default_link_state_locked);
 
 static int pcie_aspm_set_policy(const char *val,
 				const struct kernel_param *kp)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f392..b8f60864ef81 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
 #ifdef CONFIG_PCIEASPM
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
-int pci_enable_link_state(struct pci_dev *pdev, int state);
-int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
+int pci_set_default_link_state(struct pci_dev *pdev, int state);
+int pci_set_default_link_state_locked(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
 bool pcie_aspm_support_enabled(void);
 bool pcie_aspm_enabled(struct pci_dev *pdev);
@@ -1836,9 +1836,9 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
 static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
-static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
+static inline int pci_set_default_link_state(struct pci_dev *pdev, int state)
 { return 0; }
-static inline int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
+static inline int pci_set_default_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
 static inline void pcie_no_aspm(void) { }
 static inline bool pcie_aspm_support_enabled(void) { return false; }

base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.39.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=0002-PCI-ASPM-Improve-pci_set_default_link_state-_locked-.patch, Size: 3769 bytes --]

From 68bf37222e176201f478cbae12ba4a2e2d6329af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 11 Jul 2025 16:28:25 +0300
Subject: [PATCH 2/3] PCI/ASPM: Improve pci_set_default_link_state{,_locked}()
 kerneldoc
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Improve pci_set_default_link_state{,_locked}() documentation:

- Note the link state may get changed if the default policy is in use
- Better follow kerneldoc formatting guidelines (separate description
  block and return entries)

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/aspm.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 33e9885c0210..5721ebfdea71 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1465,17 +1465,21 @@ static int __pci_set_default_link_state(struct pci_dev *pdev, int state, bool lo
 }
 
 /**
- * pci_set_default_link_state - Clear and set the default device link state so
- * that the link may be allowed to enter the specified states. Note that if the
- * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
- * touch the LNKCTL register. Also note that this does not enable states
- * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ * pci_set_default_link_state - Set the default device link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Set the default device link state so that the link may be allowed to
+ * enter the specified states. If the default policy is in use, the link
+ * state may also be updated to reflect the new default 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. Also note that this does not
+ * enable states disabled by pci_disable_link_state().
  *
  * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
  * PCIe r6.0, sec 5.5.4.
  *
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
+ * Return: 0 or a negative errno.
  */
 int pci_set_default_link_state(struct pci_dev *pdev, int state)
 {
@@ -1484,20 +1488,23 @@ int pci_set_default_link_state(struct pci_dev *pdev, int state)
 EXPORT_SYMBOL(pci_set_default_link_state);
 
 /**
- * pci_set_default_link_state_locked - Clear and set the default device link
- * state so that the link may be allowed to enter the specified states. Note
+ * pci_set_default_link_state_locked - Set the default device link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Set the default device link state so that the link may be allowed to
+ * enter the specified states. If the default policy is in use, the link
+ * state may also be updated to reflect the new default 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. Also note that this does not
- * enable states disabled by pci_disable_link_state(). Return 0 or a negative
- * errno.
+ * enable states disabled by pci_disable_link_state().
+ *
+ * Context: Caller holds pci_bus_sem read lock.
  *
  * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
  * PCIe r6.0, sec 5.5.4.
  *
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
- *
- * Context: Caller holds pci_bus_sem read lock.
+ * Return: 0 or a negative errno.
  */
 int pci_set_default_link_state_locked(struct pci_dev *pdev, int state)
 {
-- 
2.39.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Type: text/x-diff; name=0003-PCI-ASPM-Add-pci_enable_link_state.patch, Size: 3428 bytes --]

From d5ef8cf951dd135e76deb32d1c315fe95d1f4c91 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 11 Jul 2025 16:28:26 +0300
Subject: [PATCH 3/3] PCI/ASPM: Add pci_enable_link_state()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

pci_disable_link_state() lacks a symmetric pair. Some drivers want to
disable ASPM during certain phases of their operation but then
re-enable it later on. If pci_disable_link_state() is made for the
device, there is currently no way to re-enable the states that were
disabled.

Add pci_enable_link_state() to remove ASPM states from the state
disable mask.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/aspm.c | 43 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5721ebfdea71..348bd79f049f 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1432,6 +1432,49 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
+/**
+ * pci_enable_link_state - Re-enable device's link state
+ * @pdev: PCI device
+ * @state: ASPM link states to re-enable
+ *
+ * Enable device's link state that were previously disable so the link is
+ * allowed to enter the specific states. Note that if the BIOS didn't grant
+ * ASPM control to the OS, this does nothing because we can't touch the
+ * LNKCTL register.
+ *
+ * Return: 0 or a negative errno.
+ */
+int pci_enable_link_state(struct pci_dev *pdev, int state)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	if (!link)
+		return -EINVAL;
+	/*
+	 * A driver requested that ASPM be enabled on this device, but
+	 * if we don't have permission to manage ASPM (e.g., on ACPI
+	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
+	 * the _OSC method), we can't honor that request.
+	 */
+	if (aspm_disabled) {
+		pci_warn(pdev, "can't enable ASPM; OS doesn't have ASPM control\n");
+		return -EPERM;
+	}
+
+	mutex_lock(&aspm_lock);
+	/* Use the disable mask variant because it relates to aspm_disable */
+	link->aspm_disable &= ~pci_calc_aspm_disable_mask(state);
+	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+
+	if (state & PCIE_LINK_STATE_CLKPM)
+		link->clkpm_disable = 0;
+	pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	mutex_unlock(&aspm_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(pci_enable_link_state);
+
 static int __pci_set_default_link_state(struct pci_dev *pdev, int state, bool locked)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b8f60864ef81..d566d86e6368 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1826,6 +1826,7 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
 #ifdef CONFIG_PCIEASPM
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
+int pci_enable_link_state(struct pci_dev *pdev, int state);
 int pci_set_default_link_state(struct pci_dev *pdev, int state);
 int pci_set_default_link_state_locked(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
-- 
2.39.5