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

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Jul 14, 2025 at 07:21:21PM +0530, 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.

Pffft.  Device-specific configuration when changing ASPM state is a
device defect in my book.  ASPM is clearly designed to be managed
outside the driver.  What is the driver supposed to do about this if
the platform retains control of ASPM?  There's no possibility of a
driver hook for that.

> 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!

I haven't looked at the implementation here, but I don't think drivers
should check LNKCTL to decide this.  If the device doesn't work with
L0s or L1 or whatever, that's what the driver should tell us.

Bjorn