[PATCH v5 0/7] Overhaul `is_thunderbolt`

[PATCH v5 0/7] Overhaul `is_thunderbolt`

[Mario Limonciello]

Various drivers in the kernel use `is_thunderbolt` or
`pci_is_thunderbolt_attached` to designate behaving differently
from a device that is internally in the machine. This currently works
by an attribute in PCI core "is_thunderbolt" which makes those drivers
only apply differences when Intel Thunderbolt controllers are encountered.

In each of these drivers' cases the code should apply whether it's another
vendor's USB4 controller or an Intel USB4/TBT3 controller.

As such, overhaul the use of "is_thunderbolt" in the PCI core to instead rally
around the device core "external" attribute. This means dropping the extra PCI
core attribute and the extra function designation to indicate thunderbolt attached.

Changes from v4->v5:
- Drop USB4 related patches.  Thoes may come at a later time if they're proven to be needed.
  At least in the integrated case vendors should be setting the _DSD to indicate the port is
  externally facing.
  For the discrete case we may bring it back later.

Changes from v3->v4:
- Add tags from last review where applicable
- Update titles of different patches
- Add more comments and commit messages to various patches to address
  comments raised in review
- Re-order the patch series, moving more contentious patches later
- Drop patch marking NHI removable
- Drop patch changing gmux on it's own, roll into patch to drop
  `is_thunderbolt`
- Modify patch to mark integrated USB4 tunnel PCIe root ports as
  "external" instead of removable.
- Modify patch to mark discrete USB4 tunnel root ports as "external"
  instead of removable.
- Fix bit mask error in discrete USB4 tunnel patch
- Fix USB IF vendor designation location in pci_ids.h

Changes from v2->v3:
- Add various tags for patches that haven't changed from v2->v3
- Add new patches for Mika's suggestions:
  * Moving Apple Thunderbolt D3 declaration into quirks
  * Detect PCIe root port used for PCIe tunneling on integrated
    controllers using `usb4-host-interface`
  * Detect PCIe root port used for PCIe tunneling on discrete
    controllers using the USB4 DVSEC specification

Changes from v1->v2:
- Add Alex's tag to first patch
- Move lack of command completion into a quirk (Lukas)
- Drop `is_thunderbolt` attribute and `pci_is_thunderbolt_attached` and
  use device core removable attribute instead
- Adjust all consumers of old attribute to use removable

Note: this spans USB/DRM/platform-x86/PCI trees.
As a majority of the changes are in PCI, it should probably come through
that tree if possible.


Mario Limonciello (7):
  PCI: Move `is_thunderbolt` check for lack of command completed to a
    quirk
  PCI: Move check for old Apple Thunderbolt controllers into a quirk
  PCI: Drop the `is_thunderbolt` attribute from PCI core
  drm/amd: drop the use of `pci_is_thunderbolt_attached`
  drm/nouveau: drop the use of `pci_is_thunderbolt_attached`
  drm/radeon: drop the use of `pci_is_thunderbolt_attached`
  PCI: drop `pci_is_thunderbolt_attached`

 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
 drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
 drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
 drivers/pci/pci.c                       | 17 +++--
 drivers/pci/probe.c                     |  2 +-
 drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
 drivers/platform/x86/apple-gmux.c       |  2 +-
 include/linux/pci.h                     | 25 +-------
 11 files changed, 108 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH v5 1/7] PCI: Move `is_thunderbolt` check for lack of command completed to a quirk

[Mario Limonciello]

The `is_thunderbolt` check is currently used to indicate the lack of
command completed support for a number of older Thunderbolt devices.

This however is heavy handed and should have been done via a quirk.  Move
the affected devices outlined in commit 493fb50e958c ("PCI: pciehp: Assume
NoCompl+ for Thunderbolt ports") into pci quirks.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |  6 +-----
 drivers/pci/quirks.c             | 17 +++++++++++++++++
 include/linux/pci.h              |  2 ++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1c1ebf3dad43..e4c42b24aba8 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -996,11 +996,7 @@ struct controller *pcie_init(struct pcie_device *dev)
 	if (pdev->hotplug_user_indicators)
 		slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP);
 
