[PATCH v3 0/8] PCI/ASPM: Add pci_force_enable_link_state() API and convert Atheros drivers

[PATCH v3 0/8] PCI/ASPM: Add pci_force_enable_link_state() API and convert Atheros drivers

[Manivannan Sadhasivam]

Hi,

This series converts the atheros WLAN drivers (ath10k/ath11k/ath12k) to
use the PCI core ASPM APIs for enabling/disabling ASPM instead of writing
to the LNKCTL register directly. Touching LNKCTL behind the PCI core's
back leaves the ASPM service driver's view of the link state out of sync
with the hardware, which is not nice.

The catch is that these drivers save the currently enabled ASPM states,
disable ASPM before firmware download, and restore the saved states
afterwards. pci_enable_link_state() cannot restore states that were
disabled via pci_disable_link_state() or blacklisted during init (like
the pre-1.1 device blacklist, which is only meant to be overridden with
pcie_aspm=force), and that is intentional. So the save/restore sequence
cannot be expressed with the existing APIs.

In v2 I tried to fix this by making pci_enable_link_state*() symmetric
with the disable APIs, but as pointed out during review, that also ends
up overriding the init-time blacklist for every caller (including VMD),
which is not something we want. So instead, this version adds a separate
pci_force_enable_link_state() API (patch 2) that re-enables the requested
states unconditionally. The caller is responsible for only asking for
states the device actually supports, and the ath drivers do this by
restoring exactly what pcie_aspm_enabled() reported earlier.

The first five patches touch the PCI core: patch 1 (from Ilpo) makes
pci_disable_link_state() disable ASPM unconditionally, patch 2 adds the
new API, patch 3 moves the D0 transition needed before enabling PCI-PM
L1 PM Substates (PCIe r6.0, sec 5.5.4) into the shared helper and drops
it from the vmd and pcie-qcom callers, patch 4 cleans up the kernel-doc,
and patch 5 makes pcie_aspm_enabled() return the enabled state mask so it
can be used for save/restore. The last three patches convert the ath
drivers.

Testing
=======

I've tested this series on a Lenovo Thinkpad T14s with WCN7850 (so that's
just the ath12k driver). Rest of the drivers are compile tested only.

Merging Strategy
================

Even though there is no build dependency between the PCI core and atheros
patches, there is a functional dependency. So I'd recommend creating an
immutable branch with the PCI patches and merging that branch into both
the PCI and linux-wireless trees, and finally merging the atheros patches
into the linux-wireless tree.

If the immutable branch seems like a hassle, then the PCI core patches
could get merged first and the atheros patches can wait for the next
cycle.

- Mani

Changes in v3:
* Dropped the change that made pci_enable_link_state*() symmetric with the
  disable APIs. Instead added a new pci_force_enable_link_state() API used
  only by the ath drivers, so the init-time blacklist stays overridable
  only via pcie_aspm=force and existing callers like VMD are left
  untouched.
* Fixed the locked path to use pci_set_power_state_locked() for the D0
  transition. The previous version called the non-locked variant, which
  re-acquires pci_bus_sem.
