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.limoncie...@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
>
