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.
