ACPI SBS: Fix intermittent hangs on Apple Macbook

Commit 7bc5a2b exposed the SBS on Apple hardware, resulting in intermittent hangs of several minutes on boot, and failure to detect or report the battery. We fix this two ways:

- SMBUS hang should not hang the whole system. Respect the specified timeout by busy waiting instead of sleeping.
This fix is already used on MSI hardware.

- Stop the SBS from hanging in the first place by introducing a 5us delay before each SMBUS transaction. This fix is the result of experimentation. This particular delay was found to completely fix the problem on an Ivybridge Macbook Pro 13. Hangs were observed with 3us delay but never with 5us.

Also, pr_warn if SBS communication fails instead of silently ignoring.

Link https://bugzilla.kernel.org/show_bug.cgi?id=94651

Signed-off-by Chris Bainbridge <[email protected]>

---
 drivers/acpi/ec.c    | 14 +++++++++++++-
 drivers/acpi/sbs.c   | 10 +++++++---
 drivers/acpi/sbshc.c | 24 ++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

drivers/acpi/ec.c +1 -0

index 220d640..7c9dacc 100644
@@ -131,6 +131,7 @@ struct acpi_ec *boot_ec, *first_ec;
 EXPORT_SYMBOL(first_ec);
 
 static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
+static int EC_FLAGS_APPLE; /* Out-of-spec Apple controller */
 static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
 static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
 static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */

drivers/acpi/ec.c +8 -0

 				usecs = ACPI_EC_MSI_UDELAY;
 				udelay(usecs);
 				if (ec_transaction_completed(ec))

@@ -1246,6 +1247,14 @@ static int ec_flag_msi(const struct dmi_system_id *id)
 	return 0;
 }
 
+/* Apple Macbook needs special treatment, enable it */
+static int ec_flag_apple(const struct dmi_system_id *id)
+{
+	pr_debug("Detected Apple hardware, enabling workarounds.\n");
+	EC_FLAGS_APPLE = 1;
+	return 0;
+}
+
 /*
  * Clevo M720 notebook actually works ok with IRQ mode, if we lifted
  * the GPE storm threshold back to 20

drivers/acpi/ec.c +1 -0

+	{
 	ec_validate_ecdt, "ASUS hardware", {
 	DMI_MATCH(DMI_BIOS_VENDOR, "ASUS") }, NULL},
 	{

drivers/acpi/sbs.c +0 -0

index cd82762..165519e 100644

drivers/acpi/sbs.c +4 -2

 			break;
+		}
 	}
 	return result;
 }

@@ -412,7 +415,6 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
 
 	int ret;
 
-
 	if (sbs->manager_present) {
 		ret = acpi_smbus_read(sbs->hc, SMBUS_READ_WORD, ACPI_SBS_MANAGER,
 				0x01, (u8 *)&value);

@@ -442,8 +444,10 @@ 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)
+	if (result) {
+		pr_warn("sbs read charger failed\n");
 		return result;
+	}
 
 	/*
 	 * The spec requires that bit 4 always be 1. If it's not set, assume

drivers/acpi/sbshc.c +1 -0

index 26e5b50..2d40c24 100644
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
+#include <linux/dmi.h>
 #include "sbshc.h"
 
 #define PREFIX "ACPI: "

drivers/acpi/sbshc.c +5 -0

+
 static inline int smb_hc_read(struct acpi_smb_hc *hc, u8 address, u8 *data)
 {
 	return ec_read(hc->offset + address, data);

@@ -132,6 +135,8 @@ static int acpi_smbus_transaction(struct acpi_smb_hc *hc, u8 protocol,
 	}
 
 	mutex_lock(&hc->lock);
+	if (SMBUS_FLAGS_APPLE)
+		udelay(5);
 	if (smb_hc_read(hc, ACPI_SMB_PROTOCOL, &temp))
 		goto end;
 	if (temp) {

@@ -257,12 +262,31 @@ extern int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
 
+static int smbus_flag_apple(const struct dmi_system_id *d)
+{

drivers/acpi/sbshc.c +7 -0

+		},
+	},
+	{ },
+};
+
 static int acpi_smbus_hc_add(struct acpi_device *device)
 {
 	int status;
 	unsigned long long val;
 	struct acpi_smb_hc *hc;
 
+	dmi_check_system(acpi_smbus_dmi_table);
+
 	if (!device)
 		return -EINVAL;
 

Quoted 14 lines · Chris Bainbridge

Commit 7bc5a2b exposed the SBS on Apple hardware, resulting in intermittent hangs of several minutes on boot, and failure to detect or report the battery. We fix this two ways:

- SMBUS hang should not hang the whole system. Respect the specified timeout by busy waiting instead of sleeping.
This fix is already used on MSI hardware.

- Stop the SBS from hanging in the first place by introducing a 5us delay before each SMBUS transaction. This fix is the result of experimentation. This particular delay was found to completely fix the problem on an Ivybridge Macbook Pro 13. Hangs were observed with 3us delay but never with 5us.

Quoted 4 lines

Also, pr_warn if SBS communication fails instead of silently ignoring.

Link https://bugzilla.kernel.org/show_bug.cgi?id=94651

Signed-off-by Chris Bainbridge <[email protected]>

Well, this looks like two patches combined to me. Are the quirks actually related except that they are both needed to fix the problem?

Do we really need this for all Apple hardware?

Regression in commit 7bc5a2bad0b8d9d1ac9f7b8b33150e4ddf197334

Author: Matthew Garrett <[email protected]>
Date:   Sat Sep 20 13:19:47 2014 +0200

ACPI: Support _OSI("Darwin") correctly

Supporting _OSI("Darwin") caused the MacBook firmware to expose the SBS, resulting in intermittent hangs of several minutes on boot, and failure to detect or report the battery. Fix this by adding a 5us delay to the start of each SMBUS transaction. This timing is the result of experimentation - hangs were observed with 3us but never with 5us.

Link https://bugzilla.kernel.org/show_bug.cgi?id=94651

Signed-off-by Chris Bainbridge <[email protected]>

---
 drivers/acpi/sbshc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/acpi/sbshc.c +22 -0

index 26e5b50..bf034f8 100644
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
+#include <linux/dmi.h>
 #include "sbshc.h"
 
 #define PREFIX "ACPI: "

@@ -87,6 +88,8 @@ enum acpi_smb_offset {
 	ACPI_SMB_ALARM_DATA = 0x26,	/* 2 bytes alarm data */
 };
 
