diff mbox

PCI: Work around poweroff & suspend-to-RAM issue on Macbook Pro 11

Message ID 20170702205948.GB18324@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bjorn Helgaas July 2, 2017, 8:59 p.m. UTC 
Neither soft poweroff (transition to ACPI power state S5) nor
suspend-to-RAM (transition to state S3) works on the Macbook Pro 11,4 and
11,5.

The problem is related to the [mem 0x7fa00000-0x7fbfffff] space.  When we
use that space, e.g., by assigning it to the 00:1c.0 Root Port, the ACPI
Power Management 1 Control Register (PM1_CNT) at [io 0x1804] doesn't work
anymore.

Linux does a soft poweroff (transition to S5) by writing to PM1_CNT.  The
theory about why this doesn't work is:

  - The write to PM1_CNT causes an SMI
  - The BIOS SMI handler depends on something in
    [mem 0x7fa00000-0x7fbfffff]
  - When Linux assigns [mem 0x7fa00000-0x7fbfffff] to the 00:1c.0 Port, it
    covers up whatever the SMI handler uses, so the SMI handler no longer
    works correctly

Reserve the [mem 0x7fa00000-0x7fbfffff] space so we don't assign it to
anything.

This is voodoo programming, since we don't know what the real conflict is,
but we've failed to find the root cause.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=103211
Tested-by: thejoe@gmail.com
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: stable@vger.kernel.org
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Chen Yu <yu.c.chen@intel.com>

Comments

