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