Re: [PATCH 1/8] PM: Add suspend block api.

["Rafael J. Wysocki" <rjw@sisk.pl>]

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