ACPI: Improve behaviour on Apple hardware

ACPI: Improve behaviour on Apple hardware

[Matthew Garrett]

Apple hardware behaves differently depending on whether or not the OS claims
to be Darwin. Failing to report Darwin results in some hardware being
disabled. However, claiming to be Darwin also alters the behaviour of
battery reporting and PCI handling. These patches add support for reporting
Darwin support and fixing up the behavioural quirks that are exposed as a
result.

-- 
Matthew Garrett | matthew.garrett@nebula.com

[PATCH 1/4] ACPI: Support _OSI("Darwin") correctly

[Matthew Garrett]

Apple hardware queries _OSI("Darwin") in order to determine whether the
system is running OS X, and changes firmware behaviour based on the answer.
The most obvious difference in behaviour is that Thunderbolt hardware is
forcibly powered down unless the system is running OS X. The obvious solution
would be to simply add Darwin to the list of supported _OSI strings, but this
causes problems.

Recent Apple hardware includes two separate methods for checking _OSI
strings. The first will check whether Darwin is supported, and if so will
exit. The second will check whether Darwin is supported, but will then
continue to check for further operating systems. If a further operating
system is found then later firmware code will assume that the OS is not OS X.
This results in the unfortunate situation where the Thunderbolt controller is
available at boot time but remains powered down after suspend.

The easiest way to handle this is to special-case it in the Linux-specific
OSI handling code. If we see Darwin, we should answer true and then disable
all other _OSI vendor strings.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/acpi/acpica/utosi.c |  1 +
 drivers/acpi/osl.c          | 10 ++++++++++
 include/acpi/actypes.h      | 25 +++++++++++++------------
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/acpica/utosi.c b/drivers/acpi/acpica/utosi.c
index 685766f..63b0a3b 100644
--- a/drivers/acpi/acpica/utosi.c
+++ b/drivers/acpi/acpica/utosi.c
@@ -87,6 +87,7 @@ ACPI_MODULE_NAME("utosi")
 static struct acpi_interface_info acpi_default_supported_interfaces[] = {
 	/* Operating System Vendor Strings */
 
+	{"Darwin", NULL, 0, ACPI_OSI_DARWIN},	/* OS X */
 	{"Windows 2000", NULL, 0, ACPI_OSI_WIN_2000},	/* Windows 2000 */
 	{"Windows 2001", NULL, 0, ACPI_OSI_WIN_XP},	/* Windows XP */
 	{"Windows 2001 SP1", NULL, 0, ACPI_OSI_WIN_XP_SP1},	/* Windows XP SP1 */
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6776c59..312ab5c 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -152,6 +152,16 @@ static u32 acpi_osi_handler(acpi_string interface, u32 supported)
 			osi_linux.dmi ? " via DMI" : "");
 	}
 
