Re: [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Mario Limonciello <mario.limonciello@amd.com>,
	Bjorn Helgaas <bhelgaas@google.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>,
	Andreas Noever <andreas.noever@gmail.com>,
	Michael Jamet <michael.jamet@intel.com>,
	Yehezkel Bernat <YehezkelShB@gmail.com>,
	Alexander.Deucher@amd.com, Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
Date: Mon, 14 Feb 2022 09:23:22 +0200	[thread overview]
Message-ID: <YgoDauh7rd18OvrM@lahna> (raw)
In-Reply-To: <20220213091920.GA15535@wunner.de>

Hi Lukas,

On Sun, Feb 13, 2022 at 10:19:20AM +0100, Lukas Wunner wrote:
> On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
> > `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
> > controller to indicate that D3 is possible.  As this is used solely
> > for older Apple systems, move it into a quirk that enumerates across
> > all Intel TBT controllers.
> 
> I'm not so sure if it is only needed on Apple systems.
> 
> 
> > @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >  		if (pci_bridge_d3_force)
> >  			return true;
> >  
> > -		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> > -		if (bridge->is_thunderbolt)
> > -			return true;
> > -
> >  		/* Platform might know better if the bridge supports D3 */
> >  		if (platform_pci_bridge_d3(bridge))
> >  			return true;
> 
> The fact that Thunderbolt PCIe ports support D3 is a property of those
> devices.  It's not a property of the platform or a quirk of a particular
> vendor.

It is in fact platform specific. For instance the non-Apple systems
without "HotPlugSupportInD3" property have not been wired to support
entering/exiting D3 runtime so putting these ports into D3 may actually
lead into problems as we are in non-validated territory.

> Hence in my view the current location of the check (pci_bridge_d3_possible())
> makes sense wheras the location you're moving it to does not.
> 
> 
> > +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
> > + * it in the ACPI tables
> > + */
> 
> Apple started shipping Thunderbolt in 2011.
> Intel brought the first chips to market in 2010.
> 
> The date is meaningful at the code's current location in
> pci_bridge_d3_possible() because a few lines further down
> there's a 2015 BIOS cut-off date.
> 
> Microsoft came up with an ACPI property that BIOS vendors may set
> so that Windows knows it may put a Thunderbolt controller into D3cold.
> I'm not even sure if that property was ever officially adopted by the
> ACPI spec or if it's just a Microsoft-defined "standard".

AFAIK It was invented by Intel Windows folks but Microsoft "documented"
it. We use the same property in ChromeOS (and therefore Linux) too so it
can be thought of as "de facto" way of declaring such port. 