Re: APM driver patch summary

Thomas Hood
2001-12-18T21:48:13Z

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