Date:	Sat, 19 Jul 2014 01:47:54 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Dmitry Torokhov <dtor@...gle.com>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Patrik Fimml <patrikf@...omium.org>,
	Bastien Nocera <hadess@...ess.net>, linux-pm@...r.kernel.org,
	Benson Leung <bleung@...gle.com>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: Power-managing devices that are not of interest at some point in time

On Friday, July 18, 2014 04:16:50 PM Dmitry Torokhov wrote:
> On Saturday, July 19, 2014 12:55:09 AM Rafael J. Wysocki wrote:
> > On Saturday, July 19, 2014 12:19:39 AM Rafael J. Wysocki wrote:
> > > On Friday, July 18, 2014 02:45:40 PM Dmitry Torokhov wrote:
> > > > On Friday, July 18, 2014 11:59:18 PM Rafael J. Wysocki wrote:
> > > > > On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> > > > > > On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > > > > > > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > > > > > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > > > > [cut]
> > > > > 
> > > > > > > > I'm not sure what the appropriate action for a video camera is
> > > > > > > > anyway.
> > > > > > > > Should it go away completely, including its device? Should it be
> > > > > > > > there,
> > > > > > > > but certainly not be the default choice when there is an
> > > > > > > > external
> > > > > > > > camera? I'm thinking along the lines of some application's
> > > > > > > > settings
> > > > > > > > dialog here, where it might be desirable to still be able to
> > > > > > > > select
> > > > > > > > the
> > > > > > > > internal camera for future recordings.
> > > > > > > > 
> > > > > > > > Of course, userspace could still decide simply not to
> > > > > > > > quiesce|deactivate|inhibit the device if that was desired.
> > > > > > > 
> > > > > > > There's some question about how much of userspace needs to get
> > > > > > > involved.  Just the daemon that manages these configuration
> > > > > > > changes, or
> > > > > > > other programs as well?  I guess that's not really our problem...
> > > > > > 
> > > > > > We need to provide means of implementing policy; the policy itself
> > > > > > is not
> > > > > > really our concern ;)
> > > > > > 
> > > > > > > In the end, it sounds like you're suggesting a new pair of PM
> > > > > > > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > > > > > > ->uninhibit().  Plus an optional (?) sysfs interface for invoking
> > > > > > > the
> > > > > > > callbacks.
> > > > > > 
> > > > > > We do need sysfs interface so that userspace can talk to the devices
> > > > > > in
> > > > > > question; and we also need to make sure that PM core is aware of the
> > > > > > new
> > > > > > callbacks and provides guarantees about their interactions with
> > > > > > system-
> > > > > > and
> > > > > > runtime-PM callbacks so that individual drivers do not have to sort
> > > > > > it out
> > > > > > on their own.
> > > > > 
> > > > > A step back, please.
> > > > > 
> > > > > I have no idea why those need to be PM callbacks.
> > > > > 
> > > > > What you need really seems to be a way to tell a driver "ignore input
> > > > > from
> > > > > this device from now on as it is most likely bogus".  A natural
> > > > > reaction of
> > > > > the driver to that might be to stop processing input from the device
> > > > > and
> > > > > then runtime suspend it (and prevent it from generating remote wakeup
> > > > > as
> > > > > that may be bogus as well), but I don't see why the PM core needs to
> > > > > be
> > > > > involved in that at all.
> > > > 
> > > > So that we do not need to handle cases like:
> > > > 
> > > > - I am already in idle state and request comes to inhibit, what do I do
> > > > (in
> > > 
> > > > driver) or:
> > > I'm not sure why being "suspended" or not matters here.  The PM core
> > > doesn't know what physical state the device is in anyway and the driver
> > > or subsystem (or another layer such as ACPI) has to track that.
> > > 
> > > Also it seems that it should be perfectly fine to ignore input from the
> > > device without suspending it as well as it is perfectly fine to be
> > > suspended while you are generally not ignoring the input (just because
> > > there is no input at the moment, for example).
> > > 
> > > Yes, it make sense to suspend the device when you know you'll ignore input
> > > going forward, but then if the real goal is to prevent bogus input from
> > > reaching applications, then this isn't a power management problem even.
> > 
> > The area where it must interact with power management is wakeup, both remote
> > wakeup at run time and wakeup from system suspend.  In particular, there's
> > the question whether or not a device ignoring its input should be regarded
> > as a wakeup source.
> 
> I'd say no.
> 
> Anyway, even though it is very tempting to declare inhibit a "deeper" state of 
> runtime suspend maybe you are right and inhibit should really be separate from 
> PM and drivers would have to sort out all the possible state permutations.
> 
> Considering input devices:
> 
> input_open(): check if device is inhibited, if so do nothing. Otherwise try 
> waking up itself and parent (via pm_runtime_get_sync() on itself), this will 
> power up the device. Do additional configuration if needed.
> 
> input_close(): check if device is inhibited, if not do pm_runtime_put (_sync? 
> to make sure we power off properly and not leave device up and running? or 
> should we power down manually not waiting for runtime PM)?

pm_runtime_put_sync() should be sufficient here I think.

> inhibit(): check if device is opened, if opened do pm_runtime_put_sync().
> 
> uninhibit(): if device is opened do pm_runtime_get_sync(), let runtime PM 
> bring up the device. Do additional config if needed -> very similar to 
> input_open(), different condition.
> 
> runtime_suspend(): power down the device. If not inhibited enable as wakeup 
> source.

User space may not want it to be a wakeup source even if not inhibited
(it may not want it to wake up from system suspend, for example, like I do
with my cordless mouse).

> runtime_resume(): power up the device if device is opened and not inhibited.
> 
> system_suspend(): check if device is opened, not inhibited and not in 
> runtimesuspend  already; power down.

I think you only need to know if the device has been powered down already
here regardless of the reason.

> system_resume(): power up the device if it is opened and not inhibited. I 
> guess it's OK to wake up device that shoudl be runtime-PM-idle since it will 
> go to back sleep shortly.
> 
> Ugh.. This is complicated...

If runtime PM and system suspend shared the status, we might simplify this
somewhat.  Problem is, there are drivers that don't support runtime PM, but
support system suspend.

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