[PATCH v5 0/4] new driver for Valve Steam Controller

[PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

This patchset implements a driver for Valve Steam Controller, based on a
reverse analysis by myself.

Sorry, I've been out of town for a few weeks and couldn't keep up with this...

@Pierre-Loup and @Clément, could you please have another look at this and
check if it is worthy? Benjamin will not commit it without an express ACK from
Valve. Of course he is right to be cautious, but I checked this driver with
the Steam Client and all seems to go just fine. I think that there is a lot of
Linux out of the desktop that could use this driver and cannot use the Steam
Client. Worst case scenario, this driver can now be blacklisted, but I hope
that will not be needed.

For full reference, I'm adding a full changelog of this patchset.

Changes in v5:
 * Fix license SPDX to GPL-2.0+.
 * Minor stylistic changes (BIT(3) instead 0x08 and so on).

Changes in v4:
 * Add command to check the wireless connection status on probe, without
   waiting for a message (thanks to Clément Vuchener for the tip).
 * Removed the error code on redundant connection/disconnection messages. That
   was harmless but polluted dmesg.
 * Added buttons for touching the left-pad and right-pad.
 * Fixed a misplaced #include from 2/4 to 1/4.

Changes in v3:
 * Use RCU to do the dynamic connec/disconnect of wireless devices.
 * Remove entries in hid-quirks.c as they are no longer needed. This allows
   this module to be blacklisted without side effects.
 * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
   existing use cases (lizard mode). A user-space tool to do that is
   linked.
 * Fully separated axes for joystick and left-pad. As it happens.
 * Add fuzz values for left/right pad axes, they are a little wiggly.

Changes in v2:
 * Remove references to USB. Now the interesting interfaces are selected by
   looking for the ones with feature reports.
 * Feature reports buffers are allocated with hid_alloc_report_buf().
 * Feature report length is checked, to avoid overflows in case of
   corrupt/malicius USB devices.
 * Resolution added to the ABS axes.
 * A lot of minor cleanups.

Rodrigo Rivas Costa (4):
  HID: add driver for Valve Steam Controller
  HID: steam: add serial number information.
  HID: steam: command to check wireless connection
  HID: steam: add battery device.

 drivers/hid/Kconfig     |   8 +
 drivers/hid/Makefile    |   1 +
 drivers/hid/hid-ids.h   |   4 +
 drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 807 insertions(+)
 create mode 100644 drivers/hid/hid-steam.c

-- 
2.16.2

[PATCH v5 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

There are two ways to connect the Steam Controller: directly to the USB
or with the USB wireless adapter.  Both methods are similar, but the
wireless adapter can connect up to 4 devices at the same time.

The wired device will appear as 3 interfaces: a virtual mouse, a virtual
keyboard and a custom HID device.

The wireless device will appear as 5 interfaces: a virtual keyboard and
4 custom HID devices, that will remain silent until a device is actually
connected.

The custom HID device has a report descriptor with all vendor specific
usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
Steam Client provices a software translation by using direct USB access
and a creates a uinput virtual gamepad.

This driver was reverse engineered to provide direct kernel support in
case you cannot, or do not want to, use Valve Steam Client. It disables
the virtual keyboard and mouse, as they are not so useful when you have
a working gamepad.

Working: buttons, axes, pads, wireless connect/disconnect.

TO-DO: Battery, force-feedback, accelerometer/gyro, led, beeper...

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
 drivers/hid/Kconfig     |   8 +
 drivers/hid/Makefile    |   1 +
 drivers/hid/hid-ids.h   |   4 +
 drivers/hid/hid-steam.c | 537 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 550 insertions(+)
 create mode 100644 drivers/hid/hid-steam.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6e80fbf04e03 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -823,6 +823,14 @@ config HID_SPEEDLINK
 	---help---
 	Support for Speedlink Vicious and Divine Cezanne mouse.
 
+config HID_STEAM
+	tristate "Steam Controller support"
+	depends on HID
+	---help---
+	Say Y here if you have a Steam Controller if you want to use it
+	without running the Steam Client. It supports both the wired and
+	the wireless adaptor.
+
 config HID_STEELSERIES
 	tristate "Steelseries SRW-S1 steering wheel support"
 	depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92d85..60a8abf84682 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_HID_SAMSUNG)	+= hid-samsung.o
 obj-$(CONFIG_HID_SMARTJOYPLUS)	+= hid-sjoy.o
 obj-$(CONFIG_HID_SONY)		+= hid-sony.o
 obj-$(CONFIG_HID_SPEEDLINK)	+= hid-speedlink.o
+obj-$(CONFIG_HID_STEAM)		+= hid-steam.o
 obj-$(CONFIG_HID_STEELSERIES)	+= hid-steelseries.o
 obj-$(CONFIG_HID_SUNPLUS)	+= hid-sunplus.o
 obj-$(CONFIG_HID_GREENASIA)	+= hid-gaff.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 43ddcdfbd0da..be31a3c20818 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -988,6 +988,10 @@
 #define USB_VENDOR_ID_STANTUM_SITRONIX		0x1403
 #define USB_DEVICE_ID_MTP_SITRONIX		0x5001
 
+#define USB_VENDOR_ID_VALVE			0x28de
+#define USB_DEVICE_ID_STEAM_CONTROLLER		0x1102
+#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS	0x1142
+
 #define USB_VENDOR_ID_STEELSERIES	0x1038
 #define USB_DEVICE_ID_STEELSERIES_SRWS1	0x1410
 
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
new file mode 100644
index 000000000000..bc8b706311a9
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,537 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * HID driver for Valve Steam Controller
+ *
+ * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
+ *
+ * Supports both the wired and wireless interfaces.
+ *
+ * For additional functions, such as disabling the virtual mouse/keyboard or
+ * changing the right-pad margin you can use the user-space tool at:
+ *
+ *   https://github.com/rodrigorc/steamctrl
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
+#include "hid-ids.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
+
+#define STEAM_QUIRK_WIRELESS		BIT(0)
+
+/* Touch pads are 40 mm in diameter and 65535 units */
+#define STEAM_PAD_RESOLUTION 1638
+/* Trigger runs are about 5 mm and 256 units */
+#define STEAM_TRIGGER_RESOLUTION 51
+/* Joystick runs are about 5 mm and 256 units */
+#define STEAM_JOYSTICK_RESOLUTION 51
+
+#define STEAM_PAD_FUZZ 256
+
+struct steam_device {
+	spinlock_t lock;
+	struct hid_device *hdev;
+	struct input_dev __rcu *input;
+	unsigned long quirks;
+	struct work_struct work_connect;
+	bool connected;
+};
+
+static int steam_input_open(struct input_dev *dev)
+{
+	struct steam_device *steam = input_get_drvdata(dev);
+
+	return hid_hw_open(steam->hdev);
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+	struct steam_device *steam = input_get_drvdata(dev);
+
+	hid_hw_close(steam->hdev);
+}
+
+static int steam_register(struct steam_device *steam)
+{
+	struct hid_device *hdev = steam->hdev;
+	struct input_dev *input;
+	int ret;
+
+	rcu_read_lock();
+	input = rcu_dereference(steam->input);
+	rcu_read_unlock();
+	if (input) {
+		dbg_hid("%s: already connected\n", __func__);
+		return 0;
+	}
+
+	hid_info(hdev, "Steam Controller connected");
+
+	input = input_allocate_device();
+	if (!input)
+		return -ENOMEM;
+
+	input_set_drvdata(input, steam);
+	input->dev.parent = &hdev->dev;
+	input->open = steam_input_open;
+	input->close = steam_input_close;
+
+	input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
+		"Wireless Steam Controller" :
+		"Steam Controller";
+	input->phys = hdev->phys;
+	input->uniq = hdev->uniq;
+	input->id.bustype = hdev->bus;
+	input->id.vendor = hdev->vendor;
+	input->id.product = hdev->product;
+	input->id.version = hdev->version;
+
+	input_set_capability(input, EV_KEY, BTN_TR2);
+	input_set_capability(input, EV_KEY, BTN_TL2);
+	input_set_capability(input, EV_KEY, BTN_TR);
+	input_set_capability(input, EV_KEY, BTN_TL);
+	input_set_capability(input, EV_KEY, BTN_Y);
+	input_set_capability(input, EV_KEY, BTN_B);
+	input_set_capability(input, EV_KEY, BTN_X);
+	input_set_capability(input, EV_KEY, BTN_A);
+	input_set_capability(input, EV_KEY, BTN_SELECT);
+	input_set_capability(input, EV_KEY, BTN_MODE);
+	input_set_capability(input, EV_KEY, BTN_START);
+	input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
+	input_set_capability(input, EV_KEY, BTN_GEAR_UP);
+	input_set_capability(input, EV_KEY, BTN_THUMBR);
+	input_set_capability(input, EV_KEY, BTN_THUMBL);
+	input_set_capability(input, EV_KEY, BTN_THUMB);
+	input_set_capability(input, EV_KEY, BTN_THUMB2);
+
+	input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
+	input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
+	input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
+	input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
+	input_set_abs_params(input, ABS_RX, -32767, 32767,
+			STEAM_PAD_FUZZ, 0);
+	input_set_abs_params(input, ABS_RY, -32767, 32767,
+			STEAM_PAD_FUZZ, 0);
+	input_set_abs_params(input, ABS_HAT1X, -32767, 32767,
+			STEAM_PAD_FUZZ, 0);
+	input_set_abs_params(input, ABS_HAT1Y, -32767, 32767,
+			STEAM_PAD_FUZZ, 0);
+	input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0);
+	input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0);
+	input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
+	input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
+	input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
+	input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
+	input_abs_set_res(input, ABS_HAT1X, STEAM_PAD_RESOLUTION);
+	input_abs_set_res(input, ABS_HAT1Y, STEAM_PAD_RESOLUTION);
+	input_abs_set_res(input, ABS_Z, STEAM_TRIGGER_RESOLUTION);
+	input_abs_set_res(input, ABS_RZ, STEAM_TRIGGER_RESOLUTION);
+
+	ret = input_register_device(input);
+	if (ret)
+		goto input_register_fail;
+
+	rcu_assign_pointer(steam->input, input);
+
+	return 0;
+
+input_register_fail:
+	input_free_device(input);
+	return ret;
+}
+
+static void steam_unregister(struct steam_device *steam)
+{
+	struct input_dev *input;
+
+	rcu_read_lock();
+	input = rcu_dereference(steam->input);
+	rcu_read_unlock();
+
+	if (input) {
+		RCU_INIT_POINTER(steam->input, NULL);
+		synchronize_rcu();
+		hid_info(steam->hdev, "Steam Controller disconnected");
+		input_unregister_device(input);
+	}
+}
+
+static void steam_work_connect_cb(struct work_struct *work)
+{
+	struct steam_device *steam = container_of(work, struct steam_device,
+							work_connect);
+	unsigned long flags;
+	bool connected;
+	int ret;
+
+	spin_lock_irqsave(&steam->lock, flags);
+	connected = steam->connected;
+	spin_unlock_irqrestore(&steam->lock, flags);
+
+	if (connected) {
+		ret = steam_register(steam);
+		if (ret) {
+			hid_err(steam->hdev,
+				"%s:steam_register failed with error %d\n",
+				__func__, ret);
+		}
+	} else {
+		steam_unregister(steam);
+	}
+}
+
+static bool steam_is_valve_interface(struct hid_device *hdev)
+{
+	struct hid_report_enum *rep_enum;
+
+	/*
+	 * The wired device creates 3 interfaces:
+	 *  0: emulated mouse.
+	 *  1: emulated keyboard.
+	 *  2: the real game pad.
+	 * The wireless device creates 5 interfaces:
+	 *  0: emulated keyboard.
+	 *  1-4: slots where up to 4 real game pads will be connected to.
+	 * We know which one is the real gamepad interface because they are the
+	 * only ones with a feature report.
+	 */
+	rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
+	return !list_empty(&rep_enum->report_list);
+}
+
+static int steam_probe(struct hid_device *hdev,
+				const struct hid_device_id *id)
+{
+	struct steam_device *steam;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev,
+			"%s:parse of hid interface failed\n", __func__);
+		return ret;
+	}
+
+	/*
+	 * Connect the HID device so that Steam Controller keeps on working
+	 * without changes.
+	 */
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret) {
+		hid_err(hdev,
+			"%s:hid_hw_start failed with error %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	if (!steam_is_valve_interface(hdev))
+		return 0;
+
+	/*
+	 * From this point on, if anything fails return 0 and ignores
+	 * the error, so that the default HID devices are still bound.
+	 */
+	steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
+	if (!steam) {
+		ret = -ENOMEM;
+		goto mem_fail;
+	}
+
+	spin_lock_init(&steam->lock);
+	steam->hdev = hdev;
+	hid_set_drvdata(hdev, steam);
+	steam->quirks = id->driver_data;
+	INIT_WORK(&steam->work_connect, steam_work_connect_cb);
+
+	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+		ret = hid_hw_open(hdev);
+		if (ret) {
+			hid_err(hdev,
+				"%s:hid_hw_open for wireless\n",
+				__func__);
+			goto hid_hw_open_fail;
+		}
+		hid_info(hdev, "Steam wireless receiver connected");
+	} else {
+		ret = steam_register(steam);
+		if (ret) {
+			hid_err(hdev,
+				"%s:steam_register failed with error %d\n",
+				__func__, ret);
+			goto input_register_fail;
+		}
+	}
+
+	return 0;
+
+input_register_fail:
+hid_hw_open_fail:
+	cancel_work_sync(&steam->work_connect);
+	hid_set_drvdata(hdev, NULL);
+mem_fail:
+	hid_err(hdev, "%s: failed with error %d\n",
+			__func__, ret);
+	return 0;
+}
+
+static void steam_remove(struct hid_device *hdev)
+{
+	struct steam_device *steam = hid_get_drvdata(hdev);
+
+	if (!steam) {
+		hid_hw_stop(hdev);
+		return;
+	}
+
+	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+		hid_info(hdev, "Steam wireless receiver disconnected");
+		hid_hw_close(hdev);
+	}
+	hid_hw_stop(hdev);
+	cancel_work_sync(&steam->work_connect);
+	steam_unregister(steam);
+}
+
+static void steam_do_connect_event(struct steam_device *steam, bool connected)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&steam->lock, flags);
+	steam->connected = connected;
+	spin_unlock_irqrestore(&steam->lock, flags);
+
+	if (schedule_work(&steam->work_connect) == 0)
+		dbg_hid("%s: connected=%d event already queued\n",
+				__func__, connected);
+}
+
+/*
+ * The size for this message payload is 60.
+ * The known values are:
+ *  (* values are not sent through wireless)
+ *  (* accelerator/gyro is disabled by default)
+ *  Offset| Type  | Mapped to |Meaning
+ * -------+-------+-----------+--------------------------
+ *  4-7   | u32   | --        | sequence number
+ *  8-10  | 24bit | see below | buttons
+ *  11    | u8    | ABS_Z     | left trigger
+ *  12    | u8    | ABS_RZ    | right trigger
+ *  13-15 | --    | --        | always 0
+ *  16-17 | s16   | ABS_X     | X value
+ *  18-19 | s16   | ABS_Y     | Y value
+ *  20-21 | s16   | ABS_RX    | right-pad X value
+ *  22-23 | s16   | ABS_RY    | right-pad Y value
+ *  24-25 | s16   | --        | * left trigger
+ *  26-27 | s16   | --        | * right trigger
+ *  28-29 | s16   | --        | * accelerometer X value
+ *  30-31 | s16   | --        | * accelerometer Y value
+ *  32-33 | s16   | --        | * accelerometer Z value
+ *  34-35 | s16   | --        | gyro X value
+ *  36-36 | s16   | --        | gyro Y value
+ *  38-39 | s16   | --        | gyro Z value
+ *  40-41 | s16   | --        | quaternion W value
+ *  42-43 | s16   | --        | quaternion X value
+ *  44-45 | s16   | --        | quaternion Y value
+ *  46-47 | s16   | --        | quaternion Z value
+ *  48-49 | --    | --        | always 0
+ *  50-51 | s16   | --        | * left trigger (uncalibrated)
+ *  52-53 | s16   | --        | * right trigger (uncalibrated)
+ *  54-55 | s16   | --        | * joystick X value (uncalibrated)
+ *  56-57 | s16   | --        | * joystick Y value (uncalibrated)
+ *  58-59 | s16   | --        | * left-pad X value
+ *  60-61 | s16   | --        | * left-pad Y value
+ *  62-63 | u16   | --        | * battery voltage
+ *
+ * The buttons are:
+ *  Bit  | Mapped to  | Description
+ * ------+------------+--------------------------------
+ *  8.0  | BTN_TR2    | right trigger fully pressed
+ *  8.1  | BTN_TL2    | left trigger fully pressed
+ *  8.2  | BTN_TR     | right shoulder
+ *  8.3  | BTN_TL     | left shoulder
+ *  8.4  | BTN_Y      | button Y
+ *  8.5  | BTN_B      | button B
+ *  8.6  | BTN_X      | button X
+ *  8.7  | BTN_A      | button A
+ *  9.0  | -ABS_HAT0Y | lef-pad up
+ *  9.1  | +ABS_HAT0X | lef-pad right
+ *  9.2  | -ABS_HAT0X | lef-pad left
+ *  9.3  | +ABS_HAT0Y | lef-pad down
+ *  9.4  | BTN_SELECT | menu left
+ *  9.5  | BTN_MODE   | steam logo
+ *  9.6  | BTN_START  | menu right
+ *  9.7  | BTN_GEAR_DOWN | left back lever
+ * 10.0  | BTN_GEAR_UP   | right back lever
+ * 10.1  | --         | left-pad clicked
+ * 10.2  | BTN_THUMBR | right-pad clicked
+ * 10.3  | BTN_THUMB  | left-pad touched (but see explanation below)
+ * 10.4  | BTN_THUMB2 | right-pad touched
+ * 10.5  | --         | unknown
+ * 10.6  | BTN_THUMBL | joystick clicked
+ * 10.7  | --         | lpad_and_joy
+ */
+
+static void steam_do_input_event(struct steam_device *steam,
+		struct input_dev *input, u8 *data)
+{
+	/* 24 bits of buttons */
+	u8 b8, b9, b10;
+	bool lpad_touched, lpad_and_joy;
+
+	b8 = data[8];
+	b9 = data[9];
+	b10 = data[10];
+
+	input_report_abs(input, ABS_Z, data[11]);
+	input_report_abs(input, ABS_RZ, data[12]);
+
+	/*
+	 * These two bits tells how to interpret the values X and Y.
+	 * lpad_and_joy tells that the joystick and the lpad are used at the
+	 * same time.
+	 * lpad_touched tells whether X/Y are to be read as lpad coord or
+	 * joystick values.
+	 * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
+	 */
+	lpad_touched = b10 & BIT(3);
+	lpad_and_joy = b10 & BIT(7);
+	input_report_abs(input, lpad_touched ? ABS_HAT1X : ABS_X,
+			(s16) le16_to_cpup((__le16 *)(data + 16)));
+	input_report_abs(input, lpad_touched ? ABS_HAT1Y : ABS_Y,
+			-(s16) le16_to_cpup((__le16 *)(data + 18)));
+	/* Check if joystick is centered */
+	if (lpad_touched && !lpad_and_joy) {
+		input_report_abs(input, ABS_X, 0);
+		input_report_abs(input, ABS_Y, 0);
+	}
+	/* Check if lpad is untouched */
+	if (!(lpad_touched || lpad_and_joy)) {
+		input_report_abs(input, ABS_HAT1X, 0);
+		input_report_abs(input, ABS_HAT1Y, 0);
+	}
+
+	input_report_abs(input, ABS_RX,
+			(s16) le16_to_cpup((__le16 *)(data + 20)));
+	input_report_abs(input, ABS_RY,
+			-(s16) le16_to_cpup((__le16 *)(data + 22)));
+
+	input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
+	input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
+	input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
+	input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
+	input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
+	input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
+	input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
+	input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
+	input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
+	input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
+	input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
+	input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
+	input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
+	input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
+	input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
+	input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
+	input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
+
+	input_report_abs(input, ABS_HAT0X,
+			!!(b9 & BIT(1)) - !!(b9 & BIT(2)));
+	input_report_abs(input, ABS_HAT0Y,
+			!!(b9 & BIT(3)) - !!(b9 & BIT(0)));
+
+	input_sync(input);
+}
+
+static int steam_raw_event(struct hid_device *hdev,
+			struct hid_report *report, u8 *data,
+			int size)
+{
+	struct steam_device *steam = hid_get_drvdata(hdev);
+	struct input_dev *input;
+
+	if (!steam)
+		return 0;
+
+	/*
+	 * All messages are size=64, all values little-endian.
+	 * The format is:
+	 *  Offset| Meaning
+	 * -------+--------------------------------------------
+	 *  0-1   | always 0x01, 0x00, maybe protocol version?
+	 *  2     | type of message
+	 *  3     | length of the real payload (not checked)
+	 *  4-n   | payload data, depends on the type
+	 *
+	 * There are these known types of message:
+	 *  0x01: input data (60 bytes)
+	 *  0x03: wireless connect/disconnect (1 byte)
+	 *  0x04: battery status (11 bytes)
+	 */
+
+	if (size != 64 || data[0] != 1 || data[1] != 0)
+		return 0;
+
+	switch (data[2]) {
+	case 0x01:
+		rcu_read_lock();
+		input = rcu_dereference(steam->input);
+		if (likely(input)) {
+			steam_do_input_event(steam, input, data);
+		} else {
+			dbg_hid("%s: input data without connect event\n",
+					__func__);
+			steam_do_connect_event(steam, true);
+		}
+		rcu_read_unlock();
+		break;
+	case 0x03:
+		/*
+		 * The payload of this event is a single byte:
+		 *  0x01: disconnected.
+		 *  0x02: connected.
+		 */
+		switch (data[4]) {
+		case 0x01:
+			steam_do_connect_event(steam, false);
+			break;
+		case 0x02:
+			steam_do_connect_event(steam, true);
+			break;
+		}
+		break;
+	case 0x04:
+		/* TODO battery status */
+		break;
+	}
+	return 0;
+}
+
+static const struct hid_device_id steam_controllers[] = {
+	{ /* Wired Steam Controller */
+	  HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+		USB_DEVICE_ID_STEAM_CONTROLLER)
+	},
+	{ /* Wireless Steam Controller */
+	  HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+		USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
+	  .driver_data = STEAM_QUIRK_WIRELESS
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(hid, steam_controllers);
+
+static struct hid_driver steam_controller_driver = {
+	.name = "hid-steam",
+	.id_table = steam_controllers,
+	.probe = steam_probe,
+	.remove = steam_remove,
+	.raw_event = steam_raw_event,
+};
+
+module_hid_driver(steam_controller_driver);
-- 
2.16.2

[PATCH v5 2/4] HID: steam: add serial number information.

[Rodrigo Rivas Costa]

This device has a feature report to send and receive commands.
Use it to get the serial number and set the device's uniq value.

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
 drivers/hid/hid-steam.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index bc8b706311a9..03eebc4c28ba 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/workqueue.h>
 #include <linux/rcupdate.h>
+#include <linux/delay.h>
 #include "hid-ids.h"
 
 MODULE_LICENSE("GPL");
@@ -25,6 +26,7 @@ MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
 
 #define STEAM_QUIRK_WIRELESS		BIT(0)
 
+#define STEAM_SERIAL_LEN 10
 /* Touch pads are 40 mm in diameter and 65535 units */
 #define STEAM_PAD_RESOLUTION 1638
 /* Trigger runs are about 5 mm and 256 units */
@@ -41,8 +43,102 @@ struct steam_device {
 	unsigned long quirks;
 	struct work_struct work_connect;
 	bool connected;
+	char serial_no[STEAM_SERIAL_LEN + 1];
 };
 
+static int steam_recv_report(struct steam_device *steam,
+		u8 *data, int size)
+{
+	struct hid_report *r;
+	u8 *buf;
+	int ret;
+
+	r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
+	if (hid_report_len(r) < 64)
+		return -EINVAL;
+
+	buf = hid_alloc_report_buf(r, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/*
+	 * The report ID is always 0, so strip the first byte from the output.
+	 * hid_report_len() is not counting the report ID, so +1 to the length
+	 * or else we get a EOVERFLOW. We are safe from a buffer overflow
+	 * because hid_alloc_report_buf() allocates +7 bytes.
+	 */
+	ret = hid_hw_raw_request(steam->hdev, 0x00,
+			buf, hid_report_len(r) + 1,
+			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret > 0)
+		memcpy(data, buf + 1, min(size, ret - 1));
+	kfree(buf);
+	return ret;
+}
+
+static int steam_send_report(struct steam_device *steam,
+		u8 *cmd, int size)
+{
+	struct hid_report *r;
+	u8 *buf;
+	unsigned int retries = 10;
+	int ret;
+
+	r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
+	if (hid_report_len(r) < 64)
+		return -EINVAL;
+
+	buf = hid_alloc_report_buf(r, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* The report ID is always 0 */
+	memcpy(buf + 1, cmd, size);
+
+	/*
+	 * Sometimes the wireless controller fails with EPIPE
+	 * when sending a feature report.
+	 * Doing a HID_REQ_GET_REPORT and waiting for a while
+	 * seems to fix that.
+	 */
+	do {
+		ret = hid_hw_raw_request(steam->hdev, 0,
+				buf, size + 1,
+				HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+		if (ret != -EPIPE)
+			break;
+		steam_recv_report(steam, NULL, 0);
+		msleep(50);
+	} while (--retries);
+
+	kfree(buf);
+	if (ret < 0)
+		hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
+				ret, size, cmd);
+	return ret;
+}
+
+static int steam_get_serial(struct steam_device *steam)
+{
+	/*
+	 * Send: 0xae 0x15 0x01
+	 * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
+	 */
+	int ret;
+	u8 cmd[] = {0xae, 0x15, 0x01};
+	u8 reply[3 + STEAM_SERIAL_LEN + 1];
+
+	ret = steam_send_report(steam, cmd, sizeof(cmd));
+	if (ret < 0)
+		return ret;
+	ret = steam_recv_report(steam, reply, sizeof(reply));
+	if (ret < 0)
+		return ret;
+	reply[3 + STEAM_SERIAL_LEN] = 0;
+	strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
+	return 0;
+}
+
 static int steam_input_open(struct input_dev *dev)
 {
 	struct steam_device *steam = input_get_drvdata(dev);
@@ -71,7 +167,12 @@ static int steam_register(struct steam_device *steam)
 		return 0;
 	}
 
-	hid_info(hdev, "Steam Controller connected");
+	ret = steam_get_serial(steam);
+	if (ret)
+		return ret;
+
+	hid_info(hdev, "Steam Controller '%s' connected",
+			steam->serial_no);
 
 	input = input_allocate_device();
 	if (!input)
@@ -86,7 +187,7 @@ static int steam_register(struct steam_device *steam)
 		"Wireless Steam Controller" :
 		"Steam Controller";
 	input->phys = hdev->phys;
-	input->uniq = hdev->uniq;
+	input->uniq = steam->serial_no;
 	input->id.bustype = hdev->bus;
 	input->id.vendor = hdev->vendor;
 	input->id.product = hdev->product;
@@ -157,7 +258,8 @@ static void steam_unregister(struct steam_device *steam)
 	if (input) {
 		RCU_INIT_POINTER(steam->input, NULL);
 		synchronize_rcu();
-		hid_info(steam->hdev, "Steam Controller disconnected");
+		hid_info(steam->hdev, "Steam Controller '%s' disconnected",
+				steam->serial_no);
 		input_unregister_device(input);
 	}
 }
-- 
2.16.2

[PATCH v5 3/4] HID: steam: command to check wireless connection

[Rodrigo Rivas Costa]

The wireless adaptor does not tell if a device is already connected when
steam_probe() is run.
Use a command to request the connection status.

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
 drivers/hid/hid-steam.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 03eebc4c28ba..3d34716ae55f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -118,6 +118,11 @@ static int steam_send_report(struct steam_device *steam,
 	return ret;
 }
 
+static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
+{
+	return steam_send_report(steam, &cmd, 1);
+}
+
 static int steam_get_serial(struct steam_device *steam)
 {
 	/*
@@ -139,6 +144,16 @@ static int steam_get_serial(struct steam_device *steam)
 	return 0;
 }
 
+/*
+ * This command requests the wireless adaptor to post an event
+ * with the connection status. Useful if this driver is loaded when
+ * the controller is already connected.
+ */
+static inline int steam_request_conn_status(struct steam_device *steam)
+{
+	return steam_send_report_byte(steam, 0xb4);
+}
+
 static int steam_input_open(struct input_dev *dev)
 {
 	struct steam_device *steam = input_get_drvdata(dev);
@@ -360,6 +375,7 @@ static int steam_probe(struct hid_device *hdev,
 			goto hid_hw_open_fail;
 		}
 		hid_info(hdev, "Steam wireless receiver connected");
+		steam_request_conn_status(steam);
 	} else {
 		ret = steam_register(steam);
 		if (ret) {
-- 
2.16.2

[PATCH v5 4/4] HID: steam: add battery device.

[Rodrigo Rivas Costa]

The wireless Steam Controller is battery operated, so add the battery
device and power information.

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
 drivers/hid/hid-steam.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 3d34716ae55f..188d8c1a69d7 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -19,6 +19,7 @@
 #include <linux/workqueue.h>
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
+#include <linux/power_supply.h>
 #include "hid-ids.h"
 
 MODULE_LICENSE("GPL");
@@ -44,6 +45,10 @@ struct steam_device {
 	struct work_struct work_connect;
 	bool connected;
 	char serial_no[STEAM_SERIAL_LEN + 1];
+	struct power_supply_desc battery_desc;
+	struct power_supply __rcu *battery;
+	u8 battery_charge;
+	u16 voltage;
 };
 
 static int steam_recv_report(struct steam_device *steam,
@@ -168,6 +173,85 @@ static void steam_input_close(struct input_dev *dev)
 	hid_hw_close(steam->hdev);
 }
 
+static enum power_supply_property steam_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_SCOPE,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int steam_battery_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct steam_device *steam = power_supply_get_drvdata(psy);
+	unsigned long flags;
+	s16 volts;
+	u8 batt;
+	int ret = 0;
+
+	spin_lock_irqsave(&steam->lock, flags);
+	volts = steam->voltage;
+	batt = steam->battery_charge;
+	spin_unlock_irqrestore(&steam->lock, flags);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = volts * 1000; /* mV -> uV */
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = batt;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int steam_battery_register(struct steam_device *steam)
+{
+	struct power_supply *battery;
+	struct power_supply_config battery_cfg = { .drv_data = steam, };
+	unsigned long flags;
+	int ret;
+
+	steam->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
+	steam->battery_desc.properties = steam_battery_props;
+	steam->battery_desc.num_properties = ARRAY_SIZE(steam_battery_props);
+	steam->battery_desc.get_property = steam_battery_get_property;
+	steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
+			GFP_KERNEL, "steam-controller-%s-battery",
+			steam->serial_no);
+	if (!steam->battery_desc.name)
+		return -ENOMEM;
+
+	/* avoid the warning of 0% battery while waiting for the first info */
+	spin_lock_irqsave(&steam->lock, flags);
+	steam->voltage = 3000;
+	steam->battery_charge = 100;
+	spin_unlock_irqrestore(&steam->lock, flags);
+
+	battery = power_supply_register(&steam->hdev->dev,
+			&steam->battery_desc, &battery_cfg);
+	if (IS_ERR(battery)) {
+		ret = PTR_ERR(battery);
+		hid_err(steam->hdev,
+				"%s:power_supply_register failed with error %d\n",
+				__func__, ret);
+		return ret;
+	}
+	rcu_assign_pointer(steam->battery, battery);
+	power_supply_powers(battery, &steam->hdev->dev);
+	return 0;
+}
+
 static int steam_register(struct steam_device *steam)
 {
 	struct hid_device *hdev = steam->hdev;
@@ -255,6 +339,10 @@ static int steam_register(struct steam_device *steam)
 
 	rcu_assign_pointer(steam->input, input);
 
+	/* ignore battery errors, we can live without it */
+	if (steam->quirks & STEAM_QUIRK_WIRELESS)
+		steam_battery_register(steam);
+
 	return 0;
 
 input_register_fail:
@@ -265,11 +353,18 @@ static int steam_register(struct steam_device *steam)
 static void steam_unregister(struct steam_device *steam)
 {
 	struct input_dev *input;
+	struct power_supply *battery;
 
 	rcu_read_lock();
 	input = rcu_dereference(steam->input);
+	battery = rcu_dereference(steam->battery);
 	rcu_read_unlock();
 
+	if (battery) {
+		RCU_INIT_POINTER(steam->battery, NULL);
+		synchronize_rcu();
+		power_supply_unregister(battery);
+	}
 	if (input) {
 		RCU_INIT_POINTER(steam->input, NULL);
 		synchronize_rcu();
@@ -565,12 +660,44 @@ static void steam_do_input_event(struct steam_device *steam,
 	input_sync(input);
 }
 
+/*
+ * The size for this message payload is 11.
+ * The known values are:
+ *  Offset| Type  | Meaning
+ * -------+-------+---------------------------
+ *  4-7   | u32   | sequence number
+ *  8-11  | --    | always 0
+ *  12-13 | u16   | voltage (mV)
+ *  14    | u8    | battery percent
+ */
+static void steam_do_battery_event(struct steam_device *steam,
+		struct power_supply *battery, u8 *data)
+{
+	unsigned long flags;
+
+	s16 volts = le16_to_cpup((__le16 *)(data + 12));
+	u8 batt = data[14];
+
+	/* Creating the battery may have failed */
+	rcu_read_lock();
+	battery = rcu_dereference(steam->battery);
+	if (likely(battery)) {
+		spin_lock_irqsave(&steam->lock, flags);
+		steam->voltage = volts;
+		steam->battery_charge = batt;
+		spin_unlock_irqrestore(&steam->lock, flags);
+		power_supply_changed(battery);
+	}
+	rcu_read_unlock();
+}
+
 static int steam_raw_event(struct hid_device *hdev,
 			struct hid_report *report, u8 *data,
 			int size)
 {
 	struct steam_device *steam = hid_get_drvdata(hdev);
 	struct input_dev *input;
+	struct power_supply *battery;
 
 	if (!steam)
 		return 0;
@@ -623,7 +750,19 @@ static int steam_raw_event(struct hid_device *hdev,
 		}
 		break;
 	case 0x04:
-		/* TODO battery status */
+		if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+			rcu_read_lock();
+			battery = rcu_dereference(steam->battery);
+			if (likely(battery)) {
+				steam_do_battery_event(steam, battery, data);
+			} else {
+				dbg_hid(
+				  "%s: battery data without connect event\n",
+				  __func__);
+				steam_do_connect_event(steam, true);
+			}
+			rcu_read_unlock();
+		}
 		break;
 	}
 	return 0;
-- 
2.16.2

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Pierre-Loup A. Griffais]

On 03/11/2018 12:58 PM, Rodrigo Rivas Costa wrote:
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
> 
> Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> 
> @Pierre-Loup and @Clément, could you please have another look at this and
> check if it is worthy? Benjamin will not commit it without an express ACK from
> Valve. Of course he is right to be cautious, but I checked this driver with
> the Steam Client and all seems to go just fine. I think that there is a lot of
> Linux out of the desktop that could use this driver and cannot use the Steam
> Client. Worst case scenario, this driver can now be blacklisted, but I hope
> that will not be needed.

Hi Rodrigo,

I think the approach you outlined earlier of only sending configuration 
commands to the device when something is actively using the driver is 
sane. I won't have the cycles to thoroughly check all the possible 
interactions with the client in the near future, so in the interest of 
not blocking development I'd say go for it. I'll try to get someone to 
take the patchset for a spin soon.

Thanks,
  - Pierre-Loup

> 
> For full reference, I'm adding a full changelog of this patchset.
> 
> Changes in v5:
>   * Fix license SPDX to GPL-2.0+.
>   * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> 
> Changes in v4:
>   * Add command to check the wireless connection status on probe, without
>     waiting for a message (thanks to Clément Vuchener for the tip).
>   * Removed the error code on redundant connection/disconnection messages. That
>     was harmless but polluted dmesg.
>   * Added buttons for touching the left-pad and right-pad.
>   * Fixed a misplaced #include from 2/4 to 1/4.
> 
> Changes in v3:
>   * Use RCU to do the dynamic connec/disconnect of wireless devices.
>   * Remove entries in hid-quirks.c as they are no longer needed. This allows
>     this module to be blacklisted without side effects.
>   * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>     existing use cases (lizard mode). A user-space tool to do that is
>     linked.
>   * Fully separated axes for joystick and left-pad. As it happens.
>   * Add fuzz values for left/right pad axes, they are a little wiggly.
> 
> Changes in v2:
>   * Remove references to USB. Now the interesting interfaces are selected by
>     looking for the ones with feature reports.
>   * Feature reports buffers are allocated with hid_alloc_report_buf().
>   * Feature report length is checked, to avoid overflows in case of
>     corrupt/malicius USB devices.
>   * Resolution added to the ABS axes.
>   * A lot of minor cleanups.
> 
> Rodrigo Rivas Costa (4):
>    HID: add driver for Valve Steam Controller
>    HID: steam: add serial number information.
>    HID: steam: command to check wireless connection
>    HID: steam: add battery device.
> 
>   drivers/hid/Kconfig     |   8 +
>   drivers/hid/Makefile    |   1 +
>   drivers/hid/hid-ids.h   |   4 +
>   drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 807 insertions(+)
>   create mode 100644 drivers/hid/hid-steam.c
> 

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Sun, Mar 11, 2018 at 04:12:41PM -0700, Pierre-Loup A. Griffais wrote:
> 
> 
> On 03/11/2018 12:58 PM, Rodrigo Rivas Costa wrote:
> > This patchset implements a driver for Valve Steam Controller, based on a
> > reverse analysis by myself.
> > 
> > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> > 
> > @Pierre-Loup and @Clément, could you please have another look at this and
> > check if it is worthy? Benjamin will not commit it without an express ACK from
> > Valve. Of course he is right to be cautious, but I checked this driver with
> > the Steam Client and all seems to go just fine. I think that there is a lot of
> > Linux out of the desktop that could use this driver and cannot use the Steam
> > Client. Worst case scenario, this driver can now be blacklisted, but I hope
> > that will not be needed.
> 
> Hi Rodrigo,
> 
> I think the approach you outlined earlier of only sending configuration
> commands to the device when something is actively using the driver is sane.
> I won't have the cycles to thoroughly check all the possible interactions
> with the client in the near future, so in the interest of not blocking
> development I'd say go for it. I'll try to get someone to take the patchset
> for a spin soon.

First of all, thank you very much for your attention.

Note that this driver does not send any configuration command at all. It
only sends commands get_serial (ae 15 01) and get_comm_status (b4), and
those just on probe time. That should be quite safe IMO.

Sending a configuration command, such as enable/disable_gyro on
input->open() is probably not so safe, so I'm leaving that for an
eventual future patch. Many games (and even X) enumerate the input
devices by opening and closing them. That could cause trouble...


Best regards.
Rodrigo

> 
> Thanks,
>  - Pierre-Loup
> 
> > 
> > For full reference, I'm adding a full changelog of this patchset.
> > 
> > Changes in v5:
> >   * Fix license SPDX to GPL-2.0+.
> >   * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> > 
> > Changes in v4:
> >   * Add command to check the wireless connection status on probe, without
> >     waiting for a message (thanks to Clément Vuchener for the tip).
> >   * Removed the error code on redundant connection/disconnection messages. That
> >     was harmless but polluted dmesg.
> >   * Added buttons for touching the left-pad and right-pad.
> >   * Fixed a misplaced #include from 2/4 to 1/4.
> > 
> > Changes in v3:
> >   * Use RCU to do the dynamic connec/disconnect of wireless devices.
> >   * Remove entries in hid-quirks.c as they are no longer needed. This allows
> >     this module to be blacklisted without side effects.
> >   * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> >     existing use cases (lizard mode). A user-space tool to do that is
> >     linked.
> >   * Fully separated axes for joystick and left-pad. As it happens.
> >   * Add fuzz values for left/right pad axes, they are a little wiggly.
> > 
> > Changes in v2:
> >   * Remove references to USB. Now the interesting interfaces are selected by
> >     looking for the ones with feature reports.
> >   * Feature reports buffers are allocated with hid_alloc_report_buf().
> >   * Feature report length is checked, to avoid overflows in case of
> >     corrupt/malicius USB devices.
> >   * Resolution added to the ABS axes.
> >   * A lot of minor cleanups.
> > 
> > Rodrigo Rivas Costa (4):
> >    HID: add driver for Valve Steam Controller
> >    HID: steam: add serial number information.
> >    HID: steam: command to check wireless connection
> >    HID: steam: add battery device.
> > 
> >   drivers/hid/Kconfig     |   8 +
> >   drivers/hid/Makefile    |   1 +
> >   drivers/hid/hid-ids.h   |   4 +
> >   drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 807 insertions(+)
> >   create mode 100644 drivers/hid/hid-steam.c
> > 
> 

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
>
> Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>
> @Pierre-Loup and @Clément, could you please have another look at this and
> check if it is worthy? Benjamin will not commit it without an express ACK from
> Valve. Of course he is right to be cautious, but I checked this driver with
> the Steam Client and all seems to go just fine. I think that there is a lot of
> Linux out of the desktop that could use this driver and cannot use the Steam
> Client. Worst case scenario, this driver can now be blacklisted, but I hope
> that will not be needed.

I tested the driver with my 4.15 fedora kernel (I only built the
module not the whole kernel) and I got double inputs (your driver
input device + steam uinput device) when testing Shovel Knight with
Steam Big Picture. It seems to work fine when the inputs are the same,
but after changing the controller configuration in Steam, the issue
became apparent.

And without Steam and your external tool, you get double inputs too. I
tried RetroArch and it was unusable because of the keyboard inputs
from the lizard mode (e.g. pressing B also presses Esc and quits
RetroArch). Having to download and compile an external tool to make
the driver work properly may be too difficult for the user. Your goal
was to provide an alternative to user space drivers but now you
actually depend on (a very simple) one.

Also the button and axis codes do not match the gamepad API doc
(https://www.kernel.org/doc/Documentation/input/gamepad.txt).

>
> For full reference, I'm adding a full changelog of this patchset.
>
> Changes in v5:
>  * Fix license SPDX to GPL-2.0+.
>  * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>
> Changes in v4:
>  * Add command to check the wireless connection status on probe, without
>    waiting for a message (thanks to Clément Vuchener for the tip).
>  * Removed the error code on redundant connection/disconnection messages. That
>    was harmless but polluted dmesg.
>  * Added buttons for touching the left-pad and right-pad.
>  * Fixed a misplaced #include from 2/4 to 1/4.
>
> Changes in v3:
>  * Use RCU to do the dynamic connec/disconnect of wireless devices.
>  * Remove entries in hid-quirks.c as they are no longer needed. This allows
>    this module to be blacklisted without side effects.
>  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>    existing use cases (lizard mode). A user-space tool to do that is
>    linked.
>  * Fully separated axes for joystick and left-pad. As it happens.
>  * Add fuzz values for left/right pad axes, they are a little wiggly.
>
> Changes in v2:
>  * Remove references to USB. Now the interesting interfaces are selected by
>    looking for the ones with feature reports.
>  * Feature reports buffers are allocated with hid_alloc_report_buf().
>  * Feature report length is checked, to avoid overflows in case of
>    corrupt/malicius USB devices.
>  * Resolution added to the ABS axes.
>  * A lot of minor cleanups.
>
> Rodrigo Rivas Costa (4):
>   HID: add driver for Valve Steam Controller
>   HID: steam: add serial number information.
>   HID: steam: command to check wireless connection
>   HID: steam: add battery device.
>
>  drivers/hid/Kconfig     |   8 +
>  drivers/hid/Makefile    |   1 +
>  drivers/hid/hid-ids.h   |   4 +
>  drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 807 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.2
>

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
> 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> > This patchset implements a driver for Valve Steam Controller, based on a
> > reverse analysis by myself.
> >
> > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> >
> > @Pierre-Loup and @Clément, could you please have another look at this and
> > check if it is worthy? Benjamin will not commit it without an express ACK from
> > Valve. Of course he is right to be cautious, but I checked this driver with
> > the Steam Client and all seems to go just fine. I think that there is a lot of
> > Linux out of the desktop that could use this driver and cannot use the Steam
> > Client. Worst case scenario, this driver can now be blacklisted, but I hope
> > that will not be needed.
> 
> I tested the driver with my 4.15 fedora kernel (I only built the
> module not the whole kernel) and I got double inputs (your driver
> input device + steam uinput device) when testing Shovel Knight with
> Steam Big Picture. It seems to work fine when the inputs are the same,
> but after changing the controller configuration in Steam, the issue
> became apparent.

I assumed that when several joysticks are available, games would listen
to one one of them. It looks like I'm wrong, and some (many?) games will
listen to all available joysticks at the same time. Thus having two
logical joysticks that represent the same physical one is not good.

An easy solution would be that Steam Client grabs this driver
(ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
would be that Steam Client blacklists this driver, of course.

> And without Steam and your external tool, you get double inputs too. I
> tried RetroArch and it was unusable because of the keyboard inputs
> from the lizard mode (e.g. pressing B also presses Esc and quits
> RetroArch). Having to download and compile an external tool to make
> the driver work properly may be too difficult for the user. Your goal
> was to provide an alternative to user space drivers but now you
> actually depend on (a very simple) one.

Yes, I noticed that. TBH, this driver without Steam Client or the
user-space tool is not very nice, precisely because you'll get constant
Escape and Enter presses, and most games react to those.

Frankly speaking, I'm not sure how to proceed. I can think of the
following options:
 1.Steam Client installation could add a file to blacklist
   hid-steam, just as it adds a few udev rules.
 2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
   on the architectures for which there is a Steam Client available.
   This way DIY projects will still be able to use it.
 3.This driver could be abandoned :-(. Just use Steam Client if possible or
   any of the user-mode drivers available.

If we decide for 1 or 2, then the lizard mode could be disabled without
ill effects. We could even enable the gyro and all the other gadgets
without worring about current compatibility.

At the end of the day, I think that it is up to Valve what to do.
Best Regards.
Rodrigo.

> Also the button and axis codes do not match the gamepad API doc
> (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
> 
> >
> > For full reference, I'm adding a full changelog of this patchset.
> >
> > Changes in v5:
> >  * Fix license SPDX to GPL-2.0+.
> >  * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> >
> > Changes in v4:
> >  * Add command to check the wireless connection status on probe, without
> >    waiting for a message (thanks to Clément Vuchener for the tip).
> >  * Removed the error code on redundant connection/disconnection messages. That
> >    was harmless but polluted dmesg.
> >  * Added buttons for touching the left-pad and right-pad.
> >  * Fixed a misplaced #include from 2/4 to 1/4.
> >
> > Changes in v3:
> >  * Use RCU to do the dynamic connec/disconnect of wireless devices.
> >  * Remove entries in hid-quirks.c as they are no longer needed. This allows
> >    this module to be blacklisted without side effects.
> >  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> >    existing use cases (lizard mode). A user-space tool to do that is
> >    linked.
> >  * Fully separated axes for joystick and left-pad. As it happens.
> >  * Add fuzz values for left/right pad axes, they are a little wiggly.
> >
> > Changes in v2:
> >  * Remove references to USB. Now the interesting interfaces are selected by
> >    looking for the ones with feature reports.
> >  * Feature reports buffers are allocated with hid_alloc_report_buf().
> >  * Feature report length is checked, to avoid overflows in case of
> >    corrupt/malicius USB devices.
> >  * Resolution added to the ABS axes.
> >  * A lot of minor cleanups.
> >
> > Rodrigo Rivas Costa (4):
> >   HID: add driver for Valve Steam Controller
> >   HID: steam: add serial number information.
> >   HID: steam: command to check wireless connection
> >   HID: steam: add battery device.
> >
> >  drivers/hid/Kconfig     |   8 +
> >  drivers/hid/Makefile    |   1 +
> >  drivers/hid/hid-ids.h   |   4 +
> >  drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 807 insertions(+)
> >  create mode 100644 drivers/hid/hid-steam.c
> >
> > --
> > 2.16.2
> >

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Benjamin Tissoires]

On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
>> 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
>> > This patchset implements a driver for Valve Steam Controller, based on a
>> > reverse analysis by myself.
>> >
>> > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>> >
>> > @Pierre-Loup and @Clément, could you please have another look at this and
>> > check if it is worthy? Benjamin will not commit it without an express ACK from
>> > Valve. Of course he is right to be cautious, but I checked this driver with
>> > the Steam Client and all seems to go just fine. I think that there is a lot of
>> > Linux out of the desktop that could use this driver and cannot use the Steam
>> > Client. Worst case scenario, this driver can now be blacklisted, but I hope
>> > that will not be needed.
>>
>> I tested the driver with my 4.15 fedora kernel (I only built the
>> module not the whole kernel) and I got double inputs (your driver
>> input device + steam uinput device) when testing Shovel Knight with
>> Steam Big Picture. It seems to work fine when the inputs are the same,
>> but after changing the controller configuration in Steam, the issue
>> became apparent.
>
> I assumed that when several joysticks are available, games would listen
> to one one of them. It looks like I'm wrong, and some (many?) games will
> listen to all available joysticks at the same time. Thus having two
> logical joysticks that represent the same physical one is not good.

Yeah, the general rule of thumb is "think of the worst thing that can
happen, someone will do something worst".

>
> An easy solution would be that Steam Client grabs this driver
> (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
> would be that Steam Client blacklists this driver, of course.

This is 2 solutions that rely on a userspace change, and this is not
acceptable in its current form. What if people do not upgrade Steam
client but upgrade their kernel? Well, Steam might be able to force
people to always run the latest shiny available version, but for other
games, you can't expect people to have a compatible version of the
userspace stack.

Also, "blacklisting the driver" from Steam client is something the OS
can do, but not the client when you run on a different distribution.
You need root for that, and I don't want to give root permissions to
Steam (or to any user space client that shouldn't have root privileges
for what it matters).

>
>> And without Steam and your external tool, you get double inputs too. I
>> tried RetroArch and it was unusable because of the keyboard inputs
>> from the lizard mode (e.g. pressing B also presses Esc and quits
>> RetroArch). Having to download and compile an external tool to make
>> the driver work properly may be too difficult for the user. Your goal
>> was to provide an alternative to user space drivers but now you
>> actually depend on (a very simple) one.
>
> Yes, I noticed that. TBH, this driver without Steam Client or the
> user-space tool is not very nice, precisely because you'll get constant
> Escape and Enter presses, and most games react to those.
>
> Frankly speaking, I'm not sure how to proceed. I can think of the
> following options:
>  1.Steam Client installation could add a file to blacklist
>    hid-steam, just as it adds a few udev rules.

But what about RetroArch? And what if you install Steam but want to
play SDL games that could benefit from your driver?

>  2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
>    on the architectures for which there is a Steam Client available.
>    This way DIY projects will still be able to use it.

But this will make the decision to include or not the driver in
distributions harder. And if no distribution uses it, you won't have
free tests, and you will be alone to maintain it. So that's not ideal
either

>  3.This driver could be abandoned :-(. Just use Steam Client if possible or
>    any of the user-mode drivers available.

This would be a waste for everybody as it's always better when we share.

>
> If we decide for 1 or 2, then the lizard mode could be disabled without
> ill effects. We could even enable the gyro and all the other gadgets
> without worring about current compatibility.

To me, 1 is out of the question. The kernel can't expect a user space
change especially if you are knowingly introducing a bug for the end
user.

2 is still on the table IMO, and 3 would be a shame.

I know we already discussed about sysfs and module parameters, but if
the driver will conflict with a userspace stack, the only way would be
to have a (dynamic) parameter "enhanced_mode" or
"kernel_awesome_support" or whatever which would be a boolean, that
defaults to false that Steam can eventually lookup if they want so in
the future we can default it to true. When this parameter is set, the
driver will create the inputs and toggle the various modes, while when
it's toggled off, it'll clean up itself and keep the device as if it
were connected to hid-generic. Bonus point, this removes the need for
the simple user space tool that enables the mode.

>
> At the end of the day, I think that it is up to Valve what to do.

Again, Valve is a big player here, but do not underestimate other
projects (like RetroArch mentioned above) because if you break their
workflow, they will have the right to request a revert of the commit
because it breaks some random user playing games in the far end of
Antarctica (yes, penguins do love to play games :-P )

Cheers,
Benjamin

> Best Regards.
> Rodrigo.
>
>> Also the button and axis codes do not match the gamepad API doc
>> (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
>>
>> >
>> > For full reference, I'm adding a full changelog of this patchset.
>> >
>> > Changes in v5:
>> >  * Fix license SPDX to GPL-2.0+.
>> >  * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>> >
>> > Changes in v4:
>> >  * Add command to check the wireless connection status on probe, without
>> >    waiting for a message (thanks to Clément Vuchener for the tip).
>> >  * Removed the error code on redundant connection/disconnection messages. That
>> >    was harmless but polluted dmesg.
>> >  * Added buttons for touching the left-pad and right-pad.
>> >  * Fixed a misplaced #include from 2/4 to 1/4.
>> >
>> > Changes in v3:
>> >  * Use RCU to do the dynamic connec/disconnect of wireless devices.
>> >  * Remove entries in hid-quirks.c as they are no longer needed. This allows
>> >    this module to be blacklisted without side effects.
>> >  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>> >    existing use cases (lizard mode). A user-space tool to do that is
>> >    linked.
>> >  * Fully separated axes for joystick and left-pad. As it happens.
>> >  * Add fuzz values for left/right pad axes, they are a little wiggly.
>> >
>> > Changes in v2:
>> >  * Remove references to USB. Now the interesting interfaces are selected by
>> >    looking for the ones with feature reports.
>> >  * Feature reports buffers are allocated with hid_alloc_report_buf().
>> >  * Feature report length is checked, to avoid overflows in case of
>> >    corrupt/malicius USB devices.
>> >  * Resolution added to the ABS axes.
>> >  * A lot of minor cleanups.
>> >
>> > Rodrigo Rivas Costa (4):
>> >   HID: add driver for Valve Steam Controller
>> >   HID: steam: add serial number information.
>> >   HID: steam: command to check wireless connection
>> >   HID: steam: add battery device.
>> >
>> >  drivers/hid/Kconfig     |   8 +
>> >  drivers/hid/Makefile    |   1 +
>> >  drivers/hid/hid-ids.h   |   4 +
>> >  drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  4 files changed, 807 insertions(+)
>> >  create mode 100644 drivers/hid/hid-steam.c
>> >
>> > --
>> > 2.16.2
>> >

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
> On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
> >> 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> >> > This patchset implements a driver for Valve Steam Controller, based on a
> >> > reverse analysis by myself.
> >> >
> >> > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> >> >
> >> > @Pierre-Loup and @Clément, could you please have another look at this and
> >> > check if it is worthy? Benjamin will not commit it without an express ACK from
> >> > Valve. Of course he is right to be cautious, but I checked this driver with
> >> > the Steam Client and all seems to go just fine. I think that there is a lot of
> >> > Linux out of the desktop that could use this driver and cannot use the Steam
> >> > Client. Worst case scenario, this driver can now be blacklisted, but I hope
> >> > that will not be needed.
> >>
> >> I tested the driver with my 4.15 fedora kernel (I only built the
> >> module not the whole kernel) and I got double inputs (your driver
> >> input device + steam uinput device) when testing Shovel Knight with
> >> Steam Big Picture. It seems to work fine when the inputs are the same,
> >> but after changing the controller configuration in Steam, the issue
> >> became apparent.
> >
> > I assumed that when several joysticks are available, games would listen
> > to one one of them. It looks like I'm wrong, and some (many?) games will
> > listen to all available joysticks at the same time. Thus having two
> > logical joysticks that represent the same physical one is not good.
> 
> Yeah, the general rule of thumb is "think of the worst thing that can
> happen, someone will do something worst".
> 
> >
> > An easy solution would be that Steam Client grabs this driver
> > (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
> > would be that Steam Client blacklists this driver, of course.
> 
> This is 2 solutions that rely on a userspace change, and this is not
> acceptable in its current form. What if people do not upgrade Steam
> client but upgrade their kernel? Well, Steam might be able to force
> people to always run the latest shiny available version, but for other
> games, you can't expect people to have a compatible version of the
> userspace stack.

Well, if you don't have Steam then you don't have the double input in
the first place. Unless you are using a different user-mode driver, of
course.
> 
> Also, "blacklisting the driver" from Steam client is something the OS
> can do, but not the client when you run on a different distribution.
> You need root for that, and I don't want to give root permissions to
> Steam (or to any user space client that shouldn't have root privileges
> for what it matters).

Actually Steam needs a system installation that adds udev rules to grant
uaccess to /dev/uinput and the USB raw device for the controller.
Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
a bit inconvenient because you'll need a distro update of the steam
package, not just the usual user-mode-only auto-update.

> >
> >> And without Steam and your external tool, you get double inputs too. I
> >> tried RetroArch and it was unusable because of the keyboard inputs
> >> from the lizard mode (e.g. pressing B also presses Esc and quits
> >> RetroArch). Having to download and compile an external tool to make
> >> the driver work properly may be too difficult for the user. Your goal
> >> was to provide an alternative to user space drivers but now you
> >> actually depend on (a very simple) one.
> >
> > Yes, I noticed that. TBH, this driver without Steam Client or the
> > user-space tool is not very nice, precisely because you'll get constant
> > Escape and Enter presses, and most games react to those.
> >
> > Frankly speaking, I'm not sure how to proceed. I can think of the
> > following options:
> >  1.Steam Client installation could add a file to blacklist
> >    hid-steam, just as it adds a few udev rules.
> 
> But what about RetroArch? And what if you install Steam but want to
> play SDL games that could benefit from your driver?

That is an issue of solution 1. I actually have the module blacklisted
in my PC, and run `sudo modprobe hid-steam` to use SDL.

> >  2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
> >    on the architectures for which there is a Steam Client available.
> >    This way DIY projects will still be able to use it.
> 
> But this will make the decision to include or not the driver in
> distributions harder. And if no distribution uses it, you won't have
> free tests, and you will be alone to maintain it. So that's not ideal
> either

Could we set the default to 'y' in non-PC systems. It would be enabled
in my Raspbian, for example... better than nothing.
> 
> >  3.This driver could be abandoned :-(. Just use Steam Client if possible or
> >    any of the user-mode drivers available.
> 
> This would be a waste for everybody as it's always better when we share.

Indeed!

I tried a new option:
  4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
     if that is the case it will disable itself. If the DEVUSB/HIDRAW is
     not in use, then the driver will work normally. A bit hackish maybe
     but it should work.

I tried doing this option 4, but I'm unable to do it properly. I don't
even know if it is possible...

> >
> > If we decide for 1 or 2, then the lizard mode could be disabled without
> > ill effects. We could even enable the gyro and all the other gadgets
> > without worring about current compatibility.
> 
> To me, 1 is out of the question. The kernel can't expect a user space
> change especially if you are knowingly introducing a bug for the end
> user.
> 
> 2 is still on the table IMO, and 3 would be a shame.
> 
> I know we already discussed about sysfs and module parameters, but if
> the driver will conflict with a userspace stack, the only way would be
> to have a (dynamic) parameter "enhanced_mode" or
> "kernel_awesome_support" or whatever which would be a boolean, that
> defaults to false that Steam can eventually lookup if they want so in
> the future we can default it to true. When this parameter is set, the
> driver will create the inputs and toggle the various modes, while when
> it's toggled off, it'll clean up itself and keep the device as if it
> were connected to hid-generic. Bonus point, this removes the need for
> the simple user space tool that enables the mode.

That is doable, but that sysfs/parameter can be changed by a non-root
user? I looked for a udev rule to grant access to those but found
nothing.

IIUC, when this parameter is false the driver will do nothing, right?
The user will just need to change it to true to be able to use it, but
that will have to be done by root.

I'll try doing this, but I'd appreciate your advice about what approach
would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?

> > At the end of the day, I think that it is up to Valve what to do.
> 
> Again, Valve is a big player here, but do not underestimate other
> projects (like RetroArch mentioned above) because if you break their
> workflow, they will have the right to request a revert of the commit
> because it breaks some random user playing games in the far end of
> Antarctica (yes, penguins do love to play games :-P )

And everybody loves penguins! If we take away Steam (say a RaspberryPi
as a canonical example) and disable the lizard mode, then this driver is
just a regular gamepad. RetroArch should be happy with that. Unless they
already have an user mode driver for the steam-controller, of course...

Best regards.
Rodrigo

> Cheers,
> Benjamin
> 
> > Best Regards.
> > Rodrigo.
> >
> >> Also the button and axis codes do not match the gamepad API doc
> >> (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
> >>
> >> >
> >> > For full reference, I'm adding a full changelog of this patchset.
> >> >
> >> > Changes in v5:
> >> >  * Fix license SPDX to GPL-2.0+.
> >> >  * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> >> >
> >> > Changes in v4:
> >> >  * Add command to check the wireless connection status on probe, without
> >> >    waiting for a message (thanks to Clément Vuchener for the tip).
> >> >  * Removed the error code on redundant connection/disconnection messages. That
> >> >    was harmless but polluted dmesg.
> >> >  * Added buttons for touching the left-pad and right-pad.
> >> >  * Fixed a misplaced #include from 2/4 to 1/4.
> >> >
> >> > Changes in v3:
> >> >  * Use RCU to do the dynamic connec/disconnect of wireless devices.
> >> >  * Remove entries in hid-quirks.c as they are no longer needed. This allows
> >> >    this module to be blacklisted without side effects.
> >> >  * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> >> >    existing use cases (lizard mode). A user-space tool to do that is
> >> >    linked.
> >> >  * Fully separated axes for joystick and left-pad. As it happens.
> >> >  * Add fuzz values for left/right pad axes, they are a little wiggly.
> >> >
> >> > Changes in v2:
> >> >  * Remove references to USB. Now the interesting interfaces are selected by
> >> >    looking for the ones with feature reports.
> >> >  * Feature reports buffers are allocated with hid_alloc_report_buf().
> >> >  * Feature report length is checked, to avoid overflows in case of
> >> >    corrupt/malicius USB devices.
> >> >  * Resolution added to the ABS axes.
> >> >  * A lot of minor cleanups.
> >> >
> >> > Rodrigo Rivas Costa (4):
> >> >   HID: add driver for Valve Steam Controller
> >> >   HID: steam: add serial number information.
> >> >   HID: steam: command to check wireless connection
> >> >   HID: steam: add battery device.
> >> >
> >> >  drivers/hid/Kconfig     |   8 +
> >> >  drivers/hid/Makefile    |   1 +
> >> >  drivers/hid/hid-ids.h   |   4 +
> >> >  drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  4 files changed, 807 insertions(+)
> >> >  create mode 100644 drivers/hid/hid-steam.c
> >> >
> >> > --
> >> > 2.16.2
> >> >

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Pierre-Loup A. Griffais]

On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
> On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
>> On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
>> <rodrigorivascosta@gmail.com> wrote:
>>> On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
>>>> 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
>>>>> This patchset implements a driver for Valve Steam Controller, based on a
>>>>> reverse analysis by myself.
>>>>>
>>>>> Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>>>>>
>>>>> @Pierre-Loup and @Clément, could you please have another look at this and
>>>>> check if it is worthy? Benjamin will not commit it without an express ACK from
>>>>> Valve. Of course he is right to be cautious, but I checked this driver with
>>>>> the Steam Client and all seems to go just fine. I think that there is a lot of
>>>>> Linux out of the desktop that could use this driver and cannot use the Steam
>>>>> Client. Worst case scenario, this driver can now be blacklisted, but I hope
>>>>> that will not be needed.
>>>>
>>>> I tested the driver with my 4.15 fedora kernel (I only built the
>>>> module not the whole kernel) and I got double inputs (your driver
>>>> input device + steam uinput device) when testing Shovel Knight with
>>>> Steam Big Picture. It seems to work fine when the inputs are the same,
>>>> but after changing the controller configuration in Steam, the issue
>>>> became apparent.
>>>
>>> I assumed that when several joysticks are available, games would listen
>>> to one one of them. It looks like I'm wrong, and some (many?) games will
>>> listen to all available joysticks at the same time. Thus having two
>>> logical joysticks that represent the same physical one is not good.
>>
>> Yeah, the general rule of thumb is "think of the worst thing that can
>> happen, someone will do something worst".
>>
>>>
>>> An easy solution would be that Steam Client grabs this driver
>>> (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
>>> would be that Steam Client blacklists this driver, of course.
>>
>> This is 2 solutions that rely on a userspace change, and this is not
>> acceptable in its current form. What if people do not upgrade Steam
>> client but upgrade their kernel? Well, Steam might be able to force
>> people to always run the latest shiny available version, but for other
>> games, you can't expect people to have a compatible version of the
>> userspace stack.
> 
> Well, if you don't have Steam then you don't have the double input in
> the first place. Unless you are using a different user-mode driver, of
> course.
>>
>> Also, "blacklisting the driver" from Steam client is something the OS
>> can do, but not the client when you run on a different distribution.
>> You need root for that, and I don't want to give root permissions to
>> Steam (or to any user space client that shouldn't have root privileges
>> for what it matters).
> 
> Actually Steam needs a system installation that adds udev rules to grant
> uaccess to /dev/uinput and the USB raw device for the controller.
> Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
> a bit inconvenient because you'll need a distro update of the steam
> package, not just the usual user-mode-only auto-update.

It's definitely a bit tricky; we've rolled out an update to our 
reference package whenever we've added support for new devices (the 
final Steam Controller, direct PS4 gamepad led/gyro access through HID, 
HTC Vive and its myriad of onboard devices, bootloaders of all these 
things for firmware updates, etc). Whenever we have to do that, the 
rollout never is as smooth as desired: many users aren't using our own 
package; even on the distributions we support directly. Then downstream 
distributions adopt these udev changes with various delays (sometimes 
never), and port them to their own mechanism of doing things, since 
everyone has their own idea of a robust security model. I wish local 
sessions always had proper access to HID devices connected to the 
physical computer the user is sitting at, but I realize that the basic 
gaming desktop is just one of many usecases distros out there have to 
support and it'd be unreasonable to expect them to focus exclusively on it.

> 
>>>
>>>> And without Steam and your external tool, you get double inputs too. I
>>>> tried RetroArch and it was unusable because of the keyboard inputs
>>>> from the lizard mode (e.g. pressing B also presses Esc and quits
>>>> RetroArch). Having to download and compile an external tool to make
>>>> the driver work properly may be too difficult for the user. Your goal
>>>> was to provide an alternative to user space drivers but now you
>>>> actually depend on (a very simple) one.
>>>
>>> Yes, I noticed that. TBH, this driver without Steam Client or the
>>> user-space tool is not very nice, precisely because you'll get constant
>>> Escape and Enter presses, and most games react to those.
>>>
>>> Frankly speaking, I'm not sure how to proceed. I can think of the
>>> following options:
>>>   1.Steam Client installation could add a file to blacklist
>>>     hid-steam, just as it adds a few udev rules.
>>
>> But what about RetroArch? And what if you install Steam but want to
>> play SDL games that could benefit from your driver?
> 
> That is an issue of solution 1. I actually have the module blacklisted
> in my PC, and run `sudo modprobe hid-steam` to use SDL.
> 
>>>   2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
>>>     on the architectures for which there is a Steam Client available.
>>>     This way DIY projects will still be able to use it.
>>
>> But this will make the decision to include or not the driver in
>> distributions harder. And if no distribution uses it, you won't have
>> free tests, and you will be alone to maintain it. So that's not ideal
>> either
> 
> Could we set the default to 'y' in non-PC systems. It would be enabled
> in my Raspbian, for example... better than nothing.
>>
>>>   3.This driver could be abandoned :-(. Just use Steam Client if possible or
>>>     any of the user-mode drivers available.
>>
>> This would be a waste for everybody as it's always better when we share.
> 
> Indeed!
> 
> I tried a new option:
>    4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
>       if that is the case it will disable itself. If the DEVUSB/HIDRAW is
>       not in use, then the driver will work normally. A bit hackish maybe
>       but it should work.
> 
> I tried doing this option 4, but I'm unable to do it properly. I don't
> even know if it is possible...
> 
>>>
>>> If we decide for 1 or 2, then the lizard mode could be disabled without
>>> ill effects. We could even enable the gyro and all the other gadgets
>>> without worring about current compatibility.
>>
>> To me, 1 is out of the question. The kernel can't expect a user space
>> change especially if you are knowingly introducing a bug for the end
>> user.
>>
>> 2 is still on the table IMO, and 3 would be a shame.
>>
>> I know we already discussed about sysfs and module parameters, but if
>> the driver will conflict with a userspace stack, the only way would be
>> to have a (dynamic) parameter "enhanced_mode" or
>> "kernel_awesome_support" or whatever which would be a boolean, that
>> defaults to false that Steam can eventually lookup if they want so in
>> the future we can default it to true. When this parameter is set, the
>> driver will create the inputs and toggle the various modes, while when
>> it's toggled off, it'll clean up itself and keep the device as if it
>> were connected to hid-generic. Bonus point, this removes the need for
>> the simple user space tool that enables the mode.
> 
> That is doable, but that sysfs/parameter can be changed by a non-root
> user? I looked for a udev rule to grant access to those but found
> nothing.
> 
> IIUC, when this parameter is false the driver will do nothing, right?
> The user will just need to change it to true to be able to use it, but
> that will have to be done by root.
> 
> I'll try doing this, but I'd appreciate your advice about what approach
> would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
> 
>>> At the end of the day, I think that it is up to Valve what to do.
>>
>> Again, Valve is a big player here, but do not underestimate other
>> projects (like RetroArch mentioned above) because if you break their
>> workflow, they will have the right to request a revert of the commit
>> because it breaks some random user playing games in the far end of
>> Antarctica (yes, penguins do love to play games :-P )
> 
> And everybody loves penguins! If we take away Steam (say a RaspberryPi
> as a canonical example) and disable the lizard mode, then this driver is
> just a regular gamepad. RetroArch should be happy with that. Unless they
> already have an user mode driver for the steam-controller, of course...

Both of these things seem reasonable to me, with a few caveats:

  - If there's an opt-in mechanism available, it would be good to ensure 
we have a way to reliably query its state without requiring extra 
permissions. This way, if we know it's likely to affect Steam client 
functionality, we'll have the right mechanism to properly message the 
situation to users.

  - If you find a way for the client to be able to program an opt out 
when it's running that is not automatic (eg. by detecting the hidraw 
device being opened), we'd be happy to participate with that scheme 
assuming it doesn't require extra permissions. As soon as the API is 
figured out, we can include it in the client, just send me a heads-up. 
The one thing that I'd be cautious of is robust behavior against 
abnormal client termination. If it's a sysfs entry we have to poke, I'm 
worried that if the client crashes we might not always be able to opt 
the driver back out. It'd be nice if it was based on opening an fd 
instead, this way the kernel would robustly clean up after us and your 
driver would kick back in.

Note that there's a general desire on our side to create a reference 
userspace implementation that would more or less have the current 
functionality of the Steam client, but would be easily usable from other 
platforms where the client doesn't currentl run. Unfortunately it's 
quite a bit of work, so it's unclear what the timeframe would be, if it 
ever does happen.

Thanks,
  - Pierre-Loup

> 
> Best regards.
> Rodrigo
> 
>> Cheers,
>> Benjamin
>>
>>> Best Regards.
>>> Rodrigo.
>>>
>>>> Also the button and axis codes do not match the gamepad API doc
>>>> (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
>>>>
>>>>>
>>>>> For full reference, I'm adding a full changelog of this patchset.
>>>>>
>>>>> Changes in v5:
>>>>>   * Fix license SPDX to GPL-2.0+.
>>>>>   * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>>>>>
>>>>> Changes in v4:
>>>>>   * Add command to check the wireless connection status on probe, without
>>>>>     waiting for a message (thanks to Clément Vuchener for the tip).
>>>>>   * Removed the error code on redundant connection/disconnection messages. That
>>>>>     was harmless but polluted dmesg.
>>>>>   * Added buttons for touching the left-pad and right-pad.
>>>>>   * Fixed a misplaced #include from 2/4 to 1/4.
>>>>>
>>>>> Changes in v3:
>>>>>   * Use RCU to do the dynamic connec/disconnect of wireless devices.
>>>>>   * Remove entries in hid-quirks.c as they are no longer needed. This allows
>>>>>     this module to be blacklisted without side effects.
>>>>>   * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>>>>>     existing use cases (lizard mode). A user-space tool to do that is
>>>>>     linked.
>>>>>   * Fully separated axes for joystick and left-pad. As it happens.
>>>>>   * Add fuzz values for left/right pad axes, they are a little wiggly.
>>>>>
>>>>> Changes in v2:
>>>>>   * Remove references to USB. Now the interesting interfaces are selected by
>>>>>     looking for the ones with feature reports.
>>>>>   * Feature reports buffers are allocated with hid_alloc_report_buf().
>>>>>   * Feature report length is checked, to avoid overflows in case of
>>>>>     corrupt/malicius USB devices.
>>>>>   * Resolution added to the ABS axes.
>>>>>   * A lot of minor cleanups.
>>>>>
>>>>> Rodrigo Rivas Costa (4):
>>>>>    HID: add driver for Valve Steam Controller
>>>>>    HID: steam: add serial number information.
>>>>>    HID: steam: command to check wireless connection
>>>>>    HID: steam: add battery device.
>>>>>
>>>>>   drivers/hid/Kconfig     |   8 +
>>>>>   drivers/hid/Makefile    |   1 +
>>>>>   drivers/hid/hid-ids.h   |   4 +
>>>>>   drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   4 files changed, 807 insertions(+)
>>>>>   create mode 100644 drivers/hid/hid-steam.c
>>>>>
>>>>> --
>>>>> 2.16.2
>>>>>
> 

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
> 
> 
> On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
> > On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
> > > On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
> > > <rodrigorivascosta@gmail.com> wrote:
> > > > On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
> > > > > 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> > > > > > This patchset implements a driver for Valve Steam Controller, based on a
> > > > > > reverse analysis by myself.
> > > > > > 
> > > > > > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> > > > > > 
> > > > > > @Pierre-Loup and @Clément, could you please have another look at this and
> > > > > > check if it is worthy? Benjamin will not commit it without an express ACK from
> > > > > > Valve. Of course he is right to be cautious, but I checked this driver with
> > > > > > the Steam Client and all seems to go just fine. I think that there is a lot of
> > > > > > Linux out of the desktop that could use this driver and cannot use the Steam
> > > > > > Client. Worst case scenario, this driver can now be blacklisted, but I hope
> > > > > > that will not be needed.
> > > > > 
> > > > > I tested the driver with my 4.15 fedora kernel (I only built the
> > > > > module not the whole kernel) and I got double inputs (your driver
> > > > > input device + steam uinput device) when testing Shovel Knight with
> > > > > Steam Big Picture. It seems to work fine when the inputs are the same,
> > > > > but after changing the controller configuration in Steam, the issue
> > > > > became apparent.
> > > > 
> > > > I assumed that when several joysticks are available, games would listen
> > > > to one one of them. It looks like I'm wrong, and some (many?) games will
> > > > listen to all available joysticks at the same time. Thus having two
> > > > logical joysticks that represent the same physical one is not good.
> > > 
> > > Yeah, the general rule of thumb is "think of the worst thing that can
> > > happen, someone will do something worst".
> > > 
> > > > 
> > > > An easy solution would be that Steam Client grabs this driver
> > > > (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
> > > > would be that Steam Client blacklists this driver, of course.
> > > 
> > > This is 2 solutions that rely on a userspace change, and this is not
> > > acceptable in its current form. What if people do not upgrade Steam
> > > client but upgrade their kernel? Well, Steam might be able to force
> > > people to always run the latest shiny available version, but for other
> > > games, you can't expect people to have a compatible version of the
> > > userspace stack.
> > 
> > Well, if you don't have Steam then you don't have the double input in
> > the first place. Unless you are using a different user-mode driver, of
> > course.
> > > 
> > > Also, "blacklisting the driver" from Steam client is something the OS
> > > can do, but not the client when you run on a different distribution.
> > > You need root for that, and I don't want to give root permissions to
> > > Steam (or to any user space client that shouldn't have root privileges
> > > for what it matters).
> > 
> > Actually Steam needs a system installation that adds udev rules to grant
> > uaccess to /dev/uinput and the USB raw device for the controller.
> > Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
> > a bit inconvenient because you'll need a distro update of the steam
> > package, not just the usual user-mode-only auto-update.
> 
> It's definitely a bit tricky; we've rolled out an update to our reference
> package whenever we've added support for new devices (the final Steam
> Controller, direct PS4 gamepad led/gyro access through HID, HTC Vive and its
> myriad of onboard devices, bootloaders of all these things for firmware
> updates, etc). Whenever we have to do that, the rollout never is as smooth
> as desired: many users aren't using our own package; even on the
> distributions we support directly. Then downstream distributions adopt these
> udev changes with various delays (sometimes never), and port them to their
> own mechanism of doing things, since everyone has their own idea of a robust
> security model. I wish local sessions always had proper access to HID
> devices connected to the physical computer the user is sitting at, but I
> realize that the basic gaming desktop is just one of many usecases distros
> out there have to support and it'd be unreasonable to expect them to focus
> exclusively on it.

I was afraid of something like that. Let's forget about that option for
the moment.

> > > > > And without Steam and your external tool, you get double inputs too. I
> > > > > tried RetroArch and it was unusable because of the keyboard inputs
> > > > > from the lizard mode (e.g. pressing B also presses Esc and quits
> > > > > RetroArch). Having to download and compile an external tool to make
> > > > > the driver work properly may be too difficult for the user. Your goal
> > > > > was to provide an alternative to user space drivers but now you
> > > > > actually depend on (a very simple) one.
> > > > 
> > > > Yes, I noticed that. TBH, this driver without Steam Client or the
> > > > user-space tool is not very nice, precisely because you'll get constant
> > > > Escape and Enter presses, and most games react to those.
> > > > 
> > > > Frankly speaking, I'm not sure how to proceed. I can think of the
> > > > following options:
> > > >   1.Steam Client installation could add a file to blacklist
> > > >     hid-steam, just as it adds a few udev rules.
> > > 
> > > But what about RetroArch? And what if you install Steam but want to
> > > play SDL games that could benefit from your driver?
> > 
> > That is an issue of solution 1. I actually have the module blacklisted
> > in my PC, and run `sudo modprobe hid-steam` to use SDL.
> > 
> > > >   2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
> > > >     on the architectures for which there is a Steam Client available.
> > > >     This way DIY projects will still be able to use it.
> > > 
> > > But this will make the decision to include or not the driver in
> > > distributions harder. And if no distribution uses it, you won't have
> > > free tests, and you will be alone to maintain it. So that's not ideal
> > > either
> > 
> > Could we set the default to 'y' in non-PC systems. It would be enabled
> > in my Raspbian, for example... better than nothing.
> > > 
> > > >   3.This driver could be abandoned :-(. Just use Steam Client if possible or
> > > >     any of the user-mode drivers available.
> > > 
> > > This would be a waste for everybody as it's always better when we share.
> > 
> > Indeed!
> > 
> > I tried a new option:
> >    4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
> >       if that is the case it will disable itself. If the DEVUSB/HIDRAW is
> >       not in use, then the driver will work normally. A bit hackish maybe
> >       but it should work.
> > 
> > I tried doing this option 4, but I'm unable to do it properly. I don't
> > even know if it is possible...
> > 
> > > > 
> > > > If we decide for 1 or 2, then the lizard mode could be disabled without
> > > > ill effects. We could even enable the gyro and all the other gadgets
> > > > without worring about current compatibility.
> > > 
> > > To me, 1 is out of the question. The kernel can't expect a user space
> > > change especially if you are knowingly introducing a bug for the end
> > > user.
> > > 
> > > 2 is still on the table IMO, and 3 would be a shame.
> > > 
> > > I know we already discussed about sysfs and module parameters, but if
> > > the driver will conflict with a userspace stack, the only way would be
> > > to have a (dynamic) parameter "enhanced_mode" or
> > > "kernel_awesome_support" or whatever which would be a boolean, that
> > > defaults to false that Steam can eventually lookup if they want so in
> > > the future we can default it to true. When this parameter is set, the
> > > driver will create the inputs and toggle the various modes, while when
> > > it's toggled off, it'll clean up itself and keep the device as if it
> > > were connected to hid-generic. Bonus point, this removes the need for
> > > the simple user space tool that enables the mode.
> > 
> > That is doable, but that sysfs/parameter can be changed by a non-root
> > user? I looked for a udev rule to grant access to those but found
> > nothing.
> > 
> > IIUC, when this parameter is false the driver will do nothing, right?
> > The user will just need to change it to true to be able to use it, but
> > that will have to be done by root.
> > 
> > I'll try doing this, but I'd appreciate your advice about what approach
> > would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
> > 
> > > > At the end of the day, I think that it is up to Valve what to do.
> > > 
> > > Again, Valve is a big player here, but do not underestimate other
> > > projects (like RetroArch mentioned above) because if you break their
> > > workflow, they will have the right to request a revert of the commit
> > > because it breaks some random user playing games in the far end of
> > > Antarctica (yes, penguins do love to play games :-P )
> > 
> > And everybody loves penguins! If we take away Steam (say a RaspberryPi
> > as a canonical example) and disable the lizard mode, then this driver is
> > just a regular gamepad. RetroArch should be happy with that. Unless they
> > already have an user mode driver for the steam-controller, of course...
> 
> Both of these things seem reasonable to me, with a few caveats:
> 
>  - If there's an opt-in mechanism available, it would be good to ensure we
> have a way to reliably query its state without requiring extra permissions.
> This way, if we know it's likely to affect Steam client functionality, we'll
> have the right mechanism to properly message the situation to users.
> 
>  - If you find a way for the client to be able to program an opt out when
> it's running that is not automatic (eg. by detecting the hidraw device being
> opened), we'd be happy to participate with that scheme assuming it doesn't
> require extra permissions. As soon as the API is figured out, we can include
> it in the client, just send me a heads-up. The one thing that I'd be
> cautious of is robust behavior against abnormal client termination. If it's
> a sysfs entry we have to poke, I'm worried that if the client crashes we
> might not always be able to opt the driver back out. It'd be nice if it was
> based on opening an fd instead, this way the kernel would robustly clean up
> after us and your driver would kick back in.

Ok, I've written the following scheme:
 * I've added a member to struct steam_device: int hid_usage_count.
 * Whenver I call hid_hw_open(), hid_usage_count is incremented.
 * Whenver I call hid_hw_close(), hid_usage_count is decremented.

Now, with this little function:

static bool steam_is_only_client(struct steam_device *steam)
{
	unsigned int count = steam->hdev->ll_open_count;
	return count == steam->hid_usage_count;
}

I can check if the hidraw device is opened from user-space. It is nice
because it works not only for SteamClient, but any other user-space
hid driver. And it is resistent to crashes, too.

It is a bit hacky because I don't think ll_open_count is intended for
that, and it is usually protected by a mutex...  The proper way, IMO,
would be to have a callback for when the hidraw is first opened/last
closed, but that does not exist, AFAICT.

But hey, it works. The mutexless access is not a big deal, because I
call this function on every input report, so I will get the right value
eventually.

Then if I am the only user, I can disable/enable the lizard mode when
opening/closing the input device. Moreover if hidraw is opened, then I
bypass the input events, and this gamepad goes idle, preventing the
double input problem. It's a bit tricky in the details, but I think I
got it right.

If you don't think this solution is too hacky, I'll post a reroll with
this code, in a few days. I still have to do a few more tests.

Now, what I would really want is a review by Valve of my set-lizard function:

static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
{
	if (enabled) {
		steam_send_report_byte(steam, 0x8e); //enable mouse
		steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
	} else {
		steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
		steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
	}
}

While it works, I find its asymmetry quite uncanny. I'm afraid that some
of these are there for a side effect, this is not their real purpose.
Could you give me a hint about this?

> Note that there's a general desire on our side to create a reference
> userspace implementation that would more or less have the current
> functionality of the Steam client, but would be easily usable from other
> platforms where the client doesn't currentl run. Unfortunately it's quite a
> bit of work, so it's unclear what the timeframe would be, if it ever does
> happen.

Do you mean the piece of Steam Client that does input-mapping, but as a
portable program without the full client? That would be awesome! And if
open-sourced, even awesomer!!

Thank you all.
Rodrigo

> Thanks,
>  - Pierre-Loup
> 
> > 
> > Best regards.
> > Rodrigo
> > 
> > > Cheers,
> > > Benjamin
> > > 
> > > > Best Regards.
> > > > Rodrigo.
> > > > 
> > > > > Also the button and axis codes do not match the gamepad API doc
> > > > > (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
> > > > > 
> > > > > > 
> > > > > > For full reference, I'm adding a full changelog of this patchset.
> > > > > > 
> > > > > > Changes in v5:
> > > > > >   * Fix license SPDX to GPL-2.0+.
> > > > > >   * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> > > > > > 
> > > > > > Changes in v4:
> > > > > >   * Add command to check the wireless connection status on probe, without
> > > > > >     waiting for a message (thanks to Clément Vuchener for the tip).
> > > > > >   * Removed the error code on redundant connection/disconnection messages. That
> > > > > >     was harmless but polluted dmesg.
> > > > > >   * Added buttons for touching the left-pad and right-pad.
> > > > > >   * Fixed a misplaced #include from 2/4 to 1/4.
> > > > > > 
> > > > > > Changes in v3:
> > > > > >   * Use RCU to do the dynamic connec/disconnect of wireless devices.
> > > > > >   * Remove entries in hid-quirks.c as they are no longer needed. This allows
> > > > > >     this module to be blacklisted without side effects.
> > > > > >   * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> > > > > >     existing use cases (lizard mode). A user-space tool to do that is
> > > > > >     linked.
> > > > > >   * Fully separated axes for joystick and left-pad. As it happens.
> > > > > >   * Add fuzz values for left/right pad axes, they are a little wiggly.
> > > > > > 
> > > > > > Changes in v2:
> > > > > >   * Remove references to USB. Now the interesting interfaces are selected by
> > > > > >     looking for the ones with feature reports.
> > > > > >   * Feature reports buffers are allocated with hid_alloc_report_buf().
> > > > > >   * Feature report length is checked, to avoid overflows in case of
> > > > > >     corrupt/malicius USB devices.
> > > > > >   * Resolution added to the ABS axes.
> > > > > >   * A lot of minor cleanups.
> > > > > > 
> > > > > > Rodrigo Rivas Costa (4):
> > > > > >    HID: add driver for Valve Steam Controller
> > > > > >    HID: steam: add serial number information.
> > > > > >    HID: steam: command to check wireless connection
> > > > > >    HID: steam: add battery device.
> > > > > > 
> > > > > >   drivers/hid/Kconfig     |   8 +
> > > > > >   drivers/hid/Makefile    |   1 +
> > > > > >   drivers/hid/hid-ids.h   |   4 +
> > > > > >   drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >   4 files changed, 807 insertions(+)
> > > > > >   create mode 100644 drivers/hid/hid-steam.c
> > > > > > 
> > > > > > --
> > > > > > 2.16.2
> > > > > > 
> > 
> 

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-03-19 21:08 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
>
> Now, what I would really want is a review by Valve of my set-lizard function:
>
> static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> {
>         if (enabled) {
>                 steam_send_report_byte(steam, 0x8e); //enable mouse
>                 steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
>         } else {
>                 steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
>                 steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
>         }
> }
>
> While it works, I find its asymmetry quite uncanny. I'm afraid that some
> of these are there for a side effect, this is not their real purpose.
> Could you give me a hint about this?
>

If I remember correctly, you can also enable the mouse with "87 03 08
00 00". But that do not explain the asymmetry or why there are two
ways of doing it. I always found it weird that the "enable" value was
0x0000 and the "disable" value 0x0007.

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Mon, Mar 19, 2018 at 10:06:09PM +0100, Clément VUCHENER wrote:
> 2018-03-19 21:08 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> > On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
> >
> > Now, what I would really want is a review by Valve of my set-lizard function:
> >
> > static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> > {
> >         if (enabled) {
> >                 steam_send_report_byte(steam, 0x8e); //enable mouse
> >                 steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
> >         } else {
> >                 steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
> >                 steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
> >         }
> > }
> >
> > While it works, I find its asymmetry quite uncanny. I'm afraid that some
> > of these are there for a side effect, this is not their real purpose.
> > Could you give me a hint about this?
> >
> 
> If I remember correctly, you can also enable the mouse with "87 03 08
> 00 00". But that do not explain the asymmetry or why there are two
> ways of doing it. I always found it weird that the "enable" value was
> 0x0000 and the "disable" value 0x0007.

This works fine, thanks. IMO, it is better than command 0x8e.
 I also found that register 0x07 controls the cursor keys emulation:
  * 87 03 07 07 00: disable
  * 87 03 07 03 00: enable (03 or 03
  * 87 03 07 00 00: set "joystick mode" (?)

But I cannot find a similar register to disable the enter/esc keys, for
that I still need commands 0x85 and 0x81. 

Can you tell me if there is a register to configure the enter/esc
emulation? That would be nice, because I could enable disable the lizard
mode with a single report: (87 09 07 07 00 08 07 00 xx 07 00).

Thanks.
Rodrigo

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Benjamin Tissoires]

Hi Rodrigo,

On Mon, Mar 19, 2018 at 9:08 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
>>
>>
>> On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
>> > On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
>> > > On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
>> > > <rodrigorivascosta@gmail.com> wrote:
>> > > > On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
>> > > > > 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
>> > > > > > This patchset implements a driver for Valve Steam Controller, based on a
>> > > > > > reverse analysis by myself.
>> > > > > >
>> > > > > > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>> > > > > >
>> > > > > > @Pierre-Loup and @Clément, could you please have another look at this and
>> > > > > > check if it is worthy? Benjamin will not commit it without an express ACK from
>> > > > > > Valve. Of course he is right to be cautious, but I checked this driver with
>> > > > > > the Steam Client and all seems to go just fine. I think that there is a lot of
>> > > > > > Linux out of the desktop that could use this driver and cannot use the Steam
>> > > > > > Client. Worst case scenario, this driver can now be blacklisted, but I hope
>> > > > > > that will not be needed.
>> > > > >
>> > > > > I tested the driver with my 4.15 fedora kernel (I only built the
>> > > > > module not the whole kernel) and I got double inputs (your driver
>> > > > > input device + steam uinput device) when testing Shovel Knight with
>> > > > > Steam Big Picture. It seems to work fine when the inputs are the same,
>> > > > > but after changing the controller configuration in Steam, the issue
>> > > > > became apparent.
>> > > >
>> > > > I assumed that when several joysticks are available, games would listen
>> > > > to one one of them. It looks like I'm wrong, and some (many?) games will
>> > > > listen to all available joysticks at the same time. Thus having two
>> > > > logical joysticks that represent the same physical one is not good.
>> > >
>> > > Yeah, the general rule of thumb is "think of the worst thing that can
>> > > happen, someone will do something worst".
>> > >
>> > > >
>> > > > An easy solution would be that Steam Client grabs this driver
>> > > > (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
>> > > > would be that Steam Client blacklists this driver, of course.
>> > >
>> > > This is 2 solutions that rely on a userspace change, and this is not
>> > > acceptable in its current form. What if people do not upgrade Steam
>> > > client but upgrade their kernel? Well, Steam might be able to force
>> > > people to always run the latest shiny available version, but for other
>> > > games, you can't expect people to have a compatible version of the
>> > > userspace stack.
>> >
>> > Well, if you don't have Steam then you don't have the double input in
>> > the first place. Unless you are using a different user-mode driver, of
>> > course.
>> > >
>> > > Also, "blacklisting the driver" from Steam client is something the OS
>> > > can do, but not the client when you run on a different distribution.
>> > > You need root for that, and I don't want to give root permissions to
>> > > Steam (or to any user space client that shouldn't have root privileges
>> > > for what it matters).
>> >
>> > Actually Steam needs a system installation that adds udev rules to grant
>> > uaccess to /dev/uinput and the USB raw device for the controller.
>> > Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
>> > a bit inconvenient because you'll need a distro update of the steam
>> > package, not just the usual user-mode-only auto-update.
>>
>> It's definitely a bit tricky; we've rolled out an update to our reference
>> package whenever we've added support for new devices (the final Steam
>> Controller, direct PS4 gamepad led/gyro access through HID, HTC Vive and its
>> myriad of onboard devices, bootloaders of all these things for firmware
>> updates, etc). Whenever we have to do that, the rollout never is as smooth
>> as desired: many users aren't using our own package; even on the
>> distributions we support directly. Then downstream distributions adopt these
>> udev changes with various delays (sometimes never), and port them to their
>> own mechanism of doing things, since everyone has their own idea of a robust
>> security model. I wish local sessions always had proper access to HID
>> devices connected to the physical computer the user is sitting at, but I
>> realize that the basic gaming desktop is just one of many usecases distros
>> out there have to support and it'd be unreasonable to expect them to focus
>> exclusively on it.
>
> I was afraid of something like that. Let's forget about that option for
> the moment.
>
>> > > > > And without Steam and your external tool, you get double inputs too. I
>> > > > > tried RetroArch and it was unusable because of the keyboard inputs
>> > > > > from the lizard mode (e.g. pressing B also presses Esc and quits
>> > > > > RetroArch). Having to download and compile an external tool to make
>> > > > > the driver work properly may be too difficult for the user. Your goal
>> > > > > was to provide an alternative to user space drivers but now you
>> > > > > actually depend on (a very simple) one.
>> > > >
>> > > > Yes, I noticed that. TBH, this driver without Steam Client or the
>> > > > user-space tool is not very nice, precisely because you'll get constant
>> > > > Escape and Enter presses, and most games react to those.
>> > > >
>> > > > Frankly speaking, I'm not sure how to proceed. I can think of the
>> > > > following options:
>> > > >   1.Steam Client installation could add a file to blacklist
>> > > >     hid-steam, just as it adds a few udev rules.
>> > >
>> > > But what about RetroArch? And what if you install Steam but want to
>> > > play SDL games that could benefit from your driver?
>> >
>> > That is an issue of solution 1. I actually have the module blacklisted
>> > in my PC, and run `sudo modprobe hid-steam` to use SDL.
>> >
>> > > >   2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
>> > > >     on the architectures for which there is a Steam Client available.
>> > > >     This way DIY projects will still be able to use it.
>> > >
>> > > But this will make the decision to include or not the driver in
>> > > distributions harder. And if no distribution uses it, you won't have
>> > > free tests, and you will be alone to maintain it. So that's not ideal
>> > > either
>> >
>> > Could we set the default to 'y' in non-PC systems. It would be enabled
>> > in my Raspbian, for example... better than nothing.
>> > >
>> > > >   3.This driver could be abandoned :-(. Just use Steam Client if possible or
>> > > >     any of the user-mode drivers available.
>> > >
>> > > This would be a waste for everybody as it's always better when we share.
>> >
>> > Indeed!
>> >
>> > I tried a new option:
>> >    4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
>> >       if that is the case it will disable itself. If the DEVUSB/HIDRAW is
>> >       not in use, then the driver will work normally. A bit hackish maybe
>> >       but it should work.
>> >
>> > I tried doing this option 4, but I'm unable to do it properly. I don't
>> > even know if it is possible...
>> >
>> > > >
>> > > > If we decide for 1 or 2, then the lizard mode could be disabled without
>> > > > ill effects. We could even enable the gyro and all the other gadgets
>> > > > without worring about current compatibility.
>> > >
>> > > To me, 1 is out of the question. The kernel can't expect a user space
>> > > change especially if you are knowingly introducing a bug for the end
>> > > user.
>> > >
>> > > 2 is still on the table IMO, and 3 would be a shame.
>> > >
>> > > I know we already discussed about sysfs and module parameters, but if
>> > > the driver will conflict with a userspace stack, the only way would be
>> > > to have a (dynamic) parameter "enhanced_mode" or
>> > > "kernel_awesome_support" or whatever which would be a boolean, that
>> > > defaults to false that Steam can eventually lookup if they want so in
>> > > the future we can default it to true. When this parameter is set, the
>> > > driver will create the inputs and toggle the various modes, while when
>> > > it's toggled off, it'll clean up itself and keep the device as if it
>> > > were connected to hid-generic. Bonus point, this removes the need for
>> > > the simple user space tool that enables the mode.
>> >
>> > That is doable, but that sysfs/parameter can be changed by a non-root
>> > user? I looked for a udev rule to grant access to those but found
>> > nothing.
>> >
>> > IIUC, when this parameter is false the driver will do nothing, right?
>> > The user will just need to change it to true to be able to use it, but
>> > that will have to be done by root.
>> >
>> > I'll try doing this, but I'd appreciate your advice about what approach
>> > would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
>> >
>> > > > At the end of the day, I think that it is up to Valve what to do.
>> > >
>> > > Again, Valve is a big player here, but do not underestimate other
>> > > projects (like RetroArch mentioned above) because if you break their
>> > > workflow, they will have the right to request a revert of the commit
>> > > because it breaks some random user playing games in the far end of
>> > > Antarctica (yes, penguins do love to play games :-P )
>> >
>> > And everybody loves penguins! If we take away Steam (say a RaspberryPi
>> > as a canonical example) and disable the lizard mode, then this driver is
>> > just a regular gamepad. RetroArch should be happy with that. Unless they
>> > already have an user mode driver for the steam-controller, of course...
>>
>> Both of these things seem reasonable to me, with a few caveats:
>>
>>  - If there's an opt-in mechanism available, it would be good to ensure we
>> have a way to reliably query its state without requiring extra permissions.
>> This way, if we know it's likely to affect Steam client functionality, we'll
>> have the right mechanism to properly message the situation to users.
>>
>>  - If you find a way for the client to be able to program an opt out when
>> it's running that is not automatic (eg. by detecting the hidraw device being
>> opened), we'd be happy to participate with that scheme assuming it doesn't

The only concern I have with detecting if hidraw is opened or not is
that there are other tools that will want to open hidraw to do
something entirely different. But I agree, this is the least intrusive
solution as far as I can see.

>> require extra permissions. As soon as the API is figured out, we can include
>> it in the client, just send me a heads-up. The one thing that I'd be
>> cautious of is robust behavior against abnormal client termination. If it's
>> a sysfs entry we have to poke, I'm worried that if the client crashes we
>> might not always be able to opt the driver back out. It'd be nice if it was
>> based on opening an fd instead, this way the kernel would robustly clean up
>> after us and your driver would kick back in.
>
> Ok, I've written the following scheme:
>  * I've added a member to struct steam_device: int hid_usage_count.
>  * Whenver I call hid_hw_open(), hid_usage_count is incremented.
>  * Whenver I call hid_hw_close(), hid_usage_count is decremented.
>
> Now, with this little function:
>
> static bool steam_is_only_client(struct steam_device *steam)
> {
>         unsigned int count = steam->hdev->ll_open_count;
>         return count == steam->hid_usage_count;
> }
>
> I can check if the hidraw device is opened from user-space. It is nice
> because it works not only for SteamClient, but any other user-space
> hid driver. And it is resistent to crashes, too.
>
> It is a bit hacky because I don't think ll_open_count is intended for

Definitively too hacky for me :)
It seems to be working, but AFAIU, there is still the emulated
keyboard/mouse in place that call hid_hw_open() when they are opened.
So you do not really know if the mismatch comes from extra eyes on the
other hidinput devices or from hidraw.

> that, and it is usually protected by a mutex...  The proper way, IMO,
> would be to have a callback for when the hidraw is first opened/last
> closed, but that does not exist, AFAICT.

The callback doesn't exists because no one actually had the need to.
This particular use case could be a reason to implement such system.
However, I think we should think of the design of the 'callback'
really carefully so it is generic enough to be reused for other cases.

One thing that would be of a help here would be if you overwrite the
low-level transport driver (struct hid_ll_driver) in hid-steam
directly. You basically just duplicate the current ll_driver and
overwrite only the functions you are interested in.
If, for instance, you add a hint to hid_hw_open/close() telling who
opened/closed the device, you could create some new entries in
hid_ll_driver .opening and .closed that would be called before
entering/leaving hid_hw_open/close().
You could even snoop the various requests in hid_hw_(raw_)request() in
case you know what to expect from the client.

This is just an idea and not a formal ack, but it might worth
investigating this path.

>
> But hey, it works. The mutexless access is not a big deal, because I
> call this function on every input report, so I will get the right value
> eventually.

ouch. That's a no from me for this implementation then. Testing it for
every input report seems overkill. Also, I do not understand correctly
how you will send your own commands if the hidraw node is opened.
Could you send the patch as an RFC so we have a good idea of what we
are talking here. But from what I understand, it would be wiser to
have the infrastructure to get notified when the hidraw node is used
by Steam or any other client and do your magic here.

[sometime later]
Actually, if there is no magic command involved to "disable" hid-steam
and let Steam handle the touchpad itself it should be fine. But if the
touchpad is configured in a non default mode, will Steam reset the
controller when it opens the hidraw node? (talking about the current
Steam client, not possible future changes)

>
> Then if I am the only user, I can disable/enable the lizard mode when
> opening/closing the input device. Moreover if hidraw is opened, then I
> bypass the input events, and this gamepad goes idle, preventing the
> double input problem. It's a bit tricky in the details, but I think I
> got it right.

Wouldn't it be better to actually kill the extra input nodes instead
of keeping them around? This way users will only see one Steam
controller joystick. Also, this would prevent the use case where you
open Steam, minimize it (so the joystick are disabled), and then start
an SDL game to wait until you download the latest patch for your
favorite game.
Of course Steam could close the hidraw node on minimize, but it'll be
better IMO to be explicit in this particular case.

>
> If you don't think this solution is too hacky, I'll post a reroll with
> this code, in a few days. I still have to do a few more tests.

Please send the reroll, even if not entirely tested. It'll allow
everyone involved to have a better idea of the solution and will help
everyone to find the correct solution.

Cheers,
Benjamin

>
> Now, what I would really want is a review by Valve of my set-lizard function:
>
> static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> {
>         if (enabled) {
>                 steam_send_report_byte(steam, 0x8e); //enable mouse
>                 steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
>         } else {
>                 steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
>                 steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
>         }
> }
>
> While it works, I find its asymmetry quite uncanny. I'm afraid that some
> of these are there for a side effect, this is not their real purpose.
> Could you give me a hint about this?
>
>> Note that there's a general desire on our side to create a reference
>> userspace implementation that would more or less have the current
>> functionality of the Steam client, but would be easily usable from other
>> platforms where the client doesn't currentl run. Unfortunately it's quite a
>> bit of work, so it's unclear what the timeframe would be, if it ever does
>> happen.
>
> Do you mean the piece of Steam Client that does input-mapping, but as a
> portable program without the full client? That would be awesome! And if
> open-sourced, even awesomer!!
>
> Thank you all.
> Rodrigo
>
>> Thanks,
>>  - Pierre-Loup
>>
>> >
>> > Best regards.
>> > Rodrigo
>> >
>> > > Cheers,
>> > > Benjamin
>> > >
>> > > > Best Regards.
>> > > > Rodrigo.
>> > > >
>> > > > > Also the button and axis codes do not match the gamepad API doc
>> > > > > (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
>> > > > >
>> > > > > >
>> > > > > > For full reference, I'm adding a full changelog of this patchset.
>> > > > > >
>> > > > > > Changes in v5:
>> > > > > >   * Fix license SPDX to GPL-2.0+.
>> > > > > >   * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>> > > > > >
>> > > > > > Changes in v4:
>> > > > > >   * Add command to check the wireless connection status on probe, without
>> > > > > >     waiting for a message (thanks to Clément Vuchener for the tip).
>> > > > > >   * Removed the error code on redundant connection/disconnection messages. That
>> > > > > >     was harmless but polluted dmesg.
>> > > > > >   * Added buttons for touching the left-pad and right-pad.
>> > > > > >   * Fixed a misplaced #include from 2/4 to 1/4.
>> > > > > >
>> > > > > > Changes in v3:
>> > > > > >   * Use RCU to do the dynamic connec/disconnect of wireless devices.
>> > > > > >   * Remove entries in hid-quirks.c as they are no longer needed. This allows
>> > > > > >     this module to be blacklisted without side effects.
>> > > > > >   * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>> > > > > >     existing use cases (lizard mode). A user-space tool to do that is
>> > > > > >     linked.
>> > > > > >   * Fully separated axes for joystick and left-pad. As it happens.
>> > > > > >   * Add fuzz values for left/right pad axes, they are a little wiggly.
>> > > > > >
>> > > > > > Changes in v2:
>> > > > > >   * Remove references to USB. Now the interesting interfaces are selected by
>> > > > > >     looking for the ones with feature reports.
>> > > > > >   * Feature reports buffers are allocated with hid_alloc_report_buf().
>> > > > > >   * Feature report length is checked, to avoid overflows in case of
>> > > > > >     corrupt/malicius USB devices.
>> > > > > >   * Resolution added to the ABS axes.
>> > > > > >   * A lot of minor cleanups.
>> > > > > >
>> > > > > > Rodrigo Rivas Costa (4):
>> > > > > >    HID: add driver for Valve Steam Controller
>> > > > > >    HID: steam: add serial number information.
>> > > > > >    HID: steam: command to check wireless connection
>> > > > > >    HID: steam: add battery device.
>> > > > > >
>> > > > > >   drivers/hid/Kconfig     |   8 +
>> > > > > >   drivers/hid/Makefile    |   1 +
>> > > > > >   drivers/hid/hid-ids.h   |   4 +
>> > > > > >   drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>> > > > > >   4 files changed, 807 insertions(+)
>> > > > > >   create mode 100644 drivers/hid/hid-steam.c
>> > > > > >
>> > > > > > --
>> > > > > > 2.16.2
>> > > > > >
>> >
>>

Re: [PATCH v5 0/4] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Wed, Mar 21, 2018 at 04:47:53PM +0100, Benjamin Tissoires wrote:
> Hi Rodrigo,
> 
> On Mon, Mar 19, 2018 at 9:08 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
> >>
> >>
> >> On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
> >> > On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
> >> > > On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
> >> > > <rodrigorivascosta@gmail.com> wrote:
> >> > > > On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
> >> > > > > 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> >> > > > > > This patchset implements a driver for Valve Steam Controller, based on a
> >> > > > > > reverse analysis by myself.
> >> > > > > >
> >> > > > > > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> >> > > > > >
> >> > > > > > @Pierre-Loup and @Clément, could you please have another look at this and
> >> > > > > > check if it is worthy? Benjamin will not commit it without an express ACK from
> >> > > > > > Valve. Of course he is right to be cautious, but I checked this driver with
> >> > > > > > the Steam Client and all seems to go just fine. I think that there is a lot of
> >> > > > > > Linux out of the desktop that could use this driver and cannot use the Steam
> >> > > > > > Client. Worst case scenario, this driver can now be blacklisted, but I hope
> >> > > > > > that will not be needed.
> >> > > > >
> >> > > > > I tested the driver with my 4.15 fedora kernel (I only built the
> >> > > > > module not the whole kernel) and I got double inputs (your driver
> >> > > > > input device + steam uinput device) when testing Shovel Knight with
> >> > > > > Steam Big Picture. It seems to work fine when the inputs are the same,
> >> > > > > but after changing the controller configuration in Steam, the issue
> >> > > > > became apparent.
> >> > > >
> >> > > > I assumed that when several joysticks are available, games would listen
> >> > > > to one one of them. It looks like I'm wrong, and some (many?) games will
> >> > > > listen to all available joysticks at the same time. Thus having two
> >> > > > logical joysticks that represent the same physical one is not good.
> >> > >
> >> > > Yeah, the general rule of thumb is "think of the worst thing that can
> >> > > happen, someone will do something worst".
> >> > >
> >> > > >
> >> > > > An easy solution would be that Steam Client grabs this driver
> >> > > > (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
> >> > > > would be that Steam Client blacklists this driver, of course.
> >> > >
> >> > > This is 2 solutions that rely on a userspace change, and this is not
> >> > > acceptable in its current form. What if people do not upgrade Steam
> >> > > client but upgrade their kernel? Well, Steam might be able to force
> >> > > people to always run the latest shiny available version, but for other
> >> > > games, you can't expect people to have a compatible version of the
> >> > > userspace stack.
> >> >
> >> > Well, if you don't have Steam then you don't have the double input in
> >> > the first place. Unless you are using a different user-mode driver, of
> >> > course.
> >> > >
> >> > > Also, "blacklisting the driver" from Steam client is something the OS
> >> > > can do, but not the client when you run on a different distribution.
> >> > > You need root for that, and I don't want to give root permissions to
> >> > > Steam (or to any user space client that shouldn't have root privileges
> >> > > for what it matters).
> >> >
> >> > Actually Steam needs a system installation that adds udev rules to grant
> >> > uaccess to /dev/uinput and the USB raw device for the controller.
> >> > Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
> >> > a bit inconvenient because you'll need a distro update of the steam
> >> > package, not just the usual user-mode-only auto-update.
> >>
> >> It's definitely a bit tricky; we've rolled out an update to our reference
> >> package whenever we've added support for new devices (the final Steam
> >> Controller, direct PS4 gamepad led/gyro access through HID, HTC Vive and its
> >> myriad of onboard devices, bootloaders of all these things for firmware
> >> updates, etc). Whenever we have to do that, the rollout never is as smooth
> >> as desired: many users aren't using our own package; even on the
> >> distributions we support directly. Then downstream distributions adopt these
> >> udev changes with various delays (sometimes never), and port them to their
> >> own mechanism of doing things, since everyone has their own idea of a robust
> >> security model. I wish local sessions always had proper access to HID
> >> devices connected to the physical computer the user is sitting at, but I
> >> realize that the basic gaming desktop is just one of many usecases distros
> >> out there have to support and it'd be unreasonable to expect them to focus
> >> exclusively on it.
> >
> > I was afraid of something like that. Let's forget about that option for
> > the moment.
> >
> >> > > > > And without Steam and your external tool, you get double inputs too. I
> >> > > > > tried RetroArch and it was unusable because of the keyboard inputs
> >> > > > > from the lizard mode (e.g. pressing B also presses Esc and quits
> >> > > > > RetroArch). Having to download and compile an external tool to make
> >> > > > > the driver work properly may be too difficult for the user. Your goal
> >> > > > > was to provide an alternative to user space drivers but now you
> >> > > > > actually depend on (a very simple) one.
> >> > > >
> >> > > > Yes, I noticed that. TBH, this driver without Steam Client or the
> >> > > > user-space tool is not very nice, precisely because you'll get constant
> >> > > > Escape and Enter presses, and most games react to those.
> >> > > >
> >> > > > Frankly speaking, I'm not sure how to proceed. I can think of the
> >> > > > following options:
> >> > > >   1.Steam Client installation could add a file to blacklist
> >> > > >     hid-steam, just as it adds a few udev rules.
> >> > >
> >> > > But what about RetroArch? And what if you install Steam but want to
> >> > > play SDL games that could benefit from your driver?
> >> >
> >> > That is an issue of solution 1. I actually have the module blacklisted
> >> > in my PC, and run `sudo modprobe hid-steam` to use SDL.
> >> >
> >> > > >   2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
> >> > > >     on the architectures for which there is a Steam Client available.
> >> > > >     This way DIY projects will still be able to use it.
> >> > >
> >> > > But this will make the decision to include or not the driver in
> >> > > distributions harder. And if no distribution uses it, you won't have
> >> > > free tests, and you will be alone to maintain it. So that's not ideal
> >> > > either
> >> >
> >> > Could we set the default to 'y' in non-PC systems. It would be enabled
> >> > in my Raspbian, for example... better than nothing.
> >> > >
> >> > > >   3.This driver could be abandoned :-(. Just use Steam Client if possible or
> >> > > >     any of the user-mode drivers available.
> >> > >
> >> > > This would be a waste for everybody as it's always better when we share.
> >> >
> >> > Indeed!
> >> >
> >> > I tried a new option:
> >> >    4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
> >> >       if that is the case it will disable itself. If the DEVUSB/HIDRAW is
> >> >       not in use, then the driver will work normally. A bit hackish maybe
> >> >       but it should work.
> >> >
> >> > I tried doing this option 4, but I'm unable to do it properly. I don't
> >> > even know if it is possible...
> >> >
> >> > > >
> >> > > > If we decide for 1 or 2, then the lizard mode could be disabled without
> >> > > > ill effects. We could even enable the gyro and all the other gadgets
> >> > > > without worring about current compatibility.
> >> > >
> >> > > To me, 1 is out of the question. The kernel can't expect a user space
> >> > > change especially if you are knowingly introducing a bug for the end
> >> > > user.
> >> > >
> >> > > 2 is still on the table IMO, and 3 would be a shame.
> >> > >
> >> > > I know we already discussed about sysfs and module parameters, but if
> >> > > the driver will conflict with a userspace stack, the only way would be
> >> > > to have a (dynamic) parameter "enhanced_mode" or
> >> > > "kernel_awesome_support" or whatever which would be a boolean, that
> >> > > defaults to false that Steam can eventually lookup if they want so in
> >> > > the future we can default it to true. When this parameter is set, the
> >> > > driver will create the inputs and toggle the various modes, while when
> >> > > it's toggled off, it'll clean up itself and keep the device as if it
> >> > > were connected to hid-generic. Bonus point, this removes the need for
> >> > > the simple user space tool that enables the mode.
> >> >
> >> > That is doable, but that sysfs/parameter can be changed by a non-root
> >> > user? I looked for a udev rule to grant access to those but found
> >> > nothing.
> >> >
> >> > IIUC, when this parameter is false the driver will do nothing, right?
> >> > The user will just need to change it to true to be able to use it, but
> >> > that will have to be done by root.
> >> >
> >> > I'll try doing this, but I'd appreciate your advice about what approach
> >> > would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
> >> >
> >> > > > At the end of the day, I think that it is up to Valve what to do.
> >> > >
> >> > > Again, Valve is a big player here, but do not underestimate other
> >> > > projects (like RetroArch mentioned above) because if you break their
> >> > > workflow, they will have the right to request a revert of the commit
> >> > > because it breaks some random user playing games in the far end of
> >> > > Antarctica (yes, penguins do love to play games :-P )
> >> >
> >> > And everybody loves penguins! If we take away Steam (say a RaspberryPi
> >> > as a canonical example) and disable the lizard mode, then this driver is
> >> > just a regular gamepad. RetroArch should be happy with that. Unless they
> >> > already have an user mode driver for the steam-controller, of course...
> >>
> >> Both of these things seem reasonable to me, with a few caveats:
> >>
> >>  - If there's an opt-in mechanism available, it would be good to ensure we
> >> have a way to reliably query its state without requiring extra permissions.
> >> This way, if we know it's likely to affect Steam client functionality, we'll
> >> have the right mechanism to properly message the situation to users.
> >>
> >>  - If you find a way for the client to be able to program an opt out when
> >> it's running that is not automatic (eg. by detecting the hidraw device being
> >> opened), we'd be happy to participate with that scheme assuming it doesn't
> 
> The only concern I have with detecting if hidraw is opened or not is
> that there are other tools that will want to open hidraw to do
> something entirely different. But I agree, this is the least intrusive
> solution as far as I can see.
> 
> >> require extra permissions. As soon as the API is figured out, we can include
> >> it in the client, just send me a heads-up. The one thing that I'd be
> >> cautious of is robust behavior against abnormal client termination. If it's
> >> a sysfs entry we have to poke, I'm worried that if the client crashes we
> >> might not always be able to opt the driver back out. It'd be nice if it was
> >> based on opening an fd instead, this way the kernel would robustly clean up
> >> after us and your driver would kick back in.
> >
> > Ok, I've written the following scheme:
> >  * I've added a member to struct steam_device: int hid_usage_count.
> >  * Whenver I call hid_hw_open(), hid_usage_count is incremented.
> >  * Whenver I call hid_hw_close(), hid_usage_count is decremented.
> >
> > Now, with this little function:
> >
> > static bool steam_is_only_client(struct steam_device *steam)
> > {
> >         unsigned int count = steam->hdev->ll_open_count;
> >         return count == steam->hid_usage_count;
> > }
> >
> > I can check if the hidraw device is opened from user-space. It is nice
> > because it works not only for SteamClient, but any other user-space
> > hid driver. And it is resistent to crashes, too.
> >
> > It is a bit hacky because I don't think ll_open_count is intended for
> 
> Definitively too hacky for me :)
> It seems to be working, but AFAIU, there is still the emulated
> keyboard/mouse in place that call hid_hw_open() when they are opened.
> So you do not really know if the mismatch comes from extra eyes on the
> other hidinput devices or from hidraw.

Not really, the emulated keyboard/mouse are different USB interfaces, so
they are separeted hidraw devices, and they do not add to the
ll_open_count.
 
> > that, and it is usually protected by a mutex...  The proper way, IMO,
> > would be to have a callback for when the hidraw is first opened/last
> > closed, but that does not exist, AFAICT.
> 
> The callback doesn't exists because no one actually had the need to.
> This particular use case could be a reason to implement such system.
> However, I think we should think of the design of the 'callback'
> really carefully so it is generic enough to be reused for other cases.
> 
> One thing that would be of a help here would be if you overwrite the
> low-level transport driver (struct hid_ll_driver) in hid-steam
> directly. You basically just duplicate the current ll_driver and
> overwrite only the functions you are interested in.
> If, for instance, you add a hint to hid_hw_open/close() telling who
> opened/closed the device, you could create some new entries in
> hid_ll_driver .opening and .closed that would be called before
> entering/leaving hid_hw_open/close().

That sounds interesting. I'll try if I can make it work.

> You could even snoop the various requests in hid_hw_(raw_)request() in
> case you know what to expect from the client.

Without a full protocol descrition, it may be too risky... maybe I could
wait until the client sends a feature report, to count it as a
worthy client.

> This is just an idea and not a formal ack, but it might worth
> investigating this path.
> 
> >
> > But hey, it works. The mutexless access is not a big deal, because I
> > call this function on every input report, so I will get the right value
> > eventually.
> 
> ouch. That's a no from me for this implementation then. Testing it for
> every input report seems overkill. Also, I do not understand correctly
> how you will send your own commands if the hidraw node is opened.
> Could you send the patch as an RFC so we have a good idea of what we
> are talking here. But from what I understand, it would be wiser to
> have the infrastructure to get notified when the hidraw node is used
> by Steam or any other client and do your magic here.

I'm not using the mutex, so it is just a unsigned comparison...
I'll post the code later today. Then, if I manage the hid_ll I will
repost.

> [sometime later]
> Actually, if there is no magic command involved to "disable" hid-steam
> and let Steam handle the touchpad itself it should be fine. But if the
> touchpad is configured in a non default mode, will Steam reset the
> controller when it opens the hidraw node? (talking about the current
> Steam client, not possible future changes)

Right, when I detect another client while hid-steam is in use, I just
sit there doing nothing, and skipping my events. OTHO, when I detect the
client is closed and hid-steam is still in use, then I disable the
lizard mode, just as if hid-steam is closed-reopened.

And I think the Steam Client is prepared for that, and resets the device
upon opening it. I've never seen Steam Client not being able to
configure the controller, and I've done some crazy configurations.

> >
> > Then if I am the only user, I can disable/enable the lizard mode when
> > opening/closing the input device. Moreover if hidraw is opened, then I
> > bypass the input events, and this gamepad goes idle, preventing the
> > double input problem. It's a bit tricky in the details, but I think I
> > got it right.
> 
> Wouldn't it be better to actually kill the extra input nodes instead
> of keeping them around? This way users will only see one Steam
> controller joystick. Also, this would prevent the use case where you
> open Steam, minimize it (so the joystick are disabled), and then start
> an SDL game to wait until you download the latest patch for your
> favorite game.

I thought of that, but I see problems there. Imagine a game that
register for changes in the input devices: when anything changes, it
closes and reopens all of them. That is:
 * The game opens all the input devices, including the hid-steam.
 * The game opens the hidraw, because it has a user-mode driver.
 * hid-steam detects a user-mode client so it deletes the mouse/keyboard
   input nodes.
 * The game detects that input devices have changed, closes all of them
   and repeats.

And you have an infinite loop.

Instead, I'm now using feature reports to the controller to
disable/enable the lizard mode, just as the Steam Client does.

> Of course Steam could close the hidraw node on minimize, but it'll be
> better IMO to be explicit in this particular case.

Steam does not close hidraw on minimize. Instead, it changes its uinput
emulation into "desktop mode". When a game is launched it changes too,
but I'm not sure about the details... Anyway, Steam Client closes the
hidraw only when it is fully closed with the "Exit Steam" option.

> > If you don't think this solution is too hacky, I'll post a reroll with
> > this code, in a few days. I still have to do a few more tests.
> 
> Please send the reroll, even if not entirely tested. It'll allow
> everyone involved to have a better idea of the solution and will help
> everyone to find the correct solution.

I'll do today, if I have the time.
Best regards.
Rodrigo

> Cheers,
> Benjamin
> 
> >
> > Now, what I would really want is a review by Valve of my set-lizard function:
> >
> > static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> > {
> >         if (enabled) {
> >                 steam_send_report_byte(steam, 0x8e); //enable mouse
> >                 steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
> >         } else {
> >                 steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
> >                 steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
> >         }
> > }
> >
> > While it works, I find its asymmetry quite uncanny. I'm afraid that some
> > of these are there for a side effect, this is not their real purpose.
> > Could you give me a hint about this?
> >
> >> Note that there's a general desire on our side to create a reference
> >> userspace implementation that would more or less have the current
> >> functionality of the Steam client, but would be easily usable from other
> >> platforms where the client doesn't currentl run. Unfortunately it's quite a
> >> bit of work, so it's unclear what the timeframe would be, if it ever does
> >> happen.
> >
> > Do you mean the piece of Steam Client that does input-mapping, but as a
> > portable program without the full client? That would be awesome! And if
> > open-sourced, even awesomer!!
> >
> > Thank you all.
> > Rodrigo
> >
> >> Thanks,
> >>  - Pierre-Loup
> >>
> >> >
> >> > Best regards.
> >> > Rodrigo
> >> >
> >> > > Cheers,
> >> > > Benjamin
> >> > >
> >> > > > Best Regards.
> >> > > > Rodrigo.
> >> > > >
> >> > > > > Also the button and axis codes do not match the gamepad API doc
> >> > > > > (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
> >> > > > >
> >> > > > > >
> >> > > > > > For full reference, I'm adding a full changelog of this patchset.
> >> > > > > >
> >> > > > > > Changes in v5:
> >> > > > > >   * Fix license SPDX to GPL-2.0+.
> >> > > > > >   * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> >> > > > > >
> >> > > > > > Changes in v4:
> >> > > > > >   * Add command to check the wireless connection status on probe, without
> >> > > > > >     waiting for a message (thanks to Clément Vuchener for the tip).
> >> > > > > >   * Removed the error code on redundant connection/disconnection messages. That
> >> > > > > >     was harmless but polluted dmesg.
> >> > > > > >   * Added buttons for touching the left-pad and right-pad.
> >> > > > > >   * Fixed a misplaced #include from 2/4 to 1/4.
> >> > > > > >
> >> > > > > > Changes in v3:
> >> > > > > >   * Use RCU to do the dynamic connec/disconnect of wireless devices.
> >> > > > > >   * Remove entries in hid-quirks.c as they are no longer needed. This allows
> >> > > > > >     this module to be blacklisted without side effects.
> >> > > > > >   * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> >> > > > > >     existing use cases (lizard mode). A user-space tool to do that is
> >> > > > > >     linked.
> >> > > > > >   * Fully separated axes for joystick and left-pad. As it happens.
> >> > > > > >   * Add fuzz values for left/right pad axes, they are a little wiggly.
> >> > > > > >
> >> > > > > > Changes in v2:
> >> > > > > >   * Remove references to USB. Now the interesting interfaces are selected by
> >> > > > > >     looking for the ones with feature reports.
> >> > > > > >   * Feature reports buffers are allocated with hid_alloc_report_buf().
> >> > > > > >   * Feature report length is checked, to avoid overflows in case of
> >> > > > > >     corrupt/malicius USB devices.
> >> > > > > >   * Resolution added to the ABS axes.
> >> > > > > >   * A lot of minor cleanups.
> >> > > > > >
> >> > > > > > Rodrigo Rivas Costa (4):
> >> > > > > >    HID: add driver for Valve Steam Controller
> >> > > > > >    HID: steam: add serial number information.
> >> > > > > >    HID: steam: command to check wireless connection
> >> > > > > >    HID: steam: add battery device.
> >> > > > > >
> >> > > > > >   drivers/hid/Kconfig     |   8 +
> >> > > > > >   drivers/hid/Makefile    |   1 +
> >> > > > > >   drivers/hid/hid-ids.h   |   4 +
> >> > > > > >   drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> > > > > >   4 files changed, 807 insertions(+)
> >> > > > > >   create mode 100644 drivers/hid/hid-steam.c
> >> > > > > >
> >> > > > > > --
> >> > > > > > 2.16.2
> >> > > > > >
> >> >
> >>