-	/*
-	 * We assume no Thunderbolt controllers support Command Complete events,
-	 * but some controllers falsely claim they do.
-	 */
-	if (pdev->is_thunderbolt)
+	if (pdev->no_cmd_complete)
 		slot_cap |= PCI_EXP_SLTCAP_NCCS;
 
 	ctrl->slot_cap = slot_cap;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 65f7f6b0576c..ceeca7d8dd90 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3675,6 +3675,23 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
 			quirk_thunderbolt_hotplug_msi);
 
+static void quirk_thunderbolt_command_completed(struct pci_dev *pdev)
+{
+	pdev->no_cmd_complete = 1;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
+			quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
+			quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
+			quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
+			quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
+			quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
+			quirk_thunderbolt_command_completed);
+
 #ifdef CONFIG_ACPI
 /*
  * Apple: Shutdown Cactus Ridge Thunderbolt controller.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8253a5413d7c..1e5b769e42fc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -443,6 +443,8 @@ struct pci_dev {
 	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 */
+
 	/*
 	 * Devices marked being untrusted are the ones that can potentially
 	 * execute DMA attacks and similar. They are typically connected
-- 
2.34.1

[PATCH v5 2/7] PCI: Move check for old Apple Thunderbolt controllers into a quirk

[Mario Limonciello]

`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
controller to indicate that D3 is possible.

This is used solely for older Apple systems, due to a variety of factors:
* Apple used SW connection manager from the beginning, other manufacturers
  used a FW connection manager (ICM)
* Apple supported D3 initially, other manfuacturers didn't introduced this
  until the `HotplugSupportInD3` _DSD was introduced in ~2015.

Apple has stopped creating new machines with Intel Thunderbolt controllers,
and all other manufacturers now support D3 via `HotPlugSupportInD3` so
this should be a fixed list.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c    | 17 +++++++----
 drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..01557c950c9f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1064,7 +1064,18 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
 	if (pci_use_mid_pm())
 		return false;
 
-	return acpi_pci_bridge_d3(dev);
+	if (acpi_pci_bridge_d3(dev))
+		return true;
+
+	/*
+	 * This is for Apple machines via a quirk
+	 * Non-Apple machines will use the ACPI property with the same name
+	 * from `acpi_pci_bridge_d3` to indciate support.
+	 */
+	if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3"))
+		return true;
+
+	return false;
 }
 
 /**
@@ -2954,10 +2965,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (pci_bridge_d3_force)
 			return true;
 
-		/* Even the oldest 2010 Thunderbolt controller supports D3. */
-		if (bridge->is_thunderbolt)
-			return true;
-
 		/* Platform might know better if the bridge supports D3 */
 		if (platform_pci_bridge_d3(bridge))
 			return true;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ceeca7d8dd90..f74f50ea0695 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3756,6 +3756,73 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
 			       quirk_apple_poweroff_thunderbolt);
 #endif
 
