LinuxLists.cc - APM driver patch summary

[permalink] [raw]

Subject: Re: APM driver patch summary

On Mon, Dec 17, 2001 at 01:28:13PM -0500, Thomas Hood wrote:
> Each of these changes is required, IMHO. However, the Russell King
> patch probably won't apply without modifications. Also, it needs to
> be modified so that it will send a resume event to listeners in case
> a driver rejects a suspend event that listeners have already
> processed.

It does do that - check the suspend() function.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2001-12-17 22:22:46

[permalink] [raw]

Subject: Re: APM driver patch summary

On Mon, Dec 17, 2001 at 10:04:02PM +0000, Russell King wrote:
> On Mon, Dec 17, 2001 at 01:28:13PM -0500, Thomas Hood wrote:
> > Each of these changes is required, IMHO. However, the Russell King
> > patch probably won't apply without modifications. Also, it needs to
> > be modified so that it will send a resume event to listeners in case
> > a driver rejects a suspend event that listeners have already
> > processed.
>
> It does do that - check the suspend() function.

Actually you're right, it doesn't send a resume event. However, It's
worse than that - it will leave all sleeping listeners still sleeping.

Try this patch instead - this should cause all listeners to get a -EIO
to indicate that the suspend failed, and also get the resume event.

Thanks for finding that. This (untested) patch should fix the problem.
(difference is in suspend()):

--- orig/arch/i386/kernel/apm.c Fri Nov 23 10:12:07 2001
+++ linux/arch/i386/kernel/apm.c Mon Dec 17 22:20:03 2001
@@ -1133,40 +1133,22 @@
#endif
}