* ath drivers now disable PCIE_LINK_STATE_ASPM_ALL instead of
  PCIE_LINK_STATE_ALL so that CLKPM is preserved and can be restored
  (pcie_aspm_enabled() doesn't report CLKPM).
* Reworked the kernel-doc patch to cover both the enable and disable APIs
  and reference the new API.
* Rebased on top of v7.2-rc1.

Changes in v2:
* Reworked the pcie_aspm_enabled() API to return the enabled states
  instead of bool and used it to save/restore the ASPM states in ath
  drivers.
* Added a patch from Ilpo to disable ASPM even if BIOS didn't grant
  permission.
* Added the CONFIG_PCIEASPM dependency to ath{10/11/12}k drivers.
* Rebased on top of v6.17-rc1.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Ilpo Järvinen (1):
      PCI/ASPM: Always disable ASPM when driver requests it

Manivannan Sadhasivam (7):
      PCI/ASPM: Add pci_force_enable_link_state() API
      PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states
      PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs
      PCI/ASPM: Return enabled ASPM states from pcie_aspm_enabled() API
      wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states
      wifi: ath11k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states
      wifi: ath10k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

 drivers/net/wireless/ath/ath10k/Kconfig |   2 +-
 drivers/net/wireless/ath/ath10k/pci.c   |  11 +--
 drivers/net/wireless/ath/ath10k/pci.h   |   5 +-
 drivers/net/wireless/ath/ath11k/Kconfig |   2 +-
 drivers/net/wireless/ath/ath11k/pci.c   |  19 +----
 drivers/net/wireless/ath/ath11k/pci.h   |   3 +-
 drivers/net/wireless/ath/ath12k/Kconfig |   2 +-
 drivers/net/wireless/ath/ath12k/pci.c   |  19 +----
 drivers/net/wireless/ath/ath12k/pci.h   |   4 +-
 drivers/pci/controller/dwc/pcie-qcom.c  |   5 --
 drivers/pci/controller/vmd.c            |   5 --
 drivers/pci/pcie/aspm.c                 | 140 ++++++++++++++++++++++++--------
 include/linux/pci.h                     |   7 +-
 13 files changed, 129 insertions(+), 95 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260708-pci-aspm-fix-331826a2cb5d

Best regards,
--  
மணிவண்ணன் சதாசிவம்

[PATCH v3 1/8] PCI/ASPM: Always disable ASPM when driver requests it

[Manivannan Sadhasivam]

From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

PCI core/ASPM service driver allows controlling ASPM state through
pci_disable_link_state() API. It was decided earlier (see the Link
below), to not allow ASPM changes when OS does not have control over it
but only log a warning about the problem 'commit 2add0ec14c25 ("PCI/ASPM:
Warn when driver asks to disable ASPM, but we can't do it")'.

A number of drivers have added workarounds to force ASPM off with own
writes into the Link Control Register (some even with comments
explaining why PCI core does not disable it under some circumstances).
According to the comments, some drivers require ASPM to be off for
reliable operation.

Having custom ASPM handling in drivers is problematic because the state
kept in the ASPM service driver is not updated by the changes made
outside the link state management API.

As the first step to address this issue, make pci_disable_link_state()
to unconditionally disable ASPM so the motivation for drivers to come
up with custom ASPM handling code is eliminated.

To fully take advantage of the ASPM handling core provides, the drivers
that need to quirk ASPM have to be altered depend on PCIEASPM and the
custom ASPM code is removed. This is to be done separately. As PCIEASPM
is already behind EXPERT, it should be no problem to limit disabling it
for configurations that do not require touching ASPM.

Make pci_disable_link_state() function comment to comply kerneldoc
formatting while changing the description.

Link: https://lore.kernel.org/all/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com/
Link: https://lore.kernel.org/all/20230511131441.45704-1-ilpo.jarvinen@linux.intel.com/
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
[mani: commit message fixup]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 172783e7f519..1c15a14718e8 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1449,16 +1449,23 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked
 		return -EINVAL;
 	/*
 	 * A driver requested that ASPM be disabled on this device, but
-	 * if we don't have permission to manage ASPM (e.g., on ACPI
+	 * if we might not 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.  Windows has
-	 * a similar mechanism using "PciASPMOptOut", which is also
-	 * ignored in this situation.
+	 * the _OSC method), previously we chose to not honor disable
+	 * request in that case. Windows has a similar mechanism using
+	 * "PciASPMOptOut", which is also ignored in this situation.
+	 *
+	 * Not honoring the requests to disable ASPM, however, led to
+	 * drivers forcing ASPM off on their own. As such changes of ASPM
+	 * state are not tracked by this service driver, the state kept here
+	 * became out of sync.
+	 *
+	 * Therefore, honor ASPM disable requests even when OS does not have
+	 * ASPM control. Plain disable for ASPM is assumed to be slightly
+	 * safer than fully managing it.
 	 */
-	if (aspm_disabled) {
-		pci_warn(pdev, "can't disable ASPM; OS doesn't have ASPM control\n");
-		return -EPERM;
-	}
+	if (aspm_disabled)
+		pci_warn(pdev, "OS doesn't have ASPM control, disabling ASPM anyway\n");
 
 	if (!locked)
 		down_read(&pci_bus_sem);
@@ -1485,13 +1492,13 @@ int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 EXPORT_SYMBOL(pci_disable_link_state_locked);
 
 /**
- * pci_disable_link_state - Disable device's link state, so the link will
- * never enter 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. Returns 0 or a negative errno.
- *
+ * pci_disable_link_state - Disable device's link state
  * @pdev: PCI device
  * @state: ASPM link state to disable
+ *
+ * Disable device's link state so the link will never enter specific states.
+ *
+ * Return: 0 or a negative errno
  */
 int pci_disable_link_state(struct pci_dev *pdev, int state)
 {

-- 
2.43.0

Re: [PATCH v3 1/8] PCI/ASPM: Always disable ASPM when driver requests it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition leading to Use-After-Free in ASPM link state management.
--

--- Patch [1]: [PATCH v3 1/8] PCI/ASPM: Always disable ASPM when driver requests it ---
commit 3f3308cae9c38dfb61e4a9d6f6d9f1413c0e0c87
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

PCI/ASPM: Always disable ASPM when driver requests it

This commit makes pci_disable_link_state() unconditionally disable ASPM so
the motivation for drivers to use custom ASPM handling code is eliminated.
This ensures ASPM state changes are properly tracked by the service driver.

Link: https://lore.kernel.org/all/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com/
Link: https://lore.kernel.org/all/20230511131441.45704-1-ilpo.jarvinen@linux.intel.com/

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1449,16 +1449,23 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked
>  {
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  
>  	if (!link)
>  		return -EINVAL;

[ ... ]

>  	if (!locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	link->aspm_disable |= pci_calc_aspm_disable_mask(state);

[Severity: High]
This is a pre-existing issue, but could this lockless fetch of the link
pointer lead to a use-after-free when we write to aspm_disable here?

If one CPU calls this asynchronously (for example, from a network driver TX
timeout handler) and another CPU concurrently removes a sibling PCI function
on the same bridge, could we hit this sequence:

CPU1
__pci_disable_link_state()
    link = pcie_aspm_get_link(pdev);

CPU2
pcie_aspm_exit_link_state()
    // acquires locks, frees parent bridge's link pointer
    bridge->link_state = NULL;
    kfree(link);

CPU1
    mutex_lock(&aspm_lock);
    link->aspm_disable |= pci_calc_aspm_disable_mask(state);

Could this concurrent removal cause the link pointer to be freed before
CPU1 acquires the locks, leading to memory corruption when modified?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=1

[PATCH v3 2/8] PCI/ASPM: Add pci_force_enable_link_state() API

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

pci_enable_link_state() and pci_enable_link_state_locked() APIs only enable
the ASPM states that were not previously disabled via
pci_disable_link_state*() API or blacklisted during init (e.g. the pre-1.1
device blacklist, which is only meant to be overridden with
'pcie_aspm=force'). This is an intentional behavior as these APIs must not
silently re-enable states that were deliberately disabled, since doing so
could enable ASPM on a link where it is known to be unsafe.

However, some drivers (e.g. the atheros WLAN drivers) save the currently
enabled ASPM states, disable all ASPM states before firmware download, and
then try to restore exactly the states that were enabled before. Restoring
those states requires re-enabling states that were just disabled via
pci_disable_link_state() API. But, this cannot be achieved using the
existing APIs.

Hence, add pci_force_enable_link_state() API for such callers. Unlike
pci_enable_link_state(), it re-enables the requested states even if they
were previously disabled via pci_disable_link_state() or disabled during
init. The caller is therefore responsible for only enabling states the
device actually supports, typically the ones previously reported by
pcie_aspm_enabled().

This API is implemented by adding a 'force' parameter to the shared
__pci_enable_link_state() helper. When 'force' is true, the requested
states are cleared from 'link->aspm_disable' before enabling.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 37 ++++++++++++++++++++++++++++++++++---
 include/linux/pci.h     |  3 +++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 1c15a14718e8..c04fb71de91c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1506,7 +1506,8 @@ 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_enable_link_state(struct pci_dev *pdev, int state, bool locked,
+				   bool force)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
@@ -1527,6 +1528,10 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 		down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
 	link->aspm_default = pci_calc_aspm_enable_mask(state);
+
+	/* Force enable states that were previously disabled */
+	if (force)
+		link->aspm_disable &= ~link->aspm_default;
 	pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
 	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
@@ -1553,7 +1558,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
  */
 int pci_enable_link_state(struct pci_dev *pdev, int state)
 {
-	return __pci_enable_link_state(pdev, state, false);
+	return __pci_enable_link_state(pdev, state, false, false);
 }
 EXPORT_SYMBOL(pci_enable_link_state);
 
@@ -1576,10 +1581,36 @@ int pci_enable_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_enable_link_state(pdev, state, true, false);
 }
 EXPORT_SYMBOL(pci_enable_link_state_locked);
 
+/**
+ * pci_force_enable_link_state - Force enable device link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Enable device link state, so the link will enter the specified states.
+ * Unlike pci_enable_link_state(), this also re-enables states previously
+ * disabled by pci_disable_link_state() and overrides the ASPM states disabled
+ * during init (e.g., the pre-1.1 device blacklist). The caller is therefore
+ * responsible for only enabling states the device supports, typically the ones
+ * previously reported by pcie_aspm_enabled().
+ *
+ * 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.
+ *
+ * Return: 0 on success, a negative errno otherwise.
+ */
+int pci_force_enable_link_state(struct pci_dev *pdev, int state)
+{
+	return __pci_enable_link_state(pdev, state, false, true);
+}
+EXPORT_SYMBOL(pci_force_enable_link_state);
+
 void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap)
 {
 	if (lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ebb5b9d76360..be6719626c8f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1918,6 +1918,7 @@ 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_force_enable_link_state(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);
@@ -1930,6 +1931,8 @@ static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
 static inline int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
+static inline int pci_force_enable_link_state(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; }
 static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }

-- 
2.43.0

Re: [PATCH v3 2/8] PCI/ASPM: Add pci_force_enable_link_state() API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] pci_force_enable_link_state() fails to override init-disabled ASPM states because it does not handle uninitialized aspm_capable masks.
- [High] pci_force_enable_link_state() fails to re-enable Clock PM (CLKPM) because it does not clear link->clkpm_disable.
- [High] pci_force_enable_link_state() fails with -EPERM when aspm_disabled is true, preventing driver state restoration.
- [Medium] The kernel doc for `pci_force_enable_link_state()` incorrectly advises callers to use `pcie_aspm_enabled()`, which returns a `bool` instead of a bitmask.

Pre-existing issues:
- [Critical] TOCTOU race causing Use-After-Free of `pcie_link_state` when device removal interleaves with `__pci_enable_link_state()`.
--

--- Patch [2]: [PATCH v3 2/8] PCI/ASPM: Add pci_force_enable_link_state() API ---
commit 2bf493d2bfb473abb29a075bfd6fe57330878665
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Add pci_force_enable_link_state() API

This patch introduces the pci_force_enable_link_state() API to allow
drivers to re-enable ASPM states that were previously disabled via
pci_disable_link_state() or disabled during initialization. It adds a force
parameter to the shared helper __pci_enable_link_state() to clear the
disabled states before enabling.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
[ ... ]
> @@ -1506,7 +1506,8 @@ 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_enable_link_state(struct pci_dev *pdev, int state, bool locked,
> +				   bool force)
>  {
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);

[Severity: Critical]
This is a pre-existing issue, but does fetching the link pointer locklessly
here lead to a Use-After-Free if device removal interleaves?

If a driver fetches the pointer, then sleeps in pci_set_power_state():

__pci_enable_link_state() {
    struct pcie_link_state *link = pcie_aspm_get_link(pdev);
    ...
    if (state & PCIE_LINK_STATE_L1_SS_PCIPM) {
        pci_set_power_state(pdev, PCI_D0);
    ...
    mutex_lock(&aspm_lock);
    link->aspm_default = ...;
}

Could a concurrent hot-remove call pcie_aspm_exit_link_state(), freeing
the link structure before we acquire the aspm_lock and dereference it?

[Severity: High]
Will this function return -EPERM on systems where BIOS ACPI _OSC hasn't
granted OS ASPM control, breaking driver state restoration?

While __pci_disable_link_state() intentionally bypasses the aspm_disabled
check, __pci_enable_link_state() strictly enforces it:

__pci_enable_link_state() {
    ...
    if (aspm_disabled) {
        pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
        return -EPERM;
    }
    ...
}

If a driver attempts a supported save/disable/restore sequence on such
a system, the restore step unconditionally fails, permanently disabling ASPM.

[ ... ]
> @@ -1527,6 +1528,10 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	link->aspm_default = pci_calc_aspm_enable_mask(state);
> +
> +	/* Force enable states that were previously disabled */
> +	if (force)
> +		link->aspm_disable &= ~link->aspm_default;

[Severity: High]
Does this logic neglect to clear the Clock PM disable mask?

If a driver calls pci_disable_link_state() with PCIE_LINK_STATE_CLKPM,
link->clkpm_disable is set to 1. When calling the new force-enable API
to restore the states, link->aspm_disable is cleared here, but
link->clkpm_disable remains 1.

Consequently, pcie_set_clkpm() will silently skip re-enabling Clock PM,
leading to permanent loss of clock power management.

>  	pcie_config_aspm_link(link, policy_to_aspm_state(link));

[Severity: High]
Can pci_force_enable_link_state() fail to override init-disabled ASPM
states here because link->aspm_capable remains uninitialized?

If a device was blacklisted during pcie_aspm_cap_init(), the initialization
returns early and link->aspm_capable remains 0. When pcie_config_aspm_link()
is called, it masks the requested state against link->aspm_capable:

pcie_config_aspm_link() {
    ...
    state &= link->aspm_capable;
    ...
}

Since aspm_capable is 0, the requested state becomes 0, silently failing to
enable ASPM and breaking the documented purpose of overriding the pre-1.1
device blacklist.

[ ... ]
> @@ -1576,10 +1581,36 @@ int pci_enable_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_enable_link_state(pdev, state, true, false);
>  }
>  EXPORT_SYMBOL(pci_enable_link_state_locked);
>  
> +/**
> + * pci_force_enable_link_state - Force enable device link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device link state, so the link will enter the specified states.
> + * Unlike pci_enable_link_state(), this also re-enables states previously
> + * disabled by pci_disable_link_state() and overrides the ASPM states disabled
> + * during init (e.g., the pre-1.1 device blacklist). The caller is therefore
> + * responsible for only enabling states the device supports, typically the ones
> + * previously reported by pcie_aspm_enabled().

[Severity: Medium]
Is it safe to advise callers to use pcie_aspm_enabled() for the state mask?

The pcie_aspm_enabled() function returns a bool, not a bitmask. If a driver
uses the return value as the state argument, the 7-bit field is truncated
to a boolean 1 or 0.

Since 1 translates to PCIE_LINK_STATE_L0S_UP (bit 0), it inadvertently
drops all other link states (like L1) the driver intended to restore.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=2

[PATCH v3 3/8] PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Per 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."

Currently, the callers of pci_enable_link_state_locked() (vmd, pcie-qcom)
transition the device to D0 themselves before enabling the link state. But
this is easy to get wrong and has to be duplicated by every caller.

Move the D0 transition into the shared __pci_enable_link_state() helper so
that all three APIs pci_enable_link_state(), pci_enable_link_state_locked()
and pci_force_enable_link_state() perform it, and only when the PCI-PM L1
PM Substates are getting enabled.

Now that the helper handles the transition, drop the redundant D0 transition
from the vmd and pcie-qcom callers.

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                | 23 +++++++++++++++++------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d8eb52857f69..45f4caeb0814 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1085,11 +1085,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 d4ae250d4bc6..20597d2cf66e 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -762,11 +762,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 c04fb71de91c..6d6862fd2ebb 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1524,6 +1524,17 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked,
 		return -EPERM;
 	}
 
+	/*
+	 * Ensure the device is in D0 before enabling PCI-PM L1 PM Substates, per
+	 * PCIe r6.0, sec 5.5.4.
+	 */
+	if (state & PCIE_LINK_STATE_L1_SS_PCIPM) {
+		if (locked)
+			pci_set_power_state_locked(pdev, PCI_D0);
+		else
+			pci_set_power_state(pdev, PCI_D0);
+	}
+
 	if (!locked)
 		down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
@@ -1550,8 +1561,8 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked,
  * 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.
+ * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
+ * are getting enabled.
  *
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
@@ -1569,8 +1580,8 @@ EXPORT_SYMBOL(pci_enable_link_state);
  * 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.
+ * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
+ * are getting enabled.
  *
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
@@ -1600,8 +1611,8 @@ EXPORT_SYMBOL(pci_enable_link_state_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.
  */

-- 
2.43.0

Re: [PATCH v3 3/8] PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving the D0 power state transition inside __pci_enable_link_state introduces a TOCTOU race leading to a Use-After-Free of the ASPM link pointer.
--

--- Patch [3]: [PATCH v3 3/8] PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states ---
commit abe3f397e26797e78eb9f5d0555a20f019b19168
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Transition the device to D0 (if required) when enabling ASPM link states

This commit moves the D0 power state transition into the shared 
__pci_enable_link_state() helper when enabling PCI-PM L1 PM Substates.
This centralizes the transition for all callers and removes the redundant
D0 transitions from the vmd and pcie-qcom drivers.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c04fb71de91c8..6d6862fd2ebb7 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1524,6 +1524,17 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked,
>  		return -EPERM;
>  	}
>  
> +	/*
> +	 * Ensure the device is in D0 before enabling PCI-PM L1 PM Substates, per
> +	 * PCIe r6.0, sec 5.5.4.
> +	 */
> +	if (state & PCIE_LINK_STATE_L1_SS_PCIPM) {
> +		if (locked)
> +			pci_set_power_state_locked(pdev, PCI_D0);
> +		else
> +			pci_set_power_state(pdev, PCI_D0);

[Severity: High]
Does this change introduce a use-after-free if the device is concurrently 
hot-removed?

The link pointer is fetched at the top of __pci_enable_link_state() 
locklessly:

	struct pcie_link_state *link = pcie_aspm_get_link(pdev);

If a caller uses the unlocked APIs, the new code takes the else branch and
calls pci_set_power_state(pdev, PCI_D0). This transition can sleep for tens 
of milliseconds without holding pci_bus_sem.

Could a concurrent hot-remove free the link structure while this thread is
sleeping?

CPU1 (Unlocked API):
__pci_enable_link_state()
    link = pcie_aspm_get_link(pdev);
    // ...
    pci_set_power_state(pdev, PCI_D0); // Sleeps without pci_bus_sem

CPU2 (Hot-remove):
pci_destroy_dev()
    pcie_aspm_exit_link_state()
        // Acquires ASPM locks and frees the `link` structure

CPU1 (wakes up):
    down_read(&pci_bus_sem);
    mutex_lock(&aspm_lock);
    link->aspm_default = pci_calc_aspm_enable_mask(state);

Would CPU1 end up writing to the freed pcie_link_state structure when it 
resumes and modifies link->aspm_default?

> +	}
> +
>  	if (!locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=3

[PATCH v3 4/8] PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Add kernel-doc for pci_disable_link_state_locked() API and fix the
kernel-doc for pci_disable_link_state() API.

Also convert the kernel-doc of pci_enable_link_state() and
pci_enable_link_state_locked() APIs to the standard format and mention that
pci_force_enable_link_state() should be used to re-enable the states
disabled by pci_disable_link_state().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 6d6862fd2ebb..5e7e199c78d2 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1483,6 +1483,17 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked
 	return 0;
 }
 
+/**
+ * pci_disable_link_state_locked - Disable device's link state
+ * @pdev: PCI device
+ * @state: ASPM link state to disable
+ *
+ * Disable device's link state so the link will never enter specific states.
+ *
+ * Context: Caller holds pci_bus_sem read lock.
+ *
+ * Return: 0 on success, a negative errno otherwise.
+ */
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 {
 	lockdep_assert_held_read(&pci_bus_sem);
@@ -1555,17 +1566,20 @@ 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
- * 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_enable_link_state - Enable device's link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Enable device's link state, so the link will 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(). Use
+ * pci_force_enable_link_state() for that.
  *
  * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
  * are getting enabled.
  *
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
+ * Return: 0 on success, a negative errno otherwise.
  */
 int pci_enable_link_state(struct pci_dev *pdev, int state)
 {
@@ -1574,19 +1588,22 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
 EXPORT_SYMBOL(pci_enable_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_enable_link_state_locked - Enable device's link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Enable device's link state, so the link will 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(). Use
+ * pci_force_enable_link_state() for that.
  *
  * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
  * are getting enabled.
  *
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
- *
  * Context: Caller holds pci_bus_sem read lock.
+ *
+ * Return: 0 on success, a negative errno otherwise.
  */
 int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
 {

-- 
2.43.0

Re: [PATCH v3 4/8] PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The kernel-doc for pci_enable_link_state_locked() incorrectly advises callers to use an incompatible API, which can lead to a recursive read lock deadlock.
- [Medium] The commit message claims to fix the kernel-doc for `pci_disable_link_state()`, but no such change is included in the patch.
--

--- Patch [4]: [PATCH v3 4/8] PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs ---
commit 544fcb510ac42577bcfc280750b6446d3bc52764
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs

This commit updates the kernel-doc comments for the PCI ASPM link state
management functions. It standardizes the format for the enable APIs and
adds missing documentation for the locked disable API.

> Add kernel-doc for pci_disable_link_state_locked() API and fix the
> kernel-doc for pci_disable_link_state() API.

[Severity: Medium]
Is the commit message accurate about fixing the kernel-doc for
pci_disable_link_state()?

Looking at the diff, it does not appear that pci_disable_link_state() was
modified in this patch, and its return format seems to remain as "Return: 0
or a negative errno" instead of the standard format applied to the other
functions.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 6d6862fd2ebb7..5e7e199c78d26 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -1574,19 +1588,22 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
>  EXPORT_SYMBOL(pci_enable_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_enable_link_state_locked - Enable device's link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device's link state, so the link will 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(). Use
> + * pci_force_enable_link_state() for that.

[Severity: High]
Does suggesting pci_force_enable_link_state() here introduce a deadlock risk?

The context for this function requires the caller to hold the pci_bus_sem
read lock. However, pci_force_enable_link_state() is an unlocked API that
internally calls down_read(&pci_bus_sem).

If a developer follows this documentation and replaces
pci_enable_link_state_locked() with pci_force_enable_link_state() while
holding pci_bus_sem, could this cause a recursive read lock deadlock if a
writer is queued in between the two acquisitions?

>  *
>  * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
>  * are getting enabled.
>  *
> - * @pdev: PCI device
> - * @state: Mask of ASPM link states to enable
> - *
>  * Context: Caller holds pci_bus_sem read lock.
> + *
> + * Return: 0 on success, a negative errno otherwise.
>  */
>  int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=4

[PATCH v3 5/8] PCI/ASPM: Return enabled ASPM states from pcie_aspm_enabled() API

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

pcie_aspm_enabled() returns 'pcie_link_state::aspm_enabled' parameter which
contains the enabled states. But the API currently returns the 'bool' type
which is used by the callers to decide if ASPM is enabled or not.

To allow the future callers to also make use of the enabled ASPM states,
return the actual type of 'pcie_link_state::aspm_enabled' parameter, 'u32'.

Existing callers can still treat the return value as a 'bool' as the C11
standard guarantees the behavior (this API relied on the same behavior
before as well).

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 6 ++++--
 include/linux/pci.h     | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5e7e199c78d2..3ce6b9319566 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1694,15 +1694,17 @@ module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
 	NULL, 0644);
 
 /**
- * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device.
+ * pcie_aspm_enabled - Get the enabled PCIe ASPM states for a device.
  * @pdev: Target device.
  *
  * Relies on the upstream bridge's link_state being valid.  The link_state
  * is deallocated only when the last child of the bridge (i.e., @pdev or a
  * sibling) is removed, and the caller should be holding a reference to
  * @pdev, so this should be safe.
+ *
+ * Return: Enabled PCIe ASPM states. 0 if ASPM is disabled.
  */
-bool pcie_aspm_enabled(struct pci_dev *pdev)
+u32 pcie_aspm_enabled(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index be6719626c8f..61f6b78c19fd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1921,7 +1921,7 @@ int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
 int pci_force_enable_link_state(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);
+u32 pcie_aspm_enabled(struct pci_dev *pdev);
 #else
 static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
@@ -1935,7 +1935,7 @@ static inline int pci_force_enable_link_state(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; }
-static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
+static inline u32 pcie_aspm_enabled(struct pci_dev *pdev) { return 0; }
 #endif
 
 #ifdef CONFIG_HOTPLUG_PCI

-- 
2.43.0

Re: [PATCH v3 5/8] PCI/ASPM: Return enabled ASPM states from pcie_aspm_enabled() API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The function `pcie_aspm_enabled()` returns a boolean literal (`false`) despite its return type being changed to `u32`.
--

--- Patch [5]: [PATCH v3 5/8] PCI/ASPM: Return enabled ASPM states from pcie_aspm_enabled() API ---
commit 075855b1906924e43de63fd5d55e7d8fedc2a01b
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Return enabled ASPM states from pcie_aspm_enabled() API

pcie_aspm_enabled() returns the pcie_link_state::aspm_enabled parameter which
contains the enabled states. But the API currently returns the bool type
which is used by the callers to decide if ASPM is enabled or not.

To allow the future callers to also make use of the enabled ASPM states,
this patch returns the actual type of pcie_link_state::aspm_enabled, u32.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 5e7e199c78d26..3ce6b93195663 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1694,15 +1694,17 @@ module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>  	NULL, 0644);
>  
>  /**
> - * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device.
> + * pcie_aspm_enabled - Get the enabled PCIe ASPM states for a device.
>   * @pdev: Target device.
>   *
>   * Relies on the upstream bridge's link_state being valid.  The link_state
>   * is deallocated only when the last child of the bridge (i.e., @pdev or a
>   * sibling) is removed, and the caller should be holding a reference to
>   * @pdev, so this should be safe.
> + *
> + * Return: Enabled PCIe ASPM states. 0 if ASPM is disabled.
>   */
> -bool pcie_aspm_enabled(struct pci_dev *pdev)
> +u32 pcie_aspm_enabled(struct pci_dev *pdev)
>  {
>  	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  

[Severity: Low]
Does this code still return a boolean literal on the error path?

Looking at pcie_aspm_enabled(), the error path still returns false:

    if (!link)
        return false;

Since the return type of pcie_aspm_enabled() was changed to u32, returning
false compiles because false evaluates to 0, but it obfuscates the integer
semantics of the function and might trigger static analysis warnings. Could
this be updated to return 0 instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=5

[PATCH v3 6/8] wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

It is not recommended to enable/disable the ASPM states on the back of the
PCI core directly using the LNKCTL register. It will break the PCI core's
knowledge about the device ASPM states. So use the APIs exposed by the PCI
core to enable/disable ASPM states.

Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Reported-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/net/wireless/ath/ath12k/Kconfig |  2 +-
 drivers/net/wireless/ath/ath12k/pci.c   | 19 +++----------------
 drivers/net/wireless/ath/ath12k/pci.h   |  4 +++-
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/Kconfig b/drivers/net/wireless/ath/ath12k/Kconfig
index 4a2b240f967a..7852ede3eaea 100644
--- a/drivers/net/wireless/ath/ath12k/Kconfig
+++ b/drivers/net/wireless/ath/ath12k/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: BSD-3-Clause-Clear
 config ATH12K
 	tristate "Qualcomm Wi-Fi 7 support (ath12k)"
-	depends on MAC80211 && HAS_DMA && PCI
+	depends on MAC80211 && HAS_DMA && PCI && PCIEASPM
 	select QCOM_QMI_HELPERS
 	select MHI_BUS
 	select QRTR
diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
index d9a22d6afbb0..6d48fb25690d 100644
--- a/drivers/net/wireless/ath/ath12k/pci.c
+++ b/drivers/net/wireless/ath/ath12k/pci.c
@@ -884,19 +884,9 @@ static void ath12k_pci_free_region(struct ath12k_pci *ab_pci)
 
 static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
 {
-	struct ath12k_base *ab = ab_pci->ab;
-
-	pcie_capability_read_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-				  &ab_pci->link_ctl);
-
-	ath12k_dbg(ab, ATH12K_DBG_PCI, "pci link_ctl 0x%04x L0s %d L1 %d\n",
-		   ab_pci->link_ctl,
-		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L0S),
-		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
+	ab_pci->aspm_states = pcie_aspm_enabled(ab_pci->pdev);
 
-	/* disable L0s and L1 */
-	pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-				   PCI_EXP_LNKCTL_ASPMC);
+	pci_disable_link_state(ab_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);
 
 	set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
 }
@@ -925,10 +915,7 @@ static void ath12k_pci_aspm_restore(struct ath12k_pci *ab_pci)
 {
 	if (ab_pci->ab->hw_params->supports_aspm &&
 	    test_and_clear_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags))
-		pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPMC,
-						   ab_pci->link_ctl &
-						   PCI_EXP_LNKCTL_ASPMC);
+		pci_force_enable_link_state(ab_pci->pdev, ab_pci->aspm_states);
 }
 
 static void ath12k_pci_cancel_workqueue(struct ath12k_base *ab)
diff --git a/drivers/net/wireless/ath/ath12k/pci.h b/drivers/net/wireless/ath/ath12k/pci.h
index 0e0e2020c6ae..409ef063cd69 100644
--- a/drivers/net/wireless/ath/ath12k/pci.h
+++ b/drivers/net/wireless/ath/ath12k/pci.h
@@ -128,7 +128,9 @@ struct ath12k_pci {
 
 	/* enum ath12k_pci_flags */
 	unsigned long flags;
-	u16 link_ctl;
+
+	/* Cached PCIe ASPM states */
+	u32 aspm_states;
 	unsigned long irq_flags;
 	const struct ath12k_pci_ops *pci_ops;
 	u32 qmi_instance;

-- 
2.43.0

Re: [PATCH v3 6/8] wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Adding `depends on PCIEASPM` unnecessarily restricts the driver from being built on kernels where ASPM control is disabled.

Pre-existing issues:
- [High] ASPM state is permanently lost (leaked as disabled) if initialization fails before `ath12k_pci_start()`.
--

--- Patch [6]: [PATCH v3 6/8] wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states ---
commit 6aeb96e7a1d3a96df659d6e1d629f287d946f802
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

This commit changes the ath12k driver to use the PCI core's
pci_disable_link_state() and pci_force_enable_link_state() APIs for managing
ASPM states instead of directly modifying the LNKCTL register. It also adds a
Kconfig dependency on PCIEASPM.

> diff --git a/drivers/net/wireless/ath/ath12k/Kconfig b/drivers/net/wireless/ath/ath12k/Kconfig
> index 4a2b240f967af..7852ede3eaea8 100644
> --- a/drivers/net/wireless/ath/ath12k/Kconfig
> +++ b/drivers/net/wireless/ath/ath12k/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: BSD-3-Clause-Clear
>  config ATH12K
>  	tristate "Qualcomm Wi-Fi 7 support (ath12k)"
> -	depends on MAC80211 && HAS_DMA && PCI
> +	depends on MAC80211 && HAS_DMA && PCI && PCIEASPM

[Severity: Medium]
Is the PCIEASPM dependency necessary here?

The PCI subsystem provides static inline no-ops for pci_disable_link_state(),
pcie_aspm_enabled(), and pci_force_enable_link_state() in linux/pci.h when
CONFIG_PCIEASPM is not set. Adding this strict dependency restricts the
driver from being built on kernels where ASPM control is intentionally
disabled.

>  	select QCOM_QMI_HELPERS
>  	select MHI_BUS
>  	select QRTR

[ ... ]

> diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> index d9a22d6afbb02..6d48fb25690de 100644
> --- a/drivers/net/wireless/ath/ath12k/pci.c
> +++ b/drivers/net/wireless/ath/ath12k/pci.c
> @@ -884,19 +884,9 @@ static void ath12k_pci_free_region(struct ath12k_pci *ab_pci)
>  
>  static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
>  {
> -	struct ath12k_base *ab = ab_pci->ab;
> -
> -	pcie_capability_read_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -				  &ab_pci->link_ctl);
> -
> -	ath12k_dbg(ab, ATH12K_DBG_PCI, "pci link_ctl 0x%04x L0s %d L1 %d\n",
> -		   ab_pci->link_ctl,
> -		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L0S),
> -		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
> +	ab_pci->aspm_states = pcie_aspm_enabled(ab_pci->pdev);

[Severity: High]
This is a pre-existing issue, but does this logic permanently lose the ASPM
state if initialization fails before ath12k_pci_start()?

If ath12k_mhi_start() fails in ath12k_pci_power_up(), the initialization
is aborted. The current ASPM state is unconditionally saved to
ab_pci->aspm_states here and then disabled. On failure,
ath12k_pci_power_down() is called to clean up, but it returns early if
ATH12K_PCI_FLAG_INIT_DONE is not set:

drivers/net/wireless/ath/ath12k/pci.c:ath12k_pci_power_down() {
	if (!test_bit(ATH12K_PCI_FLAG_INIT_DONE, &ab_pci->flags))
		return;

	/* restore aspm in case firmware bootup fails */
	ath12k_pci_aspm_restore(ab_pci);
	...
}

This early return skips the restore and leaves ASPM disabled. When the
device is later retried, ath12k_pci_power_up() is called again, overwriting
ab_pci->aspm_states with the now-disabled state (0). When initialization
finally succeeds, it restores 0, permanently disabling ASPM.

>  
> -	/* disable L0s and L1 */
> -	pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -				   PCI_EXP_LNKCTL_ASPMC);
> +	pci_disable_link_state(ab_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);
>  
>  	set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=6

Re: [PATCH v3 6/8] wifi: ath12k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[Bjorn Helgaas]

On Wed, Jul 08, 2026 at 04:30:20PM +0200, Manivannan Sadhasivam wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> It is not recommended to enable/disable the ASPM states on the back of the
> PCI core directly using the LNKCTL register. It will break the PCI core's
> knowledge about the device ASPM states. So use the APIs exposed by the PCI
> core to enable/disable ASPM states.
> 
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Reported-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath12k/Kconfig |  2 +-
>  drivers/net/wireless/ath/ath12k/pci.c   | 19 +++----------------
>  drivers/net/wireless/ath/ath12k/pci.h   |  4 +++-
>  3 files changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/Kconfig b/drivers/net/wireless/ath/ath12k/Kconfig
> index 4a2b240f967a..7852ede3eaea 100644
> --- a/drivers/net/wireless/ath/ath12k/Kconfig
> +++ b/drivers/net/wireless/ath/ath12k/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: BSD-3-Clause-Clear
>  config ATH12K
>  	tristate "Qualcomm Wi-Fi 7 support (ath12k)"
> -	depends on MAC80211 && HAS_DMA && PCI
> +	depends on MAC80211 && HAS_DMA && PCI && PCIEASPM

As Sashiko pointed out [1], I think adding the PCIEASPM dependency is
an issue.  The PCI core should provide stubs or whatever is necessary
to make ath12k work even if PCIEASPM isn't selected.

I'm guessing this hardware depends on the ability to disable ASPM even
without PCIEASPM so we probably would need more than just empty stubs.
We do build aspm.c unconditionally now and we provide LTR and L1SS
save/restore regardless of PCIEASPM.  Maybe we need something similar
here.

[1] https://lore.kernel.org/all/20260708144904.0CC4F1F000E9@smtp.kernel.org

>  	select QCOM_QMI_HELPERS
>  	select MHI_BUS
>  	select QRTR
> diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> index d9a22d6afbb0..6d48fb25690d 100644
> --- a/drivers/net/wireless/ath/ath12k/pci.c
> +++ b/drivers/net/wireless/ath/ath12k/pci.c
> @@ -884,19 +884,9 @@ static void ath12k_pci_free_region(struct ath12k_pci *ab_pci)
>  
>  static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
>  {
> -	struct ath12k_base *ab = ab_pci->ab;
> -
> -	pcie_capability_read_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -				  &ab_pci->link_ctl);
> -
> -	ath12k_dbg(ab, ATH12K_DBG_PCI, "pci link_ctl 0x%04x L0s %d L1 %d\n",
> -		   ab_pci->link_ctl,
> -		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L0S),
> -		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
> +	ab_pci->aspm_states = pcie_aspm_enabled(ab_pci->pdev);
>  
> -	/* disable L0s and L1 */
> -	pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -				   PCI_EXP_LNKCTL_ASPMC);
> +	pci_disable_link_state(ab_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);
>  
>  	set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
>  }
> @@ -925,10 +915,7 @@ static void ath12k_pci_aspm_restore(struct ath12k_pci *ab_pci)
>  {
>  	if (ab_pci->ab->hw_params->supports_aspm &&
>  	    test_and_clear_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags))
> -		pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -						   PCI_EXP_LNKCTL_ASPMC,
> -						   ab_pci->link_ctl &
> -						   PCI_EXP_LNKCTL_ASPMC);
> +		pci_force_enable_link_state(ab_pci->pdev, ab_pci->aspm_states);
>  }
>  
>  static void ath12k_pci_cancel_workqueue(struct ath12k_base *ab)
> diff --git a/drivers/net/wireless/ath/ath12k/pci.h b/drivers/net/wireless/ath/ath12k/pci.h
> index 0e0e2020c6ae..409ef063cd69 100644
> --- a/drivers/net/wireless/ath/ath12k/pci.h
> +++ b/drivers/net/wireless/ath/ath12k/pci.h
> @@ -128,7 +128,9 @@ struct ath12k_pci {
>  
>  	/* enum ath12k_pci_flags */
>  	unsigned long flags;
> -	u16 link_ctl;
> +
> +	/* Cached PCIe ASPM states */
> +	u32 aspm_states;
>  	unsigned long irq_flags;
>  	const struct ath12k_pci_ops *pci_ops;
>  	u32 qmi_instance;
> 
> -- 
> 2.43.0
> 
> 

[PATCH v3 7/8] wifi: ath11k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

It is not recommended to enable/disable the ASPM states on the back of the
PCI core directly using the LNKCTL register. It will break the PCI core's
knowledge about the device ASPM states. So use the APIs exposed by the PCI
core to enable/disable ASPM states.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/net/wireless/ath/ath11k/Kconfig |  2 +-
 drivers/net/wireless/ath/ath11k/pci.c   | 19 +++----------------
 drivers/net/wireless/ath/ath11k/pci.h   |  3 ++-
 3 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/Kconfig b/drivers/net/wireless/ath/ath11k/Kconfig
index 122726f84492..c8b418521173 100644
--- a/drivers/net/wireless/ath/ath11k/Kconfig
+++ b/drivers/net/wireless/ath/ath11k/Kconfig
@@ -19,7 +19,7 @@ config ATH11K_AHB
 
 config ATH11K_PCI
 	tristate "Atheros ath11k PCI support"
-	depends on ATH11K && PCI
+	depends on ATH11K && PCI && PCIEASPM
 	select MHI_BUS
 	select QRTR
 	select QRTR_MHI
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 35bb9e7a63a2..da980e01d1fc 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -604,19 +604,9 @@ static void ath11k_pci_free_region(struct ath11k_pci *ab_pci)
 
 static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
 {
-	struct ath11k_base *ab = ab_pci->ab;
-
-	pcie_capability_read_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-				  &ab_pci->link_ctl);
-
-	ath11k_dbg(ab, ATH11K_DBG_PCI, "link_ctl 0x%04x L0s %d L1 %d\n",
-		   ab_pci->link_ctl,
-		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L0S),
-		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
+	ab_pci->aspm_states = pcie_aspm_enabled(ab_pci->pdev);
 
-	/* disable L0s and L1 */
-	pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-				   PCI_EXP_LNKCTL_ASPMC);
+	pci_disable_link_state(ab_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);
 
 	set_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags);
 }