+	if (!strcmp("Darwin", interface)) {
+		/*
+		 * Apple firmware will behave poorly if it receives positive
+		 * answers to "Darwin" and any other OS. Respond positively
+		 * to Darwin and then disable all other vendor strings.
+		 */
+		acpi_update_interfaces(ACPI_DISABLE_ALL_VENDOR_STRINGS);
+		supported = ACPI_UINT32_MAX;
+	}
+
 	return supported;
 }
 
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index e763565..1c2ef56 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -1210,17 +1210,18 @@ struct acpi_memory_list {
 #define ACPI_ENABLE_ALL_FEATURE_STRINGS     (ACPI_ENABLE_INTERFACES | ACPI_FEATURE_STRINGS)
 #define ACPI_ENABLE_ALL_STRINGS             (ACPI_ENABLE_INTERFACES | ACPI_VENDOR_STRINGS | ACPI_FEATURE_STRINGS)
 
-#define ACPI_OSI_WIN_2000               0x01
-#define ACPI_OSI_WIN_XP                 0x02
-#define ACPI_OSI_WIN_XP_SP1             0x03
-#define ACPI_OSI_WINSRV_2003            0x04
-#define ACPI_OSI_WIN_XP_SP2             0x05
-#define ACPI_OSI_WINSRV_2003_SP1        0x06
-#define ACPI_OSI_WIN_VISTA              0x07
-#define ACPI_OSI_WINSRV_2008            0x08
-#define ACPI_OSI_WIN_VISTA_SP1          0x09
-#define ACPI_OSI_WIN_VISTA_SP2          0x0A
-#define ACPI_OSI_WIN_7                  0x0B
-#define ACPI_OSI_WIN_8                  0x0C
+#define ACPI_OSI_DARWIN                 0x01
+#define ACPI_OSI_WIN_2000               0x02
+#define ACPI_OSI_WIN_XP                 0x03
+#define ACPI_OSI_WIN_XP_SP1             0x04
+#define ACPI_OSI_WINSRV_2003            0x05
+#define ACPI_OSI_WIN_XP_SP2             0x06
+#define ACPI_OSI_WINSRV_2003_SP1        0x07
+#define ACPI_OSI_WIN_VISTA              0x08
+#define ACPI_OSI_WINSRV_2008            0x09
+#define ACPI_OSI_WIN_VISTA_SP1          0x0A
+#define ACPI_OSI_WIN_VISTA_SP2          0x0B
+#define ACPI_OSI_WIN_7                  0x0C
+#define ACPI_OSI_WIN_8                  0x0D
 
 #endif				/* __ACTYPES_H__ */
-- 
1.8.5.3

Re: [PATCH 1/4] ACPI: Support _OSI("Darwin") correctly

[Rafael J. Wysocki]

On Sunday, June 01, 2014 12:33:53 PM Matthew Garrett wrote:
> Apple hardware queries _OSI("Darwin") in order to determine whether the
> system is running OS X, and changes firmware behaviour based on the answer.
> The most obvious difference in behaviour is that Thunderbolt hardware is
> forcibly powered down unless the system is running OS X. The obvious solution
> would be to simply add Darwin to the list of supported _OSI strings, but this
> causes problems.
> 
> Recent Apple hardware includes two separate methods for checking _OSI
> strings. The first will check whether Darwin is supported, and if so will
> exit. The second will check whether Darwin is supported, but will then
> continue to check for further operating systems. If a further operating
> system is found then later firmware code will assume that the OS is not OS X.
> This results in the unfortunate situation where the Thunderbolt controller is
> available at boot time but remains powered down after suspend.
> 
> The easiest way to handle this is to special-case it in the Linux-specific
> OSI handling code. If we see Darwin, we should answer true and then disable
> all other _OSI vendor strings.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>

Does applying this patch without the rest of the series makes things worse
or better on the machines in question (or perhaps it doesn't matter at all
alone)?

Rafael


> ---
>  drivers/acpi/acpica/utosi.c |  1 +
>  drivers/acpi/osl.c          | 10 ++++++++++
>  include/acpi/actypes.h      | 25 +++++++++++++------------
>  3 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/utosi.c b/drivers/acpi/acpica/utosi.c
> index 685766f..63b0a3b 100644
> --- a/drivers/acpi/acpica/utosi.c
> +++ b/drivers/acpi/acpica/utosi.c
> @@ -87,6 +87,7 @@ ACPI_MODULE_NAME("utosi")
>  static struct acpi_interface_info acpi_default_supported_interfaces[] = {
>  	/* Operating System Vendor Strings */
>  
> +	{"Darwin", NULL, 0, ACPI_OSI_DARWIN},	/* OS X */
>  	{"Windows 2000", NULL, 0, ACPI_OSI_WIN_2000},	/* Windows 2000 */
>  	{"Windows 2001", NULL, 0, ACPI_OSI_WIN_XP},	/* Windows XP */
>  	{"Windows 2001 SP1", NULL, 0, ACPI_OSI_WIN_XP_SP1},	/* Windows XP SP1 */
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6776c59..312ab5c 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -152,6 +152,16 @@ static u32 acpi_osi_handler(acpi_string interface, u32 supported)
>  			osi_linux.dmi ? " via DMI" : "");
>  	}
>  
> +	if (!strcmp("Darwin", interface)) {
> +		/*
> +		 * Apple firmware will behave poorly if it receives positive
> +		 * answers to "Darwin" and any other OS. Respond positively
> +		 * to Darwin and then disable all other vendor strings.
> +		 */
> +		acpi_update_interfaces(ACPI_DISABLE_ALL_VENDOR_STRINGS);
> +		supported = ACPI_UINT32_MAX;
> +	}
> +
>  	return supported;
>  }
>  
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index e763565..1c2ef56 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -1210,17 +1210,18 @@ struct acpi_memory_list {
>  #define ACPI_ENABLE_ALL_FEATURE_STRINGS     (ACPI_ENABLE_INTERFACES | ACPI_FEATURE_STRINGS)
>  #define ACPI_ENABLE_ALL_STRINGS             (ACPI_ENABLE_INTERFACES | ACPI_VENDOR_STRINGS | ACPI_FEATURE_STRINGS)
>  
> -#define ACPI_OSI_WIN_2000               0x01
> -#define ACPI_OSI_WIN_XP                 0x02
> -#define ACPI_OSI_WIN_XP_SP1             0x03
> -#define ACPI_OSI_WINSRV_2003            0x04
> -#define ACPI_OSI_WIN_XP_SP2             0x05
> -#define ACPI_OSI_WINSRV_2003_SP1        0x06
> -#define ACPI_OSI_WIN_VISTA              0x07
> -#define ACPI_OSI_WINSRV_2008            0x08
> -#define ACPI_OSI_WIN_VISTA_SP1          0x09
> -#define ACPI_OSI_WIN_VISTA_SP2          0x0A
> -#define ACPI_OSI_WIN_7                  0x0B
> -#define ACPI_OSI_WIN_8                  0x0C
> +#define ACPI_OSI_DARWIN                 0x01
> +#define ACPI_OSI_WIN_2000               0x02
> +#define ACPI_OSI_WIN_XP                 0x03
> +#define ACPI_OSI_WIN_XP_SP1             0x04
> +#define ACPI_OSI_WINSRV_2003            0x05
> +#define ACPI_OSI_WIN_XP_SP2             0x06
> +#define ACPI_OSI_WINSRV_2003_SP1        0x07
> +#define ACPI_OSI_WIN_VISTA              0x08
> +#define ACPI_OSI_WINSRV_2008            0x09
> +#define ACPI_OSI_WIN_VISTA_SP1          0x0A
> +#define ACPI_OSI_WIN_VISTA_SP2          0x0B
> +#define ACPI_OSI_WIN_7                  0x0C
> +#define ACPI_OSI_WIN_8                  0x0D
>  
>  #endif				/* __ACTYPES_H__ */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/4] ACPI: Support _OSI("Darwin") correctly

[Matthew Garrett]

On Tue, Jun 17, 2014 at 02:11:36PM +0200, Rafael J. Wysocki wrote:
> On Sunday, June 01, 2014 12:33:53 PM Matthew Garrett wrote:
> > Apple hardware queries _OSI("Darwin") in order to determine whether the
> > system is running OS X, and changes firmware behaviour based on the answer.
> > The most obvious difference in behaviour is that Thunderbolt hardware is
> > forcibly powered down unless the system is running OS X. The obvious solution
> > would be to simply add Darwin to the list of supported _OSI strings, but this
> > causes problems.
> > 
> > Recent Apple hardware includes two separate methods for checking _OSI
> > strings. The first will check whether Darwin is supported, and if so will
> > exit. The second will check whether Darwin is supported, but will then
> > continue to check for further operating systems. If a further operating
> > system is found then later firmware code will assume that the OS is not OS X.
> > This results in the unfortunate situation where the Thunderbolt controller is
> > available at boot time but remains powered down after suspend.
> > 
> > The easiest way to handle this is to special-case it in the Linux-specific
> > OSI handling code. If we see Darwin, we should answer true and then disable
> > all other _OSI vendor strings.
> > 
> > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> 
> Does applying this patch without the rest of the series makes things worse
> or better on the machines in question (or perhaps it doesn't matter at all
> alone)?

On its own, I think this will do nothing.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/4] ACPI: Support _OSI("Darwin") correctly

