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 17:22:21 -0400 (EDT)
From:	Parag Warudkar <parag.lkml@...il.com>
To:	Henrik Rydberg <rydberg@...omail.se>
cc:	Guenter Roeck <linux@...ck-us.net>,
	Parag Warudkar <parag.lkml@...il.com>,
	lm-sensors@...sensors.org, linux-kernel@...r.kernel.org,
	khali@...ux-fr.org
Subject: Re: [PATCH] applesmc: Bump max wait and rearrange udelay



On Sun, 16 Sep 2012, Henrik Rydberg wrote:

> On Sat, Sep 15, 2012 at 09:31:16PM -0700, Guenter Roeck wrote:
> > That looks terribly complicated. Better keep the loop, and just replace
> > 	udelay(us);
> > with something like
> > 	usleep_range(us, us << 1);
> > 
> > Alternatively, just use a constant such as
> > 	usleep_range(us, us + APPLESMC_MIN_WAIT);
> 
Well I don't think there is anything terribly complicated there - but I 
tried to make it simpler. Below patch seems to work better for me for my 
normal workload - I got no failures or other oddities with the default 
32ms timeout. I haven't tried very hard to get to the corner cases which 
earlier required a higher timeout. 

I would like to get this in for now if there aren't any further 
suggestions, as it fixes the failures by making it use the 
32ms maximum it was originally supposed to and for bonus it's also 
converted to use usleep_range which is better from PM standpoint.

> It would be worthwhile to check if there are other bits in status that
> encodes a busy state, similar to what we now have in send_byte(). This
> is what has finally made almost all machines error-free. Increasing
> the max wait is possible, of course, but it only kills the symptoms.
> 
> So, Parag, would it be possible for you to print the status field as
> you go through one of those long waits? If you find a bit that seems
> to change occasionally, you could try to use it as a busy indicator,
> and use the APPLESMC_RETRY_WAIT for that case.
> 
Henrik - if the below patch still results in failures we can 
revisit the long  wait cases. For now I am satisfied that there aren't any 
normal case failures like before.

Thanks,
Parag

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..569aa8d 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -168,14 +168,14 @@ static struct workqueue_struct *applesmc_led_wq;
 static int wait_read(void)
 {
 	u8 status;
-	int us;
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		udelay(us);
+	int us = APPLESMC_MIN_WAIT;
+	do {
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
-	}
+		usleep_range(us, us << 1);
+	} while ((us <<= 1) <= APPLESMC_MAX_WAIT);
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
 	return -EIO;
@@ -192,19 +192,22 @@ static int send_byte(u8 cmd, u16 port)
 
 	outb(cmd, port);
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* write: wait for smc to settle */
-		if (status & 0x02)
+		if (status & 0x02) {
+			usleep_range(us, us << 1);
 			continue;
+		}
 		/* ready: cmd accepted, return */
 		if (status & 0x04)
 			return 0;
 		/* timeout: give up */
-		if (us << 1 == APPLESMC_MAX_WAIT)
+		if (us << 1 > APPLESMC_MAX_WAIT) {
+			pr_warn("Timeout with us: %d\n", us);
 			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/