Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755752Ab0D1Xfq (ORCPT ); Wed, 28 Apr 2010 19:35:46 -0400 Received: from mail-px0-f174.google.com ([209.85.212.174]:57032 "EHLO mail-px0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754016Ab0D1Xfo convert rfc822-to-8bit (ORCPT ); Wed, 28 Apr 2010 19:35:44 -0400 MIME-Version: 1.0 In-Reply-To: <201004282313.50603.rjw@sisk.pl> References: <201004282313.50603.rjw@sisk.pl> Date: Wed, 28 Apr 2010 16:35:43 -0700 Message-ID: Subject: Re: [PATCH 1/8] PM: Add suspend block api. From: =?ISO-8859-1?Q?Arve_Hj=F8nnev=E5g?= To: "Rafael J. Wysocki" Cc: Alan Stern , Linux-pm mailing list , Kernel development list , Tejun Heo , Oleg Nesterov , Len Brown , Pavel Machek , Randy Dunlap , Jesse Barnes , Nigel Cunningham , Cornelia Huck , Ming Lei , Wu Fengguang , Andrew Morton , Maxim Levitsky , linux-doc@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5584 Lines: 131 2010/4/28 Rafael J. Wysocki : > 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. That does not work though. Unless every key turns the screen on you will have a race every time the user presses a key you want to ignore. >> ?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'm not sure I agree with this. You cannot reliably turn the screen on from user space when the user presses a wakeup-key without suspend blockers. >> >> 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... OK, but the last version was what you (Alan) suggested last year. >> >> ? ? ? ... >> >> ? ? ? - 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? > Idle is irrelevant and needs to be removed. This new last step is also no longer a concrete example, but if you really think is it better I can change it. >> > +/** >> > + * 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(). > OK. > Indeed. ?And the debug statement might be moved out of the critical section IMHO. > If I move the debug statements out of the critical section you could end entering suspend while the debug log claims a suspend blocker was active, but I can move the debug statement to the start of the critical section. -- Arve Hj?nnev?g -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/