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

[Geert Uytterhoeven]

On Fri, Dec 13, 2024 at 9:56 PM Niklas Schnelle <niks@xxxxxxxxxx> 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@xxxxxxxxx/
> Fixes: 665745f27487 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller")
> Tested-by: Niklas Schnelle <niks@xxxxxxxxxx>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Signed-off-by: Niklas Schnelle <niks@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds