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);
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
