* [PATCH] applesmc: Bump max wait and rearrange udelay @ 2012-09-15 22:42 Parag Warudkar 2012-09-15 22:58 ` Guenter Roeck 0 siblings, 1 reply; 21+ messages in thread From: Parag Warudkar @ 2012-09-15 22:42 UTC (permalink / raw) To: lm-sensors, linux-kernel; +Cc: rydberg, khali, linux 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; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-15 22:42 [PATCH] applesmc: Bump max wait and rearrange udelay Parag Warudkar @ 2012-09-15 22:58 ` Guenter Roeck 2012-09-15 23:35 ` Parag Warudkar 0 siblings, 1 reply; 21+ messages in thread From: Guenter Roeck @ 2012-09-15 22:58 UTC (permalink / raw) To: Parag Warudkar; +Cc: lm-sensors, linux-kernel, rydberg, khali 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; > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-15 22:58 ` Guenter Roeck @ 2012-09-15 23:35 ` Parag Warudkar 2012-09-15 23:38 ` Parag Warudkar 2012-09-16 3:29 ` Parag Warudkar 0 siblings, 2 replies; 21+ messages in thread From: Parag Warudkar @ 2012-09-15 23:35 UTC (permalink / raw) To: Guenter Roeck; +Cc: Parag Warudkar, lm-sensors, linux-kernel, rydberg, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-15 23:35 ` Parag Warudkar @ 2012-09-15 23:38 ` Parag Warudkar 2012-09-16 3:29 ` Parag Warudkar 1 sibling, 0 replies; 21+ messages in thread From: Parag Warudkar @ 2012-09-15 23:38 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, rydberg, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-15 23:35 ` Parag Warudkar 2012-09-15 23:38 ` Parag Warudkar @ 2012-09-16 3:29 ` Parag Warudkar 2012-09-16 4:31 ` Guenter Roeck 1 sibling, 1 reply; 21+ messages in thread From: Parag Warudkar @ 2012-09-16 3:29 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, rydberg, khali 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; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-16 3:29 ` Parag Warudkar @ 2012-09-16 4:31 ` Guenter Roeck 2012-09-16 9:35 ` Henrik Rydberg 0 siblings, 1 reply; 21+ messages in thread From: Guenter Roeck @ 2012-09-16 4:31 UTC (permalink / raw) To: Parag Warudkar; +Cc: lm-sensors, linux-kernel, rydberg, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-16 4:31 ` Guenter Roeck @ 2012-09-16 9:35 ` Henrik Rydberg 2012-09-16 21:22 ` Parag Warudkar 0 siblings, 1 reply; 21+ messages in thread From: Henrik Rydberg @ 2012-09-16 9:35 UTC (permalink / raw) To: Guenter Roeck; +Cc: Parag Warudkar, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-16 9:35 ` Henrik Rydberg @ 2012-09-16 21:22 ` Parag Warudkar 2012-09-16 22:00 ` Guenter Roeck 2012-09-16 22:30 ` Henrik Rydberg 0 siblings, 2 replies; 21+ messages in thread From: Parag Warudkar @ 2012-09-16 21:22 UTC (permalink / raw) To: Henrik Rydberg Cc: Guenter Roeck, Parag Warudkar, lm-sensors, linux-kernel, khali 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); } ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-16 21:22 ` Parag Warudkar @ 2012-09-16 22:00 ` Guenter Roeck 2012-09-16 22:30 ` Henrik Rydberg 1 sibling, 0 replies; 21+ messages in thread From: Guenter Roeck @ 2012-09-16 22:00 UTC (permalink / raw) To: Parag Warudkar; +Cc: Henrik Rydberg, lm-sensors, linux-kernel, khali 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); > } > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-16 21:22 ` Parag Warudkar 2012-09-16 22:00 ` Guenter Roeck @ 2012-09-16 22:30 ` Henrik Rydberg 2012-09-17 0:11 ` Parag Warudkar 1 sibling, 1 reply; 21+ messages in thread From: Henrik Rydberg @ 2012-09-16 22:30 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-16 22:30 ` Henrik Rydberg @ 2012-09-17 0:11 ` Parag Warudkar 2012-09-17 16:27 ` Henrik Rydberg 0 siblings, 1 reply; 21+ messages in thread From: Parag Warudkar @ 2012-09-17 0:11 UTC (permalink / raw) To: Henrik Rydberg Cc: Parag Warudkar, Guenter Roeck, lm-sensors, linux-kernel, khali 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); } ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 0:11 ` Parag Warudkar @ 2012-09-17 16:27 ` Henrik Rydberg 2012-09-17 16:37 ` Guenter Roeck ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Henrik Rydberg @ 2012-09-17 16:27 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, khali > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 16:27 ` Henrik Rydberg @ 2012-09-17 16:37 ` Guenter Roeck 2012-09-17 20:14 ` Henrik Rydberg 2012-09-17 18:06 ` Parag Warudkar 2012-09-17 18:22 ` Parag Warudkar 2 siblings, 1 reply; 21+ messages in thread From: Guenter Roeck @ 2012-09-17 16:37 UTC (permalink / raw) To: Henrik Rydberg; +Cc: Parag Warudkar, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 16:37 ` Guenter Roeck @ 2012-09-17 20:14 ` Henrik Rydberg 2012-09-17 22:03 ` Guenter Roeck 0 siblings, 1 reply; 21+ messages in thread From: Henrik Rydberg @ 2012-09-17 20:14 UTC (permalink / raw) To: Guenter Roeck; +Cc: Parag Warudkar, lm-sensors, linux-kernel, khali > 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 20:14 ` Henrik Rydberg @ 2012-09-17 22:03 ` Guenter Roeck 0 siblings, 0 replies; 21+ messages in thread From: Guenter Roeck @ 2012-09-17 22:03 UTC (permalink / raw) To: Henrik Rydberg; +Cc: Parag Warudkar, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 16:27 ` Henrik Rydberg 2012-09-17 16:37 ` Guenter Roeck @ 2012-09-17 18:06 ` Parag Warudkar 2012-09-17 18:49 ` Henrik Rydberg 2012-09-17 18:22 ` Parag Warudkar 2 siblings, 1 reply; 21+ messages in thread From: Parag Warudkar @ 2012-09-17 18:06 UTC (permalink / raw) To: Henrik Rydberg Cc: Parag Warudkar, Guenter Roeck, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 18:06 ` Parag Warudkar @ 2012-09-17 18:49 ` Henrik Rydberg 2012-09-17 18:54 ` Parag Warudkar 0 siblings, 1 reply; 21+ messages in thread From: Henrik Rydberg @ 2012-09-17 18:49 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 18:49 ` Henrik Rydberg @ 2012-09-17 18:54 ` Parag Warudkar 2012-09-17 19:14 ` Henrik Rydberg 0 siblings, 1 reply; 21+ messages in thread From: Parag Warudkar @ 2012-09-17 18:54 UTC (permalink / raw) To: Henrik Rydberg Cc: Parag Warudkar, Guenter Roeck, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 18:54 ` Parag Warudkar @ 2012-09-17 19:14 ` Henrik Rydberg 2012-09-17 19:46 ` Henrik Rydberg 0 siblings, 1 reply; 21+ messages in thread From: Henrik Rydberg @ 2012-09-17 19:14 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, khali 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 19:14 ` Henrik Rydberg @ 2012-09-17 19:46 ` Henrik Rydberg 0 siblings, 0 replies; 21+ messages in thread From: Henrik Rydberg @ 2012-09-17 19:46 UTC (permalink / raw) To: Parag Warudkar; +Cc: Guenter Roeck, lm-sensors, linux-kernel, khali > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] applesmc: Bump max wait and rearrange udelay 2012-09-17 16:27 ` Henrik Rydberg 2012-09-17 16:37 ` Guenter Roeck 2012-09-17 18:06 ` Parag Warudkar @ 2012-09-17 18:22 ` Parag Warudkar 2 siblings, 0 replies; 21+ messages in thread From: Parag Warudkar @ 2012-09-17 18:22 UTC (permalink / raw) To: Henrik Rydberg Cc: Parag Warudkar, Guenter Roeck, lm-sensors, linux-kernel, khali 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; } ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-09-17 22:03 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-15 22:42 [PATCH] applesmc: Bump max wait and rearrange udelay Parag Warudkar 2012-09-15 22:58 ` Guenter Roeck 2012-09-15 23:35 ` Parag Warudkar 2012-09-15 23:38 ` Parag Warudkar 2012-09-16 3:29 ` Parag Warudkar 2012-09-16 4:31 ` Guenter Roeck 2012-09-16 9:35 ` Henrik Rydberg 2012-09-16 21:22 ` Parag Warudkar 2012-09-16 22:00 ` Guenter Roeck 2012-09-16 22:30 ` Henrik Rydberg 2012-09-17 0:11 ` Parag Warudkar 2012-09-17 16:27 ` Henrik Rydberg 2012-09-17 16:37 ` Guenter Roeck 2012-09-17 20:14 ` Henrik Rydberg 2012-09-17 22:03 ` Guenter Roeck 2012-09-17 18:06 ` Parag Warudkar 2012-09-17 18:49 ` Henrik Rydberg 2012-09-17 18:54 ` Parag Warudkar 2012-09-17 19:14 ` Henrik Rydberg 2012-09-17 19:46 ` Henrik Rydberg 2012-09-17 18:22 ` Parag Warudkar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).