Hi Hans,
On Sun, Dec 12, 2021 at 01:42:42PM +0100, Hans de Goede wrote:
> 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 <hdegoede@xxxxxxxxxx>
> ---
> 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.
Thanks.
--
Dmitry
