[lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

I have been getting a steady stream of wait_read timeouts on my 2010 MBP.

After playing around with various values of APPLESMC_MAX_WAIT a value of 
0x10000 reduces the wait_read failures to zero under most normal workloads 
- with and without AC power plugged in, at idle and and at make -j4 loads.

While there I noticed we don't really need to udelay before first inb() - 
so I moved it down to after first and subsequent failures.

Been running this for couple days without any issues.

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..46cb458 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -56,7 +56,7 @@
 /* wait up to 32 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
-#define APPLESMC_MAX_WAIT	0x8000
+#define APPLESMC_MAX_WAIT	0x10000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
@@ -170,11 +170,11 @@ static int wait_read(void)
 	u8 status;
 	int us;
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
+		udelay(us);
 	}
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
@@ -192,11 +192,12 @@ 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) {
+			udelay(us);
 			continue;
+		}
 		/* ready: cmd accepted, return */
 		if (status & 0x04)
 			return 0;

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

[PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

I have been getting a steady stream of wait_read timeouts on my 2010 MBP.

After playing around with various values of APPLESMC_MAX_WAIT a value of 
0x10000 reduces the wait_read failures to zero under most normal workloads 
- with and without AC power plugged in, at idle and and at make -j4 loads.

While there I noticed we don't really need to udelay before first inb() - 
so I moved it down to after first and subsequent failures.

Been running this for couple days without any issues.

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..46cb458 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -56,7 +56,7 @@
 /* wait up to 32 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
-#define APPLESMC_MAX_WAIT	0x8000
+#define APPLESMC_MAX_WAIT	0x10000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
@@ -170,11 +170,11 @@ static int wait_read(void)
 	u8 status;
 	int us;
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
+		udelay(us);
 	}
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
@@ -192,11 +192,12 @@ 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) {
+			udelay(us);
 			continue;
+		}
 		/* ready: cmd accepted, return */
 		if (status & 0x04)
 			return 0;

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Guenter Roeck]

On Sat, Sep 15, 2012 at 06:42:30PM -0400, Parag Warudkar wrote:
> I have been getting a steady stream of wait_read timeouts on my 2010 MBP.
> 
> After playing around with various values of APPLESMC_MAX_WAIT a value of 
> 0x10000 reduces the wait_read failures to zero under most normal workloads 
> - with and without AC power plugged in, at idle and and at make -j4 loads.
> 
> While there I noticed we don't really need to udelay before first inb() - 
> so I moved it down to after first and subsequent failures.
> 
> Been running this for couple days without any issues.
> 
> Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..46cb458 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -56,7 +56,7 @@
>  /* wait up to 32 ms for a status change. */
>  #define APPLESMC_MIN_WAIT	0x0010
>  #define APPLESMC_RETRY_WAIT	0x0100
> -#define APPLESMC_MAX_WAIT	0x8000
> +#define APPLESMC_MAX_WAIT	0x10000
>  
>  #define APPLESMC_READ_CMD	0x10
>  #define APPLESMC_WRITE_CMD	0x11
> @@ -170,11 +170,11 @@ static int wait_read(void)
>  	u8 status;
>  	int us;
>  	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {

I wonder if it would make sense to keep APPLESMC_MAX_WAIT as it is now
and replace the loop termination conditions with
				us <= APPLESMC_MAX_WAIT

That would accomplish the same, and APPLESMC_MAX_WAIT would reflect
the real maximum wait time.

Also, since the delay time can get quite large, would it make sense to replace
udelay with usleep_range() ?

Guenter

> -		udelay(us);
>  		status = inb(APPLESMC_CMD_PORT);
>  		/* read: wait for smc to settle */
>  		if (status & 0x01)
>  			return 0;
> +		udelay(us);
>  	}
>  
>  	pr_warn("wait_read() fail: 0x%02x\n", status);
> @@ -192,11 +192,12 @@ 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) {
> +			udelay(us);
>  			continue;
> +		}
>  		/* ready: cmd accepted, return */
>  		if (status & 0x04)
>  			return 0;
> 

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

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

[Guenter Roeck]

On Sat, Sep 15, 2012 at 06:42:30PM -0400, Parag Warudkar wrote:
> I have been getting a steady stream of wait_read timeouts on my 2010 MBP.
> 
> After playing around with various values of APPLESMC_MAX_WAIT a value of 
> 0x10000 reduces the wait_read failures to zero under most normal workloads 
> - with and without AC power plugged in, at idle and and at make -j4 loads.
> 
> While there I noticed we don't really need to udelay before first inb() - 
> so I moved it down to after first and subsequent failures.
> 
> Been running this for couple days without any issues.
> 
> Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..46cb458 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -56,7 +56,7 @@
>  /* wait up to 32 ms for a status change. */
>  #define APPLESMC_MIN_WAIT	0x0010
>  #define APPLESMC_RETRY_WAIT	0x0100
> -#define APPLESMC_MAX_WAIT	0x8000
> +#define APPLESMC_MAX_WAIT	0x10000
>  
>  #define APPLESMC_READ_CMD	0x10
>  #define APPLESMC_WRITE_CMD	0x11
> @@ -170,11 +170,11 @@ static int wait_read(void)
>  	u8 status;
>  	int us;
>  	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {

I wonder if it would make sense to keep APPLESMC_MAX_WAIT as it is now
and replace the loop termination conditions with
				us <= APPLESMC_MAX_WAIT

That would accomplish the same, and APPLESMC_MAX_WAIT would reflect
the real maximum wait time.

Also, since the delay time can get quite large, would it make sense to replace
udelay with usleep_range() ?

Guenter

> -		udelay(us);
>  		status = inb(APPLESMC_CMD_PORT);
>  		/* read: wait for smc to settle */
>  		if (status & 0x01)
>  			return 0;
> +		udelay(us);
>  	}
>  
>  	pr_warn("wait_read() fail: 0x%02x\n", status);
> @@ -192,11 +192,12 @@ 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) {
> +			udelay(us);
>  			continue;
> +		}
>  		/* ready: cmd accepted, return */
>  		if (status & 0x04)
>  			return 0;
> 

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