@@ -624,10 +614,7 @@ static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
 static void ath11k_pci_aspm_restore(struct ath11k_pci *ab_pci)
 {
 	if (test_and_clear_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags))
-		pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
-						   PCI_EXP_LNKCTL_ASPMC,
-						   ab_pci->link_ctl &
-						   PCI_EXP_LNKCTL_ASPMC);
+		pci_force_enable_link_state(ab_pci->pdev, ab_pci->aspm_states);
 }
 
 #ifdef CONFIG_DEV_COREDUMP
diff --git a/drivers/net/wireless/ath/ath11k/pci.h b/drivers/net/wireless/ath/ath11k/pci.h
index 1e3005a4b64c..474c18741418 100644
--- a/drivers/net/wireless/ath/ath11k/pci.h
+++ b/drivers/net/wireless/ath/ath11k/pci.h
@@ -72,7 +72,8 @@ struct ath11k_pci {
 
 	/* enum ath11k_pci_flags */
 	unsigned long flags;
-	u16 link_ctl;
+	/* Cached PCIe ASPM states */
+	u32 aspm_states;
 	u64 dma_mask;
 };
 

-- 
2.43.0

Re: [PATCH v3 7/8] wifi: ath11k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Using PCI core ASPM APIs for temporary disable/restore of ASPM permanently corrupts link state tracking (veto leak and default state overwrite).
--

