[PATCH v4 00/10] Overhaul `is_thunderbolt`

[PATCH v4 00/10] 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 relies upon checks
for a specific capability only set on Intel controllers.

Non-Intel USB4 designs should also match this designation so that they
can be treated the same regardless of the host they're connected to.

As part of adding the generic USB4 controller code, it was realized that
`is_thunderbolt` and `pcie_is_thunderbolt_attached` have been overloaded.

Instead migrate to using removable attribute from device core.

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 (10):
  PCI: Add USB4 class definition
  PCI: Move `is_thunderbolt` check for lack of command completed to a
    quirk
  PCI: Detect root port of internal USB4 controllers
  PCI: Detect PCIe root ports for discrete USB4 controllers
  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-acpi.c                  | 15 ++++-
 drivers/pci/pci.c                       | 17 +++--
 drivers/pci/probe.c                     | 52 ++++++++++++++-
 drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
 drivers/platform/x86/apple-gmux.c       |  2 +-
 drivers/thunderbolt/nhi.h               |  2 -
 include/linux/pci.h                     | 25 +-------
 include/linux/pci_ids.h                 |  3 +
 14 files changed, 173 insertions(+), 47 deletions(-)

-- 
2.34.1

[PATCH v4 01/10] PCI: Add USB4 class definition

[Mario Limonciello]

This PCI class definition of the USB4 device is currently located only in
the thunderbolt driver.

It will be needed by a few other drivers for upcoming changes. Move it into
the common include file.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/nhi.h | 2 --
 include/linux/pci_ids.h   | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index 69083aab2736..79e980b51f94 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -81,6 +81,4 @@ extern const struct tb_nhi_ops icl_nhi_ops;
 #define PCI_DEVICE_ID_INTEL_TGL_H_NHI0			0x9a1f
 #define PCI_DEVICE_ID_INTEL_TGL_H_NHI1			0x9a21
 
-#define PCI_CLASS_SERIAL_USB_USB4			0x0c0340
-
 #endif
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index aad54c666407..61b161d914f0 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -116,6 +116,7 @@
 #define PCI_CLASS_SERIAL_USB_OHCI	0x0c0310
 #define PCI_CLASS_SERIAL_USB_EHCI	0x0c0320
 #define PCI_CLASS_SERIAL_USB_XHCI	0x0c0330
+#define PCI_CLASS_SERIAL_USB_USB4	0x0c0340
 #define PCI_CLASS_SERIAL_USB_DEVICE	0x0c03fe
 #define PCI_CLASS_SERIAL_FIBER		0x0c04
 #define PCI_CLASS_SERIAL_SMBUS		0x0c05
-- 
2.34.1

[PATCH v4 02/10] 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 d2dd6a6cda60..6d3c88edde00 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 v4 03/10] PCI: Detect root port of internal USB4 controllers

[Mario Limonciello]

The root port used for PCIe tunneling is the root of the hierarchy used
for all PCIe devices that are connected downstream.  Tunnels are created
and destroyed by the USB4 SW CM.  So this port should be marked as external
meaning all devices connected to it are appropriately marked as removable
for downstream drivers to detect.

