Re: [PATCH 06/13] PM: Implement early suspend api

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Sat, 2009-02-07 at 23:47 +0100, Rafael J. Wysocki wrote:
> On Thursday 05 February 2009, Arve Hjønnevåg wrote:
> > Signed-off-by: Arve Hjønnevåg <arve@android.com>
> 
> I don't really agree with this design, because I don't think it's suitable for
> all bus types.  Namely, if any bus type partially handles suspend-resume of
> its devices to offload device drivers, I don't see how that is going to work
> along with your design.
> 
> You seem to require the drivers that will register the early resume handlers
> to take care of everything, along with the operations that should really belong
> to bus types.  I don't think it's a good approach. 

More specifically, with the work you (Rafael) are doing at the moment,
things like PCI or other low level busses would be fully restored in
the early resume phase before drivers gets their resume_early.
Henceforth, it would be possible for things like fbdev's etc... to
resume from resume_early() which is probably soon enough.

Ie. I currently have this hack in radeonfb to resume it even before that
while IRQs are still off etc... but it's a can of worms due to various
might_sleep() hits in subsystems left and right (PCI, AGP, etc...).

I'm actually thinking that when I migrate all of that to radeon DRM/KMS,
I'll probably stick it all just in late_suspend/early_resume provided
our plans to change the interrupt disabling go in and might_sleep()
becomes safe in that phase.

I don't see this Android early_suspend stuff fitting anywhere in that
scheme... looks to me like some people hacked up some ad-hoc trick for
their own local need without instead trying to figure out how to fit
things with the existing infrastructure (or possibly propose changes to
the existing infrastructure to fit their needs).

Ben.

