[PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Vicki Pfau]

This allows the touchpad input_dev to be removed and have the driver remain
functional without its presence. This will be used to allow the touchpad to
be disabled, e.g. by a module parameter.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-playstation.c | 60 +++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ab7c82c2e886..f859a8dd8a2e 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -29,6 +29,7 @@ static DEFINE_IDA(ps_player_id_allocator);
 struct ps_device {
 	struct list_head list;
 	struct hid_device *hdev;
+	struct mutex mutex;
 	spinlock_t lock;
 
 	uint32_t player_id;
@@ -130,7 +131,7 @@ struct dualsense {
 	struct ps_device base;
 	struct input_dev *gamepad;
 	struct input_dev *sensors;
-	struct input_dev *touchpad;
+	struct input_dev __rcu *touchpad;
 
 	/* Calibration data for accelerometer and gyroscope. */
 	struct ps_calibration_data accel_calib_data[3];
@@ -590,6 +591,22 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
 	return touchpad;
 }
 
+static void dualsense_unregister_touchpad(struct dualsense *ds)
+{
+	struct input_dev *touchpad;
+
+	rcu_read_lock();
+	touchpad = rcu_dereference(ds->touchpad);
+	rcu_read_unlock();
+
+	if (!touchpad)
+		return;
+
+	RCU_INIT_POINTER(ds->touchpad, NULL);
+	synchronize_rcu();
+	input_unregister_device(touchpad);
+}
+
 static ssize_t firmware_version_show(struct device *dev,
 				struct device_attribute
 				*attr, char *buf)
@@ -888,6 +905,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	struct hid_device *hdev = ps_dev->hdev;
 	struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
 	struct dualsense_input_report *ds_report;
+	struct input_dev *touchpad = NULL;
 	uint8_t battery_data, battery_capacity, charging_status, value;
 	int battery_status;
 	uint32_t sensor_timestamp;
@@ -1002,24 +1020,29 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
 	input_sync(ds->sensors);
 
-	for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
-		struct dualsense_touch_point *point = &ds_report->points[i];
-		bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
+	rcu_read_lock();
+	touchpad = rcu_dereference(ds->touchpad);
+	rcu_read_unlock();
+	if (touchpad) {
+		for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
+			struct dualsense_touch_point *point = &ds_report->points[i];
+			bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
 
-		input_mt_slot(ds->touchpad, i);
-		input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
+			input_mt_slot(ds->touchpad, i);
+			input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
 
-		if (active) {
-			int x = (point->x_hi << 8) | point->x_lo;
-			int y = (point->y_hi << 4) | point->y_lo;
+			if (active) {
+				int x = (point->x_hi << 8) | point->x_lo;
+				int y = (point->y_hi << 4) | point->y_lo;
 
-			input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
-			input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
+				input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
+				input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
+			}
 		}
+		input_mt_sync_frame(ds->touchpad);
+		input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
+		input_sync(ds->touchpad);
 	}
-	input_mt_sync_frame(ds->touchpad);
-	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
-	input_sync(ds->touchpad);
 
 	battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
 	charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
@@ -1141,6 +1164,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
 	struct ps_device *ps_dev;
+	struct input_dev *touchpad;
 	uint8_t max_output_report_size;
 	int ret;
 
@@ -1157,6 +1181,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	ps_dev = &ds->base;
 	ps_dev->hdev = hdev;
 	spin_lock_init(&ps_dev->lock);
+	mutex_init(&ps_dev->mutex);
 	ps_dev->battery_capacity = 100; /* initial value until parse_report. */
 	ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
 	ps_dev->parse_report = dualsense_parse_report;
@@ -1204,11 +1229,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
-	ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
-	if (IS_ERR(ds->touchpad)) {
-		ret = PTR_ERR(ds->touchpad);
+	touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
+	if (IS_ERR(touchpad)) {
+		ret = PTR_ERR(touchpad);
 		goto err;
 	}
+	rcu_assign_pointer(ds->touchpad, touchpad);
 
 	ret = ps_device_register_battery(ps_dev);
 	if (ret)
-- 
2.36.0

[PATCH 2/6] HID: hid-playstation: Add touchpad_mouse param

[Vicki Pfau]

Add parameter "touchpad_mouse" to enable disabling or re-enabling exposing the
touchpad input_dev, which can be changed while the module is loaded.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-playstation.c | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index f859a8dd8a2e..ad0da4470615 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -23,6 +23,8 @@ static LIST_HEAD(ps_devices_list);
 
 static DEFINE_IDA(ps_player_id_allocator);
 
+static bool touchpad_mouse = true;
+
 #define HID_PLAYSTATION_VERSION_PATCH 0x8000
 
 /* Base class for playstation devices. */
@@ -1343,6 +1345,45 @@ static void ps_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
+static int ps_param_set_touchpad_mouse(const char *val,
+					const struct kernel_param *kp)
+{
+	struct ps_device *dev;
+	struct dualsense *ds;
+	struct input_dev *touchpad;
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (ret)
+		return ret;
+
+	mutex_lock(&ps_devices_lock);
+	list_for_each_entry(dev, &ps_devices_list, list) {
+		mutex_lock(&dev->mutex);
+		if (dev->hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
+			ds = container_of(dev, struct dualsense, base);
+			if (touchpad_mouse) {
+				touchpad = ps_touchpad_create(dev->hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
+				if (IS_ERR(touchpad))
+					continue;
+				rcu_assign_pointer(ds->touchpad, touchpad);
+			} else
+				dualsense_unregister_touchpad(ds);
+		}
+		mutex_unlock(&dev->mutex);
+	}
+	mutex_unlock(&ps_devices_lock);
+	return 0;
+}
+
+static const struct kernel_param_ops ps_touchpad_mouse_ops = {
+	.set	= ps_param_set_touchpad_mouse,
+	.get	= param_get_bool,
+};
+
+module_param_cb(touchpad_mouse, &ps_touchpad_mouse_ops, &touchpad_mouse, 0644);
+MODULE_PARM_DESC(touchpad_mouse, "Enable mouse emulation using the touchpad");
+
 static const struct hid_device_id ps_devices[] = {
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
-- 
2.36.0

[PATCH 3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

[Vicki Pfau]

When using the hidraw node directly, disable the touchpad endpoint to prevent
it from sending separate mouse-like reports. This is accomplished in the same
way that the hid-steam driver does it, by creating and attaching an input_dev
with a custom low-level transport driver, which monitors and reports when the
hidraw node is opened or closed. Reports sent by the real device are reported
to the "fake" device, and the real device is prevented from creating a hidraw
node. This "fake" device is connected with only a hidraw node, and is exposed
with identifying information that is identical to the original device, so the
"fake" device's hidraw node appears as the node associated with the dev.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ad0da4470615..3746c9c550d6 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
 /* Base class for playstation devices. */
 struct ps_device {
 	struct list_head list;
-	struct hid_device *hdev;
+	struct hid_device *hdev, *client_hdev;
 	struct mutex mutex;
 	spinlock_t lock;
+	bool client_opened;
 
 	uint32_t player_id;
 
@@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
 	.attrs = ps_device_attributes,
 };
 
+static int ps_client_ll_parse(struct hid_device *hdev)
+{
+	struct ps_device *dev = hdev->driver_data;
+
+	return hid_parse_report(hdev, dev->hdev->dev_rdesc,
+			dev->hdev->dev_rsize);
+}
+
+static int ps_client_ll_start(struct hid_device *hdev)
+{
+	return 0;
+}
+
+static void ps_client_ll_stop(struct hid_device *hdev)
+{
+}
+
+static int ps_client_ll_open(struct hid_device *hdev)
+{
+	struct ps_device *dev = hdev->driver_data;
+	struct dualsense *ds;
+
+	mutex_lock(&dev->mutex);
+	dev->client_opened = true;
+	mutex_unlock(&dev->mutex);
+
+	if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
+		ds = container_of(dev, struct dualsense, base);
+		dualsense_unregister_touchpad(ds);
+	}
+
+	return 0;
+}
+
+static void ps_client_ll_close(struct hid_device *hdev)
+{
+	struct ps_device *dev = hdev->driver_data;
+	struct dualsense *ds;
+	struct input_dev *touchpad;
+
+	mutex_lock(&dev->mutex);
+	dev->client_opened = false;
+	mutex_unlock(&dev->mutex);
+
+	if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
+		ds = container_of(dev, struct dualsense, base);
+		touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
+		if (IS_ERR(touchpad))
+			return;
+		rcu_assign_pointer(ds->touchpad, touchpad);
+	}
+}
+
+static int ps_client_ll_raw_request(struct hid_device *hdev,
+				unsigned char reportnum, u8 *buf,
+				size_t count, unsigned char report_type,
+				int reqtype)
+{
+	struct ps_device *dev = hdev->driver_data;
+
+	return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
+			report_type, reqtype);
+}
+
+static struct hid_ll_driver ps_client_ll_driver = {
+	.parse = ps_client_ll_parse,
+	.start = ps_client_ll_start,
+	.stop = ps_client_ll_stop,
+	.open = ps_client_ll_open,
+	.close = ps_client_ll_close,
+	.raw_request = ps_client_ll_raw_request,
+};
+
+static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
+{
+	struct hid_device *client_hdev;
+
+	client_hdev = hid_allocate_device();
+	if (IS_ERR(client_hdev))
+		return client_hdev;
+
+	client_hdev->ll_driver = &ps_client_ll_driver;
+	client_hdev->dev.parent = hdev->dev.parent;
+	client_hdev->bus = hdev->bus;
+	client_hdev->vendor = hdev->vendor;
+	client_hdev->product = hdev->product;
+	client_hdev->version = hdev->version;
+	client_hdev->type = hdev->type;
+	client_hdev->country = hdev->country;
+	strlcpy(client_hdev->name, hdev->name,
+			sizeof(client_hdev->name));
+	strlcpy(client_hdev->phys, hdev->phys,
+			sizeof(client_hdev->phys));
+	return client_hdev;
+}
+
 static int dualsense_get_calibration_data(struct dualsense *ds)
 {
 	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
@@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	INIT_WORK(&ds->output_worker, dualsense_output_worker);
 	hid_set_drvdata(hdev, ds);
 
+	ps_dev->client_hdev = ps_create_client_hid(hdev);
+	if (IS_ERR(ps_dev->client_hdev))
+		return ERR_CAST(ps_dev->client_hdev);
+	ps_dev->client_hdev->driver_data = ps_dev;
+
 	max_output_report_size = sizeof(struct dualsense_output_report_bt);
 	ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
 	if (!ds->output_report_dmabuf)
@@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *data, int size)
 {
 	struct ps_device *dev = hid_get_drvdata(hdev);
+	int ret = 0;
+
+	if (!dev)
+		return 0;
 
-	if (dev && dev->parse_report)
+	if (dev->client_opened) {
+		ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
+		if (ret) {
+			hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (dev->parse_report)
 		return dev->parse_report(dev, report, data, size);
 
 	return 0;
@@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct ps_device *dev;
 	int ret;
+	unsigned int connect_mask = 0;
 
 	ret = hid_parse(hdev);
 	if (ret) {
@@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (hdev->ll_driver == &ps_client_ll_driver)
+		connect_mask = HID_CONNECT_HIDRAW;
+
+	ret = hid_hw_start(hdev, connect_mask);
 	if (ret) {
 		hid_err(hdev, "Failed to start HID device\n");
 		return ret;
 	}
 
+	/*
+	 * The virtual client_dev is only used for hidraw. Since we've already
+	 * started the hw, return early to avoid the recursive probe.
+	 */
+	if (hdev->ll_driver == &ps_client_ll_driver)
+		return ret;
+
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "Failed to open HID device\n");
@@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_close;
 	}
 
+	if (dev->client_hdev)
+		ret = hid_add_device(dev->client_hdev);
+	if (ret) {
+		hid_err(hdev, "Failed to start client device failed\n");
+		goto err_close;
+	}
+
 	return ret;
 
 err_close:
+	if (dev->client_hdev)
+		hid_destroy_device(dev->client_hdev);
+
 	hid_hw_close(hdev);
 err_stop:
 	hid_hw_stop(hdev);
@@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
 	ps_devices_list_remove(dev);
 	ps_device_release_player_id(dev);
 
+	if (dev->client_hdev)
+		hid_destroy_device(dev->client_hdev);
+
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }
-- 
2.36.0

[PATCH 4/6] HID: hid-sony: Allow removal of touchpad

[Vicki Pfau]

This allows the touchpad input_dev to be removed and have the driver remain
functional without its presence. This will be used to allow the touchpad to
be disabled, e.g. by a module parameter.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-sony.c | 163 +++++++++++++++++++++++++++--------------
 1 file changed, 110 insertions(+), 53 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 8319b0ce385a..1c347b3ca992 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -39,6 +39,7 @@
 #include <linux/crc32.h>
 #include <linux/usb.h>
 #include <linux/timer.h>
+#include <linux/rcupdate.h>
 #include <asm/unaligned.h>
 
 #include "hid-ids.h"
@@ -556,7 +557,7 @@ struct sony_sc {
 	spinlock_t lock;
 	struct list_head list_node;
 	struct hid_device *hdev;
-	struct input_dev *touchpad;
+	struct input_dev __rcu *touchpad;
 	struct input_dev *sensor_dev;
 	struct led_classdev *leds[MAX_LEDS];
 	unsigned long quirks;
@@ -565,6 +566,7 @@ struct sony_sc {
 	void (*send_output_report)(struct sony_sc *);
 	struct power_supply *battery;
 	struct power_supply_desc battery_desc;
+	struct mutex mutex;
 	int device_id;
 	unsigned fw_version;
 	bool fw_version_created;
@@ -1041,6 +1043,7 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	struct hid_input *hidinput = list_entry(sc->hdev->inputs.next,
 						struct hid_input, list);
 	struct input_dev *input_dev = hidinput->input;
+	struct input_dev *touchpad = NULL;
 	unsigned long flags;
 	int n, m, offset, num_touch_data, max_touch_data;
 	u8 cable_state, battery_capacity;
@@ -1050,9 +1053,15 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	/* When using Bluetooth the header is 2 bytes longer, so skip these. */
 	int data_offset = (sc->quirks & DUALSHOCK4_CONTROLLER_BT) ? 2 : 0;
 
+	rcu_read_lock();
+	touchpad = rcu_dereference(sc->touchpad);
+	rcu_read_unlock();
+
 	/* Second bit of third button byte is for the touchpad button. */
-	offset = data_offset + DS4_INPUT_REPORT_BUTTON_OFFSET;
-	input_report_key(sc->touchpad, BTN_LEFT, rd[offset+2] & 0x2);
+	if (touchpad) {
+		offset = data_offset + DS4_INPUT_REPORT_BUTTON_OFFSET;
+		input_report_key(touchpad, BTN_LEFT, rd[offset+2] & 0x2);
+	}
 
 	/*
 	 * The default behavior of the Dualshock 4 is to send reports using
@@ -1197,6 +1206,9 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	sc->battery_status = battery_status;
 	spin_unlock_irqrestore(&sc->lock, flags);
 
+	if (!touchpad)
+		return;
+
 	/*
 	 * The Dualshock 4 multi-touch trackpad data starts at offset 33 on USB
 	 * and 35 on Bluetooth.
@@ -1231,24 +1243,25 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 			y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
 
 			active = !(rd[offset] >> 7);
-			input_mt_slot(sc->touchpad, n);
-			input_mt_report_slot_state(sc->touchpad, MT_TOOL_FINGER, active);
+			input_mt_slot(touchpad, n);
+			input_mt_report_slot_state(touchpad, MT_TOOL_FINGER, active);
 
 			if (active) {
-				input_report_abs(sc->touchpad, ABS_MT_POSITION_X, x);
-				input_report_abs(sc->touchpad, ABS_MT_POSITION_Y, y);
+				input_report_abs(touchpad, ABS_MT_POSITION_X, x);
+				input_report_abs(touchpad, ABS_MT_POSITION_Y, y);
 			}
 
 			offset += 4;
 		}
-		input_mt_sync_frame(sc->touchpad);
-		input_sync(sc->touchpad);
+		input_mt_sync_frame(touchpad);
+		input_sync(touchpad);
 	}
 }
 
 static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
 {
 	int n, offset, relx, rely;
+	struct input_dev *touchpad;
 	u8 active;
 
 	/*
@@ -1271,7 +1284,13 @@ static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	 */
 	offset = 1;
 
-	input_report_key(sc->touchpad, BTN_LEFT, rd[offset] & 0x0F);
+	rcu_read_lock();
+	touchpad = rcu_dereference(sc->touchpad);
+	rcu_read_unlock();
+	if (!touchpad)
+		return;
+
+	input_report_key(touchpad, BTN_LEFT, rd[offset] & 0x0F);
 	active = (rd[offset] >> 4);
 	relx = (s8) rd[offset+5];
 	rely = ((s8) rd[offset+10]) * -1;
@@ -1285,20 +1304,20 @@ static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
 		x = rd[offset] | ((rd[offset+1] & 0x0F) << 8);
 		y = ((rd[offset+1] & 0xF0) >> 4) | (rd[offset+2] << 4);
 
-		input_mt_slot(sc->touchpad, n);
-		input_mt_report_slot_state(sc->touchpad, MT_TOOL_FINGER, active & 0x03);
+		input_mt_slot(touchpad, n);
+		input_mt_report_slot_state(touchpad, MT_TOOL_FINGER, active & 0x03);
 
 		if (active & 0x03) {
 			contactx = rd[offset+3] & 0x0F;
 			contacty = rd[offset+3] >> 4;
-			input_report_abs(sc->touchpad, ABS_MT_TOUCH_MAJOR,
+			input_report_abs(touchpad, ABS_MT_TOUCH_MAJOR,
 				max(contactx, contacty));
-			input_report_abs(sc->touchpad, ABS_MT_TOUCH_MINOR,
+			input_report_abs(touchpad, ABS_MT_TOUCH_MINOR,
 				min(contactx, contacty));
-			input_report_abs(sc->touchpad, ABS_MT_ORIENTATION,
+			input_report_abs(touchpad, ABS_MT_ORIENTATION,
 				(bool) (contactx > contacty));
-			input_report_abs(sc->touchpad, ABS_MT_POSITION_X, x);
-			input_report_abs(sc->touchpad, ABS_MT_POSITION_Y,
+			input_report_abs(touchpad, ABS_MT_POSITION_X, x);
+			input_report_abs(touchpad, ABS_MT_POSITION_Y,
 				NSG_MRXU_MAX_Y - y);
 			/*
 			 * The relative coordinates belong to the first touch
@@ -1306,8 +1325,8 @@ static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
 			 * when the first is not active.
 			 */
 			if ((n == 0) || ((n == 1) && (active & 0x01))) {
-				input_report_rel(sc->touchpad, REL_X, relx);
-				input_report_rel(sc->touchpad, REL_Y, rely);
+				input_report_rel(touchpad, REL_X, relx);
+				input_report_rel(touchpad, REL_Y, rely);
 			}
 		}
 
@@ -1315,9 +1334,9 @@ static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
 		active >>= 2;
 	}
 
-	input_mt_sync_frame(sc->touchpad);
+	input_mt_sync_frame(touchpad);
 
-	input_sync(sc->touchpad);
+	input_sync(touchpad);
 }
 
 static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
@@ -1496,19 +1515,20 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 	size_t name_sz;
 	char *name;
 	int ret;
+	struct input_dev *touchpad;
 
-	sc->touchpad = devm_input_allocate_device(&sc->hdev->dev);
-	if (!sc->touchpad)
+	touchpad = devm_input_allocate_device(&sc->hdev->dev);
+	if (!touchpad)
 		return -ENOMEM;
 
-	input_set_drvdata(sc->touchpad, sc);
-	sc->touchpad->dev.parent = &sc->hdev->dev;
-	sc->touchpad->phys = sc->hdev->phys;
-	sc->touchpad->uniq = sc->hdev->uniq;
-	sc->touchpad->id.bustype = sc->hdev->bus;
-	sc->touchpad->id.vendor = sc->hdev->vendor;
-	sc->touchpad->id.product = sc->hdev->product;
-	sc->touchpad->id.version = sc->hdev->version;
+	input_set_drvdata(touchpad, sc);
+	touchpad->dev.parent = &sc->hdev->dev;
+	touchpad->phys = sc->hdev->phys;
+	touchpad->uniq = sc->hdev->uniq;
+	touchpad->id.bustype = sc->hdev->bus;
+	touchpad->id.vendor = sc->hdev->vendor;
+	touchpad->id.product = sc->hdev->product;
+	touchpad->id.version = sc->hdev->version;
 
 	/* Append a suffix to the controller name as there are various
 	 * DS4 compatible non-Sony devices with different names.
@@ -1518,39 +1538,41 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 	if (!name)
 		return -ENOMEM;
 	snprintf(name, name_sz, "%s" DS4_TOUCHPAD_SUFFIX, sc->hdev->name);
-	sc->touchpad->name = name;
+	touchpad->name = name;
 
 	/* We map the button underneath the touchpad to BTN_LEFT. */
-	__set_bit(EV_KEY, sc->touchpad->evbit);
-	__set_bit(BTN_LEFT, sc->touchpad->keybit);
-	__set_bit(INPUT_PROP_BUTTONPAD, sc->touchpad->propbit);
+	__set_bit(EV_KEY, touchpad->evbit);
+	__set_bit(BTN_LEFT, touchpad->keybit);
+	__set_bit(INPUT_PROP_BUTTONPAD, touchpad->propbit);
 
-	input_set_abs_params(sc->touchpad, ABS_MT_POSITION_X, 0, w, 0, 0);
-	input_set_abs_params(sc->touchpad, ABS_MT_POSITION_Y, 0, h, 0, 0);
+	input_set_abs_params(touchpad, ABS_MT_POSITION_X, 0, w, 0, 0);
+	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, h, 0, 0);
 
 	if (touch_major > 0) {
-		input_set_abs_params(sc->touchpad, ABS_MT_TOUCH_MAJOR, 
+		input_set_abs_params(touchpad, ABS_MT_TOUCH_MAJOR, 
 			0, touch_major, 0, 0);
 		if (touch_minor > 0)
-			input_set_abs_params(sc->touchpad, ABS_MT_TOUCH_MINOR, 
+			input_set_abs_params(touchpad, ABS_MT_TOUCH_MINOR, 
 				0, touch_minor, 0, 0);
 		if (orientation > 0)
-			input_set_abs_params(sc->touchpad, ABS_MT_ORIENTATION, 
+			input_set_abs_params(touchpad, ABS_MT_ORIENTATION, 
 				0, orientation, 0, 0);
 	}
 
 	if (sc->quirks & NSG_MRXU_REMOTE) {
-		__set_bit(EV_REL, sc->touchpad->evbit);
+		__set_bit(EV_REL, touchpad->evbit);
 	}
 
-	ret = input_mt_init_slots(sc->touchpad, touch_count, INPUT_MT_POINTER);
+	ret = input_mt_init_slots(touchpad, touch_count, INPUT_MT_POINTER);
 	if (ret < 0)
 		return ret;
 
-	ret = input_register_device(sc->touchpad);
+	ret = input_register_device(touchpad);
 	if (ret < 0)
 		return ret;
 
+	rcu_assign_pointer(sc->touchpad, touchpad);
+
 	return 0;
 }
 
@@ -1627,6 +1649,48 @@ static int sony_register_sensors(struct sony_sc *sc)
 	return 0;
 }
 
+static void sony_unregister_touchpad(struct sony_sc *sc)
+{
+	struct input_dev *touchpad;
+
+	rcu_read_lock();
+	touchpad = rcu_dereference(sc->touchpad);
+	rcu_read_unlock();
+
+	if (!touchpad)
+		return;
+
+	RCU_INIT_POINTER(sc->touchpad, NULL);
+	synchronize_rcu();
+	input_unregister_device(touchpad);
+}
+
+static int sony_register_ds4_touchpad(struct sony_sc *sc)
+{
+	struct input_dev *touchpad;
+	int ret;
+
+	rcu_read_lock();
+	touchpad = rcu_dereference(sc->touchpad);
+	rcu_read_unlock();
+
+	if (touchpad)
+		return 0;
+
+	/*
+	 * The Dualshock 4 touchpad supports 2 touches and has a
+	 * resolution of 1920x942 (44.86 dots/mm).
+	 */
+	ret = sony_register_touchpad(sc, 2, 1920, 942, 0, 0, 0);
+	if (ret) {
+		hid_err(sc->hdev,
+		"Unable to initialize multi-touch slots: %d\n",
+		ret);
+	}
+
+	return ret;
+}
+
 /*
  * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
  * to "operational".  Without this, the ps3 controller will not report any
@@ -2876,17 +2940,9 @@ static int sony_input_configured(struct hid_device *hdev,
 		}
 		sc->hw_version_created = true;
 
-		/*
-		 * The Dualshock 4 touchpad supports 2 touches and has a
-		 * resolution of 1920x942 (44.86 dots/mm).
-		 */
-		ret = sony_register_touchpad(sc, 2, 1920, 942, 0, 0, 0);
-		if (ret) {
-			hid_err(sc->hdev,
-			"Unable to initialize multi-touch slots: %d\n",
-			ret);
+		ret = sony_register_ds4_touchpad(sc);
+		if (ret)
 			goto err_stop;
-		}
 
 		ret = sony_register_sensors(sc);
 		if (ret) {
@@ -2996,6 +3052,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	spin_lock_init(&sc->lock);
+	mutex_init(&sc->mutex);
 
 	sc->quirks = quirks;
 	hid_set_drvdata(hdev, sc);
-- 
2.36.0

[PATCH 5/6] HID: hid-sony: Add touchpad_mouse param

[Vicki Pfau]

Add parameter "touchpad_mouse" to enable disabling or re-enabling exposing the
touchpad input_dev, which can be changed while the module is loaded.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-sony.c | 49 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 1c347b3ca992..c4ccad95ee9a 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -98,6 +98,8 @@ static const char ghl_ps3wiiu_magic_data[] = {
 	0x02, 0x08, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00
 };
 
+static bool touchpad_mouse = true;
+
 /* PS/3 Motion controller */
 static u8 motion_rdesc[] = {
 	0x05, 0x01,         /*  Usage Page (Desktop),               */
@@ -525,7 +527,7 @@ struct motion_output_report_02 {
 #define SIXAXIS_INPUT_REPORT_ACC_X_OFFSET 41
 #define SIXAXIS_ACC_RES_PER_G 113
 
-static DEFINE_SPINLOCK(sony_dev_list_lock);
+static DEFINE_MUTEX(sony_dev_list_lock);
 static LIST_HEAD(sony_device_list);
 static DEFINE_IDA(sony_device_id_allocator);
 
@@ -1670,6 +1672,9 @@ static int sony_register_ds4_touchpad(struct sony_sc *sc)
 	struct input_dev *touchpad;
 	int ret;
 
+	if (!touchpad_mouse)
+		return 0;
+
 	rcu_read_lock();
 	touchpad = rcu_dereference(sc->touchpad);
 	rcu_read_unlock();
@@ -2599,10 +2604,9 @@ static inline int sony_compare_connection_type(struct sony_sc *sc0,
 static int sony_check_add_dev_list(struct sony_sc *sc)
 {
 	struct sony_sc *entry;
-	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&sony_dev_list_lock, flags);
+	mutex_lock(&sony_dev_list_lock);
 
 	list_for_each_entry(entry, &sony_device_list, list_node) {
 		ret = memcmp(sc->mac_address, entry->mac_address,
@@ -2624,18 +2628,16 @@ static int sony_check_add_dev_list(struct sony_sc *sc)
 	list_add(&(sc->list_node), &sony_device_list);
 
 unlock:
-	spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+	mutex_unlock(&sony_dev_list_lock);
 	return ret;
 }
 
 static void sony_remove_dev_list(struct sony_sc *sc)
 {
-	unsigned long flags;
-
 	if (sc->list_node.next) {
-		spin_lock_irqsave(&sony_dev_list_lock, flags);
+		mutex_lock(&sony_dev_list_lock);
 		list_del(&(sc->list_node));
-		spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+		mutex_unlock(&sony_dev_list_lock);
 	}
 }
 
@@ -3171,6 +3173,37 @@ static int sony_resume(struct hid_device *hdev)
 
 #endif
 
+static int sony_param_set_touchpad_mouse(const char *val,
+					const struct kernel_param *kp)
+{
+	struct sony_sc *sc;
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (ret)
+		return ret;
+
+	mutex_lock(&sony_dev_list_lock);
+	list_for_each_entry(sc, &sony_device_list, list_node) {
+		mutex_lock(&sc->mutex);
+		if (touchpad_mouse)
+			sony_register_ds4_touchpad(sc);
+		else
+			sony_unregister_touchpad(sc);
+		mutex_unlock(&sc->mutex);
+	}
+	mutex_unlock(&sony_dev_list_lock);
+	return 0;
+}
+
+static const struct kernel_param_ops sony_touchpad_mouse_ops = {
+	.set	= sony_param_set_touchpad_mouse,
+	.get	= param_get_bool,
+};
+
+module_param_cb(touchpad_mouse, &sony_touchpad_mouse_ops, &touchpad_mouse, 0644);
+MODULE_PARM_DESC(touchpad_mouse, "Enable mouse emulation using the touchpad");
+
 static const struct hid_device_id sony_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
 		.driver_data = SIXAXIS_CONTROLLER_USB },
-- 
2.36.0

[PATCH 6/6] HID: hid-sony: Disable touchpad reporting when hidraw open

[Vicki Pfau]

When using the hidraw node directly, disable the touchpad endpoint to prevent
it from sending separate mouse-like reports. This is accomplished in the same
way that the hid-steam driver does it, by creating and attaching an input_dev
with a custom low-level transport driver, which monitors and reports when the
hidraw node is opened or closed. Reports sent by the real device are reported
to the "fake" device, and the real device is prevented from creating a hidraw
node. This "fake" device is connected with only a hidraw node, and is exposed
with identifying information that is identical to the original device, so the
"fake" device's hidraw node appears as the node associated with the dev.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-sony.c | 167 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 155 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index c4ccad95ee9a..5b6f1e5ae8db 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -457,6 +457,8 @@ static enum power_supply_property sony_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 };
 
+static struct hid_ll_driver sony_client_ll_driver;
+
 struct sixaxis_led {
 	u8 time_enabled; /* the total time the led is active (0xff means forever) */
 	u8 duty_length;  /* how long a cycle is in deciseconds (0 means "really fast") */
@@ -558,7 +560,7 @@ enum sony_worker {
 struct sony_sc {
 	spinlock_t lock;
 	struct list_head list_node;
-	struct hid_device *hdev;
+	struct hid_device *hdev, *client_hdev;
 	struct input_dev __rcu *touchpad;
 	struct input_dev *sensor_dev;
 	struct led_classdev *leds[MAX_LEDS];
@@ -569,6 +571,7 @@ struct sony_sc {
 	struct power_supply *battery;
 	struct power_supply_desc battery_desc;
 	struct mutex mutex;
+	bool client_opened;
 	int device_id;
 	unsigned fw_version;
 	bool fw_version_created;
@@ -947,7 +950,7 @@ static u8 *sony_report_fixup(struct hid_device *hdev, u8 *rdesc,
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
 
-	if (sc->quirks & (SINO_LITE_CONTROLLER | FUTUREMAX_DANCE_MAT))
+	if (!sc || (sc->quirks & (SINO_LITE_CONTROLLER | FUTUREMAX_DANCE_MAT)))
 		return rdesc;
 
 	/*
@@ -1345,6 +1348,22 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *rd, int size)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
+	int ret;
+
+	/*
+	 * Check if we're the client hdev, which is only used for a separate
+	 * hidraw device. If so, there's nothing to be done here.
+	 */
+	if (hdev->ll_driver == &sony_client_ll_driver)
+		return 0;
+
+	if (sc->client_opened) {
+		ret = hid_input_report(sc->client_hdev, HID_INPUT_REPORT, rd, size, 0);
+		if (ret) {
+			hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
+			return ret;
+		}
+	}
 
 	/*
 	 * Sixaxis HID report has acclerometers/gyro with MSByte first, this
@@ -3034,6 +3053,92 @@ static int sony_input_configured(struct hid_device *hdev,
 	return ret;
 }
 
+static int sony_client_ll_parse(struct hid_device *hdev)
+{
+	struct sony_sc *sc = hdev->driver_data;
+
+	return hid_parse_report(hdev, sc->hdev->dev_rdesc,
+			sc->hdev->dev_rsize);
+}
+
+static int sony_client_ll_start(struct hid_device *hdev)
+{
+	return 0;
+}
+
+static void sony_client_ll_stop(struct hid_device *hdev)
+{
+}
+
+static int sony_client_ll_open(struct hid_device *hdev)
+{
+	struct sony_sc *sc = hdev->driver_data;
+
+	mutex_lock(&sc->mutex);
+	sc->client_opened = true;
+	mutex_unlock(&sc->mutex);
+
+	if (sc->quirks & DUALSHOCK4_CONTROLLER)
+		sony_unregister_touchpad(sc);
+
+	return 0;
+}
+
+static void sony_client_ll_close(struct hid_device *hdev)
+{
+	struct sony_sc *sc = hdev->driver_data;
+
+	mutex_lock(&sc->mutex);
+	sc->client_opened = false;
+	mutex_unlock(&sc->mutex);
+
+	if (sc->quirks & DUALSHOCK4_CONTROLLER)
+		sony_register_ds4_touchpad(sc);
+}
+
+static int sony_client_ll_raw_request(struct hid_device *hdev,
+				unsigned char reportnum, u8 *buf,
+				size_t count, unsigned char report_type,
+				int reqtype)
+{
+	struct sony_sc *sc = hdev->driver_data;
+
+	return hid_hw_raw_request(sc->hdev, reportnum, buf, count,
+			report_type, reqtype);
+}
+
+static struct hid_ll_driver sony_client_ll_driver = {
+	.parse = sony_client_ll_parse,
+	.start = sony_client_ll_start,
+	.stop = sony_client_ll_stop,
+	.open = sony_client_ll_open,
+	.close = sony_client_ll_close,
+	.raw_request = sony_client_ll_raw_request,
+};
+
+static struct hid_device *sony_create_client_hid(struct hid_device *hdev)
+{
+	struct hid_device *client_hdev;
+
+	client_hdev = hid_allocate_device();
+	if (IS_ERR(client_hdev))
+		return client_hdev;
+
+	client_hdev->ll_driver = &sony_client_ll_driver;
+	client_hdev->dev.parent = hdev->dev.parent;
+	client_hdev->bus = hdev->bus;
+	client_hdev->vendor = hdev->vendor;
+	client_hdev->product = hdev->product;
+	client_hdev->version = hdev->version;
+	client_hdev->type = hdev->type;
+	client_hdev->country = hdev->country;
+	strlcpy(client_hdev->name, hdev->name,
+			sizeof(client_hdev->name));
+	strlcpy(client_hdev->phys, hdev->phys,
+			sizeof(client_hdev->phys));
+	return client_hdev;
+}
+
 static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	int ret;
@@ -3041,6 +3146,19 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct sony_sc *sc;
 	unsigned int connect_mask = HID_CONNECT_DEFAULT;
 
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "parse failed\n");
+		return ret;
+	}
+
+	/*
+	 * The virtual client_dev is only used for hidraw.
+	 * Also avoid the recursive probe.
+	 */
+	if (hdev->ll_driver == &sony_client_ll_driver)
+		return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+
 	if (!strcmp(hdev->name, "FutureMax Dance Mat"))
 		quirks |= FUTUREMAX_DANCE_MAT;
 
@@ -3060,12 +3178,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hid_set_drvdata(hdev, sc);
 	sc->hdev = hdev;
 
-	ret = hid_parse(hdev);
-	if (ret) {
-		hid_err(hdev, "parse failed\n");
-		return ret;
-	}
-
 	if (sc->quirks & VAIO_RDESC_CONSTANT)
 		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
 	else if (sc->quirks & SIXAXIS_CONTROLLER)
@@ -3080,12 +3192,32 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (sc->quirks & (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER))
 		hdev->version |= 0x8000;
 
+	/* For DualShock 4 controllers, we create a client_hid device so that
+	 * we can tell when it's been opened directly and disable the touchpad
+	 * from being used as a mouse at the same time.
+	 */
+	if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+		connect_mask &= ~HID_CONNECT_HIDRAW;
+		sc->client_hdev = sony_create_client_hid(hdev);
+		if (IS_ERR(sc->client_hdev))
+			return PTR_ERR(sc->client_hdev);
+		sc->client_hdev->driver_data = sc;
+	}
+
 	ret = hid_hw_start(hdev, connect_mask);
 	if (ret) {
 		hid_err(hdev, "hw start failed\n");
 		return ret;
 	}
 
+	if (sc->client_hdev)
+		ret = hid_add_device(sc->client_hdev);
+	if (ret) {
+		hid_err(hdev, "client hw start failed\n");
+		hid_hw_stop(hdev);
+		return ret;
+	}
+
 	/* sony_input_configured can fail, but this doesn't result
 	 * in hid_hw_start failures (intended). Check whether
 	 * the HID layer claimed the device else fail.
@@ -3096,6 +3228,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 */
 	if (!(hdev->claimed & HID_CLAIMED_INPUT)) {
 		hid_err(hdev, "failed to claim input\n");
+		if (sc->client_hdev)
+			hid_destroy_device(sc->client_hdev);
 		hid_hw_stop(hdev);
 		return -ENODEV;
 	}
@@ -3113,6 +3247,13 @@ static void sony_remove(struct hid_device *hdev)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
 
+	if (!sc || hdev->ll_driver == &sony_client_ll_driver) {
+		hid_hw_stop(hdev);
+		return;
+	}
+	if (sc->client_hdev)
+		hid_destroy_device(sc->client_hdev);
+
 	if (sc->quirks & GHL_GUITAR_PS3WIIU)
 		del_timer_sync(&sc->ghl_poke_timer);
 
@@ -3186,10 +3327,12 @@ static int sony_param_set_touchpad_mouse(const char *val,
 	mutex_lock(&sony_dev_list_lock);
 	list_for_each_entry(sc, &sony_device_list, list_node) {
 		mutex_lock(&sc->mutex);
-		if (touchpad_mouse)
-			sony_register_ds4_touchpad(sc);
-		else
-			sony_unregister_touchpad(sc);
+		if (!sc->client_opened) {
+			if (touchpad_mouse)
+				sony_register_ds4_touchpad(sc);
+			else
+				sony_unregister_touchpad(sc);
+		}
 		mutex_unlock(&sc->mutex);
 	}
 	mutex_unlock(&sony_dev_list_lock);
-- 
2.36.0

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Benjamin Tissoires]

Hi Vicki,

On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>
> This allows the touchpad input_dev to be removed and have the driver remain
> functional without its presence. This will be used to allow the touchpad to
> be disabled, e.g. by a module parameter.

Thanks for the contribution.
I'd like to hear from Roderick, but I have a general comment here:
We had Wii and Steam controllers following this logic. Now we are
adding Sony PS ones... That seems like a lot of duplication, and I
wonder if we should not have something more generic.

We are working on an ioctl to revoke hidraw nodes and I wonder if we
should not take this opportunity to have a more generic mechanism in
HID to also disable input events when the hidraw node is opened...

One comment on the code itself below.

>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/hid/hid-playstation.c | 60 +++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ab7c82c2e886..f859a8dd8a2e 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -29,6 +29,7 @@ static DEFINE_IDA(ps_player_id_allocator);
>  struct ps_device {
>         struct list_head list;
>         struct hid_device *hdev;
> +       struct mutex mutex;
>         spinlock_t lock;
>
>         uint32_t player_id;
> @@ -130,7 +131,7 @@ struct dualsense {
>         struct ps_device base;
>         struct input_dev *gamepad;
>         struct input_dev *sensors;
> -       struct input_dev *touchpad;
> +       struct input_dev __rcu *touchpad;
>
>         /* Calibration data for accelerometer and gyroscope. */
>         struct ps_calibration_data accel_calib_data[3];
> @@ -590,6 +591,22 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
>         return touchpad;
>  }
>
> +static void dualsense_unregister_touchpad(struct dualsense *ds)
> +{
> +       struct input_dev *touchpad;
> +
> +       rcu_read_lock();
> +       touchpad = rcu_dereference(ds->touchpad);
> +       rcu_read_unlock();
> +
> +       if (!touchpad)
> +               return;
> +
> +       RCU_INIT_POINTER(ds->touchpad, NULL);
> +       synchronize_rcu();
> +       input_unregister_device(touchpad);
> +}
> +
>  static ssize_t firmware_version_show(struct device *dev,
>                                 struct device_attribute
>                                 *attr, char *buf)
> @@ -888,6 +905,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>         struct hid_device *hdev = ps_dev->hdev;
>         struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
>         struct dualsense_input_report *ds_report;
> +       struct input_dev *touchpad = NULL;
>         uint8_t battery_data, battery_capacity, charging_status, value;
>         int battery_status;
>         uint32_t sensor_timestamp;
> @@ -1002,24 +1020,29 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>         input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
>         input_sync(ds->sensors);
>
> -       for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
> -               struct dualsense_touch_point *point = &ds_report->points[i];
> -               bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
> +       rcu_read_lock();
> +       touchpad = rcu_dereference(ds->touchpad);
> +       rcu_read_unlock();
> +       if (touchpad) {

Access to touchpad should probably be guarded under RCU (that's what I
understand when I read
https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#rcu-dereference)

> +               for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
> +                       struct dualsense_touch_point *point = &ds_report->points[i];
> +                       bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
>
> -               input_mt_slot(ds->touchpad, i);
> -               input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
> +                       input_mt_slot(ds->touchpad, i);
> +                       input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
>
> -               if (active) {
> -                       int x = (point->x_hi << 8) | point->x_lo;
> -                       int y = (point->y_hi << 4) | point->y_lo;
> +                       if (active) {
> +                               int x = (point->x_hi << 8) | point->x_lo;
> +                               int y = (point->y_hi << 4) | point->y_lo;
>
> -                       input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
> -                       input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
> +                               input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
> +                               input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
> +                       }
>                 }
> +               input_mt_sync_frame(ds->touchpad);
> +               input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> +               input_sync(ds->touchpad);
>         }
> -       input_mt_sync_frame(ds->touchpad);
> -       input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> -       input_sync(ds->touchpad);
>
>         battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
>         charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
> @@ -1141,6 +1164,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>  {
>         struct dualsense *ds;
>         struct ps_device *ps_dev;
> +       struct input_dev *touchpad;
>         uint8_t max_output_report_size;
>         int ret;
>
> @@ -1157,6 +1181,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         ps_dev = &ds->base;
>         ps_dev->hdev = hdev;
>         spin_lock_init(&ps_dev->lock);
> +       mutex_init(&ps_dev->mutex);

This mutex is never used.

Cheers,
Benjamin

>         ps_dev->battery_capacity = 100; /* initial value until parse_report. */
>         ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
>         ps_dev->parse_report = dualsense_parse_report;
> @@ -1204,11 +1229,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>                 goto err;
>         }
>
> -       ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> -       if (IS_ERR(ds->touchpad)) {
> -               ret = PTR_ERR(ds->touchpad);
> +       touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> +       if (IS_ERR(touchpad)) {
> +               ret = PTR_ERR(touchpad);
>                 goto err;
>         }
> +       rcu_assign_pointer(ds->touchpad, touchpad);
>
>         ret = ps_device_register_battery(ps_dev);
>         if (ret)
> --
> 2.36.0
>

Re: [PATCH 2/6] HID: hid-playstation: Add touchpad_mouse param

[Benjamin Tissoires]

Hi Vicki,

On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>
> Add parameter "touchpad_mouse" to enable disabling or re-enabling exposing the
> touchpad input_dev, which can be changed while the module is loaded.

What's the point of exposing this new parameter?
Patch 3/6 automatically disables touchpad when hidraw is opened, so
who will be the user of this parameter?

The problem I have with kernel parameter is that they are effectively
kernel API, and we need to keep them forever, so I'd rather have good
arguments on why this is needed.

Cheers,
Benjamin

>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/hid/hid-playstation.c | 41 +++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index f859a8dd8a2e..ad0da4470615 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -23,6 +23,8 @@ static LIST_HEAD(ps_devices_list);
>
>  static DEFINE_IDA(ps_player_id_allocator);
>
> +static bool touchpad_mouse = true;
> +
>  #define HID_PLAYSTATION_VERSION_PATCH 0x8000
>
>  /* Base class for playstation devices. */
> @@ -1343,6 +1345,45 @@ static void ps_remove(struct hid_device *hdev)
>         hid_hw_stop(hdev);
>  }
>
> +static int ps_param_set_touchpad_mouse(const char *val,
> +                                       const struct kernel_param *kp)
> +{
> +       struct ps_device *dev;
> +       struct dualsense *ds;
> +       struct input_dev *touchpad;
> +       int ret;
> +
> +       ret = param_set_bool(val, kp);
> +       if (ret)
> +               return ret;
> +
> +       mutex_lock(&ps_devices_lock);
> +       list_for_each_entry(dev, &ps_devices_list, list) {
> +               mutex_lock(&dev->mutex);
> +               if (dev->hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> +                       ds = container_of(dev, struct dualsense, base);
> +                       if (touchpad_mouse) {
> +                               touchpad = ps_touchpad_create(dev->hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> +                               if (IS_ERR(touchpad))
> +                                       continue;
> +                               rcu_assign_pointer(ds->touchpad, touchpad);
> +                       } else
> +                               dualsense_unregister_touchpad(ds);
> +               }
> +               mutex_unlock(&dev->mutex);
> +       }
> +       mutex_unlock(&ps_devices_lock);
> +       return 0;
> +}
> +
> +static const struct kernel_param_ops ps_touchpad_mouse_ops = {
> +       .set    = ps_param_set_touchpad_mouse,
> +       .get    = param_get_bool,
> +};
> +
> +module_param_cb(touchpad_mouse, &ps_touchpad_mouse_ops, &touchpad_mouse, 0644);
> +MODULE_PARM_DESC(touchpad_mouse, "Enable mouse emulation using the touchpad");
> +
>  static const struct hid_device_id ps_devices[] = {
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
>         { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
> --
> 2.36.0
>

Re: [PATCH 3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

[Benjamin Tissoires]

On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>
> When using the hidraw node directly, disable the touchpad endpoint to prevent
> it from sending separate mouse-like reports. This is accomplished in the same
> way that the hid-steam driver does it, by creating and attaching an input_dev
> with a custom low-level transport driver, which monitors and reports when the
> hidraw node is opened or closed. Reports sent by the real device are reported
> to the "fake" device, and the real device is prevented from creating a hidraw
> node. This "fake" device is connected with only a hidraw node, and is exposed
> with identifying information that is identical to the original device, so the
> "fake" device's hidraw node appears as the node associated with the dev.

Besides the "we should have a generic way of doing this", why do we
only prevent touchpad events from being reported?

[looking at the code further down]

So it seems you are entirely disabling the input nodes, which is what
I would have expected, so the commit description needs some changes.
Or am I reading this wrong?

Cheers,
Benjamin

>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
>  1 file changed, 141 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ad0da4470615..3746c9c550d6 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
>  /* Base class for playstation devices. */
>  struct ps_device {
>         struct list_head list;
> -       struct hid_device *hdev;
> +       struct hid_device *hdev, *client_hdev;
>         struct mutex mutex;
>         spinlock_t lock;
> +       bool client_opened;
>
>         uint32_t player_id;
>
> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
>         .attrs = ps_device_attributes,
>  };
>
> +static int ps_client_ll_parse(struct hid_device *hdev)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +
> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
> +                       dev->hdev->dev_rsize);
> +}
> +
> +static int ps_client_ll_start(struct hid_device *hdev)
> +{
> +       return 0;
> +}
> +
> +static void ps_client_ll_stop(struct hid_device *hdev)
> +{
> +}
> +
> +static int ps_client_ll_open(struct hid_device *hdev)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +       struct dualsense *ds;
> +
> +       mutex_lock(&dev->mutex);
> +       dev->client_opened = true;
> +       mutex_unlock(&dev->mutex);
> +
> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> +               ds = container_of(dev, struct dualsense, base);
> +               dualsense_unregister_touchpad(ds);
> +       }
> +
> +       return 0;
> +}
> +
> +static void ps_client_ll_close(struct hid_device *hdev)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +       struct dualsense *ds;
> +       struct input_dev *touchpad;
> +
> +       mutex_lock(&dev->mutex);
> +       dev->client_opened = false;
> +       mutex_unlock(&dev->mutex);
> +
> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> +               ds = container_of(dev, struct dualsense, base);
> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> +               if (IS_ERR(touchpad))
> +                       return;
> +               rcu_assign_pointer(ds->touchpad, touchpad);
> +       }
> +}
> +
> +static int ps_client_ll_raw_request(struct hid_device *hdev,
> +                               unsigned char reportnum, u8 *buf,
> +                               size_t count, unsigned char report_type,
> +                               int reqtype)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +
> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
> +                       report_type, reqtype);
> +}
> +
> +static struct hid_ll_driver ps_client_ll_driver = {
> +       .parse = ps_client_ll_parse,
> +       .start = ps_client_ll_start,
> +       .stop = ps_client_ll_stop,
> +       .open = ps_client_ll_open,
> +       .close = ps_client_ll_close,
> +       .raw_request = ps_client_ll_raw_request,
> +};
> +
> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
> +{
> +       struct hid_device *client_hdev;
> +
> +       client_hdev = hid_allocate_device();
> +       if (IS_ERR(client_hdev))
> +               return client_hdev;
> +
> +       client_hdev->ll_driver = &ps_client_ll_driver;
> +       client_hdev->dev.parent = hdev->dev.parent;
> +       client_hdev->bus = hdev->bus;
> +       client_hdev->vendor = hdev->vendor;
> +       client_hdev->product = hdev->product;
> +       client_hdev->version = hdev->version;
> +       client_hdev->type = hdev->type;
> +       client_hdev->country = hdev->country;
> +       strlcpy(client_hdev->name, hdev->name,
> +                       sizeof(client_hdev->name));
> +       strlcpy(client_hdev->phys, hdev->phys,
> +                       sizeof(client_hdev->phys));
> +       return client_hdev;
> +}
> +
>  static int dualsense_get_calibration_data(struct dualsense *ds)
>  {
>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
>         hid_set_drvdata(hdev, ds);
>
> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
> +       if (IS_ERR(ps_dev->client_hdev))
> +               return ERR_CAST(ps_dev->client_hdev);
> +       ps_dev->client_hdev->driver_data = ps_dev;
> +
>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
>         if (!ds->output_report_dmabuf)
> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
>                 u8 *data, int size)
>  {
>         struct ps_device *dev = hid_get_drvdata(hdev);
> +       int ret = 0;
> +
> +       if (!dev)
> +               return 0;
>
> -       if (dev && dev->parse_report)
> +       if (dev->client_opened) {
> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
> +               if (ret) {
> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (dev->parse_report)
>                 return dev->parse_report(dev, report, data, size);
>
>         return 0;
> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>         struct ps_device *dev;
>         int ret;
> +       unsigned int connect_mask = 0;
>
>         ret = hid_parse(hdev);
>         if (ret) {
> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 return ret;
>         }
>
> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +       if (hdev->ll_driver == &ps_client_ll_driver)
> +               connect_mask = HID_CONNECT_HIDRAW;
> +
> +       ret = hid_hw_start(hdev, connect_mask);
>         if (ret) {
>                 hid_err(hdev, "Failed to start HID device\n");
>                 return ret;
>         }
>
> +       /*
> +        * The virtual client_dev is only used for hidraw. Since we've already
> +        * started the hw, return early to avoid the recursive probe.
> +        */
> +       if (hdev->ll_driver == &ps_client_ll_driver)
> +               return ret;
> +
>         ret = hid_hw_open(hdev);
>         if (ret) {
>                 hid_err(hdev, "Failed to open HID device\n");
> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 goto err_close;
>         }
>
> +       if (dev->client_hdev)
> +               ret = hid_add_device(dev->client_hdev);
> +       if (ret) {
> +               hid_err(hdev, "Failed to start client device failed\n");
> +               goto err_close;
> +       }
> +
>         return ret;
>
>  err_close:
> +       if (dev->client_hdev)
> +               hid_destroy_device(dev->client_hdev);
> +
>         hid_hw_close(hdev);
>  err_stop:
>         hid_hw_stop(hdev);
> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
>         ps_devices_list_remove(dev);
>         ps_device_release_player_id(dev);
>
> +       if (dev->client_hdev)
> +               hid_destroy_device(dev->client_hdev);
> +
>         hid_hw_close(hdev);
>         hid_hw_stop(hdev);
>  }
> --
> 2.36.0
>

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Dmitry Torokhov]

On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote:
> Hi Vicki,
> 
> On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> >
> > This allows the touchpad input_dev to be removed and have the driver remain
> > functional without its presence. This will be used to allow the touchpad to
> > be disabled, e.g. by a module parameter.
> 
> Thanks for the contribution.
> I'd like to hear from Roderick, but I have a general comment here:
> We had Wii and Steam controllers following this logic. Now we are
> adding Sony PS ones... That seems like a lot of duplication, and I
> wonder if we should not have something more generic.

Hmm, if userspace is not interested in data from an input device (and it
does not want to simply not open it), it can use inhibit/uninhibit sysfs
attributes to silence it.

I am not sure we need to build support for destroying input device or
introducing module parameters, etc.

Thanks.

-- 
Dmitry

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Vicki Pfau]

Hello,

On 5/5/22 01:50, Benjamin Tissoires wrote:
> Hi Vicki,
> 
> On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>>
>> This allows the touchpad input_dev to be removed and have the driver remain
>> functional without its presence. This will be used to allow the touchpad to
>> be disabled, e.g. by a module parameter.
> 
> Thanks for the contribution.
> I'd like to hear from Roderick, but I have a general comment here:
> We had Wii and Steam controllers following this logic. Now we are
> adding Sony PS ones... That seems like a lot of duplication, and I
> wonder if we should not have something more generic.
> 
> We are working on an ioctl to revoke hidraw nodes and I wonder if we
> should not take this opportunity to have a more generic mechanism in
> HID to also disable input events when the hidraw node is opened...

I would much rather that. I (attempted) to start a discussion on this a few weeks ago (https://lore.kernel.org/linux-input/b5f229c3-26e5-4fe1-aecb-504aa3c38bee@endrift.com/T/) but no one replied, so I went ahead and implemented what I assumed would be a substandard implementation, if at the very least so my sponsor would be happy, and as an attempt to start the conversation again.

> 
> One comment on the code itself below.
> 
>>
>> Signed-off-by: Vicki Pfau <vi@endrift.com>
>> ---
>>  drivers/hid/hid-playstation.c | 60 +++++++++++++++++++++++++----------
>>  1 file changed, 43 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
>> index ab7c82c2e886..f859a8dd8a2e 100644
>> --- a/drivers/hid/hid-playstation.c
>> +++ b/drivers/hid/hid-playstation.c
>> @@ -29,6 +29,7 @@ static DEFINE_IDA(ps_player_id_allocator);
>>  struct ps_device {
>>         struct list_head list;
>>         struct hid_device *hdev;
>> +       struct mutex mutex;
>>         spinlock_t lock;
>>
>>         uint32_t player_id;
>> @@ -130,7 +131,7 @@ struct dualsense {
>>         struct ps_device base;
>>         struct input_dev *gamepad;
>>         struct input_dev *sensors;
>> -       struct input_dev *touchpad;
>> +       struct input_dev __rcu *touchpad;
>>
>>         /* Calibration data for accelerometer and gyroscope. */
>>         struct ps_calibration_data accel_calib_data[3];
>> @@ -590,6 +591,22 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
>>         return touchpad;
>>  }
>>
>> +static void dualsense_unregister_touchpad(struct dualsense *ds)
>> +{
>> +       struct input_dev *touchpad;
>> +
>> +       rcu_read_lock();
>> +       touchpad = rcu_dereference(ds->touchpad);
>> +       rcu_read_unlock();
>> +
>> +       if (!touchpad)
>> +               return;
>> +
>> +       RCU_INIT_POINTER(ds->touchpad, NULL);
>> +       synchronize_rcu();
>> +       input_unregister_device(touchpad);
>> +}
>> +
>>  static ssize_t firmware_version_show(struct device *dev,
>>                                 struct device_attribute
>>                                 *attr, char *buf)
>> @@ -888,6 +905,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>>         struct hid_device *hdev = ps_dev->hdev;
>>         struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
>>         struct dualsense_input_report *ds_report;
>> +       struct input_dev *touchpad = NULL;
>>         uint8_t battery_data, battery_capacity, charging_status, value;
>>         int battery_status;
>>         uint32_t sensor_timestamp;
>> @@ -1002,24 +1020,29 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>>         input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
>>         input_sync(ds->sensors);
>>
>> -       for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
>> -               struct dualsense_touch_point *point = &ds_report->points[i];
>> -               bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
>> +       rcu_read_lock();
>> +       touchpad = rcu_dereference(ds->touchpad);
>> +       rcu_read_unlock();
>> +       if (touchpad) {
> 
> Access to touchpad should probably be guarded under RCU (that's what I
> understand when I read
> https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#rcu-dereference)

Ah, I'm new to this so I overlooked that. I'll fix this in a v2, if it's even worth having a v2 given that, as I said, I would really rather not have implemented it this way in the first place.

> 
>> +               for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
>> +                       struct dualsense_touch_point *point = &ds_report->points[i];
>> +                       bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
>>
>> -               input_mt_slot(ds->touchpad, i);
>> -               input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
>> +                       input_mt_slot(ds->touchpad, i);
>> +                       input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
>>
>> -               if (active) {
>> -                       int x = (point->x_hi << 8) | point->x_lo;
>> -                       int y = (point->y_hi << 4) | point->y_lo;
>> +                       if (active) {
>> +                               int x = (point->x_hi << 8) | point->x_lo;
>> +                               int y = (point->y_hi << 4) | point->y_lo;
>>
>> -                       input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
>> -                       input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
>> +                               input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
>> +                               input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
>> +                       }
>>                 }
>> +               input_mt_sync_frame(ds->touchpad);
>> +               input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
>> +               input_sync(ds->touchpad);
>>         }
>> -       input_mt_sync_frame(ds->touchpad);
>> -       input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
>> -       input_sync(ds->touchpad);
>>
>>         battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
>>         charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
>> @@ -1141,6 +1164,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>>  {
>>         struct dualsense *ds;
>>         struct ps_device *ps_dev;
>> +       struct input_dev *touchpad;
>>         uint8_t max_output_report_size;
>>         int ret;
>>
>> @@ -1157,6 +1181,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>>         ps_dev = &ds->base;
>>         ps_dev->hdev = hdev;
>>         spin_lock_init(&ps_dev->lock);
>> +       mutex_init(&ps_dev->mutex);
> 
> This mutex is never used.

The mutex is used in both patch 2 and 3, so I put it here in case either 2 or 3 is rejected, but not the other.

> 
> Cheers,
> Benjamin
> 
>>         ps_dev->battery_capacity = 100; /* initial value until parse_report. */
>>         ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
>>         ps_dev->parse_report = dualsense_parse_report;
>> @@ -1204,11 +1229,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>>                 goto err;
>>         }
>>
>> -       ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
>> -       if (IS_ERR(ds->touchpad)) {
>> -               ret = PTR_ERR(ds->touchpad);
>> +       touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
>> +       if (IS_ERR(touchpad)) {
>> +               ret = PTR_ERR(touchpad);
>>                 goto err;
>>         }
>> +       rcu_assign_pointer(ds->touchpad, touchpad);
>>
>>         ret = ps_device_register_battery(ps_dev);
>>         if (ret)
>> --
>> 2.36.0
>>
> 

Thanks,
Vicki

Re: [PATCH 2/6] HID: hid-playstation: Add touchpad_mouse param

[Vicki Pfau]

Hello,

On 5/5/22 01:52, Benjamin Tissoires wrote:
> Hi Vicki,
> 
> On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>>
>> Add parameter "touchpad_mouse" to enable disabling or re-enabling exposing the
>> touchpad input_dev, which can be changed while the module is loaded.
> 
> What's the point of exposing this new parameter?
> Patch 3/6 automatically disables touchpad when hidraw is opened, so
> who will be the user of this parameter?

Generally speaking, the touchpad shows up as a mouse, even though on other OSes it is only presented via gamepad APIs. This is, well, very frustrating for a lot of users, and having to tell libinput or evdev or xinput or whatever to ignore it every time you plug it in (if you're not using the hidraw directly) is...suboptimal. This was an attempt at a way to just generically say "don't do that".

> 
> The problem I have with kernel parameter is that they are effectively
> kernel API, and we need to keep them forever, so I'd rather have good
> arguments on why this is needed.

Ah, this is probably not a good enough argument to justify that then. Some other way will probably be preferred.

> 
> Cheers,
> Benjamin
> 
>>
>> Signed-off-by: Vicki Pfau <vi@endrift.com>
>> ---
>>  drivers/hid/hid-playstation.c | 41 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>
>> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
>> index f859a8dd8a2e..ad0da4470615 100644
>> --- a/drivers/hid/hid-playstation.c
>> +++ b/drivers/hid/hid-playstation.c
>> @@ -23,6 +23,8 @@ static LIST_HEAD(ps_devices_list);
>>
>>  static DEFINE_IDA(ps_player_id_allocator);
>>
>> +static bool touchpad_mouse = true;
>> +
>>  #define HID_PLAYSTATION_VERSION_PATCH 0x8000
>>
>>  /* Base class for playstation devices. */
>> @@ -1343,6 +1345,45 @@ static void ps_remove(struct hid_device *hdev)
>>         hid_hw_stop(hdev);
>>  }
>>
>> +static int ps_param_set_touchpad_mouse(const char *val,
>> +                                       const struct kernel_param *kp)
>> +{
>> +       struct ps_device *dev;
>> +       struct dualsense *ds;
>> +       struct input_dev *touchpad;
>> +       int ret;
>> +
>> +       ret = param_set_bool(val, kp);
>> +       if (ret)
>> +               return ret;
>> +
>> +       mutex_lock(&ps_devices_lock);
>> +       list_for_each_entry(dev, &ps_devices_list, list) {
>> +               mutex_lock(&dev->mutex);
>> +               if (dev->hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
>> +                       ds = container_of(dev, struct dualsense, base);
>> +                       if (touchpad_mouse) {
>> +                               touchpad = ps_touchpad_create(dev->hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
>> +                               if (IS_ERR(touchpad))
>> +                                       continue;
>> +                               rcu_assign_pointer(ds->touchpad, touchpad);
>> +                       } else
>> +                               dualsense_unregister_touchpad(ds);
>> +               }
>> +               mutex_unlock(&dev->mutex);
>> +       }
>> +       mutex_unlock(&ps_devices_lock);
>> +       return 0;
>> +}
>> +
>> +static const struct kernel_param_ops ps_touchpad_mouse_ops = {
>> +       .set    = ps_param_set_touchpad_mouse,
>> +       .get    = param_get_bool,
>> +};
>> +
>> +module_param_cb(touchpad_mouse, &ps_touchpad_mouse_ops, &touchpad_mouse, 0644);
>> +MODULE_PARM_DESC(touchpad_mouse, "Enable mouse emulation using the touchpad");
>> +
>>  static const struct hid_device_id ps_devices[] = {
>>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
>>         { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
>> --
>> 2.36.0
>>
> 

Vicki

Re: [PATCH 3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

[Vicki Pfau]

Hello,

On 5/5/22 01:57, Benjamin Tissoires wrote:
> On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>>
>> When using the hidraw node directly, disable the touchpad endpoint to prevent
>> it from sending separate mouse-like reports. This is accomplished in the same
>> way that the hid-steam driver does it, by creating and attaching an input_dev
>> with a custom low-level transport driver, which monitors and reports when the
>> hidraw node is opened or closed. Reports sent by the real device are reported
>> to the "fake" device, and the real device is prevented from creating a hidraw
>> node. This "fake" device is connected with only a hidraw node, and is exposed
>> with identifying information that is identical to the original device, so the
>> "fake" device's hidraw node appears as the node associated with the dev.
> 
> Besides the "we should have a generic way of doing this", why do we
> only prevent touchpad events from being reported?

As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.

> 
> [looking at the code further down]
> 
> So it seems you are entirely disabling the input nodes, which is what
> I would have expected, so the commit description needs some changes.
> Or am I reading this wrong?

I am unsure where this discrepancy lies in the description. Can you be more specific?

Thanks,
Vicki

> 
> Cheers,
> Benjamin

>>
>> Signed-off-by: Vicki Pfau <vi@endrift.com>
>> ---
>>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 141 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
>> index ad0da4470615..3746c9c550d6 100644
>> --- a/drivers/hid/hid-playstation.c
>> +++ b/drivers/hid/hid-playstation.c
>> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
>>  /* Base class for playstation devices. */
>>  struct ps_device {
>>         struct list_head list;
>> -       struct hid_device *hdev;
>> +       struct hid_device *hdev, *client_hdev;
>>         struct mutex mutex;
>>         spinlock_t lock;
>> +       bool client_opened;
>>
>>         uint32_t player_id;
>>
>> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
>>         .attrs = ps_device_attributes,
>>  };
>>
>> +static int ps_client_ll_parse(struct hid_device *hdev)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +
>> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
>> +                       dev->hdev->dev_rsize);
>> +}
>> +
>> +static int ps_client_ll_start(struct hid_device *hdev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static void ps_client_ll_stop(struct hid_device *hdev)
>> +{
>> +}
>> +
>> +static int ps_client_ll_open(struct hid_device *hdev)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +       struct dualsense *ds;
>> +
>> +       mutex_lock(&dev->mutex);
>> +       dev->client_opened = true;
>> +       mutex_unlock(&dev->mutex);
>> +
>> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
>> +               ds = container_of(dev, struct dualsense, base);
>> +               dualsense_unregister_touchpad(ds);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void ps_client_ll_close(struct hid_device *hdev)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +       struct dualsense *ds;
>> +       struct input_dev *touchpad;
>> +
>> +       mutex_lock(&dev->mutex);
>> +       dev->client_opened = false;
>> +       mutex_unlock(&dev->mutex);
>> +
>> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
>> +               ds = container_of(dev, struct dualsense, base);
>> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
>> +               if (IS_ERR(touchpad))
>> +                       return;
>> +               rcu_assign_pointer(ds->touchpad, touchpad);
>> +       }
>> +}
>> +
>> +static int ps_client_ll_raw_request(struct hid_device *hdev,
>> +                               unsigned char reportnum, u8 *buf,
>> +                               size_t count, unsigned char report_type,
>> +                               int reqtype)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +
>> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
>> +                       report_type, reqtype);
>> +}
>> +
>> +static struct hid_ll_driver ps_client_ll_driver = {
>> +       .parse = ps_client_ll_parse,
>> +       .start = ps_client_ll_start,
>> +       .stop = ps_client_ll_stop,
>> +       .open = ps_client_ll_open,
>> +       .close = ps_client_ll_close,
>> +       .raw_request = ps_client_ll_raw_request,
>> +};
>> +
>> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
>> +{
>> +       struct hid_device *client_hdev;
>> +
>> +       client_hdev = hid_allocate_device();
>> +       if (IS_ERR(client_hdev))
>> +               return client_hdev;
>> +
>> +       client_hdev->ll_driver = &ps_client_ll_driver;
>> +       client_hdev->dev.parent = hdev->dev.parent;
>> +       client_hdev->bus = hdev->bus;
>> +       client_hdev->vendor = hdev->vendor;
>> +       client_hdev->product = hdev->product;
>> +       client_hdev->version = hdev->version;
>> +       client_hdev->type = hdev->type;
>> +       client_hdev->country = hdev->country;
>> +       strlcpy(client_hdev->name, hdev->name,
>> +                       sizeof(client_hdev->name));
>> +       strlcpy(client_hdev->phys, hdev->phys,
>> +                       sizeof(client_hdev->phys));
>> +       return client_hdev;
>> +}
>> +
>>  static int dualsense_get_calibration_data(struct dualsense *ds)
>>  {
>>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
>> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
>>         hid_set_drvdata(hdev, ds);
>>
>> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
>> +       if (IS_ERR(ps_dev->client_hdev))
>> +               return ERR_CAST(ps_dev->client_hdev);
>> +       ps_dev->client_hdev->driver_data = ps_dev;
>> +
>>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
>>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
>>         if (!ds->output_report_dmabuf)
>> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
>>                 u8 *data, int size)
>>  {
>>         struct ps_device *dev = hid_get_drvdata(hdev);
>> +       int ret = 0;
>> +
>> +       if (!dev)
>> +               return 0;
>>
>> -       if (dev && dev->parse_report)
>> +       if (dev->client_opened) {
>> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
>> +               if (ret) {
>> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       if (dev->parse_report)
>>                 return dev->parse_report(dev, report, data, size);
>>
>>         return 0;
>> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  {
>>         struct ps_device *dev;
>>         int ret;
>> +       unsigned int connect_mask = 0;
>>
>>         ret = hid_parse(hdev);
>>         if (ret) {
>> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                 return ret;
>>         }
>>
>> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>> +       if (hdev->ll_driver == &ps_client_ll_driver)
>> +               connect_mask = HID_CONNECT_HIDRAW;
>> +
>> +       ret = hid_hw_start(hdev, connect_mask);
>>         if (ret) {
>>                 hid_err(hdev, "Failed to start HID device\n");
>>                 return ret;
>>         }
>>
>> +       /*
>> +        * The virtual client_dev is only used for hidraw. Since we've already
>> +        * started the hw, return early to avoid the recursive probe.
>> +        */
>> +       if (hdev->ll_driver == &ps_client_ll_driver)
>> +               return ret;
>> +
>>         ret = hid_hw_open(hdev);
>>         if (ret) {
>>                 hid_err(hdev, "Failed to open HID device\n");
>> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                 goto err_close;
>>         }
>>
>> +       if (dev->client_hdev)
>> +               ret = hid_add_device(dev->client_hdev);
>> +       if (ret) {
>> +               hid_err(hdev, "Failed to start client device failed\n");
>> +               goto err_close;
>> +       }
>> +
>>         return ret;
>>
>>  err_close:
>> +       if (dev->client_hdev)
>> +               hid_destroy_device(dev->client_hdev);
>> +
>>         hid_hw_close(hdev);
>>  err_stop:
>>         hid_hw_stop(hdev);
>> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
>>         ps_devices_list_remove(dev);
>>         ps_device_release_player_id(dev);
>>
>> +       if (dev->client_hdev)
>> +               hid_destroy_device(dev->client_hdev);
>> +
>>         hid_hw_close(hdev);
>>         hid_hw_stop(hdev);
>>  }
>> --
>> 2.36.0
>>
> 

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Roderick Colenbrander]

Hi Vicki,

Joining the conversation late as I on a long vacation.

On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote:
> > Hi Vicki,
> >
> > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> > >
> > > This allows the touchpad input_dev to be removed and have the driver remain
> > > functional without its presence. This will be used to allow the touchpad to
> > > be disabled, e.g. by a module parameter.
> >
> > Thanks for the contribution.
> > I'd like to hear from Roderick, but I have a general comment here:
> > We had Wii and Steam controllers following this logic. Now we are
> > adding Sony PS ones... That seems like a lot of duplication, and I
> > wonder if we should not have something more generic.
>

I understand the use case of rejecting input. I wasn't that fond of
handling it kernel side also because it would complicate the code a
lot more (and some would perhaps want to do the same for accelerometer
device). Below Dmitry gives a nice idea about inhibition.

Methods I would considered perhaps would have been a custom udev role
(it sounds like you have control of the platform). Or another method
is using EVIOCGRAB to get exclusive access to an input device to block
others from using it again depends on your specific use case. When
this topic came up some years ago with Valve it was one of the methods
considered there if I recall.

> Hmm, if userspace is not interested in data from an input device (and it
> does not want to simply not open it), it can use inhibit/uninhibit sysfs
> attributes to silence it.
>
> I am not sure we need to build support for destroying input device or
> introducing module parameters, etc.
>

Inhibition is interesting and could work. I wasn't aware of this
feature and we may actually use it for hid-playstation to save some
power as there are various HID commands for power saving when the
device isn't open. If we were to add that, some care would need to be
taken with HIDRAW, which the driver is often unaware of. I guess the
end responsibility for making sure the device would work would be with
the hidraw user (unless there will be some interop APIs).

> Thanks.
>
> --
> Dmitry


Thanks,
Roderick

Re: [PATCH 3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

[Roderick Colenbrander]

On Thu, May 5, 2022 at 3:15 PM Vicki Pfau <vi@endrift.com> wrote:
>
> Hello,
>
> On 5/5/22 01:57, Benjamin Tissoires wrote:
> > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> >>
> >> When using the hidraw node directly, disable the touchpad endpoint to prevent
> >> it from sending separate mouse-like reports. This is accomplished in the same
> >> way that the hid-steam driver does it, by creating and attaching an input_dev
> >> with a custom low-level transport driver, which monitors and reports when the
> >> hidraw node is opened or closed. Reports sent by the real device are reported
> >> to the "fake" device, and the real device is prevented from creating a hidraw
> >> node. This "fake" device is connected with only a hidraw node, and is exposed
> >> with identifying information that is identical to the original device, so the
> >> "fake" device's hidraw node appears as the node associated with the dev.
> >
> > Besides the "we should have a generic way of doing this", why do we
> > only prevent touchpad events from being reported?
>
> As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.
>

I agree with Benjamin, if we did some disabling it needs to be for everything.

I'm not convinced though that kernel side we can make the best
decision for each use case. Maybe it does make sense to hide for
example input devices when hidraw is open, which helps some use cases.
Other use cases may want to have both around.

I really like the 'inhibit' idea from Dmitry in the other email. It
would solve this use case as well can handle others.

> >
> > [looking at the code further down]
> >
> > So it seems you are entirely disabling the input nodes, which is what
> > I would have expected, so the commit description needs some changes.
> > Or am I reading this wrong?
>
> I am unsure where this discrepancy lies in the description. Can you be more specific?
>
> Thanks,
> Vicki
>
> >
> > Cheers,
> > Benjamin

Thanks,
Roderick
>
> >>
> >> Signed-off-by: Vicki Pfau <vi@endrift.com>
> >> ---
> >>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
> >>  1 file changed, 141 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> >> index ad0da4470615..3746c9c550d6 100644
> >> --- a/drivers/hid/hid-playstation.c
> >> +++ b/drivers/hid/hid-playstation.c
> >> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
> >>  /* Base class for playstation devices. */
> >>  struct ps_device {
> >>         struct list_head list;
> >> -       struct hid_device *hdev;
> >> +       struct hid_device *hdev, *client_hdev;
> >>         struct mutex mutex;
> >>         spinlock_t lock;
> >> +       bool client_opened;
> >>
> >>         uint32_t player_id;
> >>
> >> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
> >>         .attrs = ps_device_attributes,
> >>  };
> >>
> >> +static int ps_client_ll_parse(struct hid_device *hdev)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +
> >> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
> >> +                       dev->hdev->dev_rsize);
> >> +}
> >> +
> >> +static int ps_client_ll_start(struct hid_device *hdev)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >> +static void ps_client_ll_stop(struct hid_device *hdev)
> >> +{
> >> +}
> >> +
> >> +static int ps_client_ll_open(struct hid_device *hdev)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +       struct dualsense *ds;
> >> +
> >> +       mutex_lock(&dev->mutex);
> >> +       dev->client_opened = true;
> >> +       mutex_unlock(&dev->mutex);
> >> +
> >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> >> +               ds = container_of(dev, struct dualsense, base);
> >> +               dualsense_unregister_touchpad(ds);
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static void ps_client_ll_close(struct hid_device *hdev)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +       struct dualsense *ds;
> >> +       struct input_dev *touchpad;
> >> +
> >> +       mutex_lock(&dev->mutex);
> >> +       dev->client_opened = false;
> >> +       mutex_unlock(&dev->mutex);
> >> +
> >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> >> +               ds = container_of(dev, struct dualsense, base);
> >> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> >> +               if (IS_ERR(touchpad))
> >> +                       return;
> >> +               rcu_assign_pointer(ds->touchpad, touchpad);
> >> +       }
> >> +}
> >> +
> >> +static int ps_client_ll_raw_request(struct hid_device *hdev,
> >> +                               unsigned char reportnum, u8 *buf,
> >> +                               size_t count, unsigned char report_type,
> >> +                               int reqtype)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +
> >> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
> >> +                       report_type, reqtype);
> >> +}
> >> +
> >> +static struct hid_ll_driver ps_client_ll_driver = {
> >> +       .parse = ps_client_ll_parse,
> >> +       .start = ps_client_ll_start,
> >> +       .stop = ps_client_ll_stop,
> >> +       .open = ps_client_ll_open,
> >> +       .close = ps_client_ll_close,
> >> +       .raw_request = ps_client_ll_raw_request,
> >> +};
> >> +
> >> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
> >> +{
> >> +       struct hid_device *client_hdev;
> >> +
> >> +       client_hdev = hid_allocate_device();
> >> +       if (IS_ERR(client_hdev))
> >> +               return client_hdev;
> >> +
> >> +       client_hdev->ll_driver = &ps_client_ll_driver;
> >> +       client_hdev->dev.parent = hdev->dev.parent;
> >> +       client_hdev->bus = hdev->bus;
> >> +       client_hdev->vendor = hdev->vendor;
> >> +       client_hdev->product = hdev->product;
> >> +       client_hdev->version = hdev->version;
> >> +       client_hdev->type = hdev->type;
> >> +       client_hdev->country = hdev->country;
> >> +       strlcpy(client_hdev->name, hdev->name,
> >> +                       sizeof(client_hdev->name));
> >> +       strlcpy(client_hdev->phys, hdev->phys,
> >> +                       sizeof(client_hdev->phys));
> >> +       return client_hdev;
> >> +}
> >> +
> >>  static int dualsense_get_calibration_data(struct dualsense *ds)
> >>  {
> >>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> >> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> >>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
> >>         hid_set_drvdata(hdev, ds);
> >>
> >> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
> >> +       if (IS_ERR(ps_dev->client_hdev))
> >> +               return ERR_CAST(ps_dev->client_hdev);
> >> +       ps_dev->client_hdev->driver_data = ps_dev;
> >> +
> >>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
> >>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
> >>         if (!ds->output_report_dmabuf)
> >> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
> >>                 u8 *data, int size)
> >>  {
> >>         struct ps_device *dev = hid_get_drvdata(hdev);
> >> +       int ret = 0;
> >> +
> >> +       if (!dev)
> >> +               return 0;
> >>
> >> -       if (dev && dev->parse_report)
> >> +       if (dev->client_opened) {
> >> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
> >> +               if (ret) {
> >> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +
> >> +       if (dev->parse_report)
> >>                 return dev->parse_report(dev, report, data, size);
> >>
> >>         return 0;
> >> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>  {
> >>         struct ps_device *dev;
> >>         int ret;
> >> +       unsigned int connect_mask = 0;
> >>
> >>         ret = hid_parse(hdev);
> >>         if (ret) {
> >> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>                 return ret;
> >>         }
> >>
> >> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> >> +               connect_mask = HID_CONNECT_HIDRAW;
> >> +
> >> +       ret = hid_hw_start(hdev, connect_mask);
> >>         if (ret) {
> >>                 hid_err(hdev, "Failed to start HID device\n");
> >>                 return ret;
> >>         }
> >>
> >> +       /*
> >> +        * The virtual client_dev is only used for hidraw. Since we've already
> >> +        * started the hw, return early to avoid the recursive probe.
> >> +        */
> >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> >> +               return ret;
> >> +
> >>         ret = hid_hw_open(hdev);
> >>         if (ret) {
> >>                 hid_err(hdev, "Failed to open HID device\n");
> >> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>                 goto err_close;
> >>         }
> >>
> >> +       if (dev->client_hdev)
> >> +               ret = hid_add_device(dev->client_hdev);
> >> +       if (ret) {
> >> +               hid_err(hdev, "Failed to start client device failed\n");
> >> +               goto err_close;
> >> +       }
> >> +
> >>         return ret;
> >>
> >>  err_close:
> >> +       if (dev->client_hdev)
> >> +               hid_destroy_device(dev->client_hdev);
> >> +
> >>         hid_hw_close(hdev);
> >>  err_stop:
> >>         hid_hw_stop(hdev);
> >> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
> >>         ps_devices_list_remove(dev);
> >>         ps_device_release_player_id(dev);
> >>
> >> +       if (dev->client_hdev)
> >> +               hid_destroy_device(dev->client_hdev);
> >> +
> >>         hid_hw_close(hdev);
> >>         hid_hw_stop(hdev);
> >>  }
> >> --
> >> 2.36.0
> >>
> >

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Vicki Pfau]

Hello,

On 5/5/22 22:21, Roderick Colenbrander wrote:
> Hi Vicki,
> 
> Joining the conversation late as I on a long vacation.
> 
> On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote:
>>> Hi Vicki,
>>>
>>> On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>>>>
>>>> This allows the touchpad input_dev to be removed and have the driver remain
>>>> functional without its presence. This will be used to allow the touchpad to
>>>> be disabled, e.g. by a module parameter.
>>>
>>> Thanks for the contribution.
>>> I'd like to hear from Roderick, but I have a general comment here:
>>> We had Wii and Steam controllers following this logic. Now we are
>>> adding Sony PS ones... That seems like a lot of duplication, and I
>>> wonder if we should not have something more generic.
>>
> 
> I understand the use case of rejecting input. I wasn't that fond of
> handling it kernel side also because it would complicate the code a
> lot more (and some would perhaps want to do the same for accelerometer
> device). Below Dmitry gives a nice idea about inhibition.
> 
> Methods I would considered perhaps would have been a custom udev role
> (it sounds like you have control of the platform). Or another method
> is using EVIOCGRAB to get exclusive access to an input device to block
> others from using it again depends on your specific use case. When
> this topic came up some years ago with Valve it was one of the methods
> considered there if I recall.
> 
>> Hmm, if userspace is not interested in data from an input device (and it
>> does not want to simply not open it), it can use inhibit/uninhibit sysfs
>> attributes to silence it.
>>
>> I am not sure we need to build support for destroying input device or
>> introducing module parameters, etc.
>>
> 
> Inhibition is interesting and could work. I wasn't aware of this
> feature and we may actually use it for hid-playstation to save some
> power as there are various HID commands for power saving when the
> device isn't open. If we were to add that, some care would need to be
> taken with HIDRAW, which the driver is often unaware of. I guess the
> end responsibility for making sure the device would work would be with
> the hidraw user (unless there will be some interop APIs).
> 
I would like to know more about these sysfs controls. I wasn't aware of any way to do this from userspace, and paired with udev rules that allow access to these controls (can udev modify sysfs permissions?) to the right users/groups, this might be a viable way to manage this. My sponsor is Valve, so that might be why this is somewhat familiar. They want to be able to have the Steam input daemon disable various reporting from the controllers without requiring root or getting too deep into the system, but I think udev rules that allow Steam itself access to the sysfs inhibition controls may be a good approach if there's a way to do that, especially already in the kernel. I know the kernel they're shipping right now is a few versions old.

Thanks,
Vicki

>> Thanks.
>>
>> --
>> Dmitry
> 
> 
> Thanks,
> Roderick

Re: [PATCH 3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

[Benjamin Tissoires]

On Fri, May 6, 2022 at 8:00 AM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> On Thu, May 5, 2022 at 3:15 PM Vicki Pfau <vi@endrift.com> wrote:
> >
> > Hello,
> >
> > On 5/5/22 01:57, Benjamin Tissoires wrote:
> > > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> > >>
> > >> When using the hidraw node directly, disable the touchpad endpoint to prevent
> > >> it from sending separate mouse-like reports. This is accomplished in the same
> > >> way that the hid-steam driver does it, by creating and attaching an input_dev
> > >> with a custom low-level transport driver, which monitors and reports when the
> > >> hidraw node is opened or closed. Reports sent by the real device are reported
> > >> to the "fake" device, and the real device is prevented from creating a hidraw
> > >> node. This "fake" device is connected with only a hidraw node, and is exposed
> > >> with identifying information that is identical to the original device, so the
> > >> "fake" device's hidraw node appears as the node associated with the dev.
> > >
> > > Besides the "we should have a generic way of doing this", why do we
> > > only prevent touchpad events from being reported?
> >
> > As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.
> >
>
> I agree with Benjamin, if we did some disabling it needs to be for everything.
>
> I'm not convinced though that kernel side we can make the best
> decision for each use case. Maybe it does make sense to hide for
> example input devices when hidraw is open, which helps some use cases.
> Other use cases may want to have both around.

And with that said... How about eBPF to do that instead of sysfs???? :)
(I know I have a nice hammer so I see nails everywhere).

>
> I really like the 'inhibit' idea from Dmitry in the other email. It
> would solve this use case as well can handle others.

The only problem of the sysfs approach is how do we give steam/SDL the
access to this sysfs file.
Steam for example ships an ugly udev rule that gives uaccess to all
hidraw nodes on gamepads, which is worrying for a security reason.

There are a couple of systemd PR trying to solve that
(https://github.com/systemd/systemd/pull/23140 for an ioctl approach),
but we realized with Bastien and Peter that BPF was the best way
forward because we can have a central piece that makes such decisions
without changing current applications.

In this particular case, we could also use the sysfs to inhibit the
device by a daemon that would:
- monitor each hidraw_open/hidraw_close
- when a hidraw_openm comes in, it notifies userspace
- user space then calls inhibit in the sysfs

But we could also put it one step forward by adding a hook that makes
all of this logic directly in ebpf (see
https://gitlab.freedesktop.org/bentiss/logind-hidraw for such an
approach for the hidraw ioctl-like). And given that we are working
towards adding this eBPF capability to logind, we could also implement
that logic in logind directly (based on some user-defined or
session-defined rules).

Cheers,
Benjamin

>
> > >
> > > [looking at the code further down]
> > >
> > > So it seems you are entirely disabling the input nodes, which is what
> > > I would have expected, so the commit description needs some changes.
> > > Or am I reading this wrong?
> >
> > I am unsure where this discrepancy lies in the description. Can you be more specific?
> >
> > Thanks,
> > Vicki
> >
> > >
> > > Cheers,
> > > Benjamin
>
> Thanks,
> Roderick
> >
> > >>
> > >> Signed-off-by: Vicki Pfau <vi@endrift.com>
> > >> ---
> > >>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 141 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > >> index ad0da4470615..3746c9c550d6 100644
> > >> --- a/drivers/hid/hid-playstation.c
> > >> +++ b/drivers/hid/hid-playstation.c
> > >> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
> > >>  /* Base class for playstation devices. */
> > >>  struct ps_device {
> > >>         struct list_head list;
> > >> -       struct hid_device *hdev;
> > >> +       struct hid_device *hdev, *client_hdev;
> > >>         struct mutex mutex;
> > >>         spinlock_t lock;
> > >> +       bool client_opened;
> > >>
> > >>         uint32_t player_id;
> > >>
> > >> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
> > >>         .attrs = ps_device_attributes,
> > >>  };
> > >>
> > >> +static int ps_client_ll_parse(struct hid_device *hdev)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +
> > >> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
> > >> +                       dev->hdev->dev_rsize);
> > >> +}
> > >> +
> > >> +static int ps_client_ll_start(struct hid_device *hdev)
> > >> +{
> > >> +       return 0;
> > >> +}
> > >> +
> > >> +static void ps_client_ll_stop(struct hid_device *hdev)
> > >> +{
> > >> +}
> > >> +
> > >> +static int ps_client_ll_open(struct hid_device *hdev)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +       struct dualsense *ds;
> > >> +
> > >> +       mutex_lock(&dev->mutex);
> > >> +       dev->client_opened = true;
> > >> +       mutex_unlock(&dev->mutex);
> > >> +
> > >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> > >> +               ds = container_of(dev, struct dualsense, base);
> > >> +               dualsense_unregister_touchpad(ds);
> > >> +       }
> > >> +
> > >> +       return 0;
> > >> +}
> > >> +
> > >> +static void ps_client_ll_close(struct hid_device *hdev)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +       struct dualsense *ds;
> > >> +       struct input_dev *touchpad;
> > >> +
> > >> +       mutex_lock(&dev->mutex);
> > >> +       dev->client_opened = false;
> > >> +       mutex_unlock(&dev->mutex);
> > >> +
> > >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> > >> +               ds = container_of(dev, struct dualsense, base);
> > >> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> > >> +               if (IS_ERR(touchpad))
> > >> +                       return;
> > >> +               rcu_assign_pointer(ds->touchpad, touchpad);
> > >> +       }
> > >> +}
> > >> +
> > >> +static int ps_client_ll_raw_request(struct hid_device *hdev,
> > >> +                               unsigned char reportnum, u8 *buf,
> > >> +                               size_t count, unsigned char report_type,
> > >> +                               int reqtype)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +
> > >> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
> > >> +                       report_type, reqtype);
> > >> +}
> > >> +
> > >> +static struct hid_ll_driver ps_client_ll_driver = {
> > >> +       .parse = ps_client_ll_parse,
> > >> +       .start = ps_client_ll_start,
> > >> +       .stop = ps_client_ll_stop,
> > >> +       .open = ps_client_ll_open,
> > >> +       .close = ps_client_ll_close,
> > >> +       .raw_request = ps_client_ll_raw_request,
> > >> +};
> > >> +
> > >> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
> > >> +{
> > >> +       struct hid_device *client_hdev;
> > >> +
> > >> +       client_hdev = hid_allocate_device();
> > >> +       if (IS_ERR(client_hdev))
> > >> +               return client_hdev;
> > >> +
> > >> +       client_hdev->ll_driver = &ps_client_ll_driver;
> > >> +       client_hdev->dev.parent = hdev->dev.parent;
> > >> +       client_hdev->bus = hdev->bus;
> > >> +       client_hdev->vendor = hdev->vendor;
> > >> +       client_hdev->product = hdev->product;
> > >> +       client_hdev->version = hdev->version;
> > >> +       client_hdev->type = hdev->type;
> > >> +       client_hdev->country = hdev->country;
> > >> +       strlcpy(client_hdev->name, hdev->name,
> > >> +                       sizeof(client_hdev->name));
> > >> +       strlcpy(client_hdev->phys, hdev->phys,
> > >> +                       sizeof(client_hdev->phys));
> > >> +       return client_hdev;
> > >> +}
> > >> +
> > >>  static int dualsense_get_calibration_data(struct dualsense *ds)
> > >>  {
> > >>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > >> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> > >>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
> > >>         hid_set_drvdata(hdev, ds);
> > >>
> > >> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
> > >> +       if (IS_ERR(ps_dev->client_hdev))
> > >> +               return ERR_CAST(ps_dev->client_hdev);
> > >> +       ps_dev->client_hdev->driver_data = ps_dev;
> > >> +
> > >>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
> > >>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
> > >>         if (!ds->output_report_dmabuf)
> > >> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
> > >>                 u8 *data, int size)
> > >>  {
> > >>         struct ps_device *dev = hid_get_drvdata(hdev);
> > >> +       int ret = 0;
> > >> +
> > >> +       if (!dev)
> > >> +               return 0;
> > >>
> > >> -       if (dev && dev->parse_report)
> > >> +       if (dev->client_opened) {
> > >> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
> > >> +               if (ret) {
> > >> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
> > >> +                       return ret;
> > >> +               }
> > >> +       }
> > >> +
> > >> +       if (dev->parse_report)
> > >>                 return dev->parse_report(dev, report, data, size);
> > >>
> > >>         return 0;
> > >> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >>  {
> > >>         struct ps_device *dev;
> > >>         int ret;
> > >> +       unsigned int connect_mask = 0;
> > >>
> > >>         ret = hid_parse(hdev);
> > >>         if (ret) {
> > >> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >>                 return ret;
> > >>         }
> > >>
> > >> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> > >> +               connect_mask = HID_CONNECT_HIDRAW;
> > >> +
> > >> +       ret = hid_hw_start(hdev, connect_mask);
> > >>         if (ret) {
> > >>                 hid_err(hdev, "Failed to start HID device\n");
> > >>                 return ret;
> > >>         }
> > >>
> > >> +       /*
> > >> +        * The virtual client_dev is only used for hidraw. Since we've already
> > >> +        * started the hw, return early to avoid the recursive probe.
> > >> +        */
> > >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> > >> +               return ret;
> > >> +
> > >>         ret = hid_hw_open(hdev);
> > >>         if (ret) {
> > >>                 hid_err(hdev, "Failed to open HID device\n");
> > >> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >>                 goto err_close;
> > >>         }
> > >>
> > >> +       if (dev->client_hdev)
> > >> +               ret = hid_add_device(dev->client_hdev);
> > >> +       if (ret) {
> > >> +               hid_err(hdev, "Failed to start client device failed\n");
> > >> +               goto err_close;
> > >> +       }
> > >> +
> > >>         return ret;
> > >>
> > >>  err_close:
> > >> +       if (dev->client_hdev)
> > >> +               hid_destroy_device(dev->client_hdev);
> > >> +
> > >>         hid_hw_close(hdev);
> > >>  err_stop:
> > >>         hid_hw_stop(hdev);
> > >> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
> > >>         ps_devices_list_remove(dev);
> > >>         ps_device_release_player_id(dev);
> > >>
> > >> +       if (dev->client_hdev)
> > >> +               hid_destroy_device(dev->client_hdev);
> > >> +
> > >>         hid_hw_close(hdev);
> > >>         hid_hw_stop(hdev);
> > >>  }
> > >> --
> > >> 2.36.0
> > >>
> > >
>

Re: [PATCH 3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

[Benjamin Tissoires]

On Fri, May 6, 2022 at 12:04 AM Vicki Pfau <vi@endrift.com> wrote:
>
> Hello,
>
> On 5/5/22 01:57, Benjamin Tissoires wrote:
> > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> >>
> >> When using the hidraw node directly, disable the touchpad endpoint to prevent
> >> it from sending separate mouse-like reports. This is accomplished in the same
> >> way that the hid-steam driver does it, by creating and attaching an input_dev
> >> with a custom low-level transport driver, which monitors and reports when the
> >> hidraw node is opened or closed. Reports sent by the real device are reported
> >> to the "fake" device, and the real device is prevented from creating a hidraw
> >> node. This "fake" device is connected with only a hidraw node, and is exposed
> >> with identifying information that is identical to the original device, so the
> >> "fake" device's hidraw node appears as the node associated with the dev.
> >
> > Besides the "we should have a generic way of doing this", why do we
> > only prevent touchpad events from being reported?
>
> As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.
>
> >
> > [looking at the code further down]
> >
> > So it seems you are entirely disabling the input nodes, which is what
> > I would have expected, so the commit description needs some changes.
> > Or am I reading this wrong?
>
> I am unsure where this discrepancy lies in the description. Can you be more specific?
>

When I read the code, I was under the impression that when an hidraw
node was opened, it was disabling all other input nodes (joysticks,
buttons and touchpad).
But maybe this is just disabling the touchpad, which still leaves you
with duplicated events on the gamepad side.
So either we need to also disable those other events, or the code is
doing just that and the description is not correct :)

Cheersm
Benjamin

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Roderick Colenbrander]

On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote:
> > Hi Vicki,
> >
> > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> > >
> > > This allows the touchpad input_dev to be removed and have the driver remain
> > > functional without its presence. This will be used to allow the touchpad to
> > > be disabled, e.g. by a module parameter.
> >
> > Thanks for the contribution.
> > I'd like to hear from Roderick, but I have a general comment here:
> > We had Wii and Steam controllers following this logic. Now we are
> > adding Sony PS ones... That seems like a lot of duplication, and I
> > wonder if we should not have something more generic.
>

Using this to hook into the thread with Benjamin. It didn't make it to
the list (due to HTML I guess) and replying from work email messes
things up (Outlook...).

There is definitely a need for some type of hidraw / evdev interop.
What it should be I'm not fully sure. I think it needs to be some
explicit request or something from user space.

In case of the Dualsense it is a very complicated device and many
features work through HID including many audio and other features,
which I hope to support at some point. I feel the need for the driver
to be able to 'snoop' what is going through hidraw. The extra node
hack allowed for that, though ideally if a hid driver could get some
callbacks without having to go through all this extra setup code,
would be great.

For me the use cases for snooping include:
- Keep sysfs nodes in sync e.g. LED nodes with whatever a hidraw user
is setting.
- Error handling in case of a crashing hidraw client. E.g. a hidraw
client may enable rumble. If the application crashes, we probably want
the kernel driver to disable rumbling.
- Handling of audio features (speaker, microphone, headphone jack
settings, ...). One day we will provide a proper audio driver over
HID. Many of the control features work over the same HID output report
as used for rumble, LEDs etcetera.
- When no users (hidraw / evdev) are connected, potentially enable
some of the power management features (e.g. disable touchpad / sensors
at the hardware level)

There are probably some others as well.

Thanks,
Roderick

Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Benjamin Tissoires]

On Fri, May 6, 2022 at 8:59 AM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote:
> > > Hi Vicki,
> > >
> > > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> > > >
> > > > This allows the touchpad input_dev to be removed and have the driver remain
> > > > functional without its presence. This will be used to allow the touchpad to
> > > > be disabled, e.g. by a module parameter.
> > >
> > > Thanks for the contribution.
> > > I'd like to hear from Roderick, but I have a general comment here:
> > > We had Wii and Steam controllers following this logic. Now we are
> > > adding Sony PS ones... That seems like a lot of duplication, and I
> > > wonder if we should not have something more generic.
> >
>
> Using this to hook into the thread with Benjamin. It didn't make it to
> the list (due to HTML I guess) and replying from work email messes
> things up (Outlook...).
>
> There is definitely a need for some type of hidraw / evdev interop.
> What it should be I'm not fully sure. I think it needs to be some
> explicit request or something from user space.
>
> In case of the Dualsense it is a very complicated device and many
> features work through HID including many audio and other features,
> which I hope to support at some point. I feel the need for the driver
> to be able to 'snoop' what is going through hidraw. The extra node
> hack allowed for that, though ideally if a hid driver could get some
> callbacks without having to go through all this extra setup code,
> would be great.

FWIW, as you probably all know by now, I am implementing eBPF at the
HID level, and one of the use cases is to be able to have a HID
firewall.

The main reason for it right now IMO is that we allow uacess for Steam
on the PS controllers, but that also means that anybody can try to
initiate a firmware update by talking to the correct endpoints. And
even if we trust steam to do that properly or not doing it at all, we
do not trust others.

And this firewall might help you in some cases:

>
> For me the use cases for snooping include:
> - Keep sysfs nodes in sync e.g. LED nodes with whatever a hidraw user
> is setting.

BPF might help here, because we can directly snoop on HID reports. We
need to add hooks to sync the LED state, but that is doable.
A pure kernel implementation would work too though.

> - Error handling in case of a crashing hidraw client. E.g. a hidraw
> client may enable rumble. If the application crashes, we probably want
> the kernel driver to disable rumbling.

This is going to happen sooner than you think. With the systemd pull
request I mentioned in the other thread, we are going to have the
ability to revoke hidraw accesses when the client is not in foreground
or if there is a fast user switching.
Resetting to a known working state is hard if we don't know the
context, so we'll probably need some kind of helper in that situation.

> - Handling of audio features (speaker, microphone, headphone jack
> settings, ...). One day we will provide a proper audio driver over
> HID. Many of the control features work over the same HID output report
> as used for rumble, LEDs etcetera.

Definitely firewall related, possibly with on the fly modifications of
the reports.
However, there is a chance we see SDL or Steam saying "I'm going to
implement the driver on the userspace, so I can have it
cross-platform", and it's going to be hairy :(

> - When no users (hidraw / evdev) are connected, potentially enable
> some of the power management features (e.g. disable touchpad / sensors
> at the hardware level)

Shouldn't be too hard to do without bpf, and by just adding the
correct callback if any. We should already get a notification at the
HID core level when there are no users (hid_hw_open/close IIRC), so
maybe we just need a hook into the driver if there is not one already.

Cheers,
Benjamin

>
> There are probably some others as well.
>
> Thanks,
> Roderick
>