On Sat, 15 Sep 2012, Guenter Roeck wrote:

> On Sat, Sep 15, 2012 at 06:42:30PM -0400, Parag Warudkar wrote:
> > I have been getting a steady stream of wait_read timeouts on my 2010 MBP.
> > 
> > After playing around with various values of APPLESMC_MAX_WAIT a value of 
> > 0x10000 reduces the wait_read failures to zero under most normal workloads 
> > - with and without AC power plugged in, at idle and and at make -j4 loads.
> > 
> > While there I noticed we don't really need to udelay before first inb() - 
> > so I moved it down to after first and subsequent failures.
> > 
> > Been running this for couple days without any issues.
> > 
> > Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 2827088..46cb458 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -56,7 +56,7 @@
> >  /* wait up to 32 ms for a status change. */
> >  #define APPLESMC_MIN_WAIT	0x0010
> >  #define APPLESMC_RETRY_WAIT	0x0100
> > -#define APPLESMC_MAX_WAIT	0x8000
> > +#define APPLESMC_MAX_WAIT	0x10000
> >  
> >  #define APPLESMC_READ_CMD	0x10
> >  #define APPLESMC_WRITE_CMD	0x11
> > @@ -170,11 +170,11 @@ static int wait_read(void)
> >  	u8 status;
> >  	int us;
> >  	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> 
> I wonder if it would make sense to keep APPLESMC_MAX_WAIT as it is now
> and replace the loop termination conditions with
> 				us <= APPLESMC_MAX_WAIT
> 
> That would accomplish the same, and APPLESMC_MAX_WAIT would reflect
> the real maximum wait time.

Yeah, that would fix the code to match what it says and eliminate most 
failures. But in my testing sometimes it needed a max wait of 57344 us - 
I wrote a separate patch to instrument it by bumping up max wait in 
increments of 0x2000 until it no longer failed under my normal workloads. 

Under normal use 32us seemed to work but rarely when machine was 
slow - throttled, on  battery, on kernel build load etc.- the additional 
wait helped.

So I think it makes sense to fix the loop termination condition and also 
bump to max wait to 0x10000.

> 
> Also, since the delay time can get quite large, would it make sense to replace
> udelay with usleep_range() ?
> 

Yes, I think that would be a good thing to do. We could sleep in range of 
us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
factor that in for next loop iteration if necessary. Gotta think a bit on 
that one.

I will rework the patch to fix the loop termination and keep the bump to 
0x10000 in place and possibly also experiment with usleep_range().

Thanks,
Parag

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

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

[Parag Warudkar]

On Sat, 15 Sep 2012, Guenter Roeck wrote:

> On Sat, Sep 15, 2012 at 06:42:30PM -0400, Parag Warudkar wrote:
> > I have been getting a steady stream of wait_read timeouts on my 2010 MBP.
> > 
> > After playing around with various values of APPLESMC_MAX_WAIT a value of 
> > 0x10000 reduces the wait_read failures to zero under most normal workloads 
> > - with and without AC power plugged in, at idle and and at make -j4 loads.
> > 
> > While there I noticed we don't really need to udelay before first inb() - 
> > so I moved it down to after first and subsequent failures.
> > 
> > Been running this for couple days without any issues.
> > 
> > Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 2827088..46cb458 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -56,7 +56,7 @@
> >  /* wait up to 32 ms for a status change. */
> >  #define APPLESMC_MIN_WAIT	0x0010
> >  #define APPLESMC_RETRY_WAIT	0x0100
> > -#define APPLESMC_MAX_WAIT	0x8000
> > +#define APPLESMC_MAX_WAIT	0x10000
> >  
> >  #define APPLESMC_READ_CMD	0x10
> >  #define APPLESMC_WRITE_CMD	0x11
> > @@ -170,11 +170,11 @@ static int wait_read(void)
> >  	u8 status;
> >  	int us;
> >  	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> 
> I wonder if it would make sense to keep APPLESMC_MAX_WAIT as it is now
> and replace the loop termination conditions with
> 				us <= APPLESMC_MAX_WAIT
> 
> That would accomplish the same, and APPLESMC_MAX_WAIT would reflect
> the real maximum wait time.

Yeah, that would fix the code to match what it says and eliminate most 
failures. But in my testing sometimes it needed a max wait of 57344 us - 
I wrote a separate patch to instrument it by bumping up max wait in 
increments of 0x2000 until it no longer failed under my normal workloads. 

Under normal use 32us seemed to work but rarely when machine was 
slow - throttled, on  battery, on kernel build load etc.- the additional 
wait helped.

So I think it makes sense to fix the loop termination condition and also 
bump to max wait to 0x10000.

> 
> Also, since the delay time can get quite large, would it make sense to replace
> udelay with usleep_range() ?
> 

Yes, I think that would be a good thing to do. We could sleep in range of 
us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
factor that in for next loop iteration if necessary. Gotta think a bit on 
that one.

I will rework the patch to fix the loop termination and keep the bump to 
0x10000 in place and possibly also experiment with usleep_range().

Thanks,
Parag

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

On Sat, 15 Sep 2012, Parag Warudkar wrote:

> 
> Yeah, that would fix the code to match what it says and eliminate most 
> failures. But in my testing sometimes it needed a max wait of 57344 us - 
> I wrote a separate patch to instrument it by bumping up max wait in 
> increments of 0x2000 until it no longer failed under my normal workloads. 

