[RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Irina Tirdea]

Add new option to sysfs control interface, allowing the user to force
suspend the device. This is useful for devices that need to be
suspended when closing the lid of a laptop or the screen of a mobile
device, while userspace still holds open handles to it and the
system does not enter system suspend.

Add the "off" option to the sysfs control power interface, along with
the already present "on" and "auto". When this attribute is set to
"off", the device will be force suspended by calling its runtime
suspend callback and disabling runtime power management so that
further acceses to the device will not change the actual state.
The device can be resumed by setting the attribute to "on" or "auto".
The behaviour of the interface when switching only between "on"
and "auto" states remains unchanged.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---

Hi,

This is a proposal for suspending devices when the closing the lid of
a laptop or the screen of a mobile device.

I am testing this with a Goodix touchscreen [1] for an Android mobile
device. Android has an userspace layer (power HAL) that would normally
close the touchscreen when the screen is closed. Android touchscreen
drivers usually provide a custom sysfs interface to allow this.
This would be better implemented in a common place, to avoid code
duplication and to simplify the driver code (as previosly discussed
in [1]).

I know there are more ways to implement this, so I would appreciate
your feedback.

Thank you,
Irina

[1] https://lkml.org/lkml/2015/9/7/329
[2] https://lkml.org/lkml/2014/7/15/928

 drivers/base/power/main.c    |   8 +++
 drivers/base/power/runtime.c | 141 ++++++++++++++++++++++++++++++++++++-------
 drivers/base/power/sysfs.c   |  22 ++++---
 drivers/usb/core/sysfs.c     |   3 +-
 include/linux/pm.h           |   8 ++-
 include/linux/pm_runtime.h   |  18 +++++-
 include/trace/events/rpm.h   |   6 +-
 7 files changed, 169 insertions(+), 37 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 9717d5f..e497c38 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -927,6 +927,9 @@ static void device_complete(struct device *dev, pm_message_t state)
 
 	device_unlock(dev);
 
+	if (dev->power.runtime_mode == RPM_MODE_OFF)
+		pm_runtime_force_suspend(dev);
+
 	pm_runtime_put(dev);
 }
 
@@ -1596,6 +1599,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	spin_lock_irq(&dev->power.lock);
 	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;
 	spin_unlock_irq(&dev->power.lock);
+
+	if (!dev->power.direct_complete &&
+	    dev->power.runtime_mode == RPM_MODE_OFF)
+		pm_runtime_enable(dev);
+
 	return 0;
 }
 
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 5070c4f..0d94b8c 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1189,49 +1189,144 @@ void pm_runtime_enable(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pm_runtime_enable);
 
+static int dev_pm_mode_on(struct device *dev, void *data)
+{
+	return dev->power.runtime_mode != RPM_MODE_OFF;
+}
+
 /**
- * pm_runtime_forbid - Block runtime PM of a device.
+ * pm_runtime_force_off - Block runtime PM of a device and keep it suspended.
  * @dev: Device to handle.
  *
- * Increase the device's usage count and clear its power.runtime_auto flag,
- * so that it cannot be suspended at run time until pm_runtime_allow() is called
- * for it.
+ * Change the device's power.runtime_mode field to RPM_MODE_OFF and
+ * force runtime suspend (by running the device's runtime suspend
+ * and disabling runtime power management), so that it cannot be
+ * resumed at run time until pm_runtime_force_on() or
+ * pm_runtime_force_auto() is called for it.
  */
-void pm_runtime_forbid(struct device *dev)
+int pm_runtime_force_off(struct device *dev)
 {
+	int mode, ret;
+
 	spin_lock_irq(&dev->power.lock);
-	if (!dev->power.runtime_auto)
-		goto out;
+	mode = dev->power.runtime_mode;
+	if (mode == RPM_MODE_OFF) {
+		spin_unlock_irq(&dev->power.lock);
+		return 0;
+	}
 
-	dev->power.runtime_auto = false;
-	atomic_inc(&dev->power.usage_count);
-	rpm_resume(dev, 0);
+	/* Cannot force suspend if runtime pm is disabled */
+	if (!pm_runtime_enabled(dev)) {
+		spin_unlock_irq(&dev->power.lock);
+		return -EACCES;
+	}
 
- out:
+	/*
+	 * Cannot force suspend if children are on/auto and
+	 * device ignore_children flag is not set.
+	 */
+	if (!dev->power.ignore_children &&
+	    device_for_each_child(dev, NULL, dev_pm_mode_on)) {
+		spin_unlock_irq(&dev->power.lock);
+		return -EBUSY;
+	}
+
+	dev->power.runtime_mode = RPM_MODE_OFF;
+	if (mode == RPM_MODE_ON)
+		atomic_dec(&dev->power.usage_count);
 	spin_unlock_irq(&dev->power.lock);
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret) {
+		spin_lock_irq(&dev->power.lock);
+		dev->power.runtime_mode = mode;
+		if (mode == RPM_MODE_ON)
+			atomic_inc(&dev->power.usage_count);
+		spin_unlock_irq(&dev->power.lock);
+		return ret;
+	}
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(pm_runtime_forbid);
+EXPORT_SYMBOL_GPL(pm_runtime_force_off);
 
 /**
- * pm_runtime_allow - Unblock runtime PM of a device.
+ * pm_runtime_force_on - Block runtime PM of a device and keep it resumed.
  * @dev: Device to handle.
  *
- * Decrease the device's usage count and set its power.runtime_auto flag.
+ * Increase the device's usage count and set its power.runtime_mode field,
+ * to RPM_MODE_ON, so that it cannot be suspended at run time until
+ * pm_runtime_force_auto() is called for it.
  */
-void pm_runtime_allow(struct device *dev)
+int pm_runtime_force_on(struct device *dev)
 {
+	int mode;
+
 	spin_lock_irq(&dev->power.lock);
-	if (dev->power.runtime_auto)
-		goto out;
+	mode = dev->power.runtime_mode;
+	if (mode == RPM_MODE_ON) {
+		spin_unlock_irq(&dev->power.lock);
+		return 0;
+	}
 
-	dev->power.runtime_auto = true;
-	if (atomic_dec_and_test(&dev->power.usage_count))
-		rpm_idle(dev, RPM_AUTO);
+	/* Cannot resume if parent is force suspended. */
+	if (dev->parent && dev->parent->power.runtime_mode == RPM_MODE_OFF) {
+		spin_unlock_irq(&dev->power.lock);
+		return -EBUSY;
+	}
 
- out:
+	dev->power.runtime_mode = RPM_MODE_ON;
+	spin_unlock_irq(&dev->power.lock);
+
+	if (mode == RPM_MODE_OFF)
+		pm_runtime_enable(dev);
+
+	__pm_runtime_resume(dev, RPM_GET_PUT);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_force_on);
+
+/**
+ * pm_runtime_force_auto - Unblock runtime PM of a device.
+ * @dev: Device to handle.
+ *
+ * Set the device's power.runtime_mode field to RPM_MODE_AUTO.
+ * If previous state was suspended, enable runtime power
+ * management and resume. If previous state was resumed,
+ * decrease the device's usage count.
+ */
+int pm_runtime_force_auto(struct device *dev)
+{
+	int mode, flags = 0;
+
+	spin_lock_irq(&dev->power.lock);
+	mode = dev->power.runtime_mode;
+	if (mode == RPM_MODE_AUTO) {
+		spin_unlock_irq(&dev->power.lock);
+		return 0;
+	}
+
+	/* Cannot resume if both device and its parent are force suspended */
+	if (mode == RPM_MODE_OFF && dev->parent &&
+	    dev->parent->power.runtime_mode == RPM_MODE_OFF) {
+		spin_unlock_irq(&dev->power.lock);
+		return -EBUSY;
+	}
+
+	dev->power.runtime_mode = RPM_MODE_AUTO;
 	spin_unlock_irq(&dev->power.lock);
+
+	if (mode == RPM_MODE_OFF) {
+		pm_runtime_enable(dev);
+		__pm_runtime_resume(dev, 0);
+	} else if (mode == RPM_MODE_ON) {
+		flags = RPM_GET_PUT;
+	}
+
+	__pm_runtime_idle(dev, RPM_AUTO | flags);
+	return 0;
 }
