diff mbox series

[v3] PCI: Blacklist power management of Gigabyte X299 DESIGNARE EX PCIe ports

Message ID 20190124131227.45612-1-mika.westerberg@linux.intel.com (mailing list archive)
State Not Applicable, archived
Headers show
Series [v3] PCI: Blacklist power management of Gigabyte X299 DESIGNARE EX PCIe ports | expand

Commit Message

Mika Westerberg Jan. 24, 2019, 1:12 p.m. UTC 
Gigabyte X299 DESIGNARE EX motherboard has one PCIe root port that is
connected to an Alpine Ridge Thunderbolt controller. This port has slot
implemented bit set in the config space but other than that it is not
hotplug capable in the sense we are expecting in Linux (it has
dev->is_hotplug_bridge set to 0):

00:1c.4 PCI bridge: Intel Corporation 200 Series PCH PCI Express Root Port #5
        Bus: primary=00, secondary=05, subordinate=46, sec-latency=0
        Memory behind bridge: 78000000-8fffffff [size=384M]
        Prefetchable memory behind bridge: 00003800f8000000-00003800ffffffff [size=128M]
        ...
        Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
        ...
                SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-
                        Slot #8, PowerLimit 25.000W; Interlock- NoCompl+
                SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
                        Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
                SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
                        Changed: MRL- PresDet+ LinkState+

This system is using ACPI based hotplug to notify the OS that it needs
to rescan the PCI bus (ACPI hotplug).

If there is nothing connected in any of the Thunderbolt ports the root
port will not have any runtime PM active children and is thus
automatically runtime suspended pretty soon after boot by PCI PM core.
Now, when a device is connected the BIOS SMI handler responsible for
enumerating newly added devices is not able to find anything because the
port is in D3.

Prevent this from happening by blacklisting PCI power management of this
particular Gigabyte system.

Reported-by: Kedar A Dongre <kedar.a.dongre@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
Hi,

This is pretty much the same as v1 but I added Rafael's tag and rebased it
on top of v5.0-rc3. The alternative approach in v2 breaks power management
of certain discrete graphics so use blacklist for now.

v2: https://patchwork.kernel.org/patch/10750549/
v1: https://patchwork.kernel.org/patch/10711553/

 drivers/pci/pci.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Lukas Wunner Jan. 24, 2019, 1:25 p.m. UTC | #1 
On Thu, Jan 24, 2019 at 04:12:27PM +0300, Mika Westerberg wrote:
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2501,6 +2501,23 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
>  		pm_runtime_put_sync(parent);
>  }
>  
> +static const struct dmi_system_id bridge_d3_blacklist[] = {
> +	{
> +		/*
> +		 * Gigabyte X299 root port is not marked as hotplug
> +		 * capable which allows Linux to power manage it.
> +		 * However, this confuses the BIOS SMI handler so don't
> +		 * power manage root ports on that system.
> +		 */
> +		.ident = "X299 DESIGNARE EX-CF",
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> +			DMI_MATCH(DMI_BOARD_NAME, "X299 DESIGNARE EX-CF"),
> +		},
> +	},
> +	{ }
> +};

Unfortunately this doesn't take my comment voiced Jan 8 into account:

   "Please constrain either the blacklist entry for the Gigabyte mainboard
    or the blacklist check itself to x86 using IS_ENABLED() or #ifdef to
    avoid code bloat on other arches."
    https://patchwork.kernel.org/patch/10750549/#22408911

Note that CONFIG_DMI may be enabled on arm64 and ia64 which have no use
for this quirk.  It's generally undesirable to have arch-specific quirks
in generic code and folks running Linux on memory-constrained mips routers
have complained before that quirks.c contains too much code unrelated to
their arch.

Thanks,

Lukas
Mika Westerberg Jan. 24, 2019, 1:44 p.m. UTC | #2 
On Thu, Jan 24, 2019 at 02:25:42PM +0100, Lukas Wunner wrote:
> On Thu, Jan 24, 2019 at 04:12:27PM +0300, Mika Westerberg wrote:
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2501,6 +2501,23 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
> >  		pm_runtime_put_sync(parent);
> >  }
> >  
> > +static const struct dmi_system_id bridge_d3_blacklist[] = {
> > +	{
> > +		/*
> > +		 * Gigabyte X299 root port is not marked as hotplug
> > +		 * capable which allows Linux to power manage it.
> > +		 * However, this confuses the BIOS SMI handler so don't
> > +		 * power manage root ports on that system.
> > +		 */
> > +		.ident = "X299 DESIGNARE EX-CF",
> > +		.matches = {
> > +			DMI_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> > +			DMI_MATCH(DMI_BOARD_NAME, "X299 DESIGNARE EX-CF"),
> > +		},
> > +	},
> > +	{ }
> > +};
> 
> Unfortunately this doesn't take my comment voiced Jan 8 into account:
> 
>    "Please constrain either the blacklist entry for the Gigabyte mainboard
>     or the blacklist check itself to x86 using IS_ENABLED() or #ifdef to
>     avoid code bloat on other arches."
>     https://patchwork.kernel.org/patch/10750549/#22408911