--- Patch [7]: [PATCH v3 7/8] wifi: ath11k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states ---
commit 3c3d38d66376257335d9c989bd98925dd8c4bb20
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

wifi: ath11k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

This commit replaces direct LNKCTL register manipulation with PCI core APIs
to temporarily disable and restore ASPM states.

> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index 35bb9e7a63a20..da980e01d1fc2 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c

[ ... ]

> @@ -604,19 +604,9 @@ static void ath11k_pci_free_region(struct ath11k_pci *ab_pci)
>  
>  static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
>  {

[ ... ]

> +	ab_pci->aspm_states = pcie_aspm_enabled(ab_pci->pdev);
>  
> -	/* disable L0s and L1 */
> -	pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -				   PCI_EXP_LNKCTL_ASPMC);
> +	pci_disable_link_state(ab_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);

[Severity: Medium]
Will this permanently disable supported but inactive ASPM states? 

By calling pci_disable_link_state() with PCIE_LINK_STATE_ASPM_ALL, all ASPM
bits are added to the PCI core's link->aspm_disable mask.

>  	set_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags);
>  }
> @@ -624,10 +614,7 @@ static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
>  static void ath11k_pci_aspm_restore(struct ath11k_pci *ab_pci)
>  {
>  	if (test_and_clear_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags))
> -		pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> -						   PCI_EXP_LNKCTL_ASPMC,
> -						   ab_pci->link_ctl &
> -						   PCI_EXP_LNKCTL_ASPMC);
> +		pci_force_enable_link_state(ab_pci->pdev, ab_pci->aspm_states);