+/*
+ * The first machines supporting Intel Thunderbolt were released by Apple, and
+ * supported a software based connection manager including D3 support, as far
+ * back as 2010. These machines don't have ACPI companions to declare D3
+ * support.
+ *
+ * Other manufacturers introduced Thunderbolt shortly after but notably did not
+ * support:
+ * - Software based connection manager
+ * - Runtime power management
+ * Power management was handled via the BIOS when nothing was plugged in.
+ * Runtime D3 was later introduced in ~2015 and Microsoft declared when the
+ * `HotPlugSupportInD3` _DSD was present that they would support D3.
+ *
+ * This list is expected to be complete and not grow in the future as Apple
+ * has stopped producing new x86 models with Intel Thunderbolt controllers.
+ */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
+{
+	struct property_entry properties[] = {
+		PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
+		{},
+	};
+
+	if (!x86_apple_machine)
+		return;
+
+	if (device_create_managed_software_node(&dev->dev, properties, NULL))
+		pci_warn(dev, "could not add HotPlugSupportInD3 property");
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+
 /*
  * Following are device-specific reset methods which can be used to
  * reset a single function if other methods (e.g. FLR, PM D0->D3) are
-- 
2.34.1

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

[Mario Limonciello]

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;
 }
 
 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;
 }
 
 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;
 
 	return false;
-- 
2.34.1

[PATCH v5 4/7] drm/amd: drop the use of `pci_is_thunderbolt_attached`

[Mario Limonciello]

Currently `pci_is_thunderbolt_attached` is used to indicate a device
is connected externally.

The PCI core now marks such devices as removable and downstream drivers
can use this instead.

Reviewed-by: Macpaul Lin <macpaul.lin@mediatek.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 1ebb91db2274..6dbf5753b5be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -161,7 +161,7 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
 	    (amdgpu_is_atpx_hybrid() ||
 	     amdgpu_has_atpx_dgpu_power_cntl()) &&
 	    ((flags & AMD_IS_APU) == 0) &&
-	    !pci_is_thunderbolt_attached(to_pci_dev(dev->dev)))
+	    !dev_is_removable(&adev->pdev->dev))
 		flags |= AMD_IS_PX;
 
 	parent = pci_upstream_bridge(adev->pdev);
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
index ee7cab37dfd5..2c5d74d836f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
@@ -382,7 +382,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
 
 		data |= NAVI10_PCIE__LC_L0S_INACTIVITY_DEFAULT << PCIE_LC_CNTL__LC_L0S_INACTIVITY__SHIFT;
 
-		if (pci_is_thunderbolt_attached(adev->pdev))
+		if (dev_is_removable(&adev->pdev->dev))
 			data |= NAVI10_PCIE__LC_L1_INACTIVITY_TBT_DEFAULT  << PCIE_LC_CNTL__LC_L1_INACTIVITY__SHIFT;
 		else
 			data |= NAVI10_PCIE__LC_L1_INACTIVITY_DEFAULT << PCIE_LC_CNTL__LC_L1_INACTIVITY__SHIFT;
-- 
2.34.1

[PATCH v5 5/7] drm/nouveau: drop the use of `pci_is_thunderbolt_attached`

[Mario Limonciello]

Currently `pci_is_thunderbolt_attached` is used to indicate a device
is connected externally.

The PCI core now marks such devices as removable and downstream drivers
can use this instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/nouveau/nouveau_vga.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index 60cd8c0463df..2c8008cb38e0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -97,7 +97,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
 	vga_client_register(pdev, nouveau_vga_set_decode);
 
 	/* don't register Thunderbolt eGPU with vga_switcheroo */
-	if (pci_is_thunderbolt_attached(pdev))
+	if (dev_is_removable(&pdev->dev))
 		return;
 
 	vga_switcheroo_register_client(pdev, &nouveau_switcheroo_ops, runtime);
@@ -120,7 +120,7 @@ nouveau_vga_fini(struct nouveau_drm *drm)
 
 	vga_client_unregister(pdev);
 
-	if (pci_is_thunderbolt_attached(pdev))
+	if (dev_is_removable(&pdev->dev))
 		return;
 
 	vga_switcheroo_unregister_client(pdev);
-- 
2.34.1

[PATCH v5 6/7] drm/radeon: drop the use of `pci_is_thunderbolt_attached`

[Mario Limonciello]

Currently `pci_is_thunderbolt_attached` is used to indicate a device
is connected externally.

The PCI core now marks such devices as removable and downstream drivers
can use this instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/radeon/radeon_device.c | 4 ++--
 drivers/gpu/drm/radeon/radeon_kms.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 4f0fbf667431..5117fce23b3f 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1439,7 +1439,7 @@ int radeon_device_init(struct radeon_device *rdev,
 
 	if (rdev->flags & RADEON_IS_PX)
 		runtime = true;
-	if (!pci_is_thunderbolt_attached(rdev->pdev))
+	if (!dev_is_removable(&rdev->pdev->dev))
 		vga_switcheroo_register_client(rdev->pdev,
 					       &radeon_switcheroo_ops, runtime);
 	if (runtime)
@@ -1527,7 +1527,7 @@ void radeon_device_fini(struct radeon_device *rdev)
 	/* evict vram memory */
 	radeon_bo_evict_vram(rdev);
 	radeon_fini(rdev);
-	if (!pci_is_thunderbolt_attached(rdev->pdev))
+	if (!dev_is_removable(&rdev->pdev->dev))
 		vga_switcheroo_unregister_client(rdev->pdev);
 	if (rdev->flags & RADEON_IS_PX)
 		vga_switcheroo_fini_domain_pm_ops(rdev->dev);
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 11ad210919c8..e01ee7a5cf5d 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -139,7 +139,7 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags)
 	if ((radeon_runtime_pm != 0) &&
 	    radeon_has_atpx() &&
 	    ((flags & RADEON_IS_IGP) == 0) &&
-	    !pci_is_thunderbolt_attached(pdev))
+	    !dev_is_removable(&pdev->dev))
 		flags |= RADEON_IS_PX;
 
 	/* radeon_device_init should report only fatal error
-- 
2.34.1

[PATCH v5 7/7] PCI: drop `pci_is_thunderbolt_attached`

[Mario Limonciello]

Currently `pci_is_thunderbolt_attached` is used to indicate a device
is connected externally.

As all drivers now look at the removable attribute, drop this function.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 include/linux/pci.h | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index d9719eb14654..089e7e36a0d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2434,28 +2434,6 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
 	return bus->self && bus->self->ari_enabled;
 }
 
-/**
- * pci_is_thunderbolt_attached - whether device is on a Thunderbolt daisy chain
- * @pdev: PCI device to check
- *
- * Walk upwards from @pdev and check for each encountered bridge if it's part
- * of a Thunderbolt controller.  Reaching the host bridge means @pdev is not
- * Thunderbolt-attached.  (But rather soldered to the mainboard usually.)
- */
-static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
-{
-	struct pci_dev *parent = pdev;
-
-	if (dev_is_removable(&pdev->dev))
-		return true;
-
-	while ((parent = pci_upstream_bridge(parent)))
-		if (dev_is_removable(&parent->dev))
-			return true;
-
-	return false;
-}
-
 #if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
 void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 #endif