-static int send_event(apm_event_t event)
-{
- switch (event) {
- case APM_SYS_SUSPEND:
- case APM_CRITICAL_SUSPEND:
- case APM_USER_SUSPEND:
- /* map all suspends to ACPI D3 */
- if (pm_send_all(PM_SUSPEND, (void *)3)) {
- if (event == APM_CRITICAL_SUSPEND) {
- printk(KERN_CRIT
- "apm: Critical suspend was vetoed, "
- "expect armageddon\n" );
- return 0;
- }
- if (apm_info.connection_version > 0x100)
- apm_set_power_state(APM_STATE_REJECT);
- return 0;
- }
- break;
- case APM_NORMAL_RESUME:
- case APM_CRITICAL_RESUME:
- /* map all resumes to ACPI D0 */
- (void) pm_send_all(PM_RESUME, (void *)0);
- break;
- }
-
- return 1;
-}
-
-static int suspend(void)
+static int suspend(int vetoable)
{
int err;
struct apm_user *as;

+ if (pm_send_all(PM_SUSPEND, (void *)3)) {
+ if (!vetoable) {
+ printk(KERN_CRIT
+ "apm: suspend was vetoed, expect armageddon\n");
+ } else if (apm_info.connection_version > 0x100) {
+ apm_set_power_state(APM_STATE_REJECT);
+ err = -EIO;
+ goto out;
+ }
+ }
+
get_time_diff();
cli();
err = apm_set_power_state(APM_STATE_SUSPEND);
@@ -1176,12 +1158,17 @@
err = APM_SUCCESS;
if (err != APM_SUCCESS)
apm_error("suspend", err);
- send_event(APM_NORMAL_RESUME);
sti();
+
+ pm_send_all(PM_RESUME, (void *)0);
+
+ err = (err == APM_SUCCESS) ? 0 : -EIO;
+
+ out:
queue_event(APM_NORMAL_RESUME, NULL);
for (as = user_list; as != NULL; as = as->next) {
as->suspend_wait = 0;
- as->suspend_result = ((err == APM_SUCCESS) ? 0 : -EIO);
+ as->suspend_result = err;
}
ignore_normal_resume = 1;
wake_up_interruptible(&apm_suspend_waitqueue);
@@ -1241,11 +1228,9 @@
switch (event) {
case APM_SYS_STANDBY:
case APM_USER_STANDBY:
- if (send_event(event)) {
- queue_event(event, NULL);
- if (standbys_pending <= 0)
- standby();
- }
+ queue_event(event, NULL);
+ if (standbys_pending <= 0)
+ standby();
break;

case APM_USER_SUSPEND:
@@ -1270,12 +1255,10 @@
*/
if (waiting_for_resume)
return;
- if (send_event(event)) {
- queue_event(event, NULL);
- waiting_for_resume = 1;
- if (suspends_pending <= 0)
- (void) suspend();
- }
+ queue_event(event, NULL);
+ waiting_for_resume = 1;
+ if (suspends_pending <= 0)
+ (void) suspend(1);
break;

case APM_NORMAL_RESUME:
@@ -1287,7 +1270,7 @@
if ((event != APM_NORMAL_RESUME)
|| (ignore_normal_resume == 0)) {
set_time();
- send_event(event);
+ pm_send_all(PM_RESUME, (void *)0);
queue_event(event, NULL);
}
break;
@@ -1295,7 +1278,6 @@
case APM_CAPABILITY_CHANGE:
case APM_LOW_BATTERY:
case APM_POWER_STATUS_CHANGE:
- send_event(event);
queue_event(event, NULL);
break;

@@ -1304,12 +1286,11 @@
break;

case APM_CRITICAL_SUSPEND:
- send_event(event);
/*
* We can only hope it worked - we are not allowed
* to reject a critical suspend.
*/
- (void) suspend();
+ (void) suspend(0);
break;
}
}
@@ -1479,9 +1460,7 @@
as->standbys_read--;
as->standbys_pending--;
standbys_pending--;
- } else if (!send_event(APM_USER_STANDBY))
- return -EAGAIN;
- else
+ } else
queue_event(APM_USER_STANDBY, as);
if (standbys_pending <= 0)
standby();
@@ -1491,13 +1470,10 @@
as->suspends_read--;
as->suspends_pending--;
suspends_pending--;
- } else if (!send_event(APM_USER_SUSPEND))
- return -EAGAIN;
- else
+ } else
queue_event(APM_USER_SUSPEND, as);
if (suspends_pending <= 0) {
- if (suspend() != APM_SUCCESS)
- return -EIO;
+ return suspend(1);
} else {
as->suspend_wait = 1;
wait_event_interruptible(apm_suspend_waitqueue,
@@ -1528,7 +1504,7 @@
if (as->suspends_pending > 0) {
suspends_pending -= as->suspends_pending;
if (suspends_pending <= 0)
- (void) suspend();
+ (void) suspend(1);
}
if (user_list == as)
user_list = as->next;

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2001-12-18 01:23:01

[permalink] [raw]

Subject: Re: APM driver patch summary

Russell King wrote:
> Actually you're right, it doesn't send a resume event.
> However, It's worse than that - it will leave all sleeping
> listeners still sleeping.
> Try this patch instead - this should cause all listeners
> to get a -EIO to indicate that the suspend failed, and also
> get the resume event.

Hmm. You know what? I don't think the listeners really
need to be sent a resume event if they are already getting
an error code back! The listeners should take the error
code as a cue to undo whatever they did to prepare for
the suspend. The only thing is that the error code should
not be EIO, since this currently means "the BIOS returned
something other than APM_SUCCESS". The current code returns
EAGAIN when the suspend attempt is rejected by a driver
(look in do_ioctl()) and we should stick to that.

If the above is right, then in suspend() you should set
"err = -EAGAIN" instead of "err = -EIO" and you should put
"out:" just after the "queue_event(APM_NORMAL_RESUME, NULL);"
instead of just before it.

I just checked, and apmlib 3.0.2 does not return the error code
from apm_suspend(); and apmd 3.0.2 does not undo anything when
apm_suspend() returns an error. These need to be fixed right
away. I'll file bug reports against Debian apmd since the
Debian maintainer is also the upstream maintainer -- Avery Pennarun.

Finally, think carefully about where the cli() and sti() should go.

--
Thomas

2001-12-18 10:03:51

[permalink] [raw]

Subject: Re: APM driver patch summary

On Mon, Dec 17, 2001 at 08:22:40PM -0500, Thomas Hood wrote:
> Finally, think carefully about where the cli() and sti() should go.

I believe the cli() and sti() to be correct - you shouldn't call
pm_send_all with IRQs off as it was previously. pm_send_all is a
function that can sleep (check the down(&pm_devs_lock) call).

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2001-12-18 21:26:55

[permalink] [raw]

Subject: Re: APM driver patch summary

Here is an updated list of the patches:

Notify listener of suspend before notifying driver (Russell King / me)
(appended)
Fix idle handling (Andreas Steinmetz)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100754277600661&w=2
Control apm idle calling by runtime parameter (Andrej Borsenkow)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100852862320955&w=2
Detect failure to stop CPU on apm idle call (Andrej Borsenkow)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100869841008117&w=2

I added the last one which was posted today.

I have modified Russell's patch to notify (of standbys & suspends)
listeners before drivers. I made the following changes:

Return EBUSY instead of EIO (or EAGAIN) on rejection of request.
(Suggestion from apmd maintainer. Is this okay?)
Move "sti()" up a bit inside suspend() function. (Should be harmless.)
Move "out:" after "queue_event" so that no RESUME event will be queued.
(Listeners should notice the EBUSY and undo whatever they did.)
Recode suspend() a bit to make it easier to read.
(Unfortunately this makes the patch harder to read.)
Skip actual suspend even if APM version is 0x100 (... just don't
try to set APM_STATE_REJECT. I don't see why this should be
a problem, judging from a quick read of the APM spec).

The driver compiles with this patch, but I haven't tested it yet.

I still have one worry about the driver with this patch applied.
If a user requests a suspend and it is rejected by a driver
(and the APM version > 0x100) then APM_STATE_REJECT is sent
to the BIOS. If the BIOS didn't generate the request then this
REJECT is comes out of the blue. Is that acceptable, or
should we refrain from sending such REJECTS when the suspend
request didn't come from the BIOS?

--
Thomas

2001-12-18 21:45:13

[permalink] [raw]

Subject: Re: APM driver patch summary

On Tue, Dec 18, 2001 at 04:24:05PM -0500, Thomas Hood wrote:
> Move "sti()" up a bit inside suspend() function. (Should be harmless.)

Doesn't guarantee that you'll keep interrupts disabled.

suspend() calls pm_send_all() which calls down() which might call
schedule(), which I think you'll find will re-enable interrupts.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2001-12-18 21:48:13

[permalink] [raw]

Subject: Re: APM driver patch summary

[Let's try that again, with the promised patch appended .. grrr]

Here is an updated list of the patches:

Notify listener of suspend before notifying driver (Russell King / me)
(appended)
Fix idle handling (Andreas Steinmetz)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100754277600661&w=2
Control apm idle calling by runtime parameter (Andrej Borsenkow)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100852862320955&w=2
Detect failure to stop CPU on apm idle call (Andrej Borsenkow)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100869841008117&w=2

I added the last one which was posted today (18 Dec).

I have modified Russell's patch to notify (of standbys & suspends)
listeners before drivers. I made the following changes:

Return -EBUSY instead of -EIO (or -EAGAIN) to listeners if a driver
rejects suspend request. (Suggestion from apmd maintainer.
Is this okay?)
Move "sti()" up a bit inside suspend() function. (Should be harmless.)
Move "ignore_normal_resume = 1" before "sti()" (to make sure that
normal resumes are rejected even if this func is interrupted).
Move "out:" after "queue_event" so that no RESUME event will be
queued if the suspend is skipped. (Listeners must notice the
EBUSY return code and undo whatever they did to prepare for
the suspend.)
Recode suspend() a bit to make it easier to read.
(Unfortunately this makes the patch harder to read. Sorry.)
Skip doing the suspend even if APM version is 0x100 (... just don't
try to set APM_STATE_REJECT. I don't see why this should be
a problem, judging from a quick read of the APM spec).
Also, move definition of waiting_for_resume inside check_events()
where it belongs.

The driver compiles with this patch, but I haven't tested it yet.

I still have one worry about the driver with this patch applied.
If a user requests a suspend and it is rejected by a driver
(and the APM version > 0x100) then APM_STATE_REJECT is sent
to the BIOS. If the BIOS didn't generate the request then this
REJECT is comes out of the blue. Is that acceptable, or
should we refrain from sending such REJECTS when the suspend
request didn't come from the BIOS?

--
Thomas

--- linux-2.4.17-rc1_ORIG/arch/i386/kernel/apm.c Tue Nov 27 18:59:54 2001
+++ linux-2.4.17-rc1/arch/i386/kernel/apm.c Tue Dec 18 16:39:38 2001
@@ -349,13 +349,12 @@
} apm_bios_entry;
#ifdef CONFIG_APM_CPU_IDLE
static int clock_slowed;
#endif
static int suspends_pending;
static int standbys_pending;
-static int waiting_for_resume;
static int ignore_normal_resume;
static int bounce_interval = DEFAULT_BOUNCE_INTERVAL;

#ifdef CONFIG_APM_RTC_IS_GMT
# define clock_cmos_diff 0
# define got_clock_diff 1
@@ -1130,63 +1129,46 @@
outb(LATCH >> 8 , 0x40); /* MSB */
udelay(10);
restore_flags(flags);
#endif
}

-static int send_event(apm_event_t event)
+static int suspend(int vetoable)
{
- switch (event) {
- case APM_SYS_SUSPEND:
- case APM_CRITICAL_SUSPEND:
- case APM_USER_SUSPEND:
- /* map all suspends to ACPI D3 */
- if (pm_send_all(PM_SUSPEND, (void *)3)) {
- if (event == APM_CRITICAL_SUSPEND) {
- printk(KERN_CRIT
- "apm: Critical suspend was vetoed, "
- "expect armageddon\n" );
- return 0;
- }
+ int err;
+ struct apm_user *as;
+
+ if (pm_send_all(PM_SUSPEND, (void *)3)) {
+ /* Vetoed */
+ if (vetoable) {
if (apm_info.connection_version > 0x100)
apm_set_power_state(APM_STATE_REJECT);
- return 0;
+ err = -EBUSY;
+ goto out;
}
- break;
- case APM_NORMAL_RESUME:
- case APM_CRITICAL_RESUME:
- /* map all resumes to ACPI D0 */
- (void) pm_send_all(PM_RESUME, (void *)0);
- break;
+ printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
}
-
- return 1;
-}
-
-static int suspend(void)
-{
- int err;
- struct apm_user *as;
-
get_time_diff();
cli();
err = apm_set_power_state(APM_STATE_SUSPEND);
reinit_timer();
set_time();
+ ignore_normal_resume = 1;
+ sti();
if (err == APM_NO_ERROR)
err = APM_SUCCESS;
if (err != APM_SUCCESS)
apm_error("suspend", err);
- send_event(APM_NORMAL_RESUME);
- sti();
+ err = (err == APM_SUCCESS) ? 0 : -EIO;
+ pm_send_all(PM_RESUME, (void *)0);
queue_event(APM_NORMAL_RESUME, NULL);
+ out:
for (as = user_list; as != NULL; as = as->next) {
as->suspend_wait = 0;
- as->suspend_result = ((err == APM_SUCCESS) ? 0 : -EIO);
+ as->suspend_result = err;
}
- ignore_normal_resume = 1;
wake_up_interruptible(&apm_suspend_waitqueue);
return err;
}

static void standby(void)
{
@@ -1219,12 +1201,13 @@

static void check_events(void)
{
apm_event_t event;
static unsigned long last_resume;
static int ignore_bounce;
+ static int waiting_for_resume; /* = 0 */

while ((event = get_event()) != 0) {
if (debug) {
if (event <= NR_APM_EVENT_NAME)
printk(KERN_DEBUG "apm: received %s notify\n",
apm_event_name[event - 1]);
@@ -1238,17 +1221,15 @@
if (ignore_normal_resume && (event != APM_NORMAL_RESUME))
ignore_normal_resume = 0;

switch (event) {
case APM_SYS_STANDBY:
case APM_USER_STANDBY:
- if (send_event(event)) {
- queue_event(event, NULL);
- if (standbys_pending <= 0)
- standby();
- }
+ queue_event(event, NULL);
+ if (standbys_pending <= 0)
+ standby();
break;

case APM_USER_SUSPEND:
#ifdef CONFIG_APM_IGNORE_USER_SUSPEND
if (apm_info.connection_version > 0x100)
apm_set_power_state(APM_STATE_REJECT);
@@ -1267,52 +1248,48 @@
* cope with the fact that the Thinkpads keep
* sending a SUSPEND event until something else
* happens!
*/
if (waiting_for_resume)
return;
- if (send_event(event)) {
- queue_event(event, NULL);
- waiting_for_resume = 1;
- if (suspends_pending <= 0)
- (void) suspend();
- }
+ queue_event(event, NULL);
+ waiting_for_resume = 1;
+ if (suspends_pending <= 0)
+ (void) suspend(1);
break;

case APM_NORMAL_RESUME:
case APM_CRITICAL_RESUME:
case APM_STANDBY_RESUME:
waiting_for_resume = 0;
last_resume = jiffies;
ignore_bounce = 1;
if ((event != APM_NORMAL_RESUME)
|| (ignore_normal_resume == 0)) {
set_time();
- send_event(event);
+ pm_send_all(PM_RESUME, (void *)0);
queue_event(event, NULL);
}
break;

case APM_CAPABILITY_CHANGE:
case APM_LOW_BATTERY:
case APM_POWER_STATUS_CHANGE:
- send_event(event);
queue_event(event, NULL);
break;

case APM_UPDATE_TIME:
set_time();
break;

case APM_CRITICAL_SUSPEND:
- send_event(event);
/*
* We can only hope it worked - we are not allowed
* to reject a critical suspend.
*/
- (void) suspend();
+ (void) suspend(0);
break;
}
}
}

static void apm_event_handler(void)
@@ -1476,31 +1453,26 @@
switch (cmd) {
case APM_IOC_STANDBY:
if (as->standbys_read > 0) {
as->standbys_read--;
as->standbys_pending--;
standbys_pending--;
- } else if (!send_event(APM_USER_STANDBY))
- return -EAGAIN;
- else
+ } else
queue_event(APM_USER_STANDBY, as);
if (standbys_pending <= 0)
standby();
break;
case APM_IOC_SUSPEND:
if (as->suspends_read > 0) {
as->suspends_read--;
as->suspends_pending--;
suspends_pending--;
- } else if (!send_event(APM_USER_SUSPEND))
- return -EAGAIN;
- else
+ } else
queue_event(APM_USER_SUSPEND, as);
if (suspends_pending <= 0) {
- if (suspend() != APM_SUCCESS)
- return -EIO;
+ return suspend(1);
} else {
as->suspend_wait = 1;
wait_event_interruptible(apm_suspend_waitqueue,
as->suspend_wait == 0);
return as->suspend_result;
}
@@ -1525,13 +1497,13 @@
if (standbys_pending <= 0)
standby();
}
if (as->suspends_pending > 0) {
suspends_pending -= as->suspends_pending;
if (suspends_pending <= 0)
- (void) suspend();
+ (void) suspend(1);
}
if (user_list == as)
user_list = as->next;
else {
struct apm_user * as1;

2001-12-19 10:24:46

[permalink] [raw]

Subject: Re: APM driver patch summary

On Tue, Dec 18, 2001 at 08:55:11PM -0800, Ryan Cumming wrote:
> Just a quick recap for those of use who haven't picked up an x86 manual
> recently:

OR for those who don't have such a thing. 8)

Yes, it was a thinko. Just like not attaching the patch which was supposed
to be there according to the same email.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2001-12-19 13:49:56

[permalink] [raw]

Subject: Re: APM driver patch summary

Well, I'm not having any problems with the latest "notify
listeners before drivers" patch, and it does fix the problems
I was having.

I'm not sure that this is an appropriate change for 2.4,
but the decision about whether to put it in is up to Stephen.
In the meantime I will try to keep the patch up to date as
2.4 kernels are released. Definitely it should go into 2.5
though. (The idle fixes, on the other hand, should go into
2.4.)

I've just spent a couple of hours auditing the code and I
haven't found any other problems ( ... not that that proves
anything). One little worry: apm_event_handler() seems to
assume that it is called once per second, and that it will
therefore set APM_STATE_BUSY once every four seconds, as the
spec says it should. In fact it gets called a lot more often
than that, as this bit of syslog shows:
------------------------------------------------------
Dec 18 18:00:20 thanatos apmd[266]: apmd_call_proxy: executing:
'/etc/apm/apmd_proxy' 'suspend'
Dec 18 18:00:20 thanatos kernel: apm: setting state busy
Dec 18 18:00:21 thanatos last message repeated 7 times
------------------------------------------------------
However, the APM spec does not put a ceiling on how often
the state is set to BUSY, so it should be okay.

--
Thomas

2001-12-22 03:35:35

[permalink] [raw]

Subject: Re: APM driver patch summary

I wrote:
---------------------------------------------------------------------
Here is an updated list of the patches:
1. Notify listener of suspend before drivers (Russell King, me)
(appended)
2. Fix idle handling (Andreas Steinmetz)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100754277600661&w=2
3. Control apm idle calling by runtime parameter (Andrej Borsenkow)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100852862320955&w=2
4. Detect failure to stop CPU on apm idle call (Andrej Borsenkow)
http://marc.theaimsgroup.com/?l=linux-kernel&m=100869841008117&w=2
---------------------------------------------------------------------

I have just tried to combine these and I have run into trouble.
Patch 4 applies on top of patch 3, but neither of these applies
on top of patch 2. Can you guys sort these out into one big
"fix idle calling" patch that includes a runtime parameter
to control idle calling, which overrides a default selected
either by CONFIG_APM_CPU_IDLE or by a bit of code that checks
for CPU stoppage?

Or has someone already done this?

The latest Russell King (modified by me) patch is now at:
http://panopticon.csustan.edu/thood/apm.html

--
Thomas Hood

2001-12-22 10:55:32

[permalink] [raw]

Subject: Re: APM driver patch summary

I'll have a look over the weekend.

On 22-Dec-2001 Thomas Hood wrote:
> I wrote:
> ---------------------------------------------------------------------
> Here is an updated list of the patches:
> 1. Notify listener of suspend before drivers (Russell King, me)
> (appended)
> 2. Fix idle handling (Andreas Steinmetz)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=100754277600661&w=2
> 3. Control apm idle calling by runtime parameter (Andrej Borsenkow)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=100852862320955&w=2
> 4. Detect failure to stop CPU on apm idle call (Andrej Borsenkow)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=100869841008117&w=2
> ---------------------------------------------------------------------
>
> I have just tried to combine these and I have run into trouble.
> Patch 4 applies on top of patch 3, but neither of these applies
> on top of patch 2. Can you guys sort these out into one big
> "fix idle calling" patch that includes a runtime parameter
> to control idle calling, which overrides a default selected
> either by CONFIG_APM_CPU_IDLE or by a bit of code that checks
> for CPU stoppage?
>
> Or has someone already done this?
>
> The latest Russell King (modified by me) patch is now at:
> http://panopticon.csustan.edu/thood/apm.html
>
> --
> Thomas Hood
>
>
>

Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH

2001-12-22 14:57:27

[permalink] [raw]

Subject: Re: APM driver patch summary

Hi,
I merged 2., 3. and 4. (attached) with some modifications.

1. There is now a module parameter apm-idle-threshold which allows to override
the compiled in idle percentage threshold above which BIOS idle calls are
done.

2. I modified Andrej's mechanism to detect a defunct BIOS (stating 'does stop
CPU' when it actually doesn't) to take into account that there's other
interrupts than the timer interrupt that could reactivate the cpu.
As there's 16 hardware interrupts on x86 (apm is arch specific anyway) I do
use a leaky bucket counter for a maximum of 16 idle rounds until jiffies is
increased. When the counter reaches zero it stays at this value and the
system idle routine is called. If BIOS idle is a noop then the counter
reaches zero fast, thus effectively halting the cpu.

Andrej, could you please test the patch if it works for your laptop?

On 22-Dec-2001 Thomas Hood wrote:
> I wrote:
> ---------------------------------------------------------------------
> Here is an updated list of the patches:
> 1. Notify listener of suspend before drivers (Russell King, me)
> (appended)
> 2. Fix idle handling (Andreas Steinmetz)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=100754277600661&w=2
> 3. Control apm idle calling by runtime parameter (Andrej Borsenkow)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=100852862320955&w=2
> 4. Detect failure to stop CPU on apm idle call (Andrej Borsenkow)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=100869841008117&w=2
> ---------------------------------------------------------------------
>
> I have just tried to combine these and I have run into trouble.
> Patch 4 applies on top of patch 3, but neither of these applies
> on top of patch 2. Can you guys sort these out into one big
> "fix idle calling" patch that includes a runtime parameter
> to control idle calling, which overrides a default selected
> either by CONFIG_APM_CPU_IDLE or by a bit of code that checks
> for CPU stoppage?
>
> Or has someone already done this?
>
> The latest Russell King (modified by me) patch is now at:
> http://panopticon.csustan.edu/thood/apm.html
>
> --
> Thomas Hood
>
>
>

Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH

Attachments:

apm.c.diff (6.00 kB)

2001-12-22 16:16:11

[permalink] [raw]

Subject: Re: APM driver patch summary

Thanks.

I have put Andreas's idle patch, Russell King's notification patch,
and a combined patch up at:
http://panopticon.csustan.edu/thood/apm.html

--
Thomas Hood

On Sat, 2001-12-22 at 09:44, Andreas Steinmetz wrote:
> Hi,
> I merged 2., 3. and 4. (attached) with some modifications.
>
> 1. There is now a module parameter apm-idle-threshold which allows to override
> the compiled in idle percentage threshold above which BIOS idle calls are
> done.
>
> 2. I modified Andrej's mechanism to detect a defunct BIOS (stating 'does stop
> CPU' when it actually doesn't) to take into account that there's other
> interrupts than the timer interrupt that could reactivate the cpu.
> As there's 16 hardware interrupts on x86 (apm is arch specific anyway) I do
> use a leaky bucket counter for a maximum of 16 idle rounds until jiffies is
> increased. When the counter reaches zero it stays at this value and the
> system idle routine is called. If BIOS idle is a noop then the counter
> reaches zero fast, thus effectively halting the cpu.
>
> Andrej, could you please test the patch if it works for your laptop?

2001-12-23 03:22:27

[permalink] [raw]

Subject: Re: APM driver patch summary

On Sat, 2001-12-22 at 09:44, Andreas Steinmetz wrote:
> 1. There is now a module parameter apm-idle-threshold which
> allows to override the compiled in idle percentage threshold
> above which BIOS idle calls are done.

Andrej, your patch doesn't work when compiled as a module
because of a name mismatch.

I went in and cleaned the patch up a bit. Now there is only
one extra parameter, called "idle_threshold", which you can
set to 100 if you want to disable use of APM BIOS idling.

I have combined this tweaked idle patch with the
notification patch and made it available here:
http://panopticon.csustan.edu/thood/apm.html
Patch is against 2.4.17.

I hope lots of people will test it. It's working fine for me.

--
Thomas Hood

2001-12-23 12:26:19

[permalink] [raw]

Subject: Re: APM driver patch summary

Fine with me,
as I always compile APM into the kernel I just didn't see it (mental note to
self: always try module build before submitting).
I'll get the combined patch and test it.

