[v3,05/10] PCI: Store all PCIe Supported Link Speeds

  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.

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

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/probe.c           | 28 +++++++++++++++++++++++++++-
 include/linux/pci.h           |  1 +
 include/uapi/linux/pci_regs.h |  1 +
 3 files changed, 29 insertions(+), 1 deletion(-)
  

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
  

On Sat, Dec 30, 2023 at 12:45:49PM +0100, Lukas Wunner wrote:
> 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?

FWIW I went and re-read the internal review I provided on May 18.
Turns out I already mentioned back then that gaps aren't permitted:

 "Per PCIe r6.0.1 sec 8.2.1, the bitfield in the Link Capabilities 2
  register is not permitted to contain gaps between maximum supported
  speed and lowest possible speed (2.5 GT/s Gen1)."


> Also, I note that pci_set_bus_speed() doesn't use LNKCAP2.

About that, I wrote in May:

 "Actually, scratch that.  pci_set_bus_speed() is fine.  Since it's only
  interested in the *maximum* link speed, reading just LnkCap is correct.
  LnkCap2 only needs to be read to determine if a certain speed is
  *supported*.  E.g., even though 32 GT/s are supported, perhaps 16 GT/s
  are not.

  It's rather pcie_get_speed_cap() which should be changed.  There's
  no need for it to read LnkCap2.  The commit which introduced this,
  6cf57be0f78e, was misguided and had to be fixed up with f1f90e254e46.
  It could be simplified to just read LnkCap and return
  pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS].  If the device is a
  Root Port or Downstream Port, it doesn't even have to do that but
  could return the cached value in subordinate->max_bus_speed.
  If you add another attribute to struct pci_bus for the downstream
  device's maximum speed, the maximum speed for Endpoints and Upstream
  Ports could be returned directly as well from that attribute."

Thanks,

Lukas
  

On Sat, 30 Dec 2023, Lukas Wunner wrote:

> On Sat, Dec 30, 2023 at 12:45:49PM +0100, Lukas Wunner wrote:
> > 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?
> 
> FWIW I went and re-read the internal review I provided on May 18.
> Turns out I already mentioned back then that gaps aren't permitted:
> 
>  "Per PCIe r6.0.1 sec 8.2.1, the bitfield in the Link Capabilities 2
>   register is not permitted to contain gaps between maximum supported
>   speed and lowest possible speed (2.5 GT/s Gen1)."
> 
> 
> > Also, I note that pci_set_bus_speed() doesn't use LNKCAP2.
> 
> About that, I wrote in May:
> 
>  "Actually, scratch that.  pci_set_bus_speed() is fine.  Since it's only
>   interested in the *maximum* link speed, reading just LnkCap is correct.
>   LnkCap2 only needs to be read to determine if a certain speed is
>   *supported*.  E.g., even though 32 GT/s are supported, perhaps 16 GT/s
>   are not.
> 
>   It's rather pcie_get_speed_cap() which should be changed.  There's
>   no need for it to read LnkCap2.  The commit which introduced this,
>   6cf57be0f78e, was misguided and had to be fixed up with f1f90e254e46.
>   It could be simplified to just read LnkCap and return
>   pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS].  If the device is a
>   Root Port or Downstream Port, it doesn't even have to do that but
>   could return the cached value in subordinate->max_bus_speed.
>   If you add another attribute to struct pci_bus for the downstream
>   device's maximum speed, the maximum speed for Endpoints and Upstream
>   Ports could be returned directly as well from that attribute."

I know it's quite far back so it's understandable to forget :-), 
but already by May 23rd your position had changed and you wrote this:

'Per the Implementation Note at the end of PCIe r6.0.1 sec 7.5.3.18,

   "It is strongly encouraged that software primarily utilize the
    Supported Link Speeds Vector instead of the Max Link Speed field,
    so that software can determine the exact set of supported speeds on
    current and future hardware. This can avoid software being confused
    if a future specification defines Links that do not require support
    for all slower speeds."

This means that it's not sufficient if you just check that the desired 
speed is lower than the maximum.  Instead, you should check if the bit 
corresponding to the desired speed is set in the LnkCap2 register's 
Supported Link Speeds Vector.

PCIe r6.0.1 sec 8.2.1 stipulates that the bitfield is not permitted to 
contain gaps between maximum supported speed and lowest possible speed
(2.5 GT/s Gen1).  However the Implementation Note suggests that rule may 
no longer apply in future revisions of the PCIe Base Spec.'

So I'd assume I should still follow the way spec recommends, not the "old 
method" that may not function correctly after some future version of the 
spec, or have you really changed position once again on this?
  

