Re: [PATCH v8 2/8] PCI: Store all PCIe Supported Link Speeds

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed,  9 Oct 2024 12:52:17 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.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@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
One trivial request inline.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 ;)
>