[Severity: Medium]
Does this restore sequence leak the veto for inactive states and overwrite the
default ASPM state?

When restoring, pci_force_enable_link_state() only clears the bits in
link->aspm_disable that correspond to the previously active states (captured via
pcie_aspm_enabled()). Any state that was supported but not active remains in
the aspm_disable mask forever, preventing users from enabling it via sysfs or
global policies.

Additionally, pci_force_enable_link_state() overwrites link->aspm_default
with the restored states, erasing the actual BIOS defaults.

(Note: This same pattern appears to occur in the ath12k driver modified
earlier in this series).

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=7

[PATCH v3 8/8] wifi: ath10k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

It is not recommended to enable/disable the ASPM states on the back of the
PCI core directly using the LNKCTL register. It will break the PCI core's
knowledge about the device ASPM states. So use the APIs exposed by the PCI
core to enable/disable ASPM states.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/Kconfig |  2 +-
 drivers/net/wireless/ath/ath10k/pci.c   | 11 ++++-------
 drivers/net/wireless/ath/ath10k/pci.h   |  5 ++---
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig
index efb9f022d8c6..eac3ec5a4e78 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -17,7 +17,7 @@ config ATH10K_CE
 
 config ATH10K_PCI
 	tristate "Atheros ath10k PCI support"