Of course I meant 57 us instead of 57344 which I remembered from my 
unconverted debug printks.

Parag

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

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

[Parag Warudkar]

On Sat, 15 Sep 2012, Parag Warudkar wrote:

> 
> Yeah, that would fix the code to match what it says and eliminate most 
> failures. But in my testing sometimes it needed a max wait of 57344 us - 
> I wrote a separate patch to instrument it by bumping up max wait in 
> increments of 0x2000 until it no longer failed under my normal workloads. 

Of course I meant 57 us instead of 57344 which I remembered from my 
unconverted debug printks.

Parag

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

On Sat, 15 Sep 2012, Parag Warudkar wrote:

> 
> 
> On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > 
> > Also, since the delay time can get quite large, would it make sense to replace
> > udelay with usleep_range() ?
> > 
> 
> Yes, I think that would be a good thing to do. We could sleep in range of 
> us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
> factor that in for next loop iteration if necessary. Gotta think a bit on 
> that one.
> 
> I will rework the patch to fix the loop termination and keep the bump to 
> 0x10000 in place and possibly also experiment with usleep_range().
> 

Below is what I am experimenting with right now. I chose to keep the 
APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop 
termination and use of usleep_range() instead of udelay - I will try to 
run this a couple days and see if I can recreate the failure within 32ms.

Does this look ok? (I haven't yet changed send_byte to use usleep_range - 
if this approach looks ok I can change that as well.)

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..6610037 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -168,15 +168,20 @@ 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);
+	unsigned long r1_us = APPLESMC_MIN_WAIT;
+	unsigned long r2_us;
+	do {
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
-	}
-
+		r2_us = r1_us << 2;
+		if (r2_us > APPLESMC_MAX_WAIT)
+			goto fail;
+		usleep_range(r1_us, r2_us);
+		r1_us = r2_us;
+	} while (1);
+fail:
 	pr_warn("wait_read() fail: 0x%02x\n", status);
 	return -EIO;
 }
@@ -191,12 +196,13 @@ static int send_byte(u8 cmd, u16 port)
 	int us;
 
 	outb(cmd, port);
-	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);
 		/* write: wait for smc to settle */
-		if (status & 0x02)
+		if (status & 0x02) {
+			udelay(us);
 			continue;
+		}
 		/* ready: cmd accepted, return */
 		if (status & 0x04)
 			return 0;

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

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

[Parag Warudkar]

On Sat, 15 Sep 2012, Parag Warudkar wrote:

> 
> 
> On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > 
> > Also, since the delay time can get quite large, would it make sense to replace
> > udelay with usleep_range() ?
> > 
> 
> Yes, I think that would be a good thing to do. We could sleep in range of 
> us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
> factor that in for next loop iteration if necessary. Gotta think a bit on 
> that one.
> 
> I will rework the patch to fix the loop termination and keep the bump to 
> 0x10000 in place and possibly also experiment with usleep_range().
> 

Below is what I am experimenting with right now. I chose to keep the 
APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop 
termination and use of usleep_range() instead of udelay - I will try to 
run this a couple days and see if I can recreate the failure within 32ms.

Does this look ok? (I haven't yet changed send_byte to use usleep_range - 
if this approach looks ok I can change that as well.)

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..6610037 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -168,15 +168,20 @@ 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);
+	unsigned long r1_us = APPLESMC_MIN_WAIT;
+	unsigned long r2_us;
+	do {
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
-	}
-
+		r2_us = r1_us << 2;
+		if (r2_us > APPLESMC_MAX_WAIT)
+			goto fail;
+		usleep_range(r1_us, r2_us);
+		r1_us = r2_us;
+	} while (1);
+fail:
 	pr_warn("wait_read() fail: 0x%02x\n", status);
 	return -EIO;
 }
@@ -191,12 +196,13 @@ static int send_byte(u8 cmd, u16 port)
 	int us;
 
 	outb(cmd, port);
-	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);
 		/* write: wait for smc to settle */
-		if (status & 0x02)
+		if (status & 0x02) {
+			udelay(us);
 			continue;
+		}
 		/* ready: cmd accepted, return */
 		if (status & 0x04)
 			return 0;

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Guenter Roeck]

On Sat, Sep 15, 2012 at 11:29:58PM -0400, Parag Warudkar wrote:
> 
> 
> On Sat, 15 Sep 2012, Parag Warudkar wrote:
> 
> > 
> > 
> > On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > > 
> > > Also, since the delay time can get quite large, would it make sense to replace
> > > udelay with usleep_range() ?
> > > 
> > 
> > Yes, I think that would be a good thing to do. We could sleep in range of 
> > us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
> > factor that in for next loop iteration if necessary. Gotta think a bit on 
> > that one.
> > 
> > I will rework the patch to fix the loop termination and keep the bump to 
> > 0x10000 in place and possibly also experiment with usleep_range().
> > 
> 
> Below is what I am experimenting with right now. I chose to keep the 
> APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop 
> termination and use of usleep_range() instead of udelay - I will try to 
> run this a couple days and see if I can recreate the failure within 32ms.
> 
> Does this look ok? (I haven't yet changed send_byte to use usleep_range - 
> if this approach looks ok I can change that as well.)
> 
> Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..6610037 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -168,15 +168,20 @@ 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);
> +	unsigned long r1_us = APPLESMC_MIN_WAIT;
> +	unsigned long r2_us;
> +	do {
>  		status = inb(APPLESMC_CMD_PORT);
>  		/* read: wait for smc to settle */
>  		if (status & 0x01)
>  			return 0;
> -	}
> -
> +		r2_us = r1_us << 2;
> +		if (r2_us > APPLESMC_MAX_WAIT)
> +			goto fail;
> +		usleep_range(r1_us, r2_us);
> +		r1_us = r2_us;

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);

