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:
>> 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.
Ack, I've fixed both for v2 of this patch-series.
Regards,
Hans