-	depends on ATH10K && PCI
+	depends on ATH10K && PCI && PCIEASPM
 	help
 	  This module adds support for PCIE bus
 
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 335bc7c488e4..2e27a70065cc 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1965,9 +1965,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 	ath10k_pci_irq_enable(ar);
 	ath10k_pci_rx_post(ar);
 
-	pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
-					   PCI_EXP_LNKCTL_ASPMC,
-					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
+	pci_force_enable_link_state(ar_pci->pdev, ar_pci->aspm_states);
 
 	return 0;
 }
@@ -2822,10 +2820,9 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
 
 	ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif power up\n");
 
-	pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
-				  &ar_pci->link_ctl);
-	pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
-				   PCI_EXP_LNKCTL_ASPMC);
+	ar_pci->aspm_states = pcie_aspm_enabled(ar_pci->pdev);
+
+	pci_disable_link_state(ar_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);
 
 	/*
 	 * Bring the target up cleanly.
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index d114778edb41..122e365b7cdd 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -128,10 +128,9 @@ struct ath10k_pci {
 	struct timer_list rx_post_retry;
 
 	/* Due to HW quirks it is recommended to disable ASPM during device
-	 * bootup. To do that the original PCI-E Link Control is stored before
-	 * device bootup is executed and re-programmed later.
+	 * bootup. To do that the ASPM states are saved and re-programmed later.
 	 */