Guenter

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

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

[Guenter Roeck]

On Sat, Sep 15, 2012 at 11:29:58PM -0400, Parag Warudkar wrote:
> 
> 
> On Sat, 15 Sep 2012, Parag Warudkar wrote:
> 
> > 
> > 
> > On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > > 
> > > Also, since the delay time can get quite large, would it make sense to replace
> > > udelay with usleep_range() ?
> > > 
> > 
> > Yes, I think that would be a good thing to do. We could sleep in range of 
> > us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
> > factor that in for next loop iteration if necessary. Gotta think a bit on 
> > that one.
> > 
> > I will rework the patch to fix the loop termination and keep the bump to 
> > 0x10000 in place and possibly also experiment with usleep_range().
> > 
> 
> Below is what I am experimenting with right now. I chose to keep the 
> APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop 
> termination and use of usleep_range() instead of udelay - I will try to 
> run this a couple days and see if I can recreate the failure within 32ms.
> 
> Does this look ok? (I haven't yet changed send_byte to use usleep_range - 
> if this approach looks ok I can change that as well.)
> 
> Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..6610037 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -168,15 +168,20 @@ 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);
> +	unsigned long r1_us = APPLESMC_MIN_WAIT;
> +	unsigned long r2_us;
> +	do {
>  		status = inb(APPLESMC_CMD_PORT);
>  		/* read: wait for smc to settle */
>  		if (status & 0x01)
>  			return 0;
> -	}
> -
> +		r2_us = r1_us << 2;
> +		if (r2_us > APPLESMC_MAX_WAIT)
> +			goto fail;
> +		usleep_range(r1_us, r2_us);
> +		r1_us = r2_us;

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);

Guenter

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

On Sat, Sep 15, 2012 at 09:31:16PM -0700, Guenter Roeck wrote:
> On Sat, Sep 15, 2012 at 11:29:58PM -0400, Parag Warudkar wrote:
> > 
> > 
> > On Sat, 15 Sep 2012, Parag Warudkar wrote:
> > 
> > > 
> > > 
> > > On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > > > 
> > > > Also, since the delay time can get quite large, would it make sense to replace
> > > > udelay with usleep_range() ?
> > > > 
> > > 
> > > Yes, I think that would be a good thing to do. We could sleep in range of 
> > > us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
> > > factor that in for next loop iteration if necessary. Gotta think a bit on 
> > > that one.
> > > 
> > > I will rework the patch to fix the loop termination and keep the bump to 
> > > 0x10000 in place and possibly also experiment with usleep_range().
> > > 
> > 
> > Below is what I am experimenting with right now. I chose to keep the 
> > APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop 
> > termination and use of usleep_range() instead of udelay - I will try to 
> > run this a couple days and see if I can recreate the failure within 32ms.
> > 
> > Does this look ok? (I haven't yet changed send_byte to use usleep_range - 
> > if this approach looks ok I can change that as well.)
> > 
> > Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 2827088..6610037 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -168,15 +168,20 @@ 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);
> > +	unsigned long r1_us = APPLESMC_MIN_WAIT;
> > +	unsigned long r2_us;
> > +	do {
> >  		status = inb(APPLESMC_CMD_PORT);
> >  		/* read: wait for smc to settle */
> >  		if (status & 0x01)
> >  			return 0;
> > -	}
> > -
> > +		r2_us = r1_us << 2;
> > +		if (r2_us > APPLESMC_MAX_WAIT)
> > +			goto fail;
> > +		usleep_range(r1_us, r2_us);
> > +		r1_us = r2_us;
> 
> 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);

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.

As for the rearrangement to remove the initial wait time, it is quite
possible that we can simply reduce APPLESMC_MIN_WAIT to zero-ish,
provided we understand the status bits that tell us when to actually
wait.

Thanks,
Henrik

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

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

[Henrik Rydberg]

On Sat, Sep 15, 2012 at 09:31:16PM -0700, Guenter Roeck wrote:
> On Sat, Sep 15, 2012 at 11:29:58PM -0400, Parag Warudkar wrote:
> > 
> > 
> > On Sat, 15 Sep 2012, Parag Warudkar wrote:
> > 
> > > 
> > > 
> > > On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > > > 
> > > > Also, since the delay time can get quite large, would it make sense to replace
> > > > udelay with usleep_range() ?
> > > > 
> > > 
> > > Yes, I think that would be a good thing to do. We could sleep in range of 
> > > us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can 
> > > factor that in for next loop iteration if necessary. Gotta think a bit on 
> > > that one.
> > > 
> > > I will rework the patch to fix the loop termination and keep the bump to 
> > > 0x10000 in place and possibly also experiment with usleep_range().
> > > 
> > 
> > Below is what I am experimenting with right now. I chose to keep the 
> > APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop 
> > termination and use of usleep_range() instead of udelay - I will try to 
> > run this a couple days and see if I can recreate the failure within 32ms.
> > 
> > Does this look ok? (I haven't yet changed send_byte to use usleep_range - 
> > if this approach looks ok I can change that as well.)
> > 
> > Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 2827088..6610037 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -168,15 +168,20 @@ 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);
> > +	unsigned long r1_us = APPLESMC_MIN_WAIT;
> > +	unsigned long r2_us;
> > +	do {
> >  		status = inb(APPLESMC_CMD_PORT);
> >  		/* read: wait for smc to settle */
> >  		if (status & 0x01)
> >  			return 0;
> > -	}
> > -
> > +		r2_us = r1_us << 2;
> > +		if (r2_us > APPLESMC_MAX_WAIT)
> > +			goto fail;
> > +		usleep_range(r1_us, r2_us);
> > +		r1_us = r2_us;
> 
> 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);

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.

As for the rearrangement to remove the initial wait time, it is quite
possible that we can simply reduce APPLESMC_MIN_WAIT to zero-ish,
provided we understand the status bits that tell us when to actually
wait.

Thanks,
Henrik

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

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@gmail.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);
 	}
 
 

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

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

[Parag Warudkar]

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@gmail.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);
 	}
 
 

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Guenter Roeck]

On Sun, Sep 16, 2012 at 05:22:21PM -0400, Parag Warudkar wrote:
> 
> 
> 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@gmail.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);
>  
No difference to the original for loop, so might as well keep it.

Also, the problem with usleep_range() after the status read is that you end up
waiting just to abort which doesn't really make sense.

>  	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);

For the last loop iteration, this sleep doesn't provide any value
and just delays the inevitable error return.

>  			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) {

With this change, you never get here: us is at most APPLESMC_MAX_WAIT/2 and
us << 1 is thus never larger than APPLESMC_MAX_WAIT. Did you want to change
"us < APPLESMC_MAX_WAIT" to "us <= APPLESMC_MAX_WAIT" above ?

> +			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);
>  	}
>  
>  
> 

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

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

[Guenter Roeck]

On Sun, Sep 16, 2012 at 05:22:21PM -0400, Parag Warudkar wrote:
> 
> 
> 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@gmail.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);
>  
No difference to the original for loop, so might as well keep it.

Also, the problem with usleep_range() after the status read is that you end up
waiting just to abort which doesn't really make sense.

>  	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);

For the last loop iteration, this sleep doesn't provide any value
and just delays the inevitable error return.

>  			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) {

With this change, you never get here: us is at most APPLESMC_MAX_WAIT/2 and
us << 1 is thus never larger than APPLESMC_MAX_WAIT. Did you want to change
"us < APPLESMC_MAX_WAIT" to "us <= APPLESMC_MAX_WAIT" above ?

> +			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);
>  	}
>  
>  
> 

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

Hi Parag,

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

What exact model is this?

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?

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.

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.

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

Well, I am satisfied once I know the patch will work on all supported
models.

Thanks.
Henrik

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

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

[Henrik Rydberg]

Hi Parag,

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

What exact model is this?

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?

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.

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.

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

Well, I am satisfied once I know the patch will work on all supported
models.

Thanks.
Henrik

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

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@gmail.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);
 	}
 

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

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

[Parag Warudkar]

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@gmail.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);
 	}
 

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

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

This is actually by construction; The total wait time after the loop
is done is roughly 2^MAX_WAIT_TIME.

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

The current patch does exactly the same sleeps, the only difference is
that the test is also done before the first sleep. Thus, the increased
delay, if any, comes from the sleep range.

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

The MBP10,1 experiences a lot of write errors with this patch.

> 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);

Net result: new delay function and one more status test.

>  	}
>  
>  	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);
>  	}
>  

Causes (lots of) write errors on MBP10,1.

Henrik

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

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

[Henrik Rydberg]

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

This is actually by construction; The total wait time after the loop
is done is roughly 2^MAX_WAIT_TIME.

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

The current patch does exactly the same sleeps, the only difference is
that the test is also done before the first sleep. Thus, the increased
delay, if any, comes from the sleep range.

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

The MBP10,1 experiences a lot of write errors with this patch.

> 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);

Net result: new delay function and one more status test.

>  	}
>  
>  	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);
>  	}
>  

Causes (lots of) write errors on MBP10,1.

Henrik

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Guenter Roeck]

On Mon, Sep 17, 2012 at 06:27:05PM +0200, Henrik Rydberg wrote:
> > > 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.
> 
> This is actually by construction; The total wait time after the loop
> is done is roughly 2^MAX_WAIT_TIME.
> 
> > 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.
> 
> The current patch does exactly the same sleeps, the only difference is
> that the test is also done before the first sleep. Thus, the increased
> delay, if any, comes from the sleep range.
> 
> > > 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.
> 
> The MBP10,1 experiences a lot of write errors with this patch.
> 
> > 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);
> 
> Net result: new delay function and one more status test.
> 
> >  	}
> >  
> >  	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);
> >  	}
> >  
> 
> Causes (lots of) write errors on MBP10,1.
> 
So this is obviously a no-go. My wild guess is that MBP10,1 doesn't like it that
the initial wait time is removed, or something happens during the usleep_range.

Any better/other ideas ? Is the problem that we have to increase the wait time,
or is something else going on ?

Guenter

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

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

[Guenter Roeck]

On Mon, Sep 17, 2012 at 06:27:05PM +0200, Henrik Rydberg wrote:
> > > 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.
> 
> This is actually by construction; The total wait time after the loop
> is done is roughly 2^MAX_WAIT_TIME.
> 
> > 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.
> 
> The current patch does exactly the same sleeps, the only difference is
> that the test is also done before the first sleep. Thus, the increased
> delay, if any, comes from the sleep range.
> 
> > > 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.
> 
> The MBP10,1 experiences a lot of write errors with this patch.
> 
> > 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);
> 
> Net result: new delay function and one more status test.
> 
> >  	}
> >  
> >  	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);
> >  	}
> >  
> 
> Causes (lots of) write errors on MBP10,1.
> 
So this is obviously a no-go. My wild guess is that MBP10,1 doesn't like it that
the initial wait time is removed, or something happens during the usleep_range.

Any better/other ideas ? Is the problem that we have to increase the wait time,
or is something else going on ?

Guenter

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

> Any better/other ideas ? Is the problem that we have to increase the wait time,
> or is something else going on ?

No, it makes sense now that I got the right number of zeroes. ;-)

So, to be explicit, this is the patch I would like to go in. It is
completely safe, back-portable, and a no-brainer. Parag, are you ok
with being the author on this one?

Thanks,
Henrik

From 868fc42edcb3a338ce0a0334e2dc50b01a7f6844 Mon Sep 17 00:00:00 2001
From: Parag Warudkar <parag.lkml@gmail.com>
Date: Mon, 17 Sep 2012 17:49:55 +0200
Subject: [PATCH] applesmc: Bump max wait

A heavy-load test on a MacBookPro6,1 is still showing a substantial
amount of read errors.  Increasing the maximum wait time to 128 ms
resolves the issue.

maybe-Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..8f3f6f2 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -53,10 +53,10 @@
 
 #define APPLESMC_MAX_DATA_LENGTH 32
 
-/* wait up to 32 ms for a status change. */
+/* wait up to 128 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
-#define APPLESMC_MAX_WAIT	0x8000
+#define APPLESMC_MAX_WAIT	0x20000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
-- 
1.7.12


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

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

[Henrik Rydberg]

> Any better/other ideas ? Is the problem that we have to increase the wait time,
> or is something else going on ?

No, it makes sense now that I got the right number of zeroes. ;-)

So, to be explicit, this is the patch I would like to go in. It is
completely safe, back-portable, and a no-brainer. Parag, are you ok
with being the author on this one?

Thanks,
Henrik

>From 868fc42edcb3a338ce0a0334e2dc50b01a7f6844 Mon Sep 17 00:00:00 2001
From: Parag Warudkar <parag.lkml@gmail.com>
Date: Mon, 17 Sep 2012 17:49:55 +0200
Subject: [PATCH] applesmc: Bump max wait

A heavy-load test on a MacBookPro6,1 is still showing a substantial
amount of read errors.  Increasing the maximum wait time to 128 ms
resolves the issue.

maybe-Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..8f3f6f2 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -53,10 +53,10 @@
 
 #define APPLESMC_MAX_DATA_LENGTH 32
 
-/* wait up to 32 ms for a status change. */
+/* wait up to 128 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
-#define APPLESMC_MAX_WAIT	0x8000
+#define APPLESMC_MAX_WAIT	0x20000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
-- 
1.7.12

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Guenter Roeck]

On Mon, Sep 17, 2012 at 10:14:36PM +0200, Henrik Rydberg wrote:
> > Any better/other ideas ? Is the problem that we have to increase the wait time,
> > or is something else going on ?
> 
> No, it makes sense now that I got the right number of zeroes. ;-)
> 
> So, to be explicit, this is the patch I would like to go in. It is
> completely safe, back-portable, and a no-brainer. Parag, are you ok
> with being the author on this one?
> 
> Thanks,
> Henrik
> 
> From 868fc42edcb3a338ce0a0334e2dc50b01a7f6844 Mon Sep 17 00:00:00 2001
> From: Parag Warudkar <parag.lkml@gmail.com>
> Date: Mon, 17 Sep 2012 17:49:55 +0200
> Subject: [PATCH] applesmc: Bump max wait
> 
> A heavy-load test on a MacBookPro6,1 is still showing a substantial
> amount of read errors.  Increasing the maximum wait time to 128 ms
> resolves the issue.
> 
> maybe-Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Applied after s/maybe-//.

Thanks,
Guenter

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

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

[Guenter Roeck]

On Mon, Sep 17, 2012 at 10:14:36PM +0200, Henrik Rydberg wrote:
> > Any better/other ideas ? Is the problem that we have to increase the wait time,
> > or is something else going on ?
> 
> No, it makes sense now that I got the right number of zeroes. ;-)
> 
> So, to be explicit, this is the patch I would like to go in. It is
> completely safe, back-portable, and a no-brainer. Parag, are you ok
> with being the author on this one?
> 
> Thanks,
> Henrik
> 
> From 868fc42edcb3a338ce0a0334e2dc50b01a7f6844 Mon Sep 17 00:00:00 2001
> From: Parag Warudkar <parag.lkml@gmail.com>
> Date: Mon, 17 Sep 2012 17:49:55 +0200
> Subject: [PATCH] applesmc: Bump max wait
> 
> A heavy-load test on a MacBookPro6,1 is still showing a substantial
> amount of read errors.  Increasing the maximum wait time to 128 ms
> resolves the issue.
> 
> maybe-Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Applied after s/maybe-//.

Thanks,
Guenter

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> The current patch does exactly the same sleeps, the only difference is
> that the test is also done before the first sleep. Thus, the increased
> delay, if any, comes from the sleep range.
My understanding is that the original patch resulted in trying a 
max udelay(16). The new one does usleep_range(16, 32). That's a minimum 16 
max 32 - so the fact that it stops read failures means the added delay is 
present and does help.

> 
> 
> The MBP10,1 experiences a lot of write errors with this patch.

Well the send_byte changes are not necessary for fixing my problem - I 
only converted it because of usleep_range() primarily. What happens if you 
drop the send_byte() part of the patch and test with only the wait_read() 
changes? I would be very surprised if that caused write or read failures. 


Parag

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

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

[Parag Warudkar]

On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> The current patch does exactly the same sleeps, the only difference is
> that the test is also done before the first sleep. Thus, the increased
> delay, if any, comes from the sleep range.
My understanding is that the original patch resulted in trying a 
max udelay(16). The new one does usleep_range(16, 32). That's a minimum 16 
max 32 - so the fact that it stops read failures means the added delay is 
present and does help.

> 
> 
> The MBP10,1 experiences a lot of write errors with this patch.

Well the send_byte changes are not necessary for fixing my problem - I 
only converted it because of usleep_range() primarily. What happens if you 
drop the send_byte() part of the patch and test with only the wait_read() 
changes? I would be very surprised if that caused write or read failures. 


Parag

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

On Mon, Sep 17, 2012 at 02:06:05PM -0400, Parag Warudkar wrote:
> 
> 
> On Mon, 17 Sep 2012, Henrik Rydberg wrote:
> 
> > The current patch does exactly the same sleeps, the only difference is
> > that the test is also done before the first sleep. Thus, the increased
> > delay, if any, comes from the sleep range.
> My understanding is that the original patch resulted in trying a 
> max udelay(16). The new one does usleep_range(16, 32). That's a minimum 16 
> max 32 - so the fact that it stops read failures means the added delay is 
> present and does help.

So the question is, does this patch work equally well for you?

Henrik

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..8bf9011 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -56,7 +56,7 @@
 /* wait up to 32 ms for a status change. */
 #define APPLESMC_MIN_WAIT      0x0010
 #define APPLESMC_RETRY_WAIT    0x0100
