Hi,

On 12/13/21 05:56, Dmitry Torokhov wrote:

Hi Hans,

On Sun, Dec 12, 2021 at 01:42:42PM +0100, Hans de Goede wrote:

quoted

Because there is no way to detect if the touchscreen has pen support,
the driver is allocating and registering the input_pen input_dev on
receiving the first pen event.

But this means that the input_dev gets allocated after the request_irq()
call which means that the devm framework will free it before disabling
the irq, leaving a window where the irq handler may run and reference the
free-ed input_dev.

To fix this move the allocation of the input_pen input_dev to before
the request_irq() call, while still only registering it on the first pen
event so that the driver does not advertise pen capability on touchscreens
without it (most goodix touchscreens do not have pen support).

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/goodix.c | 32 ++++++++++++++++++------------
 drivers/input/touchscreen/goodix.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 04baf5a770f5..1ada40fa6de6 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -297,14 +297,14 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 	return -ENOMSG;
 }
 
-static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
+static int goodix_create_pen_input(struct goodix_ts_data *ts)
 {
 	struct device *dev = &ts->client->dev;
 	struct input_dev *input;
 
 	input = devm_input_allocate_device(dev);
 	if (!input)
-		return NULL;
+		return -ENOMEM;
 
 	input_copy_abs(input, ABS_X, ts->input_dev, ABS_MT_POSITION_X);
 	input_copy_abs(input, ABS_Y, ts->input_dev, ABS_MT_POSITION_Y);

@@ -331,23 +331,18 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
 		input->id.product = 0x1001;
 	input->id.version = ts->version;
 
-	if (input_register_device(input) != 0) {
-		input_free_device(input);
-		return NULL;
-	}
-
-	return input;
+	ts->input_pen = input;
+	return 0;
 }
 
 static void goodix_ts_report_pen_down(struct goodix_ts_data *ts, u8 *data)
 {
-	int input_x, input_y, input_w;
+	int input_x, input_y, input_w, error;
 	u8 key_value;
 
-	if (!ts->input_pen) {
-		ts->input_pen = goodix_create_pen_input(ts);
-		if (!ts->input_pen)
-			return;
+	if (!ts->pen_input_registered) {
+		error = input_register_device(ts->input_pen);
+		ts->pen_input_registered = error == 0;

I do not think you want to try and re-register input device if first
registration failed. You probably also don't want to waste time and
return early from here in case of failures.

Ack, I've fixed both for v2 of this patch-series.

Regards,

Hans