linux-kernel - Re: [PATCH] applesmc: Bump max wait and rearrange udelay

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 16 Sep 2012 20:11:29 -0400 (EDT)
From:	Parag Warudkar <parag.lkml@...il.com>
To:	Henrik Rydberg <rydberg@...omail.se>
cc:	Parag Warudkar <parag.lkml@...il.com>,
	Guenter Roeck <linux@...ck-us.net>, lm-sensors@...sensors.org,
	linux-kernel@...r.kernel.org, khali@...ux-fr.org
Subject: Re: [PATCH] applesmc: Bump max wait and rearrange udelay



On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> What exact model is this?

MacBookPro6,1 - 2010 17" MBP.

> 
> In addition to Guenter's comments, it is not clear what part of this
> patch makes things work for you. Is it a) the doubling of the wait
> time, or b) the zero initial wait? Or c) just randomly a little bit
> better?

I have addressed Guenter's comments in the revised patch below - I've 
minimized the deviation from the original code - only thing additional is 
usleep_range which I believe is a good thing to do.

> 
> If a), that is very valuable information, and the patch can be
> simplified further. If b), just crank up the wait time and be done
> with it. If c), well, then we don't have a case for a patch.
> 
I initially experimented with different max wait values and came to the 
conclusion that 0x20000 was good enough to reduce the errors to zero. But 
then Guenter pointed out that the original loop terminated too early - 
that means it only waited 16 ms instead of 32. I then corrected the loop 
termination but kept the original 32ms max wait and used usleep_range() 
instead of udelay() - testing with this gave me no errors whereas earlier 
the errors were readily reproducible and significant in quanity.

So I guess actually sleeping for 32ms along with the upward variability 
introduced by usleep_range() is helping with the fix.

> Also, it is not enough to test this on one machine, for fairly obvious
> reasons. I don't mind testing on a couple of machines close by (MBA3,1
> MBP10,1), once the comments have been addressed. However, a machine
> from around 2008 should be tested as well, since they behave
> differently.

Since we are keeping the changes minimal and using the same 32 ms max wait 
as originally intended I think this patch can only make things better. It 
sure does make things better on my machine but it will not hurt to test it 
on other models.

Parag

Signed-off-by: Parag Warudkar <parag.lkml@...il.com>

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..a168a8f 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -169,12 +169,13 @@ static int wait_read(void)
 {
 	u8 status;
 	int us;
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		udelay(us);
+	for (us = APPLESMC_MIN_WAIT; us <= APPLESMC_MAX_WAIT; us <<= 1) {
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
+		if (us < APPLESMC_MAX_WAIT)
+			usleep_range(us, us << 1);
 	}
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
@@ -192,7 +193,7 @@ static int send_byte(u8 cmd, u16 port)
 
 	outb(cmd, port);
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		udelay(us);
+		usleep_range(us, us << 1);
 		status = inb(APPLESMC_CMD_PORT);
 		/* write: wait for smc to settle */
 		if (status & 0x02)
@@ -204,7 +205,7 @@ static int send_byte(u8 cmd, u16 port)
 		if (us << 1 == APPLESMC_MAX_WAIT)
 			break;
 		/* busy: long wait and resend */
-		udelay(APPLESMC_RETRY_WAIT);
+		usleep_range(APPLESMC_RETRY_WAIT, APPLESMC_RETRY_WAIT << 1);
 		outb(cmd, port);
 	}
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/