-#define APPLESMC_MAX_WAIT      0x8000
+#define APPLESMC_MAX_WAIT      0x10000
 
 #define APPLESMC_READ_CMD      0x10
 #define APPLESMC_WRITE_CMD     0x11

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

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

[Henrik Rydberg]

On Mon, Sep 17, 2012 at 02:06:05PM -0400, Parag Warudkar wrote:
> 
> 
> On Mon, 17 Sep 2012, Henrik Rydberg wrote:
> 
> > The current patch does exactly the same sleeps, the only difference is
> > that the test is also done before the first sleep. Thus, the increased
> > delay, if any, comes from the sleep range.
> My understanding is that the original patch resulted in trying a 
> max udelay(16). The new one does usleep_range(16, 32). That's a minimum 16 
> max 32 - so the fact that it stops read failures means the added delay is 
> present and does help.

So the question is, does this patch work equally well for you?

Henrik

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..8bf9011 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -56,7 +56,7 @@
 /* wait up to 32 ms for a status change. */
 #define APPLESMC_MIN_WAIT      0x0010
 #define APPLESMC_RETRY_WAIT    0x0100
-#define APPLESMC_MAX_WAIT      0x8000
+#define APPLESMC_MAX_WAIT      0x10000
 
 #define APPLESMC_READ_CMD      0x10
 #define APPLESMC_WRITE_CMD     0x11

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> So the question is, does this patch work equally well for you?
> 
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..8bf9011 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -56,7 +56,7 @@
>  /* wait up to 32 ms for a status change. */
>  #define APPLESMC_MIN_WAIT      0x0010
>  #define APPLESMC_RETRY_WAIT    0x0100
> -#define APPLESMC_MAX_WAIT      0x8000
> +#define APPLESMC_MAX_WAIT      0x10000
>  
>  #define APPLESMC_READ_CMD      0x10
>  #define APPLESMC_WRITE_CMD     0x11
> 

That was something that I tried originally and it still resulted in 
failures - albeit lesser in number.

I had to bump it to 0x20000 (effective wait of 65536) with the original 
loop termination logic and  udelay() in order to get it to reduce to 
zero.

Note that with usleep_range - each sleep is potentially more than the 
minimum - that's may be why 32ms works with usleep_range but not with 
udelay which is precise.

So bottomline, I suspect we will need to bump to 0x20000 if you want to 
keep the current loop termination and udelay().

Parag

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

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

[Parag Warudkar]

On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> So the question is, does this patch work equally well for you?
> 
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..8bf9011 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -56,7 +56,7 @@
>  /* wait up to 32 ms for a status change. */
>  #define APPLESMC_MIN_WAIT      0x0010
>  #define APPLESMC_RETRY_WAIT    0x0100
> -#define APPLESMC_MAX_WAIT      0x8000
> +#define APPLESMC_MAX_WAIT      0x10000
>  
>  #define APPLESMC_READ_CMD      0x10
>  #define APPLESMC_WRITE_CMD     0x11
> 

That was something that I tried originally and it still resulted in 
failures - albeit lesser in number.

I had to bump it to 0x20000 (effective wait of 65536) with the original 
loop termination logic and  udelay() in order to get it to reduce to 
zero.

Note that with usleep_range - each sleep is potentially more than the 
minimum - that's may be why 32ms works with usleep_range but not with 
udelay which is precise.

So bottomline, I suspect we will need to bump to 0x20000 if you want to 
keep the current loop termination and udelay().

Parag

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

On Mon, Sep 17, 2012 at 02:54:25PM -0400, Parag Warudkar wrote:
> 
> 
> On Mon, 17 Sep 2012, Henrik Rydberg wrote:
> 
> > So the question is, does this patch work equally well for you?
> > 
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 2827088..8bf9011 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -56,7 +56,7 @@
> >  /* wait up to 32 ms for a status change. */
> >  #define APPLESMC_MIN_WAIT      0x0010
> >  #define APPLESMC_RETRY_WAIT    0x0100
> > -#define APPLESMC_MAX_WAIT      0x8000
> > +#define APPLESMC_MAX_WAIT      0x10000
> >  
> >  #define APPLESMC_READ_CMD      0x10
> >  #define APPLESMC_WRITE_CMD     0x11
> > 
> 
> That was something that I tried originally and it still resulted in 
> failures - albeit lesser in number.

Good, just wanted to touch base here, so we know what we are doing.

> I had to bump it to 0x20000 (effective wait of 65536) with the original 
> loop termination logic and  udelay() in order to get it to reduce to 
> zero.
> 
> Note that with usleep_range - each sleep is potentially more than the 
> minimum - that's may be why 32ms works with usleep_range but not with 
> udelay which is precise.

Yes, so if you also change APPLESMC_MIN_WAIT to 0x0020, what happens?

> So bottomline, I suspect we will need to bump to 0x20000 if you want to 
> keep the current loop termination and udelay().

That is just crazy, since your code works with a 32ms maximum.

Thanks,
Henrik

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

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

[Henrik Rydberg]

