Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

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

[-- Attachment #1: Type: text/plain, Size: 4278 bytes --]

On Mon, 14 Jul 2025, Manivannan Sadhasivam wrote:
> On Sun, Jul 13, 2025 at 07:27:57PM GMT, Ilpo Järvinen wrote:
> > On Sat, 12 Jul 2025, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> > > > On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> > > > 
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> > > > >  #ifdef CONFIG_PCIEASPM
> > > > >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> > > > >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > > > > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > > > 
> > > > AFAICT there's no caller of this at all.  Why do we keep it?
> > > > 
> > > 
> > > I'm just working on a series to convert the ath{10/11/12}k drivers to use this
> > > API instead of modifying LNKCTL register directly:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961
> > 
> > Great. I assume but "this API" you meant disable/enable link state that 
> > are real pair unlike the current pci_enable_link_state()?
> > 
> 
> Yes. I actually based my series on top of this series, but I guess I should just
> merge the API change patch to my series as it is relatively small compared to
> this one (so it will get merged quicker).
> 
> > Did ath1xk need to do some hw specific register updates when changing ASPM 
> > state?
> > 
> 
> Fortunately, no.
> 
> > I tried to do similar conversion in r8169 (and actually also ath1xk too) 
> > but it was a while ago already. If I understood the code correctly, r8169 
> > seems to write some HW specific registers when changing ASPM state so I 
> > would have likely need to add some ops for it to play nice with state 
> > changes not originating from the driver itself but from the ASPM driver, 
> > which is where the work then stalled.
> > 
> 
> ath driver changes are straightforward, but I need to add an inline function to
> convert lnkctl setting to aspm states:
> 
> 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;
> }
> 
> Currently, it is in ath/ath.h, but if you feel that we should move it to
> include/linux/pci.h, let me know!
> 
> > > > > -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> > > > 
> > > > We only have two callers of this (pcie-qcom.c and vmd.c, both in
> > > > drivers/pci/), so it's not clear to me that it needs to be in
> > > > include/linux/pci.h.
> > > > 
> > > > I'm a little dubious about it in the first place since I don't think
> > > > drivers should be enabling ASPM states on their own, but pcie-qcom.c
> > > > and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> > > > guess we can live with them for now.
> > > > 
> > > > IMO the "someday" goal should be that we get rid of aspm_policy and
> > > > enable all the available power saving states by default.  We have
> > > > sysfs knobs that administrators can use if necessary, and drivers or
> > > > quirks can disable states if they need to work around hardware
> > > > defects.
> > > 
> > > Yeah, I think the default should be powersave and let the users disable it for
> > > performance if they want.
> > 
> > I'm certainly not against improvements in this front, but I think we need 
> > to get rid off custom ASPM disable code from the drivers first.
> > 
> 
> It would be difficult IMO as drivers have their own usecases. For instance, the
> ath drivers need to disable ASPM states to avoid issues during firmware
> download. Then they need to enable ASPM back once the firmware is up and
> running.

I understand and I didn't meant that we should disallow drivers from 
touching ASPM state. I just meant we should convert the drivers to 
do such state changes using the ASPM driver (through disable/enable API) 
rather than what they currently do with LNKCTL directly behind the ASPM 
driver's back.

-- 
 i.