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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 31 Jul 2026, Manivannan Sadhasivam wrote:
> On Fri, Jul 10, 2026 at 03:25:46PM -0500, Bjorn Helgaas wrote:
> > 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.
> > 
> 
> This was discussed in v2 and Ath maintainer Jeff was in agreement with the
> dependency:
> https://lore.kernel.org/linux-pci/705a4fe5-658e-25ac-9e4d-6b8089abca46@linux.intel.com

To further that, any driver that has to disable ASPM clearly does depend 
on capacility to disable ASPM. So having depends on PCIEASPM makes perfect 
sense in these cases. I suppose the status quo of drivers doing that on 
their own has just gotten many blind for that obvious dependency.

In any case, PCIEASPM is gated by EXPERT so one should really know what 
one is doing if one is goind to disable it, if that ends up losing some 
driver that depends on PCIEASPM important for that config minimalist, 
clearly the person wasn't expert enough to touch that config option :-).

So I think sashiko is simply wrong in this case, and having empty stubs is 
wrong as well because it encourages drivers to do driver-side ASPM disable 
behind core's ASPM back.

I don't see much benefit from letting these drivers to build without 
PCIEASPM (the non-empty "stubs" approach Bjorn suggests). Is that ability 
to satisfy some extreme config minimalist who wants to partially disable 
ASPM on system that has a broken HW? Or just to make sashiko happy? Why 
make things harder than what "depends on PCIEASPM" already provides for 
us, regardless of sashiko thinking we "lose" something in that trade.

I, on the other hand, think it's win to properly mark drivers that need 
PCIEASPM with depends on, even if it's just for the capability to disable 
ASPM.

-- 
 i.