> Thanks,
> Rafael
> 
> > ---
> >  kernel/power/Kconfig        |   12 +++
> >  kernel/power/Makefile       |    1 +
> >  kernel/power/earlysuspend.c |  178 +++++++++++++++++++++++++++++++++++++++++++
> >  kernel/power/power.h        |    6 ++
> >  4 files changed, 197 insertions(+), 0 deletions(-)
> >  create mode 100644 kernel/power/earlysuspend.c
> > 
> > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > index dd76467..689abfe 100644
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -119,6 +119,9 @@ config SUSPEND_FREEZER
> >  config HAS_WAKELOCK
> >  	bool
> >  
> > +config HAS_EARLYSUSPEND
> > +	bool
> > +
> >  config WAKELOCK
> >  	bool "Wake lock"
> >  	depends on PM && RTC_CLASS
> > @@ -144,6 +147,15 @@ config DISABLE_SYS_POWER_STATE
> >  	  want to run user-space code that does not support wakelocks, do not
> >  	  enable this option since it removes the interface.
> >  
> > +config EARLYSUSPEND
> > +	bool "Early suspend"
> > +	depends on WAKELOCK
> > +	default y
> > +	select HAS_EARLYSUSPEND
> > +	---help---
> > +	  Call early suspend handlers when the user requested sleep state
> > +	  changes.
> > +
> >  config HIBERNATION
> >  	bool "Hibernation (aka 'suspend to disk')"
> >  	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
> > diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> > index 8d8672b..2f17e1d 100644
> > --- a/kernel/power/Makefile
> > +++ b/kernel/power/Makefile
> > @@ -7,6 +7,7 @@ obj-y				:= main.o
> >  obj-$(CONFIG_PM_SLEEP)		+= console.o
> >  obj-$(CONFIG_FREEZER)		+= process.o
> >  obj-$(CONFIG_WAKELOCK)		+= wakelock.o
> > +obj-$(CONFIG_EARLYSUSPEND)	+= earlysuspend.o
> >  obj-$(CONFIG_HIBERNATION)	+= swsusp.o disk.o snapshot.o swap.o user.o
> >  
> >  obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
> > diff --git a/kernel/power/earlysuspend.c b/kernel/power/earlysuspend.c
> > new file mode 100644
> > index 0000000..4d70a7e
> > --- /dev/null
> > +++ b/kernel/power/earlysuspend.c
> > @@ -0,0 +1,178 @@
> > +/* kernel/power/earlysuspend.c
> > + *
> > + * Copyright (C) 2005-2008 Google, Inc.
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/earlysuspend.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/rtc.h>
> > +#include <linux/syscalls.h> /* sys_sync */
> > +#include <linux/wakelock.h>
> > +#include <linux/workqueue.h>
> > +
> > +#include "power.h"
> > +
> > +enum {
> > +	DEBUG_USER_STATE = 1U << 0,
> > +	DEBUG_SUSPEND = 1U << 1,
> > +};
> > +static int debug_mask = DEBUG_USER_STATE;
> > +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> > +
> > +static DEFINE_MUTEX(early_suspend_lock);
> > +static LIST_HEAD(early_suspend_handlers);
> > +static void early_suspend(struct work_struct *work);
> > +static void late_resume(struct work_struct *work);
> > +static DECLARE_WORK(early_suspend_work, early_suspend);
> > +static DECLARE_WORK(late_resume_work, late_resume);
> > +static DEFINE_SPINLOCK(state_lock);
> > +enum {
> > +	SUSPEND_REQUESTED = 0x1,
> > +	SUSPENDED = 0x2,
> > +	SUSPEND_REQUESTED_AND_SUSPENDED = SUSPEND_REQUESTED | SUSPENDED,
> > +};
> > +static int state;
> > +
> > +void register_early_suspend(struct early_suspend *handler)
> > +{
> > +	struct list_head *pos;
> > +
> > +	mutex_lock(&early_suspend_lock);
> > +	list_for_each(pos, &early_suspend_handlers) {
> > +		struct early_suspend *e;
> > +		e = list_entry(pos, struct early_suspend, link);
> > +		if (e->level > handler->level)
> > +			break;
> > +	}
> > +	list_add_tail(&handler->link, pos);
> > +	if ((state & SUSPENDED) && handler->suspend)
> > +		handler->suspend(handler);
> > +	mutex_unlock(&early_suspend_lock);
> > +}
> > +EXPORT_SYMBOL(register_early_suspend);
> > +
> > +void unregister_early_suspend(struct early_suspend *handler)
> > +{
> > +	mutex_lock(&early_suspend_lock);
> > +	list_del(&handler->link);
> > +	mutex_unlock(&early_suspend_lock);
> > +}
> > +EXPORT_SYMBOL(unregister_early_suspend);
> > +
> > +static void early_suspend(struct work_struct *work)
> > +{
> > +	struct early_suspend *pos;
> > +	unsigned long irqflags;
> > +	int abort = 0;
> > +
> > +	mutex_lock(&early_suspend_lock);
> > +	spin_lock_irqsave(&state_lock, irqflags);
> > +	if (state == SUSPEND_REQUESTED)
> > +		state |= SUSPENDED;
> > +	else
> > +		abort = 1;
> > +	spin_unlock_irqrestore(&state_lock, irqflags);
> > +
> > +	if (abort) {
> > +		if (debug_mask & DEBUG_SUSPEND)
> > +			pr_info("early_suspend: abort, state %d\n", state);
> > +		mutex_unlock(&early_suspend_lock);
> > +		goto abort;
> > +	}
> > +
> > +	if (debug_mask & DEBUG_SUSPEND)
> > +		pr_info("early_suspend: call handlers\n");
> > +	list_for_each_entry(pos, &early_suspend_handlers, link) {
> > +		if (pos->suspend)
> > +			pos->suspend(pos);
> > +	}
> > +	mutex_unlock(&early_suspend_lock);
> > +
> > +	if (debug_mask & DEBUG_SUSPEND)
> > +		pr_info("early_suspend: sync\n");
> > +
> > +	sys_sync();
> > +abort:
> > +	spin_lock_irqsave(&state_lock, irqflags);
> > +	if (state == SUSPEND_REQUESTED_AND_SUSPENDED)
> > +		wake_unlock(&main_wake_lock);
> > +	spin_unlock_irqrestore(&state_lock, irqflags);
> > +}
> > +
> > +static void late_resume(struct work_struct *work)
> > +{
> > +	struct early_suspend *pos;
> > +	unsigned long irqflags;
> > +	int abort = 0;
> > +
> > +	mutex_lock(&early_suspend_lock);
> > +	spin_lock_irqsave(&state_lock, irqflags);
> > +	if (state == SUSPENDED)
> > +		state = 0; /* clear SUSPENDED */
> > +	else
> > +		abort = 1;
> > +	spin_unlock_irqrestore(&state_lock, irqflags);
> > +
> > +	if (abort) {
> > +		if (debug_mask & DEBUG_SUSPEND)
> > +			pr_info("late_resume: abort, state %d\n", state);
> > +		goto abort;
> > +	}
> > +	if (debug_mask & DEBUG_SUSPEND)
> > +		pr_info("late_resume: call handlers\n");
> > +	list_for_each_entry_reverse(pos, &early_suspend_handlers, link)
> > +		if (pos->resume)
> > +			pos->resume(pos);
> > +	if (debug_mask & DEBUG_SUSPEND)
> > +		pr_info("late_resume: done\n");
> > +abort:
> > +	mutex_unlock(&early_suspend_lock);
> > +}
> > +
> > +void request_suspend_state(suspend_state_t new_state)
> > +{
> > +	unsigned long irqflags;
> > +	int old_sleep;
> > +
> > +	spin_lock_irqsave(&state_lock, irqflags);
> > +	old_sleep = state & SUSPEND_REQUESTED;
> > +	if (debug_mask & DEBUG_USER_STATE) {
> > +		struct timespec ts;
> > +		struct rtc_time tm;
> > +		getnstimeofday(&ts);
> > +		rtc_time_to_tm(ts.tv_sec, &tm);
> > +		pr_info("request_suspend_state: %s (%d->%d) at %lld "
> > +			"(%d-%02d-%02d %02d:%02d:%02d.%09lu UTC)\n",
> > +			new_state != PM_SUSPEND_ON ? "sleep" : "wakeup",
> > +			requested_suspend_state, new_state,
> > +			ktime_to_ns(ktime_get()),
> > +			tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> > +			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec);
> > +	}
> > +	if (!old_sleep && new_state != PM_SUSPEND_ON) {
> > +		state |= SUSPEND_REQUESTED;
> > +		queue_work(suspend_work_queue, &early_suspend_work);
> > +	} else if (old_sleep && new_state == PM_SUSPEND_ON) {
> > +		state &= ~SUSPEND_REQUESTED;
> > +		wake_lock(&main_wake_lock);
> > +		queue_work(suspend_work_queue, &late_resume_work);
> > +	}
> > +	requested_suspend_state = new_state;
> > +	spin_unlock_irqrestore(&state_lock, irqflags);
> > +}
> > +
> > +suspend_state_t get_suspend_state(void)
> > +{
> > +	return requested_suspend_state;
> > +}
> > diff --git a/kernel/power/power.h b/kernel/power/power.h
> > index ed1b7f4..acbb13a 100644
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -231,3 +231,9 @@ extern struct wake_lock main_wake_lock;
> >  extern suspend_state_t requested_suspend_state;
> >  extern bool ignore_suspend_wakelocks;
> >  #endif
> > +
> > +#ifdef CONFIG_EARLYSUSPEND
> > +/* kernel/power/earlysuspend.c */
> > +void request_suspend_state(suspend_state_t state);
> > +suspend_state_t get_suspend_state(void);
> > +#endif
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm