All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
@ 2022-02-04 18:28 Mario Limonciello
  2022-02-04 18:28 ` [PATCH 1/2] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4 Mario Limonciello
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Mario Limonciello @ 2022-02-04 18:28 UTC (permalink / raw)
  To: Bjorn Helgaas, Andreas Noever, Mika Westerberg,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER
  Cc: Michael Jamet, Yehezkel Bernat, Alexander.Deucher, Mario Limonciello

Various drivers in the kernel use `pci_is_thunderbolt_attached` to
designate behaving differently from a device that is internally in
the machine. This function relies upon the `is_thunderbolt` designation
which 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.

Mario Limonciello (2):
  thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4
  pci: mark USB4 devices as "is_thunderbolt"

 drivers/pci/probe.c       | 4 ++--
 drivers/thunderbolt/nhi.h | 2 --
 include/linux/pci_ids.h   | 1 +
 3 files changed, 3 insertions(+), 4 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4
  2022-02-04 18:28 [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Mario Limonciello
@ 2022-02-04 18:28 ` Mario Limonciello
  2022-02-04 18:28 ` [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt" Mario Limonciello
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Mario Limonciello @ 2022-02-04 18:28 UTC (permalink / raw)
  To: Bjorn Helgaas, Andreas Noever, Mika Westerberg,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER
  Cc: Michael Jamet, Yehezkel Bernat, Alexander.Deucher, Mario Limonciello

This definition will be needed by a pci core for upcoming
changes.  Move it into common include file.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/nhi.h | 2 --
 include/linux/pci_ids.h   | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt"
  2022-02-04 18:28 [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Mario Limonciello
  2022-02-04 18:28 ` [PATCH 1/2] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4 Mario Limonciello
@ 2022-02-04 18:28 ` Mario Limonciello
  2022-02-04 22:29   ` Bjorn Helgaas
  2022-02-04 18:36 ` [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Deucher, Alexander
  2022-02-07  6:41 ` Mika Westerberg
  3 siblings, 1 reply; 12+ messages in thread
From: Mario Limonciello @ 2022-02-04 18:28 UTC (permalink / raw)
  To: Bjorn Helgaas, Andreas Noever, Mika Westerberg,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER
  Cc: Michael Jamet, Yehezkel Bernat, Alexander.Deucher, Mario Limonciello