On 23-Dec-2001 Thomas Hood wrote:
> On Sat, 2001-12-22 at 09:44, Andreas Steinmetz wrote:
>> 1. There is now a module parameter apm-idle-threshold which
>> allows to override the compiled in idle percentage threshold
>> above which BIOS idle calls are done.
>
> Andrej, your patch doesn't work when compiled as a module
> because of a name mismatch.
>
> I went in and cleaned the patch up a bit. Now there is only
> one extra parameter, called "idle_threshold", which you can
> set to 100 if you want to disable use of APM BIOS idling.
>
> I have combined this tweaked idle patch with the
> notification patch and made it available here:
> http://panopticon.csustan.edu/thood/apm.html
> Patch is against 2.4.17.
>
> I hope lots of people will test it. It's working fine for me.
>
> --
> Thomas Hood
>
>
>

Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH

2001-12-30 20:06:13

[permalink] [raw]

Subject: APM driver patch okay?

Hi Stephen

Just a note to say that the patch combining "idling"
and "notification order" changes is looking good so far.
http://panopticon.csustan.edu/thood/apm.html
(No one has complained, anyway.) I think it's ready
for more extensive testing. Should it go into a 2.4.X-preY
kernel?

--
Thomas

2001-12-31 01:03:24

by Stephen Rothwell