-- 
2.34.1

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

[Bjorn Helgaas]

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
> 

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

[Bjorn Helgaas]

On Thu, Feb 24, 2022 at 07:23:49PM -0600, Bjorn Helgaas wrote:
> 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.
> ...

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

Ah.  Guess I should read the whole series before commenting :)  I see
that you *did* remove pci_is_thunderbolt_attached() in the last patch.
I'll look more at the rest tomorrow.

Bjorn

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

[Alex Deucher]

On Thu, Feb 24, 2022 at 8:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> 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.

For GPU drivers, we need to determine which dGPU on the system has
d3cold control via ACPI and which GPU would use a mux for display
switching between the iGPU and the dGPU for hybrid graphics platforms
(e.g., iGPU + dGPU built into a laptop or all-in-one PC).  The dGPU
built into the platform would be the one we want to use for mux
switching and ACPI power control.  You would not want that for the
dGPU connected via thunderbolt (or some other hot pluggable
interface).  I had suggested that we could check if there is an ACPI
device associated with the dGPU and use that to determine this, but I
think someone brought up a case where that didn't work.  We need to
know whether the dGPU uses platform power control to determine whether
the driver should let the platform manage the power state via ACPI or
if the driver should do it (e.g., for dGPU PCIe add-in cards) for
runtime power management.

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

I think this one is a quirk specifically for thunderbolt.  Thunderbolt
attached dGPUs needs a different ASPM L1 inactivity threshold for
stability.  I can check with the relevant teams for more background on
this.

Alex

>
> 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
> >

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

[Bjorn Helgaas]

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;
>  }
>  
>  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;
>  }
>  
>  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;
>  
>  	return false;

Since you remove this function entirely later, it seems like you might
as well push this to the end of the series, so you won't have to
change it before removing it.

That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-facing"
assumption above.  Not having a Thunderbolt spec, I have no idea how
you deal with that.

But it is definitely not the case that "dev_is_removable() implies
device is Thunderbolt", so I don't think this last hunk can work.

Bjorn

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

[Mika Westerberg]

Hi Bjorn,

On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote:
> That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-facing"
> assumption above.  Not having a Thunderbolt spec, I have no idea how
> you deal with that.

You can download the spec here:

https://www.usb.org/sites/default/files/USB4%20Specification%2020211116.zip

Inside the archive there is also the DVSEC spec with name "USB4 DVSEC
Version 1.0.pdf".

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

[Limonciello, Mario]

[AMD Official Use Only]

