Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
Hi, On Mon, Feb 28, 2022 at 10:36:59PM +, Limonciello, Mario wrote: > [AMD Official Use Only] > > > -Original Message- > > From: Lukas Wunner > > Sent: Monday, February 28, 2022 16:33 > > To: Bjorn Helgaas > > Cc: Limonciello, Mario ; Mika Westerberg > > ; Michael Jamet > > ; open list:PCI SUBSYSTEM > p...@vger.kernel.org>; open list:THUNDERBOLT DRIVER > u...@vger.kernel.org>; Yehezkel Bernat ; open > > list:DRM DRIVERS ; open list:X86 > > PLATFORM DRIVERS ; Andreas > > Noever ; open list:RADEON and AMDGPU > > DRM DRIVERS ; open list:DRM DRIVER FOR > > NVIDIA GEFORCE/QUADRO GPUS ; Bjorn > > Helgaas ; Deucher, Alexander > > > > Subject: Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from > > PCI > > core > > > > On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote: > > > On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > > > > facing" > > > > > > assumption above. Not having a Thunderbolt spec, I have no idea > > how > > > > > > you deal with that. > > > > > > > > > > You can download the spec here: > > [...] > > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > > > > Version 1.0.pdf". > > > > > > > > The spec has Host_Router_indication (bits 18-19) as meaning external > > facing. > > > > I'll respin the patch 3 for using that. > > > > > > Thanks, please include the spec citation when you do. And probably > > > the URL, because it's not at all obvious how the casual reader would > > > get from "is_thunderbolt" to a recent add-on to the USB4 spec. > > > > PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec, > > hence there's no connection between "is_thunderbolt" and the USB4 spec. > > > > It's a proprietary VSEC used by Intel and the only way to recognize > > pre-USB4 Thunderbolt devices that I know of. Its ID is also > > different from the DVSEC IDs given in the above-mentioned spec. > > > > Thanks, > > The USB4 DVSEC spec makes comments about DVSEC_ID of 0x8086 and also > DVSEC VENDOR_ID of 0x8086. Is that not also present on the Intel TBT3 > controllers? > > My interpretation of this (and Mika's comment) was that rather than > looking at the Intel VSEC we should look at the USB4 DVSEC to detect > the Intel TBT3 controllers. For pre-USB4 controllers (TBT 1-3) we need to use the existing method (or a quirk based on device ID) as they don't have the USB4 DVSEC.
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote: > On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > > facing" > > > > assumption above. Not having a Thunderbolt spec, I have no idea how > > > > you deal with that. > > > > > > You can download the spec here: [...] > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > > Version 1.0.pdf". > > > > The spec has Host_Router_indication (bits 18-19) as meaning external facing. > > I'll respin the patch 3 for using that. > > Thanks, please include the spec citation when you do. And probably > the URL, because it's not at all obvious how the casual reader would > get from "is_thunderbolt" to a recent add-on to the USB4 spec. PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec, hence there's no connection between "is_thunderbolt" and the USB4 spec. It's a proprietary VSEC used by Intel and the only way to recognize pre-USB4 Thunderbolt devices that I know of. Its ID is also different from the DVSEC IDs given in the above-mentioned spec. Thanks, Lukas
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > [AMD Official Use Only] > > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > facing" > > > assumption above. Not having a Thunderbolt spec, I have no idea how > > > you deal with that. > > > > You can download the spec here: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > > w.usb.org%2Fsites%2Fdefault%2Ffiles%2FUSB4%2520Specification%2520202 > > 6.zipdata=04%7C01%7Cmario.limonciello%40amd.com%7Ca26e64 > > 7a4acf4e7681d308d9faa358fd%7C3dd8961fe4884e608e11a82d994e183d%7C0 > > %7C0%7C637816402472428689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC > > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000& > > amp;sdata=HSDqx%2BHzSnczTZxaBij8sgqvJSS8ajtjCzZd2CPSbR4%3Dre > > served=0 > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > Version 1.0.pdf". > > The spec has Host_Router_indication (bits 18-19) as meaning external facing. > I'll respin the patch 3 for using that. Thanks, please include the spec citation when you do. And probably the URL, because it's not at all obvious how the casual reader would get from "is_thunderbolt" to a recent add-on to the USB4 spec. Bjorn
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
Hi Bjorn, On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-facing" > assumption above. Not having a Thunderbolt spec, I have no idea how > you deal with that. You can download the spec here: https://www.usb.org/sites/default/files/USB4%20Specification%202026.zip Inside the archive there is also the DVSEC spec with name "USB4 DVSEC Version 1.0.pdf".
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` attribute originally had a well defined list of
> quirks that it existed for, but it has been overloaded with more
> meaning.
>
> Instead use the driver core removable attribute to indicate the
> detail a device is attached to a thunderbolt or USB4 chain.
>
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/probe.c | 2 +-
> drivers/platform/x86/apple-gmux.c | 2 +-
> include/linux/pci.h | 5 ++---
> 3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..1b752d425c47 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1584,7 +1584,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> /* Is the device part of a Thunderbolt controller? */
> vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
> PCI_VSEC_ID_INTEL_TBT);
> if (vsec)
> - dev->is_thunderbolt = 1;
> + dev->external_facing = true;
> }
>
> static void set_pcie_untrusted(struct pci_dev *dev)
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 57553f9b4d1d..da0c39b0 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
>
> static int is_thunderbolt(struct device *dev, void *data)
> {
> - return to_pci_dev(dev)->is_thunderbolt;
> + return to_pci_dev(dev)->external_facing;
> }
>
> static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1e5b769e42fc..d9719eb14654 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -442,7 +442,6 @@ struct pci_dev {
> unsigned intis_virtfn:1;
> unsigned intis_hotplug_bridge:1;
> unsigned intshpc_managed:1; /* SHPC owned by shpchp */
> - unsigned intis_thunderbolt:1; /* Thunderbolt controller */
> unsigned intno_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(>dev))
> return true;
>
> while ((parent = pci_upstream_bridge(parent)))
> - if (parent->is_thunderbolt)
> + if (dev_is_removable(>dev))
> return true;
>
> return false;
Since you remove this function entirely later, it seems like you might
as well push this to the end of the series, so you won't have to
change it before removing it.
That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-facing"
assumption above. Not having a Thunderbolt spec, I have no idea how
you deal with that.
But it is definitely not the case that "dev_is_removable() implies
device is Thunderbolt", so I don't think this last hunk can work.
Bjorn
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Thu, Feb 24, 2022 at 8:23 PM Bjorn Helgaas wrote:
>
> On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote:
> > The `is_thunderbolt` attribute originally had a well defined list of
> > quirks that it existed for, but it has been overloaded with more
> > meaning.
> >
> > Instead use the driver core removable attribute to indicate the
> > detail a device is attached to a thunderbolt or USB4 chain.
> >
> > Signed-off-by: Mario Limonciello
> > ---
> > drivers/pci/probe.c | 2 +-
> > drivers/platform/x86/apple-gmux.c | 2 +-
> > include/linux/pci.h | 5 ++---
> > 3 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 17a969942d37..1b752d425c47 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1584,7 +1584,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> > /* Is the device part of a Thunderbolt controller? */
> > vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
> > PCI_VSEC_ID_INTEL_TBT);
> > if (vsec)
> > - dev->is_thunderbolt = 1;
> > + dev->external_facing = true;
>
> I assume there's a spec for the PCI_VSEC_ID_INTEL_TBT Capability. Is
> that public? Does the spec say that a device with that capability
> must be external-facing?
>
> Even if it's not public, I think a citation (name, revision, section)
> would be useful.
>
> > }
> >
> > static void set_pcie_untrusted(struct pci_dev *dev)
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c
> > index 57553f9b4d1d..da0c39b0 100644
> > --- a/drivers/platform/x86/apple-gmux.c
> > +++ b/drivers/platform/x86/apple-gmux.c
> > @@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
> >
> > static int is_thunderbolt(struct device *dev, void *data)
> > {
> > - return to_pci_dev(dev)->is_thunderbolt;
> > + return to_pci_dev(dev)->external_facing;
>
> This looks ... sort of weird. I don't know anything about
> apple-gmux.c, so I guess I don't care, but assuming any
> external-facing device must be a Thunderbolt device seems like a
> stretch.
>
> Ugh. This is used via "bus_for_each_dev(_bus_type)", which means
> it's not hotplug-safe. I'm sure we "know" implicitly that hotplug
> isn't an issue in apple-gmux, but it's better not to have examples
> that get copied to places where it *is* an issue.
>
> > }
> >
> > 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 intis_virtfn:1;
> > unsigned intis_hotplug_bridge:1;
> > unsigned intshpc_managed:1; /* SHPC owned by shpchp */
> > - unsigned intis_thunderbolt:1; /* Thunderbolt controller */
> > unsigned intno_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(>dev))
> > return true;
> >
> > while ((parent = pci_upstream_bridge(parent)))
> > - if (parent->is_thunderbolt)
> > + if (dev_is_removable(>dev))
> > return true;
>
> I don't get this. Plain old PCI devices can be removable, too.
>
> pci_is_thunderbolt_attached() is only used by GPU drivers. What
> property of Thunderbolt do they care about?
>
> nouveau_vga_init() and radeon_device_init() use it to decide to
> register with vga_switcheroo. So maybe that's something to do with
> removability? Of course, that's not specific to Thunderbolt, because
> garden-variety PCIe devices are removable.
>
> amdgpu_driver_load_kms() and radeon_driver_load_kms() apparently use
> it for something related to power control. I don't know what the
> Thunderbolt connection is.
For GPU drivers, we need to determine which dGPU on the system has
d3cold control via ACPI and which GPU would use a mux for display
switching between the iGPU and the dGPU for hybrid graphics platforms
(e.g., iGPU + dGPU built into a laptop or all-in-one PC). The dGPU
built into the platform would be the one we want to use for mux
switching and ACPI power control. You would not want that for the
dGPU connected via thunderbolt (or some other hot pluggable
interface). I had suggested that we could check if there is an ACPI
device associated with the dGPU and use that to determine this, but I
think someone brought up a case where that didn't work. We need to
know whether the dGPU uses platform power control to determine whether
the driver should let the platform manage the power state via ACPI or
if the driver should do it (e.g., for
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Thu, Feb 24, 2022 at 07:23:49PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote: > > The `is_thunderbolt` attribute originally had a well defined list of > > quirks that it existed for, but it has been overloaded with more > > meaning. > > > > Instead use the driver core removable attribute to indicate the > > detail a device is attached to a thunderbolt or USB4 chain. > ... > If these things are not specifically related to Thunderbolt, I'd > prefer to get rid of pci_is_thunderbolt_attached() and see if we can > help the GPU folks figure out what they really need. Ah. Guess I should read the whole series before commenting :) I see that you *did* remove pci_is_thunderbolt_attached() in the last patch. I'll look more at the rest tomorrow. Bjorn
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` attribute originally had a well defined list of
> quirks that it existed for, but it has been overloaded with more
> meaning.
>
> Instead use the driver core removable attribute to indicate the
> detail a device is attached to a thunderbolt or USB4 chain.
>
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/probe.c | 2 +-
> drivers/platform/x86/apple-gmux.c | 2 +-
> include/linux/pci.h | 5 ++---
> 3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..1b752d425c47 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1584,7 +1584,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> /* Is the device part of a Thunderbolt controller? */
> vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
> PCI_VSEC_ID_INTEL_TBT);
> if (vsec)
> - dev->is_thunderbolt = 1;
> + dev->external_facing = true;
I assume there's a spec for the PCI_VSEC_ID_INTEL_TBT Capability. Is
that public? Does the spec say that a device with that capability
must be external-facing?
Even if it's not public, I think a citation (name, revision, section)
would be useful.
> }
>
> static void set_pcie_untrusted(struct pci_dev *dev)
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 57553f9b4d1d..da0c39b0 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
>
> static int is_thunderbolt(struct device *dev, void *data)
> {
> - return to_pci_dev(dev)->is_thunderbolt;
> + return to_pci_dev(dev)->external_facing;
This looks ... sort of weird. I don't know anything about
apple-gmux.c, so I guess I don't care, but assuming any
external-facing device must be a Thunderbolt device seems like a
stretch.
Ugh. This is used via "bus_for_each_dev(_bus_type)", which means
it's not hotplug-safe. I'm sure we "know" implicitly that hotplug
isn't an issue in apple-gmux, but it's better not to have examples
that get copied to places where it *is* an issue.
> }
>
> 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 intis_virtfn:1;
> unsigned intis_hotplug_bridge:1;
> unsigned intshpc_managed:1; /* SHPC owned by shpchp */
> - unsigned intis_thunderbolt:1; /* Thunderbolt controller */
> unsigned intno_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(>dev))
> return true;
>
> while ((parent = pci_upstream_bridge(parent)))
> - if (parent->is_thunderbolt)
> + if (dev_is_removable(>dev))
> return true;
I don't get this. Plain old PCI devices can be removable, too.
pci_is_thunderbolt_attached() is only used by GPU drivers. What
property of Thunderbolt do they care about?
nouveau_vga_init() and radeon_device_init() use it to decide to
register with vga_switcheroo. So maybe that's something to do with
removability? Of course, that's not specific to Thunderbolt, because
garden-variety PCIe devices are removable.
amdgpu_driver_load_kms() and radeon_driver_load_kms() apparently use
it for something related to power control. I don't know what the
Thunderbolt connection is.
nbio_v2_3_enable_aspm() looks like it uses it to change some ASPM
parameters. Seems like potentially a device erratum or quirk
material?
If these things are not specifically related to Thunderbolt, I'd
prefer to get rid of pci_is_thunderbolt_attached() and see if we can
help the GPU folks figure out what they really need.
> return false;
> --
> 2.34.1
>
