Re: [PATCH v2] HID: wacom: Update last_slot_field during pre_report phase

[Jason Gerecke <killertofu@gmail.com>]

Making sure this patch doesn't fall into the cracks...

Jason
---
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....

On 07/25/2016 02:38 AM, Benjamin Tissoires wrote:
> On Jul 21 2016 or thereabouts, Jason Gerecke wrote:
>> If a touchscreen contains both multitouch and single-touch reports in its
>> descriptor in that order, the driver may overwrite information it saved
>> about the format of the multitouch report. This can cause the report
>> processing code to get tripped up and send an incorrect event stream to
>> userspace.
>>
>> In particular, this can cause last_slot_field to be overwritten with the
>> result that the driver prematurely assumes it has finished processing a
>> slot and sending the ABS_MT_SLOT event at the wrong point in time,
>> associating events for the current contact with the following contact
>> instead.
>>
>> To prevent this from occurring, we update the value of last_slot_field
>> durring the pre_report phase to ensure that it is correct for the report
>> that is to be processed.
>>
>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> ---
>> Changes from v1:
>>  * The v1 patch cut off processing by wacom_wac_finger_usage_mapping once
>>    it saw a HID_DG_CONTACTCOUNT usage in any report. This method of
>>    handling the bug has two potential problems: 1) reports which place
>>    the HID_DG_CONTACTCOUNT usage near the beginning of the packet will
>>    not properly map the rest of the usages, and 2) devices which have
>>    multiple reports with the HID_DG_CONTACTCOUNT usage will only send
>>    reports formatted like the first discovered. Neither of these should
>>    cause issues for currently available hardware, but to maximize forward
>>    compatibility, the revised scheme contained here was devised.
> 
> Thanks for the fix. The overall difference between before and after the
> patch seems null (few variable allocations), so:
> 
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Cheers,
> Benjamin
> 
>>
>>  drivers/hid/wacom_wac.c | 62 +++++++++++++++++++++----------------------------
>>  drivers/hid/wacom_wac.h |  2 +-
>>  2 files changed, 27 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index fcf2264..d2611f3 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -1556,13 +1556,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>>  {
>>  	struct wacom *wacom = hid_get_drvdata(hdev);
>>  	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> -	struct wacom_features *features = &wacom_wac->features;
>>  	struct input_dev *input = wacom_wac->touch_input;
>>  	unsigned touch_max = wacom_wac->features.touch_max;
>>  
>>  	switch (usage->hid) {
>>  	case HID_GD_X:
>> -		features->last_slot_field = usage->hid;
>>  		if (touch_max == 1)
>>  			wacom_map_usage(input, usage, field, EV_ABS, ABS_X, 4);
>>  		else
>> @@ -1570,7 +1568,6 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>>  					ABS_MT_POSITION_X, 4);
>>  		break;
>>  	case HID_GD_Y:
>> -		features->last_slot_field = usage->hid;
>>  		if (touch_max == 1)
>>  			wacom_map_usage(input, usage, field, EV_ABS, ABS_Y, 4);
>>  		else
>> @@ -1579,22 +1576,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>>  		break;
>>  	case HID_DG_WIDTH:
>>  	case HID_DG_HEIGHT:
>> -		features->last_slot_field = usage->hid;
>>  		wacom_map_usage(input, usage, field, EV_ABS, ABS_MT_TOUCH_MAJOR, 0);
>>  		wacom_map_usage(input, usage, field, EV_ABS, ABS_MT_TOUCH_MINOR, 0);
>>  		input_set_abs_params(input, ABS_MT_ORIENTATION, 0, 1, 0, 0);
>>  		break;
>> -	case HID_DG_CONTACTID:
>> -		features->last_slot_field = usage->hid;
>> -		break;
>> -	case HID_DG_INRANGE:
>> -		features->last_slot_field = usage->hid;
>> -		break;
>> -	case HID_DG_INVERT:
>> -		features->last_slot_field = usage->hid;
>> -		break;
>>  	case HID_DG_TIPSWITCH:
>> -		features->last_slot_field = usage->hid;
>>  		wacom_map_usage(input, usage, field, EV_KEY, BTN_TOUCH, 0);
>>  		break;
>>  	case HID_DG_CONTACTCOUNT:
>> @@ -1672,7 +1658,7 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
>>  
>>  
>>  	if (usage->usage_index + 1 == field->report_count) {
>> -		if (usage->hid == wacom_wac->features.last_slot_field)
>> +		if (usage->hid == wacom_wac->hid_data.last_slot_field)
>>  			wacom_wac_finger_slot(wacom_wac, wacom_wac->touch_input);
>>  	}
>>  
>> @@ -1685,31 +1671,35 @@ static void wacom_wac_finger_pre_report(struct hid_device *hdev,
>>  	struct wacom *wacom = hid_get_drvdata(hdev);
>>  	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>>  	struct hid_data* hid_data = &wacom_wac->hid_data;
>> +	int i;
>>  
>> -	if (hid_data->cc_report != 0 &&
>> -	    hid_data->cc_report != report->id) {
>> -		int i;
>> -
>> -		hid_data->cc_report = report->id;
>> -		hid_data->cc_index = -1;
>> -		hid_data->cc_value_index = -1;
>> -
>> -		for (i = 0; i < report->maxfield; i++) {
>> -			struct hid_field *field = report->field[i];
>> -			int j;
>> -
>> -			for (j = 0; j < field->maxusage; j++) {
>> -				if (field->usage[j].hid == HID_DG_CONTACTCOUNT) {
>> -					hid_data->cc_index = i;
>> -					hid_data->cc_value_index = j;
>> -
>> -					/* break */
>> -					i = report->maxfield;
>> -					j = field->maxusage;
>> -				}
>> +	for (i = 0; i < report->maxfield; i++) {
>> +		struct hid_field *field = report->field[i];
>> +		int j;
>> +
>> +		for (j = 0; j < field->maxusage; j++) {
>> +			struct hid_usage *usage = &field->usage[j];
>> +
>> +			switch (usage->hid) {
>> +			case HID_GD_X:
>> +			case HID_GD_Y:
>> +			case HID_DG_WIDTH:
>> +			case HID_DG_HEIGHT:
>> +			case HID_DG_CONTACTID:
>> +			case HID_DG_INRANGE:
>> +			case HID_DG_INVERT:
>> +			case HID_DG_TIPSWITCH:
>> +				hid_data->last_slot_field = usage->hid;
>> +				break;
>> +			case HID_DG_CONTACTCOUNT:
>> +				hid_data->cc_report = report->id;
>> +				hid_data->cc_index = i;
>> +				hid_data->cc_value_index = j;
>> +				break;
>>  			}
>>  		}
>>  	}
>> +
>>  	if (hid_data->cc_report != 0 &&
>>  	    hid_data->cc_index >= 0) {
>>  		struct hid_field *field = report->field[hid_data->cc_index];
>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>> index 8a8974c..267025c 100644
>> --- a/drivers/hid/wacom_wac.h
>> +++ b/drivers/hid/wacom_wac.h
>> @@ -185,7 +185,6 @@ struct wacom_features {
>>  	int pktlen;
>>  	bool check_for_hid_type;
>>  	int hid_type;
>> -	int last_slot_field;
>>  };
>>  
>>  struct wacom_shared {
>> @@ -214,6 +213,7 @@ struct hid_data {
>>  	int cc_report;
>>  	int cc_index;
>>  	int cc_value_index;
>> +	int last_slot_field;
>>  	int num_expected;
>>  	int num_received;
>>  };
>> -- 
>> 2.9.0
>>