Re: [PATCH v3 8/8] wifi: ath10k: 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:
- [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