[v2] PCI/portdrv: Disable bwctrl service if port is fixed at 2.5 GT/s

Trying to enable bwctrl on an Intel JHL7540 (Titan Ridge) based
Thunderbolt port causes a boot hang on at least some systems though the
exact reason is not yet understood. As per the spec Thunderbolt PCIe
Downstream Ports have a fake Max Link Speed of 2.5 GT/s (USB4 v2 sec
11.2.1):

   "Max Link Speed field in the Link Capabilities Register set to 0001b
    (data rate of 2.5 GT/s only).
    Note: These settings do not represent actual throughput.
    Throughput is implementation specific and based on the USB4 Fabric
    performance."

More generally if 2.5 GT/s is the only supported link speed there is no
point in throtteling as this is already the lowest possible PCIe speed
so don't advertise the capability stopping bwctrl from being probed on
these ports.

The PCIe r6.2 specification section 7.5.3.18 recommends to primarily
utilize the Supported Link Speeds Vector instead of the Max Link Speeds
field to prevent confusion if future specifications allow devices not
to support lower speeds. This concern does not apply however when
specifically targeting devices claiming support only for 2.5 GT/s.

Link: https://lore.kernel.org/linux-pci/Z1R4VNwCOlh9Sg9n@wunner.de/
Fixes: 665745f27487 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller")
Tested-by: Niklas Schnelle <niks@kernel.org>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Niklas Schnelle <niks@kernel.org>
---
Note: This issue causes a boot hang on my personal workstation.

While there is an ongoing discussion about generalizing this to all
devices with a single supported speed. It turns out however that in my
case dev->supported_speeds incorrectly claims 2.5-8 GT/s requiring
a seperate second fix. So in the interest of simplicity and because I'll
be out from the 19th until January, I'd like to propose to do this simple
fix to the boot hang now and take the time to figure out a more general
approach afterwards.
---
Changes in v2:
- Improve commit message to mention the specific controller and
  why using the Max Link Speeds field should be fine here.
- Add a comment (Lukas)
- Add R-b's (no change to logic).
- Link to v1: https://lore.kernel.org/r/20241207-fix_bwctrl_thunderbolt-v1-1-b711f572a705@kernel.org
---
 drivers/pci/pcie/portdrv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241207-fix_bwctrl_thunderbolt-bd1f96b3d98f

Best regards,

On Fri, Dec 13, 2024 at 9:56 PM Niklas Schnelle <niks@kernel.org> wrote:
> Trying to enable bwctrl on an Intel JHL7540 (Titan Ridge) based
> Thunderbolt port causes a boot hang on at least some systems though the
> exact reason is not yet understood. As per the spec Thunderbolt PCIe
> Downstream Ports have a fake Max Link Speed of 2.5 GT/s (USB4 v2 sec
> 11.2.1):
>
>    "Max Link Speed field in the Link Capabilities Register set to 0001b
>     (data rate of 2.5 GT/s only).
>     Note: These settings do not represent actual throughput.
>     Throughput is implementation specific and based on the USB4 Fabric
>     performance."
>
> More generally if 2.5 GT/s is the only supported link speed there is no
> point in throtteling as this is already the lowest possible PCIe speed
> so don't advertise the capability stopping bwctrl from being probed on
> these ports.
>
> The PCIe r6.2 specification section 7.5.3.18 recommends to primarily
> utilize the Supported Link Speeds Vector instead of the Max Link Speeds
> field to prevent confusion if future specifications allow devices not
> to support lower speeds. This concern does not apply however when
> specifically targeting devices claiming support only for 2.5 GT/s.
>
> Link: https://lore.kernel.org/linux-pci/Z1R4VNwCOlh9Sg9n@wunner.de/
> Fixes: 665745f27487 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller")
> Tested-by: Niklas Schnelle <niks@kernel.org>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Niklas Schnelle <niks@kernel.org>
> ---
> Note: This issue causes a boot hang on my personal workstation.
>
> While there is an ongoing discussion about generalizing this to all
> devices with a single supported speed. It turns out however that in my
> case dev->supported_speeds incorrectly claims 2.5-8 GT/s requiring
> a seperate second fix. So in the interest of simplicity and because I'll
> be out from the 19th until January, I'd like to propose to do this simple
> fix to the boot hang now and take the time to figure out a more general
> approach afterwards.
> ---
> Changes in v2:
> - Improve commit message to mention the specific controller and
>   why using the Max Link Speeds field should be fine here.
> - Add a comment (Lukas)
> - Add R-b's (no change to logic).
> - Link to v1: https://lore.kernel.org/r/20241207-fix_bwctrl_thunderbolt-v1-1-b711f572a705@kernel.org