-	u16 link_ctl;
+	u32 aspm_states;
 
 	/* Protects ps_awake and ps_wake_refcount */
 	spinlock_t ps_lock;

-- 
2.43.0

Re: [PATCH v3 8/8] wifi: ath10k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unnecessary strict dependency on `CONFIG_PCIEASPM` breaks the driver on systems without ASPM enabled.
- [Medium] Temporary ASPM disablement potentially permanently poisons the PCI link's `aspm_disable` mask for states that were not currently active (Speculative).
--

--- Patch [8]: [PATCH v3 8/8] wifi: ath10k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states ---
commit f0b7b0f10e0387bb33b3201d0eb417f52d2cc145
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

wifi: ath10k: Use pci_{enable/disable}_link_state() APIs to enable/disable ASPM states

The commit replaces direct LNKCTL register manipulation with PCI core APIs
for enabling and disabling ASPM states. This ensures the PCI core's
knowledge of the device's ASPM state remains accurate.

> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig
> index efb9f022d8c66..eac3ec5a4e78f 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -17,7 +17,7 @@ config ATH10K_CE
>  
>  config ATH10K_PCI
>  	tristate "Atheros ath10k PCI support"
> -	depends on ATH10K && PCI
> +	depends on ATH10K && PCI && PCIEASPM