This can be done by looking for the device property specified in
the ACPI tables `usb4-host-interface`.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Link: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci-acpi.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a42dbf448860..695dbd88b8b7 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1335,12 +1335,21 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 
 static void pci_acpi_set_external_facing(struct pci_dev *dev)
 {
-	u8 val;
+	u8 val = 0;
 
 	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
 		return;
-	if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val))
-		return;
+	device_property_read_u8(&dev->dev, "ExternalFacingPort", &val);
+
+	/* check for root port for PCIe tunnels on integrated controllers */
+	if (!val) {
+		struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
+
+		if (!adev)
+			return;
+		val = fwnode_property_present(acpi_fwnode_handle(adev),
+					      "usb4-host-interface");
+	}
 
 	/*
 	 * These root ports expose PCIe (including DMA) outside of the
-- 
2.34.1

[PATCH v4 04/10] PCI: Detect PCIe root ports for discrete USB4 controllers

[Mario Limonciello]

Discrete USB4 controllers won't have ACPI nodes specifying which
root ports they are linked with when the software connection manager
creates tunnels.  These PCIe root ports should be marked as external
so that existing logic will mark tunneled devices as removable.

In order to set the external attribute, use the USB4 DVSEC extended
capabability set on these root ports to determine if they are located
on a discrete USB4 controller.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Link: https://usb.org/sites/default/files/USB4%20Specification%2020211116.zip
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/probe.c     | 50 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h |  2 ++
 2 files changed, 52 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..a07859c8675f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -25,6 +25,8 @@
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
 #define CARDBUS_RESERVE_BUSNR	3
 
+#define PCI_DVSEC_ID_USB4	0x23
+
 static struct resource busn_resource = {
 	.name	= "PCI busn",
 	.start	= 0,
@@ -1600,6 +1602,52 @@ static void set_pcie_untrusted(struct pci_dev *dev)
 		dev->untrusted = true;
 }
 
+/*
+ * Use the fields from the USB4 Designated Vendor Specific Extended Capability
+ * (DVSEC) for Power Management 1.0 to identify PCIe root ports that are for
+ * XHCI and PCIe tunneling
+ */
+static void pci_set_usb4_external(struct pci_dev *dev)
+{
+	int dvsec_val = 0, pos;
+	u32 hdr;
+
+	/*
+	 * Table 3-1 "USB4 DVSEC Header fields" says vendors can use
+	 * either the Intel or USB IF vendor ID but should look for
+	 * the appropriate DVSEC ID.
+	 */
+	pos = pci_find_dvsec_capability(dev,
+					PCI_VENDOR_ID_INTEL,
+					PCI_DVSEC_ID_USB4);
+	if (pos) {
+		dvsec_val = 0x06;
+	} else {
+		pos = pci_find_dvsec_capability(dev,
+						PCI_VENDOR_ID_USB_IF,
+						PCI_DVSEC_ID_USB4);
+		if (pos)
+			dvsec_val = 0x01;
+	}
+	if (!dvsec_val)
+		return;
+
+	pci_read_config_dword(dev, pos + PCI_DVSEC_HEADER2, &hdr);
+	if ((hdr & GENMASK(15, 0)) != dvsec_val)
+		return;
+	/*
+	 * Look at the port type field for the expected bits for PCIe tunneling
+	 * and XHCI tunneling
+	 *
+	 * 0x0 - Native Host Interface
+	 * 0x1 - PCIe Tunneled Port
+	 * 0x2 - USB Tunneled Port
+	 * 0x3-0x7 - Reserved
+	 */
+	if (hdr & GENMASK(17, 16))
+		dev->external_facing = true;
+}
+
 static void pci_set_removable(struct pci_dev *dev)
 {
 	struct pci_dev *parent = pci_upstream_bridge(dev);
@@ -1870,6 +1918,8 @@ int pci_setup_device(struct pci_dev *dev)
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
 
+	pci_set_usb4_external(dev);
+
 	pci_set_removable(dev);
 
 	pci_info(dev, "[%04x:%04x] type %02x class %#08x\n",
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 61b161d914f0..3faee1af9ace 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2567,6 +2567,8 @@
 #define PCI_VENDOR_ID_TEKRAM		0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
 
+#define PCI_VENDOR_ID_USB_IF		0x1ec0
+
 #define PCI_VENDOR_ID_TEHUTI		0x1fc9
 #define PCI_DEVICE_ID_TEHUTI_3009	0x3009
 #define PCI_DEVICE_ID_TEHUTI_3010	0x3010
-- 
2.34.1

[PATCH v4 05/10] 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 6d3c88edde00..97793cfcd8ff 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 v4 06/10] 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 a07859c8675f..fe49175770a1 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1586,7 +1586,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 v4 07/10] 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 v4 08/10] 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 v4 09/10] 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 v4 10/10] 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 v4 00/10] Overhaul `is_thunderbolt`

[Lukas Wunner]

