[PATCH 1/3] Input: wacom_w8001 - fix teardown order and device lifetime

[PATCH 1/3] Input: wacom_w8001 - fix teardown order and device lifetime

[Dmitry Torokhov]

In w8001_disconnect(), serio_close() was called before unregistering
the input devices. When input_unregister_device() calls w8001_close()
to shut down the hardware, w8001_command() fails because the serio
port is already closed, preventing W8001_CMD_STOP from reaching the
controller.

Additionally, w8001_connect() pre-allocated both pen_dev and touch_dev
upfront before serio_open() so w8001_interrupt() could dereference
them unconditionally, and then freed unused input devices mid-probe
without synchronizing with w8001_interrupt() when pen or touch was
absent. That created a race and use-after-free hazard with
w8001_interrupt(), as well as a double-free on the fail4 error path.

Fix these issues by guarding event reporting in w8001_interrupt()
against NULL device pointers, allocating and registering input devices
on demand after querying capabilities, synchronizing device pointer
publication and teardown with serio_pause_rx, and unregistering input
devices before closing the serio port in both disconnect and probe
error unwind paths.

Fixes: 66fd9385ee9c ("Input: wacom_w8001 - simplify w8001_remove")
Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")
Reported-by: sashiko-bot@kernel.org
Assisted-by: LLM
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 341 +++++++++++++-----------
 1 file changed, 186 insertions(+), 155 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index d8d1cdc3f09e..1c48e2775214 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -302,7 +302,8 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 
 		if (w8001->pktlen == w8001->idx) {
 			w8001->idx = 0;
-			if (w8001->type != BTN_TOOL_PEN &&
+			if (w8001->touch_dev &&
+			    w8001->type != BTN_TOOL_PEN &&
 			    w8001->type != BTN_TOOL_RUBBER) {
 				parse_single_touch(w8001->data, &coord);
 				report_single_touch(w8001, &coord);
@@ -321,8 +322,10 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 			break;
 
 		w8001->idx = 0;
-		parse_pen_data(w8001->data, &coord);
-		report_pen_events(w8001, &coord);
+		if (w8001->pen_dev) {
+			parse_pen_data(w8001->data, &coord);
+			report_pen_events(w8001, &coord);
+		}
 		break;
 
 	/* control packet */
@@ -340,7 +343,8 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 	/* 2 finger touch packet */
 	case W8001_PKTLEN_TOUCH2FG - 1:
 		w8001->idx = 0;
-		parse_multi_touch(w8001);
+		if (w8001->touch_dev)
+			parse_multi_touch(w8001);
 		break;
 
 	default:
@@ -349,7 +353,9 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 		 * sends invalid data packets that are larger than
 		 * W8001_PKTLEN_TPCPEN. Let's start over again.
 		 */
-		if (!w8001->touch_dev && w8001->idx > W8001_PKTLEN_TPCPEN - 1)
+		if (!w8001->touch_dev &&
+		    (w8001->data[0] & W8001_TAB_MASK) != W8001_TAB_BYTE &&
+		    w8001->idx > W8001_PKTLEN_TPCPEN - 1)
 			w8001->idx = 0;
 	}
 
@@ -404,19 +410,6 @@ static void w8001_close(struct input_dev *dev)
 		w8001_command(w8001, W8001_CMD_STOP, false);
 }
 
-static int w8001_detect(struct w8001 *w8001)
-{
-	int error;
-
-	error = w8001_command(w8001, W8001_CMD_STOP, false);
-	if (error)
-		return error;
-
-	msleep(250);	/* wait 250ms before querying the device */
-
-	return 0;
-}
-
 static void w8001_append_suffix(char *dest, const char *suffix, size_t dest_sz)
 {
 	size_t used = strnlen(dest, dest_sz);
@@ -424,88 +417,54 @@ static void w8001_append_suffix(char *dest, const char *suffix, size_t dest_sz)
 	strscpy(dest + used, suffix, dest_sz - used);
 }
 
-static int w8001_setup_pen(struct w8001 *w8001, char *basename,
-			   size_t basename_sz)
+static bool w8001_detect_pen(struct w8001 *w8001,
+			     char *basename, size_t basename_sz,
+			     struct w8001_coord *coord)
 {
-	struct input_dev *dev = w8001->pen_dev;
-	struct w8001_coord coord;
 	int error;
 
-	/* penabled? */
 	error = w8001_command(w8001, W8001_CMD_QUERY, true);
 	if (error)
-		return error;
-
-	__set_bit(EV_KEY, dev->evbit);
-	__set_bit(EV_ABS, dev->evbit);
-	__set_bit(BTN_TOUCH, dev->keybit);
-	__set_bit(BTN_TOOL_PEN, dev->keybit);
-	__set_bit(BTN_TOOL_RUBBER, dev->keybit);
-	__set_bit(BTN_STYLUS, dev->keybit);
-	__set_bit(BTN_STYLUS2, dev->keybit);
-	__set_bit(INPUT_PROP_DIRECT, dev->propbit);
-
-	parse_pen_data(w8001->response, &coord);
-	w8001->max_pen_x = coord.x;
-	w8001->max_pen_y = coord.y;
-
-	input_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
-	input_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
-	input_abs_set_res(dev, ABS_X, W8001_PEN_RESOLUTION);
-	input_abs_set_res(dev, ABS_Y, W8001_PEN_RESOLUTION);
-	input_set_abs_params(dev, ABS_PRESSURE, 0, coord.pen_pressure, 0, 0);
-	if (coord.tilt_x && coord.tilt_y) {
-		input_set_abs_params(dev, ABS_TILT_X, 0, coord.tilt_x, 0, 0);
-		input_set_abs_params(dev, ABS_TILT_Y, 0, coord.tilt_y, 0, 0);
-	}
+		return false;
 
+	parse_pen_data(w8001->response, coord);
+	w8001->max_pen_x = coord->x;
+	w8001->max_pen_y = coord->y;
 	w8001->id = 0x90;
 	w8001_append_suffix(basename, " Penabled", basename_sz);
 
-	return 0;
+	return true;
 }
 
-static int w8001_setup_touch(struct w8001 *w8001, char *basename,
-			     size_t basename_sz)
+static bool w8001_detect_touch(struct w8001 *w8001,
+			       char *basename, size_t basename_sz,
+			       struct w8001_touch_query *touch)
 {
-	struct input_dev *dev = w8001->touch_dev;
-	struct w8001_touch_query touch;
 	int error;
 
-
-	/* Touch enabled? */
 	error = w8001_command(w8001, W8001_CMD_TOUCHQUERY, true);
 	if (error)
-		return error;
+		return false;
+
 	/*
 	 * Some non-touch devices may reply to the touch query. But their
 	 * second byte is empty, which indicates touch is not supported.
 	 */
 	if (!w8001->response[1])
-		return -ENXIO;
+		return false;
 
-	__set_bit(EV_KEY, dev->evbit);
-	__set_bit(EV_ABS, dev->evbit);
-	__set_bit(BTN_TOUCH, dev->keybit);
-	__set_bit(INPUT_PROP_DIRECT, dev->propbit);
-
-	parse_touchquery(w8001->response, &touch);
-	w8001->max_touch_x = touch.x;
-	w8001->max_touch_y = touch.y;
+	parse_touchquery(w8001->response, touch);
+	w8001->max_touch_x = touch->x;
+	w8001->max_touch_y = touch->y;
 
 	if (w8001->max_pen_x && w8001->max_pen_y) {
 		/* if pen is supported scale to pen maximum */
-		touch.x = w8001->max_pen_x;
-		touch.y = w8001->max_pen_y;
-		touch.panel_res = W8001_PEN_RESOLUTION;
+		touch->x = w8001->max_pen_x;
+		touch->y = w8001->max_pen_y;
+		touch->panel_res = W8001_PEN_RESOLUTION;
 	}
 
-	input_set_abs_params(dev, ABS_X, 0, touch.x, 0, 0);
-	input_set_abs_params(dev, ABS_Y, 0, touch.y, 0, 0);
-	input_abs_set_res(dev, ABS_X, touch.panel_res);
-	input_abs_set_res(dev, ABS_Y, touch.panel_res);
-
-	switch (touch.sensor_id) {
+	switch (touch->sensor_id) {
 	case 0:
 	case 2:
 		w8001->pktlen = W8001_PKTLEN_TOUCH93;
@@ -523,24 +482,6 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 
 	case 5:
 		w8001->pktlen = W8001_PKTLEN_TOUCH2FG;
-
-		__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
-		error = input_mt_init_slots(dev, 2, 0);
-		if (error) {
-			dev_err(&w8001->serio->dev,
-				"failed to initialize MT slots: %d\n", error);
-			return error;
-		}
-
-		input_set_abs_params(dev, ABS_MT_POSITION_X,
-					0, touch.x, 0, 0);
-		input_set_abs_params(dev, ABS_MT_POSITION_Y,
-					0, touch.y, 0, 0);
-		input_set_abs_params(dev, ABS_MT_TOOL_TYPE,
-					0, MT_TOOL_MAX, 0, 0);
-		input_abs_set_res(dev, ABS_MT_POSITION_X, touch.panel_res);
-		input_abs_set_res(dev, ABS_MT_POSITION_Y, touch.panel_res);
-
 		w8001_append_suffix(basename, " 2FG", basename_sz);
 		if (w8001->max_pen_x && w8001->max_pen_y)
 			w8001->id = 0xE3;
@@ -550,13 +491,19 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 	}
 
 	w8001_append_suffix(basename, " Touchscreen", basename_sz);
-
-	return 0;
+	return true;
 }
 
-static void w8001_set_devdata(struct input_dev *dev, struct w8001 *w8001,
-			      struct serio *serio)
+static struct input_dev *w8001_allocate_device(struct w8001 *w8001,
+					       const char *name)
 {
+	struct input_dev *dev;
+
+	dev = input_allocate_device();
+	if (!dev)
+		return NULL;
+
+	dev->name = name;
 	dev->phys = w8001->phys;
 	dev->id.bustype = BUS_RS232;
 	dev->id.product = w8001->id;
@@ -565,28 +512,139 @@ static void w8001_set_devdata(struct input_dev *dev, struct w8001 *w8001,
 	dev->open = w8001_open;
 	dev->close = w8001_close;
 
-	dev->dev.parent = &serio->dev;
+	dev->dev.parent = &w8001->serio->dev;
+
+	__set_bit(INPUT_PROP_DIRECT, dev->propbit);
+	input_set_capability(dev, EV_KEY, BTN_TOUCH);
 
 	input_set_drvdata(dev, w8001);
+
+	return dev;
+}
+
+static int w8001_setup_pen(struct w8001 *w8001, const char *basename,
+			   const struct w8001_coord *coord)
+{
+	struct input_dev *dev;
+	int error;
+
+	snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Pen", basename);
+
+	dev = w8001_allocate_device(w8001, w8001->pen_name);
+	if (!dev)
+		return -ENOMEM;
+
+	__set_bit(BTN_TOOL_PEN, dev->keybit);
+	__set_bit(BTN_TOOL_RUBBER, dev->keybit);
+	__set_bit(BTN_STYLUS, dev->keybit);
+	__set_bit(BTN_STYLUS2, dev->keybit);
+
+	input_set_abs_params(dev, ABS_X, 0, coord->x, 0, 0);
+	input_set_abs_params(dev, ABS_Y, 0, coord->y, 0, 0);
+	input_abs_set_res(dev, ABS_X, W8001_PEN_RESOLUTION);
+	input_abs_set_res(dev, ABS_Y, W8001_PEN_RESOLUTION);
+	input_set_abs_params(dev, ABS_PRESSURE, 0, coord->pen_pressure, 0, 0);
+	if (coord->tilt_x && coord->tilt_y) {
+		input_set_abs_params(dev, ABS_TILT_X, 0, coord->tilt_x, 0, 0);
+		input_set_abs_params(dev, ABS_TILT_Y, 0, coord->tilt_y, 0, 0);
+	}
+
+	scoped_guard(serio_pause_rx, w8001->serio)
+		w8001->pen_dev = dev;
+
+	error = input_register_device(dev);
+	if (error)
+		goto err_unpublish_dev;
+
+	return 0;
+
+err_unpublish_dev:
+	scoped_guard(serio_pause_rx, w8001->serio)
+		w8001->pen_dev = NULL;
+	input_free_device(dev);
+	return error;
+}
+
+static int w8001_setup_touch(struct w8001 *w8001, const char *basename,
+			     const struct w8001_touch_query *touch)
+{
+	struct input_dev *dev;
+	int error;
+
+	snprintf(w8001->touch_name, sizeof(w8001->touch_name), "%s Finger", basename);
+
+	dev = w8001_allocate_device(w8001, w8001->touch_name);
+	if (!dev)
+		return -ENOMEM;
+
+	input_set_abs_params(dev, ABS_X, 0, touch->x, 0, 0);
+	input_set_abs_params(dev, ABS_Y, 0, touch->y, 0, 0);
+	input_abs_set_res(dev, ABS_X, touch->panel_res);
+	input_abs_set_res(dev, ABS_Y, touch->panel_res);
+
+	if (touch->sensor_id == 5) {
+		__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
+		error = input_mt_init_slots(dev, 2, 0);
+		if (error) {
+			dev_err(&w8001->serio->dev,
+				"failed to initialize MT slots: %d\n", error);
+			goto err_free_dev;
+		}
+
+		input_set_abs_params(dev, ABS_MT_POSITION_X, 0, touch->x, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, touch->y, 0, 0);
+		input_set_abs_params(dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
+		input_abs_set_res(dev, ABS_MT_POSITION_X, touch->panel_res);
+		input_abs_set_res(dev, ABS_MT_POSITION_Y, touch->panel_res);
+	}
+
+	scoped_guard(serio_pause_rx, w8001->serio)
+		w8001->touch_dev = dev;
+
+	error = input_register_device(dev);
+	if (error)
+		goto err_unpublish_dev;
+
+	return 0;
+
+err_unpublish_dev:
+	scoped_guard(serio_pause_rx, w8001->serio)
+		w8001->touch_dev = NULL;
+err_free_dev:
+	input_free_device(dev);
+	return error;
 }
 
 /*
  * w8001_disconnect() is the opposite of w8001_connect()
  */
 
+static void w8001_unregister_devices(struct w8001 *w8001)
+{
+	struct input_dev *pen_dev = NULL;
+	struct input_dev *touch_dev = NULL;
+
+	scoped_guard(serio_pause_rx, w8001->serio) {
+		swap(pen_dev, w8001->pen_dev);
+		swap(touch_dev, w8001->touch_dev);
+	}
+
+	if (pen_dev)
+		input_unregister_device(pen_dev);
+	if (touch_dev)
+		input_unregister_device(touch_dev);
+}
+
 static void w8001_disconnect(struct serio *serio)
 {
 	struct w8001 *w8001 = serio_get_drvdata(serio);
 
+	w8001_unregister_devices(w8001);
+
 	serio_close(serio);
+	serio_set_drvdata(serio, NULL);
 
-	if (w8001->pen_dev)
-		input_unregister_device(w8001->pen_dev);
-	if (w8001->touch_dev)
-		input_unregister_device(w8001->touch_dev);
 	kfree(w8001);
-
-	serio_set_drvdata(serio, NULL);
 }
 
 /*
@@ -598,22 +656,17 @@ static void w8001_disconnect(struct serio *serio)
 static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 {
 	struct w8001 *w8001;
-	struct input_dev *input_dev_pen;
-	struct input_dev *input_dev_touch;
+	struct w8001_coord coord;
+	struct w8001_touch_query touch;
 	char basename[64] = "Wacom Serial";
-	int err, err_pen, err_touch;
+	bool has_pen, has_touch;
+	int err;
 
 	w8001 = kzalloc_obj(*w8001);
-	input_dev_pen = input_allocate_device();
-	input_dev_touch = input_allocate_device();
-	if (!w8001 || !input_dev_pen || !input_dev_touch) {
-		err = -ENOMEM;
-		goto fail1;
-	}
+	if (!w8001)
+		return -ENOMEM;
 
 	w8001->serio = serio;
-	w8001->pen_dev = input_dev_pen;
-	w8001->touch_dev = input_dev_touch;
 	mutex_init(&w8001->mutex);
 	init_completion(&w8001->cmd_done);
 	snprintf(w8001->phys, sizeof(w8001->phys), "%s/input0", serio->phys);
@@ -621,66 +674,44 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	serio_set_drvdata(serio, w8001);
 	err = serio_open(serio, drv);
 	if (err)
-		goto fail2;
+		goto err_free_w8001;
 
-	err = w8001_detect(w8001);
+	err = w8001_command(w8001, W8001_CMD_STOP, false);
 	if (err)
-		goto fail3;
+		goto err_close_serio;
+
+	msleep(250);	/* wait 250ms before querying the device */
 
-	/* For backwards-compatibility we compose the basename based on
+	/*
+	 * For backwards-compatibility we compose the basename based on
 	 * capabilities and then just append the tool type
 	 */
-	err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
-	err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
-	if (err_pen && err_touch) {
+	has_pen = w8001_detect_pen(w8001, basename, sizeof(basename), &coord);
+	has_touch = w8001_detect_touch(w8001, basename, sizeof(basename), &touch);
+	if (!has_pen && !has_touch) {
 		err = -ENXIO;
-		goto fail3;
+		goto err_close_serio;
 	}
 
-	if (!err_pen) {
-		snprintf(w8001->pen_name, sizeof(w8001->pen_name),
-			 "%s Pen", basename);
-		input_dev_pen->name = w8001->pen_name;
-
-		w8001_set_devdata(input_dev_pen, w8001, serio);
-
-		err = input_register_device(w8001->pen_dev);
+	if (has_pen) {
+		err = w8001_setup_pen(w8001, basename, &coord);
 		if (err)
-			goto fail3;
-	} else {
-		input_free_device(input_dev_pen);
-		input_dev_pen = NULL;
-		w8001->pen_dev = NULL;
+			goto err_close_serio;
 	}
 
-	if (!err_touch) {
-		snprintf(w8001->touch_name, sizeof(w8001->touch_name),
-			 "%s Finger", basename);
-		input_dev_touch->name = w8001->touch_name;
-
-		w8001_set_devdata(input_dev_touch, w8001, serio);
-
-		err = input_register_device(w8001->touch_dev);
+	if (has_touch) {
+		err = w8001_setup_touch(w8001, basename, &touch);
 		if (err)
-			goto fail4;
-	} else {
-		input_free_device(input_dev_touch);
-		input_dev_touch = NULL;
-		w8001->touch_dev = NULL;
+			goto err_close_serio;
 	}
 
 	return 0;
 
-fail4:
-	if (w8001->pen_dev)
-		input_unregister_device(w8001->pen_dev);
-fail3:
+err_close_serio:
+	w8001_unregister_devices(w8001);
 	serio_close(serio);
-fail2:
+err_free_w8001:
 	serio_set_drvdata(serio, NULL);
-fail1:
-	input_free_device(input_dev_pen);
-	input_free_device(input_dev_touch);
 	kfree(w8001);
 	return err;
 }
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH 2/3] Input: wacom_w8001 - assign distinct phys paths to pen and touch devices

[Dmitry Torokhov]

Previously, both pen and touch input devices used the same phys path
"%s/input0", which made it impossible for userspace or udev rules to
distinguish the devices by physical topology.

Assign distinct phys paths by introducing separate buffers pen_phys
("%s/input0") and touch_phys ("%s/input1") in struct w8001. Pass the
corresponding phys path to w8001_allocate_device() during pen and touch
setup, and drop the shared phys buffer from struct w8001.

Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")
Assisted-by: LLM
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 1c48e2775214..e967f2413706 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -91,7 +91,8 @@ struct w8001 {
 	unsigned char response_type;
 	unsigned char response[W8001_MAX_LENGTH];
 	unsigned char data[W8001_MAX_LENGTH];
-	char phys[W8001_MAX_PHYS];
+	char pen_phys[W8001_MAX_PHYS];
+	char touch_phys[W8001_MAX_PHYS];
 	int type;
 	unsigned int pktlen;
 	u16 max_touch_x;
@@ -495,7 +496,8 @@ static bool w8001_detect_touch(struct w8001 *w8001,
 }
 
 static struct input_dev *w8001_allocate_device(struct w8001 *w8001,
-					       const char *name)
+					       const char *name,
+					       const char *phys)
 {
 	struct input_dev *dev;
 
@@ -504,7 +506,7 @@ static struct input_dev *w8001_allocate_device(struct w8001 *w8001,
 		return NULL;
 
 	dev->name = name;
-	dev->phys = w8001->phys;
+	dev->phys = phys;
 	dev->id.bustype = BUS_RS232;
 	dev->id.product = w8001->id;
 	dev->id.vendor = 0x056a;
@@ -529,8 +531,10 @@ static int w8001_setup_pen(struct w8001 *w8001, const char *basename,
 	int error;
 
 	snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Pen", basename);
+	snprintf(w8001->pen_phys, sizeof(w8001->pen_phys), "%s/input0",
+		 w8001->serio->phys);
 
-	dev = w8001_allocate_device(w8001, w8001->pen_name);
+	dev = w8001_allocate_device(w8001, w8001->pen_name, w8001->pen_phys);
 	if (!dev)
 		return -ENOMEM;
 
@@ -572,8 +576,10 @@ static int w8001_setup_touch(struct w8001 *w8001, const char *basename,
 	int error;
 
 	snprintf(w8001->touch_name, sizeof(w8001->touch_name), "%s Finger", basename);
+	snprintf(w8001->touch_phys, sizeof(w8001->touch_phys), "%s/input1",
+		 w8001->serio->phys);
 
-	dev = w8001_allocate_device(w8001, w8001->touch_name);
+	dev = w8001_allocate_device(w8001, w8001->touch_name, w8001->touch_phys);
 	if (!dev)
 		return -ENOMEM;
 
@@ -669,7 +675,6 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	w8001->serio = serio;
 	mutex_init(&w8001->mutex);
 	init_completion(&w8001->cmd_done);
-	snprintf(w8001->phys, sizeof(w8001->phys), "%s/input0", serio->phys);
 
 	serio_set_drvdata(serio, w8001);
 	err = serio_open(serio, drv);
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH 2/3] Input: wacom_w8001 - assign distinct phys paths to pen and touch devices

[Jason Gerecke]

On Fri, Oct 9, 2026 at 12:28 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Previously, both pen and touch input devices used the same phys path
> "%s/input0", which made it impossible for userspace or udev rules to
> distinguish the devices by physical topology.
>
> Assign distinct phys paths by introducing separate buffers pen_phys
> ("%s/input0") and touch_phys ("%s/input1") in struct w8001. Pass the
> corresponding phys path to w8001_allocate_device() during pen and touch
> setup, and drop the shared phys buffer from struct w8001.
>
> Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")

Does this really deserve a "Fixes" tag? This does not seem to be as
much of a "bug" as it is a limitation. Userspace and udev rules are
(currently) able to reliably distinguish the devices by name.
Obviously that's not ideal, but I don't see much benefit to this patch
getting backported to the stable trees -- especially when /existing/
udev rules aren't necessarily going to know what to do with this new
"input1" phys.

> Assisted-by: LLM

Same Assisted-by comments from Patch 1/3 apply.

Jason (she/they)
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....


> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 1c48e2775214..e967f2413706 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -91,7 +91,8 @@ struct w8001 {
>         unsigned char response_type;
>         unsigned char response[W8001_MAX_LENGTH];
>         unsigned char data[W8001_MAX_LENGTH];
> -       char phys[W8001_MAX_PHYS];
> +       char pen_phys[W8001_MAX_PHYS];
> +       char touch_phys[W8001_MAX_PHYS];
>         int type;
>         unsigned int pktlen;
>         u16 max_touch_x;
> @@ -495,7 +496,8 @@ static bool w8001_detect_touch(struct w8001 *w8001,
>  }
>
>  static struct input_dev *w8001_allocate_device(struct w8001 *w8001,
> -                                              const char *name)
> +                                              const char *name,
> +                                              const char *phys)
>  {
>         struct input_dev *dev;
>
> @@ -504,7 +506,7 @@ static struct input_dev *w8001_allocate_device(struct w8001 *w8001,
>                 return NULL;
>
>         dev->name = name;
> -       dev->phys = w8001->phys;
> +       dev->phys = phys;
>         dev->id.bustype = BUS_RS232;
>         dev->id.product = w8001->id;
>         dev->id.vendor = 0x056a;
> @@ -529,8 +531,10 @@ static int w8001_setup_pen(struct w8001 *w8001, const char *basename,
>         int error;
>
>         snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Pen", basename);
> +       snprintf(w8001->pen_phys, sizeof(w8001->pen_phys), "%s/input0",
> +                w8001->serio->phys);
>
> -       dev = w8001_allocate_device(w8001, w8001->pen_name);
> +       dev = w8001_allocate_device(w8001, w8001->pen_name, w8001->pen_phys);
>         if (!dev)
>                 return -ENOMEM;
>
> @@ -572,8 +576,10 @@ static int w8001_setup_touch(struct w8001 *w8001, const char *basename,
>         int error;
>
>         snprintf(w8001->touch_name, sizeof(w8001->touch_name), "%s Finger", basename);
> +       snprintf(w8001->touch_phys, sizeof(w8001->touch_phys), "%s/input1",
> +                w8001->serio->phys);
>
> -       dev = w8001_allocate_device(w8001, w8001->touch_name);
> +       dev = w8001_allocate_device(w8001, w8001->touch_name, w8001->touch_phys);
>         if (!dev)
>                 return -ENOMEM;
>
> @@ -669,7 +675,6 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>         w8001->serio = serio;
>         mutex_init(&w8001->mutex);
>         init_completion(&w8001->cmd_done);
> -       snprintf(w8001->phys, sizeof(w8001->phys), "%s/input0", serio->phys);
>
>         serio_set_drvdata(serio, w8001);
>         err = serio_open(serio, drv);
> --
> 2.56.0.385.gd3acb90ef8-goog
>
>

Re: [PATCH 2/3] Input: wacom_w8001 - assign distinct phys paths to pen and touch devices

[Dmitry Torokhov]

On Tue, Oct 13, 2026 at 03:42:10PM -0700, Jason Gerecke wrote:
> On Fri, Oct 9, 2026 at 12:28 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Previously, both pen and touch input devices used the same phys path
> > "%s/input0", which made it impossible for userspace or udev rules to
> > distinguish the devices by physical topology.
> >
> > Assign distinct phys paths by introducing separate buffers pen_phys
> > ("%s/input0") and touch_phys ("%s/input1") in struct w8001. Pass the
> > corresponding phys path to w8001_allocate_device() during pen and touch
> > setup, and drop the shared phys buffer from struct w8001.
> >
> > Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")
> 
> Does this really deserve a "Fixes" tag? This does not seem to be as
> much of a "bug" as it is a limitation. Userspace and udev rules are
> (currently) able to reliably distinguish the devices by name.

It is a bug as far as input subsystem is concerned and it was introduced
in the above referenced commit, so it has this tag.

> Obviously that's not ideal, but I don't see much benefit to this patch
> getting backported to the stable trees -- especially when /existing/
> udev rules aren't necessarily going to know what to do with this new
> "input1" phys.

Right. For input subsystem only commits that are explicitly marked
as Cc: stable@ will be backported to stable trees, so it shoudl not be
a concern.

Thanks.

-- 
Dmitry

[PATCH 3/3] Input: wacom_w8001 - use ACQUIRE() for interruptible mutex in w8001_open()

[Dmitry Torokhov]

Switch from scoped_guard(mutex_intr, ...) with an error fallback of
-EINTR to ACQUIRE(mutex_intr, lock) and ACQUIRE_ERR(mutex_intr, &lock).
This flattens the function logic and propagates the exact return value
from mutex_lock_interruptible().

Assisted-by: LLM
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/wacom_w8001.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index e967f2413706..b34fcac44f63 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -387,18 +387,20 @@ static int w8001_open(struct input_dev *dev)
 	struct w8001 *w8001 = input_get_drvdata(dev);
 	int err;
 
-	scoped_guard(mutex_intr, &w8001->mutex) {
-		if (w8001->open_count == 0) {
-			err = w8001_command(w8001, W8001_CMD_START, false);
-			if (err)
-				return err;
-		}
+	ACQUIRE(mutex_intr, lock)(&w8001->mutex);
+	err = ACQUIRE_ERR(mutex_intr, &lock);
+	if (err)
+		return err;
 
-		w8001->open_count++;
-		return 0;
+	if (w8001->open_count == 0) {
+		err = w8001_command(w8001, W8001_CMD_START, false);
+		if (err)
+			return err;
 	}
 
-	return -EINTR;
+	w8001->open_count++;
+
+	return 0;
 }
 
 static void w8001_close(struct input_dev *dev)
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH 3/3] Input: wacom_w8001 - use ACQUIRE() for interruptible mutex in w8001_open()

[Jason Gerecke]

On Fri, Oct 9, 2026 at 12:28 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Switch from scoped_guard(mutex_intr, ...) with an error fallback of
> -EINTR to ACQUIRE(mutex_intr, lock) and ACQUIRE_ERR(mutex_intr, &lock).
> This flattens the function logic and propagates the exact return value
> from mutex_lock_interruptible().
>
> Assisted-by: LLM

Aside from the Assisted-by comment in Patch 1/3, this LGTM.

Reviewed-by: Jason Gerecke <jason.gerecke@wacom.com>

Jason (she/they)
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....


> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index e967f2413706..b34fcac44f63 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -387,18 +387,20 @@ static int w8001_open(struct input_dev *dev)
>         struct w8001 *w8001 = input_get_drvdata(dev);
>         int err;
>
> -       scoped_guard(mutex_intr, &w8001->mutex) {
> -               if (w8001->open_count == 0) {
> -                       err = w8001_command(w8001, W8001_CMD_START, false);
> -                       if (err)
> -                               return err;
> -               }
> +       ACQUIRE(mutex_intr, lock)(&w8001->mutex);
> +       err = ACQUIRE_ERR(mutex_intr, &lock);
> +       if (err)
> +               return err;
>
> -               w8001->open_count++;
> -               return 0;
> +       if (w8001->open_count == 0) {
> +               err = w8001_command(w8001, W8001_CMD_START, false);
> +               if (err)
> +                       return err;
>         }
>
> -       return -EINTR;
> +       w8001->open_count++;
> +
> +       return 0;
>  }
>
>  static void w8001_close(struct input_dev *dev)
> --
> 2.56.0.385.gd3acb90ef8-goog
>
>

Re: [PATCH 1/3] Input: wacom_w8001 - fix teardown order and device lifetime

[Jason Gerecke]

On Fri, Oct 9, 2026 at 12:25 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> In w8001_disconnect(), serio_close() was called before unregistering
> the input devices. When input_unregister_device() calls w8001_close()
> to shut down the hardware, w8001_command() fails because the serio
> port is already closed, preventing W8001_CMD_STOP from reaching the
> controller.
>
> Additionally, w8001_connect() pre-allocated both pen_dev and touch_dev
> upfront before serio_open() so w8001_interrupt() could dereference
> them unconditionally, and then freed unused input devices mid-probe
> without synchronizing with w8001_interrupt() when pen or touch was
> absent. That created a race and use-after-free hazard with
> w8001_interrupt(), as well as a double-free on the fail4 error path.
>
> Fix these issues by guarding event reporting in w8001_interrupt()
> against NULL device pointers, allocating and registering input devices
> on demand after querying capabilities, synchronizing device pointer
> publication and teardown with serio_pause_rx, and unregistering input
> devices before closing the serio port in both disconnect and probe
> error unwind paths.
>

This patch seems to be pretty complicated for a 'Fixes' and I'd feel a
lot more comfortable reviewing it if it could be broken into smaller
chunks. I know that might not be possible, but it's pretty hard to
audit all the changes here. There's nothing that sets off loud alarms,
but I think at this level of complexity the best I could do is Ack the
patch...

> Fixes: 66fd9385ee9c ("Input: wacom_w8001 - simplify w8001_remove")
> Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")
> Reported-by: sashiko-bot@kernel.org
> Assisted-by: LLM

Just a note that current recommendations are to include the agent name
and model version in Assisted-by lines:

https://elixir.bootlin.com/linux/v7.2.9/source/Documentation/process/coding-assistants.rst#L39

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c | 341 +++++++++++++-----------
>  1 file changed, 186 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index d8d1cdc3f09e..1c48e2775214 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -302,7 +302,8 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>
>                 if (w8001->pktlen == w8001->idx) {
>                         w8001->idx = 0;
> -                       if (w8001->type != BTN_TOOL_PEN &&
> +                       if (w8001->touch_dev &&
> +                           w8001->type != BTN_TOOL_PEN &&
>                             w8001->type != BTN_TOOL_RUBBER) {
>                                 parse_single_touch(w8001->data, &coord);
>                                 report_single_touch(w8001, &coord);
> @@ -321,8 +322,10 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>                         break;
>
>                 w8001->idx = 0;
> -               parse_pen_data(w8001->data, &coord);
> -               report_pen_events(w8001, &coord);
> +               if (w8001->pen_dev) {
> +                       parse_pen_data(w8001->data, &coord);
> +                       report_pen_events(w8001, &coord);
> +               }
>                 break;
>
>         /* control packet */
> @@ -340,7 +343,8 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>         /* 2 finger touch packet */
>         case W8001_PKTLEN_TOUCH2FG - 1:
>                 w8001->idx = 0;
> -               parse_multi_touch(w8001);
> +               if (w8001->touch_dev)
> +                       parse_multi_touch(w8001);
>                 break;
>
>         default:
> @@ -349,7 +353,9 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>                  * sends invalid data packets that are larger than
>                  * W8001_PKTLEN_TPCPEN. Let's start over again.
>                  */
> -               if (!w8001->touch_dev && w8001->idx > W8001_PKTLEN_TPCPEN - 1)
> +               if (!w8001->touch_dev &&
> +                   (w8001->data[0] & W8001_TAB_MASK) != W8001_TAB_BYTE &&
> +                   w8001->idx > W8001_PKTLEN_TPCPEN - 1)

I'm not an expert in the w8001 protocol and don't have docs handy at
the moment. Can you explain the purpose of adding this new
W8001_TAB_BYTE check? I have a vague idea that it might have something
to do with allowing the query response packets to be processed prior
to pen/touch being initialized?

Jason (she/they)
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....

>                         w8001->idx = 0;
>         }
>
> @@ -404,19 +410,6 @@ static void w8001_close(struct input_dev *dev)
>                 w8001_command(w8001, W8001_CMD_STOP, false);
>  }
>
> -static int w8001_detect(struct w8001 *w8001)
> -{
> -       int error;
> -
> -       error = w8001_command(w8001, W8001_CMD_STOP, false);
> -       if (error)
> -               return error;
> -
> -       msleep(250);    /* wait 250ms before querying the device */
> -
> -       return 0;
> -}
> -
>  static void w8001_append_suffix(char *dest, const char *suffix, size_t dest_sz)
>  {
>         size_t used = strnlen(dest, dest_sz);
> @@ -424,88 +417,54 @@ static void w8001_append_suffix(char *dest, const char *suffix, size_t dest_sz)
>         strscpy(dest + used, suffix, dest_sz - used);
>  }
>
> -static int w8001_setup_pen(struct w8001 *w8001, char *basename,
> -                          size_t basename_sz)
> +static bool w8001_detect_pen(struct w8001 *w8001,
> +                            char *basename, size_t basename_sz,
> +                            struct w8001_coord *coord)
>  {
> -       struct input_dev *dev = w8001->pen_dev;
> -       struct w8001_coord coord;
>         int error;
>
> -       /* penabled? */
>         error = w8001_command(w8001, W8001_CMD_QUERY, true);
>         if (error)
> -               return error;
> -
> -       __set_bit(EV_KEY, dev->evbit);
> -       __set_bit(EV_ABS, dev->evbit);
> -       __set_bit(BTN_TOUCH, dev->keybit);
> -       __set_bit(BTN_TOOL_PEN, dev->keybit);
> -       __set_bit(BTN_TOOL_RUBBER, dev->keybit);
> -       __set_bit(BTN_STYLUS, dev->keybit);
> -       __set_bit(BTN_STYLUS2, dev->keybit);
> -       __set_bit(INPUT_PROP_DIRECT, dev->propbit);
> -
> -       parse_pen_data(w8001->response, &coord);
> -       w8001->max_pen_x = coord.x;
> -       w8001->max_pen_y = coord.y;
> -
> -       input_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
> -       input_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
> -       input_abs_set_res(dev, ABS_X, W8001_PEN_RESOLUTION);
> -       input_abs_set_res(dev, ABS_Y, W8001_PEN_RESOLUTION);
> -       input_set_abs_params(dev, ABS_PRESSURE, 0, coord.pen_pressure, 0, 0);
> -       if (coord.tilt_x && coord.tilt_y) {
> -               input_set_abs_params(dev, ABS_TILT_X, 0, coord.tilt_x, 0, 0);
> -               input_set_abs_params(dev, ABS_TILT_Y, 0, coord.tilt_y, 0, 0);
> -       }
> +               return false;
>
> +       parse_pen_data(w8001->response, coord);
> +       w8001->max_pen_x = coord->x;
> +       w8001->max_pen_y = coord->y;
>         w8001->id = 0x90;
>         w8001_append_suffix(basename, " Penabled", basename_sz);
>
> -       return 0;
> +       return true;
>  }
>
> -static int w8001_setup_touch(struct w8001 *w8001, char *basename,
> -                            size_t basename_sz)
> +static bool w8001_detect_touch(struct w8001 *w8001,
> +                              char *basename, size_t basename_sz,
> +                              struct w8001_touch_query *touch)
>  {
> -       struct input_dev *dev = w8001->touch_dev;
> -       struct w8001_touch_query touch;
>         int error;
>
> -
> -       /* Touch enabled? */
>         error = w8001_command(w8001, W8001_CMD_TOUCHQUERY, true);
>         if (error)
> -               return error;
> +               return false;
> +
>         /*
>          * Some non-touch devices may reply to the touch query. But their
>          * second byte is empty, which indicates touch is not supported.
>          */
>         if (!w8001->response[1])
> -               return -ENXIO;
> +               return false;
>
> -       __set_bit(EV_KEY, dev->evbit);
> -       __set_bit(EV_ABS, dev->evbit);
> -       __set_bit(BTN_TOUCH, dev->keybit);
> -       __set_bit(INPUT_PROP_DIRECT, dev->propbit);
> -
> -       parse_touchquery(w8001->response, &touch);
> -       w8001->max_touch_x = touch.x;
> -       w8001->max_touch_y = touch.y;
> +       parse_touchquery(w8001->response, touch);
> +       w8001->max_touch_x = touch->x;
> +       w8001->max_touch_y = touch->y;
>
>         if (w8001->max_pen_x && w8001->max_pen_y) {
>                 /* if pen is supported scale to pen maximum */
> -               touch.x = w8001->max_pen_x;
> -               touch.y = w8001->max_pen_y;
> -               touch.panel_res = W8001_PEN_RESOLUTION;
> +               touch->x = w8001->max_pen_x;
> +               touch->y = w8001->max_pen_y;
> +               touch->panel_res = W8001_PEN_RESOLUTION;
>         }
>
> -       input_set_abs_params(dev, ABS_X, 0, touch.x, 0, 0);
> -       input_set_abs_params(dev, ABS_Y, 0, touch.y, 0, 0);
> -       input_abs_set_res(dev, ABS_X, touch.panel_res);
> -       input_abs_set_res(dev, ABS_Y, touch.panel_res);
> -
> -       switch (touch.sensor_id) {
> +       switch (touch->sensor_id) {
>         case 0:
>         case 2:
>                 w8001->pktlen = W8001_PKTLEN_TOUCH93;
> @@ -523,24 +482,6 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
>
>         case 5:
>                 w8001->pktlen = W8001_PKTLEN_TOUCH2FG;
> -
> -               __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
> -               error = input_mt_init_slots(dev, 2, 0);
> -               if (error) {
> -                       dev_err(&w8001->serio->dev,
> -                               "failed to initialize MT slots: %d\n", error);
> -                       return error;
> -               }
> -
> -               input_set_abs_params(dev, ABS_MT_POSITION_X,
> -                                       0, touch.x, 0, 0);
> -               input_set_abs_params(dev, ABS_MT_POSITION_Y,
> -                                       0, touch.y, 0, 0);
> -               input_set_abs_params(dev, ABS_MT_TOOL_TYPE,
> -                                       0, MT_TOOL_MAX, 0, 0);
> -               input_abs_set_res(dev, ABS_MT_POSITION_X, touch.panel_res);
> -               input_abs_set_res(dev, ABS_MT_POSITION_Y, touch.panel_res);
> -
>                 w8001_append_suffix(basename, " 2FG", basename_sz);
>                 if (w8001->max_pen_x && w8001->max_pen_y)
>                         w8001->id = 0xE3;
> @@ -550,13 +491,19 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
>         }
>
>         w8001_append_suffix(basename, " Touchscreen", basename_sz);
> -
> -       return 0;
> +       return true;
>  }
>
> -static void w8001_set_devdata(struct input_dev *dev, struct w8001 *w8001,
> -                             struct serio *serio)
> +static struct input_dev *w8001_allocate_device(struct w8001 *w8001,
> +                                              const char *name)
>  {
> +       struct input_dev *dev;
> +
> +       dev = input_allocate_device();
> +       if (!dev)
> +               return NULL;
> +
> +       dev->name = name;
>         dev->phys = w8001->phys;
>         dev->id.bustype = BUS_RS232;
>         dev->id.product = w8001->id;
> @@ -565,28 +512,139 @@ static void w8001_set_devdata(struct input_dev *dev, struct w8001 *w8001,
>         dev->open = w8001_open;
>         dev->close = w8001_close;
>
> -       dev->dev.parent = &serio->dev;
> +       dev->dev.parent = &w8001->serio->dev;
> +
> +       __set_bit(INPUT_PROP_DIRECT, dev->propbit);
> +       input_set_capability(dev, EV_KEY, BTN_TOUCH);
>
>         input_set_drvdata(dev, w8001);
> +
> +       return dev;
> +}
> +
> +static int w8001_setup_pen(struct w8001 *w8001, const char *basename,
> +                          const struct w8001_coord *coord)
> +{
> +       struct input_dev *dev;
> +       int error;
> +
> +       snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Pen", basename);
> +
> +       dev = w8001_allocate_device(w8001, w8001->pen_name);
> +       if (!dev)
> +               return -ENOMEM;
> +
> +       __set_bit(BTN_TOOL_PEN, dev->keybit);
> +       __set_bit(BTN_TOOL_RUBBER, dev->keybit);
> +       __set_bit(BTN_STYLUS, dev->keybit);
> +       __set_bit(BTN_STYLUS2, dev->keybit);
> +
> +       input_set_abs_params(dev, ABS_X, 0, coord->x, 0, 0);
> +       input_set_abs_params(dev, ABS_Y, 0, coord->y, 0, 0);
> +       input_abs_set_res(dev, ABS_X, W8001_PEN_RESOLUTION);
> +       input_abs_set_res(dev, ABS_Y, W8001_PEN_RESOLUTION);
> +       input_set_abs_params(dev, ABS_PRESSURE, 0, coord->pen_pressure, 0, 0);
> +       if (coord->tilt_x && coord->tilt_y) {
> +               input_set_abs_params(dev, ABS_TILT_X, 0, coord->tilt_x, 0, 0);
> +               input_set_abs_params(dev, ABS_TILT_Y, 0, coord->tilt_y, 0, 0);
> +       }
> +
> +       scoped_guard(serio_pause_rx, w8001->serio)
> +               w8001->pen_dev = dev;
> +
> +       error = input_register_device(dev);
> +       if (error)
> +               goto err_unpublish_dev;
> +
> +       return 0;
> +
> +err_unpublish_dev:
> +       scoped_guard(serio_pause_rx, w8001->serio)
> +               w8001->pen_dev = NULL;
> +       input_free_device(dev);
> +       return error;
> +}
> +
> +static int w8001_setup_touch(struct w8001 *w8001, const char *basename,
> +                            const struct w8001_touch_query *touch)
> +{
> +       struct input_dev *dev;
> +       int error;
> +
> +       snprintf(w8001->touch_name, sizeof(w8001->touch_name), "%s Finger", basename);
> +
> +       dev = w8001_allocate_device(w8001, w8001->touch_name);
> +       if (!dev)
> +               return -ENOMEM;
> +
> +       input_set_abs_params(dev, ABS_X, 0, touch->x, 0, 0);
> +       input_set_abs_params(dev, ABS_Y, 0, touch->y, 0, 0);
> +       input_abs_set_res(dev, ABS_X, touch->panel_res);
> +       input_abs_set_res(dev, ABS_Y, touch->panel_res);
> +
> +       if (touch->sensor_id == 5) {
> +               __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
> +               error = input_mt_init_slots(dev, 2, 0);
> +               if (error) {
> +                       dev_err(&w8001->serio->dev,
> +                               "failed to initialize MT slots: %d\n", error);
> +                       goto err_free_dev;
> +               }
> +
> +               input_set_abs_params(dev, ABS_MT_POSITION_X, 0, touch->x, 0, 0);
> +               input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, touch->y, 0, 0);
> +               input_set_abs_params(dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> +               input_abs_set_res(dev, ABS_MT_POSITION_X, touch->panel_res);
> +               input_abs_set_res(dev, ABS_MT_POSITION_Y, touch->panel_res);
> +       }
> +
> +       scoped_guard(serio_pause_rx, w8001->serio)
> +               w8001->touch_dev = dev;
> +
> +       error = input_register_device(dev);
> +       if (error)
> +               goto err_unpublish_dev;
> +
> +       return 0;
> +
> +err_unpublish_dev:
> +       scoped_guard(serio_pause_rx, w8001->serio)
> +               w8001->touch_dev = NULL;
> +err_free_dev:
> +       input_free_device(dev);
> +       return error;
>  }
>
>  /*
>   * w8001_disconnect() is the opposite of w8001_connect()
>   */
>
> +static void w8001_unregister_devices(struct w8001 *w8001)
> +{
> +       struct input_dev *pen_dev = NULL;
> +       struct input_dev *touch_dev = NULL;
> +
> +       scoped_guard(serio_pause_rx, w8001->serio) {
> +               swap(pen_dev, w8001->pen_dev);
> +               swap(touch_dev, w8001->touch_dev);
> +       }
> +
> +       if (pen_dev)
> +               input_unregister_device(pen_dev);
> +       if (touch_dev)
> +               input_unregister_device(touch_dev);
> +}
> +
>  static void w8001_disconnect(struct serio *serio)
>  {
>         struct w8001 *w8001 = serio_get_drvdata(serio);
>
> +       w8001_unregister_devices(w8001);
> +
>         serio_close(serio);
> +       serio_set_drvdata(serio, NULL);
>
> -       if (w8001->pen_dev)
> -               input_unregister_device(w8001->pen_dev);
> -       if (w8001->touch_dev)
> -               input_unregister_device(w8001->touch_dev);
>         kfree(w8001);
> -
> -       serio_set_drvdata(serio, NULL);
>  }
>
>  /*
> @@ -598,22 +656,17 @@ static void w8001_disconnect(struct serio *serio)
>  static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>  {
>         struct w8001 *w8001;
> -       struct input_dev *input_dev_pen;
> -       struct input_dev *input_dev_touch;
> +       struct w8001_coord coord;
> +       struct w8001_touch_query touch;
>         char basename[64] = "Wacom Serial";
> -       int err, err_pen, err_touch;
> +       bool has_pen, has_touch;
> +       int err;
>
>         w8001 = kzalloc_obj(*w8001);
> -       input_dev_pen = input_allocate_device();
> -       input_dev_touch = input_allocate_device();
> -       if (!w8001 || !input_dev_pen || !input_dev_touch) {
> -               err = -ENOMEM;
> -               goto fail1;
> -       }
> +       if (!w8001)
> +               return -ENOMEM;
>
>         w8001->serio = serio;
> -       w8001->pen_dev = input_dev_pen;
> -       w8001->touch_dev = input_dev_touch;
>         mutex_init(&w8001->mutex);
>         init_completion(&w8001->cmd_done);
>         snprintf(w8001->phys, sizeof(w8001->phys), "%s/input0", serio->phys);
> @@ -621,66 +674,44 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>         serio_set_drvdata(serio, w8001);
>         err = serio_open(serio, drv);
>         if (err)
> -               goto fail2;
> +               goto err_free_w8001;
>
> -       err = w8001_detect(w8001);
> +       err = w8001_command(w8001, W8001_CMD_STOP, false);
>         if (err)
> -               goto fail3;
> +               goto err_close_serio;
> +
> +       msleep(250);    /* wait 250ms before querying the device */
>
> -       /* For backwards-compatibility we compose the basename based on
> +       /*
> +        * For backwards-compatibility we compose the basename based on
>          * capabilities and then just append the tool type
>          */
> -       err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
> -       err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
> -       if (err_pen && err_touch) {
> +       has_pen = w8001_detect_pen(w8001, basename, sizeof(basename), &coord);
> +       has_touch = w8001_detect_touch(w8001, basename, sizeof(basename), &touch);
> +       if (!has_pen && !has_touch) {
>                 err = -ENXIO;
> -               goto fail3;
> +               goto err_close_serio;
>         }
>
> -       if (!err_pen) {
> -               snprintf(w8001->pen_name, sizeof(w8001->pen_name),
> -                        "%s Pen", basename);
> -               input_dev_pen->name = w8001->pen_name;
> -
> -               w8001_set_devdata(input_dev_pen, w8001, serio);
> -
> -               err = input_register_device(w8001->pen_dev);
> +       if (has_pen) {
> +               err = w8001_setup_pen(w8001, basename, &coord);
>                 if (err)
> -                       goto fail3;
> -       } else {
> -               input_free_device(input_dev_pen);
> -               input_dev_pen = NULL;
> -               w8001->pen_dev = NULL;
> +                       goto err_close_serio;
>         }
>
> -       if (!err_touch) {
> -               snprintf(w8001->touch_name, sizeof(w8001->touch_name),
> -                        "%s Finger", basename);
> -               input_dev_touch->name = w8001->touch_name;
> -
> -               w8001_set_devdata(input_dev_touch, w8001, serio);
> -
> -               err = input_register_device(w8001->touch_dev);
> +       if (has_touch) {
> +               err = w8001_setup_touch(w8001, basename, &touch);
>                 if (err)
> -                       goto fail4;
> -       } else {
> -               input_free_device(input_dev_touch);
> -               input_dev_touch = NULL;
> -               w8001->touch_dev = NULL;
> +                       goto err_close_serio;
>         }
>
>         return 0;
>
> -fail4:
> -       if (w8001->pen_dev)
> -               input_unregister_device(w8001->pen_dev);
> -fail3:
> +err_close_serio:
> +       w8001_unregister_devices(w8001);
>         serio_close(serio);
> -fail2:
> +err_free_w8001:
>         serio_set_drvdata(serio, NULL);
> -fail1:
> -       input_free_device(input_dev_pen);
> -       input_free_device(input_dev_touch);
>         kfree(w8001);
>         return err;
>  }
> --
> 2.56.0.385.gd3acb90ef8-goog
>
>

Re: [PATCH 1/3] Input: wacom_w8001 - fix teardown order and device lifetime

[Dmitry Torokhov]

Hi Jason,

On Tue, Oct 13, 2026 at 03:41:53PM -0700, Jason Gerecke wrote:
> On Fri, Oct 9, 2026 at 12:25 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > In w8001_disconnect(), serio_close() was called before unregistering
> > the input devices. When input_unregister_device() calls w8001_close()
> > to shut down the hardware, w8001_command() fails because the serio
> > port is already closed, preventing W8001_CMD_STOP from reaching the
> > controller.
> >
> > Additionally, w8001_connect() pre-allocated both pen_dev and touch_dev
> > upfront before serio_open() so w8001_interrupt() could dereference
> > them unconditionally, and then freed unused input devices mid-probe
> > without synchronizing with w8001_interrupt() when pen or touch was
> > absent. That created a race and use-after-free hazard with
> > w8001_interrupt(), as well as a double-free on the fail4 error path.
> >
> > Fix these issues by guarding event reporting in w8001_interrupt()
> > against NULL device pointers, allocating and registering input devices
> > on demand after querying capabilities, synchronizing device pointer
> > publication and teardown with serio_pause_rx, and unregistering input
> > devices before closing the serio port in both disconnect and probe
> > error unwind paths.
> >
> 
> This patch seems to be pretty complicated for a 'Fixes' and I'd feel a
> lot more comfortable reviewing it if it could be broken into smaller
> chunks. I know that might not be possible, but it's pretty hard to
> audit all the changes here. There's nothing that sets off loud alarms,
> but I think at this level of complexity the best I could do is Ack the
> patch...

Sorry, I do not think it makes sense to split it further...

> 
> > Fixes: 66fd9385ee9c ("Input: wacom_w8001 - simplify w8001_remove")
> > Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")
> > Reported-by: sashiko-bot@kernel.org
> > Assisted-by: LLM
> 
> Just a note that current recommendations are to include the agent name
> and model version in Assisted-by lines:
> 
> https://elixir.bootlin.com/linux/v7.2.9/source/Documentation/process/coding-assistants.rst#L39

Not anymore since 816d9992d9ed ("coding-assistants: simplify
attribution")

The current format is:

	Assisted-by: LLM [TOOL1] [TOOL2]

as in

	Assisted-by: LLM coccinelle sparse

> > @@ -349,7 +353,9 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> >                  * sends invalid data packets that are larger than
> >                  * W8001_PKTLEN_TPCPEN. Let's start over again.
> >                  */
> > -               if (!w8001->touch_dev && w8001->idx > W8001_PKTLEN_TPCPEN - 1)
> > +               if (!w8001->touch_dev &&
> > +                   (w8001->data[0] & W8001_TAB_MASK) != W8001_TAB_BYTE &&
> > +                   w8001->idx > W8001_PKTLEN_TPCPEN - 1)
> 
> I'm not an expert in the w8001 protocol and don't have docs handy at
> the moment. Can you explain the purpose of adding this new
> W8001_TAB_BYTE check? I have a vague idea that it might have something
> to do with allowing the query response packets to be processed prior
> to pen/touch being initialized?

Yes, that's right.

Previously, w8001->touch_dev was pre-allocated before querying the
hardware, so !w8001->touch_dev was false during probing. Now that
touch_dev is allocated on demand after the queries complete,
w8001->touch_dev is NULL during probing, and without the W8001_TAB_BYTE
check we'd be junking query responses (which are longer than
W8001_PKTLEN_TPCPEN).


Thanks.

-- 
Dmitry