[Severity: High]
Does this strict dependency on PCIEASPM break the driver build on systems
where ASPM is disabled?

The PCI subsystem provides no-op stubs for ASPM functions (like
pci_disable_link_state, pci_force_enable_link_state, and pcie_aspm_enabled)
in <linux/pci.h> so drivers can use these APIs without requiring a strict
dependency.

[ ... ]

> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 335bc7c488e41..2e27a70065cce 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -1965,9 +1965,7 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
>  	ath10k_pci_irq_enable(ar);
>  	ath10k_pci_rx_post(ar);
>  
> -	pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> -					   PCI_EXP_LNKCTL_ASPMC,
> -					   ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> +	pci_force_enable_link_state(ar_pci->pdev, ar_pci->aspm_states);
>  
>  	return 0;
>  }

[ ... ]

> @@ -2822,10 +2820,9 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
>  
>  	ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif power up\n");
>  
> -	pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> -				  &ar_pci->link_ctl);
> -	pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> -				   PCI_EXP_LNKCTL_ASPMC);
> +	ar_pci->aspm_states = pcie_aspm_enabled(ar_pci->pdev);
> +
> +	pci_disable_link_state(ar_pci->pdev, PCIE_LINK_STATE_ASPM_ALL);

[Severity: Medium]
Since all ASPM states are disabled here using PCIE_LINK_STATE_ASPM_ALL, but
pci_force_enable_link_state() in ath10k_pci_hif_start() only restores the
previously active states (ar_pci->aspm_states), could this permanently
poison the PCI link's aspm_disable mask for states that were not active?

If the API only clears the disable mask for the requested states, any ASPM
states that were not initially active will remain permanently disabled in the
PCI core.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=8