On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
>  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-acpi.c                  | 15 ++++-
>  drivers/pci/pci.c                       | 17 +++--
>  drivers/pci/probe.c                     | 52 ++++++++++++++-
>  drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
>  drivers/platform/x86/apple-gmux.c       |  2 +-
>  drivers/thunderbolt/nhi.h               |  2 -
>  include/linux/pci.h                     | 25 +-------
>  include/linux/pci_ids.h                 |  3 +
>  14 files changed, 173 insertions(+), 47 deletions(-)

That's an awful lot of additional LoC for what is primarily
a refactoring job with the intent to simplify things.

Honestly this looks like an attempt to fix something that
isn't broken.  Specifically, the is_thunderbolt bit apparently
can't be removed without adding new bits to struct pci_dev.
Not sure if that can be called progress.

Thanks,

Lukas

Re: [PATCH v4 00/10] Overhaul `is_thunderbolt`

[Limonciello, Mario]

On 2/15/2022 01:29, Lukas Wunner wrote:
> On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
>>   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-acpi.c                  | 15 ++++-
>>   drivers/pci/pci.c                       | 17 +++--
>>   drivers/pci/probe.c                     | 52 ++++++++++++++-
>>   drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
>>   drivers/platform/x86/apple-gmux.c       |  2 +-
>>   drivers/thunderbolt/nhi.h               |  2 -
>>   include/linux/pci.h                     | 25 +-------
>>   include/linux/pci_ids.h                 |  3 +
>>   14 files changed, 173 insertions(+), 47 deletions(-)
> 
> That's an awful lot of additional LoC for what is primarily
> a refactoring job with the intent to simplify things.

You may recall the first version of this series was just for adding
USB4 matches to the existing code paths, and that's when it was noted
that is_thunderbolt is a bit overloaded.

> 
> Honestly this looks like an attempt to fix something that
> isn't broken.  Specifically, the is_thunderbolt bit apparently
> can't be removed without adding new bits to struct pci_dev.
> Not sure if that can be called progress. >
> Thanks,
> 
> Lukas

Within this series there are two new material patches; setting up root 
ports for both integrated and discrete USB4 controllers to behave well 
with all the existing drivers that rely upon a hint of how they're 
connected to configure devices differently.

If y'all collectively prefer this direction to not refactor 
is_thunderbolt and push into quirks, a simpler version of this series 
would be to leave all the quirks in place, just drop 
dev->is_thunderbolt, and set dev->external_facing on all 3 cases:

* Intel TBT controller
* USB4 integrated PCIe tunneling root port/XHCI tunneling root port
* USB4 disctete PCIe tunneling root port/XHCI tunneling root port

All the other drivers and symbols can stay the same then.

Re: [PATCH v4 00/10] Overhaul `is_thunderbolt`

[Mika Westerberg]

Hi all,

On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
> On 2/15/2022 01:29, Lukas Wunner wrote:
> > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
> > >   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-acpi.c                  | 15 ++++-
> > >   drivers/pci/pci.c                       | 17 +++--
> > >   drivers/pci/probe.c                     | 52 ++++++++++++++-
> > >   drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
> > >   drivers/platform/x86/apple-gmux.c       |  2 +-
> > >   drivers/thunderbolt/nhi.h               |  2 -
> > >   include/linux/pci.h                     | 25 +-------
> > >   include/linux/pci_ids.h                 |  3 +
> > >   14 files changed, 173 insertions(+), 47 deletions(-)
> > 
> > That's an awful lot of additional LoC for what is primarily
> > a refactoring job with the intent to simplify things.
> 
> You may recall the first version of this series was just for adding
> USB4 matches to the existing code paths, and that's when it was noted
> that is_thunderbolt is a bit overloaded.
> 
> > 
> > Honestly this looks like an attempt to fix something that
> > isn't broken.  Specifically, the is_thunderbolt bit apparently
> > can't be removed without adding new bits to struct pci_dev.
> > Not sure if that can be called progress. >
> > Thanks,
> > 
> > Lukas
> 
> Within this series there are two new material patches; setting up root ports
> for both integrated and discrete USB4 controllers to behave well with all
> the existing drivers that rely upon a hint of how they're connected to
> configure devices differently.
> 
> If y'all collectively prefer this direction to not refactor is_thunderbolt
> and push into quirks, a simpler version of this series would be to leave all
> the quirks in place, just drop dev->is_thunderbolt, and set
> dev->external_facing on all 3 cases:
> 
> * Intel TBT controller
> * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
> * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
> 
> All the other drivers and symbols can stay the same then.