> 
> On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote:
> > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-
> facing"
> > assumption above.  Not having a Thunderbolt spec, I have no idea how
> > you deal with that.
> 
> You can download the spec here:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w.usb.org%2Fsites%2Fdefault%2Ffiles%2FUSB4%2520Specification%2520202
> 11116.zip&data=04%7C01%7Cmario.limonciello%40amd.com%7Ca26e64
> 7a4acf4e7681d308d9faa358fd%7C3dd8961fe4884e608e11a82d994e183d%7C0
> %7C0%7C637816402472428689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&
> amp;sdata=HSDqx%2BHzSnczTZxaBij8sgqvJSS8ajtjCzZd2CPSbR4%3D&re
> served=0
> 
> Inside the archive there is also the DVSEC spec with name "USB4 DVSEC
> Version 1.0.pdf".

The spec has Host_Router_indication (bits 18-19) as meaning external facing.
I'll respin the patch 3 for using that.

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

[Bjorn Helgaas]

On Mon, Feb 28, 2022 at 03:33:13PM +0000, Limonciello, Mario wrote:
> [AMD Official Use Only]
> 
> > 
> > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote:
> > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-
> > facing"
> > > assumption above.  Not having a Thunderbolt spec, I have no idea how
> > > you deal with that.
> > 
> > You can download the spec here:
> > 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > w.usb.org%2Fsites%2Fdefault%2Ffiles%2FUSB4%2520Specification%2520202
> > 11116.zip&data=04%7C01%7Cmario.limonciello%40amd.com%7Ca26e64
> > 7a4acf4e7681d308d9faa358fd%7C3dd8961fe4884e608e11a82d994e183d%7C0
> > %7C0%7C637816402472428689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&
> > amp;sdata=HSDqx%2BHzSnczTZxaBij8sgqvJSS8ajtjCzZd2CPSbR4%3D&re
> > served=0
> > 
> > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC
> > Version 1.0.pdf".
> 
> The spec has Host_Router_indication (bits 18-19) as meaning external facing.
> I'll respin the patch 3 for using that.

Thanks, please include the spec citation when you do.  And probably
the URL, because it's not at all obvious how the casual reader would
get from "is_thunderbolt" to a recent add-on to the USB4 spec.

Bjorn

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

[Lukas Wunner]

On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 28, 2022 at 03:33:13PM +0000, Limonciello, Mario wrote:
> > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote:
> > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-
> > > facing"
> > > > assumption above.  Not having a Thunderbolt spec, I have no idea how
> > > > you deal with that.
> > > 
> > > You can download the spec here:
[...]
> > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC
> > > Version 1.0.pdf".
> > 
> > The spec has Host_Router_indication (bits 18-19) as meaning external facing.
> > I'll respin the patch 3 for using that.
> 
> Thanks, please include the spec citation when you do.  And probably
> the URL, because it's not at all obvious how the casual reader would
> get from "is_thunderbolt" to a recent add-on to the USB4 spec.

PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec,
hence there's no connection between "is_thunderbolt" and the USB4 spec.

It's a proprietary VSEC used by Intel and the only way to recognize
pre-USB4 Thunderbolt devices that I know of.  Its ID is also
different from the DVSEC IDs given in the above-mentioned spec.

Thanks,

Lukas

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

[Limonciello, Mario]

[AMD Official Use Only]

> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Monday, February 28, 2022 16:33
> To: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Michael Jamet
> <michael.jamet@intel.com>; open list:PCI SUBSYSTEM <linux-
> pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> usb@vger.kernel.org>; Yehezkel Bernat <YehezkelShB@gmail.com>; open
> list:DRM DRIVERS <dri-devel@lists.freedesktop.org>; open list:X86
> PLATFORM DRIVERS <platform-driver-x86@vger.kernel.org>; Andreas
> Noever <andreas.noever@gmail.com>; open list:RADEON and AMDGPU
> DRM DRIVERS <amd-gfx@lists.freedesktop.org>; open list:DRM DRIVER FOR
> NVIDIA GEFORCE/QUADRO GPUS <nouveau@lists.freedesktop.org>; Bjorn
> Helgaas <bhelgaas@google.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI
> core
> 
> On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote:
> > On Mon, Feb 28, 2022 at 03:33:13PM +0000, Limonciello, Mario wrote:
> > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote:
> > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-
> > > > facing"
> > > > > assumption above.  Not having a Thunderbolt spec, I have no idea
> how
> > > > > you deal with that.
> > > >
> > > > You can download the spec here:
> [...]
> > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC
> > > > Version 1.0.pdf".
> > >
> > > The spec has Host_Router_indication (bits 18-19) as meaning external
> facing.
> > > I'll respin the patch 3 for using that.
> >
> > Thanks, please include the spec citation when you do.  And probably
> > the URL, because it's not at all obvious how the casual reader would
> > get from "is_thunderbolt" to a recent add-on to the USB4 spec.
> 
> PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec,
> hence there's no connection between "is_thunderbolt" and the USB4 spec.
> 
> It's a proprietary VSEC used by Intel and the only way to recognize
> pre-USB4 Thunderbolt devices that I know of.  Its ID is also
> different from the DVSEC IDs given in the above-mentioned spec.
> 
> Thanks,