[Matthew Garrett]

On Tue, Jun 17, 2014 at 03:46:15PM +0100, Matthew Garrett wrote:
> On Tue, Jun 17, 2014 at 02:11:36PM +0200, Rafael J. Wysocki wrote:
> > Does applying this patch without the rest of the series makes things worse
> > or better on the machines in question (or perhaps it doesn't matter at all
> > alone)?
> 
> On its own, I think this will do nothing.

Ugh, no, sorry - on its own, this will break PCIe hotplug events and 
battery reporting. It should only be merged with the others.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/4] ACPI: Support _OSI("Darwin") correctly

[Rafael J. Wysocki]

On Wednesday, June 18, 2014 06:26:15 AM Matthew Garrett wrote:
> On Tue, Jun 17, 2014 at 03:46:15PM +0100, Matthew Garrett wrote:
> > On Tue, Jun 17, 2014 at 02:11:36PM +0200, Rafael J. Wysocki wrote:
> > > Does applying this patch without the rest of the series makes things worse
> > > or better on the machines in question (or perhaps it doesn't matter at all
> > > alone)?
> > 
> > On its own, I think this will do nothing.
> 
> Ugh, no, sorry - on its own, this will break PCIe hotplug events and 
> battery reporting. It should only be merged with the others.

Well, in that case it is not useful to split that material into
multiple patches.  And it will need to be Linux-specific, as adding
patch [1/4] to upstream ACPICA might break some other OSes using it.