-EXPORT_SYMBOL_GPL(pm_runtime_allow);
+EXPORT_SYMBOL_GPL(pm_runtime_force_auto);
 
 /**
  * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
@@ -1368,7 +1463,7 @@ void pm_runtime_init(struct device *dev)
 
 	atomic_set(&dev->power.child_count, 0);
 	pm_suspend_ignore_children(dev, false);
-	dev->power.runtime_auto = true;
+	dev->power.runtime_mode = RPM_MODE_AUTO;
 
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d2be3f9..db0630a 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -97,31 +97,38 @@ EXPORT_SYMBOL_GPL(power_group_name);
 
 static const char ctrl_auto[] = "auto";
 static const char ctrl_on[] = "on";
+static const char ctrl_off[] = "off";
 
 static ssize_t control_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	return sprintf(buf, "%s\n",
-				dev->power.runtime_auto ? ctrl_auto : ctrl_on);
+		       dev->power.runtime_mode == RPM_MODE_AUTO ? ctrl_auto :
+		       dev->power.runtime_mode == RPM_MODE_ON ? ctrl_on :
+		       ctrl_off);
 }
 
 static ssize_t control_store(struct device * dev, struct device_attribute *attr,
 			     const char * buf, size_t n)
 {
 	char *cp;
-	int len = n;
+	int len = n, ret = 0;
 
 	cp = memchr(buf, '\n', n);
 	if (cp)
 		len = cp - buf;
 	device_lock(dev);
 	if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0)
-		pm_runtime_allow(dev);
+		ret = pm_runtime_force_auto(dev);
 	else if (len == sizeof ctrl_on - 1 && strncmp(buf, ctrl_on, len) == 0)
-		pm_runtime_forbid(dev);
+		ret = pm_runtime_force_on(dev);
+	else if (len == sizeof ctrl_off - 1 && strncmp(buf, ctrl_off, len) == 0)
+		ret = pm_runtime_force_off(dev);
 	else
-		n = -EINVAL;
+		ret = -EINVAL;
 	device_unlock(dev);
+	if (ret)
+		return ret;
 	return n;
 }
 
@@ -545,11 +552,12 @@ static ssize_t rtpm_children_show(struct device *dev,
 static ssize_t rtpm_enabled_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
-	if ((dev->power.disable_depth) && (dev->power.runtime_auto == false))
+	if ((dev->power.disable_depth) &&
+	    (dev->power.runtime_mode != RPM_MODE_AUTO))
 		return sprintf(buf, "disabled & forbidden\n");
 	else if (dev->power.disable_depth)
 		return sprintf(buf, "disabled\n");
-	else if (dev->power.runtime_auto == false)
+	else if (dev->power.runtime_mode != RPM_MODE_AUTO)
 		return sprintf(buf, "forbidden\n");
 	return sprintf(buf, "enabled\n");
 }
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index d269738..8165102 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -408,7 +408,8 @@ static ssize_t level_show(struct device *dev, struct device_attribute *attr,
 	const char *p = auto_string;
 
 	warn_level();
-	if (udev->state != USB_STATE_SUSPENDED && !udev->dev.power.runtime_auto)
+	if (udev->state != USB_STATE_SUSPENDED &&
+	    udev->dev.power.runtime_mode != RPM_MODE_AUTO)
 		p = on_string;
 	return sprintf(buf, "%s\n", p);
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e2f1be6..69273d4 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -542,6 +542,12 @@ struct pm_subsys_data {
 #endif
 };
 
+enum rpm_mode {
+	RPM_MODE_AUTO = 0,
+	RPM_MODE_ON,
+	RPM_MODE_OFF,
+};
+
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
@@ -575,12 +581,12 @@ struct dev_pm_info {
 	unsigned int		request_pending:1;
 	unsigned int		deferred_resume:1;
 	unsigned int		run_wake:1;
-	unsigned int		runtime_auto:1;
 	unsigned int		no_callbacks:1;
 	unsigned int		irq_safe:1;
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
+	enum rpm_mode		runtime_mode;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
 	int			runtime_error;
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 30e84d4..186e9f0 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -44,8 +44,9 @@ extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
-extern void pm_runtime_allow(struct device *dev);
-extern void pm_runtime_forbid(struct device *dev);
+extern int pm_runtime_force_off(struct device *dev);
+extern int pm_runtime_force_on(struct device *dev);
+extern int pm_runtime_force_auto(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
 extern void pm_runtime_irq_safe(struct device *dev);
 extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
@@ -55,6 +56,16 @@ extern void pm_runtime_update_max_time_suspended(struct device *dev,
 						 s64 delta_ns);
 extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
 
+static inline void pm_runtime_allow(struct device *dev)
+{
+	pm_runtime_force_auto(dev);
+}
+
+static inline void pm_runtime_forbid(struct device *dev)
+{
+	pm_runtime_force_on(dev);
+}
+
 static inline bool pm_children_suspended(struct device *dev)
 {
 	return dev->power.ignore_children
@@ -155,6 +166,9 @@ static inline void pm_runtime_enable(struct device *dev) {}
 static inline void __pm_runtime_disable(struct device *dev, bool c) {}
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
+static inline int pm_runtime_force_off(struct device *dev) { return 0; }
+static inline int pm_runtime_force_on(struct device *dev) { return 0; }
+static inline int pm_runtime_force_auto(struct device *dev) { return 0; }
 
 static inline bool pm_children_suspended(struct device *dev) { return false; }
 static inline void pm_runtime_get_noresume(struct device *dev) {}
diff --git a/include/trace/events/rpm.h b/include/trace/events/rpm.h
index 33f85b6..ab9cc18 100644
--- a/include/trace/events/rpm.h
+++ b/include/trace/events/rpm.h
@@ -25,7 +25,7 @@ DECLARE_EVENT_CLASS(rpm_internal,
 		__field(        int,            flags           )
 		__field(        int ,   	usage_count	)
 		__field(        int ,   	disable_depth   )
-		__field(        int ,   	runtime_auto	)
+		__field(        int ,   	runtime_mode	)
 		__field(        int ,   	request_pending	)
 		__field(        int ,   	irq_safe	)
 		__field(        int ,   	child_count 	)
@@ -37,7 +37,7 @@ DECLARE_EVENT_CLASS(rpm_internal,
 		__entry->usage_count = atomic_read(
 			&dev->power.usage_count);
 		__entry->disable_depth = dev->power.disable_depth;
-		__entry->runtime_auto = dev->power.runtime_auto;
+		__entry->runtime_mode = dev->power.runtime_mode;
 		__entry->request_pending = dev->power.request_pending;
 		__entry->irq_safe = dev->power.irq_safe;
 		__entry->child_count = atomic_read(
@@ -49,7 +49,7 @@ DECLARE_EVENT_CLASS(rpm_internal,
 			__get_str(name), __entry->flags,
 			__entry->usage_count,
 			__entry->disable_depth,
-			__entry->runtime_auto,
+			__entry->runtime_mode,
 			__entry->request_pending,
 			__entry->irq_safe,
 			__entry->child_count
-- 
1.9.1

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Monday, September 07, 2015 11:42:41 PM Irina Tirdea wrote:
> Add new option to sysfs control interface, allowing the user to force
> suspend the device.

Had we thought this had been a good idea, we'd have added that thing to
the interface from the start.

The problem with it is that user space generally doesn't know when it is
safe to suspend a device, so it cannot force anything into runtime suspend.

> This is useful for devices that need to be
> suspended when closing the lid of a laptop or the screen of a mobile
> device, while userspace still holds open handles to it and the
> system does not enter system suspend.
> 
> Add the "off" option to the sysfs control power interface, along with
> the already present "on" and "auto". When this attribute is set to
> "off", the device will be force suspended by calling its runtime
> suspend callback and disabling runtime power management so that
> further acceses to the device will not change the actual state.

And how is user space supposed to know that it doesn't break things
this way?

> The device can be resumed by setting the attribute to "on" or "auto".
> The behaviour of the interface when switching only between "on"
> and "auto" states remains unchanged.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
> 
> Hi,
> 
> This is a proposal for suspending devices when the closing the lid of
> a laptop or the screen of a mobile device.
> 
> I am testing this with a Goodix touchscreen [1] for an Android mobile
> device. Android has an userspace layer (power HAL) that would normally
> close the touchscreen when the screen is closed. Android touchscreen
> drivers usually provide a custom sysfs interface to allow this.
> This would be better implemented in a common place, to avoid code
> duplication and to simplify the driver code (as previosly discussed
> in [1]).
> 
> I know there are more ways to implement this, so I would appreciate
> your feedback.

So the feedback is that this is not going to work in general.  Please
use a different approach.

Thanks,
Rafael

RE: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Tirdea, Irina]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4632 bytes --]



> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: 08 September, 2015 0:20
> To: Tirdea, Irina
> Cc: Alan Stern; linux-pm@vger.kernel.org; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Brown, Len; Pavel Machek;
> Purdila, Octavian; Dmitry Torokhov
> Subject: Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend
> 
> On Monday, September 07, 2015 11:42:41 PM Irina Tirdea wrote:
> > Add new option to sysfs control interface, allowing the user to force
> > suspend the device.
> 
> Had we thought this had been a good idea, we'd have added that thing to
> the interface from the start.
> 
> The problem with it is that user space generally doesn't know when it is
> safe to suspend a device, so it cannot force anything into runtime suspend.
> 

Yes, this is generally true. 

However, in the scenario I mentioned this is exactly what is happening.
When turning off the screen of a mobile device, the user space
is the one that suspends the devices that are not needed in order to save power
(like touchscreens). Each such driver export an enable/disable attribute
that calls the same code as resume/suspend (e.g. touchscreen drivers ads7846,
ad7877 and most Android drivers not merged upstream). This adds more
complexity to every driver by adding one more logical power state.
It would be good to have a common interface instead of doing this in
every driver.

I might have not used "forced" in the proper way here. What I mean by it is that
the device can be runtime suspended while ignoring the runtime usage count.

> > This is useful for devices that need to be
> > suspended when closing the lid of a laptop or the screen of a mobile
> > device, while user space still holds open handles to it and the
> > system does not enter system suspend.
> >
> > Add the "off" option to the sysfs control power interface, along with
> > the already present "on" and "auto". When this attribute is set to
> > "off", the device will be force suspended by calling its runtime
> > suspend callback and disabling runtime power management so that
> > further accesses to the device will not change the actual state.
> 
> And how is user space supposed to know that it doesn't break things
> this way?
> 

In this implementation, user space is only allowed to change the states
bottom-up in the sysfs hierarchy (it cannot force suspend a device if it
has children that have not been suspended by user space).

Another way to do this is to keep runtime power management enabled
but to force the usage count to 0 while the device is set to runtime mode
"off".

> > The device can be resumed by setting the attribute to "on" or "auto".
> > The behavior of the interface when switching only between "on"
> > and "auto" states remains unchanged.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > ---
> >
> > Hi,
> >
> > This is a proposal for suspending devices when the closing the lid of
> > a laptop or the screen of a mobile device.
> >
> > I am testing this with a Goodix touchscreen [1] for an Android mobile
> > device. Android has an user space layer (power HAL) that would normally
> > close the touchscreen when the screen is closed. Android touchscreen
> > drivers usually provide a custom sysfs interface to allow this.
> > This would be better implemented in a common place, to avoid code
> > duplication and to simplify the driver code (as previously discussed
> > in [1]).
> >
> > I know there are more ways to implement this, so I would appreciate
> > your feedback.
> 
> So the feedback is that this is not going to work in general.  Please
> use a different approach.
> 

Would it work if this would be a capability that individual drivers need
to declare?

In the previous discussion thread , there were a couple of options
mentioned, but none seemed to reach a consensus. You mentioned
adding a "more aggressive runtime PM mode" [1]. I'm not sure how
this would work except for adding a sysfs attribute that would trigger
a runtime suspend while ignoring usage count. Would that be a
better direction?

Thank you,
Irina

[1] http://marc.info/?l=linux-input&m=140564626306396&w=2

> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Tue, 2015-09-08 at 01:10 +0000, Tirdea, Irina wrote:

> However, in the scenario I mentioned this is exactly what is happening.
> When turning off the screen of a mobile device, the user space

Would you explain why user space doesn't simply stop using those
devices, which in turn will make them idle? There are obvious cases
like keyboards or SCSI hosts where the kernel controls stuff, but could
you state where you expect this to be most useful?
Or why devices cannot be closed, e.g. you need to be sure settings
be not lost or is it something else?

> is the one that suspends the devices that are not needed in order to save power
> (like touchscreens). Each such driver export an enable/disable attribute
> that calls the same code as resume/suspend (e.g. touchscreen drivers ads7846,
> ad7877 and most Android drivers not merged upstream). This adds more
> complexity to every driver by adding one more logical power state.
> It would be good to have a common interface instead of doing this in
> every 

Now these are two distinct questions.

1. a common interface
2. a capability implemented in common code

It is important to keep that apart. I suppose if we want this at all
#1 is a given. #2 however may be impossible in a generic manner

> I might have not used "forced" in the proper way here. What I mean by it is that
> the device can be runtime suspended while ignoring the runtime usage count.

That is highly problematic. You'd need to audit the locking in every
driver. Right now elevating the count means that suspend()/resume()
cannot race with user space, as in the case of the system suspending
user space is frozen.

> In this implementation, user space is only allowed to change the states
> bottom-up in the sysfs hierarchy (it cannot force suspend a device if it
> has children that have not been suspended by user space).

That is obviously not enough. Take the worst case: we are flashing some
firmware. Or far more harmless: a key is has been pressed on a keyboard

> Would it work if this would be a capability that individual drivers need
> to declare?

For some drivers. But it needs support in the driver. Right now we can
make a device idle by calling close(). In fact we can benefit for
example in mice from this. But it needs support in the drivers.

> In the previous discussion thread , there were a couple of options
> mentioned, but none seemed to reach a consensus. You mentioned
> adding a "more aggressive runtime PM mode" [1]. I'm not sure how

That would have to be done on a per driver base.

> this would work except for adding a sysfs attribute that would trigger
> a runtime suspend while ignoring usage count. Would that be a
> better direction?

No. If we want this at all, we need a new callback to notify drivers
that user space is temporarily uninterested in a device. And the reverse
of course.
The power model is good. We must not assume that devices can be
suspended at will. If we do this at all, we ought to see it as giving
strong hints to drivers when a device can be considered idle.

	Regards
		Oliver

RE: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Tue, 8 Sep 2015, Tirdea, Irina wrote:

> In the previous discussion thread , there were a couple of options
> mentioned, but none seemed to reach a consensus. You mentioned
> adding a "more aggressive runtime PM mode" [1]. I'm not sure how
> this would work except for adding a sysfs attribute that would trigger
> a runtime suspend while ignoring usage count. Would that be a
> better direction?
> 
> Thank you,
> Irina
> 
> [1] http://marc.info/?l=linux-input&m=140564626306396&w=2

Purely as a matter of interest, in that email Rafael also mentioned
that he and I had discussed a way to disable remote wakeup during 
runtime suspend.  Oddly enough, the method we decided upon was to add 
an "off" option to /sys/.../power/control.  :-)

It would not put the device into runtime suspend immediately, like you
are proposing.  Instead it would mean the same as the "auto" mode,
except that remote wakeup should be disabled during runtime suspend.

We never got around to implementing this, however.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Tue, 8 Sep 2015, Rafael J. Wysocki wrote:

> > > [1] http://marc.info/?l=linux-input&m=140564626306396&w=2
> > 
> > Purely as a matter of interest, in that email Rafael also mentioned
> > that he and I had discussed a way to disable remote wakeup during 
> > runtime suspend.  Oddly enough, the method we decided upon was to add 
> > an "off" option to /sys/.../power/control.  :-)
> 
> Wasn't that /sys/devices/.../power/wakeup rather?

Not the way I remember.  Of course, it's possible that we misunderstood 
each other at the time.

> > It would not put the device into runtime suspend immediately, like you
> > are proposing.  Instead it would mean the same as the "auto" mode,
> > except that remote wakeup should be disabled during runtime suspend.
> > 
> > We never got around to implementing this, however.
> 
> I don't think this is what we discussed then really.
> 
> There is a fundamental problem with forcing things into runtime suspend
> from user space, because that may happen in a wrong time.  In other words,
> the kernel can't guarantee that the device would actually be able to go
> into runtime suspend when requested.

Exactly.  What we discussed at LinuxCon wasn't forcing things into
runtime suspend; it was disabling remote wakeup during runtime suspend.

And even though the topic was quite different from Irina's proposal, we 
ended up settling on the same API (according to my recollection).

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Tuesday, September 08, 2015 10:44:04 AM Alan Stern wrote:
> On Tue, 8 Sep 2015, Tirdea, Irina wrote:
> 
> > In the previous discussion thread , there were a couple of options
> > mentioned, but none seemed to reach a consensus. You mentioned
> > adding a "more aggressive runtime PM mode" [1]. I'm not sure how
> > this would work except for adding a sysfs attribute that would trigger
> > a runtime suspend while ignoring usage count. Would that be a
> > better direction?
> > 
> > Thank you,
> > Irina
> > 
> > [1] http://marc.info/?l=linux-input&m=140564626306396&w=2
> 
> Purely as a matter of interest, in that email Rafael also mentioned
> that he and I had discussed a way to disable remote wakeup during 
> runtime suspend.  Oddly enough, the method we decided upon was to add 
> an "off" option to /sys/.../power/control.  :-)

Wasn't that /sys/devices/.../power/wakeup rather?

> It would not put the device into runtime suspend immediately, like you
> are proposing.  Instead it would mean the same as the "auto" mode,
> except that remote wakeup should be disabled during runtime suspend.
> 
> We never got around to implementing this, however.

I don't think this is what we discussed then really.

There is a fundamental problem with forcing things into runtime suspend
from user space, because that may happen in a wrong time.  In other words,
the kernel can't guarantee that the device would actually be able to go
into runtime suspend when requested.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

Hi,

On Tue, Sep 8, 2015 at 5:00 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 8 Sep 2015, Rafael J. Wysocki wrote:
>
>> > > [1] http://marc.info/?l=linux-input&m=140564626306396&w=2
>> >
>> > Purely as a matter of interest, in that email Rafael also mentioned
>> > that he and I had discussed a way to disable remote wakeup during
>> > runtime suspend.  Oddly enough, the method we decided upon was to add
>> > an "off" option to /sys/.../power/control.  :-)
>>
>> Wasn't that /sys/devices/.../power/wakeup rather?
>
> Not the way I remember.  Of course, it's possible that we misunderstood
> each other at the time.
>
>> > It would not put the device into runtime suspend immediately, like you
>> > are proposing.  Instead it would mean the same as the "auto" mode,
>> > except that remote wakeup should be disabled during runtime suspend.
>> >
>> > We never got around to implementing this, however.
>>
>> I don't think this is what we discussed then really.
>>
>> There is a fundamental problem with forcing things into runtime suspend
>> from user space, because that may happen in a wrong time.  In other words,
>> the kernel can't guarantee that the device would actually be able to go
>> into runtime suspend when requested.
>
> Exactly.  What we discussed at LinuxCon wasn't forcing things into
> runtime suspend; it was disabling remote wakeup during runtime suspend.
>
> And even though the topic was quite different from Irina's proposal, we
> ended up settling on the same API (according to my recollection).

So I remember that differently.

My idea was to add a third value to /sys/devices/.../power/wakeup (in
addition to "disabled" and "enabled") so user space can indicate that
remote wakeup should not be enabled for runtime suspend for the device
(since there's no way to indicate that today).  I don't see how
/sys/devices/.../power/control might help here to be honest.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Tue, Sep 8, 2015 at 9:35 AM, Oliver Neukum <oneukum@suse.com> wrote:
> On Tue, 2015-09-08 at 01:10 +0000, Tirdea, Irina wrote:

[cut]

>> this would work except for adding a sysfs attribute that would trigger
>> a runtime suspend while ignoring usage count. Would that be a
>> better direction?
>
> No. If we want this at all, we need a new callback to notify drivers
> that user space is temporarily uninterested in a device. And the reverse
> of course.
> The power model is good. We must not assume that devices can be
> suspended at will. If we do this at all, we ought to see it as giving
> strong hints to drivers when a device can be considered idle.

This is a good summary in my view.

The only thing we can add, realistically, is an interface for user
space to "kick" drivers to check if the devices they handle may be
suspended at this point (or to run their ->runtime_idle callbacks
IOW).

That would be quite similar to autosuspend except that the "kick" will
come from user space rather than from a timer function in the kernel.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Ulf Hansson]

On 8 September 2015 at 22:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Sep 8, 2015 at 9:35 AM, Oliver Neukum <oneukum@suse.com> wrote:
>> On Tue, 2015-09-08 at 01:10 +0000, Tirdea, Irina wrote:
>
> [cut]
>
>>> this would work except for adding a sysfs attribute that would trigger
>>> a runtime suspend while ignoring usage count. Would that be a
>>> better direction?
>>
>> No. If we want this at all, we need a new callback to notify drivers
>> that user space is temporarily uninterested in a device. And the reverse
>> of course.
>> The power model is good. We must not assume that devices can be
>> suspended at will. If we do this at all, we ought to see it as giving
>> strong hints to drivers when a device can be considered idle.
>
> This is a good summary in my view.
>
> The only thing we can add, realistically, is an interface for user
> space to "kick" drivers to check if the devices they handle may be
> suspended at this point (or to run their ->runtime_idle callbacks
> IOW).
>
> That would be quite similar to autosuspend except that the "kick" will
> come from user space rather than from a timer function in the kernel.

Apologize for interrupting the discussion!

Unless I miss the point, I assumes the above is somewhat already
achievable via sysfs when changing the value of the auto-suspend
timeout, since it triggers a call to
pm_runtime_set_autosuspend_delay()...

Also, according to the discussion so far, it seems like we are on
agreement that we should really think twice when considering to extend
the sysfs interface for runtime PM.

>From the change-log/description to $subject patch, I fail to
understand *why* the regular runtime PM *autosuspend* feature isn't
sufficient. Perhaps Irina can elaborate more on the use case, to help
me get a better understanding of the issue!?

Kind regards
Uffe

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

Hi,

On Wed, Sep 9, 2015 at 12:25 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 8 September 2015 at 22:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Sep 8, 2015 at 9:35 AM, Oliver Neukum <oneukum@suse.com> wrote:
>>> On Tue, 2015-09-08 at 01:10 +0000, Tirdea, Irina wrote:
>>
>> [cut]
>>
>>>> this would work except for adding a sysfs attribute that would trigger
>>>> a runtime suspend while ignoring usage count. Would that be a
>>>> better direction?
>>>
>>> No. If we want this at all, we need a new callback to notify drivers
>>> that user space is temporarily uninterested in a device. And the reverse
>>> of course.
>>> The power model is good. We must not assume that devices can be
>>> suspended at will. If we do this at all, we ought to see it as giving
>>> strong hints to drivers when a device can be considered idle.
>>
>> This is a good summary in my view.
>>
>> The only thing we can add, realistically, is an interface for user
>> space to "kick" drivers to check if the devices they handle may be
>> suspended at this point (or to run their ->runtime_idle callbacks
>> IOW).
>>
>> That would be quite similar to autosuspend except that the "kick" will
>> come from user space rather than from a timer function in the kernel.
>
> Apologize for interrupting the discussion!
>
> Unless I miss the point, I assumes the above is somewhat already
> achievable via sysfs when changing the value of the auto-suspend
> timeout, since it triggers a call to
> pm_runtime_set_autosuspend_delay()...

Well, from the initial comment in drivers/base/power/sysfs.c:

 *
 *    NOTE: The autosuspend_delay_ms attribute and the autosuspend_delay
 *    value are used only if the driver calls pm_runtime_use_autosuspend().
 *

Some drivers don't do that and they would be the primary target
audience for the new interface (if we agreed that it was useful after
all).

> Also, according to the discussion so far, it seems like we are on
> agreement that we should really think twice when considering to extend
> the sysfs interface for runtime PM.

That certainly is correct and not limited to runtime PM. :-)

> From the change-log/description to $subject patch, I fail to
> understand *why* the regular runtime PM *autosuspend* feature isn't
> sufficient. Perhaps Irina can elaborate more on the use case, to help
> me get a better understanding of the issue!?

My understanding is that the idea would be to trigger an attempt to
suspend via a specific event (eg. lid closes) rather then via an
inactivity timer.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Tue, 2015-09-08 at 10:44 -0400, Alan Stern wrote:
> It would not put the device into runtime suspend immediately, like you
> are proposing.  Instead it would mean the same as the "auto" mode,
> except that remote wakeup should be disabled during runtime suspend.

Hi,

this proposal is incomplete. If you don't want remote wakeup you
imply that input is no longer needed or possible. If that is
already known, we can just as well inform the driver, so that
it can cease IO for input.

Yet that is not necessarily the only scenario. For example
if you run a screensaver, you might not care for where the
user touches the screen, but the event as such is valuable.

	Regards
		Oliver

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Octavian Purdila]

On Wed, Sep 9, 2015 at 2:50 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Hi,
>
> On Wed, Sep 9, 2015 at 12:25 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 8 September 2015 at 22:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> On Tue, Sep 8, 2015 at 9:35 AM, Oliver Neukum <oneukum@suse.com> wrote:
> >>> On Tue, 2015-09-08 at 01:10 +0000, Tirdea, Irina wrote:
> >>
> >> [cut]
> >>
> >>>> this would work except for adding a sysfs attribute that would trigger
> >>>> a runtime suspend while ignoring usage count. Would that be a
> >>>> better direction?
> >>>
> >>> No. If we want this at all, we need a new callback to notify drivers
> >>> that user space is temporarily uninterested in a device. And the reverse
> >>> of course.
> >>> The power model is good. We must not assume that devices can be
> >>> suspended at will. If we do this at all, we ought to see it as giving
> >>> strong hints to drivers when a device can be considered idle.
> >>
> >> This is a good summary in my view.
> >>
> >> The only thing we can add, realistically, is an interface for user
> >> space to "kick" drivers to check if the devices they handle may be
> >> suspended at this point (or to run their ->runtime_idle callbacks
> >> IOW).
> >>
> >> That would be quite similar to autosuspend except that the "kick" will
> >> come from user space rather than from a timer function in the kernel.
> >
> > Apologize for interrupting the discussion!
> >
> > Unless I miss the point, I assumes the above is somewhat already
> > achievable via sysfs when changing the value of the auto-suspend
> > timeout, since it triggers a call to
> > pm_runtime_set_autosuspend_delay()...
>
> Well, from the initial comment in drivers/base/power/sysfs.c:
>
>  *
>  *    NOTE: The autosuspend_delay_ms attribute and the autosuspend_delay
>  *    value are used only if the driver calls pm_runtime_use_autosuspend().
>  *
>
> Some drivers don't do that and they would be the primary target
> audience for the new interface (if we agreed that it was useful after
> all).
>
> > Also, according to the discussion so far, it seems like we are on
> > agreement that we should really think twice when considering to extend
> > the sysfs interface for runtime PM.
>
> That certainly is correct and not limited to runtime PM. :-)
>
> > From the change-log/description to $subject patch, I fail to
> > understand *why* the regular runtime PM *autosuspend* feature isn't
> > sufficient. Perhaps Irina can elaborate more on the use case, to help
> > me get a better understanding of the issue!?
>
> My understanding is that the idea would be to trigger an attempt to
> suspend via a specific event (eg. lid closes) rather then via an
> inactivity timer.
>

The best example and actually the very specific problem we want to
solve is handling touchscreens on a phone / tablet. When the screen is
turned off, it is ideal to suspend the touchscreen for two reasons: to
lower the power consumption as much as possible and to prevent
interrupts to wake-up the CPU when the user touches the device, and
thus save even more power as we allow the CPU to stay in deep idle
states for longer periods.

Note that when the screen is turned-on again, we want to resume the
touchscreen so that it can send events again.

This is different then the lid closes examples, as in that case the
user can not generate new events and thus the usual autosuspend
feature is probably good enough (if the suspend power and autosuspend
power consumption is similar).

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

Hi,

On Wed, Sep 9, 2015 at 1:13 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Sep 9, 2015 at 2:50 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> Hi,
>>
>> On Wed, Sep 9, 2015 at 12:25 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> > On 8 September 2015 at 22:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >> On Tue, Sep 8, 2015 at 9:35 AM, Oliver Neukum <oneukum@suse.com> wrote:
>> >>> On Tue, 2015-09-08 at 01:10 +0000, Tirdea, Irina wrote:
>> >>
>> >> [cut]
>> >>
>> >>>> this would work except for adding a sysfs attribute that would trigger
>> >>>> a runtime suspend while ignoring usage count. Would that be a
>> >>>> better direction?
>> >>>
>> >>> No. If we want this at all, we need a new callback to notify drivers
>> >>> that user space is temporarily uninterested in a device. And the reverse
>> >>> of course.
>> >>> The power model is good. We must not assume that devices can be
>> >>> suspended at will. If we do this at all, we ought to see it as giving
>> >>> strong hints to drivers when a device can be considered idle.
>> >>
>> >> This is a good summary in my view.
>> >>
>> >> The only thing we can add, realistically, is an interface for user
>> >> space to "kick" drivers to check if the devices they handle may be
>> >> suspended at this point (or to run their ->runtime_idle callbacks
>> >> IOW).
>> >>
>> >> That would be quite similar to autosuspend except that the "kick" will
>> >> come from user space rather than from a timer function in the kernel.
>> >
>> > Apologize for interrupting the discussion!
>> >
>> > Unless I miss the point, I assumes the above is somewhat already
>> > achievable via sysfs when changing the value of the auto-suspend
>> > timeout, since it triggers a call to
>> > pm_runtime_set_autosuspend_delay()...
>>
>> Well, from the initial comment in drivers/base/power/sysfs.c:
>>
>>  *
>>  *    NOTE: The autosuspend_delay_ms attribute and the autosuspend_delay
>>  *    value are used only if the driver calls pm_runtime_use_autosuspend().
>>  *
>>
>> Some drivers don't do that and they would be the primary target
>> audience for the new interface (if we agreed that it was useful after
>> all).
>>
>> > Also, according to the discussion so far, it seems like we are on
>> > agreement that we should really think twice when considering to extend
>> > the sysfs interface for runtime PM.
>>
>> That certainly is correct and not limited to runtime PM. :-)
>>
>> > From the change-log/description to $subject patch, I fail to
>> > understand *why* the regular runtime PM *autosuspend* feature isn't
>> > sufficient. Perhaps Irina can elaborate more on the use case, to help
>> > me get a better understanding of the issue!?
>>
>> My understanding is that the idea would be to trigger an attempt to
>> suspend via a specific event (eg. lid closes) rather then via an
>> inactivity timer.
>>
>
> The best example and actually the very specific problem we want to
> solve is handling touchscreens on a phone / tablet. When the screen is
> turned off, it is ideal to suspend the touchscreen for two reasons: to
> lower the power consumption as much as possible and to prevent
> interrupts to wake-up the CPU when the user touches the device, and
> thus save even more power as we allow the CPU to stay in deep idle
> states for longer periods.
>
> Note that when the screen is turned-on again, we want to resume the
> touchscreen so that it can send events again.

In fact, then, what you need seems to be the feature discussed by Alan
and me some time ago allowing remote wakeup do be disabled for runtime
PM from user space as that in combination with autosuspend should
address your use case.

> This is different then the lid closes examples, as in that case the
> user can not generate new events and thus the usual autosuspend
> feature is probably good enough (if the suspend power and autosuspend
> power consumption is similar).

Autosuspend just means checking the device state periodically and
suspending it if idle (not in use).  It doesn't affect the energy
consumption in the suspended state.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Wed, 2015-09-09 at 14:22 +0200, Rafael J. Wysocki wrote:
> > Note that when the screen is turned-on again, we want to resume the
> > touchscreen so that it can send events again.

Why is it impractical to close the fd for the touchscreen?
> 
> In fact, then, what you need seems to be the feature discussed by Alan
> and me some time ago allowing remote wakeup do be disabled for runtime
> PM from user space as that in combination with autosuspend should
> address your use case.

I'd doubt that. Suppose you put the phone into your pocket while
the device isn't suspended. The continuous stream of spurious events
will keep it awake.
The ability to disable remote wakeup is necessary but not sufficient.

	Regards
		Oliver

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Wed, 9 Sep 2015, Oliver Neukum wrote:

> On Tue, 2015-09-08 at 10:44 -0400, Alan Stern wrote:
> > It would not put the device into runtime suspend immediately, like you
> > are proposing.  Instead it would mean the same as the "auto" mode,
> > except that remote wakeup should be disabled during runtime suspend.
> 
> Hi,
> 
> this proposal is incomplete. If you don't want remote wakeup you
> imply that input is no longer needed or possible. If that is
> already known, we can just as well inform the driver, so that
> it can cease IO for input.

Like I said, it was never implemented.  For that reason, it was never 
completely fleshed out.

> Yet that is not necessarily the only scenario. For example
> if you run a screensaver, you might not care for where the
> user touches the screen, but the event as such is valuable.

I suspect it's not worth the effort to distinguish between getting an 
event with all the details and merely knowing that an event occurred.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Octavian Purdila]

On Wed, Sep 9, 2015 at 4:55 PM, Oliver Neukum <oneukum@suse.com> wrote:
> On Wed, 2015-09-09 at 14:22 +0200, Rafael J. Wysocki wrote:
>> > Note that when the screen is turned-on again, we want to resume the
>> > touchscreen so that it can send events again.
>
> Why is it impractical to close the fd for the touchscreen?
>

I am not sure, but I think it is this way due to historical reasons.
On Android devices early-suspend was used originally to save power
when the screen was turned-off but the device was not suspend. Later
Android moved to power HAL [1] and run-time PM for some devices,
however that is not sufficient for touchscreen.

i2c touchscreen devices usually have two low-power states: a deep
power state where event collection is disabled and the device needs to
be poked via i2c to restart collecting events and a shallow power
state where the device reduces the internal polling rate after it is
idle for some time. The latter it is usually implemented directly in
hardware. That means that you can't really implemented auto-suspend
with the deep power state, since the device can not resume itself.

To address this limitation, Android used early suspend (and then the
power HAL mechanism) where the upper layers signals when you can turn
on or off certain devices.

I have added Colin and Arve to this thread who maybe can answer this better.

>> In fact, then, what you need seems to be the feature discussed by Alan
>> and me some time ago allowing remote wakeup do be disabled for runtime
>> PM from user space as that in combination with autosuspend should
>> address your use case.
>
> I'd doubt that. Suppose you put the phone into your pocket while
> the device isn't suspended. The continuous stream of spurious events
> will keep it awake.

I agree.

> The ability to disable remote wakeup is necessary but not sufficient.
>

I don't know enough about remote wake-up, but do we even need to use
it for this kind of devices?

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Wed, 9 Sep 2015, Rafael J. Wysocki wrote:

> > The best example and actually the very specific problem we want to
> > solve is handling touchscreens on a phone / tablet. When the screen is
> > turned off, it is ideal to suspend the touchscreen for two reasons: to
> > lower the power consumption as much as possible and to prevent
> > interrupts to wake-up the CPU when the user touches the device, and
> > thus save even more power as we allow the CPU to stay in deep idle
> > states for longer periods.
> >
> > Note that when the screen is turned-on again, we want to resume the
> > touchscreen so that it can send events again.
> 
> In fact, then, what you need seems to be the feature discussed by Alan
> and me some time ago allowing remote wakeup do be disabled for runtime
> PM from user space as that in combination with autosuspend should
> address your use case.

That, plus they want the touchscreen to go into runtime suspend 
whenever the screen is off (was this not the main reason for the 
patch?).

It seems to me that it should be possible to arrange for this to happen 
simply by making userspace close the touchscreen device when the screen 
is turned off.  Or am I missing something?

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Tue, 8 Sep 2015, Rafael J. Wysocki wrote:

> Hi,
> 
> On Tue, Sep 8, 2015 at 5:00 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 8 Sep 2015, Rafael J. Wysocki wrote:
> >
> >> > > [1] http://marc.info/?l=linux-input&m=140564626306396&w=2
> >> >
> >> > Purely as a matter of interest, in that email Rafael also mentioned
> >> > that he and I had discussed a way to disable remote wakeup during
> >> > runtime suspend.  Oddly enough, the method we decided upon was to add
> >> > an "off" option to /sys/.../power/control.  :-)
> >>
> >> Wasn't that /sys/devices/.../power/wakeup rather?
> >
> > Not the way I remember.  Of course, it's possible that we misunderstood
> > each other at the time.
> >
> >> > It would not put the device into runtime suspend immediately, like you
> >> > are proposing.  Instead it would mean the same as the "auto" mode,
> >> > except that remote wakeup should be disabled during runtime suspend.
> >> >
> >> > We never got around to implementing this, however.
> >>
> >> I don't think this is what we discussed then really.
> >>
> >> There is a fundamental problem with forcing things into runtime suspend
> >> from user space, because that may happen in a wrong time.  In other words,
> >> the kernel can't guarantee that the device would actually be able to go
> >> into runtime suspend when requested.
> >
> > Exactly.  What we discussed at LinuxCon wasn't forcing things into
> > runtime suspend; it was disabling remote wakeup during runtime suspend.
> >
> > And even though the topic was quite different from Irina's proposal, we
> > ended up settling on the same API (according to my recollection).
> 
> So I remember that differently.
> 
> My idea was to add a third value to /sys/devices/.../power/wakeup (in
> addition to "disabled" and "enabled") so user space can indicate that
> remote wakeup should not be enabled for runtime suspend for the device
> (since there's no way to indicate that today).  I don't see how
> /sys/devices/.../power/control might help here to be honest.

You're right, that does make more sense than what I was thinking.  My 
memory must have gotten messed up.  RAM corruption, no doubt...  I 
think I need an EDAC brain.  :-)

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Colin Cross]

On Wed, Sep 9, 2015 at 1:35 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, September 09, 2015 11:20:25 AM Alan Stern wrote:
> > On Wed, 9 Sep 2015, Rafael J. Wysocki wrote:
> >
> > > > The best example and actually the very specific problem we want to
> > > > solve is handling touchscreens on a phone / tablet. When the screen is
> > > > turned off, it is ideal to suspend the touchscreen for two reasons: to
> > > > lower the power consumption as much as possible and to prevent
> > > > interrupts to wake-up the CPU when the user touches the device, and
> > > > thus save even more power as we allow the CPU to stay in deep idle
> > > > states for longer periods.
> > > >
> > > > Note that when the screen is turned-on again, we want to resume the
> > > > touchscreen so that it can send events again.
> > >
> > > In fact, then, what you need seems to be the feature discussed by Alan
> > > and me some time ago allowing remote wakeup do be disabled for runtime
> > > PM from user space as that in combination with autosuspend should
> > > address your use case.
> >
> > That, plus they want the touchscreen to go into runtime suspend
> > whenever the screen is off (was this not the main reason for the
> > patch?).
>
> Right.
>
> > It seems to me that it should be possible to arrange for this to happen
> > simply by making userspace close the touchscreen device when the screen
> > is turned off.  Or am I missing something?
>
> Honestly, I don't know.
>
> Octavian, Irina, any reasons why things can't be done as Alan is suggesting?


Early Android used early suspend, which notified various kernel
drivers that the screen was turning off so they could go into low
power states.  That meant there was no reason to close the touchscreen
fd - the kernel already knew about the screen off event.  We then got
rid of the early suspend hack and moved everything into userspace
using the power HAL and sysfs files.  Getting rid of the touchscreen
sysfs files and closing the touchscreen on screen off was on the
nice-to-have list, but hasn't made it onto anybody's todo list.

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Wednesday, September 09, 2015 06:02:02 PM Octavian Purdila wrote:
> On Wed, Sep 9, 2015 at 4:55 PM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Wed, 2015-09-09 at 14:22 +0200, Rafael J. Wysocki wrote:
> >> > Note that when the screen is turned-on again, we want to resume the
> >> > touchscreen so that it can send events again.
> >
> > Why is it impractical to close the fd for the touchscreen?
> >
> 
> I am not sure, but I think it is this way due to historical reasons.
> On Android devices early-suspend was used originally to save power
> when the screen was turned-off but the device was not suspend. Later
> Android moved to power HAL [1] and run-time PM for some devices,
> however that is not sufficient for touchscreen.
> 
> i2c touchscreen devices usually have two low-power states: a deep
> power state where event collection is disabled and the device needs to
> be poked via i2c to restart collecting events and a shallow power
> state where the device reduces the internal polling rate after it is
> idle for some time. The latter it is usually implemented directly in
> hardware. That means that you can't really implemented auto-suspend
> with the deep power state, since the device can not resume itself.
> 
> To address this limitation, Android used early suspend (and then the
> power HAL mechanism) where the upper layers signals when you can turn
> on or off certain devices.
> 
> I have added Colin and Arve to this thread who maybe can answer this better.
> 
> >> In fact, then, what you need seems to be the feature discussed by Alan
> >> and me some time ago allowing remote wakeup do be disabled for runtime
> >> PM from user space as that in combination with autosuspend should
> >> address your use case.
> >
> > I'd doubt that. Suppose you put the phone into your pocket while
> > the device isn't suspended. The continuous stream of spurious events
> > will keep it awake.

Why would they be regarded as spurious then?  They are just regular touch panel
events in that case, aren't they?

> I agree.
> 
> > The ability to disable remote wakeup is necessary but not sufficient.
> >
> 
> I don't know enough about remote wake-up, but do we even need to use
> it for this kind of devices?

Well, if the device is capable of generating wakeup events while suspended,
the current expected behavior is to do that, but of course that needs to
be enabled by the driver/bus type too.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Wednesday, September 09, 2015 11:20:25 AM Alan Stern wrote:
> On Wed, 9 Sep 2015, Rafael J. Wysocki wrote:
> 
> > > The best example and actually the very specific problem we want to
> > > solve is handling touchscreens on a phone / tablet. When the screen is
> > > turned off, it is ideal to suspend the touchscreen for two reasons: to
> > > lower the power consumption as much as possible and to prevent
> > > interrupts to wake-up the CPU when the user touches the device, and
> > > thus save even more power as we allow the CPU to stay in deep idle
> > > states for longer periods.
> > >
> > > Note that when the screen is turned-on again, we want to resume the
> > > touchscreen so that it can send events again.
> > 
> > In fact, then, what you need seems to be the feature discussed by Alan
> > and me some time ago allowing remote wakeup do be disabled for runtime
> > PM from user space as that in combination with autosuspend should
> > address your use case.
> 
> That, plus they want the touchscreen to go into runtime suspend 
> whenever the screen is off (was this not the main reason for the 
> patch?).

Right.

> It seems to me that it should be possible to arrange for this to happen 
> simply by making userspace close the touchscreen device when the screen 
> is turned off.  Or am I missing something?

Honestly, I don't know.

Octavian, Irina, any reasons why things can't be done as Alan is suggesting?

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Wed, 2015-09-09 at 22:25 +0200, Rafael J. Wysocki wrote:
> > >
> > > I'd doubt that. Suppose you put the phone into your pocket while
> > > the device isn't suspended. The continuous stream of spurious
> events
> > > will keep it awake.
> 
> Why would they be regarded as spurious then?  They are just regular
> touch panel
> events in that case, aren't they?

These events are not expected to be caused by the user's hand.

But it raises a design question; whose job it is to handle
such information?
It makes no sense to gather events from a touchscreen if you
suspect the phone is randomly rubbing at things or to take
video from a camera if you know that the lid is closed covering
the lens. I think we can agree to that.

The thing is that we handle all other availability in kernel
space. You can argue that user space has an agreed interface
(evdev, V4L or whatever) and it is the kernel's job to react
if it learns that a device becomes temporarily unavailable
and this is merely a question of adding an interface to the
kernel by which user space can feed such information to the
kernel.

	Regards
		Oliver

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Pavel Machek]

> > >> In fact, then, what you need seems to be the feature discussed by Alan
> > >> and me some time ago allowing remote wakeup do be disabled for runtime
> > >> PM from user space as that in combination with autosuspend should
> > >> address your use case.
> > >
> > > I'd doubt that. Suppose you put the phone into your pocket while
> > > the device isn't suspended. The continuous stream of spurious events
> > > will keep it awake.
> 
> Why would they be regarded as spurious then?  They are just regular touch panel
> events in that case, aren't they?

>From userspace... they are spurious. Userspace does not known that
your device is commonly placed into the pocket, and the events it sees
are not from user.

I have mainline X/mate running on n900 cellphone. And this means
battery life is in "2 hours" range.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Pavel Machek]

On Wed 2015-09-09 11:20:25, Alan Stern wrote:
> On Wed, 9 Sep 2015, Rafael J. Wysocki wrote:
> 
> > > The best example and actually the very specific problem we want to
> > > solve is handling touchscreens on a phone / tablet. When the screen is
> > > turned off, it is ideal to suspend the touchscreen for two reasons: to
> > > lower the power consumption as much as possible and to prevent
> > > interrupts to wake-up the CPU when the user touches the device, and
> > > thus save even more power as we allow the CPU to stay in deep idle
> > > states for longer periods.
> > >
> > > Note that when the screen is turned-on again, we want to resume the
> > > touchscreen so that it can send events again.
> > 
> > In fact, then, what you need seems to be the feature discussed by Alan
> > and me some time ago allowing remote wakeup do be disabled for runtime
> > PM from user space as that in combination with autosuspend should
> > address your use case.
> 
> That, plus they want the touchscreen to go into runtime suspend 
> whenever the screen is off (was this not the main reason for the 
> patch?).
> 
> It seems to me that it should be possible to arrange for this to happen 
> simply by making userspace close the touchscreen device when the screen 
> is turned off.  Or am I missing something?

Well... that's not what existing userspace expects. Your X windows
server will not close the touchscreen.

..and it would be nice to have enough hardware abstraction in the
kernel so that X can be used on phones...
									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Mon, 21 Sep 2015, Pavel Machek wrote:

> > > In fact, then, what you need seems to be the feature discussed by Alan
> > > and me some time ago allowing remote wakeup do be disabled for runtime
> > > PM from user space as that in combination with autosuspend should
> > > address your use case.
> > 
> > That, plus they want the touchscreen to go into runtime suspend 
> > whenever the screen is off (was this not the main reason for the 
> > patch?).
> > 
> > It seems to me that it should be possible to arrange for this to happen 
> > simply by making userspace close the touchscreen device when the screen 
> > is turned off.  Or am I missing something?
> 
> Well... that's not what existing userspace expects. Your X windows
> server will not close the touchscreen.

Surely that's a userspace issue, rather than a kernel problem?  The X
server does have some notion of power management and power savings; why
not extend that notion to include touchscreens?

> ..and it would be nice to have enough hardware abstraction in the
> kernel so that X can be used on phones...

What -- not Wayland?!  :-)

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Dmitry Torokhov]

On Mon, Sep 21, 2015 at 10:38:46AM -0400, Alan Stern wrote:
> On Mon, 21 Sep 2015, Pavel Machek wrote:
> 
> > > > In fact, then, what you need seems to be the feature discussed by Alan
> > > > and me some time ago allowing remote wakeup do be disabled for runtime
> > > > PM from user space as that in combination with autosuspend should
> > > > address your use case.
> > > 
> > > That, plus they want the touchscreen to go into runtime suspend 
> > > whenever the screen is off (was this not the main reason for the 
> > > patch?).
> > > 
> > > It seems to me that it should be possible to arrange for this to happen 
> > > simply by making userspace close the touchscreen device when the screen 
> > > is turned off.  Or am I missing something?
> > 
> > Well... that's not what existing userspace expects. Your X windows
> > server will not close the touchscreen.
> 
> Surely that's a userspace issue, rather than a kernel problem?  The X
> server does have some notion of power management and power savings; why
> not extend that notion to include touchscreens?

It is not really practical: there are many consumers of input events, if
we build infrastructure to control it and proxy all users through it,
why not have it in kernel? Plus, there are users of input events
directly in the kernel, such as legacy VT/keyboard, or Android/ChromeOS
cpufreq_interactive governor that monitors user activity and bumps up
CPU speed when user actively interacts with the device. They would keep
input devices active even though user might not be actually able to
use some input devices.

Thanks.

-- 
Dmitry

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Mon, 21 Sep 2015, Dmitry Torokhov wrote:

> On Mon, Sep 21, 2015 at 10:38:46AM -0400, Alan Stern wrote:
> > On Mon, 21 Sep 2015, Pavel Machek wrote:
> > 
> > > > > In fact, then, what you need seems to be the feature discussed by Alan
> > > > > and me some time ago allowing remote wakeup do be disabled for runtime
> > > > > PM from user space as that in combination with autosuspend should
> > > > > address your use case.
> > > > 
> > > > That, plus they want the touchscreen to go into runtime suspend 
> > > > whenever the screen is off (was this not the main reason for the 
> > > > patch?).
> > > > 
> > > > It seems to me that it should be possible to arrange for this to happen 
> > > > simply by making userspace close the touchscreen device when the screen 
> > > > is turned off.  Or am I missing something?
> > > 
> > > Well... that's not what existing userspace expects. Your X windows
> > > server will not close the touchscreen.
> > 
> > Surely that's a userspace issue, rather than a kernel problem?  The X
> > server does have some notion of power management and power savings; why
> > not extend that notion to include touchscreens?
> 
> It is not really practical: there are many consumers of input events, if
> we build infrastructure to control it and proxy all users through it,
> why not have it in kernel? Plus, there are users of input events
> directly in the kernel, such as legacy VT/keyboard, or Android/ChromeOS
> cpufreq_interactive governor that monitors user activity and bumps up
> CPU speed when user actively interacts with the device. They would keep
> input devices active even though user might not be actually able to
> use some input devices.

It sounds like you are suggesting there should be a general mechanism
for userspace to tell the kernel (or the input core) to ignore all
events from a particular input device -- or even from all input devices
-- thereby allowing those devices to go to low power.

I don't like to think of this as "forcing runtime suspend".  It's more
like telling the kernel that a device is no longer being used, so the
natural runtime PM mechanism can put it in runtime suspend.

Perhaps another way to think about it is that these input devices 
should not increment their runtime usage counter as part of the open 
routine; they should use something other than the number of open file 
references to indicate when they can go into runtime suspend.  (I'm not 
sure what else they should use, though.)

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Dmitry Torokhov]

On Mon, Sep 21, 2015 at 12:34:56PM -0400, Alan Stern wrote:
> On Mon, 21 Sep 2015, Dmitry Torokhov wrote:
> 
> > On Mon, Sep 21, 2015 at 10:38:46AM -0400, Alan Stern wrote:
> > > On Mon, 21 Sep 2015, Pavel Machek wrote:
> > > 
> > > > > > In fact, then, what you need seems to be the feature discussed by Alan
> > > > > > and me some time ago allowing remote wakeup do be disabled for runtime
> > > > > > PM from user space as that in combination with autosuspend should
> > > > > > address your use case.
> > > > > 
> > > > > That, plus they want the touchscreen to go into runtime suspend 
> > > > > whenever the screen is off (was this not the main reason for the 
> > > > > patch?).
> > > > > 
> > > > > It seems to me that it should be possible to arrange for this to happen 
> > > > > simply by making userspace close the touchscreen device when the screen 
> > > > > is turned off.  Or am I missing something?
> > > > 
> > > > Well... that's not what existing userspace expects. Your X windows
> > > > server will not close the touchscreen.
> > > 
> > > Surely that's a userspace issue, rather than a kernel problem?  The X
> > > server does have some notion of power management and power savings; why
> > > not extend that notion to include touchscreens?
> > 
> > It is not really practical: there are many consumers of input events, if
> > we build infrastructure to control it and proxy all users through it,
> > why not have it in kernel? Plus, there are users of input events
> > directly in the kernel, such as legacy VT/keyboard, or Android/ChromeOS
> > cpufreq_interactive governor that monitors user activity and bumps up
> > CPU speed when user actively interacts with the device. They would keep
> > input devices active even though user might not be actually able to
> > use some input devices.
> 
> It sounds like you are suggesting there should be a general mechanism
> for userspace to tell the kernel (or the input core) to ignore all
> events from a particular input device -- or even from all input devices
> -- thereby allowing those devices to go to low power.

Yes. In ChromeOS we have a custim "inhibit" control that:

1. Tells input core to ignore all events form a given device
2. Allows driver to put device in low power mode if driver desires to do
so. The driver can do it via runtime PM or on it's own. Usually on it's
own since when using runtime PM userspace may disable it, which may not
be desirable.

I would love to have something generic instead of input-specific.

> 
> I don't like to think of this as "forcing runtime suspend".  It's more
> like telling the kernel that a device is no longer being used, so the
> natural runtime PM mechanism can put it in runtime suspend.

I'd call it "accelerating" runtime suspend. Userspace tells the kernel
that it intends not to use given device and kernel reacts accordingly.

> 
> Perhaps another way to think about it is that these input devices 
> should not increment their runtime usage counter as part of the open 
> routine; they should use something other than the number of open file 
> references to indicate when they can go into runtime suspend.  (I'm not 
> sure what else they should use, though.)

I do not really want input specific support; as I mentioned before we
have something like that in ChromeOS kernels but I was hesitant bringing
it upstream as I believe it is not necessarily input device specific and
I would love to have it implemented at device core level.

Thanks.

-- 
Dmitry

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Mon, 21 Sep 2015, Dmitry Torokhov wrote:

> > It sounds like you are suggesting there should be a general mechanism
> > for userspace to tell the kernel (or the input core) to ignore all
> > events from a particular input device -- or even from all input devices
> > -- thereby allowing those devices to go to low power.
> 
> Yes. In ChromeOS we have a custim "inhibit" control that:
> 
> 1. Tells input core to ignore all events form a given device
> 2. Allows driver to put device in low power mode if driver desires to do
> so. The driver can do it via runtime PM or on it's own. Usually on it's
> own since when using runtime PM userspace may disable it, which may not
> be desirable.
> 
> I would love to have something generic instead of input-specific.
> 
> > 
> > I don't like to think of this as "forcing runtime suspend".  It's more
> > like telling the kernel that a device is no longer being used, so the
> > natural runtime PM mechanism can put it in runtime suspend.
> 
> I'd call it "accelerating" runtime suspend. Userspace tells the kernel
> that it intends not to use given device and kernel reacts accordingly.

Okay.

> > Perhaps another way to think about it is that these input devices 
> > should not increment their runtime usage counter as part of the open 
> > routine; they should use something other than the number of open file 
> > references to indicate when they can go into runtime suspend.  (I'm not 
> > sure what else they should use, though.)
> 
> I do not really want input specific support; as I mentioned before we
> have something like that in ChromeOS kernels but I was hesitant bringing
> it upstream as I believe it is not necessarily input device specific and
> I would love to have it implemented at device core level.

That's not a bad idea.  On the other hand, there must be lots of 
devices which would not be suitable for this.  Disk drives, for 
instance.

What happens if the "inhibit" control is turned on and the driver puts 
the device into runtime suspend, but then an I/O request arrives?

	If the I/O request originated from userspace, it means the
	user is violating the terms of the "inhibit" control.  Should
	the request simply fail?

	What if the I/O request originated from somewhere in the
	kernel, not from the user?

	Or maybe the driver would want to carry out the request,
	overriding the "inhibit" control temporarily.  Does it simply
	turn off the control, meaning that the device won't go back
	into runtime suspend until userspace turns the control on
	again?

	Or if the driver doesn't turn off the "inhibit" control, then
	how does it know when it can safely put the device back into
	runtime suspend?

Qustions like these make me think that this mechanism is best suited 
for a kind of device that doesn't handle I/O requests.  In other words, 
something that just reports events as they occur -- which is another 
way of describing an input device!

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Dmitry Torokhov]

On Mon, Sep 21, 2015 at 01:32:38PM -0400, Alan Stern wrote:
> On Mon, 21 Sep 2015, Dmitry Torokhov wrote:
> 
> > > It sounds like you are suggesting there should be a general mechanism
> > > for userspace to tell the kernel (or the input core) to ignore all
> > > events from a particular input device -- or even from all input devices
> > > -- thereby allowing those devices to go to low power.
> > 
> > Yes. In ChromeOS we have a custim "inhibit" control that:
> > 
> > 1. Tells input core to ignore all events form a given device
> > 2. Allows driver to put device in low power mode if driver desires to do
> > so. The driver can do it via runtime PM or on it's own. Usually on it's
> > own since when using runtime PM userspace may disable it, which may not
> > be desirable.
> > 
> > I would love to have something generic instead of input-specific.
> > 
> > > 
> > > I don't like to think of this as "forcing runtime suspend".  It's more
> > > like telling the kernel that a device is no longer being used, so the
> > > natural runtime PM mechanism can put it in runtime suspend.
> > 
> > I'd call it "accelerating" runtime suspend. Userspace tells the kernel
> > that it intends not to use given device and kernel reacts accordingly.
> 
> Okay.
> 
> > > Perhaps another way to think about it is that these input devices 
> > > should not increment their runtime usage counter as part of the open 
> > > routine; they should use something other than the number of open file 
> > > references to indicate when they can go into runtime suspend.  (I'm not 
> > > sure what else they should use, though.)
> > 
> > I do not really want input specific support; as I mentioned before we
> > have something like that in ChromeOS kernels but I was hesitant bringing
> > it upstream as I believe it is not necessarily input device specific and
> > I would love to have it implemented at device core level.
> 
> That's not a bad idea.  On the other hand, there must be lots of 
> devices which would not be suitable for this.  Disk drives, for 
> instance.

Of course.

> 
> What happens if the "inhibit" control is turned on and the driver puts 
> the device into runtime suspend, but then an I/O request arrives?
> 
> 	If the I/O request originated from userspace, it means the
> 	user is violating the terms of the "inhibit" control.  Should
> 	the request simply fail?

What user? User that inhibited it or user that tried to use the device?

> 
> 	What if the I/O request originated from somewhere in the
> 	kernel, not from the user?

I think we should treat in-kernel users as all other users.

> 
> 	Or maybe the driver would want to carry out the request,
> 	overriding the "inhibit" control temporarily.  Does it simply
> 	turn off the control, meaning that the device won't go back
> 	into runtime suspend until userspace turns the control on
> 	again?
> 
> 	Or if the driver doesn't turn off the "inhibit" control, then
> 	how does it know when it can safely put the device back into
> 	runtime suspend?
> 
> Qustions like these make me think that this mechanism is best suited 
> for a kind of device that doesn't handle I/O requests.  In other words, 
> something that just reports events as they occur -- which is another 
> way of describing an input device!

Or maybe IIO device. Or hwmon. Or something else. I think if we allow
drivers (or subsystems) to opt in into this mechanism it will solve much
of worries about disks and similar devices that indeed not very suitable
for such mechanism.

Thanks.

- 
Dmitry

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Mon, 21 Sep 2015, Dmitry Torokhov wrote:

> > What happens if the "inhibit" control is turned on and the driver puts 
> > the device into runtime suspend, but then an I/O request arrives?
> > 
> > 	If the I/O request originated from userspace, it means the
> > 	user is violating the terms of the "inhibit" control.  Should
> > 	the request simply fail?
> 
> What user? User that inhibited it or user that tried to use the device?

Normally they would be the same.  But even if they aren't, someone has 
violated the kernel interface: The first user told the kernel a 
particular device wasn't going to be used, and then the second user 
tried to use it.

Of course, this issue doesn't arise for devices that merely report 
external events.

> > 	What if the I/O request originated from somewhere in the
> > 	kernel, not from the user?
> 
> I think we should treat in-kernel users as all other users.
> 
> > 
> > 	Or maybe the driver would want to carry out the request,
> > 	overriding the "inhibit" control temporarily.  Does it simply
> > 	turn off the control, meaning that the device won't go back
> > 	into runtime suspend until userspace turns the control on
> > 	again?
> > 
> > 	Or if the driver doesn't turn off the "inhibit" control, then
> > 	how does it know when it can safely put the device back into
> > 	runtime suspend?
> > 
> > Qustions like these make me think that this mechanism is best suited 
> > for a kind of device that doesn't handle I/O requests.  In other words, 
> > something that just reports events as they occur -- which is another 
> > way of describing an input device!
> 
> Or maybe IIO device. Or hwmon. Or something else. I think if we allow
> drivers (or subsystems) to opt in into this mechanism it will solve much
> of worries about disks and similar devices that indeed not very suitable
> for such mechanism.

Should the mechanism really be per-device?  Or would it be more useful 
to have a single "inhibit" setting that affected all the relevant 
devices at once?

The runtime-PM "usage" value for these devices is a little tricky to 
calculate.  It should be nonzero if there are any open files _and_ the 
device isn't "inhibited".  I don't know the best way to represent that 
kind of condition in the runtime PM framework.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Pavel Machek]

On Mon 2015-09-21 10:38:46, Alan Stern wrote:
> On Mon, 21 Sep 2015, Pavel Machek wrote:
> 
> > > > In fact, then, what you need seems to be the feature discussed by Alan
> > > > and me some time ago allowing remote wakeup do be disabled for runtime
> > > > PM from user space as that in combination with autosuspend should
> > > > address your use case.
> > > 
> > > That, plus they want the touchscreen to go into runtime suspend 
> > > whenever the screen is off (was this not the main reason for the 
> > > patch?).
> > > 
> > > It seems to me that it should be possible to arrange for this to happen 
> > > simply by making userspace close the touchscreen device when the screen 
> > > is turned off.  Or am I missing something?
> > 
> > Well... that's not what existing userspace expects. Your X windows
> > server will not close the touchscreen.
> 
> Surely that's a userspace issue, rather than a kernel problem?  The X
> server does have some notion of power management and power savings; why
> not extend that notion to include touchscreens?

Well... once upon a time, it was kernel job to mask differences
between different hardware platforms.

In a way, the hardware is "buggy" -- if your mouse clicked randomly
when you were not holding it in your hand it would be buggy... and it
would be nice for kernel to fix that "bug". 

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Dmitry Torokhov]

On Mon, Sep 21, 2015 at 04:02:01PM -0400, Alan Stern wrote:
> On Mon, 21 Sep 2015, Dmitry Torokhov wrote:
> 
> > > What happens if the "inhibit" control is turned on and the driver puts 
> > > the device into runtime suspend, but then an I/O request arrives?
> > > 
> > > 	If the I/O request originated from userspace, it means the
> > > 	user is violating the terms of the "inhibit" control.  Should
> > > 	the request simply fail?
> > 
> > What user? User that inhibited it or user that tried to use the device?
> 
> Normally they would be the same.  But even if they aren't, someone has 
> violated the kernel interface: The first user told the kernel a 
> particular device wasn't going to be used, and then the second user 
> tried to use it.
> 
> Of course, this issue doesn't arise for devices that merely report 
> external events.
> 
> > > 	What if the I/O request originated from somewhere in the
> > > 	kernel, not from the user?
> > 
> > I think we should treat in-kernel users as all other users.
> > 
> > > 
> > > 	Or maybe the driver would want to carry out the request,
> > > 	overriding the "inhibit" control temporarily.  Does it simply
> > > 	turn off the control, meaning that the device won't go back
> > > 	into runtime suspend until userspace turns the control on
> > > 	again?
> > > 
> > > 	Or if the driver doesn't turn off the "inhibit" control, then
> > > 	how does it know when it can safely put the device back into
> > > 	runtime suspend?
> > > 
> > > Qustions like these make me think that this mechanism is best suited 
> > > for a kind of device that doesn't handle I/O requests.  In other words, 
> > > something that just reports events as they occur -- which is another 
> > > way of describing an input device!
> > 
> > Or maybe IIO device. Or hwmon. Or something else. I think if we allow
> > drivers (or subsystems) to opt in into this mechanism it will solve much
> > of worries about disks and similar devices that indeed not very suitable
> > for such mechanism.
> 
> Should the mechanism really be per-device?  Or would it be more useful 
> to have a single "inhibit" setting that affected all the relevant 
> devices at once?

Definitely per device. Consider your laptop with external monitor and
keyboard connected. When you close the lid you want to inhibit internal
keyboard, touchpad and touchscreen while leaving external keyboard and
mouse working.

That's just one scenario.

Thanks.

-- 
Dmitry

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Mon, 2015-09-21 at 16:02 -0400, Alan Stern wrote:
> On Mon, 21 Sep 2015, Dmitry Torokhov wrote:
> 
> > > What happens if the "inhibit" control is turned on and the driver puts 
> > > the device into runtime suspend, but then an I/O request arrives?
> > > 
> > > 	If the I/O request originated from userspace, it means the
> > > 	user is violating the terms of the "inhibit" control.  Should
> > > 	the request simply fail?
> > 
> > What user? User that inhibited it or user that tried to use the device?
> 
> Normally they would be the same.  But even if they aren't, someone has 
> violated the kernel interface: The first user told the kernel a 
> particular device wasn't going to be used, and then the second user 
> tried to use it.

If we assume that user space speaks with a uniform voice on that
issue, it can just as well close the device. It seems to me that
declaring a device idle is a privileged operation.

> Of course, this issue doesn't arise for devices that merely report 
> external events.

Indeed. We can handle output to suspended devices by waking them.
I don't see why this case is different. We are talking about input
only.

> The runtime-PM "usage" value for these devices is a little tricky to 
> calculate.  It should be nonzero if there are any open files _and_ the 
> device isn't "inhibited".  I don't know the best way to represent that 
> kind of condition in the runtime PM framework.

Does that make sense in the generic framework at all? I still
think that drivers should cease IO for input in such cases.
That should involve a common callback, but no counter.

	Regards
		Oliver

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Tue, 22 Sep 2015, Oliver Neukum wrote:

> Indeed. We can handle output to suspended devices by waking them.
> I don't see why this case is different. We are talking about input
> only.
> 
> > The runtime-PM "usage" value for these devices is a little tricky to 
> > calculate.  It should be nonzero if there are any open files _and_ the 
> > device isn't "inhibited".  I don't know the best way to represent that 
> > kind of condition in the runtime PM framework.
> 
> Does that make sense in the generic framework at all? I still
> think that drivers should cease IO for input in such cases.
> That should involve a common callback, but no counter.

I'm not sure I understand what you're saying.  Are you suggesting that
this "inhibit" mechanism should involve a new callback different from
the existing runtime-PM callbacks?  And when this new callback is
invoked, drivers should cancel existing input requests (these devices
are input-only) and go to low power?

This would create a parallel runtime-PM mechanism which is independent
of the existing one.  Is that really a good idea?

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Tue, 2015-09-22 at 10:15 -0400, Alan Stern wrote:
> On Tue, 22 Sep 2015, Oliver Neukum wrote:
> 
> > Indeed. We can handle output to suspended devices by waking them.
> > I don't see why this case is different. We are talking about input
> > only.
> > 
> > > The runtime-PM "usage" value for these devices is a little tricky to 
> > > calculate.  It should be nonzero if there are any open files _and_ the 
> > > device isn't "inhibited".  I don't know the best way to represent that 
> > > kind of condition in the runtime PM framework.
> > 
> > Does that make sense in the generic framework at all? I still
> > think that drivers should cease IO for input in such cases.
> > That should involve a common callback, but no counter.
> 
> I'm not sure I understand what you're saying.  Are you suggesting that
> this "inhibit" mechanism should involve a new callback different from

Yes, there is no necessary relation to power management. If you put
your phone into your pocket, you will want to inhibit the touchscreen
even if that doesn't save power.

> the existing runtime-PM callbacks?  And when this new callback is
> invoked, drivers should cancel existing input requests (these devices
> are input-only) and go to low power?

Cancel, yes, going to low power is a consequence which needn't bother
the power subsystem. You need a callback. If there are spurious
events, the current heuristics will keep devices awake.
You must discard them anyway, as they are spurious. There's no point
in transporting over the bus at all. We can cease IO for input.

> This would create a parallel runtime-PM mechanism which is independent
> of the existing one.  Is that really a good idea?

It isn't strictly PM. It helps PM to do a better job, but
conceptually it is independent.

	Regards
		Oliver

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Tue, 22 Sep 2015, Oliver Neukum wrote:

> > I'm not sure I understand what you're saying.  Are you suggesting that
> > this "inhibit" mechanism should involve a new callback different from
> 
> Yes, there is no necessary relation to power management. If you put
> your phone into your pocket, you will want to inhibit the touchscreen
> even if that doesn't save power.
> 
> > the existing runtime-PM callbacks?  And when this new callback is
> > invoked, drivers should cancel existing input requests (these devices
> > are input-only) and go to low power?
> 
> Cancel, yes, going to low power is a consequence which needn't bother
> the power subsystem.

Going to low power needn't involve the power subsystem?  That sounds 
weird.

>  You need a callback. If there are spurious
> events, the current heuristics will keep devices awake.
> You must discard them anyway, as they are spurious. There's no point
> in transporting over the bus at all. We can cease IO for input.
> 
> > This would create a parallel runtime-PM mechanism which is independent
> > of the existing one.  Is that really a good idea?
> 
> It isn't strictly PM. It helps PM to do a better job, but
> conceptually it is independent.

So my next question is: _How_ can this help PM to do a better job?  
That is, what are the mechanisms?

One you have already stated: Lack of spurious events will help prevent 
unwanted wakeups (or unwanted failures to go to sleep).

But Dmitry made a stronger claim: Inhibiting an input device should 
allow the device to go to low power.  I would like to know how we can 
implement this cleanly.  The most straightforward approach is to use 
runtime PM, but it's not obvious how this can be made to work with the 
current API.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Tue, 2015-09-22 at 11:22 -0400, Alan Stern wrote:
> On Tue, 22 Sep 2015, Oliver Neukum wrote:
>  
> > Cancel, yes, going to low power is a consequence which needn't bother
> > the power subsystem.
> 
> Going to low power needn't involve the power subsystem?  That sounds 
> weird.

Think of it like rfkill. It makes sense to suspend an rfkilled device.
It still is the job of the driver to report that its device is idle.

> >  You need a callback. If there are spurious
> > events, the current heuristics will keep devices awake.
> > You must discard them anyway, as they are spurious. There's no point
> > in transporting over the bus at all. We can cease IO for input.
> > 
> > > This would create a parallel runtime-PM mechanism which is independent
> > > of the existing one.  Is that really a good idea?
> > 
> > It isn't strictly PM. It helps PM to do a better job, but
> > conceptually it is independent.
> 
> So my next question is: _How_ can this help PM to do a better job?  
> That is, what are the mechanisms?

"inhibit" -> driver stops input -> driver sets PM count to zero
-> PM subsystem acts

To go from the first to the second step a callback is needed

> One you have already stated: Lack of spurious events will help prevent 
> unwanted wakeups (or unwanted failures to go to sleep).

That too. We also save CPU cycles.

> But Dmitry made a stronger claim: Inhibiting an input device should 
> allow the device to go to low power.  I would like to know how we can 
> implement this cleanly.  The most straightforward approach is to use 
> runtime PM, but it's not obvious how this can be made to work with the 
> current API.

Yes, we can use the current API.
The key is that you think of the mechanism as induced idleness,
not forced suspend. We already have a perfectly working mechanism
for suspending idle devices.

	Regards
		Oliver

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Octavian Purdila]

On Wed, Sep 23, 2015 at 6:03 AM, Oliver Neukum <oneukum@suse.de> wrote:
>
> On Tue, 2015-09-22 at 11:22 -0400, Alan Stern wrote:
> > On Tue, 22 Sep 2015, Oliver Neukum wrote:
> >
> > > Cancel, yes, going to low power is a consequence which needn't bother
> > > the power subsystem.
> >
> > Going to low power needn't involve the power subsystem?  That sounds
> > weird.
>
> Think of it like rfkill. It makes sense to suspend an rfkilled device.
> It still is the job of the driver to report that its device is idle.
>
> > >  You need a callback. If there are spurious
> > > events, the current heuristics will keep devices awake.
> > > You must discard them anyway, as they are spurious. There's no point
> > > in transporting over the bus at all. We can cease IO for input.
> > >
> > > > This would create a parallel runtime-PM mechanism which is independent
> > > > of the existing one.  Is that really a good idea?
> > >
> > > It isn't strictly PM. It helps PM to do a better job, but
> > > conceptually it is independent.
> >
> > So my next question is: _How_ can this help PM to do a better job?
> > That is, what are the mechanisms?
>
> "inhibit" -> driver stops input -> driver sets PM count to zero
> -> PM subsystem acts
>
> To go from the first to the second step a callback is needed
>

The IIO drivers use this model. The application keeps the fd open but
there is a buffer enable switch to enable / disable input. Based on
that trigger drivers use pm runtime put operations to induce PM
idleness (and pm runtime get to wakeup the device).

> > One you have already stated: Lack of spurious events will help prevent
> > unwanted wakeups (or unwanted failures to go to sleep).
>
> That too. We also save CPU cycles.
>
> > But Dmitry made a stronger claim: Inhibiting an input device should
> > allow the device to go to low power.  I would like to know how we can
> > implement this cleanly.  The most straightforward approach is to use
> > runtime PM, but it's not obvious how this can be made to work with the
> > current API.
>
> Yes, we can use the current API.
> The key is that you think of the mechanism as induced idleness,
> not forced suspend. We already have a perfectly working mechanism
> for suspending idle devices.
>

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Wed, 23 Sep 2015, Oliver Neukum wrote:

> On Tue, 2015-09-22 at 11:22 -0400, Alan Stern wrote:
> > On Tue, 22 Sep 2015, Oliver Neukum wrote:
> >  
> > > Cancel, yes, going to low power is a consequence which needn't bother
> > > the power subsystem.
> > 
> > Going to low power needn't involve the power subsystem?  That sounds 
> > weird.
> 
> Think of it like rfkill. It makes sense to suspend an rfkilled device.
> It still is the job of the driver to report that its device is idle.

Reporting that the device is idle _does_ involve the power subsystem.  
But never mind...

> > So my next question is: _How_ can this help PM to do a better job?  
> > That is, what are the mechanisms?
> 
> "inhibit" -> driver stops input -> driver sets PM count to zero
> -> PM subsystem acts

Your third step here poses problems.  There is no runtime-PM API a 
driver or subsystem can use to set its usage counter to 0.  All it can 
do is increment or decrement the counter.

> To go from the first to the second step a callback is needed
> 
> > One you have already stated: Lack of spurious events will help prevent 
> > unwanted wakeups (or unwanted failures to go to sleep).
> 
> That too. We also save CPU cycles.
> 
> > But Dmitry made a stronger claim: Inhibiting an input device should 
> > allow the device to go to low power.  I would like to know how we can 
> > implement this cleanly.  The most straightforward approach is to use 
> > runtime PM, but it's not obvious how this can be made to work with the 
> > current API.
> 
> Yes, we can use the current API.
> The key is that you think of the mechanism as induced idleness,
> not forced suspend. We already have a perfectly working mechanism
> for suspending idle devices.

After thinking some more about this, here's what I've got.

Let's talk about input-only devices being in an "idle" state or a
"running" state:

	When the device is in the idle state, its driver does not 
	communicate with the device.  In particular, the driver does 
	not monitor for events or report them to the rest of the 
	kernel.  It is appropriate to put the device in runtime suspend
	with remote wakeup disabled.

	When the device is in the running state, its driver receives
	event reports from the device and propagates them forward.
	The subsystem/driver may choose to put the device in runtime
	suspend between events with remote wakeup enabled (like we do
	for USB keyboards if no LEDs are on) or it may choose to leave
	the device at full power the whole time.

The idea is that a device is in the idle state whenever the open file 
count is 0 _or_ it is "inhibited"; otherwise it is in the running 
state.

I tried to come up with a way to do some of this work in a central
core.  All I could think of was that the core should detect state
changes and inform the subsystem/driver when they occur.  Everything
else (starting and stopping I/O, adjusting the runtime-PM usage
counter) would have to be done by the subsystem/driver.

The problem is that a core generally isn't aware of when a file 
reference is opened or released.  Only the driver is.  Which means 
there's nothing that the core can do here; the driver and subsystem 
have to manage pretty much the whole thing.  A simple example:

open()
{
	mutex_lock(&dev->open_mutex);
	if (dev->open_count++ == 0 && !dev->inhibited) {
		pm_runtime_get_sync(dev);
		start_io(dev);
	}
	mutex_unlock(&dev->open_mutex);
}

release()
{
	mutex_lock(&dev->open_mutex);
	if (--dev->open_count == 0 && !dev->inhibited) {
		stop_io(dev);
		pm_runtime_put(dev);
	}
	mutex_unlock(&dev->open_mutex);
}

inhibit()
{
	mutex_lock(&dev->open_mutex);
	dev->inhibited = true;
	if (dev->open_count > 0) {
		stop_io(dev);
		pm_runtime_put(dev);
	}
	mutex_unlock(&dev->open_mutex);
}

uninhibit()
{
	mutex_lock(&dev->open_mutex);
	dev->inhibited = false;
	if (dev->open_count > 0) {
		pm_runtime_get_sync(dev);
		start_io(dev);
	}
	mutex_unlock(&dev->open_mutex);
}

This doesn't leave much room for the PM core or anything else.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Wednesday, September 23, 2015 10:55:52 AM Alan Stern wrote:
> On Wed, 23 Sep 2015, Oliver Neukum wrote:
> 
> > On Tue, 2015-09-22 at 11:22 -0400, Alan Stern wrote:
> > > On Tue, 22 Sep 2015, Oliver Neukum wrote:
> > >  
> > > > Cancel, yes, going to low power is a consequence which needn't bother
> > > > the power subsystem.
> > > 
> > > Going to low power needn't involve the power subsystem?  That sounds 
> > > weird.
> > 
> > Think of it like rfkill. It makes sense to suspend an rfkilled device.
> > It still is the job of the driver to report that its device is idle.
> 
> Reporting that the device is idle _does_ involve the power subsystem.  
> But never mind...
> 
> > > So my next question is: _How_ can this help PM to do a better job?  
> > > That is, what are the mechanisms?
> > 
> > "inhibit" -> driver stops input -> driver sets PM count to zero
> > -> PM subsystem acts
> 
> Your third step here poses problems.  There is no runtime-PM API a 
> driver or subsystem can use to set its usage counter to 0.  All it can 
> do is increment or decrement the counter.
> 
> > To go from the first to the second step a callback is needed
> > 
> > > One you have already stated: Lack of spurious events will help prevent 
> > > unwanted wakeups (or unwanted failures to go to sleep).
> > 
> > That too. We also save CPU cycles.
> > 
> > > But Dmitry made a stronger claim: Inhibiting an input device should 
> > > allow the device to go to low power.  I would like to know how we can 
> > > implement this cleanly.  The most straightforward approach is to use 
> > > runtime PM, but it's not obvious how this can be made to work with the 
> > > current API.
> > 
> > Yes, we can use the current API.
> > The key is that you think of the mechanism as induced idleness,
> > not forced suspend. We already have a perfectly working mechanism
> > for suspending idle devices.
> 
> After thinking some more about this, here's what I've got.
> 
> Let's talk about input-only devices being in an "idle" state or a
> "running" state:
> 
> 	When the device is in the idle state, its driver does not 
> 	communicate with the device.  In particular, the driver does 
> 	not monitor for events or report them to the rest of the 
> 	kernel.  It is appropriate to put the device in runtime suspend
> 	with remote wakeup disabled.
> 
> 	When the device is in the running state, its driver receives
> 	event reports from the device and propagates them forward.
> 	The subsystem/driver may choose to put the device in runtime
> 	suspend between events with remote wakeup enabled (like we do
> 	for USB keyboards if no LEDs are on) or it may choose to leave
> 	the device at full power the whole time.
> 
> The idea is that a device is in the idle state whenever the open file 
> count is 0 _or_ it is "inhibited"; otherwise it is in the running 
> state.
> 
> I tried to come up with a way to do some of this work in a central
> core.  All I could think of was that the core should detect state
> changes and inform the subsystem/driver when they occur.  Everything
> else (starting and stopping I/O, adjusting the runtime-PM usage
> counter) would have to be done by the subsystem/driver.
> 
> The problem is that a core generally isn't aware of when a file 
> reference is opened or released.  Only the driver is.  Which means 
> there's nothing that the core can do here; the driver and subsystem 
> have to manage pretty much the whole thing.  A simple example:
> 
> open()
> {
> 	mutex_lock(&dev->open_mutex);
> 	if (dev->open_count++ == 0 && !dev->inhibited) {
> 		pm_runtime_get_sync(dev);
> 		start_io(dev);
> 	}
> 	mutex_unlock(&dev->open_mutex);
> }
> 
> release()
> {
> 	mutex_lock(&dev->open_mutex);
> 	if (--dev->open_count == 0 && !dev->inhibited) {
> 		stop_io(dev);
> 		pm_runtime_put(dev);
> 	}
> 	mutex_unlock(&dev->open_mutex);
> }
> 
> inhibit()
> {
> 	mutex_lock(&dev->open_mutex);
> 	dev->inhibited = true;
> 	if (dev->open_count > 0) {
> 		stop_io(dev);
> 		pm_runtime_put(dev);
> 	}
> 	mutex_unlock(&dev->open_mutex);
> }
> 
> uninhibit()
> {
> 	mutex_lock(&dev->open_mutex);
> 	dev->inhibited = false;
> 	if (dev->open_count > 0) {
> 		pm_runtime_get_sync(dev);
> 		start_io(dev);
> 	}
> 	mutex_unlock(&dev->open_mutex);
> }
> 
> This doesn't leave much room for the PM core or anything else.

We are missing the "no remote wakeup" bit now (well, there is a PM QoS flag,
but it isn't very useful, so I'd prefer to replace it with a "no remote wakeup"
bit in struct dev_pm_info or something similar).

That is actually quite important, because (a) we can save energy but not
configuring the device to do remote wakeup in the first place and (b) that
may involve more than just the driver (for example, disabling PCI or ACPI
remote wakeup involves the bus type or similar).

So it looks like we need to be able to distinguish between "runtime suspend
with remote wakeup" and "runtime suspend without remote wakeup".

And if we do the latter, we may not even need the "inhibit" thing any more,
because suspended devices without that are not configured to do remote wakeup
cannot really signal anything in the majority of cases.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:

> We are missing the "no remote wakeup" bit now (well, there is a PM QoS flag,
> but it isn't very useful, so I'd prefer to replace it with a "no remote wakeup"
> bit in struct dev_pm_info or something similar).
> 
> That is actually quite important, because (a) we can save energy but not
> configuring the device to do remote wakeup in the first place and (b) that
> may involve more than just the driver (for example, disabling PCI or ACPI
> remote wakeup involves the bus type or similar).
> 
> So it looks like we need to be able to distinguish between "runtime suspend
> with remote wakeup" and "runtime suspend without remote wakeup".
> 
> And if we do the latter, we may not even need the "inhibit" thing any more,
> because suspended devices without that are not configured to do remote wakeup
> cannot really signal anything in the majority of cases.

That works only for drivers that use autosuspend to go to low power in
between events.  It doesn't work for drivers that remain at full power 
as long as the device file is open.  That kind of driver does require 
an "inhibit" interface.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Friday, September 25, 2015 10:29:55 AM Alan Stern wrote:
> On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:
> 
> > We are missing the "no remote wakeup" bit now (well, there is a PM QoS flag,
> > but it isn't very useful, so I'd prefer to replace it with a "no remote wakeup"
> > bit in struct dev_pm_info or something similar).
> > 
> > That is actually quite important, because (a) we can save energy but not
> > configuring the device to do remote wakeup in the first place and (b) that
> > may involve more than just the driver (for example, disabling PCI or ACPI
> > remote wakeup involves the bus type or similar).
> > 
> > So it looks like we need to be able to distinguish between "runtime suspend
> > with remote wakeup" and "runtime suspend without remote wakeup".
> > 
> > And if we do the latter, we may not even need the "inhibit" thing any more,
> > because suspended devices without that are not configured to do remote wakeup
> > cannot really signal anything in the majority of cases.
> 
> That works only for drivers that use autosuspend to go to low power in
> between events.  It doesn't work for drivers that remain at full power 
> as long as the device file is open.  That kind of driver does require 
> an "inhibit" interface.

Or an interface allowing user space to trigger pm_request_idle() for them.

So user space would change the "no remote wakeup" setting and then do the
"try to suspend now" thing.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:

> On Friday, September 25, 2015 10:29:55 AM Alan Stern wrote:
> > On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:
> > 
> > > We are missing the "no remote wakeup" bit now (well, there is a PM QoS flag,
> > > but it isn't very useful, so I'd prefer to replace it with a "no remote wakeup"
> > > bit in struct dev_pm_info or something similar).
> > > 
> > > That is actually quite important, because (a) we can save energy but not
> > > configuring the device to do remote wakeup in the first place and (b) that
> > > may involve more than just the driver (for example, disabling PCI or ACPI
> > > remote wakeup involves the bus type or similar).
> > > 
> > > So it looks like we need to be able to distinguish between "runtime suspend
> > > with remote wakeup" and "runtime suspend without remote wakeup".
> > > 
> > > And if we do the latter, we may not even need the "inhibit" thing any more,
> > > because suspended devices without that are not configured to do remote wakeup
> > > cannot really signal anything in the majority of cases.
> > 
> > That works only for drivers that use autosuspend to go to low power in
> > between events.  It doesn't work for drivers that remain at full power 
> > as long as the device file is open.  That kind of driver does require 
> > an "inhibit" interface.
> 
> Or an interface allowing user space to trigger pm_request_idle() for them.
> 
> So user space would change the "no remote wakeup" setting and then do the
> "try to suspend now" thing.

So something like:

	echo on >/sys/.../power/control  (in case the device was
			already in runtime suspend with wakeups enabled)
	echo off >/sys/.../power/wakeup
	echo auto >/sys/.../power/control

This should work.  But it would require that the driver doesn't
increment the usage counter when the device file is opened.  I can
imagine this might lead to trouble if you're dealing with hardware that
doesn't support remote wakeup very well.  The driver wouldn't be able
to work around the hardware issue by incrementing the usage counter.

In real life this might not be a serious issue.  I don't know.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Friday, September 25, 2015 05:13:04 PM Alan Stern wrote:
> On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:
> 
> > On Friday, September 25, 2015 10:29:55 AM Alan Stern wrote:
> > > On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:
> > > 
> > > > We are missing the "no remote wakeup" bit now (well, there is a PM QoS flag,
> > > > but it isn't very useful, so I'd prefer to replace it with a "no remote wakeup"
> > > > bit in struct dev_pm_info or something similar).
> > > > 
> > > > That is actually quite important, because (a) we can save energy but not
> > > > configuring the device to do remote wakeup in the first place and (b) that
> > > > may involve more than just the driver (for example, disabling PCI or ACPI
> > > > remote wakeup involves the bus type or similar).
> > > > 
> > > > So it looks like we need to be able to distinguish between "runtime suspend
> > > > with remote wakeup" and "runtime suspend without remote wakeup".
> > > > 
> > > > And if we do the latter, we may not even need the "inhibit" thing any more,
> > > > because suspended devices without that are not configured to do remote wakeup
> > > > cannot really signal anything in the majority of cases.
> > > 
> > > That works only for drivers that use autosuspend to go to low power in
> > > between events.  It doesn't work for drivers that remain at full power 
> > > as long as the device file is open.  That kind of driver does require 
> > > an "inhibit" interface.
> > 
> > Or an interface allowing user space to trigger pm_request_idle() for them.
> > 
> > So user space would change the "no remote wakeup" setting and then do the
> > "try to suspend now" thing.
> 
> So something like:
> 
> 	echo on >/sys/.../power/control  (in case the device was
> 			already in runtime suspend with wakeups enabled)
> 	echo off >/sys/.../power/wakeup
> 	echo auto >/sys/.../power/control

That, or there may be an additional value, say "aggressive", to write to the
control file in which case it becomes just

echo aggressive >/sys/.../power/control

> 
> This should work.  But it would require that the driver doesn't
> increment the usage counter when the device file is opened.

Right.

> I can imagine this might lead to trouble if you're dealing with hardware that
> doesn't support remote wakeup very well.  The driver wouldn't be able
> to work around the hardware issue by incrementing the usage counter.

Or the "aggressive" mode wouldn't work for it.

> In real life this might not be a serious issue.  I don't know.

Me neither.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Friday, September 25, 2015 11:52:23 PM Rafael J. Wysocki wrote:
> On Friday, September 25, 2015 05:13:04 PM Alan Stern wrote:
> > On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:
> > 
> > > On Friday, September 25, 2015 10:29:55 AM Alan Stern wrote:
> > > > On Fri, 25 Sep 2015, Rafael J. Wysocki wrote:
> > > > 
> > > > > We are missing the "no remote wakeup" bit now (well, there is a PM QoS flag,
> > > > > but it isn't very useful, so I'd prefer to replace it with a "no remote wakeup"
> > > > > bit in struct dev_pm_info or something similar).
> > > > > 
> > > > > That is actually quite important, because (a) we can save energy but not
> > > > > configuring the device to do remote wakeup in the first place and (b) that
> > > > > may involve more than just the driver (for example, disabling PCI or ACPI
> > > > > remote wakeup involves the bus type or similar).
> > > > > 
> > > > > So it looks like we need to be able to distinguish between "runtime suspend
> > > > > with remote wakeup" and "runtime suspend without remote wakeup".
> > > > > 
> > > > > And if we do the latter, we may not even need the "inhibit" thing any more,
> > > > > because suspended devices without that are not configured to do remote wakeup
> > > > > cannot really signal anything in the majority of cases.
> > > > 
> > > > That works only for drivers that use autosuspend to go to low power in
> > > > between events.  It doesn't work for drivers that remain at full power 
> > > > as long as the device file is open.  That kind of driver does require 
> > > > an "inhibit" interface.
> > > 
> > > Or an interface allowing user space to trigger pm_request_idle() for them.
> > > 
> > > So user space would change the "no remote wakeup" setting and then do the
> > > "try to suspend now" thing.
> > 
> > So something like:
> > 
> > 	echo on >/sys/.../power/control  (in case the device was
> > 			already in runtime suspend with wakeups enabled)
> > 	echo off >/sys/.../power/wakeup
> > 	echo auto >/sys/.../power/control
> 
> That, or there may be an additional value, say "aggressive", to write to the
> control file in which case it becomes just
> 
> echo aggressive >/sys/.../power/control

That said I suppose that the "off" value for the "wakeup" file might also be
useful in some other cases, so it likely is a better approach.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Sat, 26 Sep 2015, Rafael J. Wysocki wrote:

> > > So something like:
> > > 
> > > 	echo on >/sys/.../power/control  (in case the device was
> > > 			already in runtime suspend with wakeups enabled)
> > > 	echo off >/sys/.../power/wakeup
> > > 	echo auto >/sys/.../power/control

Cases where the driver wants to avoid runtime suspend (while the device
is active) because of bad wakeup support in the hardware can be handled
easily enough.  The runtime-idle or runtime-suspend callback routine
can check whether wakeup == off; if it isn't then the callback should
return -EBUSY.  Thus the driver can prevent runtime suspend without any
need to increment the usage counter.

> > That, or there may be an additional value, say "aggressive", to write to the
> > control file in which case it becomes just
> > 
> > echo aggressive >/sys/.../power/control
> 
> That said I suppose that the "off" value for the "wakeup" file might also be
> useful in some other cases, so it likely is a better approach.

We still need some sort of "inhibit" callback for cases where the
driver doesn't want to go into runtime suspend but does want to turn
off all I/O.  Should this callback be triggered when the user writes
"off" to power/wakeup, or when the user writes "inhibit" to
power/control, or should there be a separate sysfs attribute?

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Saturday, September 26, 2015 11:20:50 AM Alan Stern wrote:
> On Sat, 26 Sep 2015, Rafael J. Wysocki wrote:
> 
> > > > So something like:
> > > > 
> > > > 	echo on >/sys/.../power/control  (in case the device was
> > > > 			already in runtime suspend with wakeups enabled)
> > > > 	echo off >/sys/.../power/wakeup
> > > > 	echo auto >/sys/.../power/control
> 
> Cases where the driver wants to avoid runtime suspend (while the device
> is active) because of bad wakeup support in the hardware can be handled
> easily enough.  The runtime-idle or runtime-suspend callback routine
> can check whether wakeup == off; if it isn't then the callback should
> return -EBUSY.  Thus the driver can prevent runtime suspend without any
> need to increment the usage counter.

Right.

> > > That, or there may be an additional value, say "aggressive", to write to the
> > > control file in which case it becomes just
> > > 
> > > echo aggressive >/sys/.../power/control
> > 
> > That said I suppose that the "off" value for the "wakeup" file might also be
> > useful in some other cases, so it likely is a better approach.
> 
> We still need some sort of "inhibit" callback for cases where the
> driver doesn't want to go into runtime suspend but does want to turn
> off all I/O.  Should this callback be triggered when the user writes
> "off" to power/wakeup, or when the user writes "inhibit" to
> power/control, or should there be a separate sysfs attribute?

My first thought is that if there is a separate attribute, then it only actually
makes sense for devices that generate input events, while the "off" thing may
be generally useful in principle (eg. it may indicate to disable PME for the
device to the PCI layer etc).

OTOH, the additional "inhibit" attribute may only be exposed if the corresponding
callback is present, so I'm not really sure.

Question is, though, what's the use case for turning off I/O when we don't
go into runtime suspend.  After all, runtime suspend need not mean putting
the device into any kind of low-power state and the "off" thing may very
well be defined to mean that all input is discarded if the device is
runtime-suspended and the device is not configured to do remote wakeup
then.

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Sun, 27 Sep 2015, Rafael J. Wysocki wrote:

> On Saturday, September 26, 2015 11:20:50 AM Alan Stern wrote:
> > On Sat, 26 Sep 2015, Rafael J. Wysocki wrote:
> > 
> > > > > So something like:
> > > > > 
> > > > > 	echo on >/sys/.../power/control  (in case the device was
> > > > > 			already in runtime suspend with wakeups enabled)
> > > > > 	echo off >/sys/.../power/wakeup
> > > > > 	echo auto >/sys/.../power/control

> > We still need some sort of "inhibit" callback for cases where the
> > driver doesn't want to go into runtime suspend but does want to turn
> > off all I/O.  Should this callback be triggered when the user writes
> > "off" to power/wakeup, or when the user writes "inhibit" to
> > power/control, or should there be a separate sysfs attribute?
> 
> My first thought is that if there is a separate attribute, then it only actually
> makes sense for devices that generate input events, while the "off" thing may
> be generally useful in principle (eg. it may indicate to disable PME for the
> device to the PCI layer etc).

I'm not sure how much sense that distinction makes.  It seems to me the
only time you want to ignore potential wakeup events is if you want to
ignore _all_ input.  Which is basically what "inhibit" means.

This suggests we forget about power/wakeup == "off" and introduce an 
"inhibit" attribute instead.

> OTOH, the additional "inhibit" attribute may only be exposed if the corresponding
> callback is present, so I'm not really sure.

It could be a separate attribute, or it could be a new entry for
power/control.  Come to think of it, a separate attribute might be
better.  Otherwise we would lose track of whether runtime suspend was
permitted (the "on" vs. "auto" distinction) when the device was
inhibited.  I can imagine someone might want to forbid runtime suspend
but still inhibit a device.

However, I agree that there's no point registering a separate attribute
or accepting a write of "inhibit" to power/control if there's no
corresponding callback.

> Question is, though, what's the use case for turning off I/O when we don't
> go into runtime suspend.  After all, runtime suspend need not mean putting
> the device into any kind of low-power state and the "off" thing may very
> well be defined to mean that all input is discarded if the device is
> runtime-suspended and the device is not configured to do remote wakeup
> then.

Well, I suppose there might be a driver that supports inhibit but
doesn't support runtime PM, unlikely as that seems.  Or the driver
might support both but the user might leave power/control == "on" while
inhibiting the device.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Pavel Machek]

Hi!

> > > > That, or there may be an additional value, say "aggressive", to write to the
> > > > control file in which case it becomes just
> > > > 
> > > > echo aggressive >/sys/.../power/control
> > > 
> > > That said I suppose that the "off" value for the "wakeup" file might also be
> > > useful in some other cases, so it likely is a better approach.
> > 
> > We still need some sort of "inhibit" callback for cases where the
> > driver doesn't want to go into runtime suspend but does want to turn
> > off all I/O.  Should this callback be triggered when the user writes
> > "off" to power/wakeup, or when the user writes "inhibit" to
> > power/control, or should there be a separate sysfs attribute?
> 
> My first thought is that if there is a separate attribute, then it only actually
> makes sense for devices that generate input events, while the "off" thing may
> be generally useful in principle (eg. it may indicate to disable PME for the
> device to the PCI layer etc).
> 
> OTOH, the additional "inhibit" attribute may only be exposed if the corresponding
> callback is present, so I'm not really sure.
> 
> Question is, though, what's the use case for turning off I/O when we don't
> go into runtime suspend.  After all, runtime suspend need not mean putting

Well... In "cellphone goes to pocket" case, you want to turn off I/O even if
the touchscreen can not support runtime suspend.

See parents in the thread for explanation.

									Pavel

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Sunday, September 27, 2015 10:27:25 AM Alan Stern wrote:
> On Sun, 27 Sep 2015, Rafael J. Wysocki wrote:
> 
> > On Saturday, September 26, 2015 11:20:50 AM Alan Stern wrote:
> > > On Sat, 26 Sep 2015, Rafael J. Wysocki wrote:
> > > 
> > > > > > So something like:
> > > > > > 
> > > > > > 	echo on >/sys/.../power/control  (in case the device was
> > > > > > 			already in runtime suspend with wakeups enabled)
> > > > > > 	echo off >/sys/.../power/wakeup
> > > > > > 	echo auto >/sys/.../power/control
> 
> > > We still need some sort of "inhibit" callback for cases where the
> > > driver doesn't want to go into runtime suspend but does want to turn
> > > off all I/O.  Should this callback be triggered when the user writes
> > > "off" to power/wakeup, or when the user writes "inhibit" to
> > > power/control, or should there be a separate sysfs attribute?
> > 
> > My first thought is that if there is a separate attribute, then it only actually
> > makes sense for devices that generate input events, while the "off" thing may
> > be generally useful in principle (eg. it may indicate to disable PME for the
> > device to the PCI layer etc).
> 
> I'm not sure how much sense that distinction makes.  It seems to me the
> only time you want to ignore potential wakeup events is if you want to
> ignore _all_ input.  Which is basically what "inhibit" means.

The other case I had in mind is specific to the PCI layer and might be better
served by adding an "ignore PME" flag to PCI devices.

> This suggests we forget about power/wakeup == "off" and introduce an 
> "inhibit" attribute instead.

If we do that, can it still be regarded as a PM attribute?

And what about the corresponding callback?  Should that be a PM callback or
a general one?

> > OTOH, the additional "inhibit" attribute may only be exposed if the corresponding
> > callback is present, so I'm not really sure.
> 
> It could be a separate attribute, or it could be a new entry for
> power/control.  Come to think of it, a separate attribute might be
> better.  Otherwise we would lose track of whether runtime suspend was
> permitted (the "on" vs. "auto" distinction) when the device was
> inhibited.  I can imagine someone might want to forbid runtime suspend
> but still inhibit a device.
> 
> However, I agree that there's no point registering a separate attribute
> or accepting a write of "inhibit" to power/control if there's no
> corresponding callback.
> 
> > Question is, though, what's the use case for turning off I/O when we don't
> > go into runtime suspend.  After all, runtime suspend need not mean putting
> > the device into any kind of low-power state and the "off" thing may very
> > well be defined to mean that all input is discarded if the device is
> > runtime-suspended and the device is not configured to do remote wakeup
> > then.
> 
> Well, I suppose there might be a driver that supports inhibit but
> doesn't support runtime PM, unlikely as that seems.  Or the driver
> might support both but the user might leave power/control == "on" while
> inhibiting the device.

That sounds like a general rather than PM-related mechanism then.

I guess we need a real use case for that last thing or it will be rather
difficult to convince Greg to accept the patch. :-)

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

On Sunday, September 27, 2015 07:02:17 PM Pavel Machek wrote:
> Hi!

Hi,

> > > > > That, or there may be an additional value, say "aggressive", to write to the
> > > > > control file in which case it becomes just
> > > > > 
> > > > > echo aggressive >/sys/.../power/control
> > > > 
> > > > That said I suppose that the "off" value for the "wakeup" file might also be
> > > > useful in some other cases, so it likely is a better approach.
> > > 
> > > We still need some sort of "inhibit" callback for cases where the
> > > driver doesn't want to go into runtime suspend but does want to turn
> > > off all I/O.  Should this callback be triggered when the user writes
> > > "off" to power/wakeup, or when the user writes "inhibit" to
> > > power/control, or should there be a separate sysfs attribute?
> > 
> > My first thought is that if there is a separate attribute, then it only actually
> > makes sense for devices that generate input events, while the "off" thing may
> > be generally useful in principle (eg. it may indicate to disable PME for the
> > device to the PCI layer etc).
> > 
> > OTOH, the additional "inhibit" attribute may only be exposed if the corresponding
> > callback is present, so I'm not really sure.
> > 
> > Question is, though, what's the use case for turning off I/O when we don't
> > go into runtime suspend.  After all, runtime suspend need not mean putting
> 
> Well... In "cellphone goes to pocket" case, you want to turn off I/O even if
> the touchscreen can not support runtime suspend.
> 
> See parents in the thread for explanation.

You seem to be confusing the ability to go into low-power states with supporting
runtime PM.  The latter by no means requires the former.

Also "cellphone goes to pocket" is really two different cases.  One is when
the user indicated "I'm not going to use the phone going forward" somehow (like
by pressing a screen-off button) and one is when (s)he didn't.

In the second case we really have no reason to discard any input and in the
first one we may as well go straight for runtime suspend (or even for system
suspend for that matter).

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Mon, 28 Sep 2015, Rafael J. Wysocki wrote:

> > This suggests we forget about power/wakeup == "off" and introduce an 
> > "inhibit" attribute instead.
> 
> If we do that, can it still be regarded as a PM attribute?

Why not?  Consider this: Is there any reason to support inhibit when
CONFIG_PM is disabled?  I can't come up with any.

> And what about the corresponding callback?  Should that be a PM callback or
> a general one?

Well, if "inhibit" is a PM attribute then the callback should be a PM 
callback.  :-)

> > > Question is, though, what's the use case for turning off I/O when we don't
> > > go into runtime suspend.  After all, runtime suspend need not mean putting
> > > the device into any kind of low-power state and the "off" thing may very
> > > well be defined to mean that all input is discarded if the device is
> > > runtime-suspended and the device is not configured to do remote wakeup
> > > then.
> > 
> > Well, I suppose there might be a driver that supports inhibit but
> > doesn't support runtime PM, unlikely as that seems.  Or the driver
> > might support both but the user might leave power/control == "on" while
> > inhibiting the device.
> 
> That sounds like a general rather than PM-related mechanism then.

I don't follow your reasoning.

> I guess we need a real use case for that last thing or it will be rather
> difficult to convince Greg to accept the patch. :-)

The hard part is to come up with a design that Greg agrees with.  If 
the design is okay, there's no reason not to accept the patch.

One of the questions amounts to this: Do we want to allow situations
where input is inhibited but the user prevents the device from going
into runtime suspend by setting power/control = "on"?  If the answer is
Yes then "inhibit" should be a separate attribute.  Otherwise, we can 
just let "inhibit" be another setting in power/control.

Another question is: Do we want to make it easy for drivers to support
inhibit while still incrementing their PM usage counter every time the
device file is opened?  If we do then inhibit must be considered
separate from runtime suspend, because a device _can't_ go directly
into runtime suspend when the usage counter is > 1.  If we don't then 
we will most likely have to change the runtime-PM support in some 
drivers before they can implement inhibit.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Rafael J. Wysocki]

Hi Alan,

On Mon, Sep 28, 2015 at 4:29 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 28 Sep 2015, Rafael J. Wysocki wrote:
>
>> > This suggests we forget about power/wakeup == "off" and introduce an
>> > "inhibit" attribute instead.
>>
>> If we do that, can it still be regarded as a PM attribute?
>
> Why not?  Consider this: Is there any reason to support inhibit when
> CONFIG_PM is disabled?  I can't come up with any.

Well, the "I don't want any input from you now, because the phone is
going into a pocket" case?

It isn't stticlty dependent on PM.

>> And what about the corresponding callback?  Should that be a PM callback or
>> a general one?
>
> Well, if "inhibit" is a PM attribute then the callback should be a PM
> callback.  :-)
>
>> > > Question is, though, what's the use case for turning off I/O when we don't
>> > > go into runtime suspend.  After all, runtime suspend need not mean putting
>> > > the device into any kind of low-power state and the "off" thing may very
>> > > well be defined to mean that all input is discarded if the device is
>> > > runtime-suspended and the device is not configured to do remote wakeup
>> > > then.
>> >
>> > Well, I suppose there might be a driver that supports inhibit but
>> > doesn't support runtime PM, unlikely as that seems.  Or the driver
>> > might support both but the user might leave power/control == "on" while
>> > inhibiting the device.
>>
>> That sounds like a general rather than PM-related mechanism then.
>
> I don't follow your reasoning.

Support for "inhibit" and lack of runtime PM support means that the
feature has nothing to do with PM any more AFAICS.

That's why I think it may be regarded by more than just PM.  It should
make runtime PM behave in a specific way if supported, but then it
should work withot it too, shouldn't it?

>> I guess we need a real use case for that last thing or it will be rather
>> difficult to convince Greg to accept the patch. :-)
>
> The hard part is to come up with a design that Greg agrees with.  If
> the design is okay, there's no reason not to accept the patch.
>
> One of the questions amounts to this: Do we want to allow situations
> where input is inhibited but the user prevents the device from going
> into runtime suspend by setting power/control = "on"?  If the answer is
> Yes then "inhibit" should be a separate attribute.  Otherwise, we can
> just let "inhibit" be another setting in power/control.

I think that the answer really is "yes" in general as long as it makes
sense to discard input without closing the device.

My understanding of "inhibit" would be "discard all input including
any wakeup events from now on".  It would be quite natural for runtime
suspend to trigger if that is set (unless disabled or not supported,
of course), but I don't think that this should be a requirement.

> Another question is: Do we want to make it easy for drivers to support
> inhibit while still incrementing their PM usage counter every time the
> device file is opened?  If we do then inhibit must be considered
> separate from runtime suspend, because a device _can't_ go directly
> into runtime suspend when the usage counter is > 1.  If we don't then
> we will most likely have to change the runtime-PM support in some
> drivers before they can implement inhibit.

My opinion is that "inhibit" should affect PM, but should not require
PM to function (there's no technical reason for that).

Thanks,
Rafael

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Alan Stern]

On Mon, 28 Sep 2015, Rafael J. Wysocki wrote:

> Hi Alan,
> 
> On Mon, Sep 28, 2015 at 4:29 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Mon, 28 Sep 2015, Rafael J. Wysocki wrote:
> >
> >> > This suggests we forget about power/wakeup == "off" and introduce an
> >> > "inhibit" attribute instead.
> >>
> >> If we do that, can it still be regarded as a PM attribute?
> >
> > Why not?  Consider this: Is there any reason to support inhibit when
> > CONFIG_PM is disabled?  I can't come up with any.
> 
> Well, the "I don't want any input from you now, because the phone is
> going into a pocket" case?

But who would make a phone without CONFIG_PM?  If you're sufficiently 
unconcerned about power usage that you turn off CONFIG_PM, then you 
probably don't care about getting excess input events either.

> It isn't stticlty dependent on PM.

No, not strictly.  But it is closely enough related that people
shouldn't mind if it becomes part of the PM code.

> >> > Well, I suppose there might be a driver that supports inhibit but
> >> > doesn't support runtime PM, unlikely as that seems.  Or the driver
> >> > might support both but the user might leave power/control == "on" while
> >> > inhibiting the device.
> >>
> >> That sounds like a general rather than PM-related mechanism then.
> >
> > I don't follow your reasoning.
> 
> Support for "inhibit" and lack of runtime PM support means that the
> feature has nothing to do with PM any more AFAICS.

My example above referred to support in a single driver, not support in 
the system as a whole.  By the same reasoning, since some drivers 
support system sleep but not runtime PM, system sleep must have nothing 
to do with PM.  :-)

> That's why I think it may be regarded by more than just PM.  It should
> make runtime PM behave in a specific way if supported, but then it
> should work withot it too, shouldn't it?

If you want inhibit to be part of the device core rather than the PM
core, that's okay with me.

> My opinion is that "inhibit" should affect PM, but should not require
> PM to function (there's no technical reason for that).

All right.  Then a design should be straightforward.

Alan Stern

Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Pavel Machek]

Hi!

> > >
> > >> > This suggests we forget about power/wakeup == "off" and introduce an
> > >> > "inhibit" attribute instead.
> > >>
> > >> If we do that, can it still be regarded as a PM attribute?
> > >
> > > Why not?  Consider this: Is there any reason to support inhibit when
> > > CONFIG_PM is disabled?  I can't come up with any.
> > 
> > Well, the "I don't want any input from you now, because the phone is
> > going into a pocket" case?
> 
> But who would make a phone without CONFIG_PM?  If you're sufficiently 
> unconcerned about power usage that you turn off CONFIG_PM, then you 
> probably don't care about getting excess input events either.

Well.. .excess input events means that your phone now sends (meaningful, thanks
to advanced predictions) messages to your friends...

Better not do that.

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html