The USB4 DVSEC spec makes comments about DVSEC_ID of 0x8086 and also
DVSEC VENDOR_ID of 0x8086.  Is that not also present on the Intel TBT3 controllers?

My interpretation of this (and Mika's comment) was that rather than looking at the Intel VSEC
we should look at the USB4 DVSEC to detect the Intel TBT3 controllers.

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

[Mika Westerberg]

Hi,

On Mon, Feb 28, 2022 at 10:36:59PM +0000, Limonciello, Mario wrote:
> [AMD Official Use Only]
> 
> > -----Original Message-----
> > From: Lukas Wunner <lukas@wunner.de>
> > Sent: Monday, February 28, 2022 16:33
> > To: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Mika Westerberg
> > <mika.westerberg@linux.intel.com>; Michael Jamet
> > <michael.jamet@intel.com>; open list:PCI SUBSYSTEM <linux-
> > pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> > usb@vger.kernel.org>; Yehezkel Bernat <YehezkelShB@gmail.com>; open
> > list:DRM DRIVERS <dri-devel@lists.freedesktop.org>; open list:X86
> > PLATFORM DRIVERS <platform-driver-x86@vger.kernel.org>; Andreas
> > Noever <andreas.noever@gmail.com>; open list:RADEON and AMDGPU
> > DRM DRIVERS <amd-gfx@lists.freedesktop.org>; open list:DRM DRIVER FOR
> > NVIDIA GEFORCE/QUADRO GPUS <nouveau@lists.freedesktop.org>; Bjorn
> > Helgaas <bhelgaas@google.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>
> > Subject: Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI
> > core
> > 
> > On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Feb 28, 2022 at 03:33:13PM +0000, Limonciello, Mario wrote:
> > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote:
> > > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-
> > > > > facing"
> > > > > > assumption above.  Not having a Thunderbolt spec, I have no idea
> > how
> > > > > > you deal with that.
> > > > >
> > > > > You can download the spec here:
> > [...]
> > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC
> > > > > Version 1.0.pdf".
> > > >
> > > > The spec has Host_Router_indication (bits 18-19) as meaning external
> > facing.
> > > > I'll respin the patch 3 for using that.
> > >
> > > Thanks, please include the spec citation when you do.  And probably
> > > the URL, because it's not at all obvious how the casual reader would
> > > get from "is_thunderbolt" to a recent add-on to the USB4 spec.
> > 
> > PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec,
> > hence there's no connection between "is_thunderbolt" and the USB4 spec.
> > 
> > It's a proprietary VSEC used by Intel and the only way to recognize
> > pre-USB4 Thunderbolt devices that I know of.  Its ID is also
> > different from the DVSEC IDs given in the above-mentioned spec.
> > 
> > Thanks,
> 
> The USB4 DVSEC spec makes comments about DVSEC_ID of 0x8086 and also
> DVSEC VENDOR_ID of 0x8086.  Is that not also present on the Intel TBT3 controllers?
> 
> My interpretation of this (and Mika's comment) was that rather than
> looking at the Intel VSEC we should look at the USB4 DVSEC to detect
> the Intel TBT3 controllers.

For pre-USB4 controllers (TBT 1-3) we need to use the existing method
(or a quirk based on device ID) as they don't have the USB4 DVSEC.