I tried to take it into account based on what you said:

  Please constrain either the blacklist entry for the Gigabyte mainboard

so I constrained it to Gigabyte motherboard using DMI entry. Maybe I
misunderstood you. Sorry about that.

> Note that CONFIG_DMI may be enabled on arm64 and ia64 which have no use
> for this quirk.  It's generally undesirable to have arch-specific quirks
> in generic code and folks running Linux on memory-constrained mips routers
> have complained before that quirks.c contains too much code unrelated to
> their arch.

I understand that but you end up with code looking as ugly as this:

  if (IS_ENABLED(CONFIG_X86) && dmi_check_system(bridge_d3_blacklist))
  	return false;

just to save a function call and and couple of bytes from kernel data. I
would rather prefer readability in this case. But no problem doing the
above if that's the way Bjorn likes to have it.
Lukas Wunner Jan. 24, 2019, 2:17 p.m. UTC | #3 
On Thu, Jan 24, 2019 at 03:44:26PM +0200, Mika Westerberg wrote:
> On Thu, Jan 24, 2019 at 02:25:42PM +0100, Lukas Wunner wrote:
> > On Thu, Jan 24, 2019 at 04:12:27PM +0300, Mika Westerberg wrote:
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2501,6 +2501,23 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
> > >  		pm_runtime_put_sync(parent);
> > >  }
> > >  
> > > +static const struct dmi_system_id bridge_d3_blacklist[] = {
> > > +	{
> > > +		/*
> > > +		 * Gigabyte X299 root port is not marked as hotplug
> > > +		 * capable which allows Linux to power manage it.
> > > +		 * However, this confuses the BIOS SMI handler so don't
> > > +		 * power manage root ports on that system.
> > > +		 */
> > > +		.ident = "X299 DESIGNARE EX-CF",
> > > +		.matches = {
> > > +			DMI_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> > > +			DMI_MATCH(DMI_BOARD_NAME, "X299 DESIGNARE EX-CF"),
> > > +		},
> > > +	},
> > > +	{ }
> > > +};
> > 
> > Unfortunately this doesn't take my comment voiced Jan 8 into account:
> > 
> >    "Please constrain either the blacklist entry for the Gigabyte mainboard
> >     or the blacklist check itself to x86 using IS_ENABLED() or #ifdef to
> >     avoid code bloat on other arches."
> >     https://patchwork.kernel.org/patch/10750549/#22408911
> 
> I tried to take it into account based on what you said:
> 
>   Please constrain either the blacklist entry for the Gigabyte mainboard
> 
> so I constrained it to Gigabyte motherboard using DMI entry. Maybe I
> misunderstood you. Sorry about that.

What I meant is that you could encapsulate the struct dmi_system_id for
the Gigabyte mainboard in "#ifdef CONFIG_X86" / "#endif".


> > Note that CONFIG_DMI may be enabled on arm64 and ia64 which have no use
> > for this quirk.

The rationale for this sentence was that the compiler might be smart
enough to optimize bridge_d3_blacklist[] away if CONFIG_DMI is not
defined, but it might be defined on arm64 and ia64 so on those arches
you'd still be including an unnecessary DMI entry.

Granted those few bytes won't hurt, but people might come after you and
amend the blacklist, if the #ifdef is already in place they'll know to
add their entry within the #ifdef or add a new #ifdef if the quirk isn't
for x86, thus neatly keeping generic code free of arch-specific quirks.

Thanks,

Lukas
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c25acace7d91..47ceef050782 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2501,6 +2501,23 @@  void pci_config_pm_runtime_put(struct pci_dev *pdev)
 		pm_runtime_put_sync(parent);
 }
 
+static const struct dmi_system_id bridge_d3_blacklist[] = {
+	{
+		/*
+		 * Gigabyte X299 root port is not marked as hotplug
+		 * capable which allows Linux to power manage it.
+		 * However, this confuses the BIOS SMI handler so don't
+		 * power manage root ports on that system.
+		 */
+		.ident = "X299 DESIGNARE EX-CF",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+			DMI_MATCH(DMI_BOARD_NAME, "X299 DESIGNARE EX-CF"),
+		},
+	},
+	{ }
+};
+
 /**
  * pci_bridge_d3_possible - Is it possible to put the bridge into D3
  * @bridge: Bridge to check
@@ -2546,6 +2563,9 @@  bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (bridge->is_hotplug_bridge)
 			return false;
 
+		if (dmi_check_system(bridge_d3_blacklist))
+			return false;
+
 		/*
 		 * It should be safe to put PCIe ports from 2015 or newer
 		 * to D3.