Re: [PATCH 5/5] Input: goodix - Fix race on driver unbind

[Hans de Goede <hdegoede@redhat.com>]

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@redhat.com>
>> ---
>>  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