All of lore.kernel.org
 help / color / mirror / Atom feed
From: Henrik Rydberg <rydberg@euromail.se>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Murphy <bugzilla@colorremedies.com>,
	Josh Boyer <jwboyer@fedoraproject.org>,
	khali@linux-fr.org, lm-sensors@lm-sensors.org,
	"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>
Subject: Re: [lm-sensors] applesmc oops in 3.10/3.11
Date: Tue, 01 Oct 2013 10:55:26 +0000	[thread overview]
Message-ID: <20131001105526.GA481@polaris.bitmath.org> (raw)
In-Reply-To: <524A4384.4040403@roeck-us.net>

> >Warning message triggered with 3.12.0-0.rc3.git0.1.fc21.x86_64.
> >
> >[   10.886016] applesmc: key count changed from 261 to 1174405121
> >
> 
> Explains the crash, but the new key count is very wrong. 1174405121 = 0x46000001.
> Which I guess explains the subsequent memory allocation error in the log.
> 
> Henrik, any idea what might be going on ? Is it possible that the previous
> command failure leaves some state machine in a bad state ?

I seem to recall a report on another similar state problem on newer
machines, so maybe, yes. Older machines seem fine, I have never
encountered the problem myself. Here is a patch to test that
theory. It has been tested to be pretty harmless on two different
generations.

I really really do not want to add an 'if (value is insane)' check ;-)

Thanks,
Henrik

From d48a9e4e6e45dcd9c7e7ad88df714b92772a62d6 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Tue, 1 Oct 2013 12:16:09 +0200
Subject: [PATCH] applesmc remedy take 1

Conjectured problem: there are remnant bytes ready on the data line
which corrupts the read after a failure.

Remedy: assuming bit0 is the read valid line, try to flush it before
starting a new command.

Exception: the write-number-of-bytes-to-read command seems to differ
between models (it may not be needed on the newest), so do not try to
flush the data at that particular point.

Tested on a MacBookPro10,1 and a MacBookAir3,1, but the original problem
has not been reproduced, so the actual effect of this patch is unknown.
---
 drivers/hwmon/applesmc.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 98814d1..f6eaf6d1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -186,10 +186,23 @@ static int wait_read(void)
  * send_byte - Write to SMC port, retrying when necessary. Callers
  * must hold applesmc_lock.
  */
-static int send_byte(u8 cmd, u16 port)
+static int send_byte(u8 cmd, u16 port, bool flush)
 {
-	u8 status;
-	int us;
+	u8 status, data;
+	int us, nskip;
+
+	if (flush) {
+		/* read the data port until bit0 is cleared */
+		for (nskip = 0; nskip < 16; nskip++) {
+			udelay(APPLESMC_MIN_WAIT);
+			status = inb(APPLESMC_CMD_PORT);
+			if (!(status & 0x01))
+				break;
+			data = inb(APPLESMC_DATA_PORT);
+		}
+		if (nskip)
+			pr_warn("flushed %d bytes\n", nskip);
+	}
 
 	outb(cmd, port);
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
@@ -215,7 +228,7 @@ static int send_byte(u8 cmd, u16 port)
 
 static int send_command(u8 cmd)
 {
-	return send_byte(cmd, APPLESMC_CMD_PORT);
+	return send_byte(cmd, APPLESMC_CMD_PORT, true);
 }
 
 static int send_argument(const char *key)
@@ -223,7 +236,7 @@ static int send_argument(const char *key)
 	int i;
 
 	for (i = 0; i < 4; i++)
-		if (send_byte(key[i], APPLESMC_DATA_PORT))
+		if (send_byte(key[i], APPLESMC_DATA_PORT, true))
 			return -EIO;
 	return 0;
 }