[permalink] [raw]

Subject: Re: APM driver patch okay?

Hi Thomas,

On 30 Dec 2001 15:05:54 -0500
Thomas Hood <[email protected]> wrote:

> Hi Stephen
>
> Just a note to say that the patch combining "idling"
> and "notification order" changes is looking good so far.
> http://panopticon.csustan.edu/thood/apm.html
> (No one has complained, anyway.) I think it's ready
> for more extensive testing. Should it go into a 2.4.X-preY
> kernel?

OK, I have a slightly modified version of the patch that I created just
before starting my Christmas break. The only problem is that it crashes
the kernel hard when I remove the apm module. I have not had a chance
to get back to it since then, but I am back at work in Wednesday and
it has my highest priority at the moment.

Question: Has anyone got any empirical evidence that this patch
improves the idling behaviour? i.e. does it consume less power or
lower the temperature? I intend to try to measure this on Wednesday,
but a wider set of data is always better.

Thanks again for putting these patches together.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/

2001-12-31 02:24:07

[permalink] [raw]

Subject: Re: APM driver patch okay?

On Sun, 2001-12-30 at 20:02, Stephen Rothwell wrote:
> OK, I have a slightly modified version of the patch that I created just
> before starting my Christmas break. The only problem is that it crashes
> the kernel hard when I remove the apm module.