+static bool macbook;
+
 static inline int smb_hc_read(struct acpi_smb_hc *hc, u8 address, u8 *data)
 {
 	return ec_read(hc->offset + address, data);

@@ -132,6 +135,8 @@ static int acpi_smbus_transaction(struct acpi_smb_hc *hc, u8 protocol,
 	}
 
 	mutex_lock(&hc->lock);
+	if (macbook)
+		udelay(5);
 	if (smb_hc_read(hc, ACPI_SMB_PROTOCOL, &temp))
 		goto end;
 	if (temp) {

@@ -257,12 +262,29 @@ extern int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
 
+static int macbook_dmi_match(const struct dmi_system_id *d)
+{
+	pr_debug("Detected MacBook, enabling workaround\n");
+	macbook = true;
+	return 0;
+}
+
+static struct dmi_system_id acpi_smbus_dmi_table[] = {
+	{ macbook_dmi_match, "Apple MacBook", {
+	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
+	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook") },
+	},
+	{ },
+};
+
 static int acpi_smbus_hc_add(struct acpi_device *device)
 {
 	int status;
 	unsigned long long val;
 	struct acpi_smb_hc *hc;
 
+	dmi_check_system(acpi_smbus_dmi_table);
+
 	if (!device)
 		return -EINVAL;
 

Supporting _OSI("Darwin") caused the MacBook firmware to expose the SBS, resulting in intermittent hangs of several minutes on boot, and failure to detect or report the battery. Fix this by adding a 5us delay to the start of each SMBUS transaction. This timing is the result of experimentation - hangs were observed with 3us but never with 5us.

Quoted 6 lines · Chris Bainbridge

Supporting _OSI("Darwin") caused the MacBook firmware to expose the SBS, resulting in intermittent hangs of several minutes on boot, and failure to detect or report the battery. Fix this by adding a 5us delay to the start of each SMBUS transaction. This timing is the result of experimentation - hangs were observed with 3us but never with 5us.

G'day Chris,

Using 4.1-rc3 (whatever git head was from a couple of days ago) I've still been seeing ACPI lockups on my Macbook Pro (11,1). It appears to take a couple of suspend/resume cycles but seems to happen within 24 hours of booting whereas before this patch it was easy to hit.

As a test (and I do wonder if it'll lock up after I press send on this E-mail) I upped the delay to 10us and I've now been up for 1 day, 17:23 with the battery still responding to polling.

To make it easier to reproduce, I've been running a script that polls the battery every 60 seconds.

I figure I'll let it go for another couple of days, but I wanted to put this out there in case anyone else is seeing lockups after a longer period of uptime. The symptoms are the ACPI command just hangs as does 'cat /sys/class/power_supply/BAT0/status'. From there I can't suspend as the tasks can't be frozen and I just end up hard booting the machine.

I've removed Len & Rafael from the cc.

Regards,
Brad

drivers/acpi/sbs.c +8 -10

index 01504c8..74f8596 100644
@@ -361,16 +361,21 @@ static int acpi_manager_get_info(struct acpi_sbs *sbs)
 static int acpi_battery_get_info(struct acpi_battery *battery)
 {
 	int i, result = 0;
-
-	for (i = 0; i < ARRAY_SIZE(info_readers); ++i) {
-		result = acpi_smbus_read(battery->sbs->hc,
-					 info_readers[i].mode,
-					 ACPI_SBS_BATTERY,
-					 info_readers[i].command,
-					 (u8 *) battery +
-						info_readers[i].offset);
-		if (result)
-			break;
+	int j;
+
+	for (j = 0; j < 1000; j++) {
+		for (i = 0; i < ARRAY_SIZE(info_readers); ++i) {
+			result = acpi_smbus_read(battery->sbs->hc,
+						 info_readers[i].mode,
+						 ACPI_SBS_BATTERY,
+						 info_readers[i].command,