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: <84f1ddef-b4a1-958f-96f1-c84f2fe0baf3@linux.intel.com>
Date: Sun, 13 Jul 2025 19:38:09 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
cc: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, 
    Manivannan Sadhasivam <mani@...nel.org>, 
    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 Fri, 11 Jul 2025, 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?

It was added to match the disable side despite not having users. I don't 
oppose dropping the unused one.

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

There seem to be some drivers which have issues if certain ASPM states are 
enabled during some phase of operation, so they'd want to disable ASPM 
for a while and re-enable it after past the danger zone, which is why I 
had to create the symmetric pair for disabling states in a series trying 
to remove custom ASPM code from other drivers (these patches are extracted 
from that series). Currently those drivers mess with LNKCTL directly.

(The driver changes we not finished because it seemed I'd have needed to 
add some ops to allow writing HW specific registers of ASPM state change 
which the ASPM driver could invoke to infor the driver about state 
changes.)

> 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.
>
> I think the compiled-in aspm_policy default and the module parameters
> are basically chicken switches that only exist because aspm.c and some
> devices aren't robust enough.

There's also too much custom code in drivers currently.

-- 
 i.