[RFC] apm-emulation: implement notify/ack for /sys/power/state events

[RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Johannes Berg]

Currently, apm-emulation only implements notify/ack for events that are
initiated by the apm-emulation code itself. That doesn't work out quite
so well when X relies on the apm-emulation and you use scripts to
suspend that simply use /sys/power/state. Ultimately, those scripts
ought to notify userspace themselves, but currently X relies on apm
notifications.

This patch implements the required notification.

Rafael already told me that the EXPORT_SYMBOL_GPL() this patch adds will
no longer be required. If this patch is merged before the suspend tree
(though it probably should go through the suspend tree), that is
required, otherwise it should be removed.

I have *NOT* tested this patch with X, only with a simple dummy program
that does the apm emulation code, available here:
http://johannes.sipsolutions.net/files/test-apm-emu.c.txt

Not-yet-signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/char/apm-emulation.c |  108 ++++++++++++++++++++++++++++++++++++++++---
 kernel/power/main.c          |    1 
 2 files changed, 102 insertions(+), 7 deletions(-)

--- everything.orig/drivers/char/apm-emulation.c	2007-12-16 15:22:49.889513942 +0100
+++ everything/drivers/char/apm-emulation.c	2007-12-16 18:57:54.555874295 +0100
@@ -171,6 +171,8 @@ static void queue_event(apm_event_t even
  * return -EBUSY.  Otherwise, queue an event to all "writer"
  * users.  If there are no "writer" users, return '1' to
  * indicate that we can immediately suspend.
+ *
+ * @sender can be NULL when the event is queued from /sys/power/state
  */
 static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
 {
@@ -207,10 +209,9 @@ static int queue_suspend_event(apm_event
 	return ret;
 }
 
-static void apm_suspend(void)
+static void apm_after_resume(int err)
 {
 	struct apm_user *as;
-	int err = pm_suspend(PM_SUSPEND_MEM);
 
 	/*
 	 * Anyone on the APM queues will think we're still suspended.
@@ -236,6 +237,13 @@ static void apm_suspend(void)
 	wake_up(&apm_suspend_waitqueue);
 }
 
+static void apm_suspend(void)
+{
+	int err = pm_suspend(PM_SUSPEND_MEM);
+
+	apm_after_resume(err);
+}
+
 static ssize_t apm_read(struct file *fp, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct apm_user *as = fp->private_data;
@@ -320,6 +328,12 @@ apm_ioctl(struct inode * inode, struct f
 			mutex_unlock(&state_lock);
 
 			/*
+			 * suspends_pending changed, the notifier needs to be
+			 * woken up for this
+			 */
+			wake_up(&apm_suspend_waitqueue);
+
+			/*
 			 * If there are no further acknowledges required,
 			 * suspend the system.
 			 */
@@ -404,6 +418,8 @@ static int apm_release(struct inode * in
 		pending = suspends_pending == 0;
 	}
 	mutex_unlock(&state_lock);
+	wake_up(&apm_suspend_waitqueue);
+
 	if (pending)
 		apm_suspend();
 
@@ -575,6 +591,73 @@ static int kapmd(void *arg)
 	return 0;
 }
 
+int apm_suspend_notifier(struct notifier_block *nb,
+			 unsigned long event,
+			 void *dummy)
+{
+	int err;
+
+	switch (event) {
+	case PM_SUSPEND_PREPARE:
+		/*
+		 * This is a bit of a hack. We tell the rest of the code that we
+		 * still have a suspend pending and then go suspend ourselves.
+		 * We have to do it this way because otherwise we'll call
+		 * pm_sleep() when suspend_pending drops to zero which would
+		 * return -EBUSY and confuse everything.
+		 */
+		mutex_lock(&state_lock);
+		suspends_pending++;
+		mutex_unlock(&state_lock);
+
+		err = queue_suspend_event(APM_SYS_SUSPEND, NULL);
+		if (err < 0)
+			return NOTIFY_BAD;
+
+		/* all's well, nobody to wait for */
+		if (err > 0)
+			return NOTIFY_OK;
+
+		/*
+		 * Wait for the the suspends_pending variable to drop to 1,
+		 * meaning everybody else acked the suspend event (or the
+		 * process was killed.)
+		 *
+		 * Unfortunately we cannot do a timeout because then we'd
+		 * suspend again right away if the process that had apm_bios
+		 * open and that we timed out waiting for "acknowledges" the
+		 * event after we have resumed. If suspend doesn't work because
+		 * of a rogue process, just kill that process.
+		 *
+		 * FIXME: is the suspends_pending == 1 test racy?
+		 */
+		err = wait_event_interruptible(apm_suspend_waitqueue,
+					       suspends_pending == 1);
+
+		mutex_lock(&state_lock);
+		suspends_pending--;
+		mutex_unlock(&state_lock);
+
+		if (!err)
+			return NOTIFY_OK;
+
+		/* interrupted by signal */
+		return NOTIFY_BAD;
+
+	case PM_POST_SUSPEND:
+		/* TODO: maybe grab error code, needs core changes */
+		apm_after_resume(0);
+		return NOTIFY_OK;
+
+	default:
+		return NOTIFY_DONE;
+	}
+}
+
+struct notifier_block apm_notif_block = {
+	.notifier_call = apm_suspend_notifier,
+};
+
 static int __init apm_init(void)
 {
 	int ret;
@@ -588,7 +671,7 @@ static int __init apm_init(void)
 	if (IS_ERR(kapmd_tsk)) {
 		ret = PTR_ERR(kapmd_tsk);
 		kapmd_tsk = NULL;
-		return ret;
+		goto out;
 	}
 	wake_up_process(kapmd_tsk);
 
@@ -597,16 +680,27 @@ static int __init apm_init(void)
 #endif
 
 	ret = misc_register(&apm_device);
-	if (ret != 0) {
-		remove_proc_entry("apm", NULL);
-		kthread_stop(kapmd_tsk);
-	}
+	if (ret)
+		goto out_stop;
+
+	ret = register_pm_notifier(&apm_notif_block);
+	if (ret)
+		goto out_unregister;
 
+	return 0;
+
+ out_unregister:
+	misc_deregister(&apm_device);
+ out_stop:
+	remove_proc_entry("apm", NULL);
+	kthread_stop(kapmd_tsk);
+ out:
 	return ret;
 }
 
 static void __exit apm_exit(void)
 {
+	unregister_pm_notifier(&apm_notif_block);
 	misc_deregister(&apm_device);
 	remove_proc_entry("apm", NULL);
 
--- everything.orig/kernel/power/main.c	2007-12-16 15:23:18.689514811 +0100
+++ everything/kernel/power/main.c	2007-12-16 15:31:06.179512207 +0100
@@ -25,6 +25,7 @@
 #include "power.h"
 
 BLOCKING_NOTIFIER_HEAD(pm_chain_head);
+EXPORT_SYMBOL_GPL(pm_chain_head);
 
 DEFINE_MUTEX(pm_mutex);
 

Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 466 bytes --]


> Yes, I think it should go through the suspend tree, so that we can avoid adding
> this export.  It would be confusing otherwise.

Yes, but I think it's a fix for a regression on 2.6.24, so benh may want
to get it in there.

> > +		 * FIXME: is the suspends_pending == 1 test racy?
> 
> Make suspends_pending atomic?  Then, you'd not need the locking around it.

Yeah, I guess that's the best way to handle it. I'll send an updated
patch.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Rafael J. Wysocki]

On Sunday, 16 of December 2007, Johannes Berg wrote:
> Currently, apm-emulation only implements notify/ack for events that are
> initiated by the apm-emulation code itself. That doesn't work out quite
> so well when X relies on the apm-emulation and you use scripts to
> suspend that simply use /sys/power/state. Ultimately, those scripts
> ought to notify userspace themselves, but currently X relies on apm
> notifications.
> 
> This patch implements the required notification.
> 
> Rafael already told me that the EXPORT_SYMBOL_GPL() this patch adds will
> no longer be required. If this patch is merged before the suspend tree
> (though it probably should go through the suspend tree), that is
> required, otherwise it should be removed.

Yes, I think it should go through the suspend tree, so that we can avoid adding
this export.  It would be confusing otherwise.

> I have *NOT* tested this patch with X, only with a simple dummy program
> that does the apm emulation code, available here:
> http://johannes.sipsolutions.net/files/test-apm-emu.c.txt
> 
> Not-yet-signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  drivers/char/apm-emulation.c |  108 ++++++++++++++++++++++++++++++++++++++++---
>  kernel/power/main.c          |    1 
>  2 files changed, 102 insertions(+), 7 deletions(-)
> 
> --- everything.orig/drivers/char/apm-emulation.c	2007-12-16 15:22:49.889513942 +0100
> +++ everything/drivers/char/apm-emulation.c	2007-12-16 18:57:54.555874295 +0100
> @@ -171,6 +171,8 @@ static void queue_event(apm_event_t even
>   * return -EBUSY.  Otherwise, queue an event to all "writer"
>   * users.  If there are no "writer" users, return '1' to
>   * indicate that we can immediately suspend.
> + *
> + * @sender can be NULL when the event is queued from /sys/power/state
>   */
>  static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
>  {
> @@ -207,10 +209,9 @@ static int queue_suspend_event(apm_event
>  	return ret;
>  }
>  
> -static void apm_suspend(void)
> +static void apm_after_resume(int err)
>  {
>  	struct apm_user *as;
> -	int err = pm_suspend(PM_SUSPEND_MEM);
>  
>  	/*
>  	 * Anyone on the APM queues will think we're still suspended.
> @@ -236,6 +237,13 @@ static void apm_suspend(void)
>  	wake_up(&apm_suspend_waitqueue);
>  }
>  
> +static void apm_suspend(void)
> +{
> +	int err = pm_suspend(PM_SUSPEND_MEM);
> +
> +	apm_after_resume(err);
> +}
> +
>  static ssize_t apm_read(struct file *fp, char __user *buf, size_t count, loff_t *ppos)
>  {
>  	struct apm_user *as = fp->private_data;
> @@ -320,6 +328,12 @@ apm_ioctl(struct inode * inode, struct f
>  			mutex_unlock(&state_lock);
>  
>  			/*
> +			 * suspends_pending changed, the notifier needs to be
> +			 * woken up for this
> +			 */
> +			wake_up(&apm_suspend_waitqueue);
> +
> +			/*
>  			 * If there are no further acknowledges required,
>  			 * suspend the system.
>  			 */
> @@ -404,6 +418,8 @@ static int apm_release(struct inode * in
>  		pending = suspends_pending == 0;
>  	}
>  	mutex_unlock(&state_lock);
> +	wake_up(&apm_suspend_waitqueue);
> +
>  	if (pending)
>  		apm_suspend();
>  
> @@ -575,6 +591,73 @@ static int kapmd(void *arg)
>  	return 0;
>  }
>  
> +int apm_suspend_notifier(struct notifier_block *nb,
> +			 unsigned long event,
> +			 void *dummy)
> +{
> +	int err;
> +
> +	switch (event) {
> +	case PM_SUSPEND_PREPARE:
> +		/*
> +		 * This is a bit of a hack. We tell the rest of the code that we
> +		 * still have a suspend pending and then go suspend ourselves.
> +		 * We have to do it this way because otherwise we'll call
> +		 * pm_sleep() when suspend_pending drops to zero which would
> +		 * return -EBUSY and confuse everything.
> +		 */
> +		mutex_lock(&state_lock);
> +		suspends_pending++;
> +		mutex_unlock(&state_lock);
> +
> +		err = queue_suspend_event(APM_SYS_SUSPEND, NULL);
> +		if (err < 0)
> +			return NOTIFY_BAD;
> +
> +		/* all's well, nobody to wait for */
> +		if (err > 0)
> +			return NOTIFY_OK;
> +
> +		/*
> +		 * Wait for the the suspends_pending variable to drop to 1,
> +		 * meaning everybody else acked the suspend event (or the
> +		 * process was killed.)
> +		 *
> +		 * Unfortunately we cannot do a timeout because then we'd
> +		 * suspend again right away if the process that had apm_bios
> +		 * open and that we timed out waiting for "acknowledges" the
> +		 * event after we have resumed. If suspend doesn't work because
> +		 * of a rogue process, just kill that process.
> +		 *
> +		 * FIXME: is the suspends_pending == 1 test racy?

Make suspends_pending atomic?  Then, you'd not need the locking around it.

> +		 */
> +		err = wait_event_interruptible(apm_suspend_waitqueue,
> +					       suspends_pending == 1);
> +
> +		mutex_lock(&state_lock);
> +		suspends_pending--;
> +		mutex_unlock(&state_lock);
> +
> +		if (!err)
> +			return NOTIFY_OK;
> +
> +		/* interrupted by signal */
> +		return NOTIFY_BAD;
> +
> +	case PM_POST_SUSPEND:
> +		/* TODO: maybe grab error code, needs core changes */
> +		apm_after_resume(0);
> +		return NOTIFY_OK;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +}
> +
> +struct notifier_block apm_notif_block = {
> +	.notifier_call = apm_suspend_notifier,
> +};
> +
>  static int __init apm_init(void)
>  {
>  	int ret;
> @@ -588,7 +671,7 @@ static int __init apm_init(void)
>  	if (IS_ERR(kapmd_tsk)) {
>  		ret = PTR_ERR(kapmd_tsk);
>  		kapmd_tsk = NULL;
> -		return ret;
> +		goto out;
>  	}
>  	wake_up_process(kapmd_tsk);
>  
> @@ -597,16 +680,27 @@ static int __init apm_init(void)
>  #endif
>  
>  	ret = misc_register(&apm_device);
> -	if (ret != 0) {
> -		remove_proc_entry("apm", NULL);
> -		kthread_stop(kapmd_tsk);
> -	}
> +	if (ret)
> +		goto out_stop;
> +
> +	ret = register_pm_notifier(&apm_notif_block);
> +	if (ret)
> +		goto out_unregister;
>  
> +	return 0;
> +
> + out_unregister:
> +	misc_deregister(&apm_device);
> + out_stop:
> +	remove_proc_entry("apm", NULL);
> +	kthread_stop(kapmd_tsk);
> + out:
>  	return ret;
>  }
>  
>  static void __exit apm_exit(void)
>  {
> +	unregister_pm_notifier(&apm_notif_block);
>  	misc_deregister(&apm_device);
>  	remove_proc_entry("apm", NULL);
>  
> --- everything.orig/kernel/power/main.c	2007-12-16 15:23:18.689514811 +0100
> +++ everything/kernel/power/main.c	2007-12-16 15:31:06.179512207 +0100
> @@ -25,6 +25,7 @@
>  #include "power.h"
>  
>  BLOCKING_NOTIFIER_HEAD(pm_chain_head);
> +EXPORT_SYMBOL_GPL(pm_chain_head);
>  
>  DEFINE_MUTEX(pm_mutex);

Re: Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Johannes Berg]

On Sun, 2007-12-16 at 19:59 +0100, Johannes Berg wrote:
> > Yes, I think it should go through the suspend tree, so that we can avoid adding
> > this export.  It would be confusing otherwise.
> 
> Yes, but I think it's a fix for a regression on 2.6.24, so benh may want
> to get it in there.

Actually, erm, I misspoke. My patch was merged in 2.6.21.

> > > +		 * FIXME: is the suspends_pending == 1 test racy?
> > 
> > Make suspends_pending atomic?  Then, you'd not need the locking around it.
> 
> Yeah, I guess that's the best way to handle it. I'll send an updated
> patch.

Patch below, please comment.

johannes
---
 drivers/char/apm-emulation.c |  119 +++++++++++++++++++++++++++++++++++++------
 kernel/power/main.c          |    1 
 2 files changed, 105 insertions(+), 15 deletions(-)

--- everything.orig/drivers/char/apm-emulation.c	2007-12-16 15:22:49.889513942 +0100
+++ everything/drivers/char/apm-emulation.c	2007-12-16 20:18:15.785873752 +0100
@@ -81,7 +81,7 @@ struct apm_user {
 /*
  * Local variables
  */
-static int suspends_pending;
+static atomic_t suspends_pending;
 static int apm_disabled;
 static struct task_struct *kapmd_tsk;
 
@@ -171,6 +171,8 @@ static void queue_event(apm_event_t even
  * return -EBUSY.  Otherwise, queue an event to all "writer"
  * users.  If there are no "writer" users, return '1' to
  * indicate that we can immediately suspend.
+ *
+ * @sender can be NULL when the event is queued from /sys/power/state
  */
 static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
 {
@@ -195,7 +197,7 @@ static int queue_suspend_event(apm_event
 	list_for_each_entry(as, &apm_user_list, list) {
 		if (as != sender && as->reader && as->writer && as->suser) {
 			as->suspend_state = SUSPEND_PENDING;
-			suspends_pending++;
+			atomic_inc(&suspends_pending);
 			queue_add_event(&as->queue, event);
 			ret = 0;
 		}
@@ -207,10 +209,9 @@ static int queue_suspend_event(apm_event
 	return ret;
 }
 
-static void apm_suspend(void)
+static void apm_after_resume(int err)
 {
 	struct apm_user *as;
-	int err = pm_suspend(PM_SUSPEND_MEM);
 
 	/*
 	 * Anyone on the APM queues will think we're still suspended.
@@ -236,6 +237,13 @@ static void apm_suspend(void)
 	wake_up(&apm_suspend_waitqueue);
 }
 
+static void apm_suspend(void)
+{
+	int err = pm_suspend(PM_SUSPEND_MEM);
+
+	apm_after_resume(err);
+}
+
 static ssize_t apm_read(struct file *fp, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct apm_user *as = fp->private_data;
@@ -315,11 +323,16 @@ apm_ioctl(struct inode * inode, struct f
 			 * interpreted as an acknowledge.
 			 */
 			as->suspend_state = SUSPEND_ACKED;
-			suspends_pending--;
-			pending = suspends_pending == 0;
+			pending = atomic_dec_and_test(&suspends_pending);
 			mutex_unlock(&state_lock);
 
 			/*
+			 * suspends_pending changed, the notifier needs to be
+			 * woken up for this
+			 */
+			wake_up(&apm_suspend_waitqueue);
+
+			/*
 			 * If there are no further acknowledges required,
 			 * suspend the system.
 			 */
@@ -399,11 +412,12 @@ static int apm_release(struct inode * in
 	 * possibility of sleeping.
 	 */
 	mutex_lock(&state_lock);
-	if (as->suspend_state != SUSPEND_NONE) {
-		suspends_pending -= 1;
-		pending = suspends_pending == 0;
-	}
+	if (as->suspend_state != SUSPEND_NONE)
+		pending = atomic_dec_and_test(&suspends_pending);
 	mutex_unlock(&state_lock);
+
+	wake_up(&apm_suspend_waitqueue);
+
 	if (pending)
 		apm_suspend();
 
@@ -575,6 +589,70 @@ static int kapmd(void *arg)
 	return 0;
 }
 
+int apm_suspend_notifier(struct notifier_block *nb,
+			 unsigned long event,
+			 void *dummy)
+{
+	int err;
+
+	switch (event) {
+	case PM_SUSPEND_PREPARE:
+		/*
+		 * This is a bit of a hack. We tell the rest of the code that we
+		 * still have a suspend pending and then go suspend ourselves.
+		 * We have to do it this way because otherwise we'll call
+		 * pm_sleep() when suspend_pending drops to zero which would
+		 * return -EBUSY and confuse everything.
+		 */
+		atomic_inc(&suspends_pending);
+
+		err = queue_suspend_event(APM_SYS_SUSPEND, NULL);
+		if (err < 0)
+			return NOTIFY_BAD;
+
+		/* all's well, nobody to wait for */
+		if (err > 0)
+			return NOTIFY_OK;
+
+		/*
+		 * Wait for the the suspends_pending variable to drop to 1,
+		 * meaning everybody else acked the suspend event (or the
+		 * process was killed.)
+		 *
+		 * Unfortunately we cannot do a timeout because then we'd
+		 * suspend again right away if the process that had apm_bios
+		 * open and that we timed out waiting for "acknowledges" the
+		 * event after we have resumed. If suspend doesn't work because
+		 * of a rogue process, just kill that process.
+		 *
+		 * FIXME: is the suspends_pending == 1 test racy?
+		 */
+		err = wait_event_interruptible(
+			apm_suspend_waitqueue,
+			atomic_read(&suspends_pending) == 1);
+
+		atomic_dec(&suspends_pending);
+
+		if (!err)
+			return NOTIFY_OK;
+
+		/* interrupted by signal */
+		return NOTIFY_BAD;
+
+	case PM_POST_SUSPEND:
+		/* TODO: maybe grab error code, needs core changes */
+		apm_after_resume(0);
+		return NOTIFY_OK;
+
+	default:
+		return NOTIFY_DONE;
+	}
+}
+
+struct notifier_block apm_notif_block = {
+	.notifier_call = apm_suspend_notifier,
+};
+
 static int __init apm_init(void)
 {
 	int ret;
@@ -588,7 +666,7 @@ static int __init apm_init(void)
 	if (IS_ERR(kapmd_tsk)) {
 		ret = PTR_ERR(kapmd_tsk);
 		kapmd_tsk = NULL;
-		return ret;
+		goto out;
 	}
 	wake_up_process(kapmd_tsk);
 
@@ -597,16 +675,27 @@ static int __init apm_init(void)
 #endif
 
 	ret = misc_register(&apm_device);
-	if (ret != 0) {
-		remove_proc_entry("apm", NULL);
-		kthread_stop(kapmd_tsk);
-	}
+	if (ret)
+		goto out_stop;
+
+	ret = register_pm_notifier(&apm_notif_block);
+	if (ret)
+		goto out_unregister;
+
+	return 0;
 
+ out_unregister:
+	misc_deregister(&apm_device);
+ out_stop:
+	remove_proc_entry("apm", NULL);
+	kthread_stop(kapmd_tsk);
+ out:
 	return ret;
 }
 
 static void __exit apm_exit(void)
 {
+	unregister_pm_notifier(&apm_notif_block);
 	misc_deregister(&apm_device);
 	remove_proc_entry("apm", NULL);
 
--- everything.orig/kernel/power/main.c	2007-12-16 15:23:18.689514811 +0100
+++ everything/kernel/power/main.c	2007-12-16 15:31:06.179512207 +0100
@@ -25,6 +25,7 @@
 #include "power.h"
 
 BLOCKING_NOTIFIER_HEAD(pm_chain_head);
+EXPORT_SYMBOL_GPL(pm_chain_head);
 
 DEFINE_MUTEX(pm_mutex);
 

Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Benjamin Herrenschmidt]

On Sun, 2007-12-16 at 20:15 +0100, Rafael J. Wysocki wrote:
> > +              *
> > +              * Unfortunately we cannot do a timeout because then we'd
> > +              * suspend again right away if the process that had apm_bios
> > +              * open and that we timed out waiting for "acknowledges" the
> > +              * event after we have resumed. If suspend doesn't work because
> > +              * of a rogue process, just kill that process.
> > +              *
> > +              * FIXME: is the suspends_pending == 1 test racy?

I think we can do the timeout thing easily... Just mark the fd's that
haven't ack'ed with a flag that makes us ignore the next ack...

Ben.

> Make suspends_pending atomic?  Then, you'd not need the locking around it.
> 
> > +              */
> > +             err = wait_event_interruptible(apm_suspend_waitqueue,
> > +                                            suspends_pending == 1);
> > +
> > +             mutex_lock(&state_lock);
> > +             suspends_pending--;
> > +             mutex_unlock(&state_lock);
> > +
> > +             if (!err)
> > +                     return NOTIFY_OK;
> > +
> > +             /* interrupted by signal */
> > +             return NOTIFY_BAD;
> > +
> > +     case PM_POST_SUSPEND:
> > +             /* TODO: maybe grab error code, needs core changes */
> > +             apm_after_resume(0);
> > +             return NOTIFY_OK;
> > +
> > +     default:
> > +             return NOTIFY_DONE;
> > +     }
> > +}
> > +
> > +struct notifier_block apm_notif_block = {
> > +     .notifier_call = apm_suspend_notifier,
> > +};
> > +
> >  static int __init apm_init(void)
> >  {
> >       int ret;
> > @@ -588,7 +671,7 @@ static int __init apm_init(void)
> >       if (IS_ERR(kapmd_tsk)) {
> >               ret = PTR_ERR(kapmd_tsk);
> >               kapmd_tsk = NULL;
> > -             return ret;
> > +             goto out;
> >       }
> >       wake_up_process(kapmd_tsk);
> >  
> > @@ -597,16 +680,27 @@ static int __init apm_init(void)
> >  #endif
> >  
> >       ret = misc_register(&apm_device);
> > -     if (ret != 0) {
> > -             remove_proc_entry("apm", NULL);
> > -             kthread_stop(kapmd_tsk);
> > -     }
> > +     if (ret)
> > +             goto out_stop;
> > +
> > +     ret = register_pm_notifier(&apm_notif_block);
> > +     if (ret)
> > +             goto out_unregister;
> >  
> > +     return 0;
> > +
> > + out_unregister:
> > +     misc_deregister(&apm_device);
> > + out_stop:
> > +     remove_proc_entry("apm", NULL);
> > +     kthread_stop(kapmd_tsk);
> > + out:
> >       return ret;
> >  }
> >  
> >  static void __exit apm_exit(void)
> >  {
> > +     unregister_pm_notifier(&apm_notif_block);
> >       misc_deregister(&apm_device);
> >       remove_proc_entry("apm", NULL);
> >  
> > --- everything.orig/kernel/power/main.c       2007-12-16
> 15:23:18.689514811 +0100
> > +++ everything/kernel/power/main.c    2007-12-16 15:31:06.179512207
> +0100
> > @@ -25,6 +25,7 @@
> >  #include "power.h"
> >  
> >  BLOCKING_NOTIFIER_HEAD(pm_chain_head);
> > +EXPORT_SYMBOL_GPL(pm_chain_head);
> >  
> >  DEFINE_MUTEX(pm_mutex);
> 

Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1000 bytes --]


On Mon, 2007-12-17 at 07:03 +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2007-12-16 at 20:15 +0100, Rafael J. Wysocki wrote:
> > > +              *
> > > +              * Unfortunately we cannot do a timeout because then we'd
> > > +              * suspend again right away if the process that had apm_bios
> > > +              * open and that we timed out waiting for "acknowledges" the
> > > +              * event after we have resumed. If suspend doesn't work because
> > > +              * of a rogue process, just kill that process.
> > > +              *
> > > +              * FIXME: is the suspends_pending == 1 test racy?
> 
> I think we can do the timeout thing easily... Just mark the fd's that
> haven't ack'ed with a flag that makes us ignore the next ack...

Yeah, except, is it really worth it? I should reword the comment into
"I'm too lazy to do it" I guess, but you need CAP_SYS_ADMIN and you can
always kill -9 the program that is making it wait.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Johannes Berg]

[reordering]

> > +              *
> > +              * FIXME: is the suspends_pending == 1 test racy?
> 
> Remove the FIXME comment?

Yes, doh, thanks!

> Hm, will suspends_pending drop if it gets killed?

Yes.

> With the timeout, we can either suspend even if there's a rogue process, or
> fail the suspend altogether.

I think it should suspend anyway.

> Failing should be safe even if the process acks
> the suspend afterwards.

Yes, it'd suspend then.

>   Suspending without an ack would be harder, but
> seems doable if we set (suspends_pending = 0) unconditionally below and use
> atomic_add_unless() instead of atomic_dec() in the other places (checking
> that for the result).

No, that'd be racy. We have to go through the list and dec
suspends_pending for everybody still in READ state, then what I do is
send them to ACKTO (ack timeout) state and return -ETIMEDOUT for their
ack ioctl if they're in ACKTO state.

Patch below, tested with my test program with an additional sleep(10) in
it.

johannes

---
 drivers/char/apm-emulation.c |  156 +++++++++++++++++++++++++++++++++++++------
 kernel/power/main.c          |    1 
 2 files changed, 138 insertions(+), 19 deletions(-)

--- everything.orig/drivers/char/apm-emulation.c	2007-12-16 15:22:49.889513942 +0100
+++ everything/drivers/char/apm-emulation.c	2007-12-16 21:42:43.749024090 +0100
@@ -74,6 +74,7 @@ struct apm_user {
 #define SUSPEND_ACKED	3		/* suspend acked */
 #define SUSPEND_WAIT	4		/* waiting for suspend */
 #define SUSPEND_DONE	5		/* suspend completed */
+#define SUSPEND_ACKTO	6		/* suspend ack timeout */
 
 	struct apm_queue	queue;
 };
@@ -81,7 +82,7 @@ struct apm_user {
 /*
  * Local variables
  */
-static int suspends_pending;
+static atomic_t suspends_pending;
 static int apm_disabled;
 static struct task_struct *kapmd_tsk;
 
@@ -171,6 +172,8 @@ static void queue_event(apm_event_t even
  * return -EBUSY.  Otherwise, queue an event to all "writer"
  * users.  If there are no "writer" users, return '1' to
  * indicate that we can immediately suspend.
+ *
+ * @sender can be NULL when the event is queued from /sys/power/state
  */
 static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
 {
@@ -195,7 +198,7 @@ static int queue_suspend_event(apm_event
 	list_for_each_entry(as, &apm_user_list, list) {
 		if (as != sender && as->reader && as->writer && as->suser) {
 			as->suspend_state = SUSPEND_PENDING;
-			suspends_pending++;
+			atomic_inc(&suspends_pending);
 			queue_add_event(&as->queue, event);
 			ret = 0;
 		}
@@ -207,10 +210,9 @@ static int queue_suspend_event(apm_event
 	return ret;
 }
 
-static void apm_suspend(void)
+static void apm_after_resume(int err)
 {
 	struct apm_user *as;
-	int err = pm_suspend(PM_SUSPEND_MEM);
 
 	/*
 	 * Anyone on the APM queues will think we're still suspended.
@@ -236,6 +238,13 @@ static void apm_suspend(void)
 	wake_up(&apm_suspend_waitqueue);
 }
 
+static void apm_suspend(void)
+{
+	int err = pm_suspend(PM_SUSPEND_MEM);
+
+	apm_after_resume(err);
+}
+
 static ssize_t apm_read(struct file *fp, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct apm_user *as = fp->private_data;
@@ -296,6 +305,7 @@ apm_ioctl(struct inode * inode, struct f
 {
 	struct apm_user *as = filp->private_data;
 	int err = -EINVAL;
+	int pending;
 
 	if (!as->suser || !as->writer)
 		return -EPERM;
@@ -306,20 +316,24 @@ apm_ioctl(struct inode * inode, struct f
 
 		as->suspend_result = -EINTR;
 
-		if (as->suspend_state == SUSPEND_READ) {
-			int pending;
-
+		switch (as->suspend_state) {
+		case SUSPEND_READ:
 			/*
 			 * If we read a suspend command from /dev/apm_bios,
 			 * then the corresponding APM_IOC_SUSPEND ioctl is
 			 * interpreted as an acknowledge.
 			 */
 			as->suspend_state = SUSPEND_ACKED;
-			suspends_pending--;
-			pending = suspends_pending == 0;
+			pending = atomic_dec_and_test(&suspends_pending);
 			mutex_unlock(&state_lock);
 
 			/*
+			 * suspends_pending changed, the notifier needs to be
+			 * woken up for this
+			 */
+			wake_up(&apm_suspend_waitqueue);
+
+			/*
 			 * If there are no further acknowledges required,
 			 * suspend the system.
 			 */
@@ -340,7 +354,13 @@ apm_ioctl(struct inode * inode, struct f
 			 * try_to_freeze() in freezer_count() will not trigger
 			 */
 			freezer_count();
-		} else {
+			break;
+		case SUSPEND_ACKTO:
+			as->suspend_state = SUSPEND_NONE;
+			as->suspend_result = -ETIMEDOUT;
+			mutex_unlock(&state_lock);
+			break;
+		default:
 			as->suspend_state = SUSPEND_WAIT;
 			mutex_unlock(&state_lock);
 
@@ -399,11 +419,12 @@ static int apm_release(struct inode * in
 	 * possibility of sleeping.
 	 */
 	mutex_lock(&state_lock);
-	if (as->suspend_state != SUSPEND_NONE) {
-		suspends_pending -= 1;
-		pending = suspends_pending == 0;
-	}
+	if (as->suspend_state != SUSPEND_NONE)
+		pending = atomic_dec_and_test(&suspends_pending);
 	mutex_unlock(&state_lock);
+
+	wake_up(&apm_suspend_waitqueue);
+
 	if (pending)
 		apm_suspend();
 
@@ -575,6 +596,92 @@ static int kapmd(void *arg)
 	return 0;
 }
 
+int apm_suspend_notifier(struct notifier_block *nb,
+			 unsigned long event,
+			 void *dummy)
+{
+	int err;
+
+	switch (event) {
+	case PM_SUSPEND_PREPARE:
+		/*
+		 * This is a bit of a hack. We tell the rest of the code that we
+		 * still have a suspend pending and then go suspend ourselves.
+		 * We have to do it this way because otherwise we'll call
+		 * pm_sleep() when suspend_pending drops to zero which would
+		 * return -EBUSY and confuse everything.
+		 */
+		atomic_inc(&suspends_pending);
+
+		err = queue_suspend_event(APM_SYS_SUSPEND, NULL);
+		if (err < 0)
+			return NOTIFY_BAD;
+
+		/* all's well, nobody to wait for */
+		if (err > 0)
+			return NOTIFY_OK;
+
+		/*
+		 * Wait for the the suspends_pending variable to drop to 1,
+		 * meaning everybody else acked the suspend event (or the
+		 * process was killed.)
+		 *
+		 * If the app won't answer within a short while we assume
+		 * it locked itself up and ignore it, the count will be
+		 * fixed up later.
+		 */
+		err = wait_event_interruptible_timeout(
+			apm_suspend_waitqueue,
+			atomic_read(&suspends_pending) == 1,
+			5*HZ);
+
+		/* timed out */
+		if (err == 0) {
+			struct apm_user *as;
+
+			/*
+			 * Move anybody who timed out to ack timeout state.
+			 *
+			 * We could time out and the userspace does the ACK
+			 * right after we time out but before we enter the
+			 * locked section here, but that's fine.
+			 */
+			mutex_lock(&state_lock);
+			down_read(&user_list_lock);
+			list_for_each_entry(as, &apm_user_list, list) {
+				if (as->suspend_state == SUSPEND_READ) {
+					printk(KERN_DEBUG "apm-emu: got timeout\n");
+					as->suspend_state = SUSPEND_ACKTO;
+					atomic_dec(&suspends_pending);
+				}
+			}
+			up_read(&user_list_lock);
+			mutex_unlock(&state_lock);
+		}
+
+		atomic_dec(&suspends_pending);
+
+		/* let suspend proceed */
+		if (err >= 0)
+			return NOTIFY_OK;
+
+		/* interrupted by signal */
+		return NOTIFY_BAD;
+
+	case PM_POST_SUSPEND:
+		/* TODO: maybe grab error code, needs core changes */
+		apm_after_resume(0);
+		return NOTIFY_OK;
+
+	default:
+		return NOTIFY_DONE;
+	}
+}
+
+struct notifier_block apm_notif_block = {
+	.notifier_call = apm_suspend_notifier,
+};
+
 static int __init apm_init(void)
 {
 	int ret;
@@ -588,7 +695,7 @@ static int __init apm_init(void)
 	if (IS_ERR(kapmd_tsk)) {
 		ret = PTR_ERR(kapmd_tsk);
 		kapmd_tsk = NULL;
-		return ret;
+		goto out;
 	}
 	wake_up_process(kapmd_tsk);
 
@@ -597,16 +704,27 @@ static int __init apm_init(void)
 #endif
 
 	ret = misc_register(&apm_device);
-	if (ret != 0) {
-		remove_proc_entry("apm", NULL);
-		kthread_stop(kapmd_tsk);
-	}
+	if (ret)
+		goto out_stop;
+
+	ret = register_pm_notifier(&apm_notif_block);
+	if (ret)
+		goto out_unregister;
 
+	return 0;
+
+ out_unregister:
+	misc_deregister(&apm_device);
+ out_stop:
+	remove_proc_entry("apm", NULL);
+	kthread_stop(kapmd_tsk);
+ out:
 	return ret;
 }
 
 static void __exit apm_exit(void)
 {
+	unregister_pm_notifier(&apm_notif_block);
 	misc_deregister(&apm_device);
 	remove_proc_entry("apm", NULL);
 
--- everything.orig/kernel/power/main.c	2007-12-16 15:23:18.689514811 +0100
+++ everything/kernel/power/main.c	2007-12-16 15:31:06.179512207 +0100
@@ -25,6 +25,7 @@
 #include "power.h"
 
 BLOCKING_NOTIFIER_HEAD(pm_chain_head);
+EXPORT_SYMBOL_GPL(pm_chain_head);
 
 DEFINE_MUTEX(pm_mutex);
 

Re: Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Rafael J. Wysocki]

On Sunday, 16 of December 2007, Johannes Berg wrote:
> 
> On Sun, 2007-12-16 at 19:59 +0100, Johannes Berg wrote:
> > > Yes, I think it should go through the suspend tree, so that we can avoid adding
> > > this export.  It would be confusing otherwise.
> > 
> > Yes, but I think it's a fix for a regression on 2.6.24, so benh may want
> > to get it in there.
> 
> Actually, erm, I misspoke. My patch was merged in 2.6.21.
> 
> > > > +		 * FIXME: is the suspends_pending == 1 test racy?
> > > 
> > > Make suspends_pending atomic?  Then, you'd not need the locking around it.
> > 
> > Yeah, I guess that's the best way to handle it. I'll send an updated
> > patch.
> 
> Patch below, please comment.
> 
> johannes
> ---
>  drivers/char/apm-emulation.c |  119 +++++++++++++++++++++++++++++++++++++------
>  kernel/power/main.c          |    1 
>  2 files changed, 105 insertions(+), 15 deletions(-)
> 
> --- everything.orig/drivers/char/apm-emulation.c	2007-12-16 15:22:49.889513942 +0100
> +++ everything/drivers/char/apm-emulation.c	2007-12-16 20:18:15.785873752 +0100
> @@ -81,7 +81,7 @@ struct apm_user {
>  /*
>   * Local variables
>   */
> -static int suspends_pending;
> +static atomic_t suspends_pending;
>  static int apm_disabled;
>  static struct task_struct *kapmd_tsk;
>  
> @@ -171,6 +171,8 @@ static void queue_event(apm_event_t even
>   * return -EBUSY.  Otherwise, queue an event to all "writer"
>   * users.  If there are no "writer" users, return '1' to
>   * indicate that we can immediately suspend.
> + *
> + * @sender can be NULL when the event is queued from /sys/power/state
>   */
>  static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
>  {
> @@ -195,7 +197,7 @@ static int queue_suspend_event(apm_event
>  	list_for_each_entry(as, &apm_user_list, list) {
>  		if (as != sender && as->reader && as->writer && as->suser) {
>  			as->suspend_state = SUSPEND_PENDING;
> -			suspends_pending++;
> +			atomic_inc(&suspends_pending);
>  			queue_add_event(&as->queue, event);
>  			ret = 0;
>  		}
> @@ -207,10 +209,9 @@ static int queue_suspend_event(apm_event
>  	return ret;
>  }
>  
> -static void apm_suspend(void)
> +static void apm_after_resume(int err)
>  {
>  	struct apm_user *as;
> -	int err = pm_suspend(PM_SUSPEND_MEM);
>  
>  	/*
>  	 * Anyone on the APM queues will think we're still suspended.
> @@ -236,6 +237,13 @@ static void apm_suspend(void)
>  	wake_up(&apm_suspend_waitqueue);
>  }
>  
> +static void apm_suspend(void)
> +{
> +	int err = pm_suspend(PM_SUSPEND_MEM);
> +
> +	apm_after_resume(err);
> +}
> +
>  static ssize_t apm_read(struct file *fp, char __user *buf, size_t count, loff_t *ppos)
>  {
>  	struct apm_user *as = fp->private_data;
> @@ -315,11 +323,16 @@ apm_ioctl(struct inode * inode, struct f
>  			 * interpreted as an acknowledge.
>  			 */
>  			as->suspend_state = SUSPEND_ACKED;
> -			suspends_pending--;
> -			pending = suspends_pending == 0;
> +			pending = atomic_dec_and_test(&suspends_pending);
>  			mutex_unlock(&state_lock);
>
>  			/*
> +			 * suspends_pending changed, the notifier needs to be
> +			 * woken up for this
> +			 */
> +			wake_up(&apm_suspend_waitqueue);
> +
> +			/*
>  			 * If there are no further acknowledges required,
>  			 * suspend the system.
>  			 */
> @@ -399,11 +412,12 @@ static int apm_release(struct inode * in
>  	 * possibility of sleeping.
>  	 */
>  	mutex_lock(&state_lock);
> -	if (as->suspend_state != SUSPEND_NONE) {
> -		suspends_pending -= 1;
> -		pending = suspends_pending == 0;
> -	}
> +	if (as->suspend_state != SUSPEND_NONE)
> +		pending = atomic_dec_and_test(&suspends_pending);
>  	mutex_unlock(&state_lock);
> +
> +	wake_up(&apm_suspend_waitqueue);
> +
>  	if (pending)
>  		apm_suspend();
>  
> @@ -575,6 +589,70 @@ static int kapmd(void *arg)
>  	return 0;
>  }
>  
> +int apm_suspend_notifier(struct notifier_block *nb,
> +			 unsigned long event,
> +			 void *dummy)
> +{
> +	int err;
> +
> +	switch (event) {
> +	case PM_SUSPEND_PREPARE:
> +		/*
> +		 * This is a bit of a hack. We tell the rest of the code that we
> +		 * still have a suspend pending and then go suspend ourselves.
> +		 * We have to do it this way because otherwise we'll call
> +		 * pm_sleep() when suspend_pending drops to zero which would
> +		 * return -EBUSY and confuse everything.
> +		 */
> +		atomic_inc(&suspends_pending);
> +
> +		err = queue_suspend_event(APM_SYS_SUSPEND, NULL);
> +		if (err < 0)
> +			return NOTIFY_BAD;
> +
> +		/* all's well, nobody to wait for */
> +		if (err > 0)
> +			return NOTIFY_OK;
> +
> +		/*
> +		 * Wait for the the suspends_pending variable to drop to 1,
> +		 * meaning everybody else acked the suspend event (or the
> +		 * process was killed.)
> +		 *
> +		 * Unfortunately we cannot do a timeout because then we'd
> +		 * suspend again right away if the process that had apm_bios
> +		 * open and that we timed out waiting for "acknowledges" the
> +		 * event after we have resumed. If suspend doesn't work because
> +		 * of a rogue process, just kill that process.

Hm, will suspends_pending drop if it gets killed?

With the timeout, we can either suspend even if there's a rogue process, or
fail the suspend altogether.  Failing should be safe even if the process acks
the suspend afterwards.  Suspending without an ack would be harder, but
seems doable if we set (suspends_pending = 0) unconditionally below and use
atomic_add_unless() instead of atomic_dec() in the other places (checking
that for the result).

> +		 *
> +		 * FIXME: is the suspends_pending == 1 test racy?

Remove the FIXME comment?

> +		 */
> +		err = wait_event_interruptible(
> +			apm_suspend_waitqueue,
> +			atomic_read(&suspends_pending) == 1);
> +		atomic_dec(&suspends_pending);
> +
> +		if (!err)
> +			return NOTIFY_OK;
> +
> +		/* interrupted by signal */
> +		return NOTIFY_BAD;

		return err ? NOTIFY_BAD : NOTIFY_OK;

would be shorter. ;-)

> +
> +	case PM_POST_SUSPEND:
> +		/* TODO: maybe grab error code, needs core changes */
> +		apm_after_resume(0);
> +		return NOTIFY_OK;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +}
> +
> +struct notifier_block apm_notif_block = {
> +	.notifier_call = apm_suspend_notifier,
> +};
> +
>  static int __init apm_init(void)
>  {
>  	int ret;
> @@ -588,7 +666,7 @@ static int __init apm_init(void)
>  	if (IS_ERR(kapmd_tsk)) {
>  		ret = PTR_ERR(kapmd_tsk);
>  		kapmd_tsk = NULL;
> -		return ret;
> +		goto out;
>  	}
>  	wake_up_process(kapmd_tsk);
>  
> @@ -597,16 +675,27 @@ static int __init apm_init(void)
>  #endif
>  
>  	ret = misc_register(&apm_device);
> -	if (ret != 0) {
> -		remove_proc_entry("apm", NULL);
> -		kthread_stop(kapmd_tsk);
> -	}
> +	if (ret)
> +		goto out_stop;
> +
> +	ret = register_pm_notifier(&apm_notif_block);
> +	if (ret)
> +		goto out_unregister;
> +
> +	return 0;
>  
> + out_unregister:
> +	misc_deregister(&apm_device);
> + out_stop:
> +	remove_proc_entry("apm", NULL);
> +	kthread_stop(kapmd_tsk);
> + out:
>  	return ret;
>  }
>  
>  static void __exit apm_exit(void)
>  {
> +	unregister_pm_notifier(&apm_notif_block);
>  	misc_deregister(&apm_device);
>  	remove_proc_entry("apm", NULL);
>  
> --- everything.orig/kernel/power/main.c	2007-12-16 15:23:18.689514811 +0100
> +++ everything/kernel/power/main.c	2007-12-16 15:31:06.179512207 +0100
> @@ -25,6 +25,7 @@
>  #include "power.h"
>  
>  BLOCKING_NOTIFIER_HEAD(pm_chain_head);
> +EXPORT_SYMBOL_GPL(pm_chain_head);
>  
>  DEFINE_MUTEX(pm_mutex);

Re: Re: [RFC] apm-emulation: implement notify/ack for /sys/power/state events

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 206 bytes --]


> Patch below, tested with my test program with an additional sleep(10) in
> it.

Of course, that patch is going to behave very badly if we're asked to
suspend from within apm! Will fix.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]