Rafael

[PATCH 2/4] ACPI: Don't call PCI OSC on Apple hardware when claiming to be Darwin

[Matthew Garrett]

The Apple PCI _OSC method has the following code:

if (LEqual (0x01, OSDW ()))
  if (LAnd (LEqual (Arg0, GUID), NEXP)
    (do stuff)
  else
    (fail)

NEXP is a value in high memory and is presumably under the control of the
firmware. No methods set it. The methods that are called in the "do stuff"
path are dummies. Unless there's some additional firmware call in early boot,
there's no way for this call to succeed - and even if it does, it doesn't do
anything.

The easiest way to handle this is simply to ignore it. We know which flags
would be set, so just set them by hand if the platform is running in Darwin
mode.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/acpi/pci_root.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d388f13..f04b6b9 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -430,6 +430,17 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	acpi_handle handle = device->handle;
 
 	/*
+	 * Apple always return failure on _OSC calls when _OSI("Darwin") has
+	 * been called successfully. We know the feature set supported by the
+	 * platform, so avoid calling _OSC at all
+	 */
+
+	if (acpi_gbl_osi_data == ACPI_OSI_DARWIN) {
+		root->osc_control_set = ~OSC_PCI_EXPRESS_PME_CONTROL;
+		return;
+	}
+
+	/*
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
 	 */
-- 
1.8.5.3

[PATCH 3/4] ACPI: Don't assume the existence of an SBS charger

[Matthew Garrett]

Apple hardware continues to expose an ACPI AC charger even when using SBS
to report battery state. The charger status byte returns all 0s in this case.
Since the spec requires that bit 4 be 1 at all times, assume that there's not
really a charger if it's set to zero.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/acpi/sbs.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 366ca40..b1df4ee 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -109,6 +109,7 @@ struct acpi_sbs {
 	u8 batteries_supported:4;
 	u8 manager_present:1;
 	u8 charger_present:1;
+	u8 charger_exists:1;
 };
 
 #define to_acpi_sbs(x) container_of(x, struct acpi_sbs, charger)
@@ -429,9 +430,19 @@ static int acpi_ac_get_present(struct acpi_sbs *sbs)
 
 	result = acpi_smbus_read(sbs->hc, SMBUS_READ_WORD, ACPI_SBS_CHARGER,
 				 0x13, (u8 *) & status);
