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