On Mon, Sep 17, 2012 at 02:54:25PM -0400, Parag Warudkar wrote:
> 
> 
> On Mon, 17 Sep 2012, Henrik Rydberg wrote:
> 
> > So the question is, does this patch work equally well for you?
> > 
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 2827088..8bf9011 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -56,7 +56,7 @@
> >  /* wait up to 32 ms for a status change. */
> >  #define APPLESMC_MIN_WAIT      0x0010
> >  #define APPLESMC_RETRY_WAIT    0x0100
> > -#define APPLESMC_MAX_WAIT      0x8000
> > +#define APPLESMC_MAX_WAIT      0x10000
> >  
> >  #define APPLESMC_READ_CMD      0x10
> >  #define APPLESMC_WRITE_CMD     0x11
> > 
> 
> That was something that I tried originally and it still resulted in 
> failures - albeit lesser in number.

Good, just wanted to touch base here, so we know what we are doing.

> I had to bump it to 0x20000 (effective wait of 65536) with the original 
> loop termination logic and  udelay() in order to get it to reduce to 
> zero.
> 
> Note that with usleep_range - each sleep is potentially more than the 
> minimum - that's may be why 32ms works with usleep_range but not with 
> udelay which is precise.

Yes, so if you also change APPLESMC_MIN_WAIT to 0x0020, what happens?

> So bottomline, I suspect we will need to bump to 0x20000 if you want to 
> keep the current loop termination and udelay().

That is just crazy, since your code works with a 32ms maximum.

Thanks,
Henrik

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Henrik Rydberg]

> > So bottomline, I suspect we will need to bump to 0x20000 if you want to 
> > keep the current loop termination and udelay().
> 
> That is just crazy, since your code works with a 32ms maximum.

I am sorry, I misread the number of zeroes here. If you are saying
that fours times the current number is enough for the error rate to
drop off on your machine, then I think we should just do that. It is
the only change I can think of that is safe enough at this stage.

Given that the irqsoff latency on a normal machine can be as high as
200 ms or so, it does not seem all that strange.

Thanks,
Henrik

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

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

[Henrik Rydberg]

> > So bottomline, I suspect we will need to bump to 0x20000 if you want to 
> > keep the current loop termination and udelay().
> 
> That is just crazy, since your code works with a 32ms maximum.

I am sorry, I misread the number of zeroes here. If you are saying
that fours times the current number is enough for the error rate to
drop off on your machine, then I think we should just do that. It is
the only change I can think of that is safe enough at this stage.

Given that the irqsoff latency on a normal machine can be as high as
200 ms or so, it does not seem all that strange.

Thanks,
Henrik

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> The MBP10,1 experiences a lot of write errors with this patch.

I just noticed a single write failure - are you seeing something similar?

[ 1660.362997] applesmc: send_byte(0x00, 0x0300) fail: 0x00
[ 1660.363002] applesmc: LKSB: write data fail

Since the write fails are confirmed, I've locally dropped the send_byte 
changes and just kept the wait_read ones that haven't caused me any 
trouble yet.

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..2ba298a 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -169,14 +169,14 @@ 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);
 	return -EIO;
 }

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

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

[Parag Warudkar]

On Mon, 17 Sep 2012, Henrik Rydberg wrote:

> The MBP10,1 experiences a lot of write errors with this patch.

I just noticed a single write failure - are you seeing something similar?

[ 1660.362997] applesmc: send_byte(0x00, 0x0300) fail: 0x00
[ 1660.363002] applesmc: LKSB: write data fail

Since the write fails are confirmed, I've locally dropped the send_byte 
changes and just kept the wait_read ones that haven't caused me any 
trouble yet.

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 2827088..2ba298a 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -169,14 +169,14 @@ 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);
 	return -EIO;
 }

Re: [lm-sensors] [PATCH] applesmc: Bump max wait and rearrange udelay

[Parag Warudkar]

[-- Attachment #1.1: Type: text/plain, Size: 1765 bytes --]

On Monday, September 17, 2012, Henrik Rydberg <rydberg@euromail.se> wrote:
>> Any better/other ideas ? Is the problem that we have to increase the
wait time,
>> or is something else going on ?
>
> No, it makes sense now that I got the right number of zeroes. ;-)
>
> So, to be explicit, this is the patch I would like to go in. It is
> completely safe, back-portable, and a no-brainer. Parag, are you ok
> with being the author on this one?
>

I am OK with the proposed timeout as it works for me. I'll add an

Tested-by: Parag Warudkar <parag.lkml@gmail.com>

Thanks :)

Parag
> Thanks,
> Henrik
>
> From 868fc42edcb3a338ce0a0334e2dc50b01a7f6844 Mon Sep 17 00:00:00 2001
> From: Parag Warudkar <parag.lkml@gmail.com>
> Date: Mon, 17 Sep 2012 17:49:55 +0200
> Subject: [PATCH] applesmc: Bump max wait
>
> A heavy-load test on a MacBookPro6,1 is still showing a substantial
> amount of read errors.  Increasing the maximum wait time to 128 ms
> resolves the issue.
>
> maybe-Signed-off-by: Parag Warudkar <parag.lkml@gmail.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..8f3f6f2 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -53,10 +53,10 @@
>
>  #define APPLESMC_MAX_DATA_LENGTH 32
>
> -/* wait up to 32 ms for a status change. */
> +/* wait up to 128 ms for a status change. */
>  #define APPLESMC_MIN_WAIT      0x0010
>  #define APPLESMC_RETRY_WAIT    0x0100
> -#define APPLESMC_MAX_WAIT      0x8000
> +#define APPLESMC_MAX_WAIT      0x20000
>
>  #define APPLESMC_READ_CMD      0x10
>  #define APPLESMC_WRITE_CMD     0x11
> --
> 1.7.12
>
>

[-- Attachment #1.2: Type: text/html, Size: 2352 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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