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
> > -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.
- Mani
--
மணிவண்ணன் சதாசிவம்