Chen Yu July 3, 2017, 4:26 a.m. UTC | #1 
Hi,
On Sun, Jul 02, 2017 at 03:59:48PM -0500, Bjorn Helgaas wrote:
> Neither soft poweroff (transition to ACPI power state S5) nor
> suspend-to-RAM (transition to state S3) works on the Macbook Pro 11,4 and
> 11,5.
> 
> The problem is related to the [mem 0x7fa00000-0x7fbfffff] space.  When we
> use that space, e.g., by assigning it to the 00:1c.0 Root Port, the ACPI
> Power Management 1 Control Register (PM1_CNT) at [io 0x1804] doesn't work
> anymore.
> 
> Linux does a soft poweroff (transition to S5) by writing to PM1_CNT.  The
> theory about why this doesn't work is:
> 
>   - The write to PM1_CNT causes an SMI
>   - The BIOS SMI handler depends on something in
>     [mem 0x7fa00000-0x7fbfffff]
>   - When Linux assigns [mem 0x7fa00000-0x7fbfffff] to the 00:1c.0 Port, it
>     covers up whatever the SMI handler uses, so the SMI handler no longer
>     works correctly
> 
> Reserve the [mem 0x7fa00000-0x7fbfffff] space so we don't assign it to
> anything.
> 
> This is voodoo programming, since we don't know what the real conflict is,
> but we've failed to find the root cause.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=103211
> Tested-by: thejoe@gmail.com
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: stable@vger.kernel.org
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Chen Yu <yu.c.chen@intel.com>
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 6d52b94f4bb9..20fa7c84109d 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -571,3 +571,35 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
> +
> +/*
> + * Apple MacBook Pro: Avoid [mem 0x7fa00000-0x7fbfffff]
> + *
> + * Using the [mem 0x7fa00000-0x7fbfffff] region, e.g., by assigning it to
> + * the 00:1c.0 Root Port, causes a conflict with [io 0x1804], which is used
> + * for soft poweroff and suspend-to-RAM.
> + *
> + * As far as we know, this is related to the address space, not to the Root
> + * Port itself.  Attaching the quirk to the Root Port is a convenience, but
> + * it could probably also be a standalone DMI quirk.
> + *
> + * https://bugzilla.kernel.org/show_bug.cgi?id=103211
> + */
> +static void quirk_apple_mbp_poweroff(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +
> +	if ((!dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,4") &&
> +	     !dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,5")) ||
> +	    pdev->bus->number != 0 || pdev->devfn != PCI_DEVFN(0x1c, 0))
> +		return;
> +
Not sure why we need to also the bus number of devfn, as there
is only one PCI bridge that would match 0x8c10? according to
https://bugzilla.kernel.org/attachment.cgi?id=210611
am I missing something?
thanks,
Yu
> +	res = request_mem_region(0x7fa00000, 0x200000,
> +				 "MacBook Pro poweroff workaround");
> +	if (res)
> +		dev_info(dev, "claimed %s %pR\n", res->name, res);
> +	else
> +		dev_info(dev, "can't work around MacBook Pro poweroff issue\n");
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x8c10, quirk_apple_mbp_poweroff);
Ethan Zhao July 3, 2017, 6:33 a.m. UTC | #2 
On Mon, Jul 3, 2017 at 12:26 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> Hi,
> On Sun, Jul 02, 2017 at 03:59:48PM -0500, Bjorn Helgaas wrote:
>> Neither soft poweroff (transition to ACPI power state S5) nor
>> suspend-to-RAM (transition to state S3) works on the Macbook Pro 11,4 and
>> 11,5.
>>
>> The problem is related to the [mem 0x7fa00000-0x7fbfffff] space.  When we
>> use that space, e.g., by assigning it to the 00:1c.0 Root Port, the ACPI
>> Power Management 1 Control Register (PM1_CNT) at [io 0x1804] doesn't work
>> anymore.
>>
>> Linux does a soft poweroff (transition to S5) by writing to PM1_CNT.  The
>> theory about why this doesn't work is:
>>
>>   - The write to PM1_CNT causes an SMI
>>   - The BIOS SMI handler depends on something in
>>     [mem 0x7fa00000-0x7fbfffff]
>>   - When Linux assigns [mem 0x7fa00000-0x7fbfffff] to the 00:1c.0 Port, it
>>     covers up whatever the SMI handler uses, so the SMI handler no longer
>>     works correctly
>>
>> Reserve the [mem 0x7fa00000-0x7fbfffff] space so we don't assign it to
>> anything.
>>
>> This is voodoo programming, since we don't know what the real conflict is,
>> but we've failed to find the root cause.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=103211
>> Tested-by: thejoe@gmail.com
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: stable@vger.kernel.org
>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>> Cc: Lukas Wunner <lukas@wunner.de>
>> Cc: Chen Yu <yu.c.chen@intel.com>
>>
>> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
>> index 6d52b94f4bb9..20fa7c84109d 100644
>> --- a/arch/x86/pci/fixup.c
>> +++ b/arch/x86/pci/fixup.c
>> @@ -571,3 +571,35 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
>>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
>> +
>> +/*
>> + * Apple MacBook Pro: Avoid [mem 0x7fa00000-0x7fbfffff]
>> + *
>> + * Using the [mem 0x7fa00000-0x7fbfffff] region, e.g., by assigning it to
>> + * the 00:1c.0 Root Port, causes a conflict with [io 0x1804], which is used
>> + * for soft poweroff and suspend-to-RAM.
>> + *
>> + * As far as we know, this is related to the address space, not to the Root
>> + * Port itself.  Attaching the quirk to the Root Port is a convenience, but
>> + * it could probably also be a standalone DMI quirk.
>> + *
>> + * https://bugzilla.kernel.org/show_bug.cgi?id=103211
>> + */
>> +static void quirk_apple_mbp_poweroff(struct pci_dev *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct resource *res;
>> +
>> +     if ((!dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,4") &&
>> +          !dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,5")) ||
>> +         pdev->bus->number != 0 || pdev->devfn != PCI_DEVFN(0x1c, 0))
>> +             return;
>> +
> Not sure why we need to also the bus number of devfn, as there
> is only one PCI bridge that would match 0x8c10? according to
> https://bugzilla.kernel.org/attachment.cgi?id=210611
> am I missing something?

 To make sure it runs only once only when 00:1c.0 is 0x8c10 ?

Thanks,
Ethan

> thanks,
> Yu
>> +     res = request_mem_region(0x7fa00000, 0x200000,
>> +                              "MacBook Pro poweroff workaround");
>> +     if (res)
>> +             dev_info(dev, "claimed %s %pR\n", res->name, res);
>> +     else
>> +             dev_info(dev, "can't work around MacBook Pro poweroff issue\n");
>> +}
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x8c10, quirk_apple_mbp_poweroff);
Lukas Wunner July 3, 2017, 9:10 a.m. UTC | #3 
On Mon, Jul 03, 2017 at 02:33:39PM +0800, Ethan Zhao wrote:
> On Mon, Jul 3, 2017 at 12:26 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> > On Sun, Jul 02, 2017 at 03:59:48PM -0500, Bjorn Helgaas wrote:
> >> --- a/arch/x86/pci/fixup.c
> >> +++ b/arch/x86/pci/fixup.c
[snip]
> >> +static void quirk_apple_mbp_poweroff(struct pci_dev *pdev)
> >> +{
> >> +     struct device *dev = &pdev->dev;
> >> +     struct resource *res;
> >> +
> >> +     if ((!dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,4") &&
> >> +          !dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,5")) ||
> >> +         pdev->bus->number != 0 || pdev->devfn != PCI_DEVFN(0x1c, 0))
> >> +             return;
> >> +
> > Not sure why we need to also the bus number of devfn, as there
> > is only one PCI bridge that would match 0x8c10? according to
> > https://bugzilla.kernel.org/attachment.cgi?id=210611
> > am I missing something?
> 
>  To make sure it runs only once only when 00:1c.0 is 0x8c10 ?

Each root port has a different PCI device ID and is only present once
on the affected machines, so I think Chen Yu is right.

Actually, since the quirk is now related to a memory region and no longer
to a specific PCI device, it need not be a PCI fixup.  It could go into
arch/x86/kernel/setup.c:trim_platform_memory_ranges() as a DMI quirk.

This would also allow declaration of the code as __init.

Thanks,

Lukas

> >> +     res = request_mem_region(0x7fa00000, 0x200000,
> >> +                              "MacBook Pro poweroff workaround");
> >> +     if (res)
> >> +             dev_info(dev, "claimed %s %pR\n", res->name, res);
> >> +     else
> >> +             dev_info(dev, "can't work around MacBook Pro poweroff issue\n");
> >> +}
> >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x8c10, quirk_apple_mbp_poweroff);
Bjorn Helgaas July 3, 2017, 1:05 p.m. UTC | #4 
On Mon, Jul 03, 2017 at 11:10:48AM +0200, Lukas Wunner wrote:
> On Mon, Jul 03, 2017 at 02:33:39PM +0800, Ethan Zhao wrote:
> > On Mon, Jul 3, 2017 at 12:26 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> > > On Sun, Jul 02, 2017 at 03:59:48PM -0500, Bjorn Helgaas wrote:
> > >> --- a/arch/x86/pci/fixup.c
> > >> +++ b/arch/x86/pci/fixup.c
> [snip]
> > >> +static void quirk_apple_mbp_poweroff(struct pci_dev *pdev)
> > >> +{
> > >> +     struct device *dev = &pdev->dev;
> > >> +     struct resource *res;
> > >> +
> > >> +     if ((!dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,4") &&
> > >> +          !dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,5")) ||
> > >> +         pdev->bus->number != 0 || pdev->devfn != PCI_DEVFN(0x1c, 0))
> > >> +             return;
> > >> +
> > > Not sure why we need to also the bus number of devfn, as there
> > > is only one PCI bridge that would match 0x8c10? according to
> > > https://bugzilla.kernel.org/attachment.cgi?id=210611
> > > am I missing something?
> > 
> >  To make sure it runs only once only when 00:1c.0 is 0x8c10 ?
> 
> Each root port has a different PCI device ID and is only present once
> on the affected machines, so I think Chen Yu is right.
> 
> Actually, since the quirk is now related to a memory region and no longer
> to a specific PCI device, it need not be a PCI fixup.  It could go into
> arch/x86/kernel/setup.c:trim_platform_memory_ranges() as a DMI quirk.
> 
> This would also allow declaration of the code as __init.

That's a good idea, but I don't know what (if any) ordering issues
we'd have with reserving this region vs. the host bridge windows.

If somebody wants to rework this and test it, I'd be glad to replace
the current patch.  If you do, please collect the /proc/iomem contents
to make sure they look reasonable.

Bjorn
diff mbox

Patch

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 6d52b94f4bb9..20fa7c84109d 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -571,3 +571,35 @@  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
+
+/*
+ * Apple MacBook Pro: Avoid [mem 0x7fa00000-0x7fbfffff]
+ *
+ * Using the [mem 0x7fa00000-0x7fbfffff] region, e.g., by assigning it to
+ * the 00:1c.0 Root Port, causes a conflict with [io 0x1804], which is used
+ * for soft poweroff and suspend-to-RAM.
+ *
+ * As far as we know, this is related to the address space, not to the Root
+ * Port itself.  Attaching the quirk to the Root Port is a convenience, but
+ * it could probably also be a standalone DMI quirk.
+ *
+ * https://bugzilla.kernel.org/show_bug.cgi?id=103211
+ */
+static void quirk_apple_mbp_poweroff(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+
+	if ((!dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,4") &&
+	     !dmi_match(DMI_PRODUCT_NAME, "MacBookPro11,5")) ||
+	    pdev->bus->number != 0 || pdev->devfn != PCI_DEVFN(0x1c, 0))
+		return;
+
+	res = request_mem_region(0x7fa00000, 0x200000,
+				 "MacBook Pro poweroff workaround");
+	if (res)
+		dev_info(dev, "claimed %s %pR\n", res->name, res);
+	else
+		dev_info(dev, "can't work around MacBook Pro poweroff issue\n");
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x8c10, quirk_apple_mbp_poweroff);