APM driver patch summary

APM driver patch summary

[Thomas Hood]

Stephen:

A number of patches to the APM driver have been submitted to
lkml recently.  To summarize, these are (Did I miss any?):

Notify listener of suspend before notifying driver  (Russell King)
    http://marc.theaimsgroup.com/?l=linux-kernel&m=100819765228734&w=2
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

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.  I think that this change should wait for 2.5, improvement
though it be, because it changes the behavior of the apm driver in
a significant way.

The other patches ought to apply cleanly to 2.4.17 and are independent
of Russell's changes, so ought to go into 2.4.18-pre, I think.  Can
you get them ready for Marcelo?

Cheers
Thomas Hood

Re: APM driver patch summary

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: APM driver patch summary

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: APM driver patch summary

[Thomas Hood]

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

Re: APM driver patch summary

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: APM driver patch summary

[Thomas Hood]

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

Re: APM driver patch summary

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: APM driver patch summary

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: APM driver patch summary

[Thomas Hood]

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

Re: APM driver patch summary

[Thomas Hood]

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

Re: APM driver patch summary

[Thomas Hood]

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

Re: APM driver patch summary

[Andreas Steinmetz]

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

Re: APM driver patch summary

[Andreas Steinmetz]

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

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

[-- Attachment #2: apm.c.diff --]
[-- Type: text/plain, Size: 6146 bytes --]

--- /tmp/apm.c.orig	Sat Dec 22 15:19:42 2001
+++ /tmp/apm.c	Sat Dec 22 15:17:23 2001
@@ -341,6 +341,11 @@
 #define APM_BIOS_MAGIC		0x4101
 
 /*
+ * idle percentage above which bios idle calls are done
+ */
+#define BIOS_IDLE_TRIGGER	95
+
+/*
  * Local variables
  */
 static struct {
@@ -349,6 +354,8 @@
 }				apm_bios_entry;
 #ifdef CONFIG_APM_CPU_IDLE
 static int			clock_slowed;
+static int			idle_enabled = 1;
+static int			idle_threshold = BIOS_IDLE_TRIGGER;
 #endif
 static int			suspends_pending;
 static int			standbys_pending;
@@ -735,63 +742,84 @@
 	}
 }
 
-#if 0
-extern int hlt_counter;
-
 /*
- * If no process has been interested in this
- * CPU for some time, we want to wake up the
- * power management thread - we probably want
+ * If no process has really been interested in
+ * CPU for some time, we want to call BIOS
+ * power management - we probably want
  * to conserve power.
  */
 #define HARD_IDLE_TIMEOUT (HZ/3)
+#define IDLE_CALC_LIMIT   (HZ*100)
+#define IDLE_LEAKY_MAX    16
 