-	if (!result)
-		sbs->charger_present = (status >> 15) & 0x1;
-	return result;
+
+	if (result)
+		return result;
+
+	/*
+	 * The spec requires that bit 4 always be 1. If it's not set, assume
+	 * that the implementation doesn't support an SBS charger
+	 */
+	if (!(status >> 4) & 0x1)
+		return -ENODEV;
+
+	sbs->charger_present = (status >> 15) & 0x1;
+	return 0;
 }
 
 static ssize_t acpi_battery_alarm_show(struct device *dev,
@@ -554,6 +565,7 @@ static int acpi_charger_add(struct acpi_sbs *sbs)
 	if (result)
 		goto end;
 
+	sbs->charger_exists = 1;
 	sbs->charger.name = "sbs-charger";
 	sbs->charger.type = POWER_SUPPLY_TYPE_MAINS;
 	sbs->charger.properties = sbs_ac_props;
@@ -580,9 +592,12 @@ static void acpi_sbs_callback(void *context)
 	struct acpi_battery *bat;
 	u8 saved_charger_state = sbs->charger_present;
 	u8 saved_battery_state;
-	acpi_ac_get_present(sbs);
-	if (sbs->charger_present != saved_charger_state)
-		kobject_uevent(&sbs->charger.dev->kobj, KOBJ_CHANGE);
+
+	if (sbs->charger_exists) {
+		acpi_ac_get_present(sbs);
+		if (sbs->charger_present != saved_charger_state)
+			kobject_uevent(&sbs->charger.dev->kobj, KOBJ_CHANGE);
+	}
 
 	if (sbs->manager_present) {
 		for (id = 0; id < MAX_SBS_BAT; ++id) {
@@ -619,7 +634,7 @@ static int acpi_sbs_add(struct acpi_device *device)
 	device->driver_data = sbs;
 
 	result = acpi_charger_add(sbs);
-	if (result)
+	if (result && result != -ENODEV)
 		goto end;
 
 	result = acpi_manager_get_info(sbs);
-- 
1.8.5.3

[PATCH 4/4] ACPI: Disable smart battery manager on Apple

[Matthew Garrett]

Touching the smart battery manager at all on Apple hardware appears to make
it unhappy - unplugging the AC adapter triggers accesses that hang the
controller for several minutes. Quirk it out via DMI in order to avoid this.
Compensate by changing battery presence if we fail to communicate with the
battery.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/acpi/sbs.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index b1df4ee..32aecea 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -35,6 +35,7 @@
 #include <linux/jiffies.h>
 #include <linux/delay.h>
 #include <linux/power_supply.h>
+#include <linux/dmi.h>
 
 #include "sbshc.h"
 #include "battery.h"
@@ -61,6 +62,8 @@ static unsigned int cache_time = 1000;
 module_param(cache_time, uint, 0644);
 MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
 
+static bool sbs_manager_broken;
+
 #define MAX_SBS_BAT			4
 #define ACPI_SBS_BLOCK_MAX		32
 
@@ -494,16 +497,21 @@ static int acpi_battery_read(struct acpi_battery *battery)
 				  ACPI_SBS_MANAGER, 0x01, (u8 *)&state, 2);
 	} else if (battery->id == 0)
 		battery->present = 1;
+
 	if (result || !battery->present)
 		return result;
 
 	if (saved_present != battery->present) {
 		battery->update_time = 0;
 		result = acpi_battery_get_info(battery);
-		if (result)
+		if (result) {
+			battery->present = 0;
 			return result;
+		}
 	}
 	result = acpi_battery_get_state(battery);
+	if (result)
+		battery->present = 0;
 	return result;
 }
 
@@ -535,6 +543,7 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
 	result = power_supply_register(&sbs->device->dev, &battery->bat);
 	if (result)
 		goto end;
+
 	result = device_create_file(battery->bat.dev, &alarm_attr);
 	if (result)
 		goto end;
@@ -613,12 +622,31 @@ static void acpi_sbs_callback(void *context)
 	}
 }
 
+static int disable_sbs_manager(const struct dmi_system_id *d)
+{
+	sbs_manager_broken = true;
+	return 0;
+}
+
+static struct dmi_system_id acpi_sbs_dmi_table[] = {
+	{
+		.callback = disable_sbs_manager,
+		.ident = "Apple",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.")
+		},
+	},
+	{ },
+};
+
 static int acpi_sbs_add(struct acpi_device *device)
 {
 	struct acpi_sbs *sbs;
 	int result = 0;
 	int id;
 
+	dmi_check_system(acpi_sbs_dmi_table);
+
 	sbs = kzalloc(sizeof(struct acpi_sbs), GFP_KERNEL);
 	if (!sbs) {
 		result = -ENOMEM;
@@ -637,14 +665,21 @@ static int acpi_sbs_add(struct acpi_device *device)
 	if (result && result != -ENODEV)
 		goto end;
 
-	result = acpi_manager_get_info(sbs);
-	if (!result) {
-		sbs->manager_present = 1;
-		for (id = 0; id < MAX_SBS_BAT; ++id)
-			if ((sbs->batteries_supported & (1 << id)))
-				acpi_battery_add(sbs, id);
-	} else
+	result = 0;
+
+	if (!sbs_manager_broken) {
+		result = acpi_manager_get_info(sbs);
+		if (!result) {
+			sbs->manager_present = 0;
+			for (id = 0; id < MAX_SBS_BAT; ++id)
+				if ((sbs->batteries_supported & (1 << id)))
+					acpi_battery_add(sbs, id);
+		}
+	}
+
+	if (!sbs->manager_present)
 		acpi_battery_add(sbs, 0);
+
 	acpi_smbus_register_callback(sbs->hc, acpi_sbs_callback, sbs);
       end:
 	if (result)
-- 
1.8.5.3