mbox series

[v4,00/10] Overhaul `is_thunderbolt`

Message ID 20220215000200.242799-1-mario.limonciello@amd.com (mailing list archive)
Headers show
Series Overhaul `is_thunderbolt` | expand

Message

Mario Limonciello Feb. 15, 2022, 12:01 a.m. UTC
Various drivers in the kernel use `is_thunderbolt` or
`pci_is_thunderbolt_attached` to designate behaving differently
from a device that is internally in the machine. This relies upon checks
for a specific capability only set on Intel controllers.

Non-Intel USB4 designs should also match this designation so that they
can be treated the same regardless of the host they're connected to.

As part of adding the generic USB4 controller code, it was realized that
`is_thunderbolt` and `pcie_is_thunderbolt_attached` have been overloaded.

Instead migrate to using removable attribute from device core.

Changes from v3->v4:
- Add tags from last review where applicable
- Update titles of different patches
- Add more comments and commit messages to various patches to address
  comments raised in review
- Re-order the patch series, moving more contentious patches later
- Drop patch marking NHI removable
- Drop patch changing gmux on it's own, roll into patch to drop
  `is_thunderbolt`
- Modify patch to mark integrated USB4 tunnel PCIe root ports as
  "external" instead of removable.
- Modify patch to mark discrete USB4 tunnel root ports as "external"
  instead of removable.
- Fix bit mask error in discrete USB4 tunnel patch
- Fix USB IF vendor designation location in pci_ids.h

Changes from v2->v3:
- Add various tags for patches that haven't changed from v2->v3
- Add new patches for Mika's suggestions:
  * Moving Apple Thunderbolt D3 declaration into quirks
  * Detect PCIe root port used for PCIe tunneling on integrated
    controllers using `usb4-host-interface`
  * Detect PCIe root port used for PCIe tunneling on discrete
    controllers using the USB4 DVSEC specification

Changes from v1->v2:
- Add Alex's tag to first patch
- Move lack of command completion into a quirk (Lukas)
- Drop `is_thunderbolt` attribute and `pci_is_thunderbolt_attached` and
  use device core removable attribute instead
- Adjust all consumers of old attribute to use removable

Note: this spans USB/DRM/platform-x86/PCI trees.
As a majority of the changes are in PCI, it should probably come through
that tree if possible.

Mario Limonciello (10):
  PCI: Add USB4 class definition
  PCI: Move `is_thunderbolt` check for lack of command completed to a
    quirk
  PCI: Detect root port of internal USB4 controllers
  PCI: Detect PCIe root ports for discrete USB4 controllers
  PCI: Move check for old Apple Thunderbolt controllers into a quirk
  PCI: Drop the `is_thunderbolt` attribute from PCI core
  drm/amd: drop the use of `pci_is_thunderbolt_attached`
  drm/nouveau: drop the use of `pci_is_thunderbolt_attached`
  drm/radeon: drop the use of `pci_is_thunderbolt_attached`
  PCI: drop `pci_is_thunderbolt_attached`

 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
 drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
 drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
 drivers/pci/pci-acpi.c                  | 15 ++++-
 drivers/pci/pci.c                       | 17 +++--
 drivers/pci/probe.c                     | 52 ++++++++++++++-
 drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
 drivers/platform/x86/apple-gmux.c       |  2 +-
 drivers/thunderbolt/nhi.h               |  2 -
 include/linux/pci.h                     | 25 +-------
 include/linux/pci_ids.h                 |  3 +
 14 files changed, 173 insertions(+), 47 deletions(-)

Comments

Lukas Wunner Feb. 15, 2022, 7:29 a.m. UTC | #1
On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
>  drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
>  drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
>  drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
>  drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
>  drivers/pci/pci-acpi.c                  | 15 ++++-
>  drivers/pci/pci.c                       | 17 +++--
>  drivers/pci/probe.c                     | 52 ++++++++++++++-
>  drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
>  drivers/platform/x86/apple-gmux.c       |  2 +-
>  drivers/thunderbolt/nhi.h               |  2 -
>  include/linux/pci.h                     | 25 +-------
>  include/linux/pci_ids.h                 |  3 +
>  14 files changed, 173 insertions(+), 47 deletions(-)

That's an awful lot of additional LoC for what is primarily
a refactoring job with the intent to simplify things.

Honestly this looks like an attempt to fix something that
isn't broken.  Specifically, the is_thunderbolt bit apparently
can't be removed without adding new bits to struct pci_dev.
Not sure if that can be called progress.

Thanks,

Lukas
Mario Limonciello Feb. 15, 2022, 7:07 p.m. UTC | #2
On 2/15/2022 01:29, Lukas Wunner wrote:
> On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
>>   drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
>>   drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
>>   drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
>>   drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
>>   drivers/pci/pci-acpi.c                  | 15 ++++-
>>   drivers/pci/pci.c                       | 17 +++--
>>   drivers/pci/probe.c                     | 52 ++++++++++++++-
>>   drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
>>   drivers/platform/x86/apple-gmux.c       |  2 +-
>>   drivers/thunderbolt/nhi.h               |  2 -
>>   include/linux/pci.h                     | 25 +-------
>>   include/linux/pci_ids.h                 |  3 +
>>   14 files changed, 173 insertions(+), 47 deletions(-)
> 
> That's an awful lot of additional LoC for what is primarily
> a refactoring job with the intent to simplify things.

You may recall the first version of this series was just for adding
USB4 matches to the existing code paths, and that's when it was noted
that is_thunderbolt is a bit overloaded.

> 
> Honestly this looks like an attempt to fix something that
> isn't broken.  Specifically, the is_thunderbolt bit apparently
> can't be removed without adding new bits to struct pci_dev.
> Not sure if that can be called progress. >
> Thanks,
> 
> Lukas

Within this series there are two new material patches; setting up root 
ports for both integrated and discrete USB4 controllers to behave well 
with all the existing drivers that rely upon a hint of how they're 
connected to configure devices differently.

If y'all collectively prefer this direction to not refactor 
is_thunderbolt and push into quirks, a simpler version of this series 
would be to leave all the quirks in place, just drop 
dev->is_thunderbolt, and set dev->external_facing on all 3 cases:

* Intel TBT controller
* USB4 integrated PCIe tunneling root port/XHCI tunneling root port
* USB4 disctete PCIe tunneling root port/XHCI tunneling root port

All the other drivers and symbols can stay the same then.
Mika Westerberg Feb. 16, 2022, 2:34 p.m. UTC | #3
Hi all,

On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
> On 2/15/2022 01:29, Lukas Wunner wrote:
> > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
> > >   drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
> > >   drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
> > >   drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
> > >   drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
> > >   drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
> > >   drivers/pci/pci-acpi.c                  | 15 ++++-
> > >   drivers/pci/pci.c                       | 17 +++--
> > >   drivers/pci/probe.c                     | 52 ++++++++++++++-
> > >   drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
> > >   drivers/platform/x86/apple-gmux.c       |  2 +-
> > >   drivers/thunderbolt/nhi.h               |  2 -
> > >   include/linux/pci.h                     | 25 +-------
> > >   include/linux/pci_ids.h                 |  3 +
> > >   14 files changed, 173 insertions(+), 47 deletions(-)
> > 
> > That's an awful lot of additional LoC for what is primarily
> > a refactoring job with the intent to simplify things.
> 
> You may recall the first version of this series was just for adding
> USB4 matches to the existing code paths, and that's when it was noted
> that is_thunderbolt is a bit overloaded.
> 
> > 
> > Honestly this looks like an attempt to fix something that
> > isn't broken.  Specifically, the is_thunderbolt bit apparently
> > can't be removed without adding new bits to struct pci_dev.
> > Not sure if that can be called progress. >
> > Thanks,
> > 
> > Lukas
> 
> Within this series there are two new material patches; setting up root ports
> for both integrated and discrete USB4 controllers to behave well with all
> the existing drivers that rely upon a hint of how they're connected to
> configure devices differently.
> 
> If y'all collectively prefer this direction to not refactor is_thunderbolt
> and push into quirks, a simpler version of this series would be to leave all
> the quirks in place, just drop dev->is_thunderbolt, and set
> dev->external_facing on all 3 cases:
> 
> * Intel TBT controller
> * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
> * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
> 
> All the other drivers and symbols can stay the same then.

If I understand correctly the original intention of this patch series is
to be able to differentiate whether the device is "permanently"
connected to the motherboard, or it is connected over some hot-pluggable
bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
fit into this picture too). Specifically this is needed for discrete
GPUs because of power management differences or so (please correct me if
I'm mistaken).

If we set the is_thunderbolt debate aside and concentrate on that issue,
I think the way to do this is to check whether the root port the GPU is
connected to has an ACPI power resource (returned from _PR3() method).
IF it is present then most likely the platform has provided all the
necessary wiring to move the GPU into D3cold (and the BIOS knows this).
If it is not present then the device cannot even go into D3cold as there
is not means to power of the device in PCIe spec.

Perhaps we can simply use pci_pr3_present() here as nouveau is already
doing? Granted it is not too elegant solution either but better than
using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
perhaps we can have a helper in DRM core that handles this.

Then going back to is_thunderbolt debate :) I really don't think the
drivers should care whether they are connected over a tunnel or not.
They should work regardless of the underlying transport of the native
protocol. They should also be prepared for the fact that the hardware
can vanish under them at any point (e.g user unplugs the device). For
this reason I don't really like to see is_thunderbolt to be used more
and prefer to get rid if it completely if possible at all. If there is
still need to differentiate whether the device can be hot-removed or
not, I think "removable" in the driver core is the way to go. That is
not dependent on any single transport.
Alex Deucher Feb. 16, 2022, 2:44 p.m. UTC | #4
On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi all,
>
> On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
> > On 2/15/2022 01:29, Lukas Wunner wrote:
> > > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
> > > >   drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
> > > >   drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
> > > >   drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
> > > >   drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
> > > >   drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
> > > >   drivers/pci/pci-acpi.c                  | 15 ++++-
> > > >   drivers/pci/pci.c                       | 17 +++--
> > > >   drivers/pci/probe.c                     | 52 ++++++++++++++-
> > > >   drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
> > > >   drivers/platform/x86/apple-gmux.c       |  2 +-
> > > >   drivers/thunderbolt/nhi.h               |  2 -
> > > >   include/linux/pci.h                     | 25 +-------
> > > >   include/linux/pci_ids.h                 |  3 +
> > > >   14 files changed, 173 insertions(+), 47 deletions(-)
> > >
> > > That's an awful lot of additional LoC for what is primarily
> > > a refactoring job with the intent to simplify things.
> >
> > You may recall the first version of this series was just for adding
> > USB4 matches to the existing code paths, and that's when it was noted
> > that is_thunderbolt is a bit overloaded.
> >
> > >
> > > Honestly this looks like an attempt to fix something that
> > > isn't broken.  Specifically, the is_thunderbolt bit apparently
> > > can't be removed without adding new bits to struct pci_dev.
> > > Not sure if that can be called progress. >
> > > Thanks,
> > >
> > > Lukas
> >
> > Within this series there are two new material patches; setting up root ports
> > for both integrated and discrete USB4 controllers to behave well with all
> > the existing drivers that rely upon a hint of how they're connected to
> > configure devices differently.
> >
> > If y'all collectively prefer this direction to not refactor is_thunderbolt
> > and push into quirks, a simpler version of this series would be to leave all
> > the quirks in place, just drop dev->is_thunderbolt, and set
> > dev->external_facing on all 3 cases:
> >
> > * Intel TBT controller
> > * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
> > * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
> >
> > All the other drivers and symbols can stay the same then.
>
> If I understand correctly the original intention of this patch series is
> to be able to differentiate whether the device is "permanently"
> connected to the motherboard, or it is connected over some hot-pluggable
> bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
> fit into this picture too). Specifically this is needed for discrete
> GPUs because of power management differences or so (please correct me if
> I'm mistaken).
>
> If we set the is_thunderbolt debate aside and concentrate on that issue,
> I think the way to do this is to check whether the root port the GPU is
> connected to has an ACPI power resource (returned from _PR3() method).
> IF it is present then most likely the platform has provided all the
> necessary wiring to move the GPU into D3cold (and the BIOS knows this).
> If it is not present then the device cannot even go into D3cold as there
> is not means to power of the device in PCIe spec.
>
> Perhaps we can simply use pci_pr3_present() here as nouveau is already
> doing? Granted it is not too elegant solution either but better than
> using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
> perhaps we can have a helper in DRM core that handles this.

The tricky part is that there were AMD and NVIDIA specific proprietary
_PR3-like ACPI methods (plus whatever Apple did) prior to GPU power
control standardizing on _PR3.  Currently those methods are handled in
the drivers directly, sort of tangled up with vga_switcheroo.  I think
ideally that logic would move to the ACPI core and be handled the same
way as _PR3, but I'm not sure how well that would work because of the
various bios date checks around _PR3 and the lack of general _PR3
support in those older platforms.  So I think we still need some sort
of "is this soldered in" check.

Alex


>
> Then going back to is_thunderbolt debate :) I really don't think the
> drivers should care whether they are connected over a tunnel or not.
> They should work regardless of the underlying transport of the native
> protocol. They should also be prepared for the fact that the hardware
> can vanish under them at any point (e.g user unplugs the device). For
> this reason I don't really like to see is_thunderbolt to be used more
> and prefer to get rid if it completely if possible at all. If there is
> still need to differentiate whether the device can be hot-removed or
> not, I think "removable" in the driver core is the way to go. That is
> not dependent on any single transport.
Mario Limonciello Feb. 16, 2022, 4:50 p.m. UTC | #5
On 2/16/2022 08:44, Alex Deucher wrote:
> On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>>
>> Hi all,
>>
>> On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
>>> On 2/15/2022 01:29, Lukas Wunner wrote:
>>>> On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
>>>>>    drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
>>>>>    drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
>>>>>    drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
>>>>>    drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
>>>>>    drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
>>>>>    drivers/pci/pci-acpi.c                  | 15 ++++-
>>>>>    drivers/pci/pci.c                       | 17 +++--
>>>>>    drivers/pci/probe.c                     | 52 ++++++++++++++-
>>>>>    drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
>>>>>    drivers/platform/x86/apple-gmux.c       |  2 +-
>>>>>    drivers/thunderbolt/nhi.h               |  2 -
>>>>>    include/linux/pci.h                     | 25 +-------
>>>>>    include/linux/pci_ids.h                 |  3 +
>>>>>    14 files changed, 173 insertions(+), 47 deletions(-)
>>>>
>>>> That's an awful lot of additional LoC for what is primarily
>>>> a refactoring job with the intent to simplify things.
>>>
>>> You may recall the first version of this series was just for adding
>>> USB4 matches to the existing code paths, and that's when it was noted
>>> that is_thunderbolt is a bit overloaded.
>>>
>>>>
>>>> Honestly this looks like an attempt to fix something that
>>>> isn't broken.  Specifically, the is_thunderbolt bit apparently
>>>> can't be removed without adding new bits to struct pci_dev.
>>>> Not sure if that can be called progress. >
>>>> Thanks,
>>>>
>>>> Lukas
>>>
>>> Within this series there are two new material patches; setting up root ports
>>> for both integrated and discrete USB4 controllers to behave well with all
>>> the existing drivers that rely upon a hint of how they're connected to
>>> configure devices differently.
>>>
>>> If y'all collectively prefer this direction to not refactor is_thunderbolt
>>> and push into quirks, a simpler version of this series would be to leave all
>>> the quirks in place, just drop dev->is_thunderbolt, and set
>>> dev->external_facing on all 3 cases:
>>>
>>> * Intel TBT controller
>>> * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
>>> * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
>>>
>>> All the other drivers and symbols can stay the same then.
>>
>> If I understand correctly the original intention of this patch series is
>> to be able to differentiate whether the device is "permanently"
>> connected to the motherboard, or it is connected over some hot-pluggable
>> bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
>> fit into this picture too). Specifically this is needed for discrete
>> GPUs because of power management differences or so (please correct me if
>> I'm mistaken).

Correct.  It might be possible to drop the patch for the integrated case 
(patch 3) because I do think that by Microsoft having the _DSD for 
"ExternalFacingPort" it's very likely that most implementations will 
have used it for the appropriate PCIe root ports.  If something shows up 
in the wild that this isn't the case it could be revisited.  If it's 
found pre-production presumably the OEM can still fix it and if it's 
post production and there are problems we can dust it off then.

The discrete USB4 controller I would be more concerned that this isn't 
populated, and that (patch 4) should be more important to let the driver 
core set it removable.

>>
>> If we set the is_thunderbolt debate aside and concentrate on that issue,
>> I think the way to do this is to check whether the root port the GPU is
>> connected to has an ACPI power resource (returned from _PR3() method).
>> IF it is present then most likely the platform has provided all the
>> necessary wiring to move the GPU into D3cold (and the BIOS knows this).
>> If it is not present then the device cannot even go into D3cold as there
>> is not means to power of the device in PCIe spec.
>>
>> Perhaps we can simply use pci_pr3_present() here as nouveau is already
>> doing? Granted it is not too elegant solution either but better than
>> using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
>> perhaps we can have a helper in DRM core that handles this.
> 
> The tricky part is that there were AMD and NVIDIA specific proprietary
> _PR3-like ACPI methods (plus whatever Apple did) prior to GPU power
> control standardizing on _PR3.  Currently those methods are handled in
> the drivers directly, sort of tangled up with vga_switcheroo.  I think
> ideally that logic would move to the ACPI core and be handled the same
> way as _PR3, but I'm not sure how well that would work because of the
> various bios date checks around _PR3 and the lack of general _PR3
> support in those older platforms.  So I think we still need some sort
> of "is this soldered in" check.

Considering that limitation if `dev->external_facing` already exists in 
PCI core may as well use it for this instead of `is_thunderbolt`.

> 
> Alex
> 
> 
>>
>> Then going back to is_thunderbolt debate :) I really don't think the
>> drivers should care whether they are connected over a tunnel or not.
>> They should work regardless of the underlying transport of the native
>> protocol. They should also be prepared for the fact that the hardware
>> can vanish under them at any point (e.g user unplugs the device). For
>> this reason I don't really like to see is_thunderbolt to be used more
>> and prefer to get rid if it completely if possible at all. If there is
>> still need to differentiate whether the device can be hot-removed or
>> not, I think "removable" in the driver core is the way to go. That is
>> not dependent on any single transport.

Hopefully that is what the patch series does right now as of v4. As I

If we really don't want another pci_device attribute we can probably 
invent another device property for the quirked things in patch 2 instead 
of "no_cmd_complete".
Mika Westerberg Feb. 17, 2022, 9:33 a.m. UTC | #6
Hi Mario,

On Wed, Feb 16, 2022 at 10:50:31AM -0600, Limonciello, Mario wrote:
> On 2/16/2022 08:44, Alex Deucher wrote:
> > On Wed, Feb 16, 2022 at 9:34 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > 
> > > Hi all,
> > > 
> > > On Tue, Feb 15, 2022 at 01:07:00PM -0600, Limonciello, Mario wrote:
> > > > On 2/15/2022 01:29, Lukas Wunner wrote:
> > > > > On Mon, Feb 14, 2022 at 06:01:50PM -0600, Mario Limonciello wrote:
> > > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
> > > > > >    drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c  |  2 +-
> > > > > >    drivers/gpu/drm/nouveau/nouveau_vga.c   |  4 +-
> > > > > >    drivers/gpu/drm/radeon/radeon_device.c  |  4 +-
> > > > > >    drivers/gpu/drm/radeon/radeon_kms.c     |  2 +-
> > > > > >    drivers/pci/hotplug/pciehp_hpc.c        |  6 +-
> > > > > >    drivers/pci/pci-acpi.c                  | 15 ++++-
> > > > > >    drivers/pci/pci.c                       | 17 +++--
> > > > > >    drivers/pci/probe.c                     | 52 ++++++++++++++-
> > > > > >    drivers/pci/quirks.c                    | 84 +++++++++++++++++++++++++
> > > > > >    drivers/platform/x86/apple-gmux.c       |  2 +-
> > > > > >    drivers/thunderbolt/nhi.h               |  2 -
> > > > > >    include/linux/pci.h                     | 25 +-------
> > > > > >    include/linux/pci_ids.h                 |  3 +
> > > > > >    14 files changed, 173 insertions(+), 47 deletions(-)
> > > > > 
> > > > > That's an awful lot of additional LoC for what is primarily
> > > > > a refactoring job with the intent to simplify things.
> > > > 
> > > > You may recall the first version of this series was just for adding
> > > > USB4 matches to the existing code paths, and that's when it was noted
> > > > that is_thunderbolt is a bit overloaded.
> > > > 
> > > > > 
> > > > > Honestly this looks like an attempt to fix something that
> > > > > isn't broken.  Specifically, the is_thunderbolt bit apparently
> > > > > can't be removed without adding new bits to struct pci_dev.
> > > > > Not sure if that can be called progress. >
> > > > > Thanks,
> > > > > 
> > > > > Lukas
> > > > 
> > > > Within this series there are two new material patches; setting up root ports
> > > > for both integrated and discrete USB4 controllers to behave well with all
> > > > the existing drivers that rely upon a hint of how they're connected to
> > > > configure devices differently.
> > > > 
> > > > If y'all collectively prefer this direction to not refactor is_thunderbolt
> > > > and push into quirks, a simpler version of this series would be to leave all
> > > > the quirks in place, just drop dev->is_thunderbolt, and set
> > > > dev->external_facing on all 3 cases:
> > > > 
> > > > * Intel TBT controller
> > > > * USB4 integrated PCIe tunneling root port/XHCI tunneling root port
> > > > * USB4 disctete PCIe tunneling root port/XHCI tunneling root port
> > > > 
> > > > All the other drivers and symbols can stay the same then.
> > > 
> > > If I understand correctly the original intention of this patch series is
> > > to be able to differentiate whether the device is "permanently"
> > > connected to the motherboard, or it is connected over some hot-pluggable
> > > bus (PCIe, USB, USB4 for example but I'm sure there are other buses that
> > > fit into this picture too). Specifically this is needed for discrete
> > > GPUs because of power management differences or so (please correct me if
> > > I'm mistaken).
> 
> Correct.  It might be possible to drop the patch for the integrated case
> (patch 3) because I do think that by Microsoft having the _DSD for
> "ExternalFacingPort" it's very likely that most implementations will have
> used it for the appropriate PCIe root ports.  If something shows up in the
> wild that this isn't the case it could be revisited.  If it's found
> pre-production presumably the OEM can still fix it and if it's post
> production and there are problems we can dust it off then.

Yeah, that's most likely the case.

> The discrete USB4 controller I would be more concerned that this isn't
> populated, and that (patch 4) should be more important to let the driver
> core set it removable.

Agreed.

[I actually only now noticed that the PCI core actually already marks
 devices connected to external facing ports as "removable" in
 pci_set_removable().]

> > > If we set the is_thunderbolt debate aside and concentrate on that issue,
> > > I think the way to do this is to check whether the root port the GPU is
> > > connected to has an ACPI power resource (returned from _PR3() method).
> > > IF it is present then most likely the platform has provided all the
> > > necessary wiring to move the GPU into D3cold (and the BIOS knows this).
> > > If it is not present then the device cannot even go into D3cold as there
> > > is not means to power of the device in PCIe spec.
> > > 
> > > Perhaps we can simply use pci_pr3_present() here as nouveau is already
> > > doing? Granted it is not too elegant solution either but better than
> > > using is_thunderbolt IMHO. Since this seem to be common for many GPUs,
> > > perhaps we can have a helper in DRM core that handles this.
> > 
> > The tricky part is that there were AMD and NVIDIA specific proprietary
> > _PR3-like ACPI methods (plus whatever Apple did) prior to GPU power
> > control standardizing on _PR3.  Currently those methods are handled in
> > the drivers directly, sort of tangled up with vga_switcheroo.  I think
> > ideally that logic would move to the ACPI core and be handled the same
> > way as _PR3, but I'm not sure how well that would work because of the
> > various bios date checks around _PR3 and the lack of general _PR3
> > support in those older platforms.  So I think we still need some sort
> > of "is this soldered in" check.
> 
> Considering that limitation if `dev->external_facing` already exists in PCI
> core may as well use it for this instead of `is_thunderbolt`.

Indeed.

> > Alex
> > 
> > 
> > > 
> > > Then going back to is_thunderbolt debate :) I really don't think the
> > > drivers should care whether they are connected over a tunnel or not.
> > > They should work regardless of the underlying transport of the native
> > > protocol. They should also be prepared for the fact that the hardware
> > > can vanish under them at any point (e.g user unplugs the device). For
> > > this reason I don't really like to see is_thunderbolt to be used more
> > > and prefer to get rid if it completely if possible at all. If there is
> > > still need to differentiate whether the device can be hot-removed or
> > > not, I think "removable" in the driver core is the way to go. That is
> > > not dependent on any single transport.
> 
> Hopefully that is what the patch series does right now as of v4. As I

It does yes. I think the detection of internal and discrete tunneled
ports can be dropped from this series for now to make this leaner. We
can add those later when needed.