Re: [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt"

[Bjorn Helgaas <helgaas@kernel.org>]

Follow subject line capitalization conventions:

  $ git log --oneline drivers/pci/probe.c
  661c4c4f2693 PCI: Let pcibios_root_bridge_prepare() access bridge->windows
  d2c64f98c387 PCI: Use pci_find_vsec_capability() when looking for TBT devices
  fd1ae23b495b PCI: Prefer 'unsigned int' over bare 'unsigned'
  e1b0d0bb2032 PCI: Re-enable Downstream Port LTR after reset or hotplug
  7c3855c423b1 PCI: Coalesce host bridge contiguous apertures
  06dc660e6eb8 PCI: Rename pcibios_add_device() to pcibios_device_add()
  41dd40fd7179 PCI: Support populating MSI domains of root buses via bridges

On Fri, Feb 04, 2022 at 12:28:20PM -0600, Mario Limonciello wrote:

Add an intro sentence to tell us what this patch does (mark USB4
devices as 'is_thunderbolt'.  I know it's in the subject, but people
read the commit log separately and it should be complete in itself.
A title is not the first sentence of a book.

> Downstream drivers use this information to declare functional
> differences in how the drivers performed by knowing that they are
> connected to an upstream TBT/USB4 port.

s/Downstream drivers/Drivers of downstream devices/
s/performed/perform/  (I guess?)

I'm guessing this really refers to differences in how *devices*
(not drivers) perform when they are below a Thunderbolt or USB port.

I've never liked "is_thunderbolt" because it tells us nothing about
what functionality is of interest, so it's an unmaintainable mess.

Right now:

  - We assume Root Ports and Switch Ports marked "is_thunderbolt"
    support D3 (pci_bridge_d3_possible()).

  - Downstream Ports marked "is_thunderbolt" don't support native
    hotplug Command Completed events, even if they claim they do
    (pcie_init()).

  - Apparently, if *any* device in the system is marked
    "is_thunderbolt", a GPU external DP port is not fully switchable
    because ? (gmux_probe()).

  - Whether an AMD GPU is attached via Thunderbolt tells us something
    about what sort of power control and runtime power management we
    can do (amdgpu_driver_load_kms(), radeon_driver_load_kms()).

  - We don't register Thunderbolt eGPU devices with VGA switcheroo
    because ? (nouveau_vga_init(), radeon_device_init()).

  - If an AMD GPU is attached via Thunderbolt, we program different
    ASPM time values because ? (nbio_v2_3_enable_aspm()).

This is totally bonkers.

Broken things like hotplug Command Completed should be quirks.  That
one is my fault.

The ASPM thing should be integrated with the PCI core somehow.  It's
just asking for trouble to have the PCI core assuming *it* is managing
ASPM and then have drivers doing non-standard ASPM stuff behind its
back.

I have no idea about the VGA switching and power management stuff.

But it's not clear to me that these all fit under the "is_thunderbolt"
umbrella.  If they actually do correspond to Thunderbolt- or USB4-
specific features, we should include references to the pertinent
sections of the spec.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..b59f6c05e606 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1581,9 +1581,9 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
>  {
>  	u16 vsec;
>  
> -	/* Is the device part of a Thunderbolt controller? */
> +	/* Is the device part of a Thunderbolt or USB4 controller? */
>  	vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
> -	if (vsec)
> +	if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4)
>  		dev->is_thunderbolt = 1;

This could be rewritten as:

  if (dev->class == PCI_CLASS_SERIAL_USB_USB4 ||
      pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT))
          dev->is_thunderbolt = 1;

to avoid searching USB4 devices for a TBT capability unnecessarily.

>  }
>  
> -- 
> 2.34.1
>