linux-kernel - Re: [PATCH v8 2/8] 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]

Message-ID: <20241017112511.0000503b@Huawei.com>
Date: Thu, 17 Oct 2024 11:25:11 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-pci@...r.kernel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, "Lorenzo
 Pieralisi" <lorenzo.pieralisi@....com>, Rob Herring <robh@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>, "Maciej W. Rozycki"
	<macro@...am.me.uk>, Lukas Wunner <lukas@...ner.de>, 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>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@....com>, <linux-kernel@...r.kernel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>, Amit Kucheria <amitk@...nel.org>,
	Zhang Rui <rui.zhang@...el.com>, Christophe JAILLET
	<christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH v8 2/8] PCI: Store all PCIe Supported Link Speeds

On Wed,  9 Oct 2024 12:52:17 +0300
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> wrote:


> 
> supported_speeds field keeps the extra reserved zero at the least
> significant bit to match the Link Capabilities 2 Register layouting.
layout.

> 
> An attempt was made to store supported_speeds field into the struct
> pci_bus as an intersection of the both ends of the Link, however, the
> subordinate struct pci_bus is not available early enough. The Target
> Speed quirk (in pcie_failed_link_retrain()) can run either during
> initial scan or later requiring it to use the API PCIe bandwidth
> controller provides to set the Target Link Speed in order to co-exist
> with the bandwidth controller. When the Target Speed quirk is calling
> the bandwidth controller during initial scan, the struct pci_bus is not
> yet initialized. As such, storing supported_speeds into the struct
> pci_bus is not viable.

Excellent detailed description!

> 
> Suggested-by: Lukas Wunner <lukas@...ner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
One trivial request inline.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

> ---
>  drivers/pci/pci.c             | 58 ++++++++++++++++++++++++-----------
>  drivers/pci/probe.c           |  3 ++
>  include/linux/pci.h           | 11 ++++++-
>  include/uapi/linux/pci_regs.h |  1 +
>  4 files changed, 54 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7d85c04fbba2..b28ab4761b18 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

> -enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
> +u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  {
>  	u32 lnkcap2, lnkcap;
> +	u8 speeds;
>  
> -	/*
> -	 * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18.  The
> -	 * implementation note there recommends using the Supported Link
> -	 * Speeds Vector in Link Capabilities 2 when supported.
> -	 *
> -	 * Without Link Capabilities 2, i.e., prior to PCIe r3.0, software
> -	 * should use the Supported Link Speeds field in Link Capabilities,
> -	 * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
> -	 */
>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
> +	speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS;

I'd be tempted to leave a breadcrumb here in the form of a comment
to stop the obviously 'correct' but totally wrong FIELD_GET()
being used to replace this because of that LSB 0.
You have it well commented elsewhere but anyone editing this
code might not look at where it is written to.

(I'd written that feedback before realizing my error ;)
>