linux-kernel - Re: [PATCH v3 05/10] PCI: Store all PCIe Supported Link Speeds

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]

Date: Sat, 30 Dec 2023 12:45:49 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Rob Herring <robh@...nel.org>, Krzysztof Wilczy??ski <kw@...ux.com>,
	Alexandru Gagniuc <mr.nuke.me@...il.com>,
	Krishna chaitanya chundru <quic_krichai@...cinc.com>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	"Rafael J . Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	Alex Deucher <alexdeucher@...il.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Amit Kucheria <amitk@...nel.org>, Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH v3 05/10] PCI: Store all PCIe Supported Link Speeds

On Fri, Sep 29, 2023 at 02:57:18PM +0300, Ilpo Järvinen wrote:
> struct pci_bus stores max_bus_speed. Implementation Note in PCIe r6.0.1
> sec 7.5.3.18, however, recommends determining supported Link Speeds
> using the Supported Link Speeds Vector in the Link Capabilities 2
> Register (when available).
> 
> Add pcie_bus_speeds into struct pci_bus which caches the Supported Link
> Speeds. The value is taken directly from the Supported Link Speeds
> Vector or synthetized from the Max Link Speed in the Link Capabilities
> Register when the Link Capabilities 2 Register is not available.

Remind me, what's the reason again to cache this and why is
max_bus_speed not sufficient?  Is the point that there may be
"gaps" in the supported link speeds, i.e. not every bit below
the maximum supported speed may be set?  And you need to skip
over those gaps when throttling to a lower speed?

Maybe this becomes apparent in a later patch but from a reviewer's
perspective starting at patch 1 and working one's way forward through
the series, it's a bit puzzling, so an explanation in the commit
message would be beneficial.

> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
[...]
> +static u8 pcie_get_supported_speeds(u32 linkcap, u32 linkcap2)
> +{
> +	u8 speeds;
> +
> +	speeds = linkcap2 & PCI_EXP_LNKCAP2_SLS;
> +	if (speeds)
> +		return speeds;
> +
> +	/*
> +	 * Synthetize supported link speeds from the Max Link Speed in the
> +	 * Link Capabilities Register.
> +	 */
> +	speeds = PCI_EXP_LNKCAP2_SLS_2_5GB;
> +	if ((linkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
> +		speeds |= PCI_EXP_LNKCAP2_SLS_5_0GB;
> +	return speeds;
> +}

This seems to duplicate portions of pcie_get_speed_cap().

Can you refactor that function to take advantage of the cached value,
i.e. basically return PCIE_LNKCAP2_SLS2SPEED(dev->bus->pcie_bus_speeds)?

Also, I note that pci_set_bus_speed() doesn't use LNKCAP2.
Presumably that's a historic artefact but maybe it can be
converted to use LNKCAP2 as well.  Granted, it's not directly
related to this series, but always nice to clean up and
rationalize the code.

Thanks,

Lukas