Received-SPF: pass (google.com: domain of linux-kernel+bounces-13903-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 1 Jan 2024 18:26:40 +0200 (EET)
From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
cc: linux-pci@vger.kernel.org, Bjorn Helgaas <helgaas@kernel.org>, 
    Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, 
    Rob Herring <robh@kernel.org>, Krzysztof Wilczy??ski <kw@linux.com>, 
    Alexandru Gagniuc <mr.nuke.me@gmail.com>, 
    Krishna chaitanya chundru <quic_krichai@quicinc.com>, 
    Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, 
    "Rafael J . Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org, 
    Bjorn Helgaas <bhelgaas@google.com>, LKML <linux-kernel@vger.kernel.org>, 
    Alex Deucher <alexdeucher@gmail.com>, 
    Daniel Lezcano <daniel.lezcano@linaro.org>, 
    Amit Kucheria <amitk@kernel.org>, Zhang Rui <rui.zhang@intel.com>
Subject: Re: [PATCH v3 05/10] PCI: Store all PCIe Supported Link Speeds
In-Reply-To: <20231230193000.GA11331@wunner.de>
Message-ID: <5dd84bbf-2e5-dded-bf49-f4db17b265f0@linux.intel.com>
References: <20230929115723.7864-1-ilpo.jarvinen@linux.intel.com> <20230929115723.7864-6-ilpo.jarvinen@linux.intel.com> <20231230114549.GB12257@wunner.de> <20231230193000.GA11331@wunner.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323329-737575340-1704125951=:2521"
Content-ID: <a0e641d6-2894-330-dff2-2c4cfef8318@linux.intel.com>

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-737575340-1704125951=:2521
Content-Type: text/plain; CHARSET=ISO-8859-15
Content-Transfer-Encoding: 8BIT
Content-ID: <e45f3096-c420-8925-b335-96f1482f6de9@linux.intel.com>

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.
--8323329-737575340-1704125951=:2521--