@@ -237,7 +250,7 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	if (send_byte(len, APPLESMC_DATA_PORT)) {
+	if (send_byte(len, APPLESMC_DATA_PORT, false)) {
 		pr_warn("%.4s: read len fail\n", key);
 		return -EIO;
 	}
@@ -262,13 +275,13 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	if (send_byte(len, APPLESMC_DATA_PORT)) {
+	if (send_byte(len, APPLESMC_DATA_PORT, true)) {
 		pr_warn("%.4s: write len fail\n", key);
 		return -EIO;
 	}
 
 	for (i = 0; i < len; i++) {
-		if (send_byte(buffer[i], APPLESMC_DATA_PORT)) {
+		if (send_byte(buffer[i], APPLESMC_DATA_PORT, true)) {
 			pr_warn("%s: write data fail\n", key);
 			return -EIO;
 		}
-- 
1.8.3.4


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

WARNING: multiple messages have this Message-ID (diff)
From: Henrik Rydberg <rydberg@euromail.se>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Murphy <bugzilla@colorremedies.com>,
	Josh Boyer <jwboyer@fedoraproject.org>,
	khali@linux-fr.org, lm-sensors@lm-sensors.org,
	"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>
Subject: Re: applesmc oops in 3.10/3.11
Date: Tue, 1 Oct 2013 12:55:26 +0200	[thread overview]
Message-ID: <20131001105526.GA481@polaris.bitmath.org> (raw)
In-Reply-To: <524A4384.4040403@roeck-us.net>

> >Warning message triggered with 3.12.0-0.rc3.git0.1.fc21.x86_64.
> >
> >[   10.886016] applesmc: key count changed from 261 to 1174405121
> >
> 
> Explains the crash, but the new key count is very wrong. 1174405121 = 0x46000001.
> Which I guess explains the subsequent memory allocation error in the log.
> 
> Henrik, any idea what might be going on ? Is it possible that the previous
> command failure leaves some state machine in a bad state ?

I seem to recall a report on another similar state problem on newer
machines, so maybe, yes. Older machines seem fine, I have never
encountered the problem myself. Here is a patch to test that
theory. It has been tested to be pretty harmless on two different
generations.

I really really do not want to add an 'if (value is insane)' check ;-)

Thanks,
Henrik

>From d48a9e4e6e45dcd9c7e7ad88df714b92772a62d6 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Tue, 1 Oct 2013 12:16:09 +0200
Subject: [PATCH] applesmc remedy take 1

Conjectured problem: there are remnant bytes ready on the data line
which corrupts the read after a failure.

Remedy: assuming bit0 is the read valid line, try to flush it before
starting a new command.

Exception: the write-number-of-bytes-to-read command seems to differ
between models (it may not be needed on the newest), so do not try to
flush the data at that particular point.

Tested on a MacBookPro10,1 and a MacBookAir3,1, but the original problem
has not been reproduced, so the actual effect of this patch is unknown.
---
 drivers/hwmon/applesmc.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 98814d1..f6eaf6d1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -186,10 +186,23 @@ static int wait_read(void)
  * send_byte - Write to SMC port, retrying when necessary. Callers
  * must hold applesmc_lock.
  */
-static int send_byte(u8 cmd, u16 port)
+static int send_byte(u8 cmd, u16 port, bool flush)
 {
-	u8 status;
-	int us;
+	u8 status, data;
+	int us, nskip;
+
+	if (flush) {
+		/* read the data port until bit0 is cleared */
+		for (nskip = 0; nskip < 16; nskip++) {
+			udelay(APPLESMC_MIN_WAIT);
+			status = inb(APPLESMC_CMD_PORT);
+			if (!(status & 0x01))
+				break;
+			data = inb(APPLESMC_DATA_PORT);
+		}
+		if (nskip)
+			pr_warn("flushed %d bytes\n", nskip);
+	}
 
 	outb(cmd, port);
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
@@ -215,7 +228,7 @@ static int send_byte(u8 cmd, u16 port)
 
 static int send_command(u8 cmd)
 {
-	return send_byte(cmd, APPLESMC_CMD_PORT);
+	return send_byte(cmd, APPLESMC_CMD_PORT, true);
 }
 
 static int send_argument(const char *key)
@@ -223,7 +236,7 @@ static int send_argument(const char *key)
 	int i;
 
 	for (i = 0; i < 4; i++)
-		if (send_byte(key[i], APPLESMC_DATA_PORT))
+		if (send_byte(key[i], APPLESMC_DATA_PORT, true))
 			return -EIO;
 	return 0;
 }
@@ -237,7 +250,7 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	if (send_byte(len, APPLESMC_DATA_PORT)) {
+	if (send_byte(len, APPLESMC_DATA_PORT, false)) {
 		pr_warn("%.4s: read len fail\n", key);
 		return -EIO;
 	}
@@ -262,13 +275,13 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	if (send_byte(len, APPLESMC_DATA_PORT)) {
+	if (send_byte(len, APPLESMC_DATA_PORT, true)) {
 		pr_warn("%.4s: write len fail\n", key);
 		return -EIO;
 	}
 
 	for (i = 0; i < len; i++) {
-		if (send_byte(buffer[i], APPLESMC_DATA_PORT)) {
+		if (send_byte(buffer[i], APPLESMC_DATA_PORT, true)) {
 			pr_warn("%s: write data fail\n", key);
 			return -EIO;
 		}
-- 
1.8.3.4


  reply	other threads:[~2013-10-01 10:55 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-25 19:06 [lm-sensors] applesmc oops in 3.10/3.11 Josh Boyer
2013-09-25 19:06 ` Josh Boyer
2013-09-25 19:56 ` [lm-sensors] " Guenter Roeck
2013-09-25 19:56   ` Guenter Roeck
2013-09-25 21:48   ` [lm-sensors] " Henrik Rydberg
2013-09-25 21:48     ` Henrik Rydberg
2013-09-25 22:08     ` [lm-sensors] " Guenter Roeck
2013-09-25 22:08       ` Guenter Roeck
2013-09-26  6:34       ` [lm-sensors] " Henrik Rydberg
2013-09-26  6:34         ` Henrik Rydberg
2013-09-26 10:36         ` [lm-sensors] " Guenter Roeck
2013-09-26 10:36           ` Guenter Roeck
2013-09-26 11:13           ` [lm-sensors] " Henrik Rydberg
2013-09-26 11:13             ` Henrik Rydberg
2013-09-26 10:53         ` [lm-sensors] " Guenter Roeck
2013-09-26 10:53           ` Guenter Roeck
2013-09-26 11:11           ` [lm-sensors] " Henrik Rydberg
2013-09-26 11:11             ` Henrik Rydberg
2013-09-27 16:21         ` [lm-sensors] " Josh Boyer
2013-09-27 16:21           ` Josh Boyer
2013-09-27 17:12           ` [lm-sensors] " Guenter Roeck
2013-09-27 17:12             ` Guenter Roeck
2013-09-27 17:41             ` [lm-sensors] " Chris Murphy
2013-09-27 17:41               ` Chris Murphy
2013-09-27 17:59               ` [lm-sensors] " Guenter Roeck
2013-09-27 17:59                 ` Guenter Roeck
2013-09-27 18:03                 ` [lm-sensors] " Chris Murphy
2013-09-27 18:03                   ` Chris Murphy
2013-09-27 23:33                   ` [lm-sensors] " Guenter Roeck
2013-09-27 23:33                     ` Guenter Roeck
2013-10-01  1:57                     ` [lm-sensors] " Chris Murphy
2013-10-01  1:57                       ` Chris Murphy
2013-10-01  3:37                       ` [lm-sensors] " Guenter Roeck
2013-10-01  3:37                         ` Guenter Roeck
2013-10-01 10:55                         ` Henrik Rydberg [this message]
2013-10-01 10:55                           ` Henrik Rydberg
2013-10-01 15:19                           ` [lm-sensors] " Guenter Roeck
2013-10-01 15:19                             ` Guenter Roeck
2013-10-01 15:33                             ` [lm-sensors] " Chris Murphy
2013-10-01 15:33                               ` Chris Murphy
2013-10-01 16:24                               ` [lm-sensors] " Guenter Roeck
2013-10-01 16:24                                 ` Guenter Roeck
2013-10-02  1:09                                 ` [lm-sensors] " Chris Murphy
2013-10-02  1:09                                   ` Chris Murphy
2013-10-02  3:51                                   ` [lm-sensors] " Guenter Roeck
2013-10-02  3:51                                     ` Guenter Roeck
2013-10-02  3:55                                     ` [lm-sensors] " Chris Murphy
2013-10-02  3:55                                       ` Chris Murphy
2013-10-02  4:02                                       ` [lm-sensors] " Guenter Roeck
2013-10-02  4:02                                         ` Guenter Roeck
2013-10-02  9:53                                     ` [lm-sensors] " Henrik Rydberg
2013-10-02  9:53                                       ` Henrik Rydberg
2013-10-02 13:30                                       ` [lm-sensors] " Guenter Roeck
2013-10-02 13:30                                         ` Guenter Roeck
2013-10-02 16:34                                         ` [lm-sensors] " Henrik Rydberg
2013-10-02 16:34                                           ` Henrik Rydberg
2013-10-02 16:47                                           ` [lm-sensors] " Guenter Roeck
2013-10-02 16:47                                             ` Guenter Roeck
2013-10-02 17:24                                             ` [lm-sensors] " Henrik Rydberg
2013-10-02 17:24                                               ` Henrik Rydberg
2013-10-02 18:02                                               ` [lm-sensors] " Guenter Roeck
2013-10-02 18:02                                                 ` Guenter Roeck
2013-10-02 18:33                                                 ` [lm-sensors] " Chris Murphy
2013-10-02 18:33                                                   ` Chris Murphy
2013-10-02 20:59                                                   ` [lm-sensors] " Guenter Roeck
2013-10-02 20:59                                                     ` Guenter Roeck
2013-10-02 21:34                                                     ` [lm-sensors] " Chris Murphy
2013-10-02 21:34                                                       ` Chris Murphy
2013-10-02 23:32                                                       ` [lm-sensors] " Guenter Roeck
2013-10-02 23:32                                                         ` Guenter Roeck
2013-10-07 23:42                                               ` [lm-sensors] " Guenter Roeck
2013-10-07 23:42                                                 ` Guenter Roeck
2013-10-07 23:46                                                 ` [lm-sensors] " Chris Murphy
2013-10-07 23:46                                                   ` Chris Murphy
2013-10-08 15:48                                                   ` [lm-sensors] " Guenter Roeck
2013-10-08 15:48                                                     ` Guenter Roeck
2013-10-08 16:29                                                     ` [lm-sensors] " Henrik Rydberg
2013-10-08 16:29                                                       ` Henrik Rydberg
2013-10-08 16:29                                                       ` [lm-sensors] " Guenter Roeck
2013-10-08 16:29                                                         ` Guenter Roeck
2013-10-09  8:29                                                         ` [lm-sensors] " Henrik Rydberg
2013-10-09  8:29                                                           ` Henrik Rydberg
2013-10-09 16:52                                                           ` [lm-sensors] " Guenter Roeck
2013-10-09 16:52                                                             ` Guenter Roeck
2013-09-27 18:01           ` [lm-sensors] " Guenter Roeck
2013-09-27 18:01             ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131001105526.GA481@polaris.bitmath.org \
    --to=rydberg@euromail.se \
    --cc=bugzilla@colorremedies.com \
    --cc=jwboyer@fedoraproject.org \
    --cc=khali@linux-fr.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lm-sensors@lm-sensors.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.