Message-Id: <201004282313.50603.rjw@sisk.pl>
Date:	Wed, 28 Apr 2010 23:13:50 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Alan Stern <stern@...land.harvard.edu>,
	Arve Hjønnevåg <arve@...roid.com>
Cc:	"Linux-pm mailing list" <linux-pm@...ts.linux-foundation.org>,
	Kernel development list <linux-kernel@...r.kernel.org>,
	Tejun Heo <tj@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	Randy Dunlap <rdunlap@...otime.net>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Nigel Cunningham <nigel@...onice.net>,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	Ming Lei <tom.leiming@...il.com>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Maxim Levitsky <maximlevitsky@...il.com>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/8] PM: Add suspend block api.

On Wednesday 28 April 2010, Alan Stern wrote:
> On Tue, 27 Apr 2010, [UTF-8] Arve Hjønnevåg wrote:
> 
> > +For example, in cell phones or other embedded systems, where powering the screen
> > +is a significant drain on the battery, suspend blockers can be used to allow
> > +user-space to decide whether a keystroke received while the system is suspended
> > +should cause the screen to be turned back on or allow the system to go back into
> > +suspend. Use set_irq_wake or a platform specific api to make sure the keypad
> > +interrupt wakes up the cpu. Once the keypad driver has resumed, the sequence of
> > +events can look like this:
> > +
> > +- The Keypad driver gets an interrupt. It then calls suspend_block on the
> > +  keypad-scan suspend_blocker and starts scanning the keypad matrix.
> > +- The keypad-scan code detects a key change and reports it to the input-event
> > +  driver.
> > +- The input-event driver sees the key change, enqueues an event, and calls
> > +  suspend_block on the input-event-queue suspend_blocker.
> > +- The keypad-scan code detects that no keys are held and calls suspend_unblock
> > +  on the keypad-scan suspend_blocker.
> > +- The user-space input-event thread returns from select/poll, calls
> > +  suspend_block on the process-input-events suspend_blocker and then calls read
> > +  on the input-event device.
> > +- The input-event driver dequeues the key-event and, since the queue is now
> > +  empty, it calls suspend_unblock on the input-event-queue suspend_blocker.
> > +- The user-space input-event thread returns from read. If it determines that
> > +  the key should leave the screen off, it calls suspend_unblock on the
> > +  process_input_events suspend_blocker and then calls select or poll. The
> > +  system will automatically suspend again, since now no suspend blockers are
> > +  active.
> > +
> > +                 Key pressed   Key released
> > +                     |             |
> > +keypad-scan          ++++++++++++++++++
> > +input-event-queue        +++          +++
> > +process-input-events       +++          +++
> 
> This is better than before, but it still isn't ideal.  Here's what I 
> mean:
> 
> >  suspend blockers can be used to allow
> > +user-space to decide whether a keystroke received while the system is suspended
> > +should cause the screen to be turned back on or allow the system to go back into
> > +suspend.
> 
> That's not right.  Handling the screen doesn't need suspend blockers:
> The program decides what to do and then either turns on the screen or
> else writes "mem" to /sys/power/state.  What suspend blockers add is
> the ability to resolve races and satisfy multiple constraints when
> going into suspend -- which has nothing to do with operating the
> screen.
> 
> I _think_ what you're trying to get at can be expressed this way:
> 
> 	Here's an example showing how a cell phone or other embedded
> 	system can handle keystrokes (or other input events) in the 
> 	presence of suspend blockers.  Use set_irq_wake...
> 
> 	...
> 
>       - The user-space input-event thread returns from read.  It 
> 	carries out whatever activities are appropriate (for example,
> 	powering up the display screen, running other programs, and so 
> 	on).  When it is finished, it calls suspend_unblock on the
> 	process_input_events suspend_blocker and then calls select or
> 	poll.  The system will automatically suspend again when it is
> 	idle and no suspend blockers remain active.

Yeah, that sounds better.  Arve, what do you think?

> > +/**
> > + * suspend_block() - Block suspend
> > + * @blocker:	The suspend blocker to use
> > + *
> > + * It is safe to call this function from interrupt context.
> > + */
> > +void suspend_block(struct suspend_blocker *blocker)
> > +{
> > +	unsigned long irqflags;
> > +
> > +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> > +		return;
> > +
> > +	spin_lock_irqsave(&list_lock, irqflags);
> > +	blocker->flags |= SB_ACTIVE;
> > +	list_del(&blocker->link);
> > +
> > +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> > +		pr_info("suspend_block: %s\n", blocker->name);
> > +
> > +	list_add(&blocker->link, &active_blockers);
> 
> Here and in suspend_unblock(), you can use list_move() in place of 
> list_del() followed by list_add().

Indeed.  And the debug statement might be moved out of the critical section IMHO.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists