RE: [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute

Limonciello, Mario
2022-02-11T19:37:46Z

[Public]

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Friday, February 11, 2022 04:24
> 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>; open list:RADEON and AMDGPU DRM DRIVERS <amd-
> gfx@lists.freedesktop.org>; open list:DRM DRIVERS <dri-
> devel@lists.freedesktop.org>; open list:DRM DRIVER FOR NVIDIA
> GEFORCE/QUADRO GPUS <nouveau@lists.freedesktop.org>; open list:X86
> PLATFORM DRIVERS <platform-driver-x86@vger.kernel.org>; Michael Jamet
> <michael.jamet@intel.com>; Yehezkel Bernat <YehezkelShB@gmail.com>;
> Lukas Wunner <lukas@wunner.de>; Deucher, Alexander
> <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute
>
> Hi Mario,
>
> On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote:
> > The `is_thunderbolt` attribute is currently a dumping ground for a
> > variety of things.
> >
> > Instead use the driver core removable attribute to indicate the
> > detail a device is attached to a thunderbolt or USB4 chain.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> > drivers/pci/pci.c | 2 +-
> > drivers/pci/probe.c | 20 +++++++-------------
> > drivers/platform/x86/apple-gmux.c | 2 +-
> > include/linux/pci.h | 5 ++---
> > 4 files changed, 11 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 9ecce435fb3f..1264984d5e6d 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2955,7 +2955,7 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> > return true;
> >
> > /* Even the oldest 2010 Thunderbolt controller supports D3. */
> > - if (bridge->is_thunderbolt)
> > + if (dev_is_removable(&bridge->dev))
>
> For this, I'm not entirely sure this is what we want. The purpose of
> this check is to enable port power management of Apple systems with
> Intel Thunderbolt controller and therefore checking for "removable" here
> is kind of misleading IMHO.
>
> I wonder if we could instead remove the check completely here and rely
> on the below:
>
> if (platform_pci_bridge_d3(bridge))
> return true;
>
> and that would then look like:
>
> static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> {
> if (pci_use_mid_pm())
> return false;
>
> if (acpi_pci_bridge_d3(dev))
> return true;
>
> if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3"))
> return true;
>
> return false;
> }
>
> and then make a quirk in quirks.c that adds the software node property
> for the Apple systems? Or something along those lines.
>
> @Lukas, what do you think?

I took a stab at doing this for V3, but I'm unsure whether ALL of the TBT controllers
in pci_ids.h have been used in Apple laptops, so it might be a bit wasteful of a quirk
list. If there is a known list somewhere that is shorter than that, it may be possible
to pare down. Lukas, if you can please look closely at patch 3 of v3.