| Message ID | 20220211193250.1904843-6-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Overhaul `is_thunderbolt` | expand |
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-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/pci/pci-acpi.c | 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)))) > dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > -- > 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-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > drivers/pci/pci-acpi.c | 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
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);
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-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/pci/pci-acpi.c | 10 ++++++++++ drivers/pci/pci.h | 5 +++++ drivers/pci/probe.c | 1 + 3 files changed, 16 insertions(+)