* [PATCH] input: Add support for Wacom protocol 4 serial tablets
From: Hans de Goede @ 2014-07-19 16:38 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Julian Squires, linux-input
Hi All,
Recent version of xf86-input-wacom no longer support directly accessing
serial tablets. Instead xf86-input-wacom now expects all wacom tablets to
be driven by the kernel and to show up as evdev devices.
This has caused old serial Wacom tablets to stop working for people who still
have such tablets. Julian Squires has written a serio input driver to fix this:
https://github.com/tokenrove/wacom-serial-iv
About a year ago Julian submitted this upstream, back then the response by
Dmitry was:
> The driver looks quite reasonable, however that would be the 4th Wacom
> driver in the tree... I wonder if it is time to split out transport
> parts from core wacom functionality so that it can be shared by USB, I2C
> and serio devices.
I've taken a quick look at this, the i2c driver is for touchscreens and in
the protocol has nothing in common with the other wacom drivers. The w8001
driver is for serial touchscreens and again a completely different protocol.
The wacom usb driver already deals with what appears to be a lot of different
protocol nuances, see for example wacom_wac.c, and non of all the different
package formats parsed in wacom_wac.c seem to have anything in common with
the wacom protocol 4 spoken over serial. So it seems that there is nothing
to be shared since these are really significant different devices which happen
to be from the same vendor. Moreover the usb wacom bits are being reworked
to use hid instead of to more or less parse usb hid reports themselves, and
I certainly don't want this driver to get in the way of that.
So all in all I believe it is best to keep this as a standalone driver.
Hence I'm hereby re-submitting this driver for upstream inclusion.
This is a significantly cleaned up version of the driver initially submitted
by Julian, with improved Graphire support (I own an old Graphire myself).
Regards,
Hans
^ permalink raw reply
* [PATCH] input: Add support for Wacom protocol 4 serial tablets
From: Hans de Goede @ 2014-07-19 16:38 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Julian Squires, linux-input, Hans de Goede
In-Reply-To: <1405787920-3354-1-git-send-email-hdegoede@redhat.com>
Recent version of xf86-input-wacom no longer support directly accessing
serial tablets. Instead xf86-input-wacom now expects all wacom tablets to
be driven by the kernel and to show up as evdev devices.
This has caused old serial Wacom tablets to stop working for people who still
have such tablets. Julian Squires has written a serio input driver to fix this:
https://github.com/tokenrove/wacom-serial-iv
This is a cleaned up version of this driver with improved Graphire support
(I own an old Graphire myself).
Signed-off-by: Julian Squires <julian@cipht.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
MAINTAINERS | 7 +
drivers/input/tablet/Kconfig | 12 +
drivers/input/tablet/Makefile | 1 +
drivers/input/tablet/wacom_serial4.c | 600 +++++++++++++++++++++++++++++++++++
include/uapi/linux/serio.h | 1 +
5 files changed, 621 insertions(+)
create mode 100644 drivers/input/tablet/wacom_serial4.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1629ac9..1128b57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9855,6 +9855,13 @@ M: Pierre Ossman <pierre@ossman.eu>
S: Maintained
F: drivers/mmc/host/wbsd.*
+WACOM PROTOCOL 4 SERIAL TABLETS
+M: Julian Squires <julian@cipht.net>
+M: Hans de Goede <hdegoede@redhat.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: drivers/input/tablet/wacom_serial4.c
+
WATCHDOG DEVICE DRIVERS
M: Wim Van Sebroeck <wim@iguana.be>
L: linux-watchdog@vger.kernel.org
diff --git a/drivers/input/tablet/Kconfig b/drivers/input/tablet/Kconfig
index bed7cbf..d7c764a 100644
--- a/drivers/input/tablet/Kconfig
+++ b/drivers/input/tablet/Kconfig
@@ -89,4 +89,16 @@ config TABLET_USB_WACOM
To compile this driver as a module, choose M here: the
module will be called wacom.
+config TABLET_SERIAL_WACOM4
+ tristate "Wacom protocol 4 serial tablet support"
+ select SERIO
+ help
+ Say Y here if you want to use Wacom protocol 4 serial tablets.
+ E.g. serial versions of the Cintiq, Graphire or Penpartner.
+ Make sure to say Y to "Mouse support" (CONFIG_INPUT_MOUSEDEV) and/or
+ "Event interface support" (CONFIG_INPUT_EVDEV) as well.
+
+ To compile this driver as a module, choose M here: the
+ module will be called wacom_serial4.
+
endif
diff --git a/drivers/input/tablet/Makefile b/drivers/input/tablet/Makefile
index 3f6c252..4d9339f 100644
--- a/drivers/input/tablet/Makefile
+++ b/drivers/input/tablet/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_TABLET_USB_GTCO) += gtco.o
obj-$(CONFIG_TABLET_USB_HANWANG) += hanwang.o
obj-$(CONFIG_TABLET_USB_KBTAB) += kbtab.o
obj-$(CONFIG_TABLET_USB_WACOM) += wacom.o
+obj-$(CONFIG_TABLET_SERIAL_WACOM4) += wacom_serial4.o
diff --git a/drivers/input/tablet/wacom_serial4.c b/drivers/input/tablet/wacom_serial4.c
new file mode 100644
index 0000000..587e47c
--- /dev/null
+++ b/drivers/input/tablet/wacom_serial4.c
@@ -0,0 +1,600 @@
+/*
+ * Wacom protocol 4 serial tablet driver
+ *
+ * Copyright 2014 Hans de Goede <hdegoede@redhat.com>
+ * Copyright 2011-2012 Julian Squires <julian@cipht.net>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version of 2 of the License, or (at your
+ * option) any later version. See the file COPYING in the main directory of
+ * this archive for more details.
+ *
+ * Many thanks to Bill Seremetis, without whom PenPartner support
+ * would not have been possible. Thanks to Patrick Mahoney.
+ *
+ * This driver was developed with reference to much code written by others,
+ * particularly:
+ * - elo, gunze drivers by Vojtech Pavlik <vojtech@ucw.cz>;
+ * - wacom_w8001 driver by Jaya Kumar <jayakumar.lkml@gmail.com>;
+ * - the USB wacom input driver, credited to many people
+ * (see drivers/input/tablet/wacom.h);
+ * - new and old versions of linuxwacom / xf86-input-wacom credited to
+ * Frederic Lepied, France. <Lepied@XFree86.org> and
+ * Ping Cheng, Wacom. <pingc@wacom.com>;
+ * - and xf86wacom.c (a presumably ancient version of the linuxwacom code),
+ * by Frederic Lepied and Raph Levien <raph@gtk.org>.
+ *
+ * To do:
+ * - support pad buttons; (requires access to a model with pad buttons)
+ * - support (protocol 4-style) tilt (requires access to a > 1.4 rom model)
+ */
+
+/*
+ * Wacom serial protocol 4 documentation taken from linuxwacom-0.9.9 code,
+ * protocol 4 uses 7 or 9 byte of data in the following format:
+ *
+ * Byte 1
+ * bit 7 Sync bit always 1
+ * bit 6 Pointing device detected
+ * bit 5 Cursor = 0 / Stylus = 1
+ * bit 4 Reserved
+ * bit 3 1 if a button on the pointing device has been pressed
+ * bit 2 P0 (optional)
+ * bit 1 X15
+ * bit 0 X14
+ *
+ * Byte 2
+ * bit 7 Always 0
+ * bits 6-0 = X13 - X7
+ *
+ * Byte 3
+ * bit 7 Always 0
+ * bits 6-0 = X6 - X0
+ *
+ * Byte 4
+ * bit 7 Always 0
+ * bit 6 B3
+ * bit 5 B2
+ * bit 4 B1
+ * bit 3 B0
+ * bit 2 P1 (optional)
+ * bit 1 Y15
+ * bit 0 Y14
+ *
+ * Byte 5
+ * bit 7 Always 0
+ * bits 6-0 = Y13 - Y7
+ *
+ * Byte 6
+ * bit 7 Always 0
+ * bits 6-0 = Y6 - Y0
+ *
+ * Byte 7
+ * bit 7 Always 0
+ * bit 6 Sign of pressure data; or wheel-rel for cursor tool
+ * bit 5 P7; or REL1 for cursor tool
+ * bit 4 P6; or REL0 for cursor tool
+ * bit 3 P5
+ * bit 2 P4
+ * bit 1 P3
+ * bit 0 P2
+ *
+ * byte 8 and 9 are optional and present only
+ * in tilt mode.
+ *
+ * Byte 8
+ * bit 7 Always 0
+ * bit 6 Sign of tilt X
+ * bit 5 Xt6
+ * bit 4 Xt5
+ * bit 3 Xt4
+ * bit 2 Xt3
+ * bit 1 Xt2
+ * bit 0 Xt1
+ *
+ * Byte 9
+ * bit 7 Always 0
+ * bit 6 Sign of tilt Y
+ * bit 5 Yt6
+ * bit 4 Yt5
+ * bit 3 Yt4
+ * bit 2 Yt3
+ * bit 1 Yt2
+ * bit 0 Yt1
+ *
+ * For more info see:
+ * http://sourceforge.net/apps/mediawiki/linuxwacom/index.php?title=Serial_Protocol_IV
+ */
+
+#include <linux/completion.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/serio.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include "wacom_wac.h"
+
+MODULE_AUTHOR("Julian Squires <julian@cipht.net>, Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("Wacom protocol 4 serial tablet driver");
+MODULE_LICENSE("GPL");
+
+#define REQUEST_MODEL_AND_ROM_VERSION "~#"
+#define REQUEST_MAX_COORDINATES "~C\r"
+#define REQUEST_CONFIGURATION_STRING "~R\r"
+#define REQUEST_RESET_TO_PROTOCOL_IV "\r#"
+/* Note: sending "\r$\r" causes at least the Digitizer II to send
+ * packets in ASCII instead of binary. "\r#" seems to undo that. */
+
+#define COMMAND_START_SENDING_PACKETS "ST\r"
+#define COMMAND_STOP_SENDING_PACKETS "SP\r"
+#define COMMAND_MULTI_MODE_INPUT "MU1\r"
+#define COMMAND_ORIGIN_IN_UPPER_LEFT "OC1\r"
+#define COMMAND_ENABLE_ALL_MACRO_BUTTONS "~M0\r"
+#define COMMAND_DISABLE_GROUP_1_MACRO_BUTTONS "~M1\r"
+#define COMMAND_TRANSMIT_AT_MAX_RATE "IT0\r"
+#define COMMAND_DISABLE_INCREMENTAL_MODE "IN0\r"
+#define COMMAND_ENABLE_CONTINUOUS_MODE "SR\r"
+#define COMMAND_ENABLE_PRESSURE_MODE "PH1\r"
+#define COMMAND_Z_FILTER "ZF1\r"
+
+/* Note that this is a protocol 4 packet without tilt information. */
+#define PACKET_LENGTH 7
+#define DATA_SIZE 32
+
+/* flags */
+#define F_COVERS_SCREEN 0x01
+#define F_HAS_STYLUS2 0x02
+#define F_HAS_SCROLLWHEEL 0x04
+
+enum { STYLUS = 1, ERASER, CURSOR };
+struct { int device_id; int input_id; } tools[] = {
+ { 0, 0 },
+ { STYLUS_DEVICE_ID, BTN_TOOL_PEN },
+ { ERASER_DEVICE_ID, BTN_TOOL_RUBBER },
+ { CURSOR_DEVICE_ID, BTN_TOOL_MOUSE },
+};
+
+struct wacom {
+ struct input_dev *dev;
+ struct completion cmd_done;
+ int expect;
+ int result;
+ int extra_z_bits;
+ int eraser_mask;
+ int flags;
+ int res_x, res_y;
+ int max_x, max_y;
+ int tool;
+ int idx;
+ unsigned char data[DATA_SIZE];
+ char phys[32];
+};
+
+enum {
+ MODEL_CINTIQ = 0x504C, /* PL */
+ MODEL_CINTIQ2 = 0x4454, /* DT */
+ MODEL_DIGITIZER_II = 0x5544, /* UD */
+ MODEL_GRAPHIRE = 0x4554, /* ET */
+ MODEL_PENPARTNER = 0x4354, /* CT */
+};
+
+static void handle_model_response(struct wacom *wacom)
+{
+ int major_v, minor_v, r = 0;
+ char *p;
+
+ p = strrchr(wacom->data, 'V');
+ if (p)
+ r = sscanf(p + 1, "%u.%u", &major_v, &minor_v);
+ if (r != 2)
+ major_v = minor_v = 0;
+
+ switch (wacom->data[2] << 8 | wacom->data[3]) {
+ case MODEL_CINTIQ: /* UNTESTED */
+ case MODEL_CINTIQ2:
+ if ((wacom->data[2] << 8 | wacom->data[3]) == MODEL_CINTIQ) {
+ wacom->dev->name = "Wacom Cintiq";
+ wacom->dev->id.version = MODEL_CINTIQ;
+ } else {
+ wacom->dev->name = "Wacom Cintiq II";
+ wacom->dev->id.version = MODEL_CINTIQ2;
+ }
+ wacom->res_x = 508;
+ wacom->res_y = 508;
+ switch (wacom->data[5]<<8 | wacom->data[6]) {
+ case 0x3731: /* PL-710 */
+ wacom->res_x = 2540;
+ wacom->res_y = 2540;
+ /* fall through */
+ case 0x3535: /* PL-550 */
+ case 0x3830: /* PL-800 */
+ wacom->extra_z_bits = 2;
+ }
+ wacom->flags = F_COVERS_SCREEN;
+ break;
+ case MODEL_PENPARTNER:
+ wacom->dev->name = "Wacom Penpartner";
+ wacom->dev->id.version = MODEL_PENPARTNER;
+ wacom->res_x = 1000;
+ wacom->res_y = 1000;
+ break;
+ case MODEL_GRAPHIRE:
+ wacom->dev->name = "Wacom Graphire";
+ wacom->dev->id.version = MODEL_GRAPHIRE;
+ wacom->res_x = 1016;
+ wacom->res_y = 1016;
+ wacom->max_x = 5103;
+ wacom->max_y = 3711;
+ wacom->extra_z_bits = 2;
+ wacom->eraser_mask = 0x08;
+ wacom->flags = F_HAS_STYLUS2 | F_HAS_SCROLLWHEEL;
+ break;
+ case MODEL_DIGITIZER_II:
+ wacom->dev->name = "Wacom Digitizer II";
+ wacom->dev->id.version = MODEL_DIGITIZER_II;
+ if (major_v == 1 && minor_v <= 2)
+ wacom->extra_z_bits = 0; /* UNTESTED */
+ break;
+ default:
+ dev_err(&wacom->dev->dev, "Unsupported Wacom model %s\n",
+ wacom->data);
+ wacom->result = -ENODEV;
+ return;
+ }
+ dev_info(&wacom->dev->dev, "%s tablet, version %u.%u\n",
+ wacom->dev->name, major_v, minor_v);
+}
+
+static void handle_configuration_response(struct wacom *wacom)
+{
+ int r, skip;
+
+ dev_dbg(&wacom->dev->dev, "Configuration string: %s\n", wacom->data);
+ r = sscanf(wacom->data, "~R%x,%u,%u,%u,%u", &skip, &skip, &skip,
+ &wacom->res_x, &wacom->res_y);
+ if (r != 5)
+ dev_warn(&wacom->dev->dev, "could not get resolution\n");
+}
+
+static void handle_coordinates_response(struct wacom *wacom)
+{
+ int r;
+
+ dev_dbg(&wacom->dev->dev, "Coordinates string: %s\n", wacom->data);
+ r = sscanf(wacom->data, "~C%u,%u", &wacom->max_x, &wacom->max_y);
+ if (r != 2)
+ dev_warn(&wacom->dev->dev, "could not get max coordinates\n");
+}
+
+static void handle_response(struct wacom *wacom)
+{
+ if (wacom->data[0] != '~' || wacom->data[1] != wacom->expect) {
+ dev_err(&wacom->dev->dev,
+ "Wacom got an unexpected response: %s\n", wacom->data);
+ wacom->result = -EIO;
+ complete(&wacom->cmd_done);
+ return;
+ }
+
+ wacom->result = 0;
+
+ switch (wacom->data[1]) {
+ case '#':
+ handle_model_response(wacom);
+ break;
+ case 'R':
+ handle_configuration_response(wacom);
+ break;
+ case 'C':
+ handle_coordinates_response(wacom);
+ break;
+ }
+
+ complete(&wacom->cmd_done);
+}
+
+static void handle_packet(struct wacom *wacom)
+{
+ int in_proximity_p, stylus_p, button, x, y, z;
+ int tool;
+
+ in_proximity_p = wacom->data[0] & 0x40;
+ stylus_p = wacom->data[0] & 0x20;
+ button = (wacom->data[3] & 0x78) >> 3;
+ x = (wacom->data[0] & 3) << 14 | wacom->data[1]<<7 | wacom->data[2];
+ y = (wacom->data[3] & 3) << 14 | wacom->data[4]<<7 | wacom->data[5];
+
+ if (in_proximity_p && stylus_p) {
+ z = wacom->data[6] & 0x7f;
+ if (wacom->extra_z_bits >= 1)
+ z = z << 1 | (wacom->data[3] & 0x4) >> 2;
+ if (wacom->extra_z_bits > 1)
+ z = z << 1 | (wacom->data[0] & 0x4) >> 2;
+ z = z ^ (0x40 << wacom->extra_z_bits);
+ } else {
+ z = -1;
+ }
+
+ if (stylus_p)
+ tool = (button & wacom->eraser_mask) ? ERASER : STYLUS;
+ else
+ tool = CURSOR;
+
+ if (tool != wacom->tool && wacom->tool != 0) {
+ input_report_key(wacom->dev, tools[wacom->tool].input_id, 0);
+ input_sync(wacom->dev);
+ }
+ wacom->tool = tool;
+
+ input_report_key(wacom->dev, tools[tool].input_id, in_proximity_p);
+ input_report_abs(wacom->dev, ABS_MISC,
+ in_proximity_p ? tools[tool].device_id : 0);
+ input_report_abs(wacom->dev, ABS_X, x);
+ input_report_abs(wacom->dev, ABS_Y, y);
+ input_report_abs(wacom->dev, ABS_PRESSURE, z);
+ if (stylus_p) {
+ input_report_key(wacom->dev, BTN_TOUCH, button & 1);
+ input_report_key(wacom->dev, BTN_STYLUS, button & 2);
+ input_report_key(wacom->dev, BTN_STYLUS2, button & 4);
+ } else {
+ input_report_key(wacom->dev, BTN_LEFT, button & 1);
+ input_report_key(wacom->dev, BTN_RIGHT, button & 2);
+ input_report_key(wacom->dev, BTN_MIDDLE, button & 4);
+ /* handle relative wheel for non-stylus device */
+ z = (wacom->data[6] & 0x30) >> 4;
+ if (wacom->data[6] & 0x40)
+ z = -z;
+ input_report_rel(wacom->dev, REL_WHEEL, z);
+ }
+ input_sync(wacom->dev);
+}
+
+static void wacom_clear_data_buf(struct wacom *wacom)
+{
+ memset(wacom->data, 0, DATA_SIZE);
+ wacom->idx = 0;
+}
+
+static irqreturn_t wacom_interrupt(struct serio *serio, unsigned char data,
+ unsigned int flags)
+{
+ struct wacom *wacom = serio_get_drvdata(serio);
+
+ if (data & 0x80)
+ wacom->idx = 0;
+
+ /*
+ * We're either expecting a carriage return-terminated ASCII
+ * response string, or a seven-byte packet with the MSB set on
+ * the first byte.
+ *
+ * Note however that some tablets (the PenPartner, for
+ * example) don't send a carriage return at the end of a
+ * command. We handle these by waiting for timeout.
+ */
+ if (data == '\r' && !(wacom->data[0] & 0x80)) {
+ handle_response(wacom);
+ wacom_clear_data_buf(wacom);
+ return IRQ_HANDLED;
+ }
+
+ /* Leave place for 0 termination */
+ if (wacom->idx > (DATA_SIZE - 2)) {
+ dev_dbg(&wacom->dev->dev,
+ "throwing away %d bytes of garbage\n", wacom->idx);
+ wacom_clear_data_buf(wacom);
+ }
+ wacom->data[wacom->idx++] = data;
+
+ if (wacom->idx == PACKET_LENGTH && (wacom->data[0] & 0x80)) {
+ handle_packet(wacom);
+ wacom_clear_data_buf(wacom);
+ }
+ return IRQ_HANDLED;
+}
+
+static void wacom_disconnect(struct serio *serio)
+{
+ struct wacom *wacom = serio_get_drvdata(serio);
+
+ serio_close(serio);
+ serio_set_drvdata(serio, NULL);
+ input_unregister_device(wacom->dev);
+ kfree(wacom);
+}
+
+static int wacom_send(struct serio *serio, const char *command)
+{
+ int err = 0;
+
+ for (; !err && *command; command++)
+ err = serio_write(serio, *command);
+
+ return err;
+}
+
+static int send_setup_string(struct wacom *wacom, struct serio *serio)
+{
+ const char *s;
+
+ switch (wacom->dev->id.version) {
+ case MODEL_CINTIQ: /* UNTESTED */
+ s = COMMAND_ORIGIN_IN_UPPER_LEFT
+ COMMAND_TRANSMIT_AT_MAX_RATE
+ COMMAND_ENABLE_CONTINUOUS_MODE
+ COMMAND_START_SENDING_PACKETS;
+ break;
+ case MODEL_PENPARTNER:
+ s = COMMAND_ENABLE_PRESSURE_MODE
+ COMMAND_START_SENDING_PACKETS;
+ break;
+ default:
+ s = COMMAND_MULTI_MODE_INPUT
+ COMMAND_ORIGIN_IN_UPPER_LEFT
+ COMMAND_ENABLE_ALL_MACRO_BUTTONS
+ COMMAND_DISABLE_GROUP_1_MACRO_BUTTONS
+ COMMAND_TRANSMIT_AT_MAX_RATE
+ COMMAND_DISABLE_INCREMENTAL_MODE
+ COMMAND_ENABLE_CONTINUOUS_MODE
+ COMMAND_Z_FILTER
+ COMMAND_START_SENDING_PACKETS;
+ break;
+ }
+ return wacom_send(serio, s);
+}
+
+static int wacom_send_and_wait(struct wacom *wacom, struct serio *serio,
+ const char *cmd, const char *desc)
+{
+ int err;
+ unsigned long u;
+
+ wacom->expect = cmd[1];
+ init_completion(&wacom->cmd_done);
+ err = wacom_send(serio, cmd);
+ if (err)
+ return err;
+ u = wait_for_completion_timeout(&wacom->cmd_done, HZ);
+ if (u == 0) {
+ /* Timeout, process what we've received. */
+ handle_response(wacom);
+ }
+ wacom->expect = 0;
+ return wacom->result;
+}
+
+static int wacom_setup(struct wacom *wacom, struct serio *serio)
+{
+ int err;
+
+ /* Note that setting the link speed is the job of inputattach.
+ * We assume that reset negotiation has already happened,
+ * here. */
+ err = wacom_send_and_wait(wacom, serio, REQUEST_MODEL_AND_ROM_VERSION,
+ "model and version");
+ if (err)
+ return err;
+
+ if (!(wacom->res_x && wacom->res_y)) {
+ err = wacom_send_and_wait(wacom, serio,
+ REQUEST_CONFIGURATION_STRING,
+ "configuration string");
+ if (err)
+ return err;
+ }
+
+ if (!(wacom->max_x && wacom->max_y)) {
+ err = wacom_send_and_wait(wacom, serio,
+ REQUEST_MAX_COORDINATES,
+ "coordinates string");
+ if (err)
+ return err;
+ }
+
+ return send_setup_string(wacom, serio);
+}
+
+static int wacom_connect(struct serio *serio, struct serio_driver *drv)
+{
+ struct wacom *wacom;
+ struct input_dev *input_dev;
+ int err = -ENOMEM;
+
+ wacom = kzalloc(sizeof(struct wacom), GFP_KERNEL);
+ input_dev = input_allocate_device();
+ if (!wacom || !input_dev)
+ goto free_device;
+
+ wacom->dev = input_dev;
+ wacom->extra_z_bits = 1;
+ wacom->eraser_mask = 0x04;
+ wacom->tool = wacom->idx = 0;
+ snprintf(wacom->phys, sizeof(wacom->phys), "%s/input0", serio->phys);
+ input_dev->phys = wacom->phys;
+ input_dev->id.bustype = BUS_RS232;
+ input_dev->id.vendor = SERIO_WACOM_IV;
+ input_dev->id.product = serio->id.extra;
+ input_dev->dev.parent = &serio->dev;
+
+ input_dev->evbit[0] =
+ BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS) | BIT_MASK(EV_REL);
+ set_bit(ABS_MISC, input_dev->absbit);
+ set_bit(BTN_TOOL_PEN, input_dev->keybit);
+ set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
+ set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
+ set_bit(BTN_TOUCH, input_dev->keybit);
+ set_bit(BTN_STYLUS, input_dev->keybit);
+ set_bit(BTN_LEFT, input_dev->keybit);
+ set_bit(BTN_RIGHT, input_dev->keybit);
+ set_bit(BTN_MIDDLE, input_dev->keybit);
+
+ serio_set_drvdata(serio, wacom);
+
+ err = serio_open(serio, drv);
+ if (err)
+ goto free_device;
+
+ err = wacom_setup(wacom, serio);
+ if (err)
+ goto close_serio;
+
+ set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+ if (!(wacom->flags & F_COVERS_SCREEN))
+ set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
+ if (wacom->flags & F_HAS_STYLUS2)
+ set_bit(BTN_STYLUS2, input_dev->keybit);
+
+ if (wacom->flags & F_HAS_SCROLLWHEEL)
+ set_bit(REL_WHEEL, input_dev->relbit);
+
+ input_abs_set_res(wacom->dev, ABS_X, wacom->res_x);
+ input_abs_set_res(wacom->dev, ABS_Y, wacom->res_y);
+ input_set_abs_params(wacom->dev, ABS_X, 0, wacom->max_x, 0, 0);
+ input_set_abs_params(wacom->dev, ABS_Y, 0, wacom->max_y, 0, 0);
+ input_set_abs_params(wacom->dev, ABS_PRESSURE, -1,
+ (1 << (7 + wacom->extra_z_bits)) - 1, 0, 0);
+
+ err = input_register_device(wacom->dev);
+ if (err)
+ goto close_serio;
+
+ return 0;
+
+close_serio:
+ serio_close(serio);
+free_device:
+ serio_set_drvdata(serio, NULL);
+ input_free_device(input_dev);
+ kfree(wacom);
+ return err;
+}
+
+static struct serio_device_id wacom_serio_ids[] = {
+ {
+ .type = SERIO_RS232,
+ .proto = SERIO_WACOM_IV,
+ .id = SERIO_ANY,
+ .extra = SERIO_ANY,
+ },
+ { 0 }
+};
+
+MODULE_DEVICE_TABLE(serio, wacom_serio_ids);
+
+static struct serio_driver wacom_drv = {
+ .driver = {
+ .name = "wacom_serial4",
+ },
+ .description = "Wacom protocol 4 serial tablet driver",
+ .id_table = wacom_serio_ids,
+ .interrupt = wacom_interrupt,
+ .connect = wacom_connect,
+ .disconnect = wacom_disconnect,
+};
+
+module_serio_driver(wacom_drv);
diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
index 9f53fa7..becdd78 100644
--- a/include/uapi/linux/serio.h
+++ b/include/uapi/linux/serio.h
@@ -76,5 +76,6 @@
#define SERIO_HAMPSHIRE 0x3b
#define SERIO_PS2MULT 0x3c
#define SERIO_TSC40 0x3d
+#define SERIO_WACOM_IV 0x3e
#endif /* _UAPI_SERIO_H */
--
2.0.1
^ permalink raw reply related
* Re: Power-managing devices that are not of interest at some point in time
From: Benson Leung @ 2014-07-19 15:23 UTC (permalink / raw)
To: Alan Stern
Cc: Dmitry Torokhov, Rafael J. Wysocki, Patrik Fimml, Bastien Nocera,
linux-pm, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <Pine.LNX.4.44L0.1407191039160.25162-100000@netrider.rowland.org>
On Sat, Jul 19, 2014 at 7:51 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 18 Jul 2014, Dmitry Torokhov wrote:
>
>> > The area where it must interact with power management is wakeup, both remote
>> > wakeup at run time and wakeup from system suspend. In particular, there's
>> > the question whether or not a device ignoring its input should be regarded
>> > as a wakeup source.
>>
>> I'd say no.
>
> This raises an interesting question. Suppose the system gets suspended
> while the lid is closed. At that point, shouldn't wakeup devices be
> enabled, even if they were already inhibited?
It's possible that this could be a policy decision, ie, whether
power/wakeup is set to enabled for those devices or not.
However, I'd say that there's only one policy that makes sense in that
case : wakeups should be disabled while suspended.
If we inhibited the device during runtime to prevent stray input
events from being generated, it wouldn't make sense to allow the
device to potentially generate an accidental wakeup while suspended.
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Alan Stern @ 2014-07-19 14:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rafael J. Wysocki, Patrik Fimml, Bastien Nocera, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <1451627.FURGiR949u@dtor-glaptop>
On Fri, 18 Jul 2014, Dmitry Torokhov wrote:
> > The area where it must interact with power management is wakeup, both remote
> > wakeup at run time and wakeup from system suspend. In particular, there's
> > the question whether or not a device ignoring its input should be regarded
> > as a wakeup source.
>
> I'd say no.
This raises an interesting question. Suppose the system gets suspended
while the lid is closed. At that point, shouldn't wakeup devices be
enabled, even if they were already inhibited?
Alan Stern
^ permalink raw reply
* [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: David Herrmann @ 2014-07-19 13:10 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Dmitry Torokhov, David Herrmann
In-Reply-To: <1405775445-4454-1-git-send-email-dh.herrmann@gmail.com>
When we introduced the slotted MT ABS extensions, we didn't take care to
make SYN_DROPPED recoverable. Imagine a client recevies a SYN_DROPPED and
syncs its current state via EVIOCGABS. It has to call this ioctl for each
and every ABS code separately. Besides being horribly slow, this series
of ioctl-calls is not atomic. The kernel might queue new ABS events while
the client fetches the data.
Now for normal ABS codes this is negligible as ABS values provide absolute
data. That is, there is usually no need to resync ABS codes as we don't
need previous values to interpret the next ABS code. Furthermore, most ABS
codes are also sent pretty frequently so a refresh is usually useless.
However, with the introduction of slotted ABS axes we added a relative
component: ABS_MT_SLOT. If a client syncs all ABS codes via EVIOCGABS
while the kernel has ABS-events and an ABS_MT_SLOT event queued, the
client will think any read ABS-event is for the retrieved SLOT, however,
this is not true as all events until the next ABS_MT_SLOT event are for
the previously active slot:
Kernel queue is: { ABS_DROPPED,
ABS_MT_POSITION_X(slot: 0),
ABS_MT_SLOT(slot: 1),
ABS_MT_POSITION_X(slot: 1) }
Client reads ABS_DROPPED from queue.
Client syncs all ABS values:
As part of that, client syncs ABS_MT_SLOT, which is 1 in the current
view of the kernel.
Client reads ABS_MT_POSITION_X and attributes it to slot 1 instead of
slot 0, as the slot-value is not explicit.
This is just a simple example how the relative information provided by the
ABS_MT_SLOT axis can be problematic to clients.
Now there are many ways to fix this:
* Make ABS_MT_SLOT a per-evdev-client attribute. On each
EVIOCGABS(ABS_MT_SLOT) we can add fake ABS_MT_SLOT events to the queue.
=> Ugly and overkill
* Flush all ABS events when clients read ABS_MT_SLOT.
=> Ugly hack and client might loose important ABS_MT_* events
* Provide atomic EVIOCGABS API.
=> Sounds good!
This patch introduces EVIOCGABSRANGE. Unlike EVIOCGABS, this ioctl only
fetches ABS values, rather than the whole "struct input_absinfo" set.
However, the new ioctl can fetch a range of ABS axes atomically and will
flush matching events from the client's receive queue. Moreover, you can
fetch all axes for *all* slots with a single call.
This way, a client can simply run EVIOCGABSRANGE(0, ABS_CNT) and it will
receive a consistent view of the whole ABS state, while the kernel flushes
the receive-buffer for a consistent view.
While most clients probably only need
EVIOCGABSRANGE(ABS_MT_SLOT, ABS_MT_TOOL_y - ABS_MT_SLOT + 1), the ioctl
allows to receive an arbitrary range of axes.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/input/evdev.c | 180 ++++++++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/input.h | 44 ++++++++++-
2 files changed, 219 insertions(+), 5 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 6386882..7a25a7a 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -175,8 +175,9 @@ static bool __evdev_is_filtered(struct evdev_client *client,
return mask && !test_bit(code, mask);
}
-/* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+/* flush queued, matching events, caller must hold client->buffer_lock */
+static void __evdev_flush_queue(struct evdev_client *client, unsigned int type,
+ unsigned int code_first, unsigned int code_last)
{
unsigned int i, head, num;
unsigned int mask = client->bufsize - 1;
@@ -195,7 +196,9 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
ev = &client->buffer[i];
is_report = ev->type == EV_SYN && ev->code == SYN_REPORT;
- if (ev->type == type) {
+ if (ev->type == type &&
+ ev->code >= code_first &&
+ ev->code <= code_last) {
/* drop matched entry */
continue;
} else if (is_report && !num) {
@@ -786,6 +789,172 @@ static int handle_eviocgbit(struct input_dev *dev,
return bits_to_user(bits, len, size, p, compat_mode);
}
+static inline void free_absrange(s32 **pages, size_t page_cnt)
+{
+ if (page_cnt > 1) {
+ while (page_cnt > 0) {
+ if (!pages[--page_cnt])
+ break;
+ __free_page(virt_to_page(pages[page_cnt]));
+ }
+ kfree(pages);
+ } else if (page_cnt == 1) {
+ kfree(pages);
+ }
+}
+
+static inline s32 *absrange_ptr(s32 **pages, size_t page_cnt, size_t slots,
+ size_t i_code, size_t j_slot)
+{
+ size_t idx, off;
+
+ idx = (i_code * slots + j_slot) / (PAGE_SIZE / sizeof(s32));
+ off = (i_code * slots + j_slot) % (PAGE_SIZE / sizeof(s32));
+
+ if (page_cnt == 1)
+ return &((s32*)pages)[off];
+ else
+ return &pages[idx][off];
+}
+
+static inline ssize_t fetch_absrange(struct evdev_client *client,
+ struct input_dev *dev, size_t start,
+ size_t count, size_t slots, s32 ***out)
+{
+ size_t size, page_cnt, i, j;
+ unsigned long flags;
+ s32 **pages;
+
+ /*
+ * Fetch data atomically from the device and flush buffers. We need to
+ * allocate a temporary buffer as copy_to_user() is not allowed while
+ * holding spinlocks. However, to-be-copied data might be huge and
+ * high-order allocations should be avoided. Therefore, do the
+ * page-allocation manually.
+ */
+
+ BUILD_BUG_ON(PAGE_SIZE % sizeof(s32) != 0);
+
+ size = sizeof(s32) * count * slots;
+ page_cnt = DIV_ROUND_UP(size, PAGE_SIZE);
+ if (page_cnt < 1) {
+ return 0;
+ } else if (page_cnt == 1) {
+ pages = kzalloc(size, GFP_TEMPORARY);
+ if (!pages)
+ return -ENOMEM;
+ } else {
+ pages = kzalloc(sizeof(*pages) * page_cnt, GFP_TEMPORARY);
+ if (!pages)
+ return -ENOMEM;
+
+ for (i = 0; i < page_cnt; ++i) {
+ pages[i] = (void*)get_zeroed_page(GFP_TEMPORARY);
+ if (!pages[i]) {
+ free_absrange(pages, page_cnt);
+ return -ENOMEM;
+ }
+ }
+ }
+
+ spin_lock_irqsave(&dev->event_lock, flags);
+ spin_lock(&client->buffer_lock);
+
+ for (i = 0; i < count; ++i) {
+ __u16 code;
+ bool is_mt;
+
+ code = start + i;
+ is_mt = input_is_mt_value(code);
+ if (is_mt && !dev->mt)
+ continue;
+
+ for (j = 0; j < slots; ++j) {
+ __s32 v;
+
+ if (is_mt)
+ v = input_mt_get_value(&dev->mt->slots[j],
+ code);
+ else
+ v = dev->absinfo[code].value;
+
+ *absrange_ptr(pages, page_cnt, slots, i, j) = v;
+
+ if (!is_mt)
+ break;
+ }
+ }
+
+ spin_unlock(&client->buffer_lock);
+ __evdev_flush_queue(client, EV_ABS, start, start + count - 1);
+ spin_unlock_irqrestore(&dev->event_lock, flags);
+
+ *out = pages;
+ return page_cnt;
+}
+
+static int evdev_handle_get_absrange(struct evdev_client *client,
+ struct input_dev *dev,
+ struct input_absrange __user *p)
+{
+ size_t slots, code, count, i, j;
+ struct input_absrange absbuf;
+ s32 **vals = NULL;
+ ssize_t val_cnt;
+ s32 __user *b;
+ int retval;
+
+ if (!dev->absinfo)
+ return -EINVAL;
+ if (copy_from_user(&absbuf, p, sizeof(absbuf)))
+ return -EFAULT;
+
+ slots = min_t(size_t, dev->mt ? dev->mt->num_slots : 1, absbuf.slots);
+ code = min_t(size_t, absbuf.code, ABS_CNT);
+ count = min_t(size_t, absbuf.count, ABS_CNT);
+
+ /* first fetch data atomically from device */
+
+ if (code + count > ABS_CNT)
+ count = ABS_CNT - code;
+
+ if (!slots || !count) {
+ val_cnt = 0;
+ } else {
+ val_cnt = fetch_absrange(client, dev, code, count,
+ slots, &vals);
+ if (val_cnt < 0)
+ return val_cnt;
+ }
+
+ /* now copy data to user-space */
+
+ b = (void __user*)(unsigned long)absbuf.buffer;
+ for (i = 0; i < absbuf.count; ++i) {
+ for (j = 0; j < absbuf.slots; ++j, ++b) {
+ s32 v;
+
+ if (i >= count || j >= slots)
+ v = 0;
+ else
+ v = *absrange_ptr(vals, val_cnt, slots, i, j);
+
+ if (put_user(v, b)) {
+ retval = -EFAULT;
+ goto out;
+ }
+ }
+ }
+
+ retval = 0;
+
+out:
+ free_absrange(vals, val_cnt);
+ if (retval < 0)
+ evdev_queue_syn_dropped(client);
+ return retval;
+}
+
static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
{
struct input_keymap_entry ke = {
@@ -889,7 +1058,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
spin_unlock(&dev->event_lock);
- __evdev_flush_queue(client, type);
+ __evdev_flush_queue(client, type, 0, UINT_MAX);
spin_unlock_irq(&client->buffer_lock);
@@ -1006,6 +1175,9 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
else
return evdev_revoke(evdev, client, file);
+ case EVIOCGABSRANGE:
+ return evdev_handle_get_absrange(client, dev, p);
+
case EVIOCGMASK:
if (copy_from_user(&mask, p, sizeof(mask)))
return -EFAULT;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index f6ace0e..9f851d4 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -32,7 +32,7 @@ struct input_event {
* Protocol version.
*/
-#define EV_VERSION 0x010001
+#define EV_VERSION 0x010002
/*
* IOCTLs (0x00 - 0x7f)
@@ -210,6 +210,48 @@ struct input_mask {
*/
#define EVIOCSMASK _IOW('E', 0x93, struct input_mask) /* Set event-masks */
+struct input_absrange {
+ __u16 slots;
+ __u16 code;
+ __u32 count;
+ __u64 buffer;
+};
+
+/**
+ * EVIOCGABSRANGE - Fetch range of ABS values
+ *
+ * This fetches the current values of a range of ABS codes atomically. The range
+ * of codes to fetch and the buffer-types are passed as "struct input_absrange",
+ * which has the following fields:
+ * slots: Number of MT slots to fetch data for.
+ * code: First ABS axis to query.
+ * count: Number of ABS axes to query starting at @code.
+ * buffer: Pointer to a receive buffer where to store the fetched ABS
+ * values. This buffer must be an array of __s32 with at least
+ * (@slots * @code) elements. The buffer is interpreted as two
+ * dimensional __s32 array, declared as: __s32[slots][codes]
+ *
+ * Compared to EVIOCGABS this ioctl allows to retrieve a range of ABS codes
+ * atomically regarding any concurrent buffer modifications. Furthermore, any
+ * pending events for codes that were retrived via this call are flushed from
+ * the client's receive buffer. But unlike EVIOCGABS, this ioctl only returns
+ * the current value of an axis, rather than the whole "struct input_absinfo"
+ * set. All fields of "struct input_absinfo" except for the value are constant,
+ * though.
+ *
+ * The kernel's current view of the ABS axes is copied into the provided buffer.
+ * If an ABS axis is not enabled on the device, its value will be zero. Also, if
+ * an axis is not a slotted MT-axis, values for all but the first slot will be
+ * 0. If @slots is greater than the actual number of slots provided by the
+ * device, values for all slots higher than that will be 0.
+ *
+ * This call may fail with -EINVAL if the kernel doesn't support this call or
+ * the arguments are invalid, with -ENODEV if access was revoked, -ENOMEM if the
+ * kernel couldn't allocate temporary buffers for data-copy or -EFAULT if the
+ * passed pointer was invalid.
+ */
+#define EVIOCGABSRANGE _IOR('E', 0x94, struct input_absrange)
+
#define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */
/*
--
2.0.2
^ permalink raw reply related
* [PATCH RESEND 4/5] Input: uinput - add new UINPUT_DEV_SETUP ioctl
From: David Herrmann @ 2014-07-19 13:10 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Dmitry Torokhov, David Herrmann
In-Reply-To: <1405775445-4454-1-git-send-email-dh.herrmann@gmail.com>
This adds a new ioctl UINPUT_DEV_SETUP that replaces the old device setup
method (by write()'ing "struct uinput_user_dev" to the node). The old
method is not easily extendable and requires huge payloads. Furthermore,
overloading write() without properly versioned objects is error-prone.
Therefore, we introduce a new ioctl to replace the old method. The ioctl
supports all features of the old method, plus a "resolution" field for
absinfo. Furthermore, it's properly forward-compatible to new ABS codes
and a growing "struct input_absinfo" structure.
The ioctl also allows user-space to skip unknown axes if not set. The
payload-size can now be specified by the caller. There is no need to copy
the whole array temporarily into the kernel, but instead we can iterate
over it and copy each value manually.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/input/misc/uinput.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
include/uapi/linux/uinput.h | 55 ++++++++++++++++++++++++++++++++++--
2 files changed, 118 insertions(+), 6 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index a2a3895..0f45595 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -371,8 +371,67 @@ static int uinput_allocate_device(struct uinput_device *udev)
return 0;
}
-static int uinput_setup_device(struct uinput_device *udev,
- const char __user *buffer, size_t count)
+static int uinput_dev_setup(struct uinput_device *udev,
+ struct uinput_setup __user *arg)
+{
+ struct uinput_setup setup;
+ struct input_dev *dev;
+ int i, retval;
+
+ if (udev->state == UIST_CREATED)
+ return -EINVAL;
+ if (copy_from_user(&setup, arg, sizeof(setup)))
+ return -EFAULT;
+ if (!setup.name[0])
+ return -EINVAL;
+ /* So far we only support the original "struct input_absinfo", but be
+ * forward compatible and allow larger payloads. */
+ if (setup.absinfo_size < sizeof(struct input_absinfo))
+ return -EINVAL;
+
+ dev = udev->dev;
+ dev->id = setup.id;
+ udev->ff_effects_max = setup.ff_effects_max;
+
+ kfree(dev->name);
+ dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL);
+ if (!dev->name)
+ return -ENOMEM;
+
+ if (setup.abs_cnt > ABS_CNT)
+ setup.abs_cnt = ABS_CNT;
+
+ if (setup.abs_cnt > 0) {
+ u8 __user *p = (u8 __user*)arg->abs;
+
+ input_alloc_absinfo(dev);
+ if (!dev->absinfo)
+ return -ENOMEM;
+
+ for (i = 0; i < setup.abs_cnt; ++i, p += setup.absinfo_size) {
+ struct input_absinfo absinfo;
+
+ if (!test_bit(i, dev->absbit))
+ continue;
+
+ if (copy_from_user(&absinfo, p, sizeof(absinfo)))
+ return -EFAULT;
+
+ dev->absinfo[i] = absinfo;
+ }
+ }
+
+ retval = uinput_validate_absbits(dev);
+ if (retval < 0)
+ return retval;
+
+ udev->state = UIST_SETUP_COMPLETE;
+ return 0;
+}
+
+/* legacy setup via write() */
+static int uinput_setup_device_legacy(struct uinput_device *udev,
+ const char __user *buffer, size_t count)
{
struct uinput_user_dev *user_dev;
struct input_dev *dev;
@@ -475,7 +534,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
retval = udev->state == UIST_CREATED ?
uinput_inject_events(udev, buffer, count) :
- uinput_setup_device(udev, buffer, count);
+ uinput_setup_device_legacy(udev, buffer, count);
mutex_unlock(&udev->mutex);
@@ -736,6 +795,10 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
uinput_destroy_device(udev);
goto out;
+ case UI_DEV_SETUP:
+ retval = uinput_dev_setup(udev, p);
+ goto out;
+
case UI_SET_EVBIT:
retval = uinput_set_bit(arg, evbit, EV_MAX);
goto out;
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index 19339cf..04a3876 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.5 06/20/2014 (David Herrmann <dh.herrmann@gmail.com>
+ * - add UI_DEV_SETUP ioctl
* 0.4 01/09/2014 (Benjamin Tissoires <benjamin.tissoires@redhat.com>)
* - add UI_GET_SYSNAME ioctl
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
@@ -37,8 +39,8 @@
#include <linux/types.h>
#include <linux/input.h>
-#define UINPUT_VERSION 4
-
+#define UINPUT_VERSION 5
+#define UINPUT_MAX_NAME_SIZE 80
struct uinput_ff_upload {
__u32 request_id;
@@ -93,6 +95,54 @@ struct uinput_ff_erase {
*/
#define UI_GET_VERSION _IOR(UINPUT_IOCTL_BASE, 301, unsigned int)
+struct uinput_setup {
+ __u64 absinfo_size;
+ struct input_id id;
+ char name[UINPUT_MAX_NAME_SIZE];
+ __u32 ff_effects_max;
+ __u32 abs_cnt;
+ struct input_absinfo abs[];
+};
+
+/**
+ * UI_DEV_SETUP - Set device parameters for setup
+ *
+ * This ioctl sets parameters for the input-device to be created. It must be
+ * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes
+ * the old "struct uinput_user_dev" method, which wrote this data via write().
+ *
+ * This ioctl takes a "struct uinput_setup" object as argument. The fields of
+ * this object are as follows:
+ * absinfo_size: This field *must* be initialized to
+ * "sizeof(struct input_absinfo)". It allows to extend the API
+ * to support more absinfo fields. Older kernels ignore unknown
+ * extensions to "struct input_absinfo".
+ * id: See the description of "struct input_id". This field is
+ * copied unchanged into the new device.
+ * name: This is used unchanged as name for the new device.
+ * ff_effects_max: This limits the maximum numbers of force-feedback effects.
+ * See below for a description of FF with uinput.
+ * abs_cnt: This field defines the amount of elements in the following
+ * "abs" array. Axes beyond the kernel's ABS_CNT are ignored.
+ * abs: This is an appended array that contains parameters for ABS
+ * axes. See "struct input_absinfo" for a description of these
+ * fields. This array is copied unchanged into the kernel for
+ * all specified axes. Axes not enabled via UI_SET_ABSBIT are
+ * ignored.
+ *
+ * This ioctl can be called multiple times and will overwrite previous values.
+ * If this ioctl fails with -EINVAL, you're recommended to use the old
+ * "uinput_user_dev" method via write() as fallback, in case you run on an old
+ * kernel that does not support this ioctl.
+ *
+ * This ioctl may fail with -EINVAL if it is not supported or if you passed
+ * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
+ * passed uinput_setup object cannot be read/written.
+ * If this call fails, partial data may have already been applied to the
+ * internal device.
+ */
+#define UI_DEV_SETUP _IOWR(UINPUT_IOCTL_BASE, 302, struct uinput_setup)
+
/*
* To write a force-feedback-capable driver, the upload_effect
* and erase_effect callbacks in input_dev must be implemented.
@@ -144,7 +194,6 @@ struct uinput_ff_erase {
#define UI_FF_UPLOAD 1
#define UI_FF_ERASE 2
-#define UINPUT_MAX_NAME_SIZE 80
struct uinput_user_dev {
char name[UINPUT_MAX_NAME_SIZE];
struct input_id id;
--
2.0.2
^ permalink raw reply related
* [PATCH RESEND 3/5] Input: uinput - add UI_GET_VERSION ioctl
From: David Herrmann @ 2014-07-19 13:10 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Dmitry Torokhov, David Herrmann
In-Reply-To: <1405775445-4454-1-git-send-email-dh.herrmann@gmail.com>
This ioctl is the counterpart to EVIOCGVERSION and returns the
uinput-version the kernel was compiled with.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/input/misc/uinput.c | 6 ++++++
include/uapi/linux/uinput.h | 9 +++++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 615324c..a2a3895 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -722,6 +722,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
}
switch (cmd) {
+ case UI_GET_VERSION:
+ if (put_user(UINPUT_VERSION,
+ (unsigned int __user*)p))
+ retval = -EFAULT;
+ goto out;
+
case UI_DEV_CREATE:
retval = uinput_create_device(udev);
goto out;
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index 0389b48..19339cf 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -84,6 +84,15 @@ struct uinput_ff_erase {
*/
#define UI_GET_SYSNAME(len) _IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
+/**
+ * UI_GET_VERSION - Return uinput version of the kernel
+ *
+ * This writes the kernel's uinput version into the integer pointed to by the
+ * ioctl argument. The uinput-version is hard-coded in the kernel and
+ * independent of the uinput device.
+ */
+#define UI_GET_VERSION _IOR(UINPUT_IOCTL_BASE, 301, unsigned int)
+
/*
* To write a force-feedback-capable driver, the upload_effect
* and erase_effect callbacks in input_dev must be implemented.
--
2.0.2
^ permalink raw reply related
* [PATCH RESEND 2/5] Input: uinput - uinput_validate_absbits() cleanup
From: David Herrmann @ 2014-07-19 13:10 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Dmitry Torokhov, David Herrmann
In-Reply-To: <1405775445-4454-1-git-send-email-dh.herrmann@gmail.com>
This moves basic checks and setup from uinput_setup_device() into
uinput_validate_absbits() to make it easier to use. This way, we can call
it from other places without copying the boilerplate code.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/input/misc/uinput.c | 40 +++++++++++++++++++++-------------------
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 8569362..615324c 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -311,7 +311,13 @@ static int uinput_open(struct inode *inode, struct file *file)
static int uinput_validate_absbits(struct input_dev *dev)
{
unsigned int cnt;
- int retval = 0;
+ int nslot;
+
+ if (!test_bit(EV_ABS, dev->evbit))
+ return 0;
+
+ /* check if absmin/absmax/absfuzz/absflat are filled as
+ * told in Documentation/input/input-programming.txt */
for (cnt = 0; cnt < ABS_CNT; cnt++) {
int min, max;
@@ -327,8 +333,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
UINPUT_NAME, cnt,
input_abs_get_min(dev, cnt),
input_abs_get_max(dev, cnt));
- retval = -EINVAL;
- break;
+ return -EINVAL;
}
if (input_abs_get_flat(dev, cnt) >
@@ -340,11 +345,18 @@ static int uinput_validate_absbits(struct input_dev *dev)
input_abs_get_flat(dev, cnt),
input_abs_get_min(dev, cnt),
input_abs_get_max(dev, cnt));
- retval = -EINVAL;
- break;
+ return -EINVAL;
}
}
- return retval;
+
+ if (test_bit(ABS_MT_SLOT, dev->absbit)) {
+ nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
+ input_mt_init_slots(dev, nslot, 0);
+ } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
+ input_set_events_per_packet(dev, 60);
+ }
+
+ return 0;
}
static int uinput_allocate_device(struct uinput_device *udev)
@@ -410,19 +422,9 @@ static int uinput_setup_device(struct uinput_device *udev,
input_abs_set_flat(dev, i, user_dev->absflat[i]);
}
- /* check if absmin/absmax/absfuzz/absflat are filled as
- * told in Documentation/input/input-programming.txt */
- if (test_bit(EV_ABS, dev->evbit)) {
- retval = uinput_validate_absbits(dev);
- if (retval < 0)
- goto exit;
- if (test_bit(ABS_MT_SLOT, dev->absbit)) {
- int nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
- input_mt_init_slots(dev, nslot, 0);
- } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
- input_set_events_per_packet(dev, 60);
- }
- }
+ retval = uinput_validate_absbits(dev);
+ if (retval < 0)
+ goto exit;
udev->state = UIST_SETUP_COMPLETE;
retval = count;
--
2.0.2
^ permalink raw reply related
* [PATCH RESEND 1/5] Input: evdev - add event-mask API
From: David Herrmann @ 2014-07-19 13:10 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Dmitry Torokhov, David Herrmann
In-Reply-To: <1405775445-4454-1-git-send-email-dh.herrmann@gmail.com>
Hardware manufacturers group keys in the weirdest way possible. This may
cause a power-key to be grouped together with normal keyboard keys and
thus be reported on the same kernel interface.
However, user-space is often only interested in specific sets of events.
For instance, daemons dealing with system-reboot (like systemd-logind)
listen for KEY_POWER, but are not interested in any main keyboard keys.
Usually, power keys are reported via separate interfaces, however,
some i8042 boards report it in the AT matrix. To avoid waking up those
system daemons on each key-press, we had two ideas:
- split off KEY_POWER into a separate interface unconditionally
- allow filtering a specific set of events on evdev FDs
Splitting of KEY_POWER is a rather weird way to deal with this and may
break backwards-compatibility. It is also specific to KEY_POWER and might
be required for other stuff, too. Moreover, we might end up with a huge
set of input-devices just to have them properly split.
Hence, this patchset implements the second idea: An event-mask to specify
which events you're interested in. Two ioctls allow setting this mask for
each event-type. If not set, all events are reported. The type==0 entry is
used same as in EVIOCGBIT to set the actual EV_* mask of filtered events.
This way, you have a two-level filter.
We are heavily forward-compatible to new event-types and event-codes. So
new user-space will be able to run on an old kernel which doesn't know the
given event-codes or event-types.
Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/input/evdev.c | 156 ++++++++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/input.h | 56 ++++++++++++++++
2 files changed, 210 insertions(+), 2 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index fd325ec..6386882 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -51,10 +51,130 @@ struct evdev_client {
struct list_head node;
int clkid;
bool revoked;
+ unsigned long *evmasks[EV_CNT];
unsigned int bufsize;
struct input_event buffer[];
};
+static size_t evdev_get_mask_cnt(unsigned int type)
+{
+ static size_t counts[EV_CNT] = {
+ /* EV_SYN==0 is EV_CNT, _not_ SYN_CNT, see EVIOCGBIT */
+ [EV_SYN] = EV_CNT,
+ [EV_KEY] = KEY_CNT,
+ [EV_REL] = REL_CNT,
+ [EV_ABS] = ABS_CNT,
+ [EV_MSC] = MSC_CNT,
+ [EV_SW] = SW_CNT,
+ [EV_LED] = LED_CNT,
+ [EV_SND] = SND_CNT,
+ [EV_FF] = FF_CNT,
+ };
+
+ return (type < EV_CNT) ? counts[type] : 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_set_mask(struct evdev_client *client,
+ unsigned int type,
+ const void __user *codes,
+ u32 codes_size)
+{
+ unsigned long flags, *mask, *oldmask;
+ size_t cnt, size;
+
+ /* unknown masks are simply ignored for forward-compat */
+ cnt = evdev_get_mask_cnt(type);
+ if (!cnt)
+ return 0;
+
+ /* we allow 'codes_size > size' for forward-compat */
+ size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+
+ mask = kzalloc(size, GFP_KERNEL);
+ if (!mask)
+ return -ENOMEM;
+
+ if (copy_from_user(mask, codes, min_t(size_t, codes_size, size))) {
+ kfree(mask);
+ return -EFAULT;
+ }
+
+ spin_lock_irqsave(&client->buffer_lock, flags);
+ oldmask = client->evmasks[type];
+ client->evmasks[type] = mask;
+ spin_unlock_irqrestore(&client->buffer_lock, flags);
+
+ kfree(oldmask);
+
+ return 0;
+}
+
+/* must be called with evdev-mutex held */
+static int evdev_get_mask(struct evdev_client *client,
+ unsigned int type,
+ void __user *codes,
+ u32 codes_size)
+{
+ unsigned long *mask;
+ size_t cnt, size, min, i;
+ u8 __user *out;
+
+ /* we allow unknown types and 'codes_size > size' for forward-compat */
+ cnt = evdev_get_mask_cnt(type);
+ size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
+ min = min_t(size_t, codes_size, size);
+
+ if (cnt > 0) {
+ mask = client->evmasks[type];
+ if (mask) {
+ if (copy_to_user(codes, mask, min))
+ return -EFAULT;
+ } else {
+ /* fake mask with all bits set */
+ out = (u8 __user*)codes;
+ for (i = 0; i < min; ++i) {
+ if (put_user((u8)0xff, out + i))
+ return -EFAULT;
+ }
+ }
+ }
+
+ codes = (u8*)codes + min;
+ codes_size -= min;
+
+ if (codes_size > 0 && clear_user(codes, codes_size))
+ return -EFAULT;
+
+ return 0;
+}
+
+/* requires the buffer lock to be held */
+static bool __evdev_is_filtered(struct evdev_client *client,
+ unsigned int type,
+ unsigned int code)
+{
+ unsigned long *mask;
+ size_t cnt;
+
+ /* EV_SYN and unknown codes are never filtered */
+ if (type == EV_SYN || type >= EV_CNT)
+ return false;
+
+ /* first test whether the type is filtered */
+ mask = client->evmasks[0];
+ if (mask && !test_bit(type, mask))
+ return true;
+
+ /* unknown values are never filtered */
+ cnt = evdev_get_mask_cnt(type);
+ if (!cnt || code >= cnt)
+ return false;
+
+ mask = client->evmasks[type];
+ return mask && !test_bit(code, mask);
+}
+
/* flush queued events of type @type, caller must hold client->buffer_lock */
static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
{
@@ -177,12 +297,21 @@ static void evdev_pass_values(struct evdev_client *client,
spin_lock(&client->buffer_lock);
for (v = vals; v != vals + count; v++) {
+ if (__evdev_is_filtered(client, v->type, v->code))
+ continue;
+
+ if (v->type == EV_SYN && v->code == SYN_REPORT) {
+ /* drop empty SYN_REPORT */
+ if (client->packet_head == client->head)
+ continue;
+
+ wakeup = true;
+ }
+
event.type = v->type;
event.code = v->code;
event.value = v->value;
__pass_event(client, &event);
- if (v->type == EV_SYN && v->code == SYN_REPORT)
- wakeup = true;
}
spin_unlock(&client->buffer_lock);
@@ -365,6 +494,7 @@ static int evdev_release(struct inode *inode, struct file *file)
{
struct evdev_client *client = file->private_data;
struct evdev *evdev = client->evdev;
+ unsigned int i;
mutex_lock(&evdev->mutex);
evdev_ungrab(evdev, client);
@@ -372,6 +502,9 @@ static int evdev_release(struct inode *inode, struct file *file)
evdev_detach_client(evdev, client);
+ for (i = 0; i < EV_CNT; ++i)
+ kfree(client->evmasks[i]);
+
if (is_vmalloc_addr(client))
vfree(client);
else
@@ -811,6 +944,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
struct evdev *evdev = client->evdev;
struct input_dev *dev = evdev->handle.dev;
struct input_absinfo abs;
+ struct input_mask mask;
struct ff_effect effect;
int __user *ip = (int __user *)p;
unsigned int i, t, u, v;
@@ -872,6 +1006,24 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
else
return evdev_revoke(evdev, client, file);
+ case EVIOCGMASK:
+ if (copy_from_user(&mask, p, sizeof(mask)))
+ return -EFAULT;
+
+ return evdev_get_mask(client,
+ mask.type,
+ (void*)(long)mask.codes_ptr,
+ mask.codes_size);
+
+ case EVIOCSMASK:
+ if (copy_from_user(&mask, p, sizeof(mask)))
+ return -EFAULT;
+
+ return evdev_set_mask(client,
+ mask.type,
+ (const void*)(long)mask.codes_ptr,
+ mask.codes_size);
+
case EVIOCSCLOCKID:
if (copy_from_user(&i, p, sizeof(unsigned int)))
return -EFAULT;
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 19df18c..f6ace0e 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -97,6 +97,12 @@ struct input_keymap_entry {
__u8 scancode[32];
};
+struct input_mask {
+ __u32 type;
+ __u32 codes_size;
+ __u64 codes_ptr;
+};
+
#define EVIOCGVERSION _IOR('E', 0x01, int) /* get driver version */
#define EVIOCGID _IOR('E', 0x02, struct input_id) /* get device ID */
#define EVIOCGREP _IOR('E', 0x03, unsigned int[2]) /* get repeat settings */
@@ -154,6 +160,56 @@ struct input_keymap_entry {
#define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
#define EVIOCREVOKE _IOW('E', 0x91, int) /* Revoke device access */
+/**
+ * EVIOCGMASK - Retrieve current event-mask
+ *
+ * This retrieves the current event-mask for a specific event-type. The
+ * argument must be of type "struct input_mask" and specifies the event-type to
+ * query, the receive buffer and the size of the receive buffer.
+ *
+ * The event-mask is a per-client mask that specifies which events are forwarded
+ * to the client. Each event-code is represented by a single bit in the
+ * event-mask. If the bit is set, the event is passed to the client normally.
+ * Otherwise, the event is filtered and and will never be queued on the
+ * client's receive buffer.
+ * Event-masks do not affect global state of an input-device. They only affect
+ * the open-file they're applied on. Each open-file (i.e, file-description) can
+ * have a different event-mask.
+ *
+ * The default event-mask for a client has all bits set, i.e. all events are
+ * forwarded to the client. If a kernel is queried for an unknown event-type
+ * or if the receive buffer is larger than the number of event-codes known to
+ * the kernel, the kernel returns all zeroes for those codes.
+ *
+ * At maximum, codes_size bytes are copied.
+ *
+ * This ioctl may fail with ENODEV in case the file is revoked, EFAULT
+ * if the receive-buffer points to invalid memory, or EINVAL if the kernel
+ * does not implement the ioctl.
+ */
+#define EVIOCGMASK _IOR('E', 0x92, struct input_mask) /* Get event-masks */
+
+/**
+ * EVIOCSMASK - Set event-mask
+ *
+ * This is the counterpart to EVIOCGMASK. Instead of receiving the current
+ * event-mask, this changes the client's event-mask for a specific type. See
+ * EVIOCGMASK for a description of event-masks and the argument-type.
+ *
+ * This ioctl provides full forward-compatibility. If the passed event-type is
+ * unknown to the kernel, or if the number of codes is bigger than known to the
+ * kernel, the ioctl is still accepted and applied. However, any unknown codes
+ * are left untouched and stay cleared. That means, the kernel always filters
+ * unknown codes regardless of what the client requests.
+ * If the new mask doesn't cover all known event-codes, all remaining codes are
+ * automatically cleared and thus filtered.
+ *
+ * This ioctl may fail with ENODEV in case the file is revoked. EFAULT is
+ * returned if the receive-buffer points to invalid memory. EINVAL is returned
+ * if the kernel does not implement the ioctl.
+ */
+#define EVIOCSMASK _IOW('E', 0x93, struct input_mask) /* Set event-masks */
+
#define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */
/*
--
2.0.2
^ permalink raw reply related
* [PATCH RESEND 0/5] Evdev Extensions
From: David Herrmann @ 2014-07-19 13:10 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Dmitry Torokhov, David Herrmann
Hi Dmitry
I posted all these ~2 months ago, but haven't seen any comments from you. Seeing
that you switched jobs (congratulations, btw!), I guess you were quite busy the
last few weeks. Hence, here's a resend of all the evdev changes squashed into a
single series. They're also available in my fdo repo [1] in case you wanna fetch
them directly.
No big changes except for a single "udev->state == UIST_CREATED" fix spotted by
Peter.
The last patch is still RFC, so please omit it in case you apply the patches!
All patches but the last are tested via libevdev and run fine here since several
months.
Thanks
David
[1] fdo repo: http://cgit.freedesktop.org/~dvdhrm/linux/log/?h=input-next
David Herrmann (5):
Input: evdev - add event-mask API
Input: uinput - uinput_validate_absbits() cleanup
Input: uinput - add UI_GET_VERSION ioctl
Input: uinput - add new UINPUT_DEV_SETUP ioctl
Input: evdev - add new EVIOCGABSRANGE ioctl
drivers/input/evdev.c | 336 +++++++++++++++++++++++++++++++++++++++++++-
drivers/input/misc/uinput.c | 115 ++++++++++++---
include/uapi/linux/input.h | 100 ++++++++++++-
include/uapi/linux/uinput.h | 64 ++++++++-
4 files changed, 583 insertions(+), 32 deletions(-)
--
2.0.2
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 23:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Patrik Fimml, Bastien Nocera, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <1451627.FURGiR949u@dtor-glaptop>
On Friday, July 18, 2014 04:16:50 PM Dmitry Torokhov wrote:
> On Saturday, July 19, 2014 12:55:09 AM Rafael J. Wysocki wrote:
> > On Saturday, July 19, 2014 12:19:39 AM Rafael J. Wysocki wrote:
> > > On Friday, July 18, 2014 02:45:40 PM Dmitry Torokhov wrote:
> > > > On Friday, July 18, 2014 11:59:18 PM Rafael J. Wysocki wrote:
> > > > > On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> > > > > > On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > > > > > > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > > > > > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > > > > [cut]
> > > > >
> > > > > > > > I'm not sure what the appropriate action for a video camera is
> > > > > > > > anyway.
> > > > > > > > Should it go away completely, including its device? Should it be
> > > > > > > > there,
> > > > > > > > but certainly not be the default choice when there is an
> > > > > > > > external
> > > > > > > > camera? I'm thinking along the lines of some application's
> > > > > > > > settings
> > > > > > > > dialog here, where it might be desirable to still be able to
> > > > > > > > select
> > > > > > > > the
> > > > > > > > internal camera for future recordings.
> > > > > > > >
> > > > > > > > Of course, userspace could still decide simply not to
> > > > > > > > quiesce|deactivate|inhibit the device if that was desired.
> > > > > > >
> > > > > > > There's some question about how much of userspace needs to get
> > > > > > > involved. Just the daemon that manages these configuration
> > > > > > > changes, or
> > > > > > > other programs as well? I guess that's not really our problem...
> > > > > >
> > > > > > We need to provide means of implementing policy; the policy itself
> > > > > > is not
> > > > > > really our concern ;)
> > > > > >
> > > > > > > In the end, it sounds like you're suggesting a new pair of PM
> > > > > > > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > > > > > > ->uninhibit(). Plus an optional (?) sysfs interface for invoking
> > > > > > > the
> > > > > > > callbacks.
> > > > > >
> > > > > > We do need sysfs interface so that userspace can talk to the devices
> > > > > > in
> > > > > > question; and we also need to make sure that PM core is aware of the
> > > > > > new
> > > > > > callbacks and provides guarantees about their interactions with
> > > > > > system-
> > > > > > and
> > > > > > runtime-PM callbacks so that individual drivers do not have to sort
> > > > > > it out
> > > > > > on their own.
> > > > >
> > > > > A step back, please.
> > > > >
> > > > > I have no idea why those need to be PM callbacks.
> > > > >
> > > > > What you need really seems to be a way to tell a driver "ignore input
> > > > > from
> > > > > this device from now on as it is most likely bogus". A natural
> > > > > reaction of
> > > > > the driver to that might be to stop processing input from the device
> > > > > and
> > > > > then runtime suspend it (and prevent it from generating remote wakeup
> > > > > as
> > > > > that may be bogus as well), but I don't see why the PM core needs to
> > > > > be
> > > > > involved in that at all.
> > > >
> > > > So that we do not need to handle cases like:
> > > >
> > > > - I am already in idle state and request comes to inhibit, what do I do
> > > > (in
> > >
> > > > driver) or:
> > > I'm not sure why being "suspended" or not matters here. The PM core
> > > doesn't know what physical state the device is in anyway and the driver
> > > or subsystem (or another layer such as ACPI) has to track that.
> > >
> > > Also it seems that it should be perfectly fine to ignore input from the
> > > device without suspending it as well as it is perfectly fine to be
> > > suspended while you are generally not ignoring the input (just because
> > > there is no input at the moment, for example).
> > >
> > > Yes, it make sense to suspend the device when you know you'll ignore input
> > > going forward, but then if the real goal is to prevent bogus input from
> > > reaching applications, then this isn't a power management problem even.
> >
> > The area where it must interact with power management is wakeup, both remote
> > wakeup at run time and wakeup from system suspend. In particular, there's
> > the question whether or not a device ignoring its input should be regarded
> > as a wakeup source.
>
> I'd say no.
>
> Anyway, even though it is very tempting to declare inhibit a "deeper" state of
> runtime suspend maybe you are right and inhibit should really be separate from
> PM and drivers would have to sort out all the possible state permutations.
>
> Considering input devices:
>
> input_open(): check if device is inhibited, if so do nothing. Otherwise try
> waking up itself and parent (via pm_runtime_get_sync() on itself), this will
> power up the device. Do additional configuration if needed.
>
> input_close(): check if device is inhibited, if not do pm_runtime_put (_sync?
> to make sure we power off properly and not leave device up and running? or
> should we power down manually not waiting for runtime PM)?
pm_runtime_put_sync() should be sufficient here I think.
> inhibit(): check if device is opened, if opened do pm_runtime_put_sync().
>
> uninhibit(): if device is opened do pm_runtime_get_sync(), let runtime PM
> bring up the device. Do additional config if needed -> very similar to
> input_open(), different condition.
>
> runtime_suspend(): power down the device. If not inhibited enable as wakeup
> source.
User space may not want it to be a wakeup source even if not inhibited
(it may not want it to wake up from system suspend, for example, like I do
with my cordless mouse).
> runtime_resume(): power up the device if device is opened and not inhibited.
>
> system_suspend(): check if device is opened, not inhibited and not in
> runtimesuspend already; power down.
I think you only need to know if the device has been powered down already
here regardless of the reason.
> system_resume(): power up the device if it is opened and not inhibited. I
> guess it's OK to wake up device that shoudl be runtime-PM-idle since it will
> go to back sleep shortly.
>
> Ugh.. This is complicated...
If runtime PM and system suspend shared the status, we might simplify this
somewhat. Problem is, there are drivers that don't support runtime PM, but
support system suspend.
Rafael
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Dmitry Torokhov @ 2014-07-18 23:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alan Stern, Patrik Fimml, Bastien Nocera, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <1693424.kPNUQHOHlX@vostro.rjw.lan>
On Saturday, July 19, 2014 12:55:09 AM Rafael J. Wysocki wrote:
> On Saturday, July 19, 2014 12:19:39 AM Rafael J. Wysocki wrote:
> > On Friday, July 18, 2014 02:45:40 PM Dmitry Torokhov wrote:
> > > On Friday, July 18, 2014 11:59:18 PM Rafael J. Wysocki wrote:
> > > > On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> > > > > On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > > > > > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > > > > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > > > [cut]
> > > >
> > > > > > > I'm not sure what the appropriate action for a video camera is
> > > > > > > anyway.
> > > > > > > Should it go away completely, including its device? Should it be
> > > > > > > there,
> > > > > > > but certainly not be the default choice when there is an
> > > > > > > external
> > > > > > > camera? I'm thinking along the lines of some application's
> > > > > > > settings
> > > > > > > dialog here, where it might be desirable to still be able to
> > > > > > > select
> > > > > > > the
> > > > > > > internal camera for future recordings.
> > > > > > >
> > > > > > > Of course, userspace could still decide simply not to
> > > > > > > quiesce|deactivate|inhibit the device if that was desired.
> > > > > >
> > > > > > There's some question about how much of userspace needs to get
> > > > > > involved. Just the daemon that manages these configuration
> > > > > > changes, or
> > > > > > other programs as well? I guess that's not really our problem...
> > > > >
> > > > > We need to provide means of implementing policy; the policy itself
> > > > > is not
> > > > > really our concern ;)
> > > > >
> > > > > > In the end, it sounds like you're suggesting a new pair of PM
> > > > > > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > > > > > ->uninhibit(). Plus an optional (?) sysfs interface for invoking
> > > > > > the
> > > > > > callbacks.
> > > > >
> > > > > We do need sysfs interface so that userspace can talk to the devices
> > > > > in
> > > > > question; and we also need to make sure that PM core is aware of the
> > > > > new
> > > > > callbacks and provides guarantees about their interactions with
> > > > > system-
> > > > > and
> > > > > runtime-PM callbacks so that individual drivers do not have to sort
> > > > > it out
> > > > > on their own.
> > > >
> > > > A step back, please.
> > > >
> > > > I have no idea why those need to be PM callbacks.
> > > >
> > > > What you need really seems to be a way to tell a driver "ignore input
> > > > from
> > > > this device from now on as it is most likely bogus". A natural
> > > > reaction of
> > > > the driver to that might be to stop processing input from the device
> > > > and
> > > > then runtime suspend it (and prevent it from generating remote wakeup
> > > > as
> > > > that may be bogus as well), but I don't see why the PM core needs to
> > > > be
> > > > involved in that at all.
> > >
> > > So that we do not need to handle cases like:
> > >
> > > - I am already in idle state and request comes to inhibit, what do I do
> > > (in
> >
> > > driver) or:
> > I'm not sure why being "suspended" or not matters here. The PM core
> > doesn't know what physical state the device is in anyway and the driver
> > or subsystem (or another layer such as ACPI) has to track that.
> >
> > Also it seems that it should be perfectly fine to ignore input from the
> > device without suspending it as well as it is perfectly fine to be
> > suspended while you are generally not ignoring the input (just because
> > there is no input at the moment, for example).
> >
> > Yes, it make sense to suspend the device when you know you'll ignore input
> > going forward, but then if the real goal is to prevent bogus input from
> > reaching applications, then this isn't a power management problem even.
>
> The area where it must interact with power management is wakeup, both remote
> wakeup at run time and wakeup from system suspend. In particular, there's
> the question whether or not a device ignoring its input should be regarded
> as a wakeup source.
I'd say no.
Anyway, even though it is very tempting to declare inhibit a "deeper" state of
runtime suspend maybe you are right and inhibit should really be separate from
PM and drivers would have to sort out all the possible state permutations.
Considering input devices:
input_open(): check if device is inhibited, if so do nothing. Otherwise try
waking up itself and parent (via pm_runtime_get_sync() on itself), this will
power up the device. Do additional configuration if needed.
input_close(): check if device is inhibited, if not do pm_runtime_put (_sync?
to make sure we power off properly and not leave device up and running? or
should we power down manually not waiting for runtime PM)?
inhibit(): check if device is opened, if opened do pm_runtime_put_sync().
uninhibit(): if device is opened do pm_runtime_get_sync(), let runtime PM
bring up the device. Do additional config if needed -> very similar to
input_open(), different condition.
runtime_suspend(): power down the device. If not inhibited enable as wakeup
source.
runtime_resume(): power up the device if device is opened and not inhibited.
system_suspend(): check if device is opened, not inhibited and not in
runtimesuspend already; power down.
system_resume(): power up the device if it is opened and not inhibited. I
guess it's OK to wake up device that shoudl be runtime-PM-idle since it will
go to back sleep shortly.
Ugh.. This is complicated...
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] input: wacom: Support up to 2048 pressure levels with ISDv4
From: Jason Gerecke @ 2014-07-18 23:12 UTC (permalink / raw)
To: Linux Input, Ping Cheng, Dmitry Torokhov; +Cc: Jason Gerecke
In-Reply-To: <1404164806-5141-1-git-send-email-killertofu@gmail.com>
On Mon, Jun 30, 2014 at 2:46 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
> drivers/input/tablet/wacom_wac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c
> index 977d05c..61a45a5 100644
> --- a/drivers/input/tablet/wacom_wac.c
> +++ b/drivers/input/tablet/wacom_wac.c
> @@ -1093,7 +1093,7 @@ static int wacom_tpc_pen(struct wacom_wac *wacom)
> input_report_key(input, BTN_STYLUS2, data[1] & 0x10);
> input_report_abs(input, ABS_X, le16_to_cpup((__le16 *)&data[2]));
> input_report_abs(input, ABS_Y, le16_to_cpup((__le16 *)&data[4]));
> - input_report_abs(input, ABS_PRESSURE, ((data[7] & 0x03) << 8) | data[6]);
> + input_report_abs(input, ABS_PRESSURE, ((data[7] & 0x07) << 8) | data[6]);
> input_report_key(input, BTN_TOUCH, data[1] & 0x05);
> input_report_key(input, wacom->tool[0], prox);
> return 1;
> --
> 2.0.0
>
Just making sure these two patches don't get lost in all the work
Benjamin's doing. They're going to conflict, so depending on the
timeline for merging his patches, we can either push these up now(ish)
or I'll rebase afterwards.
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 22:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Patrik Fimml, Bastien Nocera, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <2443949.K0iePsg3Aa@vostro.rjw.lan>
On Saturday, July 19, 2014 12:19:39 AM Rafael J. Wysocki wrote:
> On Friday, July 18, 2014 02:45:40 PM Dmitry Torokhov wrote:
> > On Friday, July 18, 2014 11:59:18 PM Rafael J. Wysocki wrote:
> > > On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> > > > On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > > > > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > > > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > > [cut]
> > >
> > > > > > I'm not sure what the appropriate action for a video camera is anyway.
> > > > > > Should it go away completely, including its device? Should it be
> > > > > > there,
> > > > > > but certainly not be the default choice when there is an external
> > > > > > camera? I'm thinking along the lines of some application's settings
> > > > > > dialog here, where it might be desirable to still be able to select
> > > > > > the
> > > > > > internal camera for future recordings.
> > > > > >
> > > > > > Of course, userspace could still decide simply not to
> > > > > > quiesce|deactivate|inhibit the device if that was desired.
> > > > >
> > > > > There's some question about how much of userspace needs to get
> > > > > involved. Just the daemon that manages these configuration changes, or
> > > > > other programs as well? I guess that's not really our problem...
> > > >
> > > > We need to provide means of implementing policy; the policy itself is not
> > > > really our concern ;)
> > > >
> > > > > In the end, it sounds like you're suggesting a new pair of PM
> > > > > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > > > > ->uninhibit(). Plus an optional (?) sysfs interface for invoking the
> > > > > callbacks.
> > > >
> > > > We do need sysfs interface so that userspace can talk to the devices in
> > > > question; and we also need to make sure that PM core is aware of the new
> > > > callbacks and provides guarantees about their interactions with system-
> > > > and
> > > > runtime-PM callbacks so that individual drivers do not have to sort it out
> > > > on their own.
> > >
> > > A step back, please.
> > >
> > > I have no idea why those need to be PM callbacks.
> > >
> > > What you need really seems to be a way to tell a driver "ignore input from
> > > this device from now on as it is most likely bogus". A natural reaction of
> > > the driver to that might be to stop processing input from the device and
> > > then runtime suspend it (and prevent it from generating remote wakeup as
> > > that may be bogus as well), but I don't see why the PM core needs to be
> > > involved in that at all.
> >
> > So that we do not need to handle cases like:
> >
> > - I am already in idle state and request comes to inhibit, what do I do (in
> > driver) or:
>
> I'm not sure why being "suspended" or not matters here. The PM core doesn't
> know what physical state the device is in anyway and the driver or subsystem
> (or another layer such as ACPI) has to track that.
>
> Also it seems that it should be perfectly fine to ignore input from the device
> without suspending it as well as it is perfectly fine to be suspended while
> you are generally not ignoring the input (just because there is no input at
> the moment, for example).
>
> Yes, it make sense to suspend the device when you know you'll ignore input going
> forward, but then if the real goal is to prevent bogus input from reaching
> applications, then this isn't a power management problem even.
The area where it must interact with power management is wakeup, both remote
wakeup at run time and wakeup from system suspend. In particular, there's the
question whether or not a device ignoring its input should be regarded as a
wakeup source.
Rafael
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 22:19 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Patrik Fimml, Bastien Nocera, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <17819865.W1zWUlPo9n@dtor-glaptop>
On Friday, July 18, 2014 02:45:40 PM Dmitry Torokhov wrote:
> On Friday, July 18, 2014 11:59:18 PM Rafael J. Wysocki wrote:
> > On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> > > On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > > > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > [cut]
> >
> > > > > I'm not sure what the appropriate action for a video camera is anyway.
> > > > > Should it go away completely, including its device? Should it be
> > > > > there,
> > > > > but certainly not be the default choice when there is an external
> > > > > camera? I'm thinking along the lines of some application's settings
> > > > > dialog here, where it might be desirable to still be able to select
> > > > > the
> > > > > internal camera for future recordings.
> > > > >
> > > > > Of course, userspace could still decide simply not to
> > > > > quiesce|deactivate|inhibit the device if that was desired.
> > > >
> > > > There's some question about how much of userspace needs to get
> > > > involved. Just the daemon that manages these configuration changes, or
> > > > other programs as well? I guess that's not really our problem...
> > >
> > > We need to provide means of implementing policy; the policy itself is not
> > > really our concern ;)
> > >
> > > > In the end, it sounds like you're suggesting a new pair of PM
> > > > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > > > ->uninhibit(). Plus an optional (?) sysfs interface for invoking the
> > > > callbacks.
> > >
> > > We do need sysfs interface so that userspace can talk to the devices in
> > > question; and we also need to make sure that PM core is aware of the new
> > > callbacks and provides guarantees about their interactions with system-
> > > and
> > > runtime-PM callbacks so that individual drivers do not have to sort it out
> > > on their own.
> >
> > A step back, please.
> >
> > I have no idea why those need to be PM callbacks.
> >
> > What you need really seems to be a way to tell a driver "ignore input from
> > this device from now on as it is most likely bogus". A natural reaction of
> > the driver to that might be to stop processing input from the device and
> > then runtime suspend it (and prevent it from generating remote wakeup as
> > that may be bogus as well), but I don't see why the PM core needs to be
> > involved in that at all.
>
> So that we do not need to handle cases like:
>
> - I am already in idle state and request comes to inhibit, what do I do (in
> driver) or:
I'm not sure why being "suspended" or not matters here. The PM core doesn't
know what physical state the device is in anyway and the driver or subsystem
(or another layer such as ACPI) has to track that.
Also it seems that it should be perfectly fine to ignore input from the device
without suspending it as well as it is perfectly fine to be suspended while
you are generally not ignoring the input (just because there is no input at
the moment, for example).
Yes, it make sense to suspend the device when you know you'll ignore input going
forward, but then if the real goal is to prevent bogus input from reaching
applications, then this isn't a power management problem even.
> - I inhibited the device, now system suspend comes, what do I do? Also what do
> I do at resume? I'd rather not have driver checks host of flags to figure out
> the end state if PM core could spell it out for me.
Well, how will the new PM callbacks help here, exactly?
> We already have to sort
> out open/close and suspend/resume iteractions (i.e what one needs to do to
> suspend or resume device that has not been opened, and if it is different from
> devices that have been opened).
I guess it would help if you gave an example.
Rafael
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Dmitry Torokhov @ 2014-07-18 21:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alan Stern, Patrik Fimml, Bastien Nocera, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <1539989.Y0kuKjTTVX@vostro.rjw.lan>
On Friday, July 18, 2014 11:59:18 PM Rafael J. Wysocki wrote:
> On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> > On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> [cut]
>
> > > > I'm not sure what the appropriate action for a video camera is anyway.
> > > > Should it go away completely, including its device? Should it be
> > > > there,
> > > > but certainly not be the default choice when there is an external
> > > > camera? I'm thinking along the lines of some application's settings
> > > > dialog here, where it might be desirable to still be able to select
> > > > the
> > > > internal camera for future recordings.
> > > >
> > > > Of course, userspace could still decide simply not to
> > > > quiesce|deactivate|inhibit the device if that was desired.
> > >
> > > There's some question about how much of userspace needs to get
> > > involved. Just the daemon that manages these configuration changes, or
> > > other programs as well? I guess that's not really our problem...
> >
> > We need to provide means of implementing policy; the policy itself is not
> > really our concern ;)
> >
> > > In the end, it sounds like you're suggesting a new pair of PM
> > > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > > ->uninhibit(). Plus an optional (?) sysfs interface for invoking the
> > > callbacks.
> >
> > We do need sysfs interface so that userspace can talk to the devices in
> > question; and we also need to make sure that PM core is aware of the new
> > callbacks and provides guarantees about their interactions with system-
> > and
> > runtime-PM callbacks so that individual drivers do not have to sort it out
> > on their own.
>
> A step back, please.
>
> I have no idea why those need to be PM callbacks.
>
> What you need really seems to be a way to tell a driver "ignore input from
> this device from now on as it is most likely bogus". A natural reaction of
> the driver to that might be to stop processing input from the device and
> then runtime suspend it (and prevent it from generating remote wakeup as
> that may be bogus as well), but I don't see why the PM core needs to be
> involved in that at all.
So that we do not need to handle cases like:
- I am already in idle state and request comes to inhibit, what do I do (in
driver) or:
- I inhibited the device, now system suspend comes, what do I do? Also what do
I do at resume? I'd rather not have driver checks host of flags to figure out
the end state if PM core could spell it out for me. We already have to sort
out open/close and suspend/resume iteractions (i.e what one needs to do to
suspend or resume device that has not been opened, and if it is different from
devices that have been opened).
Thanks,
Dmitry
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 21:59 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Patrik Fimml, Bastien Nocera, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <1660498.3ZcWSfNcWh@dtor-glaptop>
On Friday, July 18, 2014 02:26:21 PM Dmitry Torokhov wrote:
> On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> > On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
[cut]
> > > I'm not sure what the appropriate action for a video camera is anyway.
> > > Should it go away completely, including its device? Should it be there,
> > > but certainly not be the default choice when there is an external
> > > camera? I'm thinking along the lines of some application's settings
> > > dialog here, where it might be desirable to still be able to select the
> > > internal camera for future recordings.
> > >
> > > Of course, userspace could still decide simply not to
> > > quiesce|deactivate|inhibit the device if that was desired.
> >
> > There's some question about how much of userspace needs to get
> > involved. Just the daemon that manages these configuration changes, or
> > other programs as well? I guess that's not really our problem...
>
> We need to provide means of implementing policy; the policy itself is not
> really our concern ;)
>
> >
> > In the end, it sounds like you're suggesting a new pair of PM
> > callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> > ->uninhibit(). Plus an optional (?) sysfs interface for invoking the
> > callbacks.
>
> We do need sysfs interface so that userspace can talk to the devices in
> question; and we also need to make sure that PM core is aware of the new
> callbacks and provides guarantees about their interactions with system- and
> runtime-PM callbacks so that individual drivers do not have to sort it out on
> their own.
A step back, please.
I have no idea why those need to be PM callbacks.
What you need really seems to be a way to tell a driver "ignore input from this
device from now on as it is most likely bogus". A natural reaction of the
driver to that might be to stop processing input from the device and then
runtime suspend it (and prevent it from generating remote wakeup as that may
be bogus as well), but I don't see why the PM core needs to be involved in
that at all.
Rafael
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Dmitry Torokhov @ 2014-07-18 21:26 UTC (permalink / raw)
To: Alan Stern
Cc: Patrik Fimml, Rafael J. Wysocki, Bastien Nocera, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <Pine.LNX.4.44L0.1407181557020.979-100000@iolanthe.rowland.org>
On Friday, July 18, 2014 04:09:46 PM Alan Stern wrote:
> On Fri, 18 Jul 2014, Patrik Fimml wrote:
> > On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > > "Quiescing" is the wrong word. "Quiescing a device" means stopping the
> > > device from doing anything, which isn't what you want. You want to
> > > ignore any activity the device may generate and reduce the device's
> > > power consumption as much as possible. A better word would be
> > > "deactivating".
> >
> > Yeah, I agree that terminology is a bit tricky here, and we have some
> > words conveying a specific idea already ("suspend"). To me, deactivating
> > suggests a more permanent condition. FWIW, I've used "inhibit" in this
> > context before and think it captures the idea well, to add another
> > choice to the list.
>
> Well, you can always reactivate a device after it has been deactivated.
> But "inhibit" is good too.
>
> > > You mentioned that handles to the device would remain open. So when
> > > the user opens the lid again, the old handles would start functioning,
> > > right?
> >
> > That's the idea, yes, and I think this would be desirable for the input
> > device class at least.
>
> For the input class, it may not matter. For example, the X server
> should deal well with a touchpad that effectively gets hot-unplugged
> and then hot-plugged later.
I thin kit would be nice if we could differentiate between unplugged
(physically gone) and disabled devices. If we shut off the touchpad does it
mean that user can't use UI to activate edge scrolling vs. two-finger
scrolling because our implementation made device to vanish?
>
> > > This has the disadvantage that the class device could not be
> > > unregistered, because doing so would invalidate the open handles.
> > > Under such circumestances, how would a userspace video program know not
> > > to list a camera built into the lid among the possible video sources
> > > (an example given earlier in this discussion)? The program would have
> > > to make an explicit test of the "deactivate" property -- it wouldn't
> > > automatically become aware that the camera wasn't available.
> > >
> > > Would it sometimes be okay to unregister the class device and
> > > invalidate the old handles, forcing programs to open new handles when
> > > the lid is opened? This would reduce the number of changes user
> > > programs would need.
> >
> > I guess we could potentially leave this for the device class to decide.
>
> Makes sense.
>
> > I'm not sure what the appropriate action for a video camera is anyway.
> > Should it go away completely, including its device? Should it be there,
> > but certainly not be the default choice when there is an external
> > camera? I'm thinking along the lines of some application's settings
> > dialog here, where it might be desirable to still be able to select the
> > internal camera for future recordings.
> >
> > Of course, userspace could still decide simply not to
> > quiesce|deactivate|inhibit the device if that was desired.
>
> There's some question about how much of userspace needs to get
> involved. Just the daemon that manages these configuration changes, or
> other programs as well? I guess that's not really our problem...
We need to provide means of implementing policy; the policy itself is not
really our concern ;)
>
> In the end, it sounds like you're suggesting a new pair of PM
> callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
> ->uninhibit(). Plus an optional (?) sysfs interface for invoking the
> callbacks.
We do need sysfs interface so that userspace can talk to the devices in
question; and we also need to make sure that PM core is aware of the new
callbacks and provides guarantees about their interactions with system- and
runtime-PM callbacks so that individual drivers do not have to sort it out on
their own.
Thanks,
Dmitry
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Alan Stern @ 2014-07-18 20:09 UTC (permalink / raw)
To: Patrik Fimml
Cc: Rafael J. Wysocki, Dmitry Torokhov, Bastien Nocera, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <20140718192334.GA10631@google.com>
On Fri, 18 Jul 2014, Patrik Fimml wrote:
> On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> > "Quiescing" is the wrong word. "Quiescing a device" means stopping the
> > device from doing anything, which isn't what you want. You want to
> > ignore any activity the device may generate and reduce the device's
> > power consumption as much as possible. A better word would be
> > "deactivating".
>
> Yeah, I agree that terminology is a bit tricky here, and we have some
> words conveying a specific idea already ("suspend"). To me, deactivating
> suggests a more permanent condition. FWIW, I've used "inhibit" in this
> context before and think it captures the idea well, to add another
> choice to the list.
Well, you can always reactivate a device after it has been deactivated.
But "inhibit" is good too.
> > You mentioned that handles to the device would remain open. So when
> > the user opens the lid again, the old handles would start functioning,
> > right?
>
> That's the idea, yes, and I think this would be desirable for the input
> device class at least.
For the input class, it may not matter. For example, the X server
should deal well with a touchpad that effectively gets hot-unplugged
and then hot-plugged later.
> > This has the disadvantage that the class device could not be
> > unregistered, because doing so would invalidate the open handles.
> > Under such circumestances, how would a userspace video program know not
> > to list a camera built into the lid among the possible video sources
> > (an example given earlier in this discussion)? The program would have
> > to make an explicit test of the "deactivate" property -- it wouldn't
> > automatically become aware that the camera wasn't available.
> >
> > Would it sometimes be okay to unregister the class device and
> > invalidate the old handles, forcing programs to open new handles when
> > the lid is opened? This would reduce the number of changes user
> > programs would need.
>
> I guess we could potentially leave this for the device class to decide.
Makes sense.
> I'm not sure what the appropriate action for a video camera is anyway.
> Should it go away completely, including its device? Should it be there,
> but certainly not be the default choice when there is an external
> camera? I'm thinking along the lines of some application's settings
> dialog here, where it might be desirable to still be able to select the
> internal camera for future recordings.
>
> Of course, userspace could still decide simply not to
> quiesce|deactivate|inhibit the device if that was desired.
There's some question about how much of userspace needs to get
involved. Just the daemon that manages these configuration changes, or
other programs as well? I guess that's not really our problem...
In the end, it sounds like you're suggesting a new pair of PM
callbacks: ->deactivate() and ->reactivate(), or ->inhibit() and
->uninhibit(). Plus an optional (?) sysfs interface for invoking the
callbacks.
Alan Stern
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Patrik Fimml @ 2014-07-18 19:23 UTC (permalink / raw)
To: Alan Stern
Cc: Patrik Fimml, Rafael J. Wysocki, Dmitry Torokhov, Bastien Nocera,
linux-pm, Benson Leung, linux-input, linux-kernel
In-Reply-To: <Pine.LNX.4.44L0.1407181439560.979-100000@iolanthe.rowland.org>
On Fri, Jul 18, 2014 at 03:00:46PM -0400, Alan Stern wrote:
> "Quiescing" is the wrong word. "Quiescing a device" means stopping the
> device from doing anything, which isn't what you want. You want to
> ignore any activity the device may generate and reduce the device's
> power consumption as much as possible. A better word would be
> "deactivating".
Yeah, I agree that terminology is a bit tricky here, and we have some
words conveying a specific idea already ("suspend"). To me, deactivating
suggests a more permanent condition. FWIW, I've used "inhibit" in this
context before and think it captures the idea well, to add another
choice to the list.
> You mentioned that handles to the device would remain open. So when
> the user opens the lid again, the old handles would start functioning,
> right?
That's the idea, yes, and I think this would be desirable for the input
device class at least.
> This has the disadvantage that the class device could not be
> unregistered, because doing so would invalidate the open handles.
> Under such circumestances, how would a userspace video program know not
> to list a camera built into the lid among the possible video sources
> (an example given earlier in this discussion)? The program would have
> to make an explicit test of the "deactivate" property -- it wouldn't
> automatically become aware that the camera wasn't available.
>
> Would it sometimes be okay to unregister the class device and
> invalidate the old handles, forcing programs to open new handles when
> the lid is opened? This would reduce the number of changes user
> programs would need.
I guess we could potentially leave this for the device class to decide.
I'm not sure what the appropriate action for a video camera is anyway.
Should it go away completely, including its device? Should it be there,
but certainly not be the default choice when there is an external
camera? I'm thinking along the lines of some application's settings
dialog here, where it might be desirable to still be able to select the
internal camera for future recordings.
Of course, userspace could still decide simply not to
quiesce|deactivate|inhibit the device if that was desired.
Regards,
Patrik
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Alan Stern @ 2014-07-18 19:00 UTC (permalink / raw)
To: Patrik Fimml
Cc: Rafael J. Wysocki, Dmitry Torokhov, Bastien Nocera, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <20140718174712.GA3156@google.com>
On Fri, 18 Jul 2014, Patrik Fimml wrote:
> 2. Give userspace a way of quiescing a device. Handles to the device
> stay open, but the device will no longer perform its function and,
> if possible, power down.
"Quiescing" is the wrong word. "Quiescing a device" means stopping the
device from doing anything, which isn't what you want. You want to
ignore any activity the device may generate and reduce the device's
power consumption as much as possible. A better word would be
"deactivating".
> For input devices, this makes a lot of sense in my opinion.
I agree.
> The notable difference to runtime PM as we have it today is that with
> runtime PM, a device typically goes to a low-power state because it is
> idle. In our scenario, we want to forcibly quiesce it even if it is not
> idle (and if it continues to provide events in a low-power mode, we
> want to ignore them instead of passing them on). So maybe integration
> with runtime PM is not the best fit.
>
> Perhaps the best way to go forward would be to add a new property for
> all devices that support quiescing so userspace has a uniform way of
> doing things, but have subsystems (and potentially the device driver)
> decide what action is appropriate to quiesce a device. Some subsystems
> might be able to provide a default implementation (e.g. input could just
> skip all events that are reported from the device driver).
You mentioned that handles to the device would remain open. So when
the user opens the lid again, the old handles would start functioning,
right?
This has the disadvantage that the class device could not be
unregistered, because doing so would invalidate the open handles.
Under such circumestances, how would a userspace video program know not
to list a camera built into the lid among the possible video sources
(an example given earlier in this discussion)? The program would have
to make an explicit test of the "deactivate" property -- it wouldn't
automatically become aware that the camera wasn't available.
Would it sometimes be okay to unregister the class device and
invalidate the old handles, forcing programs to open new handles when
the lid is opened? This would reduce the number of changes user
programs would need.
Alan Stern
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-18 18:02 UTC (permalink / raw)
To: Lennox Wu
Cc: Richard Weinberger, Arnd Bergmann, Lars-Peter Clausen,
Guenter Roeck, Greg Kroah-Hartman, Dmitry Torokhov, linux-iio,
Benjamin Herrenschmidt, Tom Gundersen, Thierry Reding,
Marek Vasut, Liqin Chen, msalter, linux-pwm, devel,
linux-watchdog, linux-input, linux-kernel@vger.kernel.org,
knaack.h, Martin Schwidefsky, Mischa.Jonker, jic23,
Geert Uytterhoeven
In-Reply-To: <CAF0htA6Vvp+d4=wPuqJJhT+FxEUoT9mbJ3_NQuEJTP-hniS-KQ@mail.gmail.com>
On 07/18/2014 11:37 PM, Lennox Wu wrote:
> Score can provide dummy functions if HAS_IOMEM and NO_IOMEM will be
> removed, even if we indeed have no IOMEM.
>
Thank you for your reply, for score, your ideas is OK to me.
And for the COMPILE_TEST needs still discussing below:
> 2014-07-18 18:51 GMT+08:00 Richard Weinberger <richard@nod.at>:
>> Am 18.07.2014 12:44, schrieb Chen Gang:
>>> On 07/18/2014 03:35 PM, Richard Weinberger wrote:
>>>> Am 18.07.2014 02:36, schrieb Chen Gang:
>>>>>
>>>>> On 07/18/2014 02:09 AM, Richard Weinberger wrote:
>>>>>> Am 17.07.2014 12:48, schrieb Arnd Bergmann:
>>>>>>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>>>>>>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>>>>>>> to build on UML seems pointless to me and we special-case it in a number of
>>>>>>> places already.
>>>>>>
>>>>>> If UML is the only arch without io memory the dependency on !UML seems
>>>>>> reasonable to me. :-)
>>>>>>
>>>>>
>>>>> For me, if only uml left, I suggest to implement dummy functions within
>>>>> uml instead of let CONFIG_UML appear in generic include directory. And
>>>>> then remove all HAS_IOMEM and NO_IOMEM from kernel.
>>>>
>>>> Erm, this is something completely different.
>>>> I thought we're focusing on COMPILE_TEST?
>>>>
>>>
>>> COMPILE_TEST is none-architecture specific, but UML is. So in generic
>>> include folder, if we're focusing on choosing whether COMPILE_TEST or
>>> UML, for me, I will choose COMPILE_TEST.
>>>
>>> If we're not only focusing on COMPILE_TEST, for me, if something only
>>> depend on one architecture, I'd like to put them under "arch/*/" folder.
>>>
>>> Especially, after that, we can remove all HAS_IOMEM and NO_IOMEM, nobody
>>> has to think of them again. :-)
>>
>> And then we end up with a solution that on UML a lot of completely useless
>> drivers are build which fail in various interesting manners because you'll
>> add stubs for all kinds of io memory related functions to arch/um/?
>> We had this kind of discussion already. You'll need more than ioremap...
>>
>> I like Arnd's idea *much* more to make COMPILE_TEST depend on !UML.
>>
That will let UML itself against COMPILE_TEST (but all the other
architectures not).
And if let COMPILE_TEST depend on !UML, can we still remove all
HAS_IOMEM and NO_IOMEM from kernel? (I guess so).
If we can remove them, we can send related patch firstly -- that will
let current discussion be in UML architecture wide instead of kernel
wide.
Thanks.
--
Chen Gang
Open share and attitude like air water and life which God blessed
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Patrik Fimml @ 2014-07-18 17:47 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Dmitry Torokhov, Alan Stern, Bastien Nocera, Patrik Fimml,
linux-pm, Benson Leung, linux-input, linux-kernel
In-Reply-To: <8587268.sIIBz6Y2QN@vostro.rjw.lan>
On Fri, Jul 18, 2014 at 02:43:02AM +0200, Rafael J. Wysocki wrote:
> From past discussions on similar topics it followed that there really was
> no generic way for individual drivers to quiesce devices on demand as long as
> user space was running. Everything we could come up with was racy, this way or
> another. That is the reason for using the freezer during system suspend. In
> other words, if you want drivers to quiesce devices by force, you need to quiesce
> user space by force to start with - for example by freezing it.
Can you give me some pointers?
> For runtime PM, on the other hand, the underlying observation is that drivers
> should be able to detect when devices are already quiescent and initialize power
> transitions at those points. It's role is to help with that, but not with quiescing
> things.
>
> That said, in the "laptop lid closed" scenario (assuming that the system is not
> supposed to suspend in response to that, which in my opinion is the best approach)
> the problem really seems to be that drivers are not aggressive enough with starting
> PM transitions (using runtime PM) when they see no activity. Thus it seems that
> when the lid is closed, it'll be good to switch the drivers into a "more aggressive
> runtime PM mode" in which they will use any opportunity to start a power transition
> without worrying about extra latencies resulting from that. In that mode they
> should also disable remote wakeup. I think this should be sufficient to address
> the use case at hand.
Let me try to clear things up a little.
Of course, in most cases, a laptop would enter suspend after closing the
lid. But you can think of scenarios where the laptop would stay on:
- Your laptop is docked, and you use an external monitor, but you don't
want to have your laptop open (maybe due to lack of space).
- You have your laptop in your backpack and use it as a music player.
In those scenarios, you wouldn't want to see bogus input (our worst-case
example is the mouse pointer being over the 1-Click Buy button on
Amazon - stray clicks that result from the laptop lid interfering with
the touchpad wouldn't be a good thing).
Now there are basically two approaches with pros and cons:
1. Get userspace to close handles to all such devices while the lid is
closed, and
1a. Have drivers with sufficiently aggressive PM so that devices are
actually going to a low-power/off state when userspace doesn't have
any handles open.
This can be done with the infrastructure that we have in the kernel
today, although drivers might have to be improved in order to actually
get devices to power off.
2. Give userspace a way of quiescing a device. Handles to the device
stay open, but the device will no longer perform its function and,
if possible, power down.
For input devices, this makes a lot of sense in my opinion.
The notable difference to runtime PM as we have it today is that with
runtime PM, a device typically goes to a low-power state because it is
idle. In our scenario, we want to forcibly quiesce it even if it is not
idle (and if it continues to provide events in a low-power mode, we
want to ignore them instead of passing them on). So maybe integration
with runtime PM is not the best fit.
Perhaps the best way to go forward would be to add a new property for
all devices that support quiescing so userspace has a uniform way of
doing things, but have subsystems (and potentially the device driver)
decide what action is appropriate to quiesce a device. Some subsystems
might be able to provide a default implementation (e.g. input could just
skip all events that are reported from the device driver).
Regards,
Patrik
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Lennox Wu @ 2014-07-18 15:37 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-iio, Benjamin Herrenschmidt, Tom Gundersen, Thierry Reding,
linux-watchdog, Marek Vasut, Liqin Chen, Lars-Peter Clausen,
Geert Uytterhoeven, msalter, Guenter Roeck, linux-pwm, devel,
Chen Gang, Arnd Bergmann, linux-input, Greg Kroah-Hartman,
Dmitry Torokhov, linux-kernel@vger.kernel.org, knaack.h,
Martin Schwidefsky, Mischa.Jonker, jic23
In-Reply-To: <53C8FC1E.9090003@nod.at>
Score can provide dummy functions if HAS_IOMEM and NO_IOMEM will be
removed, even if we indeed have no IOMEM.
Best,
Lennox
2014-07-18 18:51 GMT+08:00 Richard Weinberger <richard@nod.at>:
> Am 18.07.2014 12:44, schrieb Chen Gang:
>> On 07/18/2014 03:35 PM, Richard Weinberger wrote:
>>> Am 18.07.2014 02:36, schrieb Chen Gang:
>>>>
>>>> On 07/18/2014 02:09 AM, Richard Weinberger wrote:
>>>>> Am 17.07.2014 12:48, schrieb Arnd Bergmann:
>>>>>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>>>>>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>>>>>> to build on UML seems pointless to me and we special-case it in a number of
>>>>>> places already.
>>>>>
>>>>> If UML is the only arch without io memory the dependency on !UML seems
>>>>> reasonable to me. :-)
>>>>>
>>>>
>>>> For me, if only uml left, I suggest to implement dummy functions within
>>>> uml instead of let CONFIG_UML appear in generic include directory. And
>>>> then remove all HAS_IOMEM and NO_IOMEM from kernel.
>>>
>>> Erm, this is something completely different.
>>> I thought we're focusing on COMPILE_TEST?
>>>
>>
>> COMPILE_TEST is none-architecture specific, but UML is. So in generic
>> include folder, if we're focusing on choosing whether COMPILE_TEST or
>> UML, for me, I will choose COMPILE_TEST.
>>
>> If we're not only focusing on COMPILE_TEST, for me, if something only
>> depend on one architecture, I'd like to put them under "arch/*/" folder.
>>
>> Especially, after that, we can remove all HAS_IOMEM and NO_IOMEM, nobody
>> has to think of them again. :-)
>
> And then we end up with a solution that on UML a lot of completely useless
> drivers are build which fail in various interesting manners because you'll
> add stubs for all kinds of io memory related functions to arch/um/?
> We had this kind of discussion already. You'll need more than ioremap...
>
> I like Arnd's idea *much* more to make COMPILE_TEST depend on !UML.
>
> Thanks,
> //richard
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Alan Stern @ 2014-07-18 15:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Dmitry Torokhov, Bastien Nocera, Patrik Fimml, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <2674541.jV2qHCZlHf@vostro.rjw.lan>
On Fri, 18 Jul 2014, Rafael J. Wysocki wrote:
> > > the problem really seems to be that drivers are not
> > > aggressive enough with starting PM transitions (using runtime PM) when they
> > > see no activity. Thus it seems that when the lid is closed, it'll be good
> > > to switch the drivers into a "more aggressive runtime PM mode" in which
> > > they will use any opportunity to start a power transition without worrying
> > > about extra latencies resulting from that. In that mode they should also
> > > disable remote wakeup. I think this should be sufficient to address the
> > > use case at hand.
> >
> > OK, so how do we let the drivers know that they should start being aggressive
> > with PM and that they should disable remote wakeup? I'd rather not add
> > subsystem-specific interface for this, that is why we are reaching out in the
> > first place.
>
> For disabling remote wakeup we have a PM QoS flag that I'm not entirely happy
> with, so I guess we can replace it with something saner (I talked about that
> with Alan during the last year's LinuxCon, but then didn't have the time to
> get to that).
Right. We discussed changing .../power/wakeup so that it could contain
"enabled", "disabled", or "off", where "off" would mean remote wakeup
should be disabled even during runtime suspend.
> For the whole thing I guess we can add a sysfs attribute under devices/.../power
> that will need to be exposed by drivers supporting that feature. I'm not sure
> how to call it, though.
"runtime_mode"?
Will this really be capable of doing what Dmitry wants? I don't know
how the drivers in question work. But if a driver increments the
runtime usage count each time the device file is opened, forcibly
setting the usage count back to 0 won't be easy.
Also, would putting the camera into runtime suspend prevent it from
showing up on a list of available video devices? I doubt it. More
likely, the video driver would have to unregister the class device
while remaining bound to the physical device. Probably other drivers
would have to do the same sort of thing. (I don't know whether doing
this would retain the settings as Oliver wants, though.)
Alan Stern
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox