Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCHv2 0/7] Support inhibiting input devices
       [not found] <20200506002746.GB89269@dtor-ws>
@ 2020-05-15 16:49 ` Andrzej Pietrasiewicz
  2020-05-15 16:51   ` [PATCHv2 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
                     ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-15 16:49 UTC (permalink / raw)
  To: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
	linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
	platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Hans de Goede, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, Andrzej Pietrasiewicz, kernel

Userspace might want to implement a policy to temporarily disregard input
from certain devices, including not treating them as wakeup sources.

An example use case is a laptop, whose keyboard can be folded under the
screen to create tablet-like experience. The user then must hold the laptop
in such a way that it is difficult to avoid pressing the keyboard keys. It
is therefore desirable to temporarily disregard input from the keyboard,
until it is folded back. This obviously is a policy which should be kept
out of the kernel, but the kernel must provide suitable means to implement
such a policy.

Due to interactions with suspend/resume, a helper has been added for drivers
to decide if the device is being used or not (PATCH 1/7) and it has been
applied to relevant drivers (PATCH 2-5/7). Patches 2-5 are only being sent
to relevant mailing lists and maintainers.

PATCH 6/7 adds support for inhibiting input devices, while PATCH 7/7
provides an example how to convert a driver to take advantage of this
new feature. Patch 7/7 is only being sent to input mailing list and
maintainer.

This work is inspired by:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/45c2d7bb398f74adfae0017e20b224152fde3822

and

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4ce0e8a3697edb8fd071110b3af65014512061c7

v1..v2:
- added input_device_enabled() helper and used it in drivers (Dmitry)
- the fact of open() and close() being called in inhibit/uninhibit paths has
been emphasized in the commit message of PATCH 6/7 (Dmitry)

Andrzej Pietrasiewicz (5):
  Input: add input_device_enabled()
  Input: use input_device_enabled()
  ACPI: button: Use input_device_enabled() helper
  iio: adc: exynos: Use input_device_enabled()
  platform/x86: thinkpad_acpi: Use input_device_enabled()

Dmitry Torokhov (1):
  Input: elan_i2c: Support inhibiting

Patrik Fimml (1):
  Input: Add "inhibited" property

 drivers/acpi/button.c                       |   8 +-
 drivers/iio/adc/exynos_adc.c                |  11 +-
 drivers/input/input.c                       | 142 ++++++++++++++++++--
 drivers/input/joystick/xpad.c               |   4 +-
 drivers/input/keyboard/ep93xx_keypad.c      |   2 +-
 drivers/input/keyboard/gpio_keys.c          |   4 +-
 drivers/input/keyboard/imx_keypad.c         |   4 +-
 drivers/input/keyboard/ipaq-micro-keys.c    |   2 +-
 drivers/input/keyboard/lpc32xx-keys.c       |   4 +-
 drivers/input/keyboard/pmic8xxx-keypad.c    |   4 +-
 drivers/input/keyboard/pxa27x_keypad.c      |   2 +-
 drivers/input/keyboard/samsung-keypad.c     |   4 +-
 drivers/input/keyboard/spear-keyboard.c     |   8 +-
 drivers/input/keyboard/st-keyscan.c         |   4 +-
 drivers/input/keyboard/tegra-kbc.c          |   4 +-
 drivers/input/misc/drv260x.c                |   4 +-
 drivers/input/misc/drv2665.c                |   4 +-
 drivers/input/misc/drv2667.c                |   4 +-
 drivers/input/misc/gp2ap002a00f.c           |   4 +-
 drivers/input/misc/kxtj9.c                  |   4 +-
 drivers/input/misc/sirfsoc-onkey.c          |   2 +-
 drivers/input/mouse/elan_i2c_core.c         | 112 +++++++++++----
 drivers/input/mouse/navpoint.c              |   4 +-
 drivers/input/touchscreen/ad7879.c          |   6 +-
 drivers/input/touchscreen/atmel_mxt_ts.c    |   4 +-
 drivers/input/touchscreen/auo-pixcir-ts.c   |   8 +-
 drivers/input/touchscreen/bu21029_ts.c      |   4 +-
 drivers/input/touchscreen/chipone_icn8318.c |   4 +-
 drivers/input/touchscreen/cyttsp_core.c     |   4 +-
 drivers/input/touchscreen/eeti_ts.c         |   4 +-
 drivers/input/touchscreen/ektf2127.c        |   4 +-
 drivers/input/touchscreen/imx6ul_tsc.c      |   4 +-
 drivers/input/touchscreen/ipaq-micro-ts.c   |   2 +-
 drivers/input/touchscreen/iqs5xx.c          |   4 +-
 drivers/input/touchscreen/lpc32xx_ts.c      |   4 +-
 drivers/input/touchscreen/melfas_mip4.c     |   4 +-
 drivers/input/touchscreen/mms114.c          |   6 +-
 drivers/input/touchscreen/pixcir_i2c_ts.c   |   8 +-
 drivers/input/touchscreen/ucb1400_ts.c      |   4 +-
 drivers/input/touchscreen/wm97xx-core.c     |  14 +-
 drivers/input/touchscreen/zforce_ts.c       |   8 +-
 drivers/platform/x86/thinkpad_acpi.c        |   4 +-
 include/linux/input.h                       |  10 ++
 43 files changed, 336 insertions(+), 119 deletions(-)


base-commit: 2ef96a5bb12be62ef75b5828c0aab838ebb29cb8
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCHv2 1/7] Input: add input_device_enabled()
  2020-05-15 16:49 ` [PATCHv2 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
@ 2020-05-15 16:51   ` Andrzej Pietrasiewicz
  2020-05-15 16:52   ` [PATCHv2 6/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-15 16:51 UTC (permalink / raw)
  To: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
	linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
	platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Hans de Goede, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, Andrzej Pietrasiewicz, kernel

A helper function for drivers to decide if the device is used or not.
A lockdep check is introduced as inspecting ->users should be done under
input device's mutex.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/input/input.c | 8 ++++++++
 include/linux/input.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3cfd2c18eebd..41377bfa142d 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2127,6 +2127,14 @@ void input_enable_softrepeat(struct input_dev *dev, int delay, int period)
 }
 EXPORT_SYMBOL(input_enable_softrepeat);
 
+bool input_device_enabled(struct input_dev *dev)
+{
+	lockdep_assert_held(&dev->mutex);
+
+	return dev->users > 0;
+}
+EXPORT_SYMBOL_GPL(input_device_enabled);
+
 /**
  * input_register_device - register device with input core
  * @dev: device to be registered
diff --git a/include/linux/input.h b/include/linux/input.h
index 56f2fd32e609..eda4587dba67 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -502,6 +502,8 @@ bool input_match_device_id(const struct input_dev *dev,
 
 void input_enable_softrepeat(struct input_dev *dev, int delay, int period);
 
+bool input_device_enabled(struct input_dev *dev);
+
 extern struct class input_class;
 
 /**
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCHv2 6/7] Input: Add "inhibited" property
  2020-05-15 16:49 ` [PATCHv2 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
  2020-05-15 16:51   ` [PATCHv2 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
@ 2020-05-15 16:52   ` Andrzej Pietrasiewicz
  2020-05-15 16:53   ` [PATCHv2 3/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
  2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
  3 siblings, 0 replies; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-15 16:52 UTC (permalink / raw)
  To: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
	linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
	platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Hans de Goede, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, Andrzej Pietrasiewicz, kernel,
	Patrik Fimml

From: Patrik Fimml <patrikf@chromium.org>

Userspace might want to implement a policy to temporarily disregard input
from certain devices, including not treating them as wakeup sources.

An example use case is a laptop, whose keyboard can be folded under the
screen to create tablet-like experience. The user then must hold the laptop
in such a way that it is difficult to avoid pressing the keyboard keys. It
is therefore desirable to temporarily disregard input from the keyboard,
until it is folded back. This obviously is a policy which should be kept
out of the kernel, but the kernel must provide suitable means to implement
such a policy.

This patch adds a sysfs interface for exactly this purpose.

To implement the said interface it adds an "inhibited" property to struct
input_dev and two optional methods - inhibit() and uninhibit(), and
effectively creates four states a device can be in: closed uninhibited,
closed inhibited, open uninhibited, open inhibited. It also defers calling
driver's ->open() and ->close() to until they are actually needed, e.g. it
makes no sense to prepare the underlying device for generating events
(->open()) if the device is inhibited.

              uninhibit
closed      <------------ closed
uninhibited ------------> inhibited
      | ^     inhibit        | ^
 1st  | |               1st  | |
 open | |               open | |
      | |                    | |
      | | last               | | last
      | | close              | | close
      v |     uninhibit      v |
open        <------------ open
uninhibited ------------> inhibited

The top inhibit/uninhibit transition happens when users == 0.
The bottom inhibit/uninhibit transition happens when users > 0.
The left open/close transition happens when !inhibited.
The right open/close transition happens when inhibited.
Due to all transitions being serialized with dev->mutex, it is impossible
to have "diagonal" transitions between closed uninhibited and open
inhibited or between open uninhibited and closed inhibited.

open() and close() - if provided - are called in both inhibit and uninhibit
paths. Please note that close() does not return a value, so if your driver
might need failing inhibiting, you need to provide inhibit() so that it
returns a value to check.

It is drivers' responsibility to implement their inhibiting capability in
terms of whatever is suitable in their context, be it open/close,
inhibit/uninhibit or a combination of both. The drivers should also ensure
that they properly interact with suspend/resume and PM runtime, because
most likely a side effect of inhibiting a device should be its going into
low power mode. Properly inhibiting a device means to prevent it from being
a wakeup source, so drivers should also take care of that.

Signed-off-by: Patrik Fimml <patrikf@chromium.org>
Co-developed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/input/input.c | 136 ++++++++++++++++++++++++++++++++++++++----
 include/linux/input.h |   8 +++
 2 files changed, 134 insertions(+), 10 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 41377bfa142d..5b859a178c11 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -367,8 +367,13 @@ static int input_get_disposition(struct input_dev *dev,
 static void input_handle_event(struct input_dev *dev,
 			       unsigned int type, unsigned int code, int value)
 {
-	int disposition = input_get_disposition(dev, type, code, &value);
+	int disposition;
 
+	/* filter-out events from inhibited devices */
+	if (dev->inhibited)
+		return;
+
+	disposition = input_get_disposition(dev, type, code, &value);
 	if (disposition != INPUT_IGNORE_EVENT && type != EV_SYN)
 		add_input_randomness(type, code, value);
 
@@ -612,7 +617,7 @@ int input_open_device(struct input_handle *handle)
 
 	handle->open++;
 
-	if (dev->users++) {
+	if (dev->users++ || dev->inhibited) {
 		/*
 		 * Device is already opened, so we can exit immediately and
 		 * report success.
@@ -660,6 +665,14 @@ int input_flush_device(struct input_handle *handle, struct file *file)
 }
 EXPORT_SYMBOL(input_flush_device);
 
+static inline void input_stop(struct input_dev *dev)
+{
+	if (dev->poller)
+		input_dev_poller_stop(dev->poller);
+	if (dev->close)
+		dev->close(dev);
+}
+
 /**
  * input_close_device - close input device
  * @handle: handle through which device is being accessed
@@ -675,13 +688,8 @@ void input_close_device(struct input_handle *handle)
 
 	__input_release_device(handle);
 
-	if (!--dev->users) {
-		if (dev->poller)
-			input_dev_poller_stop(dev->poller);
-
-		if (dev->close)
-			dev->close(dev);
-	}
+	if (!dev->inhibited && !--dev->users)
+		input_stop(dev);
 
 	if (!--handle->open) {
 		/*
@@ -1416,12 +1424,49 @@ static ssize_t input_dev_show_properties(struct device *dev,
 }
 static DEVICE_ATTR(properties, S_IRUGO, input_dev_show_properties, NULL);
 
+static int input_inhibit(struct input_dev *dev);
+static int input_uninhibit(struct input_dev *dev);
+
+static ssize_t inhibited_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct input_dev *input_dev = to_input_dev(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", input_dev->inhibited);
+}
+
+static ssize_t inhibited_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t len)
+{
+	struct input_dev *input_dev = to_input_dev(dev);
+	ssize_t rv;
+	bool inhibited;
+
+	if (strtobool(buf, &inhibited))
+		return -EINVAL;
+
+	if (inhibited)
+		rv = input_inhibit(input_dev);
+	else
+		rv = input_uninhibit(input_dev);
+
+	if (rv != 0)
+		return rv;
+
+	return len;
+}
+
+static DEVICE_ATTR_RW(inhibited);
+
 static struct attribute *input_dev_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_phys.attr,
 	&dev_attr_uniq.attr,
 	&dev_attr_modalias.attr,
 	&dev_attr_properties.attr,
+	&dev_attr_inhibited.attr,
 	NULL
 };
 
@@ -1703,6 +1748,77 @@ void input_reset_device(struct input_dev *dev)
 }
 EXPORT_SYMBOL(input_reset_device);
 
+static int input_inhibit(struct input_dev *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->mutex);
+
+	if (dev->inhibited)
+		goto out;
+
+	if (dev->users) {
+		if (dev->inhibit) {
+			ret = dev->inhibit(dev);
+			if (ret)
+				goto out;
+		}
+		input_stop(dev);
+	}
+
+	spin_lock_irq(&dev->event_lock);
+	input_dev_release_keys(dev);
+	input_dev_toggle(dev, false);
+	spin_unlock_irq(&dev->event_lock);
+
+	dev->inhibited = true;
+
+out:
+	mutex_unlock(&dev->mutex);
+	return ret;
+}
+
+static int input_uninhibit(struct input_dev *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->mutex);
+
+	if (!dev->inhibited)
+		goto out;
+
+	if (dev->users) {
+		if (dev->open) {
+			ret = dev->open(dev);
+			if (ret)
+				goto toggle;
+		}
+		if (dev->uninhibit) {
+			ret = dev->uninhibit(dev);
+			if (ret) {
+				if (dev->close)
+					dev->close(dev);
+				goto toggle;
+			}
+		}
+		if (dev->poller)
+			input_dev_poller_start(dev->poller);
+	}
+
+	dev->inhibited = false;
+
+toggle:
+	if (!dev->inhibited) {
+		spin_lock_irq(&dev->event_lock);
+		input_dev_toggle(dev, true);
+		spin_unlock_irq(&dev->event_lock);
+	}
+
+out:
+	mutex_unlock(&dev->mutex);
+	return ret;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int input_dev_suspend(struct device *dev)
 {
@@ -2131,7 +2247,7 @@ bool input_device_enabled(struct input_dev *dev)
 {
 	lockdep_assert_held(&dev->mutex);
 
-	return dev->users > 0;
+	return !dev->inhibited && dev->users > 0;
 }
 EXPORT_SYMBOL_GPL(input_device_enabled);
 
diff --git a/include/linux/input.h b/include/linux/input.h
index eda4587dba67..8d0dcfaeaf6f 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -127,6 +127,9 @@ enum input_clock_type {
  *	and needs not be explicitly unregistered or freed.
  * @timestamp: storage for a timestamp set by input_set_timestamp called
  *  by a driver
+ * @inhibit: makes the device ignore all input
+ * @uninhibit: undoes the effect of inhibit
+ * @inhibited: indicates that the input device is inhibited
  */
 struct input_dev {
 	const char *name;
@@ -201,6 +204,11 @@ struct input_dev {
 	bool devres_managed;
 
 	ktime_t timestamp[INPUT_CLK_MAX];
+
+	int (*inhibit)(struct input_dev *dev);
+	int (*uninhibit)(struct input_dev *dev);
+
+	bool inhibited;
 };
 #define to_input_dev(d) container_of(d, struct input_dev, dev)
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCHv2 3/7] ACPI: button: Use input_device_enabled() helper
  2020-05-15 16:49 ` [PATCHv2 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
  2020-05-15 16:51   ` [PATCHv2 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
  2020-05-15 16:52   ` [PATCHv2 6/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
@ 2020-05-15 16:53   ` Andrzej Pietrasiewicz
  2020-05-18 12:40     ` Rafael J. Wysocki
  2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
  3 siblings, 1 reply; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-15 16:53 UTC (permalink / raw)
  To: linux-input, linux-acpi
  Cc: Rafael J . Wysocki, Len Brown, Andrzej Pietrasiewicz, kernel

A new helper is available, so use it. Inspecting input device's 'users'
member should be done under device's mutex, so add appropriate invocations.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/acpi/button.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 78cfc70cb320..7e081bae48ab 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -411,7 +411,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 		input = button->input;
 		if (button->type == ACPI_BUTTON_TYPE_LID) {
 			mutex_lock(&button->input->mutex);
-			users = button->input->users;
+			users = input_device_enabled(button->input);
 			mutex_unlock(&button->input->mutex);
 			if (users)
 				acpi_lid_update_state(device, true);
@@ -456,13 +456,17 @@ static int acpi_button_resume(struct device *dev)
 {
 	struct acpi_device *device = to_acpi_device(dev);
 	struct acpi_button *button = acpi_driver_data(device);
+	struct input_dev *input = button->input;
 
 	button->suspended = false;
-	if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) {
+	mutex_lock(&input->mutex);
+	if (button->type == ACPI_BUTTON_TYPE_LID &&
+	    input_device_enabled(input)) {
 		button->last_state = !!acpi_lid_evaluate_state(device);
 		button->last_time = ktime_get();
 		acpi_lid_initialize_state(device);
 	}
+	mutex_unlock(&input->mutex);
 	return 0;
 }
 #endif
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-15 16:49 ` [PATCHv2 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
                     ` (2 preceding siblings ...)
  2020-05-15 16:53   ` [PATCHv2 3/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
@ 2020-05-15 18:19   ` Hans de Goede
  2020-05-17 22:55     ` Peter Hutterer
  2020-05-18 10:48     ` Andrzej Pietrasiewicz
  3 siblings, 2 replies; 18+ messages in thread
From: Hans de Goede @ 2020-05-15 18:19 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Andrezj,

On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
> Userspace might want to implement a policy to temporarily disregard input
> from certain devices, including not treating them as wakeup sources.
> 
> An example use case is a laptop, whose keyboard can be folded under the
> screen to create tablet-like experience. The user then must hold the laptop
> in such a way that it is difficult to avoid pressing the keyboard keys. It
> is therefore desirable to temporarily disregard input from the keyboard,
> until it is folded back. This obviously is a policy which should be kept
> out of the kernel, but the kernel must provide suitable means to implement
> such a policy.

Actually libinput already binds together (inside libinput) SW_TABLET_MODE
generating evdev nodes and e.g. internal keyboards on devices with 360°
hinges for this reason. libinput simply closes the /dev/input/event#
node when folded and re-opens it when the keyboard should become active
again. Thus not only suppresses events but allows e.g. touchpads to
enter runtime suspend mode which saves power. Typically closing the
/dev/input/event# node will also disable the device as wakeup source.

So I wonder what this series actually adds for functionality for
userspace which can not already be achieved this way?

I also noticed that you keep the device open (do not call the
input_device's close callback) when inhibited and just throw away
any events generated. This seems inefficient and may lead to
the internal state getting out of sync. What if a key is pressed
while inhibited and then the device is uninhibited while the key
is still pressed?  Now the press event is lost and userspace
querying the current state will see the pressed key as being
released.

On top of this you add special inhibit and uninhibit callbacks
and implement those for just a few devices. How do these differ
from just closing the device and later opening it again ?

Also using a sysfs property for this is very weird given that the
rest of the evdev interface is using ioctls for everything...

So all in all I see a lot of question marks here and I think we
need to have a detailed discussion about what use-cases this
series tries to enable before moving forward with this.

Regards,

Hans


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
@ 2020-05-17 22:55     ` Peter Hutterer
  2020-05-18  2:40       ` Dmitry Torokhov
  2020-05-18 10:48     ` Andrzej Pietrasiewicz
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Hutterer @ 2020-05-17 22:55 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
	Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
	Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
> Hi Andrezj,
> 
> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
> > Userspace might want to implement a policy to temporarily disregard input
> > from certain devices, including not treating them as wakeup sources.
> > 
> > An example use case is a laptop, whose keyboard can be folded under the
> > screen to create tablet-like experience. The user then must hold the laptop
> > in such a way that it is difficult to avoid pressing the keyboard keys. It
> > is therefore desirable to temporarily disregard input from the keyboard,
> > until it is folded back. This obviously is a policy which should be kept
> > out of the kernel, but the kernel must provide suitable means to implement
> > such a policy.
> 
> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
> generating evdev nodes and e.g. internal keyboards on devices with 360°
> hinges for this reason. libinput simply closes the /dev/input/event#
> node when folded and re-opens it when the keyboard should become active
> again. Thus not only suppresses events but allows e.g. touchpads to
> enter runtime suspend mode which saves power. Typically closing the
> /dev/input/event# node will also disable the device as wakeup source.
> 
> So I wonder what this series actually adds for functionality for
> userspace which can not already be achieved this way?

Thanks Hans. To expand on this:
libinput has heuristics to guess which input devices (keyboards, touchpads)
are built-in ones. When the tablet mode switch is on, we disable these
devices internally (this is not visible to callers), and re-enable it again
later when the tablet mode switch is off again.

This is done for keyboards and touchpads atm (and I think pointing sticks)
and where the heuristics fail we have extra quirks in place. For example
the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode
but buttons (e.g. volume keys) around the screen send events through the
same event node. So on those devices we don't disable the keyboard.

We've had this code for a few years now and the only changes to it have been
the various device quirks for devices that must not suspend the keyboard,
it's otherwise working as expected.

If we ever have a device where we need to disable parts of the keyboard
only, we could address this with EVIOCSMASK but so far that hasn't been
necessary.

I agree with Hans, right now I don't see the usefulness of this new sysfs
toggle. For it to be really useful you'd have to guarantee that it's
available for 100% of the devices and that's IMO unlikely to happen.

Cheers,
   Peter

> I also noticed that you keep the device open (do not call the
> input_device's close callback) when inhibited and just throw away
> any events generated. This seems inefficient and may lead to
> the internal state getting out of sync. What if a key is pressed
> while inhibited and then the device is uninhibited while the key
> is still pressed?  Now the press event is lost and userspace
> querying the current state will see the pressed key as being
> released.
> 
> On top of this you add special inhibit and uninhibit callbacks
> and implement those for just a few devices. How do these differ
> from just closing the device and later opening it again ?
> 
> Also using a sysfs property for this is very weird given that the
> rest of the evdev interface is using ioctls for everything...
> 
> So all in all I see a lot of question marks here and I think we
> need to have a detailed discussion about what use-cases this
> series tries to enable before moving forward with this.
> 
> Regards,
> 
> Hans
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-17 22:55     ` Peter Hutterer
@ 2020-05-18  2:40       ` Dmitry Torokhov
  2020-05-18  7:36         ` Hans de Goede
  2020-05-22 15:35         ` Andrzej Pietrasiewicz
  0 siblings, 2 replies; 18+ messages in thread
From: Dmitry Torokhov @ 2020-05-18  2:40 UTC (permalink / raw)
  To: Peter Hutterer
  Cc: Hans de Goede, Andrzej Pietrasiewicz, linux-input, linux-acpi,
	linux-iio, linux-arm-kernel, linux-samsung-soc, linux-tegra,
	patches, ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
	Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Vladimir Zapolskiy, Sylvain Lemieux,
	Laxman Dewangan, Thierry Reding, Jonathan Hunter, Barry Song,
	Michael Hennerich, Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Hans, Peter,

On Mon, May 18, 2020 at 08:55:10AM +1000, Peter Hutterer wrote:
> On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
> > Hi Andrezj,
> > 
> > On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
> > > Userspace might want to implement a policy to temporarily disregard input
> > > from certain devices, including not treating them as wakeup sources.
> > > 
> > > An example use case is a laptop, whose keyboard can be folded under the
> > > screen to create tablet-like experience. The user then must hold the laptop
> > > in such a way that it is difficult to avoid pressing the keyboard keys. It
> > > is therefore desirable to temporarily disregard input from the keyboard,
> > > until it is folded back. This obviously is a policy which should be kept
> > > out of the kernel, but the kernel must provide suitable means to implement
> > > such a policy.
> > 
> > Actually libinput already binds together (inside libinput) SW_TABLET_MODE
> > generating evdev nodes and e.g. internal keyboards on devices with 360°
> > hinges for this reason. libinput simply closes the /dev/input/event#
> > node when folded and re-opens it when the keyboard should become active
> > again. Thus not only suppresses events but allows e.g. touchpads to
> > enter runtime suspend mode which saves power. Typically closing the
> > /dev/input/event# node will also disable the device as wakeup source.
> > 
> > So I wonder what this series actually adds for functionality for
> > userspace which can not already be achieved this way?
> 
> Thanks Hans. To expand on this:
> libinput has heuristics to guess which input devices (keyboards, touchpads)
> are built-in ones. When the tablet mode switch is on, we disable these
> devices internally (this is not visible to callers), and re-enable it again
> later when the tablet mode switch is off again.

I think that is great that libinput has tried solving this for the
tablet mode, but unfortunately libinput only works for users of
libinput, leaving cases such as:

1. In-kernel input handlers, such as SysRq, VT and others
2. Systems that do not rely on libinput for userspace handing (Android,
Chrome OS)
3. Systems with policies that are more complex than tablet mode only.

Because of libinput's inability to affect the kernel, and the presence
of "always on" input handlers (sysrq, VT keyboard, potentially others),
while libinput may control whether consumers receive events from certain
input devices, it will not allow power savings that an explicit
"inhibit" allows when coming from dedicated power policy manager.

I think pushing policy decisions into a library, and trying to have all
clients agree with it, is much harder and leaks unnecessary knowledge
into quite a few layers. A dedicated power policy manager, that is not
only responsible for input device, but power state of the system as a
whole, is a very viable architecture.

> 
> This is done for keyboards and touchpads atm (and I think pointing sticks)
> and where the heuristics fail we have extra quirks in place. For example
> the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode
> but buttons (e.g. volume keys) around the screen send events through the
> same event node. So on those devices we don't disable the keyboard.
> 
> We've had this code for a few years now and the only changes to it have been
> the various device quirks for devices that must not suspend the keyboard,
> it's otherwise working as expected.
> 
> If we ever have a device where we need to disable parts of the keyboard
> only, we could address this with EVIOCSMASK but so far that hasn't been
> necessary.
> 
> I agree with Hans, right now I don't see the usefulness of this new sysfs
> toggle. For it to be really useful you'd have to guarantee that it's
> available for 100% of the devices and that's IMO unlikely to happen.

The inhibiting of the events works for 100% of input devices, the power
savings work for the ones that implement it. It is responsibility of
folds shipping the systems to make sure drivers they use support inhibit
if they believe it will help their battery life.

> 
> Cheers,
>    Peter
> 
> > I also noticed that you keep the device open (do not call the
> > input_device's close callback) when inhibited and just throw away
> > any events generated. This seems inefficient and may lead to
> > the internal state getting out of sync. What if a key is pressed
> > while inhibited and then the device is uninhibited while the key
> > is still pressed?  Now the press event is lost and userspace
> > querying the current state will see the pressed key as being
> > released.

This is a good point. We should look into signalling that some events
have been dropped (via EV_SYN/SYN_DROPPED) so that clients are aware of
it.

> > 
> > On top of this you add special inhibit and uninhibit callbacks
> > and implement those for just a few devices. How do these differ
> > from just closing the device and later opening it again ?

I believe majority will simply reuse open/close callbacks. In Chrome OS
we have dedicated inhibit/uninhibit, but I would like to allow using
open/close as alternatives.

> > 
> > Also using a sysfs property for this is very weird given that the
> > rest of the evdev interface is using ioctls for everything...

This is not evdev interface, it is at the level above evdev (so that it
can affect all handlers, not only evdev). As such it is not bound by
evdev interface.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-18  2:40       ` Dmitry Torokhov
@ 2020-05-18  7:36         ` Hans de Goede
  2020-05-22 15:35         ` Andrzej Pietrasiewicz
  1 sibling, 0 replies; 18+ messages in thread
From: Hans de Goede @ 2020-05-18  7:36 UTC (permalink / raw)
  To: Dmitry Torokhov, Peter Hutterer
  Cc: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
	Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Vladimir Zapolskiy, Sylvain Lemieux,
	Laxman Dewangan, Thierry Reding, Jonathan Hunter, Barry Song,
	Michael Hennerich, Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi,

On 5/18/20 4:40 AM, Dmitry Torokhov wrote:
> Hi Hans, Peter,
> 
> On Mon, May 18, 2020 at 08:55:10AM +1000, Peter Hutterer wrote:
>> On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
>>> Hi Andrezj,
>>>
>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>>> Userspace might want to implement a policy to temporarily disregard input
>>>> from certain devices, including not treating them as wakeup sources.
>>>>
>>>> An example use case is a laptop, whose keyboard can be folded under the
>>>> screen to create tablet-like experience. The user then must hold the laptop
>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>>> is therefore desirable to temporarily disregard input from the keyboard,
>>>> until it is folded back. This obviously is a policy which should be kept
>>>> out of the kernel, but the kernel must provide suitable means to implement
>>>> such a policy.
>>>
>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>>> hinges for this reason. libinput simply closes the /dev/input/event#
>>> node when folded and re-opens it when the keyboard should become active
>>> again. Thus not only suppresses events but allows e.g. touchpads to
>>> enter runtime suspend mode which saves power. Typically closing the
>>> /dev/input/event# node will also disable the device as wakeup source.
>>>
>>> So I wonder what this series actually adds for functionality for
>>> userspace which can not already be achieved this way?
>>
>> Thanks Hans. To expand on this:
>> libinput has heuristics to guess which input devices (keyboards, touchpads)
>> are built-in ones. When the tablet mode switch is on, we disable these
>> devices internally (this is not visible to callers), and re-enable it again
>> later when the tablet mode switch is off again.
> 
> I think that is great that libinput has tried solving this for the
> tablet mode, but unfortunately libinput only works for users of
> libinput, leaving cases such as:
> 
> 1. In-kernel input handlers, such as SysRq, VT and others
> 2. Systems that do not rely on libinput for userspace handing (Android,
> Chrome OS)
> 3. Systems with policies that are more complex than tablet mode only.
> 
> Because of libinput's inability to affect the kernel, and the presence
> of "always on" input handlers (sysrq, VT keyboard, potentially others),
> while libinput may control whether consumers receive events from certain
> input devices, it will not allow power savings that an explicit
> "inhibit" allows when coming from dedicated power policy manager.

Ok, the sysrq and vt keyboard handlers keeping the device open and thus
make it keep using power is a valid reason for a separate inhibit mechanism.

> I think pushing policy decisions into a library, and trying to have all
> clients agree with it, is much harder and leaks unnecessary knowledge
> into quite a few layers. A dedicated power policy manager, that is not
> only responsible for input device, but power state of the system as a
> whole, is a very viable architecture.

Well AFAIK the kernel-policy has always been to leave policy decisions
up to userspace as much as possible, but this just adds a mechanism to
implement the policy so that is fine.

>> This is done for keyboards and touchpads atm (and I think pointing sticks)
>> and where the heuristics fail we have extra quirks in place. For example
>> the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode
>> but buttons (e.g. volume keys) around the screen send events through the
>> same event node. So on those devices we don't disable the keyboard.
>>
>> We've had this code for a few years now and the only changes to it have been
>> the various device quirks for devices that must not suspend the keyboard,
>> it's otherwise working as expected.
>>
>> If we ever have a device where we need to disable parts of the keyboard
>> only, we could address this with EVIOCSMASK but so far that hasn't been
>> necessary.
>>
>> I agree with Hans, right now I don't see the usefulness of this new sysfs
>> toggle. For it to be really useful you'd have to guarantee that it's
>> available for 100% of the devices and that's IMO unlikely to happen.
> 
> The inhibiting of the events works for 100% of input devices, the power
> savings work for the ones that implement it. It is responsibility of
> folds shipping the systems to make sure drivers they use support inhibit
> if they believe it will help their battery life.
> 
>>
>> Cheers,
>>     Peter
>>
>>> I also noticed that you keep the device open (do not call the
>>> input_device's close callback) when inhibited and just throw away
>>> any events generated. This seems inefficient and may lead to
>>> the internal state getting out of sync. What if a key is pressed
>>> while inhibited and then the device is uninhibited while the key
>>> is still pressed?  Now the press event is lost and userspace
>>> querying the current state will see the pressed key as being
>>> released.
> 
> This is a good point. We should look into signalling that some events
> have been dropped (via EV_SYN/SYN_DROPPED) so that clients are aware of
> it.
> 
>>>
>>> On top of this you add special inhibit and uninhibit callbacks
>>> and implement those for just a few devices. How do these differ
>>> from just closing the device and later opening it again ?
> 
> I believe majority will simply reuse open/close callbacks. In Chrome OS
> we have dedicated inhibit/uninhibit, but I would like to allow using
> open/close as alternatives.

Ack, maybe some driver flag to just call close on inhibit and
open on unhibit (also taking input_device.users into account of course) ?

>>> Also using a sysfs property for this is very weird given that the
>>> rest of the evdev interface is using ioctls for everything...
> 
> This is not evdev interface, it is at the level above evdev (so that it
> can affect all handlers, not only evdev). As such it is not bound by
> evdev interface.

Ok I can see how on some systems the process implementing the policy
of when to inhibit would be separate from the process which has the
device open. But in e.g. the libinput case it would be good if
libinput could activate any potential power-savings by setting
the inhibit flag.

The problem with sysfs interfaces is that they typically require
root rights and that they are not really compatible with FD
passing. libinput runs as a normal user, getting a fd to the
/dev/input/event# node passed by systemd-logind.

As said I can see the reason for wanting a sysfs attribute for
this, can we perhaps have both a sysfs interface and an ioctl?

Note both could share the same boolean in the kernel, it would be
up to userspace to not try and write to both. E.g. chrome-os
would use the sysfs attr, libinput would use the ioctl.

Regards,

Hans


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
  2020-05-17 22:55     ` Peter Hutterer
@ 2020-05-18 10:48     ` Andrzej Pietrasiewicz
  2020-05-18 12:24       ` Hans de Goede
  1 sibling, 1 reply; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-18 10:48 UTC (permalink / raw)
  To: Hans de Goede, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Hans,

W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
> Hi Andrezj,
> 
> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>> Userspace might want to implement a policy to temporarily disregard input
>> from certain devices, including not treating them as wakeup sources.
>>
>> An example use case is a laptop, whose keyboard can be folded under the
>> screen to create tablet-like experience. The user then must hold the laptop
>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>> is therefore desirable to temporarily disregard input from the keyboard,
>> until it is folded back. This obviously is a policy which should be kept
>> out of the kernel, but the kernel must provide suitable means to implement
>> such a policy.
> 
> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
> generating evdev nodes and e.g. internal keyboards on devices with 360°
> hinges for this reason. libinput simply closes the /dev/input/event#
> node when folded and re-opens it when the keyboard should become active
> again. Thus not only suppresses events but allows e.g. touchpads to
> enter runtime suspend mode which saves power. Typically closing the
> /dev/input/event# node will also disable the device as wakeup source.
> 
> So I wonder what this series actually adds for functionality for
> userspace which can not already be achieved this way?
> 
> I also noticed that you keep the device open (do not call the
> input_device's close callback) when inhibited and just throw away

I'm not sure if I understand you correctly, it is called:

+static inline void input_stop(struct input_dev *dev)
+{
+	if (dev->poller)
+		input_dev_poller_stop(dev->poller);
+	if (dev->close)
+		dev->close(dev);
                 ^^^^^^^^^^^^^^^^
+static int input_inhibit(struct input_dev *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->mutex);
+
+	if (dev->inhibited)
+		goto out;
+
+	if (dev->users) {
+		if (dev->inhibit) {
+			ret = dev->inhibit(dev);
+			if (ret)
+				goto out;
+		}
+		input_stop(dev);
                 ^^^^^^^^^^^^^^^^

It will not be called when dev->users is zero, but if it is zero,
then nobody has opened the device yet so there is nothing to close.

Andrzej

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-18 10:48     ` Andrzej Pietrasiewicz
@ 2020-05-18 12:24       ` Hans de Goede
  2020-05-18 13:49         ` Andrzej Pietrasiewicz
  0 siblings, 1 reply; 18+ messages in thread
From: Hans de Goede @ 2020-05-18 12:24 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi,

On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans,
> 
> W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
>> Hi Andrezj,
>>
>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>> Userspace might want to implement a policy to temporarily disregard input
>>> from certain devices, including not treating them as wakeup sources.
>>>
>>> An example use case is a laptop, whose keyboard can be folded under the
>>> screen to create tablet-like experience. The user then must hold the laptop
>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>> is therefore desirable to temporarily disregard input from the keyboard,
>>> until it is folded back. This obviously is a policy which should be kept
>>> out of the kernel, but the kernel must provide suitable means to implement
>>> such a policy.
>>
>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>> hinges for this reason. libinput simply closes the /dev/input/event#
>> node when folded and re-opens it when the keyboard should become active
>> again. Thus not only suppresses events but allows e.g. touchpads to
>> enter runtime suspend mode which saves power. Typically closing the
>> /dev/input/event# node will also disable the device as wakeup source.
>>
>> So I wonder what this series actually adds for functionality for
>> userspace which can not already be achieved this way?
>>
>> I also noticed that you keep the device open (do not call the
>> input_device's close callback) when inhibited and just throw away
> 
> I'm not sure if I understand you correctly, it is called:
> 
> +static inline void input_stop(struct input_dev *dev)
> +{
> +    if (dev->poller)
> +        input_dev_poller_stop(dev->poller);
> +    if (dev->close)
> +        dev->close(dev);
>                  ^^^^^^^^^^^^^^^^
> +static int input_inhibit(struct input_dev *dev)
> +{
> +    int ret = 0;
> +
> +    mutex_lock(&dev->mutex);
> +
> +    if (dev->inhibited)
> +        goto out;
> +
> +    if (dev->users) {
> +        if (dev->inhibit) {
> +            ret = dev->inhibit(dev);
> +            if (ret)
> +                goto out;
> +        }
> +        input_stop(dev);
>                  ^^^^^^^^^^^^^^^^
> 
> It will not be called when dev->users is zero, but if it is zero,
> then nobody has opened the device yet so there is nothing to close.

Ah, I missed that.

So if the device implements the inhibit call back then on
inhibit it will get both the inhibit and close callback called?

And what happens if the last user goes away and the device
is not inhibited?

I'm trying to understand here what the difference between the 2
is / what the goal of having a separate inhibit callback ?

IOW is there something which we want to do on close when
the close is being done to inhibit the device, which we do
not want to do on a normal close ?

Regards,

Hans



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 3/7] ACPI: button: Use input_device_enabled() helper
  2020-05-15 16:53   ` [PATCHv2 3/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
@ 2020-05-18 12:40     ` Rafael J. Wysocki
  0 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2020-05-18 12:40 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz; +Cc: linux-input, linux-acpi, Len Brown, kernel

On Friday, May 15, 2020 6:53:27 PM CEST Andrzej Pietrasiewicz wrote:
> A new helper is available, so use it. Inspecting input device's 'users'
> member should be done under device's mutex, so add appropriate invocations.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/acpi/button.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 78cfc70cb320..7e081bae48ab 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -411,7 +411,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>  		input = button->input;
>  		if (button->type == ACPI_BUTTON_TYPE_LID) {
>  			mutex_lock(&button->input->mutex);
> -			users = button->input->users;
> +			users = input_device_enabled(button->input);
>  			mutex_unlock(&button->input->mutex);
>  			if (users)
>  				acpi_lid_update_state(device, true);
> @@ -456,13 +456,17 @@ static int acpi_button_resume(struct device *dev)
>  {
>  	struct acpi_device *device = to_acpi_device(dev);
>  	struct acpi_button *button = acpi_driver_data(device);
> +	struct input_dev *input = button->input;
>  
>  	button->suspended = false;
> -	if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) {
> +	mutex_lock(&input->mutex);
> +	if (button->type == ACPI_BUTTON_TYPE_LID &&
> +	    input_device_enabled(input)) {
>  		button->last_state = !!acpi_lid_evaluate_state(device);
>  		button->last_time = ktime_get();
>  		acpi_lid_initialize_state(device);
>  	}
> +	mutex_unlock(&input->mutex);
>  	return 0;
>  }
>  #endif
> 

This appears to conflate a fix (the introduction of the missing mutex locking)
with adding new functionality through input_device_enabled().

Please do the fix separately in advance so it can be backported.

Thanks!




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-18 12:24       ` Hans de Goede
@ 2020-05-18 13:49         ` Andrzej Pietrasiewicz
  2020-05-18 14:23           ` Hans de Goede
  0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-18 13:49 UTC (permalink / raw)
  To: Hans de Goede, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Hans,

W dniu 18.05.2020 o 14:24, Hans de Goede pisze:
> Hi,
> 
> On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote:
>> Hi Hans,
>>
>> W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
>>> Hi Andrezj,
>>>
>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>>> Userspace might want to implement a policy to temporarily disregard input
>>>> from certain devices, including not treating them as wakeup sources.
>>>>
>>>> An example use case is a laptop, whose keyboard can be folded under the
>>>> screen to create tablet-like experience. The user then must hold the laptop
>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>>> is therefore desirable to temporarily disregard input from the keyboard,
>>>> until it is folded back. This obviously is a policy which should be kept
>>>> out of the kernel, but the kernel must provide suitable means to implement
>>>> such a policy.
>>>
>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>>> hinges for this reason. libinput simply closes the /dev/input/event#
>>> node when folded and re-opens it when the keyboard should become active
>>> again. Thus not only suppresses events but allows e.g. touchpads to
>>> enter runtime suspend mode which saves power. Typically closing the
>>> /dev/input/event# node will also disable the device as wakeup source.
>>>
>>> So I wonder what this series actually adds for functionality for
>>> userspace which can not already be achieved this way?
>>>
>>> I also noticed that you keep the device open (do not call the
>>> input_device's close callback) when inhibited and just throw away
>>
>> I'm not sure if I understand you correctly, it is called:
>>
>> +static inline void input_stop(struct input_dev *dev)
>> +{
>> +    if (dev->poller)
>> +        input_dev_poller_stop(dev->poller);
>> +    if (dev->close)
>> +        dev->close(dev);
>>                  ^^^^^^^^^^^^^^^^
>> +static int input_inhibit(struct input_dev *dev)
>> +{
>> +    int ret = 0;
>> +
>> +    mutex_lock(&dev->mutex);
>> +
>> +    if (dev->inhibited)
>> +        goto out;
>> +
>> +    if (dev->users) {
>> +        if (dev->inhibit) {
>> +            ret = dev->inhibit(dev);
>> +            if (ret)
>> +                goto out;
>> +        }
>> +        input_stop(dev);
>>                  ^^^^^^^^^^^^^^^^
>>
>> It will not be called when dev->users is zero, but if it is zero,
>> then nobody has opened the device yet so there is nothing to close.
> 
> Ah, I missed that.
> 
> So if the device implements the inhibit call back then on
> inhibit it will get both the inhibit and close callback called?
> 

That's right. And conversely, upon uninhibit open() and uninhibit()
callbacks will be invoked. Please note that just as with open()/close(),
providing inhibit()/uninhibit() is optional.

> And what happens if the last user goes away and the device
> is not inhibited?

close() is called as usually.

> 
> I'm trying to understand here what the difference between the 2
> is / what the goal of having a separate inhibit callback ?
> 

Drivers have very different ideas about what it means to suspend/resume
and open/close. The optional inhibit/uninhibit callbacks are meant for
the drivers to know that it is this particular action going on.

For inhibit() there's one more argument: close() does not return a value,
so its meaning is "do some last cleanup" and as such it is not allowed
to fail - whatever its effect is, we must deem it successful. inhibit()
does return a value and so it is allowed to fail.

All in all, it is up to the drivers to decide which callback they
provide. Based on my work so far I would say that there are tens
of simple cases where open() and close() are sufficient, out of total
~400 users of input_allocate_device():

$ git grep "input_allocate_device(" | grep -v ^Documentation | \
cut -f1 -d: | sort | uniq | wc
     390     390   13496

Andrzej

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-18 13:49         ` Andrzej Pietrasiewicz
@ 2020-05-18 14:23           ` Hans de Goede
  2020-05-19  9:02             ` Andrzej Pietrasiewicz
  0 siblings, 1 reply; 18+ messages in thread
From: Hans de Goede @ 2020-05-18 14:23 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi,

On 5/18/20 3:49 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans,
> 
> W dniu 18.05.2020 o 14:24, Hans de Goede pisze:
>> Hi,
>>
>> On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote:
>>> Hi Hans,
>>>
>>> W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
>>>> Hi Andrezj,
>>>>
>>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>>>> Userspace might want to implement a policy to temporarily disregard input
>>>>> from certain devices, including not treating them as wakeup sources.
>>>>>
>>>>> An example use case is a laptop, whose keyboard can be folded under the
>>>>> screen to create tablet-like experience. The user then must hold the laptop
>>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>>>> is therefore desirable to temporarily disregard input from the keyboard,
>>>>> until it is folded back. This obviously is a policy which should be kept
>>>>> out of the kernel, but the kernel must provide suitable means to implement
>>>>> such a policy.
>>>>
>>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>>>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>>>> hinges for this reason. libinput simply closes the /dev/input/event#
>>>> node when folded and re-opens it when the keyboard should become active
>>>> again. Thus not only suppresses events but allows e.g. touchpads to
>>>> enter runtime suspend mode which saves power. Typically closing the
>>>> /dev/input/event# node will also disable the device as wakeup source.
>>>>
>>>> So I wonder what this series actually adds for functionality for
>>>> userspace which can not already be achieved this way?
>>>>
>>>> I also noticed that you keep the device open (do not call the
>>>> input_device's close callback) when inhibited and just throw away
>>>
>>> I'm not sure if I understand you correctly, it is called:
>>>
>>> +static inline void input_stop(struct input_dev *dev)
>>> +{
>>> +    if (dev->poller)
>>> +        input_dev_poller_stop(dev->poller);
>>> +    if (dev->close)
>>> +        dev->close(dev);
>>>                  ^^^^^^^^^^^^^^^^
>>> +static int input_inhibit(struct input_dev *dev)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    mutex_lock(&dev->mutex);
>>> +
>>> +    if (dev->inhibited)
>>> +        goto out;
>>> +
>>> +    if (dev->users) {
>>> +        if (dev->inhibit) {
>>> +            ret = dev->inhibit(dev);
>>> +            if (ret)
>>> +                goto out;
>>> +        }
>>> +        input_stop(dev);
>>>                  ^^^^^^^^^^^^^^^^
>>>
>>> It will not be called when dev->users is zero, but if it is zero,
>>> then nobody has opened the device yet so there is nothing to close.
>>
>> Ah, I missed that.
>>
>> So if the device implements the inhibit call back then on
>> inhibit it will get both the inhibit and close callback called?
>>
> 
> That's right. And conversely, upon uninhibit open() and uninhibit()
> callbacks will be invoked. Please note that just as with open()/close(),
> providing inhibit()/uninhibit() is optional.

Ack.

>> And what happens if the last user goes away and the device
>> is not inhibited?
> 
> close() is called as usually.

But not inhibit, hmm, see below.

>> I'm trying to understand here what the difference between the 2
>> is / what the goal of having a separate inhibit callback ?
>>
> 
> Drivers have very different ideas about what it means to suspend/resume
> and open/close. The optional inhibit/uninhibit callbacks are meant for
> the drivers to know that it is this particular action going on.

So the inhibit() callback triggers the "suspend" behavior ?
But shouldn't drivers which are capable of suspending the device
always do so on close() ?

Since your current proposal also calls close() on inhibit() I
really see little difference between an inhibit() and the last
user of the device closing it and IMHO unless there is a good
reason to actually differentiate the 2 it would be better
to only stick with the existing close() and in cases where
that does not put the device in a low-power mode yet, fix
the existing close() callback to do the low-power mode
setting instead of adding a new callback.

> For inhibit() there's one more argument: close() does not return a value,
> so its meaning is "do some last cleanup" and as such it is not allowed
> to fail - whatever its effect is, we must deem it successful. inhibit()
> does return a value and so it is allowed to fail.

Well, we could make close() return an error and at least in the inhibit()
case propagate that to userspace. I wonder if userspace is going to
do anything useful with that error though...

In my experience errors during cleanup/shutdown are best logged
(using dev_err) and otherwise ignored, so that we try to clean up
as much possible. Unless the very first step of the shutdown process
fails the device is going to be in some twilight zone state anyways
at this point we might as well try to cleanup as much as possible.

> All in all, it is up to the drivers to decide which callback they
> provide. Based on my work so far I would say that there are tens
> of simple cases where open() and close() are sufficient, out of total
> ~400 users of input_allocate_device():
> 
> $ git grep "input_allocate_device(" | grep -v ^Documentation | \
> cut -f1 -d: | sort | uniq | wc
>      390     390   13496

So can you explain a bit more about the cases where only having
open/close is not sufficient?  So far I have the feeling that
those are all we need and that we really do not need separate
[un]inhibit callbacks.

Regards,

Hans


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-18 14:23           ` Hans de Goede
@ 2020-05-19  9:02             ` Andrzej Pietrasiewicz
  2020-05-19  9:36               ` Hans de Goede
  0 siblings, 1 reply; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-19  9:02 UTC (permalink / raw)
  To: Hans de Goede, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Hans, Hi Dmitry,

W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
> Hi,

<snip>

>>>>>
>>>>> So I wonder what this series actually adds for functionality for
>>>>> userspace which can not already be achieved this way?
>>>>>
>>>>> I also noticed that you keep the device open (do not call the
>>>>> input_device's close callback) when inhibited and just throw away
>>>>
>>>> I'm not sure if I understand you correctly, it is called:
>>>>
>>>> +static inline void input_stop(struct input_dev *dev)
>>>> +{
>>>> +    if (dev->poller)
>>>> +        input_dev_poller_stop(dev->poller);
>>>> +    if (dev->close)
>>>> +        dev->close(dev);
>>>>                  ^^^^^^^^^^^^^^^^
>>>> +static int input_inhibit(struct input_dev *dev)
>>>> +{
>>>> +    int ret = 0;
>>>> +
>>>> +    mutex_lock(&dev->mutex);
>>>> +
>>>> +    if (dev->inhibited)
>>>> +        goto out;
>>>> +
>>>> +    if (dev->users) {
>>>> +        if (dev->inhibit) {
>>>> +            ret = dev->inhibit(dev);
>>>> +            if (ret)
>>>> +                goto out;
>>>> +        }
>>>> +        input_stop(dev);
>>>>                  ^^^^^^^^^^^^^^^^
>>>>
>>>> It will not be called when dev->users is zero, but if it is zero,
>>>> then nobody has opened the device yet so there is nothing to close.
>>>
>>> Ah, I missed that.
>>>
>>> So if the device implements the inhibit call back then on
>>> inhibit it will get both the inhibit and close callback called?
>>>
>>
>> That's right. And conversely, upon uninhibit open() and uninhibit()
>> callbacks will be invoked. Please note that just as with open()/close(),
>> providing inhibit()/uninhibit() is optional.
> 
> Ack.
> 
>>> And what happens if the last user goes away and the device
>>> is not inhibited?
>>
>> close() is called as usually.
> 
> But not inhibit, hmm, see below.
> 
>>> I'm trying to understand here what the difference between the 2
>>> is / what the goal of having a separate inhibit callback ?
>>>
>>
>> Drivers have very different ideas about what it means to suspend/resume
>> and open/close. The optional inhibit/uninhibit callbacks are meant for
>> the drivers to know that it is this particular action going on.
> 
> So the inhibit() callback triggers the "suspend" behavior ?
> But shouldn't drivers which are capable of suspending the device
> always do so on close() ?
> 
> Since your current proposal also calls close() on inhibit() I
> really see little difference between an inhibit() and the last
> user of the device closing it and IMHO unless there is a good
> reason to actually differentiate the 2 it would be better
> to only stick with the existing close() and in cases where
> that does not put the device in a low-power mode yet, fix
> the existing close() callback to do the low-power mode
> setting instead of adding a new callback.
> 
>> For inhibit() there's one more argument: close() does not return a value,
>> so its meaning is "do some last cleanup" and as such it is not allowed
>> to fail - whatever its effect is, we must deem it successful. inhibit()
>> does return a value and so it is allowed to fail.
> 
> Well, we could make close() return an error and at least in the inhibit()
> case propagate that to userspace. I wonder if userspace is going to
> do anything useful with that error though...
> 
> In my experience errors during cleanup/shutdown are best logged
> (using dev_err) and otherwise ignored, so that we try to clean up
> as much possible. Unless the very first step of the shutdown process
> fails the device is going to be in some twilight zone state anyways
> at this point we might as well try to cleanup as much as possible.

What you say makes sense to me.
@Dmitry?

> 
>> All in all, it is up to the drivers to decide which callback they
>> provide. Based on my work so far I would say that there are tens
>> of simple cases where open() and close() are sufficient, out of total
>> ~400 users of input_allocate_device():
>>
>> $ git grep "input_allocate_device(" | grep -v ^Documentation | \
>> cut -f1 -d: | sort | uniq | wc
>>      390     390   13496
> 
> So can you explain a bit more about the cases where only having
> open/close is not sufficient?  So far I have the feeling that
> those are all we need and that we really do not need separate
> [un]inhibit callbacks.

My primary concern was not being able to propagate inhibit() error
to userspace, and then if we have inhibit(), uninhibit() should be
there for completeness. If propagating the error to userspace can
be neglected then yes, it seems open/close should be sufficient,
even more because the real meaning of "open" is "prepare the device
for generating input events".

To validate the idea of not introducing inhibit()/uninhibit() callbacks
to implement device inhibiting/uninhibiting let's look at
drivers/input/mouse/elan_i2c_core.c (PATCH 7/7):

static int elan_inhibit(struct input_dev *input)
{
[...]

	ret = mutex_lock_interruptible(&data->sysfs_mutex);
	if (ret)
		return ret;

	disable_irq(client->irq);

	ret = elan_disable_power(data);
	if (ret)
		enable_irq(client->irq);
[...]
}

First, close() does not exist in this driver. Of course this can be
fixed. Then it doesn't return a value. Then, if either taking the
mutex or disabling the power fails, the close() is still deemed
successful. Is it ok?
@Dmitry?

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-19  9:02             ` Andrzej Pietrasiewicz
@ 2020-05-19  9:36               ` Hans de Goede
  2020-05-27  6:34                 ` Dmitry Torokhov
  0 siblings, 1 reply; 18+ messages in thread
From: Hans de Goede @ 2020-05-19  9:36 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86
  Cc: Rafael J . Wysocki, Len Brown, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Kukjin Kim,
	Krzysztof Kozlowski, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Vladimir Zapolskiy, Sylvain Lemieux, Laxman Dewangan,
	Thierry Reding, Jonathan Hunter, Barry Song, Michael Hennerich,
	Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi,

On 5/19/20 11:02 AM, Andrzej Pietrasiewicz wrote:
> Hi Hans, Hi Dmitry,
> 
> W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
>> Hi,
> 
> <snip>
> 
>>>>>>
>>>>>> So I wonder what this series actually adds for functionality for
>>>>>> userspace which can not already be achieved this way?
>>>>>>
>>>>>> I also noticed that you keep the device open (do not call the
>>>>>> input_device's close callback) when inhibited and just throw away
>>>>>
>>>>> I'm not sure if I understand you correctly, it is called:
>>>>>
>>>>> +static inline void input_stop(struct input_dev *dev)
>>>>> +{
>>>>> +    if (dev->poller)
>>>>> +        input_dev_poller_stop(dev->poller);
>>>>> +    if (dev->close)
>>>>> +        dev->close(dev);
>>>>>                  ^^^^^^^^^^^^^^^^
>>>>> +static int input_inhibit(struct input_dev *dev)
>>>>> +{
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    mutex_lock(&dev->mutex);
>>>>> +
>>>>> +    if (dev->inhibited)
>>>>> +        goto out;
>>>>> +
>>>>> +    if (dev->users) {
>>>>> +        if (dev->inhibit) {
>>>>> +            ret = dev->inhibit(dev);
>>>>> +            if (ret)
>>>>> +                goto out;
>>>>> +        }
>>>>> +        input_stop(dev);
>>>>>                  ^^^^^^^^^^^^^^^^
>>>>>
>>>>> It will not be called when dev->users is zero, but if it is zero,
>>>>> then nobody has opened the device yet so there is nothing to close.
>>>>
>>>> Ah, I missed that.
>>>>
>>>> So if the device implements the inhibit call back then on
>>>> inhibit it will get both the inhibit and close callback called?
>>>>
>>>
>>> That's right. And conversely, upon uninhibit open() and uninhibit()
>>> callbacks will be invoked. Please note that just as with open()/close(),
>>> providing inhibit()/uninhibit() is optional.
>>
>> Ack.
>>
>>>> And what happens if the last user goes away and the device
>>>> is not inhibited?
>>>
>>> close() is called as usually.
>>
>> But not inhibit, hmm, see below.
>>
>>>> I'm trying to understand here what the difference between the 2
>>>> is / what the goal of having a separate inhibit callback ?
>>>>
>>>
>>> Drivers have very different ideas about what it means to suspend/resume
>>> and open/close. The optional inhibit/uninhibit callbacks are meant for
>>> the drivers to know that it is this particular action going on.
>>
>> So the inhibit() callback triggers the "suspend" behavior ?
>> But shouldn't drivers which are capable of suspending the device
>> always do so on close() ?
>>
>> Since your current proposal also calls close() on inhibit() I
>> really see little difference between an inhibit() and the last
>> user of the device closing it and IMHO unless there is a good
>> reason to actually differentiate the 2 it would be better
>> to only stick with the existing close() and in cases where
>> that does not put the device in a low-power mode yet, fix
>> the existing close() callback to do the low-power mode
>> setting instead of adding a new callback.
>>
>>> For inhibit() there's one more argument: close() does not return a value,
>>> so its meaning is "do some last cleanup" and as such it is not allowed
>>> to fail - whatever its effect is, we must deem it successful. inhibit()
>>> does return a value and so it is allowed to fail.
>>
>> Well, we could make close() return an error and at least in the inhibit()
>> case propagate that to userspace. I wonder if userspace is going to
>> do anything useful with that error though...
>>
>> In my experience errors during cleanup/shutdown are best logged
>> (using dev_err) and otherwise ignored, so that we try to clean up
>> as much possible. Unless the very first step of the shutdown process
>> fails the device is going to be in some twilight zone state anyways
>> at this point we might as well try to cleanup as much as possible.
> 
> What you say makes sense to me.
> @Dmitry?
> 
>>
>>> All in all, it is up to the drivers to decide which callback they
>>> provide. Based on my work so far I would say that there are tens
>>> of simple cases where open() and close() are sufficient, out of total
>>> ~400 users of input_allocate_device():
>>>
>>> $ git grep "input_allocate_device(" | grep -v ^Documentation | \
>>> cut -f1 -d: | sort | uniq | wc
>>>      390     390   13496
>>
>> So can you explain a bit more about the cases where only having
>> open/close is not sufficient?  So far I have the feeling that
>> those are all we need and that we really do not need separate
>> [un]inhibit callbacks.
> 
> My primary concern was not being able to propagate inhibit() error
> to userspace, and then if we have inhibit(), uninhibit() should be
> there for completeness. If propagating the error to userspace can
> be neglected then yes, it seems open/close should be sufficient,
> even more because the real meaning of "open" is "prepare the device
> for generating input events".
> 
> To validate the idea of not introducing inhibit()/uninhibit() callbacks
> to implement device inhibiting/uninhibiting let's look at
> drivers/input/mouse/elan_i2c_core.c (PATCH 7/7):
> 
> static int elan_inhibit(struct input_dev *input)
> {
> [...]
> 
>      ret = mutex_lock_interruptible(&data->sysfs_mutex);
>      if (ret)
>          return ret;
> 
>      disable_irq(client->irq);
> 
>      ret = elan_disable_power(data);
>      if (ret)
>          enable_irq(client->irq);
> [...]
> }
> 
> First, close() does not exist in this driver. Of course this can be
> fixed. Then it doesn't return a value. Then, if either taking the
> mutex or disabling the power fails, the close() is still deemed
> successful. Is it ok?

Note I also mentioned another solution for the error propagation,
which would require a big "flag day" commit adding "return 0"
to all existing close callbacks, but otherwise should work for your
purposes:

 > Well, we could make close() return an error and at least in the inhibit()
 > case propagate that to userspace. I wonder if userspace is going to
 > do anything useful with that error though...

And I guess we could log an error that close failed in the old close() path
where we cannot propagate the error.

Also why the mutex_lock_interruptible() ?  If you change that to
a normal mutex_lock() you loose one of the possible 2 error cases and
I doubt anyone is going to do a CTRL-C of the process doing the
inhibiting (or that that process starts a timer using a signal
to ensure the inhibit does not take to long or some such).

Regards,

Hans


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-18  2:40       ` Dmitry Torokhov
  2020-05-18  7:36         ` Hans de Goede
@ 2020-05-22 15:35         ` Andrzej Pietrasiewicz
  2020-05-27  6:13           ` Peter Hutterer
  1 sibling, 1 reply; 18+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-22 15:35 UTC (permalink / raw)
  To: Dmitry Torokhov, Peter Hutterer
  Cc: Hans de Goede, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
	Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Vladimir Zapolskiy, Sylvain Lemieux,
	Laxman Dewangan, Thierry Reding, Jonathan Hunter, Barry Song,
	Michael Hennerich, Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Hans, hi Dmitry,

W dniu 18.05.2020 o 04:40, Dmitry Torokhov pisze:
> Hi Hans, Peter,
> 
> On Mon, May 18, 2020 at 08:55:10AM +1000, Peter Hutterer wrote:
>> On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
>>> Hi Andrezj,
>>>

<snip>

>>
>>> I also noticed that you keep the device open (do not call the
>>> input_device's close callback) when inhibited and just throw away
>>> any events generated. This seems inefficient and may lead to
>>> the internal state getting out of sync. What if a key is pressed
>>> while inhibited and then the device is uninhibited while the key
>>> is still pressed?  Now the press event is lost and userspace
>>> querying the current state will see the pressed key as being
>>> released.
> 
> This is a good point. We should look into signalling that some events
> have been dropped (via EV_SYN/SYN_DROPPED) so that clients are aware of
> it.
> 

It seems to me that the situation Hans envisions is not possible,
or will not be possible with a simple change. Let me explain.

For a start, let's recall that the input core prevents consecutive
events of the same kind (type _and_ code _and_ value) from being
delivered to handlers. The decision is made in input_get_disposition().
For EV_KEY it is:

		if (is_event_supported(code, dev->keybit, KEY_MAX)) {

			/* auto-repeat bypasses state updates */
			if (value == 2) {
				disposition = INPUT_PASS_TO_HANDLERS;
				break;
			}

			if (!!test_bit(code, dev->key) != !!value) {

				__change_bit(code, dev->key);
				disposition = INPUT_PASS_TO_HANDLERS;
			}
		}

Let's now focus on value != 2 (events other than auto-repeat).
The disposition changes from the default INPUT_IGNORE_EVENT to
INPUT_PASS_TO_HANDLERS only when the event in question changes
the current state: either by releasing a pressed key, or by
pressing a released key. Subsequent releases of a released key
or subsequent presses of a pressed key will be ignored.

What Hans points out is the possibility of uninhibiting a device
while its key is pressed and then releasing the key. First of all,
during inhibiting input_dev_release_keys() is called, so input_dev's
internal state will be cleared of all pressed keys. Then the device
- after being uninhibited - all of a sudden produces a key release
event. It will be ignored as per the "subsequent releases of a
released key" case, so the handlers will not be passed an unmatched
key release event. Assuming that passing an unmatched key release
event was Hans's concern, in this case it seems impossible.

Now, the value of 2 (auto-repeat) needs some attention. There are two
cases to consider: the device uses input core's software repeat or it
uses its own (hardware) repeat.

Let's consider the first case. The timer which generates auto-repeat
is only started on a key press event and only stopped on a key release
event. As such, if any auto-repeat was in progress when inhibiting
happened, it must have been stopped as per input_dev_release_keys().
Then the key is pressed and held after the device has been inhibited,
and the device is being uninhibited. Since it uses software auto-repeat,
no events will be reported by the device until the key is released,
and, as explained above, the release event will be ignored.

Let's consider the second case. The key is pressed and held after the
device has been inhibited and the device is being uninhibited. The worst
thing that can happen is unmatched key repeat events will start coming
from the device. We must prevent them from reaching the handlers and
ignore them instead. So I suggest something on the lines of:

if (is_event_supported(code, dev->keybit, KEY_MAX)) {

			/* auto-repeat bypasses state updates */
-			if (value == 2) {
+			if (value == 2 && test_bit(code, dev->key)) {
				disposition = INPUT_PASS_TO_HANDLERS;
				break;
			}

The intended meaning is "ignore key repeat events if the key is not
pressed".

With this small change I believe it is not possible to have neither
unmatched release nor unmatched repeat being delivered to handlers.

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-22 15:35         ` Andrzej Pietrasiewicz
@ 2020-05-27  6:13           ` Peter Hutterer
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Hutterer @ 2020-05-27  6:13 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz
  Cc: Dmitry Torokhov, Hans de Goede, linux-input, linux-acpi,
	linux-iio, linux-arm-kernel, linux-samsung-soc, linux-tegra,
	patches, ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
	Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Vladimir Zapolskiy, Sylvain Lemieux,
	Laxman Dewangan, Thierry Reding, Jonathan Hunter, Barry Song,
	Michael Hennerich, Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

Hi Andrzej,

On Fri, May 22, 2020 at 05:35:56PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Hans, hi Dmitry,
> 
> W dniu 18.05.2020 o 04:40, Dmitry Torokhov pisze:
> > Hi Hans, Peter,
> > 
> > On Mon, May 18, 2020 at 08:55:10AM +1000, Peter Hutterer wrote:
> > > On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
> > > > Hi Andrezj,
> > > > 
> 
> <snip>
> 
> > > 
> > > > I also noticed that you keep the device open (do not call the
> > > > input_device's close callback) when inhibited and just throw away
> > > > any events generated. This seems inefficient and may lead to
> > > > the internal state getting out of sync. What if a key is pressed
> > > > while inhibited and then the device is uninhibited while the key
> > > > is still pressed?  Now the press event is lost and userspace
> > > > querying the current state will see the pressed key as being
> > > > released.
> > 
> > This is a good point. We should look into signalling that some events
> > have been dropped (via EV_SYN/SYN_DROPPED) so that clients are aware of
> > it.
> > 
> 
> It seems to me that the situation Hans envisions is not possible,
> or will not be possible with a simple change. Let me explain.
> 
> For a start, let's recall that the input core prevents consecutive
> events of the same kind (type _and_ code _and_ value) from being
> delivered to handlers. The decision is made in input_get_disposition().
> For EV_KEY it is:
> 
> 		if (is_event_supported(code, dev->keybit, KEY_MAX)) {
> 
> 			/* auto-repeat bypasses state updates */
> 			if (value == 2) {
> 				disposition = INPUT_PASS_TO_HANDLERS;
> 				break;
> 			}
> 
> 			if (!!test_bit(code, dev->key) != !!value) {
> 
> 				__change_bit(code, dev->key);
> 				disposition = INPUT_PASS_TO_HANDLERS;
> 			}
> 		}

note that this isn't per-process state, userspace can get release events
after open() for keys it never got the press event for. Simple test:
type evtest<enter> and KEY_ENTER up is the first event you'll get.

But otherwise I agree with you that press/release should always be balanced
if input_dev_release_keys() is called on inhibit and with that autorepeat
snippet below. At least I couldn't come up with any combination of multiple
clients opening/closing/inhibiting that resulted in an unwanted release
event after uninhibit.

Cheers,
   Peter

> Let's now focus on value != 2 (events other than auto-repeat).
> The disposition changes from the default INPUT_IGNORE_EVENT to
> INPUT_PASS_TO_HANDLERS only when the event in question changes
> the current state: either by releasing a pressed key, or by
> pressing a released key. Subsequent releases of a released key
> or subsequent presses of a pressed key will be ignored.
>
> What Hans points out is the possibility of uninhibiting a device
> while its key is pressed and then releasing the key. First of all,
> during inhibiting input_dev_release_keys() is called, so input_dev's
> internal state will be cleared of all pressed keys. Then the device
> - after being uninhibited - all of a sudden produces a key release
> event. It will be ignored as per the "subsequent releases of a
> released key" case, so the handlers will not be passed an unmatched
> key release event. Assuming that passing an unmatched key release
> event was Hans's concern, in this case it seems impossible.
> 
> Now, the value of 2 (auto-repeat) needs some attention. There are two
> cases to consider: the device uses input core's software repeat or it
> uses its own (hardware) repeat.
> 
> Let's consider the first case. The timer which generates auto-repeat
> is only started on a key press event and only stopped on a key release
> event. As such, if any auto-repeat was in progress when inhibiting
> happened, it must have been stopped as per input_dev_release_keys().
> Then the key is pressed and held after the device has been inhibited,
> and the device is being uninhibited. Since it uses software auto-repeat,
> no events will be reported by the device until the key is released,
> and, as explained above, the release event will be ignored.
> 
> Let's consider the second case. The key is pressed and held after the
> device has been inhibited and the device is being uninhibited. The worst
> thing that can happen is unmatched key repeat events will start coming
> from the device. We must prevent them from reaching the handlers and
> ignore them instead. So I suggest something on the lines of:
> 
> if (is_event_supported(code, dev->keybit, KEY_MAX)) {
> 
> 			/* auto-repeat bypasses state updates */
> -			if (value == 2) {
> +			if (value == 2 && test_bit(code, dev->key)) {
> 				disposition = INPUT_PASS_TO_HANDLERS;
> 				break;
> 			}
> 
> The intended meaning is "ignore key repeat events if the key is not
> pressed".
> 
> With this small change I believe it is not possible to have neither
> unmatched release nor unmatched repeat being delivered to handlers.
> 
> Regards,
> 
> Andrzej

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 0/7] Support inhibiting input devices
  2020-05-19  9:36               ` Hans de Goede
@ 2020-05-27  6:34                 ` Dmitry Torokhov
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry Torokhov @ 2020-05-27  6:34 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Andrzej Pietrasiewicz, linux-input, linux-acpi, linux-iio,
	linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
	ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
	Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Vladimir Zapolskiy, Sylvain Lemieux,
	Laxman Dewangan, Thierry Reding, Jonathan Hunter, Barry Song,
	Michael Hennerich, Nick Dyer, Ferruh Yigit, Sangwon Jee,
	Henrique de Moraes Holschuh, kernel, Peter Hutterer,
	Benjamin Tissoires

On Tue, May 19, 2020 at 11:36:34AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 5/19/20 11:02 AM, Andrzej Pietrasiewicz wrote:
> > Hi Hans, Hi Dmitry,
> > 
> > W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
> > > Hi,
> > 
> > <snip>
> > 
> > > > > > > 
> > > > > > > So I wonder what this series actually adds for functionality for
> > > > > > > userspace which can not already be achieved this way?
> > > > > > > 
> > > > > > > I also noticed that you keep the device open (do not call the
> > > > > > > input_device's close callback) when inhibited and just throw away
> > > > > > 
> > > > > > I'm not sure if I understand you correctly, it is called:
> > > > > > 
> > > > > > +static inline void input_stop(struct input_dev *dev)
> > > > > > +{
> > > > > > +    if (dev->poller)
> > > > > > +        input_dev_poller_stop(dev->poller);
> > > > > > +    if (dev->close)
> > > > > > +        dev->close(dev);
> > > > > >                  ^^^^^^^^^^^^^^^^
> > > > > > +static int input_inhibit(struct input_dev *dev)
> > > > > > +{
> > > > > > +    int ret = 0;
> > > > > > +
> > > > > > +    mutex_lock(&dev->mutex);
> > > > > > +
> > > > > > +    if (dev->inhibited)
> > > > > > +        goto out;
> > > > > > +
> > > > > > +    if (dev->users) {
> > > > > > +        if (dev->inhibit) {
> > > > > > +            ret = dev->inhibit(dev);
> > > > > > +            if (ret)
> > > > > > +                goto out;
> > > > > > +        }
> > > > > > +        input_stop(dev);
> > > > > >                  ^^^^^^^^^^^^^^^^
> > > > > > 
> > > > > > It will not be called when dev->users is zero, but if it is zero,
> > > > > > then nobody has opened the device yet so there is nothing to close.
> > > > > 
> > > > > Ah, I missed that.
> > > > > 
> > > > > So if the device implements the inhibit call back then on
> > > > > inhibit it will get both the inhibit and close callback called?
> > > > > 
> > > > 
> > > > That's right. And conversely, upon uninhibit open() and uninhibit()
> > > > callbacks will be invoked. Please note that just as with open()/close(),
> > > > providing inhibit()/uninhibit() is optional.
> > > 
> > > Ack.
> > > 
> > > > > And what happens if the last user goes away and the device
> > > > > is not inhibited?
> > > > 
> > > > close() is called as usually.
> > > 
> > > But not inhibit, hmm, see below.
> > > 
> > > > > I'm trying to understand here what the difference between the 2
> > > > > is / what the goal of having a separate inhibit callback ?
> > > > > 
> > > > 
> > > > Drivers have very different ideas about what it means to suspend/resume
> > > > and open/close. The optional inhibit/uninhibit callbacks are meant for
> > > > the drivers to know that it is this particular action going on.
> > > 
> > > So the inhibit() callback triggers the "suspend" behavior ?
> > > But shouldn't drivers which are capable of suspending the device
> > > always do so on close() ?
> > > 
> > > Since your current proposal also calls close() on inhibit() I
> > > really see little difference between an inhibit() and the last
> > > user of the device closing it and IMHO unless there is a good
> > > reason to actually differentiate the 2 it would be better
> > > to only stick with the existing close() and in cases where
> > > that does not put the device in a low-power mode yet, fix
> > > the existing close() callback to do the low-power mode
> > > setting instead of adding a new callback.
> > > 
> > > > For inhibit() there's one more argument: close() does not return a value,
> > > > so its meaning is "do some last cleanup" and as such it is not allowed
> > > > to fail - whatever its effect is, we must deem it successful. inhibit()
> > > > does return a value and so it is allowed to fail.
> > > 
> > > Well, we could make close() return an error and at least in the inhibit()
> > > case propagate that to userspace. I wonder if userspace is going to
> > > do anything useful with that error though...

It really can't do anything. Have you ever seen userspace handling
errors from close()? And what can be done? A program is terminating, but
the kernel says "no, you closing input device failed, you have to
continue running indefinitely..."

> > > 
> > > In my experience errors during cleanup/shutdown are best logged
> > > (using dev_err) and otherwise ignored, so that we try to clean up
> > > as much possible. Unless the very first step of the shutdown process
> > > fails the device is going to be in some twilight zone state anyways
> > > at this point we might as well try to cleanup as much as possible.
> > 
> > What you say makes sense to me.
> > @Dmitry?

I will note here, that inhibit is closer to suspend() than to close(),
and we do report errors for suspend(). Therefore we could conceivably
try to handle errors if driver really wants to be fancy. But I think
majority of cases will be quite happy with using close() and simply
logging errors, as Hans said.

That said, I think the way we should handle inhibit/uninhibit, is that
if we have the callback defined, then we call it, and only call open and
close if uninhibit or inhibit are _not_ defined.

> > 
> > > 
> > > > All in all, it is up to the drivers to decide which callback they
> > > > provide. Based on my work so far I would say that there are tens
> > > > of simple cases where open() and close() are sufficient, out of total
> > > > ~400 users of input_allocate_device():
> > > > 
> > > > $ git grep "input_allocate_device(" | grep -v ^Documentation | \
> > > > cut -f1 -d: | sort | uniq | wc
> > > >      390     390   13496
> > > 
> > > So can you explain a bit more about the cases where only having
> > > open/close is not sufficient?  So far I have the feeling that
> > > those are all we need and that we really do not need separate
> > > [un]inhibit callbacks.
> > 
> > My primary concern was not being able to propagate inhibit() error
> > to userspace, and then if we have inhibit(), uninhibit() should be
> > there for completeness. If propagating the error to userspace can
> > be neglected then yes, it seems open/close should be sufficient,
> > even more because the real meaning of "open" is "prepare the device
> > for generating input events".
> > 
> > To validate the idea of not introducing inhibit()/uninhibit() callbacks
> > to implement device inhibiting/uninhibiting let's look at
> > drivers/input/mouse/elan_i2c_core.c (PATCH 7/7):
> > 
> > static int elan_inhibit(struct input_dev *input)
> > {
> > [...]
> > 
> >      ret = mutex_lock_interruptible(&data->sysfs_mutex);
> >      if (ret)
> >          return ret;
> > 
> >      disable_irq(client->irq);
> > 
> >      ret = elan_disable_power(data);
> >      if (ret)
> >          enable_irq(client->irq);
> > [...]
> > }
> > 
> > First, close() does not exist in this driver. Of course this can be
> > fixed. Then it doesn't return a value. Then, if either taking the
> > mutex or disabling the power fails, the close() is still deemed
> > successful. Is it ok?
> 
> Note I also mentioned another solution for the error propagation,
> which would require a big "flag day" commit adding "return 0"
> to all existing close callbacks, but otherwise should work for your
> purposes:

No, please, no flag days and no changing close() to return error, it
makes no sense for close().

> 
> > Well, we could make close() return an error and at least in the inhibit()
> > case propagate that to userspace. I wonder if userspace is going to
> > do anything useful with that error though...
> 
> And I guess we could log an error that close failed in the old close() path
> where we cannot propagate the error.
> 
> Also why the mutex_lock_interruptible() ?  If you change that to
> a normal mutex_lock() you loose one of the possible 2 error cases and
> I doubt anyone is going to do a CTRL-C of the process doing the
> inhibiting (or that that process starts a timer using a signal
> to ensure the inhibit does not take to long or some such).

Well, we have the dedicated callbacks in Chrome OS, so when I did the
patch I could even handle Ctrl-C, so why not? But it indeed can easily
be dropped in favor of straight mutex_lock().

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200506002746.GB89269@dtor-ws>
2020-05-15 16:49 ` [PATCHv2 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
2020-05-15 16:51   ` [PATCHv2 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-05-15 16:52   ` [PATCHv2 6/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-05-15 16:53   ` [PATCHv2 3/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
2020-05-18 12:40     ` Rafael J. Wysocki
2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
2020-05-17 22:55     ` Peter Hutterer
2020-05-18  2:40       ` Dmitry Torokhov
2020-05-18  7:36         ` Hans de Goede
2020-05-22 15:35         ` Andrzej Pietrasiewicz
2020-05-27  6:13           ` Peter Hutterer
2020-05-18 10:48     ` Andrzej Pietrasiewicz
2020-05-18 12:24       ` Hans de Goede
2020-05-18 13:49         ` Andrzej Pietrasiewicz
2020-05-18 14:23           ` Hans de Goede
2020-05-19  9:02             ` Andrzej Pietrasiewicz
2020-05-19  9:36               ` Hans de Goede
2020-05-27  6:34                 ` Dmitry Torokhov

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git