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 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 (12): thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4 PCI: Move `is_thunderbolt` check for lack of command completed to a quirk PCI: Move check for old Apple Thunderbolt controllers into a quirk PCI: Drop the `is_thunderbolt` attribute from PCI core PCI: Detect root port of internal USB4 devices by `usb4-host-interface` PCI: Explicitly mark USB4 NHI devices as removable PCI: Set ports for discrete USB4 controllers appropriately 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` platform/x86: amd-gmux: 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 | 10 ++++ drivers/pci/pci.c | 12 +++-- drivers/pci/pci.h | 5 ++ drivers/pci/probe.c | 55 ++++++++++++++----- drivers/pci/quirks.c | 70 +++++++++++++++++++++++++ drivers/platform/x86/apple-gmux.c | 2 +- drivers/thunderbolt/nhi.h | 2 - include/linux/pci.h | 25 +-------- include/linux/pci_ids.h | 3 ++ 15 files changed, 148 insertions(+), 56 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: 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
On Fri, Feb 11, 2022 at 01:32:39PM -0600, Mario Limonciello wrote:
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: Alex Deucher alexander.deucher@amd.com Acked-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com
I would change the subject to:
PCI: Add USB4 class definition
because this seems like more of a PCI thing than a Thunderbolt thing, but either way:
Acked-by: Bjorn Helgaas bhelgaas@google.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
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
Update subject to something like:
PCI: pciehp: Quirk broken Command Completed support on Intel Thunderbolt controllers
On Fri, Feb 11, 2022 at 01:32:40PM -0600, Mario Limonciello wrote:
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);
Please add a comment above to the effect that PCI_EXP_SLTCAP_NCCS being clear means the controller generates a Command Completed software notification when it completes a command, and these controllers don't generate those notifications even though PCI_EXP_SLTCAP_NCCS is clear (PCIe r6.0, sec 7.5.3.9).
+static void quirk_thunderbolt_command_completed(struct pci_dev *pdev)
Rename to quirk_no_command_completed(). This doesn't have anything to do with Thunderbolt; it's just that the affected devices happen to be Thunderbolt controllers.
+{
- 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);
Can we put these in drivers/pci/hotplug/pciehp_hpc.c? We already have a few similar quirks there.
#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
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt controller to indicate that D3 is possible. As this is used solely for older Apple systems, move it into a quirk that enumerates across all Intel TBT controllers.
Suggested-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com --- drivers/pci/pci.c | 12 +++++----- drivers/pci/quirks.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9ecce435fb3f..5002e214c9a6 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1064,7 +1064,13 @@ 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; + + if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3")) + return true; + + return false; }
/** @@ -2954,10 +2960,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..aaf098ca7d54 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL, quirk_apple_poweroff_thunderbolt); #endif
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify + * it in the ACPI tables + */ +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
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt controller to indicate that D3 is possible. As this is used solely for older Apple systems, move it into a quirk that enumerates across all Intel TBT controllers.
Suggested-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/pci/pci.c | 12 +++++----- drivers/pci/quirks.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9ecce435fb3f..5002e214c9a6 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1064,7 +1064,13 @@ 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;- if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3"))
return true;
Why do we need this? acpi_pci_bridge_d3() already looks for "HotPlugSupportInD3".
- return false;
}
/** @@ -2954,10 +2960,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..aaf098ca7d54 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL, quirk_apple_poweroff_thunderbolt); #endif
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
- it in the ACPI tables
Wrap to fit in 80 columns like the rest of the file. Also use the:
/* * comment ... */
style if it's more than one line.
I don't think "as old as 2010" is helpful here -- I assume 2010 is there because there *were* no Thunderbolt controllers before 2010, but the code doesn't check any dates, so we basically assume all Apple machines of any age with the listed controllers can do this.
- */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev) +{
- struct property_entry properties[] = {
PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),{},- };
- if (!x86_apple_machine)
return;
The current code doesn't check x86_apple_machine, so this needs some justification. How do I know this works the same as before?
- 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);
The current code assumes *all* Thunderbolt controllers support D3, so it would assume a controller released next year would support D3, but this code would assume the opposite. Are we supposed to add everything to this list, or do newer machines supply HotPlugSupportInD3, or ...?
How did you derive this list? (Question for the commit log and/or comments here.)
+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
On 2/11/2022 15:35, Bjorn Helgaas wrote:
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt controller to indicate that D3 is possible. As this is used solely for older Apple systems, move it into a quirk that enumerates across all Intel TBT controllers.
Suggested-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/pci/pci.c | 12 +++++----- drivers/pci/quirks.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9ecce435fb3f..5002e214c9a6 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1064,7 +1064,13 @@ 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;- if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3"))
return true;Why do we need this? acpi_pci_bridge_d3() already looks for "HotPlugSupportInD3".
The Apple machines don't have ACPI companion devices that specify this property.
I guess this probes a different question; can `device_property_read_bool` be used in `acpi_pci_bridge_d3` instead of:
if (acpi_dev_get_property(adev, "HotPlugSupportInD3", ACPI_TYPE_INTEGER, &obj) < 0) return false;
return obj->integer.value == 1;
If so, then yeah this can probably be simplified.
return false; }
/**
@@ -2954,10 +2960,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..aaf098ca7d54 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL, quirk_apple_poweroff_thunderbolt); #endif
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
- it in the ACPI tables
Wrap to fit in 80 columns like the rest of the file. Also use the:
/* * comment ... */
style if it's more than one line.
I don't think "as old as 2010" is helpful here -- I assume 2010 is there because there *were* no Thunderbolt controllers before 2010, but the code doesn't check any dates, so we basically assume all Apple machines of any age with the listed controllers can do this.
The old comment was saying that, which is where I got it from. Yeah, I'll update it.
- */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev) +{
- struct property_entry properties[] = {
PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),{},- };
- if (!x86_apple_machine)
return;The current code doesn't check x86_apple_machine, so this needs some justification. How do I know this works the same as before?
Mika and Lucas were saying the only reason for this codepath was Apple machines in the first place, which is where this idea came from.
Something specifically relevant is that the Apple machines use a SW connection manager, whereas everyone else up until USB4 devices use a firmware based connection manager with varying behaviors on generation (ICM).
- 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);The current code assumes *all* Thunderbolt controllers support D3, so it would assume a controller released next year would support D3, but this code would assume the opposite. Are we supposed to add everything to this list, or do newer machines supply HotPlugSupportInD3, or ...?
This quirk is intended specifically for Apple, which has stopped making Intel machines with Intel TBT controllers.
So I don't believe the list should be growing any more, if anything it might need to shrink if I got too many models that weren't actually in Apple products. Lucas probably needs to confirm that.
How did you derive this list? (Question for the commit log and/or comments here.)
I went to pci_ids.h and got all the Thunderbolt controllers listed there.
+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
Hi,
On Fri, Feb 11, 2022 at 04:06:20PM -0600, Limonciello, Mario wrote:
On 2/11/2022 15:35, Bjorn Helgaas wrote:
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt controller to indicate that D3 is possible. As this is used solely for older Apple systems, move it into a quirk that enumerates across all Intel TBT controllers.
Suggested-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/pci/pci.c | 12 +++++----- drivers/pci/quirks.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9ecce435fb3f..5002e214c9a6 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1064,7 +1064,13 @@ 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;- if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3"))
return true;Why do we need this? acpi_pci_bridge_d3() already looks for "HotPlugSupportInD3".
The Apple machines don't have ACPI companion devices that specify this property.
I guess this probes a different question; can `device_property_read_bool` be used in `acpi_pci_bridge_d3` instead of:
if (acpi_dev_get_property(adev, "HotPlugSupportInD3", ACPI_TYPE_INTEGER, &obj) < 0) return false;
return obj->integer.value == 1;
If so, then yeah this can probably be simplified.
Unfortunately the code in acpi_pci_bridge_d3() expects the device to have an ACPI_COMPANION() which may not be the case with software nodes.
- return false; } /**
@@ -2954,10 +2960,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..aaf098ca7d54 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL, quirk_apple_poweroff_thunderbolt); #endif +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
- it in the ACPI tables
Wrap to fit in 80 columns like the rest of the file. Also use the:
/* * comment ... */
style if it's more than one line.
I don't think "as old as 2010" is helpful here -- I assume 2010 is there because there *were* no Thunderbolt controllers before 2010, but the code doesn't check any dates, so we basically assume all Apple machines of any age with the listed controllers can do this.
The old comment was saying that, which is where I got it from. Yeah, I'll update it.
- */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev) +{
- struct property_entry properties[] = {
PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),{},- };
- if (!x86_apple_machine)
return;The current code doesn't check x86_apple_machine, so this needs some justification. How do I know this works the same as before?
Mika and Lucas were saying the only reason for this codepath was Apple machines in the first place, which is where this idea came from.
Yes, that's the reason.
Nobody else is going to need this except Apple machines with Intel Thunderbolt controller.
Something specifically relevant is that the Apple machines use a SW connection manager, whereas everyone else up until USB4 devices use a firmware based connection manager with varying behaviors on generation (ICM).
Yup.
- 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);The current code assumes *all* Thunderbolt controllers support D3, so it would assume a controller released next year would support D3, but this code would assume the opposite. Are we supposed to add everything to this list, or do newer machines supply HotPlugSupportInD3, or ...?
This quirk is intended specifically for Apple, which has stopped making Intel machines with Intel TBT controllers.
So I don't believe the list should be growing any more, if anything it might need to shrink if I got too many models that weren't actually in Apple products. Lucas probably needs to confirm that.
Yes correct it won't be growing more.
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt controller to indicate that D3 is possible. As this is used solely for older Apple systems, move it into a quirk that enumerates across all Intel TBT controllers.
I'm not so sure if it is only needed on Apple systems.
@@ -2954,10 +2960,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;
The fact that Thunderbolt PCIe ports support D3 is a property of those devices. It's not a property of the platform or a quirk of a particular vendor.
Hence in my view the current location of the check (pci_bridge_d3_possible()) makes sense wheras the location you're moving it to does not.
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
- it in the ACPI tables
- */
Apple started shipping Thunderbolt in 2011. Intel brought the first chips to market in 2010.
The date is meaningful at the code's current location in pci_bridge_d3_possible() because a few lines further down there's a 2015 BIOS cut-off date.
Microsoft came up with an ACPI property that BIOS vendors may set so that Windows knows it may put a Thunderbolt controller into D3cold. I'm not even sure if that property was ever officially adopted by the ACPI spec or if it's just a Microsoft-defined "standard".
Apple had been using its own scheme to put Thunderbolt controllers into D3cold when nothing is plugged in, about a decade before Microsoft defined the ACPI property.
I'm not sure if other vendors came up with their own schemes to power-manage Thunderbolt. We may regress those with the present patch.
Thanks,
Lukas
On Sun, Feb 13, 2022 at 10:19:20AM +0100, Lukas Wunner wrote:
Apple had been using its own scheme to put Thunderbolt controllers into D3cold when nothing is plugged in, about a decade before Microsoft defined the ACPI property.
I meant to say "half a decade", sorry.
Hi Lukas,
On Sun, Feb 13, 2022 at 10:19:20AM +0100, Lukas Wunner wrote:
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt controller to indicate that D3 is possible. As this is used solely for older Apple systems, move it into a quirk that enumerates across all Intel TBT controllers.
I'm not so sure if it is only needed on Apple systems.
@@ -2954,10 +2960,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;
The fact that Thunderbolt PCIe ports support D3 is a property of those devices. It's not a property of the platform or a quirk of a particular vendor.
It is in fact platform specific. For instance the non-Apple systems without "HotPlugSupportInD3" property have not been wired to support entering/exiting D3 runtime so putting these ports into D3 may actually lead into problems as we are in non-validated territory.
Hence in my view the current location of the check (pci_bridge_d3_possible()) makes sense wheras the location you're moving it to does not.
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
- it in the ACPI tables
- */
Apple started shipping Thunderbolt in 2011. Intel brought the first chips to market in 2010.
The date is meaningful at the code's current location in pci_bridge_d3_possible() because a few lines further down there's a 2015 BIOS cut-off date.
Microsoft came up with an ACPI property that BIOS vendors may set so that Windows knows it may put a Thunderbolt controller into D3cold. I'm not even sure if that property was ever officially adopted by the ACPI spec or if it's just a Microsoft-defined "standard".
AFAIK It was invented by Intel Windows folks but Microsoft "documented" it. We use the same property in ChromeOS (and therefore Linux) too so it can be thought of as "de facto" way of declaring such port.
The `is_thunderbolt` attribute is currently a dumping ground for a variety of things.
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 | 20 +++++++------------- drivers/platform/x86/apple-gmux.c | 2 +- include/linux/pci.h | 5 ++--- 3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..e41656cdd8f0 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1577,16 +1577,6 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) pdev->is_hotplug_bridge = 1; }
-static void set_pcie_thunderbolt(struct pci_dev *dev) -{ - u16 vsec; - - /* 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; -} - static void set_pcie_untrusted(struct pci_dev *dev) { struct pci_dev *parent; @@ -1603,6 +1593,10 @@ static void set_pcie_untrusted(struct pci_dev *dev) static void pci_set_removable(struct pci_dev *dev) { struct pci_dev *parent = pci_upstream_bridge(dev); + u16 vsec; + + /* Is the device a Thunderbolt controller? */ + vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
/* * We (only) consider everything downstream from an external_facing @@ -1615,8 +1609,9 @@ static void pci_set_removable(struct pci_dev *dev) * accessible to user / may not be removed by end user, and thus not * exposed as "removable" to userspace. */ - if (parent && - (parent->external_facing || dev_is_removable(&parent->dev))) + if (vsec || + (parent && + (parent->external_facing || dev_is_removable(&parent->dev)))) dev_set_removable(&dev->dev, DEVICE_REMOVABLE); }
@@ -1860,7 +1855,6 @@ int pci_setup_device(struct pci_dev *dev) dev->cfg_size = pci_cfg_space_size(dev);
/* Need to have dev->cfg_size ready */ - set_pcie_thunderbolt(dev);
set_pcie_untrusted(dev);
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 57553f9b4d1d..04232fbc7d56 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 pci_is_thunderbolt_attached(to_pci_dev(dev)); }
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;
On Fri, Feb 11, 2022 at 01:32:42PM -0600, Mario Limonciello wrote:
The `is_thunderbolt` attribute is currently a dumping ground for a variety of things.
It's not as arbitrary as it may seem. Quite a bit of thought went into the current design.
Instead use the driver core removable attribute to indicate the detail a device is attached to a thunderbolt or USB4 chain.
You're missing the point that "is_thunderbolt" is set on the *controller* (i.e. its upstream and downstream ports).
The controller itself is *not* removable if it's the host controller.
However a device can be assumed to be removable if it has an ancestor which has the "is_thunderbolt" flag set.
static void pci_set_removable(struct pci_dev *dev) { struct pci_dev *parent = pci_upstream_bridge(dev);
- u16 vsec;
- /* Is the device a Thunderbolt controller? */
- vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
This doesn't make any sense because the host controller is not removable.
@@ -1860,7 +1855,6 @@ int pci_setup_device(struct pci_dev *dev) dev->cfg_size = pci_cfg_space_size(dev);
/* Need to have dev->cfg_size ready */
set_pcie_thunderbolt(dev);
set_pcie_untrusted(dev);
Either drop the blank line or drop the code comment if set_pcie_untrusted() doesn't need dev->cfg_size.
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 57553f9b4d1d..04232fbc7d56 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 pci_is_thunderbolt_attached(to_pci_dev(dev));
}
No, the gmux driver changes its behavior if a Thunderbolt host controller is present. Not if there's a Thunderbolt-attached device present.
Thanks,
Lukas
On 2/13/2022 02:20, Lukas Wunner wrote:
On Fri, Feb 11, 2022 at 01:32:42PM -0600, Mario Limonciello wrote:
The `is_thunderbolt` attribute is currently a dumping ground for a variety of things.
It's not as arbitrary as it may seem. Quite a bit of thought went into the current design.
Instead use the driver core removable attribute to indicate the detail a device is attached to a thunderbolt or USB4 chain.
You're missing the point that "is_thunderbolt" is set on the *controller* (i.e. its upstream and downstream ports).
The controller itself is *not* removable if it's the host controller.
However a device can be assumed to be removable if it has an ancestor which has the "is_thunderbolt" flag set.
Ah right... I wonder if really what this series should be about then is setting up the the PCIe endpoints for PCIe tunneling and XHCI tunneling to be marked as "external" instead then. It would mean that existing code will apply the removable attribute to everything downstream (and presumably at least some of those drivers it will continue to make sense to drop "pcie_is_thunderbolt_attached" and instead check dev_is_removable.
static void pci_set_removable(struct pci_dev *dev) { struct pci_dev *parent = pci_upstream_bridge(dev);
- u16 vsec;
- /* Is the device a Thunderbolt controller? */
- vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
This doesn't make any sense because the host controller is not removable.
@@ -1860,7 +1855,6 @@ int pci_setup_device(struct pci_dev *dev) dev->cfg_size = pci_cfg_space_size(dev);
/* Need to have dev->cfg_size ready */
set_pcie_thunderbolt(dev);
set_pcie_untrusted(dev);
Either drop the blank line or drop the code comment if set_pcie_untrusted() doesn't need dev->cfg_size.
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 57553f9b4d1d..04232fbc7d56 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 pci_is_thunderbolt_attached(to_pci_dev(dev)); }
No, the gmux driver changes its behavior if a Thunderbolt host controller is present. Not if there's a Thunderbolt-attached device present.
Present in the system? Like not necessarily part of the topology?
Thanks,
Lukas
Hi Mario,
On Sun, Feb 13, 2022 at 11:26:56AM -0600, Limonciello, Mario wrote:
On 2/13/2022 02:20, Lukas Wunner wrote:
On Fri, Feb 11, 2022 at 01:32:42PM -0600, Mario Limonciello wrote:
The `is_thunderbolt` attribute is currently a dumping ground for a variety of things.
It's not as arbitrary as it may seem. Quite a bit of thought went into the current design.
Instead use the driver core removable attribute to indicate the detail a device is attached to a thunderbolt or USB4 chain.
You're missing the point that "is_thunderbolt" is set on the *controller* (i.e. its upstream and downstream ports).
The controller itself is *not* removable if it's the host controller.
However a device can be assumed to be removable if it has an ancestor which has the "is_thunderbolt" flag set.
Ah right... I wonder if really what this series should be about then is setting up the the PCIe endpoints for PCIe tunneling and XHCI tunneling to be marked as "external" instead then. It would mean that existing code will apply the removable attribute to everything downstream (and presumably at least some of those drivers it will continue to make sense to drop "pcie_is_thunderbolt_attached" and instead check dev_is_removable.
Yes, I think this is the right thing to do. Anything connected over PCIe/USB 3.x tunnel is pretty much "removable" whereas the host controllers may or may not. Typically they are not.
The root port used for PCIe tunneling should be marked as removable to ensure that the entire chain is marked removable.
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 | 10 ++++++++++ drivers/pci/pci.h | 5 +++++ drivers/pci/probe.c | 1 + 3 files changed, 16 insertions(+)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index a42dbf448860..6368e5633b1b 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -1391,6 +1391,16 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev) } }
+bool pci_acpi_is_usb4(struct pci_dev *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(&dev->dev); + + if (!adev) + return false; + return fwnode_property_present(acpi_fwnode_handle(adev), + "usb4-host-interface"); +} + static struct fwnode_handle *(*pci_msi_get_fwnode_cb)(struct device *dev);
/** diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 3d60cabde1a1..359607c0542d 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -695,6 +695,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); int acpi_pci_wakeup(struct pci_dev *dev, bool enable); bool acpi_pci_need_resume(struct pci_dev *dev); pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); +bool pci_acpi_is_usb4(struct pci_dev *dev); #else static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) { @@ -734,6 +735,10 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) { return PCI_POWER_ERROR; } +static inline bool pci_acpi_is_usb4(struct pci_dev *dev) +{ + return false; +} #endif
#ifdef CONFIG_PCIEASPM diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index e41656cdd8f0..2693211d31cf 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev) * exposed as "removable" to userspace. */ if (vsec || + pci_acpi_is_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(&parent->dev)))) dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
On Fri, Feb 11, 2022 at 01:32:43PM -0600, Mario Limonciello wrote:
The root port used for PCIe tunneling should be marked as removable to ensure that the entire chain is marked removable.
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 | 10 ++++++++++ drivers/pci/pci.h | 5 +++++ drivers/pci/probe.c | 1 + 3 files changed, 16 insertions(+)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index a42dbf448860..6368e5633b1b 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -1391,6 +1391,16 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev) } }
+bool pci_acpi_is_usb4(struct pci_dev *dev) +{
- struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
- if (!adev)
return false;- return fwnode_property_present(acpi_fwnode_handle(adev),
"usb4-host-interface");
Maybe it's obvious to everybody but me that "USB4" means this device is removable. The Microsoft reference above doesn't say anything about removability.
My expectation is that "USB" (like "PCI" and "PCIe") tells me something about how a device is electrically connected and how software can operate it. It doesn't really tell me anything about whether those electrical connections are permanent, made through an internal slot, or made through an external connector and cable.
+}
static struct fwnode_handle *(*pci_msi_get_fwnode_cb)(struct device *dev);
/** diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 3d60cabde1a1..359607c0542d 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -695,6 +695,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); int acpi_pci_wakeup(struct pci_dev *dev, bool enable); bool acpi_pci_need_resume(struct pci_dev *dev); pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); +bool pci_acpi_is_usb4(struct pci_dev *dev); #else static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) { @@ -734,6 +735,10 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) { return PCI_POWER_ERROR; } +static inline bool pci_acpi_is_usb4(struct pci_dev *dev) +{
- return false;
+} #endif
#ifdef CONFIG_PCIEASPM diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index e41656cdd8f0..2693211d31cf 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev) * exposed as "removable" to userspace. */ if (vsec ||
pci_acpi_is_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(&parent->dev))))-- 2.34.1
Hi Bjorn,
On Fri, Feb 11, 2022 at 03:45:46PM -0600, Bjorn Helgaas wrote:
On Fri, Feb 11, 2022 at 01:32:43PM -0600, Mario Limonciello wrote:
The root port used for PCIe tunneling should be marked as removable to ensure that the entire chain is marked removable.
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 | 10 ++++++++++ drivers/pci/pci.h | 5 +++++ drivers/pci/probe.c | 1 + 3 files changed, 16 insertions(+)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index a42dbf448860..6368e5633b1b 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -1391,6 +1391,16 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev) } }
+bool pci_acpi_is_usb4(struct pci_dev *dev) +{
- struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
- if (!adev)
return false;- return fwnode_property_present(acpi_fwnode_handle(adev),
"usb4-host-interface");Maybe it's obvious to everybody but me that "USB4" means this device is removable. The Microsoft reference above doesn't say anything about removability.
My expectation is that "USB" (like "PCI" and "PCIe") tells me something about how a device is electrically connected and how software can operate it. It doesn't really tell me anything about whether those electrical connections are permanent, made through an internal slot, or made through an external connector and cable.
It is used to identify "tunneled" ports (whether PCIe, USB 3.x or DisplayPort). Tunnels are created by software (in Linux it is the Thunderbolt driver) and are dynamic in nature. The USB4 links go over USB Type-C cable which also is something user can plug/unplug freely.
I would say it is reasonable expectation that anything behind these ports can be assumed as "removable".
On Mon, Feb 14, 2022 at 09:34:26AM +0200, Mika Westerberg wrote:
On Fri, Feb 11, 2022 at 03:45:46PM -0600, Bjorn Helgaas wrote:
My expectation is that "USB" (like "PCI" and "PCIe") tells me something about how a device is electrically connected and how software can operate it. It doesn't really tell me anything about whether those electrical connections are permanent, made through an internal slot, or made through an external connector and cable.
It is used to identify "tunneled" ports (whether PCIe, USB 3.x or DisplayPort). Tunnels are created by software (in Linux it is the Thunderbolt driver) and are dynamic in nature. The USB4 links go over USB Type-C cable which also is something user can plug/unplug freely.
I would say it is reasonable expectation that anything behind these ports can be assumed as "removable".
USB gadgets may be soldered to the mainboard. Those cannot be unplugged freely. It is common practice to solder USB Ethernet or USB FTDI serial ports and nothing's preventing a vendor to solder USB4/Thunderbolt gadgets.
On Mon, Feb 14, 2022 at 09:52:02AM +0100, Lukas Wunner wrote:
On Mon, Feb 14, 2022 at 09:34:26AM +0200, Mika Westerberg wrote:
On Fri, Feb 11, 2022 at 03:45:46PM -0600, Bjorn Helgaas wrote:
My expectation is that "USB" (like "PCI" and "PCIe") tells me something about how a device is electrically connected and how software can operate it. It doesn't really tell me anything about whether those electrical connections are permanent, made through an internal slot, or made through an external connector and cable.
It is used to identify "tunneled" ports (whether PCIe, USB 3.x or DisplayPort). Tunnels are created by software (in Linux it is the Thunderbolt driver) and are dynamic in nature. The USB4 links go over USB Type-C cable which also is something user can plug/unplug freely.
I would say it is reasonable expectation that anything behind these ports can be assumed as "removable".
USB gadgets may be soldered to the mainboard. Those cannot be unplugged freely. It is common practice to solder USB Ethernet or USB FTDI serial ports and nothing's preventing a vendor to solder USB4/Thunderbolt gadgets.
Right, that's why I say it is "reasonable expectation" that anything behind these ports can be assumed "removable" :) Of course they don't have to be but if we assume that in the driver where this actually matters we should be on the safe side, no?
On Mon, Feb 14, 2022 at 01:11:05PM +0200, Mika Westerberg wrote:
It is used to identify "tunneled" ports (whether PCIe, USB 3.x or DisplayPort). Tunnels are created by software (in Linux it is the Thunderbolt driver) and are dynamic in nature. The USB4 links go over USB Type-C cable which also is something user can plug/unplug freely.
I would say it is reasonable expectation that anything behind these ports can be assumed as "removable".
USB gadgets may be soldered to the mainboard. Those cannot be unplugged freely. It is common practice to solder USB Ethernet or USB FTDI serial ports and nothing's preventing a vendor to solder USB4/Thunderbolt gadgets.
Right, that's why I say it is "reasonable expectation" that anything behind these ports can be assumed "removable" :) Of course they don't have to be but if we assume that in the driver where this actually matters we should be on the safe side, no?
Also the tunnels are not permanent anyway.
On Mon, Feb 14, 2022 at 12:56:58PM +0200, Mika Westerberg wrote:
On Mon, Feb 14, 2022 at 09:52:02AM +0100, Lukas Wunner wrote:
On Mon, Feb 14, 2022 at 09:34:26AM +0200, Mika Westerberg wrote:
On Fri, Feb 11, 2022 at 03:45:46PM -0600, Bjorn Helgaas wrote:
My expectation is that "USB" (like "PCI" and "PCIe") tells me something about how a device is electrically connected and how software can operate it. It doesn't really tell me anything about whether those electrical connections are permanent, made through an internal slot, or made through an external connector and cable.
It is used to identify "tunneled" ports (whether PCIe, USB 3.x or DisplayPort). Tunnels are created by software (in Linux it is the Thunderbolt driver) and are dynamic in nature. The USB4 links go over USB Type-C cable which also is something user can plug/unplug freely.
I would say it is reasonable expectation that anything behind these ports can be assumed as "removable".
USB gadgets may be soldered to the mainboard. Those cannot be unplugged freely. It is common practice to solder USB Ethernet or USB FTDI serial ports and nothing's preventing a vendor to solder USB4/Thunderbolt gadgets.
Right, that's why I say it is "reasonable expectation" that anything behind these ports can be assumed "removable" :) Of course they don't have to be but if we assume that in the driver where this actually matters we should be on the safe side, no?
Spec citations help maintain things over the long term. Reasonable expectations that are part of today's folklore but are not written down and shared leads to things that work today but not tomorrow.
Bjorn
USB4 class devices are also removable like Intel Thunderbolt devices.
Drivers of downstream devices use this information to declare functional differences in how the drivers perform by knowing that they are connected to an upstream TBT/USB4 port.
Reviewed-by: Macpaul Lin macpaul.lin@mediatek.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com --- drivers/pci/probe.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2693211d31cf..67ca33188cba 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev) * exposed as "removable" to userspace. */ if (vsec || + dev->class == PCI_CLASS_SERIAL_USB_USB4 || pci_acpi_is_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(&parent->dev))))
On Fri, Feb 11, 2022 at 01:32:44PM -0600, Mario Limonciello wrote:
USB4 class devices are also removable like Intel Thunderbolt devices.
Spec reference, please.
Drivers of downstream devices use this information to declare functional differences in how the drivers perform by knowing that they are connected to an upstream TBT/USB4 port.
Reviewed-by: Macpaul Lin macpaul.lin@mediatek.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/pci/probe.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2693211d31cf..67ca33188cba 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev) * exposed as "removable" to userspace. */ if (vsec ||
dev->class == PCI_CLASS_SERIAL_USB_USB4 || pci_acpi_is_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(&parent->dev))))-- 2.34.1
Discrete USB4 controllers won't have ACPI nodes specifying which PCIe or XHCI port they are linked with.
In order to set the removable attribute appropriately, 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 | 33 +++++++++++++++++++++++++++++++++ include/linux/pci_ids.h | 2 ++ 2 files changed, 35 insertions(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 67ca33188cba..1ed3e24db11e 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, @@ -1590,6 +1592,36 @@ static void set_pcie_untrusted(struct pci_dev *dev) dev->untrusted = true; }
+static bool pci_is_discrete_usb4(struct pci_dev *dev) +{ + int dvsec_val = 0, pos; + u32 hdr; + + /* USB4 spec says vendors can use either */ + 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 false; + + pci_read_config_dword(dev, pos + PCI_DVSEC_HEADER2, &hdr); + if ((hdr & GENMASK(15, 0)) != dvsec_val) + return false; + /* this port is used for either NHI/PCIe tunnel/USB tunnel */ + if (hdr & GENMASK(18, 16)) + return true; + return false; +} + static void pci_set_removable(struct pci_dev *dev) { struct pci_dev *parent = pci_upstream_bridge(dev); @@ -1612,6 +1644,7 @@ static void pci_set_removable(struct pci_dev *dev) if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4 || pci_acpi_is_usb4(dev) || + pci_is_discrete_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(&parent->dev)))) dev_set_removable(&dev->dev, DEVICE_REMOVABLE); diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 61b161d914f0..271326e058b9 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -3097,4 +3097,6 @@
#define PCI_VENDOR_ID_NCUBE 0x10ff
+#define PCI_VENDOR_ID_USB_IF 0x1EC0 + #endif /* _LINUX_PCI_IDS_H */
Make the subject specific, not just "appropriately." I think you're marking something *removable*, so include that.
On Fri, Feb 11, 2022 at 01:32:45PM -0600, Mario Limonciello wrote:
Discrete USB4 controllers won't have ACPI nodes specifying which PCIe or XHCI port they are linked with.
In order to set the removable attribute appropriately, use the USB4 DVSEC extended capabability set on these root ports to determine if they are located on a discrete USB4 controller.
s/capabability/capability/
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 | 33 +++++++++++++++++++++++++++++++++ include/linux/pci_ids.h | 2 ++ 2 files changed, 35 insertions(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 67ca33188cba..1ed3e24db11e 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, @@ -1590,6 +1592,36 @@ static void set_pcie_untrusted(struct pci_dev *dev) dev->untrusted = true; }
+static bool pci_is_discrete_usb4(struct pci_dev *dev) +{
- int dvsec_val = 0, pos;
- u32 hdr;
- /* USB4 spec says vendors can use either */
- 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 false;- pci_read_config_dword(dev, pos + PCI_DVSEC_HEADER2, &hdr);
- if ((hdr & GENMASK(15, 0)) != dvsec_val)
return false;- /* this port is used for either NHI/PCIe tunnel/USB tunnel */
Capitalize comment like others in this file.
Spec reference would be helpful, too. I don't know how to verify any of these values. The Link: above is great, but name, revision, section number would be even better.
- if (hdr & GENMASK(18, 16))
return true;- return false;
+}
static void pci_set_removable(struct pci_dev *dev) { struct pci_dev *parent = pci_upstream_bridge(dev); @@ -1612,6 +1644,7 @@ static void pci_set_removable(struct pci_dev *dev) if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4 || pci_acpi_is_usb4(dev) ||
pci_is_discrete_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(&parent->dev))))diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 61b161d914f0..271326e058b9 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -3097,4 +3097,6 @@
#define PCI_VENDOR_ID_NCUBE 0x10ff
+#define PCI_VENDOR_ID_USB_IF 0x1EC0
This file is supposed to be sorted by Vendor ID. PCI_VENDOR_ID_XEN, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_NCUBE screwed up, but you can put USB_IF in the correct spot.
It's not 100%, but most entries use lower-case hex.
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.
The PCI core now marks such devices as removable and downstream drivers can use this instead.
Acked-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Mario Limonciello mario.limonciello@amd.com --- drivers/platform/x86/apple-gmux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 04232fbc7d56..ffac15b9befd 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 pci_is_thunderbolt_attached(to_pci_dev(dev)); + return dev_is_removable(dev); }
static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
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
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Limonciello, Mario -
Lukas Wunner -
Mario Limonciello
-
Mika Westerberg