[PATCH] ACPI SBS: Fix intermittent hangs on Apple Macbook

[PATCH] ACPI SBS: Fix intermittent hangs on Apple Macbook

[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.

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 <chris.bainbridge@gmail.com>
---
 drivers/acpi/ec.c    | 14 +++++++++++++-
 drivers/acpi/sbs.c   | 10 +++++++---
 drivers/acpi/sbshc.c | 24 ++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 220d640..7c9dacc 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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 */
@@ -505,7 +506,7 @@ static int ec_poll(struct acpi_ec *ec)
 		unsigned long usecs = ACPI_EC_UDELAY_POLL;
 		do {
 			/* don't sleep with disabled interrupts */
-			if (EC_FLAGS_MSI || irqs_disabled()) {
+			if (EC_FLAGS_APPLE || EC_FLAGS_MSI || irqs_disabled()) {
 				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
@@ -1323,6 +1332,9 @@ static struct dmi_system_id ec_dmi_table[] __initdata = {
 	DMI_MATCH(DMI_SYS_VENDOR, "CLEVO CO."),
 	DMI_MATCH(DMI_PRODUCT_NAME, "W35_37ET"),}, NULL},
 	{
+	ec_flag_apple, "Apple", {
+	DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.")}, NULL},
+	{
 	ec_validate_ecdt, "ASUS hardware", {
 	DMI_MATCH(DMI_BIOS_VENDOR, "ASUS") }, NULL},
 	{
diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index cd82762..165519e 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -369,8 +369,11 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
 					 info_readers[i].command,
 					 (u8 *) battery +
 						info_readers[i].offset);
-		if (result)
+		if (result) {
+			pr_warn("sbs read battery failed (cmd=%x)\n",
+				info_readers[i].command);
 			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
diff --git a/drivers/acpi/sbshc.c b/drivers/acpi/sbshc.c
index 26e5b50..2d40c24 100644
--- a/drivers/acpi/sbshc.c
+++ b/drivers/acpi/sbshc.c
@@ -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 int SMBUS_FLAGS_APPLE;
+
 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)
+{
+	SMBUS_FLAGS_APPLE = 1;
+	return 0;
+}
+
+static struct dmi_system_id acpi_smbus_dmi_table[] = {
+	{
+		.callback = smbus_flag_apple,
+		.ident = "Apple",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.")
+		},
+	},
+	{ },
+};
+
 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;
 
-- 
2.1.4

Re: [PATCH] ACPI SBS: Fix intermittent hangs on Apple Macbook

[Rafael J. Wysocki]

On Friday, April 24, 2015 02:25:30 AM Chris Bainbridge wrote:
> 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 <chris.bainbridge@gmail.com>

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

> ---
>  drivers/acpi/ec.c    | 14 +++++++++++++-
>  drivers/acpi/sbs.c   | 10 +++++++---
>  drivers/acpi/sbshc.c | 24 ++++++++++++++++++++++++
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 220d640..7c9dacc 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -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 */
> @@ -505,7 +506,7 @@ static int ec_poll(struct acpi_ec *ec)
>  		unsigned long usecs = ACPI_EC_UDELAY_POLL;
>  		do {
>  			/* don't sleep with disabled interrupts */
> -			if (EC_FLAGS_MSI || irqs_disabled()) {
> +			if (EC_FLAGS_APPLE || EC_FLAGS_MSI || irqs_disabled()) {

Does EC_FLAGS_MSI do anything in addition to this?  If not, it might be better
to rename it and use it for both Apple and MSI.

>  				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
> @@ -1323,6 +1332,9 @@ static struct dmi_system_id ec_dmi_table[] __initdata = {
>  	DMI_MATCH(DMI_SYS_VENDOR, "CLEVO CO."),
>  	DMI_MATCH(DMI_PRODUCT_NAME, "W35_37ET"),}, NULL},
>  	{
> +	ec_flag_apple, "Apple", {
> +	DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.")}, NULL},

Do we really need this for all Apple hardware?

> +	{
>  	ec_validate_ecdt, "ASUS hardware", {
>  	DMI_MATCH(DMI_BIOS_VENDOR, "ASUS") }, NULL},
>  	{
> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index cd82762..165519e 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c
> @@ -369,8 +369,11 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
>  					 info_readers[i].command,
>  					 (u8 *) battery +
>  						info_readers[i].offset);
> -		if (result)
> +		if (result) {
> +			pr_warn("sbs read battery failed (cmd=%x)\n",
> +				info_readers[i].command);

This should be a debug printk() and the one below too.  Also I'm not sure
why it belongs to this patch.

>  			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
> diff --git a/drivers/acpi/sbshc.c b/drivers/acpi/sbshc.c
> index 26e5b50..2d40c24 100644
> --- a/drivers/acpi/sbshc.c
> +++ b/drivers/acpi/sbshc.c
> @@ -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 int SMBUS_FLAGS_APPLE;

Why upper case?  The EC driver has this weird naming convention, but is there
any good reason to spread it?

> +
>  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)
> +{
> +	SMBUS_FLAGS_APPLE = 1;
> +	return 0;
> +}
> +
> +static struct dmi_system_id acpi_smbus_dmi_table[] = {
> +	{
> +		.callback = smbus_flag_apple,
> +		.ident = "Apple",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.")

Again, is this really needed for all Apple hardware?

> +		},
> +	},
> +	{ },
> +};
> +
>  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;
>  
> 

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

Re: [PATCH] ACPI SBS: Fix intermittent hangs on Apple Macbook

[Chris Bainbridge]

On Wed, Apr 29, 2015 at 02:37:57AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 24, 2015 02:25:30 AM Chris Bainbridge wrote:
> > 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 <chris.bainbridge@gmail.com>
> 
> Well, this looks like two patches combined to me.  Are the quirks actually
> related except that they are both needed to fix the problem?
> 

Not related, so should have been separate patches. One fixes the problem
of causing the SBS controller to stop responding, which results in a
hang.  The other fixes the problem of the resulting hang - but now I'm
thinking we don't need this, since it should never happen unless we hang
the SBS. I'll resend just the first fix as a new patch.

> 
> Do we really need this for all Apple hardware?
> 

Just MacBooks. I don't know which ones - the hang has been reported on:

    MacBookAir6,1 hsw 11" 2013
    MacBookPro10,2 ivb 13" 2012/2013
    MacBookPro11,1 hsw 15" 2013/2014

But it could affect earlier or later models too. The next patch will
limit this to MacBooks only - I have no way to narrow down the list of
affected hardware further, but since the fix only adds a 5us delay to
each SBS read it shouldn't cause any problems on unaffected hardware.

Thanks for the review.

[PATCH] ACPI/sbshc: Add 5us delay to fix SBS hang on MacBook

[Chris Bainbridge]

Regression in commit 7bc5a2bad0b8d9d1ac9f7b8b33150e4ddf197334
Author: Matthew Garrett <matthew.garrett@nebula.com>
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 <chris.bainbridge@gmail.com>
---
 drivers/acpi/sbshc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/acpi/sbshc.c b/drivers/acpi/sbshc.c
index 26e5b50..bf034f8 100644
--- a/drivers/acpi/sbshc.c
+++ b/drivers/acpi/sbshc.c
@@ -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;
 
-- 
2.1.4

Re: [PATCH] ACPI/sbshc: Add 5us delay to fix SBS hang on MacBook

[Rafael J. Wysocki]

On Wednesday, April 29, 2015 09:21:40 PM Chris Bainbridge wrote:
> Regression in commit 7bc5a2bad0b8d9d1ac9f7b8b33150e4ddf197334
> Author: Matthew Garrett <matthew.garrett@nebula.com>
> 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 <chris.bainbridge@gmail.com>

Looks much better, I've queued it up as a fix for 4.1, thanks!

> ---
>  drivers/acpi/sbshc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/acpi/sbshc.c b/drivers/acpi/sbshc.c
> index 26e5b50..bf034f8 100644
> --- a/drivers/acpi/sbshc.c
> +++ b/drivers/acpi/sbshc.c
> @@ -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;
>  
> 

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

Re: [PATCH] ACPI/sbshc: Add 5us delay to fix SBS hang on MacBook

[Brad Campbell]

On 30/04/15 04:21, Chris Bainbridge wrote:
>
> 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

Re: [PATCH] ACPI/sbshc: Add 5us delay to fix SBS hang on MacBook

[Chris Bainbridge]

On Mon, May 18, 2015 at 10:20:33AM +0800, Brad Campbell wrote:
> On 30/04/15 04:21, Chris Bainbridge wrote:
> >
> >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

I used a patch similar to the following to test, it just runs the SBS
read in a loop. acpi_battery_get_info is run when the sbs module is
loaded. So start off with the 5us delay and a few runs through the loop
and verify you hit the hang, then increase the delay to 10us or whatever
and run the loop for a few thousand cycles until you're happy that
increasing the delay fixes the problem.


diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 01504c8..74f8596 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -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,
+						 (u8 *) battery +
+							info_readers[i].offset);
+			if (result) {
+				printk("FAIL\n");
+				break;
+			}
+		}
 	}
 	return result;
 }

Re: [PATCH] ACPI/sbshc: Add 5us delay to fix SBS hang on MacBook

[Chris Bainbridge]

On Mon, May 18, 2015 at 01:27:37PM +0100, Chris Bainbridge wrote:
> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index 01504c8..74f8596 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c
> @@ -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,
> +						 (u8 *) battery +
> +							info_readers[i].offset);
> +			if (result) {
> +				printk("FAIL\n");
> +				break;
> +			}
> +		}
>  	}
>  	return result;
>  }


Better to exit immediately when SBS hangs, otherwise you could be
waiting a long time for modprobe to return:


diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 01504c8..84f422f 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -361,17 +361,22 @@ 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 < 100; 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,
+						 (u8 *) battery +
+							info_readers[i].offset);
+			if (result)
+				goto end;
+		}
 	}
+      end:
+	printk(result ? "FAIL" : "OK");
 	return result;
 }
 

I'm sure you get the idea. Forcing repeated SBS reads is a much quicker
way to uncover an intermittent fault than through normal usage. After
adding the 5us delay, I was able to run the acpi_battery_get_info loop
for 20,000 cycles without fault, but this is just one Macbook, others
may be different. It's also possible that the delay is just covering up
some other problem; some patches were posted a few days ago to fix some
underlying hang issues: http://www.spinics.net/lists/linux-acpi/msg57572.html