[PATCH] HID: wacom: Stop mapping touch usages after processing HID_DG_CONTACTCOUNT

[PATCH] HID: wacom: Stop mapping touch usages after processing HID_DG_CONTACTCOUNT

[Jason Gerecke]

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 occuring, we stop mapping usages after having seen
HID_DG_CONTACTCOUNT. This usage is only present in multitouch reports,
so the format of any following single-touch reports will have no effect.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index fcf2264..e499cdb 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1560,6 +1560,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
 	struct input_dev *input = wacom_wac->touch_input;
 	unsigned touch_max = wacom_wac->features.touch_max;
 
+	/* stop processing after the first multitouch report */
+	if (wacom_wac->hid_data.cc_report &&
+	    wacom_wac->hid_data.cc_report != field->report->id)
+		return;
+
 	switch (usage->hid) {
 	case HID_GD_X:
 		features->last_slot_field = usage->hid;
-- 
2.9.0

Re: [PATCH] HID: wacom: Stop mapping touch usages after processing HID_DG_CONTACTCOUNT

[Benjamin Tissoires]

Hi Jason,

On Jul 11 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 occuring, we stop mapping usages after having seen
> HID_DG_CONTACTCOUNT. This usage is only present in multitouch reports,
> so the format of any following single-touch reports will have no effect.

I had a quick look at the driver, and it looks like the Cintiq Companion
2 has more than one multitouch collection (see 499522c "HID: wacom: Tie
cached HID_DG_CONTACTCOUNT indices to report ID").

So if this doesn't break the cintiq companion 2, you have my
reviewed-by, but I'd rather be sure (and please see if we really need to
keep 499522c then).

Cheers,
Benjamin

> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/wacom_wac.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index fcf2264..e499cdb 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1560,6 +1560,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>  	struct input_dev *input = wacom_wac->touch_input;
>  	unsigned touch_max = wacom_wac->features.touch_max;
>  
> +	/* stop processing after the first multitouch report */
> +	if (wacom_wac->hid_data.cc_report &&
> +	    wacom_wac->hid_data.cc_report != field->report->id)
> +		return;
> +
>  	switch (usage->hid) {
>  	case HID_GD_X:
>  		features->last_slot_field = usage->hid;
> -- 
> 2.9.0
> 

Re: [PATCH] HID: wacom: Stop mapping touch usages after processing HID_DG_CONTACTCOUNT

[Jason Gerecke]

On 07/12/2016 12:32 AM, Benjamin Tissoires wrote:
> Hi Jason,
> 
> On Jul 11 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 occuring, we stop mapping usages after having seen
>> HID_DG_CONTACTCOUNT. This usage is only present in multitouch reports,
>> so the format of any following single-touch reports will have no effect.
> 
> I had a quick look at the driver, and it looks like the Cintiq Companion
> 2 has more than one multitouch collection (see 499522c "HID: wacom: Tie
> cached HID_DG_CONTACTCOUNT indices to report ID").
> 
> So if this doesn't break the cintiq companion 2, you have my
> reviewed-by, but I'd rather be sure (and please see if we really need to
> keep 499522c then).
> 
> Cheers,
> Benjamin
> 

Thanks for the reminder about 499522c. This patch should supersede it in
terms of functionality, so I can make a patch to remove it instead.

As far as compatibility goes, I don't have a Companion 2 to actually
test, but looking at the descriptor [1], it shouldn't pose any problems:
the second report containing HID_DG_CONTACTCOUNT is the single-touch
report that we don't want to use anyway.

[1]:
https://github.com/linuxwacom/wacom-hid-descriptors/blob/master/Wacom%20Cintiq%20Companion%202/0003:056A:0326.0002.hid.txt

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

>>
>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> ---
>>  drivers/hid/wacom_wac.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index fcf2264..e499cdb 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -1560,6 +1560,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>>  	struct input_dev *input = wacom_wac->touch_input;
>>  	unsigned touch_max = wacom_wac->features.touch_max;
>>  
>> +	/* stop processing after the first multitouch report */
>> +	if (wacom_wac->hid_data.cc_report &&
>> +	    wacom_wac->hid_data.cc_report != field->report->id)
>> +		return;
>> +
>>  	switch (usage->hid) {
>>  	case HID_GD_X:
>>  		features->last_slot_field = usage->hid;
>> -- 
>> 2.9.0
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: wacom: Stop mapping touch usages after processing HID_DG_CONTACTCOUNT

[Jason Gerecke]

On 07/20/2016 09:36 AM, Jason Gerecke wrote:
> On 07/12/2016 12:32 AM, Benjamin Tissoires wrote:
>> Hi Jason,
>>
>> On Jul 11 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 occuring, we stop mapping usages after having seen
>>> HID_DG_CONTACTCOUNT. This usage is only present in multitouch reports,
>>> so the format of any following single-touch reports will have no effect.
>>
>> I had a quick look at the driver, and it looks like the Cintiq Companion
>> 2 has more than one multitouch collection (see 499522c "HID: wacom: Tie
>> cached HID_DG_CONTACTCOUNT indices to report ID").
>>
>> So if this doesn't break the cintiq companion 2, you have my
>> reviewed-by, but I'd rather be sure (and please see if we really need to
>> keep 499522c then).
>>
>> Cheers,
>> Benjamin
>>
> 
> Thanks for the reminder about 499522c. This patch should supersede it in
> terms of functionality, so I can make a patch to remove it instead.
> 
> As far as compatibility goes, I don't have a Companion 2 to actually
> test, but looking at the descriptor [1], it shouldn't pose any problems:
> the second report containing HID_DG_CONTACTCOUNT is the single-touch
> report that we don't want to use anyway.
> 
> [1]:
> https://github.com/linuxwacom/wacom-hid-descriptors/blob/master/Wacom%20Cintiq%20Companion%202/0003:056A:0326.0002.hid.txt
> 
> 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 closer inspection, it's probably not right to say that this patch
supersedes 499522c. It does remove the need for it, but only under the
(currently valid) assumption that the first report with
HID_DG_CONTACTCOUNT is the report the tablet will send.

If we want to be free of that assumption, it might be a better idea to
replace this patch with something that instead does additional work in
the pre_report phase to update last_slot_field (just like how 499522c
updates cc_index and cc_value_index during pre_report).

Thoughts?

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

>>>
>>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>>> ---
>>>  drivers/hid/wacom_wac.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>>> index fcf2264..e499cdb 100644
>>> --- a/drivers/hid/wacom_wac.c
>>> +++ b/drivers/hid/wacom_wac.c
>>> @@ -1560,6 +1560,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>>>  	struct input_dev *input = wacom_wac->touch_input;
>>>  	unsigned touch_max = wacom_wac->features.touch_max;
>>>  
>>> +	/* stop processing after the first multitouch report */
>>> +	if (wacom_wac->hid_data.cc_report &&
>>> +	    wacom_wac->hid_data.cc_report != field->report->id)
>>> +		return;
>>> +
>>>  	switch (usage->hid) {
>>>  	case HID_GD_X:
>>>  		features->last_slot_field = usage->hid;
>>> -- 
>>> 2.9.0
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jason Gerecke]

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.

 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

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

[Ping Cheng]

On Thu, Jul 21, 2016 at 9:10 AM, Jason Gerecke <killertofu@gmail.com> 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.

Nice job, Jason! I like this approach. The patch is:

Reviewed-by: Ping Cheng <pingc@wacom.com>

Cheers,

Ping

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

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

[Benjamin Tissoires]

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
> 

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

[Jason Gerecke]

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

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

[Jiri Kosina]

On Wed, 10 Aug 2016, Jason Gerecke wrote:

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

Alright, I apparently wasn't CCed; please don't forget to do so on patches 
you want me to apply, otherwise they might easily be missed.

My understanding is that this is rather 4.8 material still, do you agree?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jason Gerecke]

On 08/11/2016 01:27 AM, Jiri Kosina wrote:
> On Wed, 10 Aug 2016, Jason Gerecke wrote:
> 
>> Making sure this patch doesn't fall into the cracks...
> 
> Alright, I apparently wasn't CCed; please don't forget to do so on patches 
> you want me to apply, otherwise they might easily be missed.
> 
> My understanding is that this is rather 4.8 material still, do you agree?
> 
> Thanks,
> 

Yes, it should be fine to target for 4.8.

Thanks,
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....

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

[Jiri Kosina]

On Thu, 21 Jul 2016, 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>

Applied to for-4.8/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs