linux-kernel - Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <gwpn5no4i2wwwk3qo4tcgp6z6floo6k2ymzfuucg3tbwtmccte@ydsupmrd2nqu>
Date: Mon, 14 Jul 2025 19:21:21 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Jingoo Han <jingoohan1@...il.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Rob Herring <robh@...nel.org>, Jeff Johnson <jjohnson@...nel.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
	linux-arm-msm@...r.kernel.org, mhi@...ts.linux.dev, linux-wireless@...r.kernel.org, 
	ath11k@...ts.infradead.org, qiang.yu@....qualcomm.com, quic_vbadigan@...cinc.com, 
	quic_vpernami@...cinc.com, quic_mrana@...cinc.com, 
	Jeff Johnson <jeff.johnson@....qualcomm.com>
Subject: Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of
 __pci_enable_link_state()

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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்