[PATCH] HID: multitouch: fix calculation of last slot field in multi-touch reports

[PATCH] HID: multitouch: fix calculation of last slot field in multi-touch reports

[Ben Chan]

According to [1] and also seemingly agreed by [2], the Scan Time usage
(0x0D 0x56) is a report level usage, not a contact level usage.

However, the hid-multitouch driver currently includes HID_DG_SCANTIME
when calculating `td->last_slot_field', which may lead to
mt_complete_slot() being prematurely called in certain cases (e.g. when
each touch input report includes more than one contact and the Scan Time
usage appears before any contact logical collection).

This patch fixes the issue by skipping mt_store_field() on
HID_DG_SCANTIME, similar to how HID_DG_CONTACTCOUNT and
HID_DG_CONTACTMAX are handled.

[1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections#windows-precision-touchpad-input-reports
[2] https://patchwork.kernel.org/patch/1742181/

Fixes: 29cc309d8bf19 ("HID: hid-multitouch: forward MSC_TIMESTAMP")
Signed-off-by: Ben Chan <benchan@chromium.org>
---
 drivers/hid/hid-multitouch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index dad2fbb0e3f8..161551aab496 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -620,13 +620,16 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			hid_map_usage(hi, usage, bit, max,
 				EV_MSC, MSC_TIMESTAMP);
 			input_set_capability(hi->input, EV_MSC, MSC_TIMESTAMP);
-			mt_store_field(usage, td, hi);
 			/* Ignore if indexes are out of bounds. */
 			if (field->index >= field->report->maxfield ||
 			    usage->usage_index >= field->report_count)
 				return 1;
 			td->scantime_index = field->index;
 			td->scantime_val_index = usage->usage_index;
+			/*
+			 * We don't set td->last_slot_field as scan time is
+			 * global to the report.
+			 */
 			return 1;
 		case HID_DG_CONTACTCOUNT:
 			/* Ignore if indexes are out of bounds. */
-- 
2.17.0.921.gf22659ad46-goog

Re: [PATCH] HID: multitouch: fix calculation of last slot field in multi-touch reports

[Dmitry Torokhov]

On Tue, May 29, 2018 at 03:56:55PM -0700, Ben Chan wrote:
> According to [1] and also seemingly agreed by [2], the Scan Time usage
> (0x0D 0x56) is a report level usage, not a contact level usage.
> 
> However, the hid-multitouch driver currently includes HID_DG_SCANTIME
> when calculating `td->last_slot_field', which may lead to
> mt_complete_slot() being prematurely called in certain cases (e.g. when
> each touch input report includes more than one contact and the Scan Time
> usage appears before any contact logical collection).
> 
> This patch fixes the issue by skipping mt_store_field() on
> HID_DG_SCANTIME, similar to how HID_DG_CONTACTCOUNT and
> HID_DG_CONTACTMAX are handled.
> 
> [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections#windows-precision-touchpad-input-reports
> [2] https://patchwork.kernel.org/patch/1742181/
> 
> Fixes: 29cc309d8bf19 ("HID: hid-multitouch: forward MSC_TIMESTAMP")
> Signed-off-by: Ben Chan <benchan@chromium.org>

This looks reasonable to me.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/hid/hid-multitouch.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index dad2fbb0e3f8..161551aab496 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -620,13 +620,16 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			hid_map_usage(hi, usage, bit, max,
>  				EV_MSC, MSC_TIMESTAMP);
>  			input_set_capability(hi->input, EV_MSC, MSC_TIMESTAMP);
> -			mt_store_field(usage, td, hi);
>  			/* Ignore if indexes are out of bounds. */
>  			if (field->index >= field->report->maxfield ||
>  			    usage->usage_index >= field->report_count)
>  				return 1;
>  			td->scantime_index = field->index;
>  			td->scantime_val_index = usage->usage_index;
> +			/*
> +			 * We don't set td->last_slot_field as scan time is
> +			 * global to the report.
> +			 */
>  			return 1;
>  		case HID_DG_CONTACTCOUNT:
>  			/* Ignore if indexes are out of bounds. */
> -- 
> 2.17.0.921.gf22659ad46-goog
> 
> --
> 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

-- 
Dmitry

Re: [PATCH] HID: multitouch: fix calculation of last slot field in multi-touch reports

[Jiri Kosina]

On Tue, 29 May 2018, Ben Chan wrote:

> According to [1] and also seemingly agreed by [2], the Scan Time usage
> (0x0D 0x56) is a report level usage, not a contact level usage.
> 
> However, the hid-multitouch driver currently includes HID_DG_SCANTIME
> when calculating `td->last_slot_field', which may lead to
> mt_complete_slot() being prematurely called in certain cases (e.g. when
> each touch input report includes more than one contact and the Scan Time
> usage appears before any contact logical collection).
> 
> This patch fixes the issue by skipping mt_store_field() on
> HID_DG_SCANTIME, similar to how HID_DG_CONTACTCOUNT and
> HID_DG_CONTACTMAX are handled.
> 
> [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-required-hid-top-level-collections#windows-precision-touchpad-input-reports
> [2] https://patchwork.kernel.org/patch/1742181/
> 
> Fixes: 29cc309d8bf19 ("HID: hid-multitouch: forward MSC_TIMESTAMP")
> Signed-off-by: Ben Chan <benchan@chromium.org>

Applied, thanks Ben.

-- 
Jiri Kosina
SUSE Labs