This is now commit e50e27a613db6f18 ("PCI/portdrv: Disable bwctrl
service if port is fixed at 2.5 GT/s") in pci/next, which conflicts
with  commit 774c71c52aa48700 ("PCI/bwctrl: Enable only if more than
one speed is supported") in v6.13-rc4.

Gr{oetje,eeting}s,

                        Geert

On Tue, 2025-01-14 at 09:50 +0100, Geert Uytterhoeven wrote:
> On Fri, Dec 13, 2024 at 9:56 PM Niklas Schnelle <niks@kernel.org> wrote:
> > Trying to enable bwctrl on an Intel JHL7540 (Titan Ridge) based
> > Thunderbolt port causes a boot hang on at least some systems though the
> > exact reason is not yet understood. As per the spec Thunderbolt PCIe
> > Downstream Ports have a fake Max Link Speed of 2.5 GT/s (USB4 v2 sec
> > 11.2.1):
> > 
> >    "Max Link Speed field in the Link Capabilities Register set to 0001b
> >     (data rate of 2.5 GT/s only).
> >     Note: These settings do not represent actual throughput.
> >     Throughput is implementation specific and based on the USB4 Fabric
> >     performance."
> > 
> > More generally if 2.5 GT/s is the only supported link speed there is no
> > point in throtteling as this is already the lowest possible PCIe speed
> > so don't advertise the capability stopping bwctrl from being probed on
> > these ports.
> > 
> > The PCIe r6.2 specification section 7.5.3.18 recommends to primarily
> > utilize the Supported Link Speeds Vector instead of the Max Link Speeds
> > field to prevent confusion if future specifications allow devices not
> > to support lower speeds. This concern does not apply however when
> > specifically targeting devices claiming support only for 2.5 GT/s.
> > 
> > Link: https://lore.kernel.org/linux-pci/Z1R4VNwCOlh9Sg9n@wunner.de/
> > Fixes: 665745f27487 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller")
> > Tested-by: Niklas Schnelle <niks@kernel.org>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Niklas Schnelle <niks@kernel.org>
> > ---
> > Note: This issue causes a boot hang on my personal workstation.
> > 
> > While there is an ongoing discussion about generalizing this to all
> > devices with a single supported speed. It turns out however that in my
> > case dev->supported_speeds incorrectly claims 2.5-8 GT/s requiring
> > a seperate second fix. So in the interest of simplicity and because I'll
> > be out from the 19th until January, I'd like to propose to do this simple
> > fix to the boot hang now and take the time to figure out a more general
> > approach afterwards.
> > ---
> > Changes in v2:
> > - Improve commit message to mention the specific controller and
> >   why using the Max Link Speeds field should be fine here.
> > - Add a comment (Lukas)
> > - Add R-b's (no change to logic).
> > - Link to v1: https://lore.kernel.org/r/20241207-fix_bwctrl_thunderbolt-v1-1-b711f572a705@kernel.org
> 
> This is now commit e50e27a613db6f18 ("PCI/portdrv: Disable bwctrl
> service if port is fixed at 2.5 GT/s") in pci/next, which conflicts
> with  commit 774c71c52aa48700 ("PCI/bwctrl: Enable only if more than
> one speed is supported") in v6.13-rc4.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert

Oh, that's not right, good catch. This patch is superseded by
"PCI/bwctrl: Enable only if more than one speed is supported" and
should be dropped. Sorry if that wasn't clear from the the limited
context of this patch discussion. @Bjorn I think this needs to be taken
care of by you?

Thanks,
Niklas

Hello,

[...]
> > This is now commit e50e27a613db6f18 ("PCI/portdrv: Disable bwctrl
> > service if port is fixed at 2.5 GT/s") in pci/next, which conflicts
> > with  commit 774c71c52aa48700 ("PCI/bwctrl: Enable only if more than
> > one speed is supported") in v6.13-rc4.
> > 
> > Gr{oetje,eeting}s,
> > 
> >                         Geert
> 
> Oh, that's not right, good catch. This patch is superseded by
> "PCI/bwctrl: Enable only if more than one speed is supported" and
> should be dropped.

Done.

> Sorry if that wasn't clear from the the limited context of this patch
> discussion. @Bjorn I think this needs to be taken care of by you?

No worries.  I removed it from our next branch and marked accordingly in
Patchwork.

	Krzysztof