If I understand correctly the original intention of this patch series is
to be able to differentiate whether the device is "permanently"
connected to the motherboard, or it is connected over some hot-pluggable
bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
fit into this picture too). Specifically this is needed for discrete
GPUs because of power management differences or so (please correct me if
I'm mistaken).

If we set the is_thunderbolt debate aside and concentrate on that issue,
I think the way to do this is to check whether the root port the GPU is
connected to has an ACPI power resource (returned from _PR3() method).
IF it is present then most likely the platform has provided all the
necessary wiring to move the GPU into D3cold (and the BIOS knows this).
If it is not present then the device cannot even go into D3cold as there
is not means to power of the device in PCIe spec.

Perhaps we can simply use pci_pr3_present() here as nouveau is already
doing? Granted it is not too elegant solution either but better than
using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
perhaps we can have a helper in DRM core that handles this.

Then going back to is_thunderbolt debate :) I really don't think the
drivers should care whether they are connected over a tunnel or not.
They should work regardless of the underlying transport of the native
protocol. They should also be prepared for the fact that the hardware
can vanish under them at any point (e.g user unplugs the device). For
this reason I don't really like to see is_thunderbolt to be used more
and prefer to get rid if it completely if possible at all. If there is
still need to differentiate whether the device can be hot-removed or
not, I think "removable" in the driver core is the way to go. That is
not dependent on any single transport.

Re: [PATCH v4 00/10] Overhaul `is_thunderbolt`

[Alex Deucher]

On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi all,
>
> On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
> > On 2/15/2022 01:29, Lukas Wunner wrote:
> > > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
> > > >   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-acpi.c                  | 15 ++++-
> > > >   drivers/pci/pci.c                       | 17 +++--
> > > >   drivers/pci/probe.c                     | 52 ++++++++++++++-
> > > >   drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
> > > >   drivers/platform/x86/apple-gmux.c       |  2 +-
> > > >   drivers/thunderbolt/nhi.h               |  2 -
> > > >   include/linux/pci.h                     | 25 +-------
> > > >   include/linux/pci_ids.h                 |  3 +
> > > >   14 files changed, 173 insertions(+), 47 deletions(-)
> > >
> > > That's an awful lot of additional LoC for what is primarily
> > > a refactoring job with the intent to simplify things.
> >
> > You may recall the first version of this series was just for adding
> > USB4 matches to the existing code paths, and that's when it was noted
> > that is_thunderbolt is a bit overloaded.
> >
> > >
> > > Honestly this looks like an attempt to fix something that
> > > isn't broken.  Specifically, the is_thunderbolt bit apparently
> > > can't be removed without adding new bits to struct pci_dev.
> > > Not sure if that can be called progress. >
> > > Thanks,
> > >
> > > Lukas
> >
> > Within this series there are two new material patches; setting up root ports
> > for both integrated and discrete USB4 controllers to behave well with all
> > the existing drivers that rely upon a hint of how they're connected to
> > configure devices differently.
> >
> > If y'all collectively prefer this direction to not refactor is_thunderbolt
> > and push into quirks, a simpler version of this series would be to leave all
> > the quirks in place, just drop dev->is_thunderbolt, and set
> > dev->external_facing on all 3 cases:
> >
> > * Intel TBT controller
> > * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
> > * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
> >
> > All the other drivers and symbols can stay the same then.
>
> If I understand correctly the original intention of this patch series is
> to be able to differentiate whether the device is "permanently"
> connected to the motherboard, or it is connected over some hot-pluggable
> bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
> fit into this picture too). Specifically this is needed for discrete
> GPUs because of power management differences or so (please correct me if
> I'm mistaken).
>
> If we set the is_thunderbolt debate aside and concentrate on that issue,
> I think the way to do this is to check whether the root port the GPU is
> connected to has an ACPI power resource (returned from _PR3() method).
> IF it is present then most likely the platform has provided all the
> necessary wiring to move the GPU into D3cold (and the BIOS knows this).
> If it is not present then the device cannot even go into D3cold as there
> is not means to power of the device in PCIe spec.
>
> Perhaps we can simply use pci_pr3_present() here as nouveau is already
> doing? Granted it is not too elegant solution either but better than
> using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
> perhaps we can have a helper in DRM core that handles this.

The tricky part is that there were AMD and NVIDIA specific proprietary
_PR3-like ACPI methods (plus whatever Apple did) prior to GPU power
control standardizing on _PR3.  Currently those methods are handled in
the drivers directly, sort of tangled up with vga_switcheroo.  I think
ideally that logic would move to the ACPI core and be handled the same
way as _PR3, but I'm not sure how well that would work because of the
various bios date checks around _PR3 and the lack of general _PR3
support in those older platforms.  So I think we still need some sort
of "is this soldered in" check.

Alex


>
> Then going back to is_thunderbolt debate :) I really don't think the
> drivers should care whether they are connected over a tunnel or not.
> They should work regardless of the underlying transport of the native
> protocol. They should also be prepared for the fact that the hardware
> can vanish under them at any point (e.g user unplugs the device). For
> this reason I don't really like to see is_thunderbolt to be used more
> and prefer to get rid if it completely if possible at all. If there is
> still need to differentiate whether the device can be hot-removed or
> not, I think "removable" in the driver core is the way to go. That is
> not dependent on any single transport.

