This patch fixes a problem where my new powerbook would sometimes hang
or crash when changing CPU speed. We had schedule_timeout(HZ/1000) in
there, intended to provide a delay of one millisecond. However, even
with HZ=1000, it was (I believe) only waiting for the next jiffy
before proceeding, which could be less than a millisecond. Changing
the code to use msleep, and specifying a time of 1 jiffy + 1ms has
fixed the problem. (When I looked at the msleep code, it appeared to
me that msleep(1) with HZ=1000 would sleep for between 0 and 1ms.)
Ben also asked me to remove the code that changes the AACK delay
enable, after looking in the Darwin sources and seeing that Darwin
does not change this in its corresponding code.
Signed-off-by: Paul Mackerras <[email protected]>
diff -urN linux-2.5/arch/ppc/platforms/pmac_cpufreq.c pmac-2.5/arch/ppc/platforms/pmac_cpufreq.c
--- linux-2.5/arch/ppc/platforms/pmac_cpufreq.c 2004-09-24 15:23:06.000000000 +1000
+++ pmac-2.5/arch/ppc/platforms/pmac_cpufreq.c 2004-10-10 11:23:43.000000000 +1000
@@ -140,11 +140,8 @@
if (low_speed == 0) {
/* ramping up, set voltage first */
pmac_call_feature(PMAC_FTR_WRITE_GPIO, NULL, voltage_gpio, 0x05);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ/1000);
- } else {
- /* ramping down, enable aack delay first */
- pmac_call_feature(PMAC_FTR_AACK_DELAY_ENABLE, NULL, 1, 0);
+ /* Make sure we sleep for at least 1ms */
+ msleep(1 + jiffies_to_msecs(1));
}
/* set frequency */
@@ -153,11 +150,7 @@
if (low_speed == 1) {
/* ramping down, set voltage last */
pmac_call_feature(PMAC_FTR_WRITE_GPIO, NULL, voltage_gpio, 0x04);
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ/1000);
- } else {
- /* ramping up, disable aack delay last */
- pmac_call_feature(PMAC_FTR_AACK_DELAY_ENABLE, NULL, 0, 0);
+ msleep(1 + jiffies_to_msecs(1));
}
return 0;
Hi Paul,
On Sun, Oct 10, 2004 at 01:49:36PM +1000, Paul Mackerras wrote:
> This patch fixes a problem where my new powerbook would sometimes hang
> or crash when changing CPU speed. We had schedule_timeout(HZ/1000) in
> there, intended to provide a delay of one millisecond. However, even
> with HZ=1000, it was (I believe) only waiting for the next jiffy
> before proceeding, which could be less than a millisecond. Changing
> the code to use msleep, and specifying a time of 1 jiffy + 1ms has
> fixed the problem. (When I looked at the msleep code, it appeared to
> me that msleep(1) with HZ=1000 would sleep for between 0 and 1ms.)
<snip>
While looking through the latest bk changelogs, I noticed that you had
submitted this patch using msleep(). When I read the comment, though,
that you were offsetting the 1 millisecond with a jiffy, I was slightly
confused as msleep() is designed to sleep for *at least* the time
requested. So if you just use msleep(1) in these cases, you should have
the desired effect. msleep() is designed to be independent of HZ (as the
timeout is specified in non-jiffy units). Not using the
jiffies_to_msecs() macro would remove some extra instructions... The
attached patch makes this change (on top of your patch currently in bk7)
and also changes the other schedule_timeout()s (at least, those that can
be) to msleep.
-Nish
Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays as expected. Two of the changes are reworks of previous
msleep() calls which unnecessarily added a jiffy to the parameter.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.9-bk7-vanilla/arch/ppc/platforms/pmac_cpufreq.c 2004-10-22 10:41:49.000000000 -0700
+++ 2.6.9-bk7/arch/ppc/platforms/pmac_cpufreq.c 2004-10-22 10:57:03.000000000 -0700
@@ -141,7 +141,7 @@ static int __pmac dfs_set_cpu_speed(int
/* ramping up, set voltage first */
pmac_call_feature(PMAC_FTR_WRITE_GPIO, NULL, voltage_gpio, 0x05);
/* Make sure we sleep for at least 1ms */
- msleep(1 + jiffies_to_msecs(1));
+ msleep(1);
}
/* set frequency */
@@ -150,7 +150,7 @@ static int __pmac dfs_set_cpu_speed(int
if (low_speed == 1) {
/* ramping down, set voltage last */
pmac_call_feature(PMAC_FTR_WRITE_GPIO, NULL, voltage_gpio, 0x04);
- msleep(1 + jiffies_to_msecs(1));
+ msleep(1);
}
return 0;
@@ -167,8 +167,7 @@ static int __pmac gpios_set_cpu_speed(in
if (low_speed == 0) {
pmac_call_feature(PMAC_FTR_WRITE_GPIO, NULL, voltage_gpio, 0x05);
/* Delay is way too big but it's ok, we schedule */
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ/100);
+ msleep(10);
}
/* Set frequency */
@@ -185,8 +184,7 @@ static int __pmac gpios_set_cpu_speed(in
if (low_speed == 1) {
pmac_call_feature(PMAC_FTR_WRITE_GPIO, NULL, voltage_gpio, 0x04);
/* Delay is way too big but it's ok, we schedule */
- set_current_state(TASK_UNINTERRUPTIBLE);
- schedule_timeout(HZ/100);
+ msleep(10);
}
#ifdef DEBUG_FREQ
On Sat, 2004-10-23 at 04:01, Nishanth Aravamudan wrote:
> While looking through the latest bk changelogs, I noticed that you had
> submitted this patch using msleep(). When I read the comment, though,
> that you were offsetting the 1 millisecond with a jiffy, I was slightly
> confused as msleep() is designed to sleep for *at least* the time
> requested. So if you just use msleep(1) in these cases, you should have
> the desired effect. msleep() is designed to be independent of HZ (as the
> timeout is specified in non-jiffy units). Not using the
> jiffies_to_msecs() macro would remove some extra instructions... The
> attached patch makes this change (on top of your patch currently in bk7)
> and also changes the other schedule_timeout()s (at least, those that can
> be) to msleep.
No, please leave them as-is at least for now... Last we saw, there was
a potential issue with schedule_timeout(1) itself not guaranteeing it would
sleep for an entire jiffie, but only up to the next jiffie...
Ben.
On Sat, Oct 23, 2004 at 08:34:24AM +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2004-10-23 at 04:01, Nishanth Aravamudan wrote:
>
> > While looking through the latest bk changelogs, I noticed that you had
> > submitted this patch using msleep(). When I read the comment, though,
> > that you were offsetting the 1 millisecond with a jiffy, I was slightly
> > confused as msleep() is designed to sleep for *at least* the time
> > requested. So if you just use msleep(1) in these cases, you should have
> > the desired effect. msleep() is designed to be independent of HZ (as the
> > timeout is specified in non-jiffy units). Not using the
> > jiffies_to_msecs() macro would remove some extra instructions... The
> > attached patch makes this change (on top of your patch currently in bk7)
> > and also changes the other schedule_timeout()s (at least, those that can
> > be) to msleep.
>
> No, please leave them as-is at least for now... Last we saw, there was
> a potential issue with schedule_timeout(1) itself not guaranteeing it would
> sleep for an entire jiffie, but only up to the next jiffie...
Ah, ok. Sorry, I was not aware of this issue. Would you mind giving me
more details (off-list, if you'd prefer?).
Thanks,
Nish