Re: [PATCH] PCI: PM: Quirk bridge D3 on Elo i2

[Bjorn Helgaas <helgaas@kernel.org>]

Hi Rafael,

On Thu, Mar 31, 2022 at 07:38:51PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If one of the PCIe root ports on Elo i2 is put into D3cold and then
> back into D0, the downstream device becomes permanently inaccessible,
> so add a bridge D3 DMI quirk for that system.
> 
> This was exposed by commit 14858dcc3b35 ("PCI: Use
> pci_update_current_state() in pci_enable_device_flags()"), but before
> that commit the root port in question had never been put into D3cold
> for real due to a mismatch between its power state retrieved from the
> PCI_PM_CTRL register (which was accessible even though the platform
> firmware indicated that the port was in D3cold) and the state of an
> ACPI power resource involved in its power management.

In the bug report you suspect a firmware issue.  Any idea what that
might be?  It looks like a Gemini Lake Root Port, so I wouldn't think
it would be a hardware issue.

Weird how things come in clumps.  Was just looking at Mario's patch,
which also has to do with bridges and D3.

Do we need a Fixes line?  E.g.,

  Fixes: 14858dcc3b35 ("PCI: Use pci_update_current_state() in pci_enable_device_flags()")

> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215715
> Reported-by: Stefan Gottwald <gottwald@igel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/pci.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -2920,6 +2920,16 @@ static const struct dmi_system_id bridge
>  			DMI_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>  			DMI_MATCH(DMI_BOARD_NAME, "X299 DESIGNARE EX-CF"),
>  		},
> +		/*
> +		 * Downstream device is not accessible after putting a root port
> +		 * into D3cold and back into D0 on Elo i2.
> +		 */
> +		.ident = "Elo i2",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Elo Touch Solutions"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Elo i2"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "RevB"),
> +		},

Is this bridge_d3_blacklist[] similar to the PCI_DEV_FLAGS_NO_D3 bit?
Could they be folded together?  We have a lot of bits that seem
similar but maybe not exactly the same (dev->bridge_d3,
dev->no_d3cold, dev->d3cold_allowed, dev->runtime_d3cold,
PCI_DEV_FLAGS_NO_D3, pci_bridge_d3_force, etc.)  Ugh.

bridge_d3_blacklist[] itself was added by 85b0cae89d52 ("PCI:
Blacklist power management of Gigabyte X299 DESIGNARE EX PCIe ports"),
which honestly looks kind of random, i.e., it doesn't seem to be
working around a hardware or even a firmware defect.

Apparently the X299 issue is that 00:1c.4 is connected to a
Thunderbolt controller, and the BIOS keeps the Thunderbolt controller
powered off unless something is attached to it?  At least, 00:1c.4
leads to bus 05, and in the dmesg log attached to [1] shows no devices
on bus 05.

It also says the platform doesn't support PCIe native hotplug, which
matches what Mika said about it using ACPI hotplug.  If a system is
using ACPI hotplug, it seems like maybe *that* should prevent us from
putting things in D3cold?  How can we know whether ACPI hotplug
depends on a certain power state?

Bjorn

[1] https://bugzilla.kernel.org/show_bug.cgi?id=202031

>  	},
>  #endif
>  	{ }
> 
> 
>