Re: [PATCH v4 00/10] Overhaul `is_thunderbolt`

[Limonciello, Mario]

On 2/16/2022 08:44, Alex Deucher wrote:
> On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>>
>> Hi all,
>>
>> On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
>>> On 2/15/2022 01:29, Lukas Wunner wrote:
>>>> On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
>>>>>    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-acpi.c                  | 15 ++++-
>>>>>    drivers/pci/pci.c                       | 17 +++--
>>>>>    drivers/pci/probe.c                     | 52 ++++++++++++++-
>>>>>    drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
>>>>>    drivers/platform/x86/apple-gmux.c       |  2 +-
>>>>>    drivers/thunderbolt/nhi.h               |  2 -
>>>>>    include/linux/pci.h                     | 25 +-------
>>>>>    include/linux/pci_ids.h                 |  3 +
>>>>>    14 files changed, 173 insertions(+), 47 deletions(-)
>>>>
>>>> That's an awful lot of additional LoC for what is primarily
>>>> a refactoring job with the intent to simplify things.
>>>
>>> You may recall the first version of this series was just for adding
>>> USB4 matches to the existing code paths, and that's when it was noted
>>> that is_thunderbolt is a bit overloaded.
>>>
>>>>
>>>> Honestly this looks like an attempt to fix something that
>>>> isn't broken.  Specifically, the is_thunderbolt bit apparently
>>>> can't be removed without adding new bits to struct pci_dev.
>>>> Not sure if that can be called progress. >
>>>> Thanks,
>>>>
>>>> Lukas
>>>
>>> Within this series there are two new material patches; setting up root ports
>>> for both integrated and discrete USB4 controllers to behave well with all
>>> the existing drivers that rely upon a hint of how they're connected to
>>> configure devices differently.
>>>
>>> If y'all collectively prefer this direction to not refactor is_thunderbolt
>>> and push into quirks, a simpler version of this series would be to leave all
>>> the quirks in place, just drop dev->is_thunderbolt, and set
>>> dev->external_facing on all 3 cases:
>>>
>>> * Intel TBT controller
>>> * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
>>> * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
>>>
>>> All the other drivers and symbols can stay the same then.
>>
>> If I understand correctly the original intention of this patch series is
>> to be able to differentiate whether the device is "permanently"
>> connected to the motherboard, or it is connected over some hot-pluggable
>> bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
>> fit into this picture too). Specifically this is needed for discrete
>> GPUs because of power management differences or so (please correct me if
>> I'm mistaken).

Correct.  It might be possible to drop the patch for the integrated case 
(patch 3) because I do think that by Microsoft having the _DSD for 
"ExternalFacingPort" it's very likely that most implementations will 
have used it for the appropriate PCIe root ports.  If something shows up 
in the wild that this isn't the case it could be revisited.  If it's 
found pre-production presumably the OEM can still fix it and if it's 
post production and there are problems we can dust it off then.

The discrete USB4 controller I would be more concerned that this isn't 
populated, and that (patch 4) should be more important to let the driver 
core set it removable.

>>
>> If we set the is_thunderbolt debate aside and concentrate on that issue,
>> I think the way to do this is to check whether the root port the GPU is
>> connected to has an ACPI power resource (returned from _PR3() method).
>> IF it is present then most likely the platform has provided all the
>> necessary wiring to move the GPU into D3cold (and the BIOS knows this).
>> If it is not present then the device cannot even go into D3cold as there
>> is not means to power of the device in PCIe spec.
>>
>> Perhaps we can simply use pci_pr3_present() here as nouveau is already
>> doing? Granted it is not too elegant solution either but better than
>> using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
>> perhaps we can have a helper in DRM core that handles this.
> 
> The tricky part is that there were AMD and NVIDIA specific proprietary
> _PR3-like ACPI methods (plus whatever Apple did) prior to GPU power
> control standardizing on _PR3.  Currently those methods are handled in
> the drivers directly, sort of tangled up with vga_switcheroo.  I think
> ideally that logic would move to the ACPI core and be handled the same
> way as _PR3, but I'm not sure how well that would work because of the
> various bios date checks around _PR3 and the lack of general _PR3
> support in those older platforms.  So I think we still need some sort
> of "is this soldered in" check.

Considering that limitation if `dev->external_facing` already exists in 
PCI core may as well use it for this instead of `is_thunderbolt`.

> 
> Alex
> 
> 
>>
>> Then going back to is_thunderbolt debate :) I really don't think the
>> drivers should care whether they are connected over a tunnel or not.
>> They should work regardless of the underlying transport of the native
>> protocol. They should also be prepared for the fact that the hardware
>> can vanish under them at any point (e.g user unplugs the device). For
>> this reason I don't really like to see is_thunderbolt to be used more
>> and prefer to get rid if it completely if possible at all. If there is
>> still need to differentiate whether the device can be hot-removed or
>> not, I think "removable" in the driver core is the way to go. That is
>> not dependent on any single transport.