Downstream drivers use this information to declare functional
differences in how the drivers performed by knowing that they are
connected to an upstream TBT/USB4 port.

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

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..b59f6c05e606 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1581,9 +1581,9 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
 {
 	u16 vsec;
 
-	/* Is the device part of a Thunderbolt controller? */
+	/* Is the device part of a Thunderbolt or USB4 controller? */
 	vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
-	if (vsec)
+	if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4)
 		dev->is_thunderbolt = 1;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* RE: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-04 18:28 [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Mario Limonciello
  2022-02-04 18:28 ` [PATCH 1/2] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4 Mario Limonciello
  2022-02-04 18:28 ` [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt" Mario Limonciello
@ 2022-02-04 18:36 ` Deucher, Alexander
  2022-02-07  6:41 ` Mika Westerberg
  3 siblings, 0 replies; 12+ messages in thread
From: Deucher, Alexander @ 2022-02-04 18:36 UTC (permalink / raw)
  To: Limonciello, Mario, Bjorn Helgaas, Andreas Noever,
	Mika Westerberg, open list:PCI SUBSYSTEM,
	open list:THUNDERBOLT DRIVER
  Cc: Michael Jamet, Yehezkel Bernat

[Public]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, February 4, 2022 1:28 PM
> To: Bjorn Helgaas <bhelgaas@google.com>; Andreas Noever
> <andreas.noever@gmail.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; open list:PCI SUBSYSTEM <linux-
> pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> usb@vger.kernel.org>
> Cc: Michael Jamet <michael.jamet@intel.com>; Yehezkel Bernat
> <YehezkelShB@gmail.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
> 
> Various drivers in the kernel use `pci_is_thunderbolt_attached` to designate
> behaving differently from a device that is internally in the machine. This
> function relies upon the `is_thunderbolt` designation which 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.
> 
> Mario Limonciello (2):
>   thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4
>   pci: mark USB4 devices as "is_thunderbolt"

Series is:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> 
>  drivers/pci/probe.c       | 4 ++--
>  drivers/thunderbolt/nhi.h | 2 --
>  include/linux/pci_ids.h   | 1 +
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> --
> 2.34.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt"
  2022-02-04 18:28 ` [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt" Mario Limonciello
@ 2022-02-04 22:29   ` Bjorn Helgaas
  2022-02-05  9:39     ` Lukas Wunner
  0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2022-02-04 22:29 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Bjorn Helgaas, Andreas Noever, Mika Westerberg,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER,
	Michael Jamet, Yehezkel Bernat, Alexander.Deucher

Follow subject line capitalization conventions:

  $ git log --oneline drivers/pci/probe.c
  661c4c4f2693 PCI: Let pcibios_root_bridge_prepare() access bridge->windows
  d2c64f98c387 PCI: Use pci_find_vsec_capability() when looking for TBT devices
  fd1ae23b495b PCI: Prefer 'unsigned int' over bare 'unsigned'
  e1b0d0bb2032 PCI: Re-enable Downstream Port LTR after reset or hotplug
  7c3855c423b1 PCI: Coalesce host bridge contiguous apertures
  06dc660e6eb8 PCI: Rename pcibios_add_device() to pcibios_device_add()
  41dd40fd7179 PCI: Support populating MSI domains of root buses via bridges

On Fri, Feb 04, 2022 at 12:28:20PM -0600, Mario Limonciello wrote:

Add an intro sentence to tell us what this patch does (mark USB4
devices as 'is_thunderbolt'.  I know it's in the subject, but people
read the commit log separately and it should be complete in itself.
A title is not the first sentence of a book.

> Downstream drivers use this information to declare functional
> differences in how the drivers performed by knowing that they are
> connected to an upstream TBT/USB4 port.

s/Downstream drivers/Drivers of downstream devices/
s/performed/perform/  (I guess?)

I'm guessing this really refers to differences in how *devices*
(not drivers) perform when they are below a Thunderbolt or USB port.

I've never liked "is_thunderbolt" because it tells us nothing about
what functionality is of interest, so it's an unmaintainable mess.

Right now:

  - We assume Root Ports and Switch Ports marked "is_thunderbolt"
    support D3 (pci_bridge_d3_possible()).

  - Downstream Ports marked "is_thunderbolt" don't support native
    hotplug Command Completed events, even if they claim they do
    (pcie_init()).

  - Apparently, if *any* device in the system is marked
    "is_thunderbolt", a GPU external DP port is not fully switchable
    because ? (gmux_probe()).

  - Whether an AMD GPU is attached via Thunderbolt tells us something
    about what sort of power control and runtime power management we
    can do (amdgpu_driver_load_kms(), radeon_driver_load_kms()).

  - We don't register Thunderbolt eGPU devices with VGA switcheroo
    because ? (nouveau_vga_init(), radeon_device_init()).

  - If an AMD GPU is attached via Thunderbolt, we program different
    ASPM time values because ? (nbio_v2_3_enable_aspm()).

This is totally bonkers.

Broken things like hotplug Command Completed should be quirks.  That
one is my fault.

The ASPM thing should be integrated with the PCI core somehow.  It's
just asking for trouble to have the PCI core assuming *it* is managing
ASPM and then have drivers doing non-standard ASPM stuff behind its
back.

I have no idea about the VGA switching and power management stuff.

But it's not clear to me that these all fit under the "is_thunderbolt"
umbrella.  If they actually do correspond to Thunderbolt- or USB4-
specific features, we should include references to the pertinent
sections of the spec.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..b59f6c05e606 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1581,9 +1581,9 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
>  {
>  	u16 vsec;
>  
> -	/* Is the device part of a Thunderbolt controller? */
> +	/* Is the device part of a Thunderbolt or USB4 controller? */
>  	vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
> -	if (vsec)
> +	if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4)
>  		dev->is_thunderbolt = 1;

This could be rewritten as:

  if (dev->class == PCI_CLASS_SERIAL_USB_USB4 ||
      pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT))
          dev->is_thunderbolt = 1;

to avoid searching USB4 devices for a TBT capability unnecessarily.

>  }
>  
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt"
  2022-02-04 22:29   ` Bjorn Helgaas
@ 2022-02-05  9:39     ` Lukas Wunner
  0 siblings, 0 replies; 12+ messages in thread
From: Lukas Wunner @ 2022-02-05  9:39 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Mario Limonciello, Andreas Noever, Mika Westerberg,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER,
	Michael Jamet, Yehezkel Bernat, Alexander.Deucher

On Fri, Feb 04, 2022 at 04:29:56PM -0600, Bjorn Helgaas wrote:
> I've never liked "is_thunderbolt" because it tells us nothing about
> what functionality is of interest, so it's an unmaintainable mess.
> 
> Right now:
> 
>   - We assume Root Ports and Switch Ports marked "is_thunderbolt"
>     support D3 (pci_bridge_d3_possible()).

We don't allow D3 on hotplug bridges because:

		/*
		 * Hotplug ports handled natively by the OS were not validated
		 * by vendors for runtime D3 at least until 2018 because there
		 * was no OS support.
		 */
		if (bridge->is_hotplug_bridge)
			return false;

And we don't allow D3 on older non-hotplug bridges because:

		/*
		 * It should be safe to put PCIe ports from 2015 or newer
		 * to D3.
		 */
		if (dmi_get_bios_year() >= 2015)
			return true;

However we must allow D3 on *Thunderbolt* bridges to take advantage
of power savings.  So the following check is an exception of the
above-stated rules:

		/* Even the oldest 2010 Thunderbolt controller supports D3. */
		if (bridge->is_thunderbolt)
			return true;

This is most likely necessary for AMD Thunderbolt as well, but
could be achieved by adding another check to pci_bridge_d3_possible()
which returns true for the USB4 class.


>   - Downstream Ports marked "is_thunderbolt" don't support native
>     hotplug Command Completed events, even if they claim they do
>     (pcie_init()).

That's a quirk needed for older Thunderbolt controllers.  It could be
replaced by a check for the device IDs listed in 493fb50e958c.

It most likely does not affect AMD Thunderbolt.


>   - Apparently, if *any* device in the system is marked
>     "is_thunderbolt", a GPU external DP port is not fully switchable
>     because ? (gmux_probe()).

This could be replaced by a DMI check for the affected MacBook Pro
models.  Those happen not to possess a Thunderbolt controller,
so checking for Thunderbolt presence seemed simpler and more clever
at the time...

I can produce a list of affected models if you want.

This does not affect AMD Thunderbolt.


>   - Whether an AMD GPU is attached via Thunderbolt tells us something
>     about what sort of power control and runtime power management we
>     can do (amdgpu_driver_load_kms(), radeon_driver_load_kms()).

External eGPUs are not supposed to be managed by vga_switcheroo
(which is only responsible for switching between a chipset-integrated iGPU
and an on-board discrete dGPU), that's what these checks are for.

This does affect AMD Thunderbolt.


>   - We don't register Thunderbolt eGPU devices with VGA switcheroo
>     because ? (nouveau_vga_init(), radeon_device_init()).

Same as above.


>   - If an AMD GPU is attached via Thunderbolt, we program different
>     ASPM time values because ? (nbio_v2_3_enable_aspm()).

That wasn't introduced by me, so not sure what the rationale is.

Let me know if I can help clarify things further so that we can
find a solution that you feel more comfortable with.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-04 18:28 [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Mario Limonciello
                   ` (2 preceding siblings ...)
  2022-02-04 18:36 ` [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Deucher, Alexander
@ 2022-02-07  6:41 ` Mika Westerberg
  2022-02-07 15:00   ` Deucher, Alexander
  3 siblings, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2022-02-07  6:41 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Bjorn Helgaas, Andreas Noever, open list:PCI SUBSYSTEM,
	open list:THUNDERBOLT DRIVER, Michael Jamet, Yehezkel Bernat,
	Alexander.Deucher

Hi Mario,

On Fri, Feb 04, 2022 at 12:28:18PM -0600, Mario Limonciello wrote:
> Various drivers in the kernel use `pci_is_thunderbolt_attached` to
> designate behaving differently from a device that is internally in
> the machine. This function relies upon the `is_thunderbolt` designation
> which 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.

Not objecting this if really needed but since USB4 is supposed to be
transparent to the native (tunneled) protocol, I would rather try to
figure out if there is really need to change driver behaviour whether it
is connected over USB4 or plugged natively on the PCIe slot.

Can you elaborate a bit what kind of functionality needs this? Perhaps
we can figure a better alternative?

Thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-07  6:41 ` Mika Westerberg
@ 2022-02-07 15:00   ` Deucher, Alexander
  2022-02-07 15:45     ` Mika Westerberg
  0 siblings, 1 reply; 12+ messages in thread
From: Deucher, Alexander @ 2022-02-07 15:00 UTC (permalink / raw)
  To: Mika Westerberg, Limonciello, Mario
  Cc: Bjorn Helgaas, Andreas Noever, open list:PCI SUBSYSTEM,
	open list:THUNDERBOLT DRIVER, Michael Jamet, Yehezkel Bernat

[Public]

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Monday, February 7, 2022 1:42 AM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; Andreas Noever
> <andreas.noever@gmail.com>; open list:PCI SUBSYSTEM <linux-
> pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> usb@vger.kernel.org>; Michael Jamet <michael.jamet@intel.com>; Yehezkel
> Bernat <YehezkelShB@gmail.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
> 
> Hi Mario,
> 
> On Fri, Feb 04, 2022 at 12:28:18PM -0600, Mario Limonciello wrote:
> > Various drivers in the kernel use `pci_is_thunderbolt_attached` to
> > designate behaving differently from a device that is internally in the
> > machine. This function relies upon the `is_thunderbolt` designation
> > which 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.
> 
> Not objecting this if really needed but since USB4 is supposed to be
> transparent to the native (tunneled) protocol, I would rather try to figure out
> if there is really need to change driver behaviour whether it is connected
> over USB4 or plugged natively on the PCIe slot.
> 
> Can you elaborate a bit what kind of functionality needs this? Perhaps we can
> figure a better alternative?

In the AMD GPU driver we use it to determine which dGPU is built into a platform vs. externally connected since the internal one uses ACPI for certain things and the external one does not.  There are probably other ways to determine this, but it's not in place at the moment.

Alex

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-07 15:00   ` Deucher, Alexander
@ 2022-02-07 15:45     ` Mika Westerberg
  2022-02-07 15:52       ` Deucher, Alexander
  0 siblings, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2022-02-07 15:45 UTC (permalink / raw)
  To: Deucher, Alexander
  Cc: Limonciello, Mario, Bjorn Helgaas, Andreas Noever,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER,
	Michael Jamet, Yehezkel Bernat

Hi,

On Mon, Feb 07, 2022 at 03:00:19PM +0000, Deucher, Alexander wrote:
> [Public]
> 
> > -----Original Message-----
> > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Sent: Monday, February 7, 2022 1:42 AM
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>; Andreas Noever
> > <andreas.noever@gmail.com>; open list:PCI SUBSYSTEM <linux-
> > pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> > usb@vger.kernel.org>; Michael Jamet <michael.jamet@intel.com>; Yehezkel
> > Bernat <YehezkelShB@gmail.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>
> > Subject: Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
> > 
> > Hi Mario,
> > 
> > On Fri, Feb 04, 2022 at 12:28:18PM -0600, Mario Limonciello wrote:
> > > Various drivers in the kernel use `pci_is_thunderbolt_attached` to
> > > designate behaving differently from a device that is internally in the
> > > machine. This function relies upon the `is_thunderbolt` designation
> > > which 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.
> > 
> > Not objecting this if really needed but since USB4 is supposed to be
> > transparent to the native (tunneled) protocol, I would rather try to figure out
> > if there is really need to change driver behaviour whether it is connected
> > over USB4 or plugged natively on the PCIe slot.
> > 
> > Can you elaborate a bit what kind of functionality needs this? Perhaps we can
> > figure a better alternative?
> 
> In the AMD GPU driver we use it to determine which dGPU is built into
> a platform vs. externally connected since the internal one uses ACPI
> for certain things and the external one does not.  There are probably
> other ways to determine this, but it's not in place at the moment.

Can't you check if the device is behind a hotplug bridge? Then for
certain it is "removable".

The other option is to look for ACPI companion (ACPI_COMPANION()) of the
device. AFAICT dGPUs don't have one (as the BIOS does not know in
advance what will be connected to the hotplug ports) whereas internal
does typically have one.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-07 15:45     ` Mika Westerberg
@ 2022-02-07 15:52       ` Deucher, Alexander
  2022-02-07 17:47         ` Lukas Wunner
  0 siblings, 1 reply; 12+ messages in thread
From: Deucher, Alexander @ 2022-02-07 15:52 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Limonciello, Mario, Bjorn Helgaas, Andreas Noever,
	open list:PCI SUBSYSTEM, open list:THUNDERBOLT DRIVER,
	Michael Jamet, Yehezkel Bernat

[Public]

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Monday, February 7, 2022 10:45 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Bjorn Helgaas
> <bhelgaas@google.com>; Andreas Noever <andreas.noever@gmail.com>;
> open list:PCI SUBSYSTEM <linux-pci@vger.kernel.org>; open
> list:THUNDERBOLT DRIVER <linux-usb@vger.kernel.org>; Michael Jamet
> <michael.jamet@intel.com>; Yehezkel Bernat <YehezkelShB@gmail.com>
> Subject: Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
> 
> Hi,
> 
> On Mon, Feb 07, 2022 at 03:00:19PM +0000, Deucher, Alexander wrote:
> > [Public]
> >
> > > -----Original Message-----
> > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Sent: Monday, February 7, 2022 1:42 AM
> > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>; Andreas Noever
> > > <andreas.noever@gmail.com>; open list:PCI SUBSYSTEM <linux-
> > > pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> > > usb@vger.kernel.org>; Michael Jamet <michael.jamet@intel.com>;
> > > Yehezkel Bernat <YehezkelShB@gmail.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>
> > > Subject: Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
> > >
> > > Hi Mario,
> > >
> > > On Fri, Feb 04, 2022 at 12:28:18PM -0600, Mario Limonciello wrote:
> > > > Various drivers in the kernel use `pci_is_thunderbolt_attached` to
> > > > designate behaving differently from a device that is internally in
> > > > the machine. This function relies upon the `is_thunderbolt`
> > > > designation which 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.
> > >
> > > Not objecting this if really needed but since USB4 is supposed to be
> > > transparent to the native (tunneled) protocol, I would rather try to
> > > figure out if there is really need to change driver behaviour
> > > whether it is connected over USB4 or plugged natively on the PCIe slot.
> > >
> > > Can you elaborate a bit what kind of functionality needs this?
> > > Perhaps we can figure a better alternative?
> >
> > In the AMD GPU driver we use it to determine which dGPU is built into
> > a platform vs. externally connected since the internal one uses ACPI
> > for certain things and the external one does not.  There are probably
> > other ways to determine this, but it's not in place at the moment.
> 
> Can't you check if the device is behind a hotplug bridge? Then for certain it is
> "removable".

Unfortunately, most (all?) of the root ports on modern platforms are hotplug capable even if the hardware is soldered to the board.

> 
> The other option is to look for ACPI companion (ACPI_COMPANION()) of the
> device. AFAICT dGPUs don't have one (as the BIOS does not know in advance
> what will be connected to the hotplug ports) whereas internal does typically
> have one.

Yeah, this is probably the right way to do this.

Alex

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-07 15:52       ` Deucher, Alexander
@ 2022-02-07 17:47         ` Lukas Wunner
  2022-02-08  6:33           ` Mika Westerberg
  0 siblings, 1 reply; 12+ messages in thread
From: Lukas Wunner @ 2022-02-07 17:47 UTC (permalink / raw)
  To: Deucher, Alexander
  Cc: Mika Westerberg, Limonciello, Mario, Bjorn Helgaas,
	Andreas Noever, open list:PCI SUBSYSTEM,
	open list:THUNDERBOLT DRIVER, Michael Jamet, Yehezkel Bernat

On Mon, Feb 07, 2022 at 03:52:13PM +0000, Deucher, Alexander wrote:
> > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > The other option is to look for ACPI companion (ACPI_COMPANION()) of the
> > device. AFAICT dGPUs don't have one (as the BIOS does not know in advance
> > what will be connected to the hotplug ports) whereas internal does typically
> > have one.
> 
> Yeah, this is probably the right way to do this.

No, that doesn't work.  At least Apple represents the first few devices
in the Thunderbolt daisy-chain in the ACPI namespace, so IIUC you'd find
an ACPI companion for those but not for the remainder of the daisy-chain.
This is from a 2019/2020 MacBookPro16,1:

$ grep 'Device ' acpidump/mbp161/ssdt6.dsl
            Device (UPSB)
                Device (DSB0)
                    Device (NHI0)
                Device (DSB1)
                    Device (UPS0)
                        Device (DSB0)
                            Device (DEV0)
                        Device (DSB3)
                            Device (UPS0)
                                Device (DSB0)
                                    Device (DEV0)
                                Device (DSB3)
                                    Device (DEV0)
            ...

There's a *reason* why I introduced the is_thunderbolt flag,
there is no other reliable way to detect externally attached devices.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Mark USB4 controllers as is_thunderbolt
  2022-02-07 17:47         ` Lukas Wunner
@ 2022-02-08  6:33           ` Mika Westerberg
  0 siblings, 0 replies; 12+ messages in thread
From: Mika Westerberg @ 2022-02-08  6:33 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Deucher, Alexander, Limonciello, Mario, Bjorn Helgaas,
	Andreas Noever, open list:PCI SUBSYSTEM,
	open list:THUNDERBOLT DRIVER, Michael Jamet, Yehezkel Bernat

Hi,

On Mon, Feb 07, 2022 at 06:47:03PM +0100, Lukas Wunner wrote:
> On Mon, Feb 07, 2022 at 03:52:13PM +0000, Deucher, Alexander wrote:
> > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > The other option is to look for ACPI companion (ACPI_COMPANION()) of the
> > > device. AFAICT dGPUs don't have one (as the BIOS does not know in advance
> > > what will be connected to the hotplug ports) whereas internal does typically
> > > have one.
> > 
> > Yeah, this is probably the right way to do this.
> 
> No, that doesn't work.  At least Apple represents the first few devices
> in the Thunderbolt daisy-chain in the ACPI namespace, so IIUC you'd find
> an ACPI companion for those but not for the remainder of the daisy-chain.
> This is from a 2019/2020 MacBookPro16,1:
> 
> $ grep 'Device ' acpidump/mbp161/ssdt6.dsl
>             Device (UPSB)
>                 Device (DSB0)
>                     Device (NHI0)
>                 Device (DSB1)
>                     Device (UPS0)
>                         Device (DSB0)
>                             Device (DEV0)
>                         Device (DSB3)
>                             Device (UPS0)
>                                 Device (DSB0)
>                                     Device (DEV0)
>                                 Device (DSB3)
>                                     Device (DEV0)
>             ...
> 
> There's a *reason* why I introduced the is_thunderbolt flag,
> there is no other reliable way to detect externally attached devices.

In case of dGPU, the thing (I think) that is needed is whether there is
some sort of power resource attached to the root port that allows
powering it down or so. That can still be done without is_thunderbolt
flag using the current ACPI functions.

[ IMHO we should get rid of that flag completely, or rename it to some
 more generic like "removable" (following USB, perhaps moving to driver
 core) that allows drivers to check if the device is soldered on the
 motherboard or hanging off of some hotpluggable port. ]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-02-08  6:33 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-04 18:28 [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Mario Limonciello
2022-02-04 18:28 ` [PATCH 1/2] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4 Mario Limonciello
2022-02-04 18:28 ` [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt" Mario Limonciello
2022-02-04 22:29   ` Bjorn Helgaas
2022-02-05  9:39     ` Lukas Wunner
2022-02-04 18:36 ` [PATCH 0/2] Mark USB4 controllers as is_thunderbolt Deucher, Alexander
2022-02-07  6:41 ` Mika Westerberg
2022-02-07 15:00   ` Deucher, Alexander
2022-02-07 15:45     ` Mika Westerberg
2022-02-07 15:52       ` Deucher, Alexander
2022-02-07 17:47         ` Lukas Wunner
2022-02-08  6:33           ` Mika Westerberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.