linux-kernel - Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

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: <20251107152547.GA1998555@bhelgaas>
Date: Fri, 7 Nov 2025 09:25:47 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: linux-pci@...r.kernel.org, Christian Zigotzky <chzigotzky@...osoft.de>,
	Manivannan Sadhasivam <mani@...nel.org>,
	mad skateman <madskateman@...il.com>,
	"R . T . Dickinson" <rtd2@...a.co.nz>,
	Darren Stevens <darren@...vens-zone.net>,
	John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
	luigi burdo <intermediadc@...mail.com>, Al <al@...azap.net>,
	Roland <rol7and@....com>, Hongxing Zhu <hongxing.zhu@....com>,
	hypexed@...oo.com.au, linuxppc-dev@...ts.ozlabs.org,
	debian-powerpc@...ts.debian.org, linux-kernel@...r.kernel.org,
	Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can
 override them

On Fri, Nov 07, 2025 at 06:32:14AM +0100, Lukas Wunner wrote:
> On Thu, Nov 06, 2025 at 12:36:38PM -0600, Bjorn Helgaas wrote:
> > Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> > remove features to avoid hardware defects.  The idea is:
> > 
> >   - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
> >     dev->lnkcap
> > 
> >   - HEADER quirks can update the cached dev->lnkcap to remove advertised
> >     features that don't work correctly
> > 
> >   - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
> >     advertised there
> 
> I realize that memory is cheap, but it still feels a bit wasteful
> to cache the entire 32-bit register wholesale.  It contains
> reserved bits as of PCIe r7.0, various uninteresting bits and
> portions of it are already cached elsewhere and thus now duplicated.
> I'm wondering if it would make sense to instead only cache the ASPM bits
> that are relevant here?  That's the approach we've followed so far.

My first try (which I didn't post) cached only the two bits we need
for this.  It's not awful, and the aspm.c patch was smaller, so maybe
it's the right approach, at least for v6.18.

One thing I didn't like about pci_disable_aspm_cap() (which I know you
said you *did* like :)) is that it adds a layer of indirection.  I
like having PCI_EXP_LNKCAP_ASPM_L0S in the quirk because it's more
directly connected to the spec and the hardware register, and grep
works better for code readers.

But if we only cache the ASPM cap bits, we would need
pci_disable_aspm_cap() to manage converting PCI_EXP_LNKCAP_ASPM_L0S or
PCIE_LINK_STATE_L0S to the right place.

(A bit of a tangent, but I've never liked the PCIE_LINK_STATE_* bits
because they look like they ought to be register bits, but they're
not.  I think the code would be improved overall if we could remove
them.)

> You're initializing the link_active_reporting bit from the newly
> cached lnkcap register, I'd prefer having a static inline instead
> which extracts the bit from the cached register on demand,
> thus obviating the need to have a duplicate cached copy of the bit.
> 
> pci_set_bus_speed() caches bus->max_bus_speed from the Link
> Capabilities register and isn't converted by this patch to use
> the cached register.  There are various others, e.g.
> get_port_device_capability() in drivers/pci/pcie/portdrv.c
> could also get PCI_EXP_LNKCAP_LBNC from the cached lnkcap
> register.  Same for pcie_get_supported_speeds().  If the
> intention is to convert these in a separate step in v6.19,
> it would be good to mention that in the changelog.

I agree with all of that, and there are several other PCI_EXP_LNKCAP
reads that could be replaced, but that would have to be for v6.19.

Bjorn