Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` attribute originally had a well defined list of
> quirks that it existed for, but it has been overloaded with more
> meaning.
> 
> Instead use the driver core removable attribute to indicate the
> detail a device is attached to a thunderbolt or USB4 chain.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/probe.c               | 2 +-
>  drivers/platform/x86/apple-gmux.c | 2 +-
>  include/linux/pci.h               | 5 ++---
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..1b752d425c47 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1584,7 +1584,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
>  	/* Is the device part of a Thunderbolt controller? */
>  	vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
>  	if (vsec)
> -		dev->is_thunderbolt = 1;
> +		dev->external_facing = true;

I assume there's a spec for the PCI_VSEC_ID_INTEL_TBT Capability.  Is
that public?  Does the spec say that a device with that capability
must be external-facing?

Even if it's not public, I think a citation (name, revision, section)
would be useful.

>  }
>  
>  static void set_pcie_untrusted(struct pci_dev *dev)
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 57553f9b4d1d..4444da0c39b0 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
>  
>  static int is_thunderbolt(struct device *dev, void *data)
>  {
> -	return to_pci_dev(dev)->is_thunderbolt;
> +	return to_pci_dev(dev)->external_facing;

This looks ... sort of weird.  I don't know anything about
apple-gmux.c, so I guess I don't care, but assuming any
external-facing device must be a Thunderbolt device seems like a
stretch.

Ugh.  This is used via "bus_for_each_dev(&pci_bus_type)", which means
it's not hotplug-safe.  I'm sure we "know" implicitly that hotplug
isn't an issue in apple-gmux, but it's better not to have examples
that get copied to places where it *is* an issue.

>  }
>  
>  static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1e5b769e42fc..d9719eb14654 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -442,7 +442,6 @@ struct pci_dev {
>  	unsigned int	is_virtfn:1;
>  	unsigned int	is_hotplug_bridge:1;
>  	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
> -	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
>  	unsigned int	no_cmd_complete:1;	/* Lies about command completed events */
>  
>  	/*
> @@ -2447,11 +2446,11 @@ static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
>  {
>  	struct pci_dev *parent = pdev;
>  
> -	if (pdev->is_thunderbolt)
> +	if (dev_is_removable(&pdev->dev))
>  		return true;
>  
>  	while ((parent = pci_upstream_bridge(parent)))
> -		if (parent->is_thunderbolt)
> +		if (dev_is_removable(&parent->dev))
>  			return true;

I don't get this.  Plain old PCI devices can be removable, too.

pci_is_thunderbolt_attached() is only used by GPU drivers.  What
property of Thunderbolt do they care about?

nouveau_vga_init() and radeon_device_init() use it to decide to
register with vga_switcheroo.  So maybe that's something to do with
removability?  Of course, that's not specific to Thunderbolt, because
garden-variety PCIe devices are removable.

amdgpu_driver_load_kms() and radeon_driver_load_kms() apparently use
it for something related to power control.  I don't know what the
Thunderbolt connection is.

nbio_v2_3_enable_aspm() looks like it uses it to change some ASPM
parameters.  Seems like potentially a device erratum or quirk
material?

If these things are not specifically related to Thunderbolt, I'd
prefer to get rid of pci_is_thunderbolt_attached() and see if we can
help the GPU folks figure out what they really need.

>  	return false;
> -- 
> 2.34.1
>