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

Manivannan Sadhasivam mani at kernel.org
Mon Jul 21 04:08:52 PDT 2025


On Mon, Jul 21, 2025 at 01:09:05PM GMT, Ilpo Järvinen wrote:
> On Mon, 21 Jul 2025, Manivannan Sadhasivam wrote:
> > On Mon, Jul 21, 2025 at 11:04:10AM GMT, Ilpo Järvinen wrote:
> > > On Wed, 16 Jul 2025, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam at 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 at oss.qualcomm.com>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam at oss.qualcomm.com>
> > > > ---
> > > >  drivers/net/wireless/ath/ath.h        | 14 ++++++++++++++
> > > >  drivers/net/wireless/ath/ath12k/pci.c | 10 ++++------
> > > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
> > > > index 34654f710d8a1e63f65a47d4602e2035262a4d9e..ef685123b66bf4f41428fec67c1967f242a9ef27 100644
> > > > --- a/drivers/net/wireless/ath/ath.h
> > > > +++ b/drivers/net/wireless/ath/ath.h
> > > > @@ -21,6 +21,8 @@
> > > >  #include <linux/skbuff.h>
> > > >  #include <linux/if_ether.h>
> > > >  #include <linux/spinlock.h>
> > > > +#include <linux/pci.h>
> > > > +#include <linux/pci_regs.h>
> > > >  #include <net/mac80211.h>
> > > >  
> > > >  /*
> > > > @@ -336,4 +338,16 @@ static inline const char *ath_bus_type_to_string(enum ath_bus_type bustype)
> > > >  	return ath_bus_type_strings[bustype];
> > > >  }
> > > >  
> > > > +static inline int ath_pci_aspm_state(u16 lnkctl)
> > > > +{
> > > > +	int state = 0;
> > > > +
> > > > +	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
> > > > +		state |= PCIE_LINK_STATE_L0S;
> > > > +	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
> > > > +		state |= PCIE_LINK_STATE_L1;
> > > > +
> > > > +	return state;
> > > > +}
> > > > +
> > > >  #endif /* ATH_H */
> > > > diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> > > > index 489d546390fcdab8f615cc9184006a958d9f140a..a5e11509e3ab8faad6638ff78ce6a8a5e9c3cbbd 100644
> > > > --- a/drivers/net/wireless/ath/ath12k/pci.c
> > > > +++ b/drivers/net/wireless/ath/ath12k/pci.c
> > > > @@ -16,6 +16,8 @@
> > > >  #include "mhi.h"
> > > >  #include "debug.h"
> > > >  
> > > > +#include "../ath.h"
> > > > +
> > > >  #define ATH12K_PCI_BAR_NUM		0
> > > >  #define ATH12K_PCI_DMA_MASK		36
> > > >  
> > > > @@ -928,8 +930,7 @@ static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
> > > >  		   u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
> > > >  
> > > >  	/* 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_L0S | PCIE_LINK_STATE_L1);
> > > 
> > > I'd remove to comment too as the code is self-explanatory after this 
> > > change.
> > > 
> > 
> > Ack
> > 
> > > >  
> > > >  	set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
> > > >  }
> > > > @@ -958,10 +959,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_enable_link_state(ab_pci->pdev, ath_pci_aspm_state(ab_pci->link_ctl));
> > > >  }
> > > >  
> > > >  static void ath12k_pci_cancel_workqueue(struct ath12k_base *ab)
> > > 
> > > As you now depend on ASPM driver being there, these should also add to 
> > > Kconfig:
> > > 
> > > depends on PCIEASPM
> > > 
> > 
> > I thought about it, but since this driver doesn't necessarily enable ASPM for
> > all the devices it supports, I didn't add the dependency. But looking at it
> > again, I think makes sense to add the dependency since the driver cannot work
> > reliably without disabling ASPM (for the supported devices).
> 
> PCIEASPM is already default y and if EXPERT so it is not something 
> that is expected to be disabled.
> 
> You also no longer need to move the ASPM link state defines LKP found out 
> about after adding the depends on.
> 

Yes, it will fix the reported issue, but guarding the definitions feels wrong to
me still. Maybe that's something we can worry later.

> I'm a bit worried this series will regress in the cases where OS doesn't 
> control ASPM so it might be necessary to include something along the 
> lines of the patch below too (the earlier discussion on this is in Link 
> tags):
> 

atheros drivers didn't have such comment (why they were manually changing the
LNKCTL register), but I agree that there is a chance that they could cause issue
on platforms where BIOS didn't give ASPM control to the OS.

But as a non-ACPI developer, I don't know what does 'ACPI doesn't give
permission to manage ASPM' mean exactly. Does ACPI allow to disable ASPM but not
enable it?

- Mani

> -----
> From: Ilpo Järvinen <ilpo.jarvinen at linux.intel.com>
> Subject: [PATCH] PCI/ASPM: Always disable ASPM when driver requests it
> 
> 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 at linux.intel.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 5721ebfdea71..11732031e342 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1382,16 +1382,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);
> @@ -1418,13 +1425,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)
>  {
> 
> -- 
> tg: (9f4972a5d481..) aspm/disable-always (depends on: pci/set-default-comment2)


-- 
மணிவண்ணன் சதாசிவம்



More information about the ath12k mailing list