On Mon, Jan 01, 2024 at 06:26:40PM +0200, Ilpo Järvinen wrote:
> On Sat, 30 Dec 2023, Lukas Wunner wrote:
> 
> > On Sat, Dec 30, 2023 at 12:45:49PM +0100, Lukas Wunner wrote:
> > > 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?
> > 
> > FWIW I went and re-read the internal review I provided on May 18.
> > Turns out I already mentioned back then that gaps aren't permitted:
> > 
> >  "Per PCIe r6.0.1 sec 8.2.1, the bitfield in the Link Capabilities 2
> >   register is not permitted to contain gaps between maximum supported
> >   speed and lowest possible speed (2.5 GT/s Gen1)."
> > 
> > 
> > > Also, I note that pci_set_bus_speed() doesn't use LNKCAP2.
> > 
> > About that, I wrote in May:
> > 
> >  "Actually, scratch that.  pci_set_bus_speed() is fine.  Since it's only
> >   interested in the *maximum* link speed, reading just LnkCap is correct.
> >   LnkCap2 only needs to be read to determine if a certain speed is
> >   *supported*.  E.g., even though 32 GT/s are supported, perhaps 16 GT/s
> >   are not.
> > 
> >   It's rather pcie_get_speed_cap() which should be changed.  There's
> >   no need for it to read LnkCap2.  The commit which introduced this,
> >   6cf57be0f78e, was misguided and had to be fixed up with f1f90e254e46.
> >   It could be simplified to just read LnkCap and return
> >   pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS].  If the device is a
> >   Root Port or Downstream Port, it doesn't even have to do that but
> >   could return the cached value in subordinate->max_bus_speed.
> >   If you add another attribute to struct pci_bus for the downstream
> >   device's maximum speed, the maximum speed for Endpoints and Upstream
> >   Ports could be returned directly as well from that attribute."
> 
> I know it's quite far back so it's understandable to forget :-), 
> but already by May 23rd your position had changed and you wrote this:
> 
> 'Per the Implementation Note at the end of PCIe r6.0.1 sec 7.5.3.18,
> 
>    "It is strongly encouraged that software primarily utilize the
>     Supported Link Speeds Vector instead of the Max Link Speed field,
>     so that software can determine the exact set of supported speeds on
>     current and future hardware. This can avoid software being confused
>     if a future specification defines Links that do not require support
>     for all slower speeds."
> 
> This means that it's not sufficient if you just check that the desired 
> speed is lower than the maximum.  Instead, you should check if the bit 
> corresponding to the desired speed is set in the LnkCap2 register's 
> Supported Link Speeds Vector.
> 
> PCIe r6.0.1 sec 8.2.1 stipulates that the bitfield is not permitted to 
> contain gaps between maximum supported speed and lowest possible speed
> (2.5 GT/s Gen1).  However the Implementation Note suggests that rule may 
> no longer apply in future revisions of the PCIe Base Spec.'
> 
> So I'd assume I should still follow the way spec recommends, not the "old 
> method" that may not function correctly after some future version of the 
> spec, or have you really changed position once again on this?

I haven't, you're right, I forgot about all those details.
Thanks for that blast from the past. ;)

But it would be good to extend the commit message because without all
that context, it's difficult to understand why the max_bus_speed isn't
sufficient.

Lukas
  

On Mon, 1 Jan 2024, Lukas Wunner wrote:

> On Mon, Jan 01, 2024 at 06:26:40PM +0200, Ilpo Järvinen wrote:
> > On Sat, 30 Dec 2023, Lukas Wunner wrote:
> > 
> > > On Sat, Dec 30, 2023 at 12:45:49PM +0100, Lukas Wunner wrote:
> > > > 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?
> > > 
> > > FWIW I went and re-read the internal review I provided on May 18.
> > > Turns out I already mentioned back then that gaps aren't permitted:
> > > 
> > >  "Per PCIe r6.0.1 sec 8.2.1, the bitfield in the Link Capabilities 2
> > >   register is not permitted to contain gaps between maximum supported
> > >   speed and lowest possible speed (2.5 GT/s Gen1)."
> > > 
> > > 
> > > > Also, I note that pci_set_bus_speed() doesn't use LNKCAP2.
> > > 
> > > About that, I wrote in May:
> > > 
> > >  "Actually, scratch that.  pci_set_bus_speed() is fine.  Since it's only
> > >   interested in the *maximum* link speed, reading just LnkCap is correct.
> > >   LnkCap2 only needs to be read to determine if a certain speed is
> > >   *supported*.  E.g., even though 32 GT/s are supported, perhaps 16 GT/s
> > >   are not.
> > > 
> > >   It's rather pcie_get_speed_cap() which should be changed.  There's
> > >   no need for it to read LnkCap2.  The commit which introduced this,
> > >   6cf57be0f78e, was misguided and had to be fixed up with f1f90e254e46.
> > >   It could be simplified to just read LnkCap and return
> > >   pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS].  If the device is a
> > >   Root Port or Downstream Port, it doesn't even have to do that but
> > >   could return the cached value in subordinate->max_bus_speed.
> > >   If you add another attribute to struct pci_bus for the downstream
> > >   device's maximum speed, the maximum speed for Endpoints and Upstream
> > >   Ports could be returned directly as well from that attribute."
> > 
> > I know it's quite far back so it's understandable to forget :-), 
> > but already by May 23rd your position had changed and you wrote this:
> > 
> > 'Per the Implementation Note at the end of PCIe r6.0.1 sec 7.5.3.18,
> > 
> >    "It is strongly encouraged that software primarily utilize the
> >     Supported Link Speeds Vector instead of the Max Link Speed field,
> >     so that software can determine the exact set of supported speeds on
> >     current and future hardware. This can avoid software being confused
> >     if a future specification defines Links that do not require support
> >     for all slower speeds."
> > 
> > This means that it's not sufficient if you just check that the desired 
> > speed is lower than the maximum.  Instead, you should check if the bit 
> > corresponding to the desired speed is set in the LnkCap2 register's 
> > Supported Link Speeds Vector.
> > 
> > PCIe r6.0.1 sec 8.2.1 stipulates that the bitfield is not permitted to 
> > contain gaps between maximum supported speed and lowest possible speed
> > (2.5 GT/s Gen1).  However the Implementation Note suggests that rule may 
> > no longer apply in future revisions of the PCIe Base Spec.'
> > 
> > So I'd assume I should still follow the way spec recommends, not the "old 
> > method" that may not function correctly after some future version of the 
> > spec, or have you really changed position once again on this?
> 
> I haven't, you're right, I forgot about all those details.
> Thanks for that blast from the past. ;)
>
> But it would be good to extend the commit message because without all
> that context, it's difficult to understand why the max_bus_speed isn't
> sufficient.

Thanks. I'll extend the commit message.