Erp. Obviously that wasn't something I tested.

I was just looking at the code to make sure that when the
module is removed it can't leave the machine in the idle
state. Unfortunately the apm_cpu_idle() function uses
several gotos, continues and breaks, and uses both "t1"
and "t2" for more than one purpose. Here's a patch that
makes that function a bit easier to read. // TH

--- linux-2.4.17/arch/i386/kernel/apm.c_1 Sun Dec 23 14:53:12 2001
+++ linux-2.4.17/arch/i386/kernel/apm.c Sun Dec 30 21:19:29 2001
@@ -757,12 +757,10 @@
#define HARD_IDLE_TIMEOUT (HZ/3)
#define IDLE_CALC_LIMIT (HZ*100)
#define IDLE_LEAKY_MAX 16

static void (*sys_idle)(void); /* = 0 */
-static unsigned int last_jiffies; /* = 0 */
-static unsigned int last_stime; /* = 0 */

/**
* apm_cpu_idle - cpu idling for APM capable Linux
*
* This is the idling function the kernel executes when APM is available. It
@@ -770,57 +768,62 @@
* Furthermore it calls the system default idle routine.
*/

static void apm_cpu_idle(void)
{
- static int use_apm_idle = 0;
+ static int use_apm_idle; /* = 0 */
+ static unsigned int last_jiffies; /* = 0 */
+ static unsigned int last_stime; /* = 0 */
int apm_is_idle = 0;
- unsigned int t1 = jiffies - last_jiffies;
- unsigned int t2;
+ unsigned int delta_jiffies;
+ unsigned int leak;

-recalc: if(t1 > IDLE_CALC_LIMIT)
+ delta_jiffies = jiffies - last_jiffies;
+
+recalc: if(delta_jiffies > IDLE_CALC_LIMIT)
goto reset;

- if(t1 > HARD_IDLE_TIMEOUT) {
- t2 = current->times.tms_stime - last_stime;
- t2 *= 100;
- t2 /= t1;
- if (t2 > idle_threshold)
+ if(delta_jiffies > HARD_IDLE_TIMEOUT) {
+ unsigned int t = current->times.tms_stime - last_stime;
+ t *= 100;
+ t /= delta_jiffies;
+ if (t > idle_threshold)
use_apm_idle = 1;
else
reset: use_apm_idle = 0;
last_jiffies = jiffies;
last_stime = current->times.tms_stime;
}

- t2 = IDLE_LEAKY_MAX;
+ leak = IDLE_LEAKY_MAX;

while (!current->need_resched) {
if(use_apm_idle) {
- t1 = jiffies;
+ unsigned int jiffies_before = jiffies;
switch (apm_do_idle()) {
case 0: apm_is_idle = 1;
- if (t1 != jiffies) {
- if (t2) {
- t2 = IDLE_LEAKY_MAX;
+ if (jiffies_before != jiffies) {
+ if (leak) {
+ leak = IDLE_LEAKY_MAX;
continue;
}
- } else if (t2) {
- t2--;
+ } else if (leak) {
+ leak--;
continue;
}
break;
case 1: apm_is_idle = 1;
break;
+ /* case -1: did not idle */
}
}

if (sys_idle)
sys_idle();

- t1 = jiffies - last_jiffies;
- if (t1 > HARD_IDLE_TIMEOUT)
+ delta_jiffies = jiffies - last_jiffies;
+ if (delta_jiffies > HARD_IDLE_TIMEOUT)
goto recalc;
}

if (apm_is_idle)
apm_do_busy();

2001-12-31 14:40:37

[permalink] [raw]

Subject: Re: APM driver patch okay?

Sorry if the code was to unreadable for you.
I did just try to keep the apm code overhead small and I'm no friend of "local
local" variables. If the code is now better readable that's fine with me.

If you remove the apm module as far as I can see the system can't be in apm
idle state as during module removal the system is not idle, so the system idle
task is not called, so apm is in busy state as apm_cpu_idle() is called via the
system idle task function pointer and asserts that apm_do_busy() is called on
function exit when apm_do_idle was called.

As I can't get useful temperature data out of my laptop (CPU temp 5?C) I do have
to rely on not really measurable things like fan behaviour and estimated
battery lifetime. With the patch I'm back to 2.2 behaviour which is better than
unpatched 2.4 behaviour.

Even if the idle patch may not improve things for biosses that do work
correctly I can't see any degradation in this case. For broken biosses it is an
improvement in any case.

Furthermore the patch removes confusion where the apm kernel thread and the
idle task were both racing for idle time with the result that an idle system
seemed 'heavily loaded' by the apm kernel thread. This may even have caused
unexpected behaviour for processes that use system load information for
processing decisions.

I would suggest to submit the patch in the early 2.4.18pre stages for wider
testing after the hard lockup on module removal is solved as there seem to be no
other complaints.

Regarding the hard lockup there could be a problem in apm_exit(). Assuming that
sys_idle could be NULL if apm initialization was not complete a safer way would
be:

if (idle_threshold < 100 && sys_idle) {
pm_idle = sys_idle;
sys_idle = NULL;
}

Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH

2002-01-04 19:56:52

by Borsenkow Andrej

[permalink] [raw]

Subject: Re: APM driver patch summary

Sorry for the delay, I was off before New Year and then could not test
it ...

On ???, 2001-12-22 at 17:44, Andreas Steinmetz wrote:
> Hi,
> I merged 2., 3. and 4. (attached) with some modifications.
>
> 1. There is now a module parameter apm-idle-threshold which allows to override
> the compiled in idle percentage threshold above which BIOS idle calls are
> done.
>
> 2. I modified Andrej's mechanism to detect a defunct BIOS (stating 'does stop
> CPU' when it actually doesn't) to take into account that there's other
> interrupts than the timer interrupt that could reactivate the cpu.
> As there's 16 hardware interrupts on x86 (apm is arch specific anyway) I do
> use a leaky bucket counter for a maximum of 16 idle rounds until jiffies is
> increased. When the counter reaches zero it stays at this value and the
> system idle routine is called. If BIOS idle is a noop then the counter
> reaches zero fast, thus effectively halting the cpu.
>

I do not think you need it. Either interrupt waked up somebody and set
need_resched and we exit loop or nobody is ready to run and we can sleep
again. Why complicate things any more than needed?

> Andrej, could you please test the patch if it works for your laptop?
>

It does not work and I am very surprised it works for somebody (well,
there are conditios when it will work). By default pm_idle is always
NULL so we *never* actually call kernel function that really stops CPU.
Main idle task is cpu_idle that does

if (pm_idle)
pm_idle()
or
default_idle

and CPU is halted in default_idle. So your patch just enters busy loop
calling BIOS APM Idle over and over again just like it was before.

Attached patch makes apm_cpu_idle do the same and call either old
pm_idle (a.k.a. sys_idle) or default_idle. I removed your interrupt
handling - it does not actually affect the problem but it still is not
needed IMHO. t1, t2 are changed from int into long because jiffies is
long - not sure if it is really needed.

cheers and sorry for delay

-andrej

Attachments:

apm.diff (1.52 kB)

2002-01-05 12:08:45