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(-)
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
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
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-r... 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
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
`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
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;
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;
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);
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
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
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
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.
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.
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.
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".
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.
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Limonciello, Mario -
Lukas Wunner -
Mario Limonciello
-
Mika Westerberg