-/* This should wake up kapmd and ask it to slow the CPU */
-#define powermanagement_idle()  do { } while (0)
+static void (*sys_idle)(void);
+static unsigned int last_jiffies = 0;
+static unsigned int last_stime = 0;
+static int use_apm_idle = 0;
 
 /**
  * apm_cpu_idle		-	cpu idling for APM capable Linux
  *
  * This is the idling function the kernel executes when APM is available. It 
- * tries to save processor time directly by using hlt instructions. A
- * separate apm thread tries to do the BIOS power management.
- *
- * N.B. This is curently not used for kernels 2.4.x.
+ * tries to do BIOS powermanagement based on the average system idle time.
+ * Furthermore it calls the system default idle routine.
  */
 
 static void apm_cpu_idle(void)
 {
-	unsigned int start_idle;
-
-	start_idle = jiffies;
-	while (1) {
-		if (!current->need_resched) {
-			if (jiffies - start_idle < HARD_IDLE_TIMEOUT) {
-				if (!current_cpu_data.hlt_works_ok)
-					continue;
-				if (hlt_counter)
+	int apm_is_idle = 0;
+	unsigned int t1 = jiffies - last_jiffies;
+	unsigned int t2;
+
+recalc:	if(t1 > 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)
+			use_apm_idle = 1;
+		else
+reset:			use_apm_idle = 0;
+		last_jiffies = jiffies;
+		last_stime = current->times.tms_stime;
+	}
+
+	t2 = IDLE_LEAKY_MAX;
+
+	while (!current->need_resched) {
+		if(use_apm_idle) {
+			t1 = jiffies;
+			switch (apm_do_idle()) {
+			case 0:	apm_is_idle = 1;
+				if (t1 != jiffies) {
+					if (t2) {
+						t2 = IDLE_LEAKY_MAX;
+						continue;
+					}
+				} else if (t2) {
+					t2--;
 					continue;
-				__cli();
-				if (!current->need_resched)
-					safe_halt();
-				else
-					__sti();
-				continue;
+				}
+				break;
+			case 1:	apm_is_idle = 1;
+				break;
 			}
-
-			/*
-			 * Ok, do some power management - we've been idle for too long
-			 */
-			powermanagement_idle();
 		}
 
-		schedule();
-		check_pgt_cache();
-		start_idle = jiffies;
+		if (sys_idle)
+			sys_idle();
+
+		t1 = jiffies - last_jiffies;
+		if (t1 > HARD_IDLE_TIMEOUT)
+			goto recalc;
 	}
+
+	if (apm_is_idle)
+		apm_do_busy();
 }
 #endif
-#endif
 
 #ifdef CONFIG_SMP
 static int apm_magic(void * unused)
@@ -1346,17 +1374,12 @@
 
 static void apm_mainloop(void)
 {
-	int timeout = HZ;
 	DECLARE_WAITQUEUE(wait, current);
 
 	add_wait_queue(&apm_waitqueue, &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
 	for (;;) {
-		/* Nothing to do, just sleep for the timeout */
-		timeout = 2 * timeout;
-		if (timeout > APM_CHECK_TIMEOUT)
-			timeout = APM_CHECK_TIMEOUT;
-		schedule_timeout(timeout);
+		schedule_timeout(APM_CHECK_TIMEOUT);
 		if (exit_kapmd)
 			break;
 
@@ -1366,34 +1389,6 @@
 		 */
 		set_current_state(TASK_INTERRUPTIBLE);
 		apm_event_handler();
-#ifdef CONFIG_APM_CPU_IDLE
-		if (!system_idle())
-			continue;
-		
-		/*
-		 *	If we can idle...
-		 */
-		if (apm_do_idle() != -1) {
-			unsigned long start = jiffies;
-			while ((!exit_kapmd) && system_idle()) {
-				if (apm_do_idle()) {
-					set_current_state(TASK_INTERRUPTIBLE);
-					/* APM needs us to snooze .. either
-					   the BIOS call failed (-1) or it
-					   slowed the clock (1). We sleep
-					   until it talks to us again */
-					schedule_timeout(1);
-				}
-				if ((jiffies - start) > APM_CHECK_TIMEOUT) {
-					apm_event_handler();
-					start = jiffies;
-				}
-			}
-			apm_do_busy();
-			apm_event_handler();
-			timeout = 1;
-		}
-#endif
 	}
 	remove_wait_queue(&apm_waitqueue, &wait);
 }
@@ -1814,6 +1809,17 @@
 		if ((strncmp(str, "realmode-power-off", 18) == 0) ||
 		    (strncmp(str, "realmode_power_off", 18) == 0))
 			apm_info.realmode_power_off = !invert;
+#ifdef CONFIG_APM_CPU_IDLE
+		if ((strncmp(str, "apm-idle", 8) == 0) ||
+		    (strncmp(str, "apm_idle", 8) == 0))
+			idle_enabled = !invert;
+		if ((strncmp(str, "apm-idle-threshold=", 19) == 0) ||
+		    (strncmp(str, "apm_idle_threshold=", 19) == 0)) {
+			idle_threshold = simple_strtol(str + 19, NULL, 0);
+			if ( idle_threshold < 0 || idle_threshold > 99 )
+				idle_threshold = BIOS_IDLE_TRIGGER;
+		}
+#endif
 		str = strchr(str, ',');
 		if (str != NULL)
 			str += strspn(str, ", \t");
@@ -1968,6 +1974,13 @@
 
 	misc_register(&apm_device);
 
+#ifdef CONFIG_APM_CPU_IDLE
+	if (idle_enabled) {
+		sys_idle = pm_idle;
+		pm_idle  = apm_cpu_idle;
+	}
+#endif
+
 	return 0;
 }
 
@@ -1975,6 +1988,11 @@
 {
 	int	error;
 
+#ifdef CONFIG_APM_CPU_IDLE
+	if (idle_enabled)
+		pm_idle = sys_idle;
+#endif
+
 	if (((apm_info.bios.flags & APM_BIOS_DISENGAGED) == 0)
 	    && (apm_info.connection_version > 0x0100)) {
 		error = apm_engage_power_management(APM_DEVICE_ALL, 0);
@@ -2012,5 +2030,11 @@
 MODULE_PARM(realmode_power_off, "i");
 MODULE_PARM_DESC(realmode_power_off,
 		"Switch to real mode before powering off");
+#ifdef CONFIG_APM_CPU_IDLE
+MODULE_PARM(apm_idle, "i");
+MODULE_PARM_DESC(apm_idle, "Make BIOS idle calls when idle");
+MODULE_PARM(apm_idle_threshold, "i");
+MODULE_PARM_DESC(apm_idle_threshold, "System idle percentage for BIOS idle call
s");
+#endif
 
 EXPORT_NO_SYMBOLS;

Re: APM driver patch summary

[Thomas Hood]

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?

Re: APM driver patch summary

[Borsenkow Andrej]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

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




[-- Attachment #2: apm.diff --]
[-- Type: text/x-diff, Size: 1613 bytes --]

--- linux-2.4.16-9mdk/arch/i386/kernel/apm.c.combined	Sun Dec 23 22:15:43 2001
+++ linux-2.4.16-9mdk/arch/i386/kernel/apm.c	Fri Jan  4 22:26:09 2002
@@ -761,6 +761,7 @@
 static void (*sys_idle)(void);
 static unsigned int last_jiffies = 0;
 static unsigned int last_stime = 0;
+extern void default_idle(void);
 
 /**
  * apm_cpu_idle		-	cpu idling for APM capable Linux
@@ -774,8 +775,8 @@
 {
 	static int use_apm_idle = 0;
 	int apm_is_idle = 0;
-	unsigned int t1 = jiffies - last_jiffies;
-	unsigned int t2;
+	unsigned long t1 = jiffies - last_jiffies;
+	unsigned long t2;
 
 recalc:	if(t1 > IDLE_CALC_LIMIT)
 		goto reset;
@@ -799,15 +800,8 @@
 			t1 = jiffies;
 			switch (apm_do_idle()) {
 			case 0:	apm_is_idle = 1;
-				if (t1 != jiffies) {
-					if (t2) {
-						t2 = IDLE_LEAKY_MAX;
-						continue;
-					}
-				} else if (t2) {
-					t2--;
+				if (t1 != jiffies)
 					continue;
-				}
 				break;
 			case 1:	apm_is_idle = 1;
 				break;
@@ -816,6 +810,8 @@
 
 		if (sys_idle)
 			sys_idle();
+		else
+		    default_idle();
 
 		t1 = jiffies - last_jiffies;
 		if (t1 > HARD_IDLE_TIMEOUT)
--- linux-2.4.16-9mdk/arch/i386/kernel/process.c.org	Thu Dec 13 13:12:46 2001
+++ linux-2.4.16-9mdk/arch/i386/kernel/process.c	Fri Jan  4 22:24:47 2002
@@ -77,8 +77,10 @@
 /*
  * We use this if we don't have any better
  * idle routine..
+ * It is also called from apm_cpu_idle if BIOS does not stop clock
+ * for us
  */
-static void default_idle(void)
+void default_idle(void)
 {
 	if (current_cpu_data.hlt_works_ok && !hlt_counter) {
 		__cli();

Re: APM driver patch summary

[Andreas Steinmetz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 2797 bytes --]


On 04-Jan-2002 Borsenkow Andrej wrote:
> 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? 
> 

NIC interrupt with fragmented packet, usb, sound, ... - there's interrupts with
nobody ready to run. Have a look at /proc/interrupts from time to time while
your system is idle.

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

Well, if your BIOS is not broken it works. That's why I asked you to test the
patch.

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

Granted.

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

Please don't  do it like this. It breaks apm module build for sure. I would
suggest to implement the functionality of default_idle() into apm_cpu_idle().
Though I could do this right now I'd ask all participating parties to agree on
a current code status on which to work on.

> cheers and sorry for delay
> 
> -andrej
> 
> 
> 

Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH

Re: APM driver patch summary

[Thomas Hood]

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

Re: APM driver patch summary

[Andreas Steinmetz]

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