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

[sashiko-bot@kernel.org]

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