Hopefully that is what the patch series does right now as of v4. As I

If we really don't want another pci_device attribute we can probably 
invent another device property for the quirked things in patch 2 instead 
of "no_cmd_complete".

Re: [PATCH v4 00/10] Overhaul `is_thunderbolt`

[Mika Westerberg]

Hi Mario,

On Wed, Feb 16, 2022 at 10:50:31AM -0600, Limonciello, Mario wrote:
> On 2/16/2022 08:44, Alex Deucher wrote:
> > On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > 
> > > Hi all,
> > > 
> > > On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
> > > > On 2/15/2022 01:29, Lukas Wunner wrote:
> > > > > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
> > > > > >    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-acpi.c                  | 15 ++++-
> > > > > >    drivers/pci/pci.c                       | 17 +++--
> > > > > >    drivers/pci/probe.c                     | 52 ++++++++++++++-
> > > > > >    drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
> > > > > >    drivers/platform/x86/apple-gmux.c       |  2 +-
> > > > > >    drivers/thunderbolt/nhi.h               |  2 -
> > > > > >    include/linux/pci.h                     | 25 +-------
> > > > > >    include/linux/pci_ids.h                 |  3 +
> > > > > >    14 files changed, 173 insertions(+), 47 deletions(-)
> > > > > 
> > > > > That's an awful lot of additional LoC for what is primarily
> > > > > a refactoring job with the intent to simplify things.
> > > > 
> > > > You may recall the first version of this series was just for adding
> > > > USB4 matches to the existing code paths, and that's when it was noted
> > > > that is_thunderbolt is a bit overloaded.
> > > > 
> > > > > 
> > > > > Honestly this looks like an attempt to fix something that
> > > > > isn't broken.  Specifically, the is_thunderbolt bit apparently
> > > > > can't be removed without adding new bits to struct pci_dev.
> > > > > Not sure if that can be called progress. >
> > > > > Thanks,
> > > > > 
> > > > > Lukas
> > > > 
> > > > Within this series there are two new material patches; setting up root ports
> > > > for both integrated and discrete USB4 controllers to behave well with all
> > > > the existing drivers that rely upon a hint of how they're connected to
> > > > configure devices differently.
> > > > 
> > > > If y'all collectively prefer this direction to not refactor is_thunderbolt
> > > > and push into quirks, a simpler version of this series would be to leave all
> > > > the quirks in place, just drop dev->is_thunderbolt, and set
> > > > dev->external_facing on all 3 cases:
> > > > 
> > > > * Intel TBT controller
> > > > * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
> > > > * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
> > > > 
> > > > All the other drivers and symbols can stay the same then.
> > > 
> > > If I understand correctly the original intention of this patch series is
> > > to be able to differentiate whether the device is "permanently"
> > > connected to the motherboard, or it is connected over some hot-pluggable
> > > bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
> > > fit into this picture too). Specifically this is needed for discrete
> > > GPUs because of power management differences or so (please correct me if
> > > I'm mistaken).
> 
> Correct.  It might be possible to drop the patch for the integrated case
> (patch 3) because I do think that by Microsoft having the _DSD for
> "ExternalFacingPort" it's very likely that most implementations will have
> used it for the appropriate PCIe root ports.  If something shows up in the
> wild that this isn't the case it could be revisited.  If it's found
> pre-production presumably the OEM can still fix it and if it's post
> production and there are problems we can dust it off then.

Yeah, that's most likely the case.

> The discrete USB4 controller I would be more concerned that this isn't
> populated, and that (patch 4) should be more important to let the driver
> core set it removable.

Agreed.

[I actually only now noticed that the PCI core actually already marks
 devices connected to external facing ports as "removable" in
 pci_set_removable().]

> > > If we set the is_thunderbolt debate aside and concentrate on that issue,
> > > I think the way to do this is to check whether the root port the GPU is
> > > connected to has an ACPI power resource (returned from _PR3() method).
> > > IF it is present then most likely the platform has provided all the
> > > necessary wiring to move the GPU into D3cold (and the BIOS knows this).
> > > If it is not present then the device cannot even go into D3cold as there
> > > is not means to power of the device in PCIe spec.
> > > 
> > > Perhaps we can simply use pci_pr3_present() here as nouveau is already
> > > doing? Granted it is not too elegant solution either but better than
> > > using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
> > > perhaps we can have a helper in DRM core that handles this.
> > 
> > The tricky part is that there were AMD and NVIDIA specific proprietary
> > _PR3-like ACPI methods (plus whatever Apple did) prior to GPU power
> > control standardizing on _PR3.  Currently those methods are handled in
> > the drivers directly, sort of tangled up with vga_switcheroo.  I think
> > ideally that logic would move to the ACPI core and be handled the same
> > way as _PR3, but I'm not sure how well that would work because of the
> > various bios date checks around _PR3 and the lack of general _PR3
> > support in those older platforms.  So I think we still need some sort
> > of "is this soldered in" check.
> 
> Considering that limitation if `dev->external_facing` already exists in PCI
> core may as well use it for this instead of `is_thunderbolt`.

Indeed.

> > Alex
> > 
> > 
> > > 
> > > Then going back to is_thunderbolt debate :) I really don't think the
> > > drivers should care whether they are connected over a tunnel or not.
> > > They should work regardless of the underlying transport of the native
> > > protocol. They should also be prepared for the fact that the hardware
> > > can vanish under them at any point (e.g user unplugs the device). For
> > > this reason I don't really like to see is_thunderbolt to be used more
> > > and prefer to get rid if it completely if possible at all. If there is
> > > still need to differentiate whether the device can be hot-removed or
> > > not, I think "removable" in the driver core is the way to go. That is
> > > not dependent on any single transport.
> 
> Hopefully that is what the patch series does right now as of v4. As I

It does yes. I think the detection of internal and discrete tunneled
ports can be dropped from this series for now to make this leaner. We
can add those later when needed.