Nouveau - Feb 2022 - [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 at 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?

[Public]
> -----Original Message-----
> From: Mika Westerberg <mika.westerberg at linux.intel.com>
> Sent: Friday, February 11, 2022 04:24
> To: Limonciello, Mario <Mario.Limonciello at amd.com>
> Cc: Bjorn Helgaas <bhelgaas at google.com>; Andreas Noever
> <andreas.noever at gmail.com>; open list:PCI SUBSYSTEM <linux-
> pci at vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux-
> usb at vger.kernel.org>; open list:RADEON and AMDGPU DRM DRIVERS
<amd-
> gfx at lists.freedesktop.org>; open list:DRM DRIVERS <dri-
> devel at lists.freedesktop.org>; open list:DRM DRIVER FOR NVIDIA
> GEFORCE/QUADRO GPUS <nouveau at lists.freedesktop.org>; open list:X86
> PLATFORM DRIVERS <platform-driver-x86 at vger.kernel.org>; Michael
Jamet
> <michael.jamet at intel.com>; Yehezkel Bernat <YehezkelShB at
gmail.com>;
> Lukas Wunner <lukas at wunner.de>; Deucher, Alexander
> <Alexander.Deucher at 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 at 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.

On Fri, Feb 11, 2022 at 12:23:51PM +0200, Mika Westerberg
wrote:
> On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote:
> > @@ -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.
[...]
> and then make a quirk in quirks.c that adds the software node property
> for the Apple systems? Or something along those lines.
Honestly, that feels wrong to me.

There are non-Apple products with Thunderbolt controllers,
e.g. Supermicro X10SAT was a Xeon board with Redwood Ridge
which was introduced in 2013.  This was way before Microsoft
came up with the HotPlugSupportInD3 property.  It was also way
before the 2015 BIOS cut-off date that we use to disable
power management on older boards.

Still, we currently whitelist the Thunderbolt ports on that
board for D3 because we know it works.  What if products like
this one use their own power management scheme and we'd cause
a power regression if we needlessly disable D3 for them now?

Thanks,

Lukas