[PATCH] HID: multitouch: Fetch feature reports on demand for Win8 devices

[PATCH] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Mika Westerberg]

Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
fail when initial feature reports are fetched from it. Below is an example
output with some additional debug included:

 i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
 i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
 ...
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
 i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
 i2c_hid i2c-DLL0704:01: report id 13
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
 i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
 i2c_hid i2c-DLL0704:01: report id 7
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
 i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
 i2c_hid i2c-DLL0704:01: report id 4
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00

We manage to fetch few reports but then the touchpad dies:

 i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
 i2c_hid i2c-DLL0704:01: failed to retrieve report from device.

it eventually pulls the whole I2C bus low:

 i2c_designware i2c_designware.1: controller timed out
 i2c_hid i2c-DLL0704:01: failed to set a report to device.

Fix this by preventing initial feature report retrieval for Win8 devices.
Instead we fetch reports as needed in mt_feature_mapping(). This prevents
fetching reports which might cause problems with the device in question.

Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
This is a second version. The first one which just prevented initial report
reading can be found here:

  https://lkml.org/lkml/2015/9/24/191

This version tries to do what Benjamin suggested by reading feature values
as needed in mt_feature_mapping(). This fixes the issue in the Dell laptop
I have here.

 drivers/hid/hid-multitouch.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 661b4fce1a5d..d9ca2b96ab68 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -309,6 +309,38 @@ static struct attribute_group mt_attribute_group = {
 	.attrs = sysfs_attrs
 };
 
+static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
+{
+	int ret, size = hid_report_len(report);
+	u8 *buf;
+
+	/*
+	 * Only fetch the feature report if initial reports are not already
+	 * been retrieved. Currently this is only done for Windows 8 touch
+	 * devices.
+	 */
+	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS))
+		return;
+
+	buf = hid_alloc_report_buf(report, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ret = hid_hw_raw_request(hdev, report->id, buf, size,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
+			 report->id);
+	} else {
+		ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf,
+					   size, 0);
+		if (ret)
+			dev_warn(&hdev->dev, "failed to report feature\n");
+	}
+
+	kfree(buf);
+}
+
 static void mt_feature_mapping(struct hid_device *hdev,
 		struct hid_field *field, struct hid_usage *usage)
 {
@@ -327,6 +359,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
 
 		break;
 	case HID_DG_CONTACTMAX:
+		mt_get_feature(hdev, field->report);
+
 		td->maxcontact_report_id = field->report->id;
 		td->maxcontacts = field->value[0];
 		if (!td->maxcontacts &&
@@ -343,6 +377,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			break;
 		}
 
+		mt_get_feature(hdev, field->report);
 		if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
 			td->is_buttonpad = true;
 
@@ -1029,8 +1064,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * reports. Fortunately, the Win8 spec says that all touches
 		 * should be sent during each report, making the initialization
 		 * of input reports unnecessary.
+		 *
+		 * In addition some touchpads do not behave well if we read
+		 * all feature reports from them. Instead we prevent
+		 * initial report fetching and then selectively fetch each
+		 * report we are interested in.
 		 */
-		hdev->quirks |= HID_QUIRK_NO_INIT_INPUT_REPORTS;
+		hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
 	td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
-- 
2.5.1

Re: [PATCH] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Benjamin Tissoires]

On Sep 28 2015 or thereabouts, Mika Westerberg wrote:
> Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
> fail when initial feature reports are fetched from it. Below is an example
> output with some additional debug included:
> 
>  i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
>  i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
>  ...
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
>  i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
>  i2c_hid i2c-DLL0704:01: report id 13
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
>  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>  i2c_hid i2c-DLL0704:01: report id 7
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
>  i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
>  i2c_hid i2c-DLL0704:01: report id 4
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00
> 
> We manage to fetch few reports but then the touchpad dies:
> 
>  i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
>  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
> 
> it eventually pulls the whole I2C bus low:
> 
>  i2c_designware i2c_designware.1: controller timed out
>  i2c_hid i2c-DLL0704:01: failed to set a report to device.
> 
> Fix this by preventing initial feature report retrieval for Win8 devices.
> Instead we fetch reports as needed in mt_feature_mapping(). This prevents
> fetching reports which might cause problems with the device in question.
> 
> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> This is a second version. The first one which just prevented initial report
> reading can be found here:
> 
>   https://lkml.org/lkml/2015/9/24/191
> 
> This version tries to do what Benjamin suggested by reading feature values
> as needed in mt_feature_mapping(). This fixes the issue in the Dell laptop
> I have here.

Thanks for the re-spin Mika.

I just have one comment that needs discussion:

> 
>  drivers/hid/hid-multitouch.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 661b4fce1a5d..d9ca2b96ab68 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -309,6 +309,38 @@ static struct attribute_group mt_attribute_group = {
>  	.attrs = sysfs_attrs
>  };
>  
> +static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
> +{
> +	int ret, size = hid_report_len(report);
> +	u8 *buf;
> +
> +	/*
> +	 * Only fetch the feature report if initial reports are not already
> +	 * been retrieved. Currently this is only done for Windows 8 touch
> +	 * devices.
> +	 */
> +	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS))
> +		return;

Some Win 7 panels have this quirk set too because they do not support
either querying random feature or input reports.

I think this test is safe given that this is, I think, what the Windows
driver does (I remember that it queries for the features it needs).
However I am not 100% sure if this will not break one low quality
touchscreen.

I would be in favor of applying the patch in its current shape, but if
Jiri thinks we need a little bit more caution here, we would need to add
a test regarding the Win8 capability here.

Jiri?

If we apply it, this patch is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> +
> +	buf = hid_alloc_report_buf(report, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	ret = hid_hw_raw_request(hdev, report->id, buf, size,
> +				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +	if (ret < 0) {
> +		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
> +			 report->id);
> +	} else {
> +		ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf,
> +					   size, 0);
> +		if (ret)
> +			dev_warn(&hdev->dev, "failed to report feature\n");
> +	}
> +
> +	kfree(buf);
> +}
> +
>  static void mt_feature_mapping(struct hid_device *hdev,
>  		struct hid_field *field, struct hid_usage *usage)
>  {
> @@ -327,6 +359,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  
>  		break;
>  	case HID_DG_CONTACTMAX:
> +		mt_get_feature(hdev, field->report);
> +
>  		td->maxcontact_report_id = field->report->id;
>  		td->maxcontacts = field->value[0];
>  		if (!td->maxcontacts &&
> @@ -343,6 +377,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  			break;
>  		}
>  
> +		mt_get_feature(hdev, field->report);
>  		if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
>  			td->is_buttonpad = true;
>  
> @@ -1029,8 +1064,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		 * reports. Fortunately, the Win8 spec says that all touches
>  		 * should be sent during each report, making the initialization
>  		 * of input reports unnecessary.
> +		 *
> +		 * In addition some touchpads do not behave well if we read
> +		 * all feature reports from them. Instead we prevent
> +		 * initial report fetching and then selectively fetch each
> +		 * report we are interested in.
>  		 */
> -		hdev->quirks |= HID_QUIRK_NO_INIT_INPUT_REPORTS;
> +		hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
>  
>  	td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
>  	if (!td) {
> -- 
> 2.5.1
> 

Re: [PATCH] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Mika Westerberg]

On Tue, Sep 29, 2015 at 05:04:31AM -0400, Benjamin Tissoires wrote:
> > +static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
> > +{
> > +	int ret, size = hid_report_len(report);
> > +	u8 *buf;
> > +
> > +	/*
> > +	 * Only fetch the feature report if initial reports are not already
> > +	 * been retrieved. Currently this is only done for Windows 8 touch
> > +	 * devices.
> > +	 */
> > +	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS))
> > +		return;
> 
> Some Win 7 panels have this quirk set too because they do not support
> either querying random feature or input reports.
> 
> I think this test is safe given that this is, I think, what the Windows
> driver does (I remember that it queries for the features it needs).
> However I am not 100% sure if this will not break one low quality
> touchscreen.
> 
> I would be in favor of applying the patch in its current shape, but if
> Jiri thinks we need a little bit more caution here, we would need to add
> a test regarding the Win8 capability here.

What about adding following on top of this patch? It checks the device
in question belongs to "win8 class".

I can send a new version of the patch if this seems reasonable.

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index d9ca2b96ab68..72c55a3f650f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -311,6 +311,7 @@ static struct attribute_group mt_attribute_group = {
 
 static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
 {
+	struct mt_device *td = hid_get_drvdata(hdev);
 	int ret, size = hid_report_len(report);
 	u8 *buf;
 
@@ -319,7 +320,8 @@ static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
 	 * been retrieved. Currently this is only done for Windows 8 touch
 	 * devices.
 	 */
-	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS))
+	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS) ||
+	    td->mtclass.name != MT_CLS_WIN_8)
 		return;
 
 	buf = hid_alloc_report_buf(report, GFP_KERNEL);

Re: [PATCH] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Jiri Kosina]

On Wed, 30 Sep 2015, Mika Westerberg wrote:

> > I would be in favor of applying the patch in its current shape, but if
> > Jiri thinks we need a little bit more caution here, we would need to add
> > a test regarding the Win8 capability here.
> 
> What about adding following on top of this patch? It checks the device
> in question belongs to "win8 class".
> 
> I can send a new version of the patch if this seems reasonable.

If this actually maintains the backwards compatibility for that particular 
device Benjamin mentioned (no idea what mtclass does it have?), I'd be in 
favor of doing that.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Mika Westerberg]

Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
fail when initial feature reports are fetched from it. Below is an example
output with some additional debug included:

 i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
 i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
 ...
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
 i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
 i2c_hid i2c-DLL0704:01: report id 13
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
 i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
 i2c_hid i2c-DLL0704:01: report id 7
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
 i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
 i2c_hid i2c-DLL0704:01: report id 4
 i2c_hid i2c-DLL0704:01: i2c_hid_get_report
 i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00

We manage to fetch few reports but then the touchpad dies:

 i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
 i2c_hid i2c-DLL0704:01: failed to retrieve report from device.

it eventually pulls the whole I2C bus low:

 i2c_designware i2c_designware.1: controller timed out
 i2c_hid i2c-DLL0704:01: failed to set a report to device.

Fix this by preventing initial feature report retrieval for Win8 devices.
Instead we fetch reports as needed in mt_feature_mapping(). This prevents
fetching reports which might cause problems with the device in question.

Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Added check for MT_CLS_WIN_8 so that the fix is only done for real Win8
devices.

 drivers/hid/hid-multitouch.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 661b4fce1a5d..dd64461e5e85 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -309,6 +309,41 @@ static struct attribute_group mt_attribute_group = {
 	.attrs = sysfs_attrs
 };
 
+static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	int ret, size = hid_report_len(report);
+	u8 *buf;
+
+	/*
+	 * Only fetch the feature report if initial reports are not already
+	 * been retrieved. Currently this is only done for Windows 8 touch
+	 * devices.
+	 */
+	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS))
+		return;
+	if (td->mtclass.name != MT_CLS_WIN_8)
+		return;
+
+	buf = hid_alloc_report_buf(report, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ret = hid_hw_raw_request(hdev, report->id, buf, size,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
+			 report->id);
+	} else {
+		ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf,
+					   size, 0);
+		if (ret)
+			dev_warn(&hdev->dev, "failed to report feature\n");
+	}
+
+	kfree(buf);
+}
+
 static void mt_feature_mapping(struct hid_device *hdev,
 		struct hid_field *field, struct hid_usage *usage)
 {
@@ -327,6 +362,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
 
 		break;
 	case HID_DG_CONTACTMAX:
+		mt_get_feature(hdev, field->report);
+
 		td->maxcontact_report_id = field->report->id;
 		td->maxcontacts = field->value[0];
 		if (!td->maxcontacts &&
@@ -343,6 +380,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			break;
 		}
 
+		mt_get_feature(hdev, field->report);
 		if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
 			td->is_buttonpad = true;
 
@@ -1029,8 +1067,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * reports. Fortunately, the Win8 spec says that all touches
 		 * should be sent during each report, making the initialization
 		 * of input reports unnecessary.
+		 *
+		 * In addition some touchpads do not behave well if we read
+		 * all feature reports from them. Instead we prevent
+		 * initial report fetching and then selectively fetch each
+		 * report we are interested in.
 		 */
-		hdev->quirks |= HID_QUIRK_NO_INIT_INPUT_REPORTS;
+		hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
 	td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
-- 
2.5.3

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Benjamin Tissoires]

[sorry for the late answer Jiri, I was on vacations last week]

On Oct 07 2015 or thereabouts, Mika Westerberg wrote:
> Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
> fail when initial feature reports are fetched from it. Below is an example
> output with some additional debug included:
> 
>  i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
>  i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
>  ...
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
>  i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
>  i2c_hid i2c-DLL0704:01: report id 13
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
>  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>  i2c_hid i2c-DLL0704:01: report id 7
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
>  i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
>  i2c_hid i2c-DLL0704:01: report id 4
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00
> 
> We manage to fetch few reports but then the touchpad dies:
> 
>  i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
>  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
> 
> it eventually pulls the whole I2C bus low:
> 
>  i2c_designware i2c_designware.1: controller timed out
>  i2c_hid i2c-DLL0704:01: failed to set a report to device.
> 
> Fix this by preventing initial feature report retrieval for Win8 devices.
> Instead we fetch reports as needed in mt_feature_mapping(). This prevents
> fetching reports which might cause problems with the device in question.
> 
> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Added check for MT_CLS_WIN_8 so that the fix is only done for real Win8
> devices.

I think we are good now. I would just like to have Seth or Andrew giving 
a tested-by for Windows precision touchpads that can be found on the Dell
laptops.

If the buttonpad property is set correctly, this is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 
>  drivers/hid/hid-multitouch.c | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 661b4fce1a5d..dd64461e5e85 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -309,6 +309,41 @@ static struct attribute_group mt_attribute_group = {
>  	.attrs = sysfs_attrs
>  };
>  
> +static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	int ret, size = hid_report_len(report);
> +	u8 *buf;
> +
> +	/*
> +	 * Only fetch the feature report if initial reports are not already
> +	 * been retrieved. Currently this is only done for Windows 8 touch
> +	 * devices.
> +	 */
> +	if (!(hdev->quirks & HID_QUIRK_NO_INIT_REPORTS))
> +		return;
> +	if (td->mtclass.name != MT_CLS_WIN_8)
> +		return;
> +
> +	buf = hid_alloc_report_buf(report, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	ret = hid_hw_raw_request(hdev, report->id, buf, size,
> +				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +	if (ret < 0) {
> +		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
> +			 report->id);
> +	} else {
> +		ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf,
> +					   size, 0);
> +		if (ret)
> +			dev_warn(&hdev->dev, "failed to report feature\n");
> +	}
> +
> +	kfree(buf);
> +}
> +
>  static void mt_feature_mapping(struct hid_device *hdev,
>  		struct hid_field *field, struct hid_usage *usage)
>  {
> @@ -327,6 +362,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  
>  		break;
>  	case HID_DG_CONTACTMAX:
> +		mt_get_feature(hdev, field->report);
> +
>  		td->maxcontact_report_id = field->report->id;
>  		td->maxcontacts = field->value[0];
>  		if (!td->maxcontacts &&
> @@ -343,6 +380,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  			break;
>  		}
>  
> +		mt_get_feature(hdev, field->report);
>  		if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
>  			td->is_buttonpad = true;
>  
> @@ -1029,8 +1067,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		 * reports. Fortunately, the Win8 spec says that all touches
>  		 * should be sent during each report, making the initialization
>  		 * of input reports unnecessary.
> +		 *
> +		 * In addition some touchpads do not behave well if we read
> +		 * all feature reports from them. Instead we prevent
> +		 * initial report fetching and then selectively fetch each
> +		 * report we are interested in.
>  		 */
> -		hdev->quirks |= HID_QUIRK_NO_INIT_INPUT_REPORTS;
> +		hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
>  
>  	td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
>  	if (!td) {
> -- 
> 2.5.3
> 

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Seth Forshee]

On Wed, Oct 07, 2015 at 09:34:10AM -0400, Benjamin Tissoires wrote:
> [sorry for the late answer Jiri, I was on vacations last week]
> 
> On Oct 07 2015 or thereabouts, Mika Westerberg wrote:
> > Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
> > fail when initial feature reports are fetched from it. Below is an example
> > output with some additional debug included:
> > 
> >  i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
> >  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
> >  i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
> >  ...
> >  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
> >  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
> >  i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
> >  i2c_hid i2c-DLL0704:01: report id 13
> >  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
> >  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
> >  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
> >  i2c_hid i2c-DLL0704:01: report id 7
> >  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
> >  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
> >  i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
> >  i2c_hid i2c-DLL0704:01: report id 4
> >  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
> >  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00
> > 
> > We manage to fetch few reports but then the touchpad dies:
> > 
> >  i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
> >  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
> > 
> > it eventually pulls the whole I2C bus low:
> > 
> >  i2c_designware i2c_designware.1: controller timed out
> >  i2c_hid i2c-DLL0704:01: failed to set a report to device.
> > 
> > Fix this by preventing initial feature report retrieval for Win8 devices.
> > Instead we fetch reports as needed in mt_feature_mapping(). This prevents
> > fetching reports which might cause problems with the device in question.
> > 
> > Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Added check for MT_CLS_WIN_8 so that the fix is only done for real Win8
> > devices.
> 
> I think we are good now. I would just like to have Seth or Andrew giving 
> a tested-by for Windows precision touchpads that can be found on the Dell
> laptops.
> 
> If the buttonpad property is set correctly, this is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

This is working fine on my XPS 13 9343.

Tested-by: Seth Forshee <seth.forshee@canonical.com>

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Andrew Duggan]

On 10/07/2015 08:56 AM, Seth Forshee wrote:
> On Wed, Oct 07, 2015 at 09:34:10AM -0400, Benjamin Tissoires wrote:
>> [sorry for the late answer Jiri, I was on vacations last week]
>>
>> On Oct 07 2015 or thereabouts, Mika Westerberg wrote:
>>> Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
>>> fail when initial feature reports are fetched from it. Below is an example
>>> output with some additional debug included:
>>>
>>>   i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
>>>   i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
>>>   ...
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
>>>   i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
>>>   i2c_hid i2c-DLL0704:01: report id 13
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
>>>   i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>>>   i2c_hid i2c-DLL0704:01: report id 7
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
>>>   i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
>>>   i2c_hid i2c-DLL0704:01: report id 4
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00
>>>
>>> We manage to fetch few reports but then the touchpad dies:
>>>
>>>   i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
>>>   i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>>>
>>> it eventually pulls the whole I2C bus low:
>>>
>>>   i2c_designware i2c_designware.1: controller timed out
>>>   i2c_hid i2c-DLL0704:01: failed to set a report to device.
>>>
>>> Fix this by preventing initial feature report retrieval for Win8 devices.
>>> Instead we fetch reports as needed in mt_feature_mapping(). This prevents
>>> fetching reports which might cause problems with the device in question.
>>>
>>> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> ---
>>> Added check for MT_CLS_WIN_8 so that the fix is only done for real Win8
>>> devices.
>> I think we are good now. I would just like to have Seth or Andrew giving
>> a tested-by for Windows precision touchpads that can be found on the Dell
>> laptops.
>>
>> If the buttonpad property is set correctly, this is:
>> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> This is working fine on my XPS 13 9343.
>
> Tested-by: Seth Forshee <seth.forshee@canonical.com>
I can confirm that it is working on the Dell XPS 13 and the buttonpad 
property is set correctly. However, I also tried it on an older non 
production precision touchpad and it didn't work. It reports that it 
fails to fetch feature 8 and then doesn't report any input. I'm 
concerned that some of the early PTPs in some Dell and Acer systems 
might not work with this change. I'll test this tomorrow on a few more 
systems to see if the problem is isolated to just this one non 
production touchpad.

[ 2969.222125] usb 1-1: New USB device found, idVendor=06cb, idProduct=2393
[ 2969.222128] usb 1-1: New USB device strings: Mfr=0, Product=2, 
SerialNumber=0
[ 2969.222130] usb 1-1: Product: Synaptics T Pad V 01.31
[ 2974.223578] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
[ 2979.226037] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
[ 2979.226137] input: Synaptics T Pad V 01.31 UNKNOWN as 
/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/0003:06CB:2393.0008/input/input34
[ 2979.226431] hid-multitouch 0003:06CB:2393.0008: 
input,hiddev0,hidraw2: USB HID v1.11 Mouse [Synaptics T Pad V 01.31] on 
usb-0000:00:14.0-1/input0

Feature 8 looks like this:
   FEATURE(8)[FEATURE]
     Field(0)
       Application(Digitizers.TouchPad)
       Usage(2)
         Digitizers.ContactMaximumNumber
         Digitizers.ButtonType
       Logical Minimum(0)
       Logical Maximum(15)
       Physical Minimum(0)
       Physical Maximum(65535)
       Unit Exponent(-4)
       Unit(SI Linear : Seconds)
       Report Size(4)
       Report Count(2)
       Report Offset(0)
       Flags( Variable Absolute )

Andrew

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Andrew Duggan]

On 10/07/2015 08:56 AM, Seth Forshee wrote:
> On Wed, Oct 07, 2015 at 09:34:10AM -0400, Benjamin Tissoires wrote:
>> [sorry for the late answer Jiri, I was on vacations last week]
>>
>> On Oct 07 2015 or thereabouts, Mika Westerberg wrote:
>>> Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
>>> fail when initial feature reports are fetched from it. Below is an example
>>> output with some additional debug included:
>>>
>>>   i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
>>>   i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
>>>   ...
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
>>>   i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
>>>   i2c_hid i2c-DLL0704:01: report id 13
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
>>>   i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>>>   i2c_hid i2c-DLL0704:01: report id 7
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
>>>   i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
>>>   i2c_hid i2c-DLL0704:01: report id 4
>>>   i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>>>   i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00
>>>
>>> We manage to fetch few reports but then the touchpad dies:
>>>
>>>   i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
>>>   i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>>>
>>> it eventually pulls the whole I2C bus low:
>>>
>>>   i2c_designware i2c_designware.1: controller timed out
>>>   i2c_hid i2c-DLL0704:01: failed to set a report to device.
>>>
>>> Fix this by preventing initial feature report retrieval for Win8 devices.
>>> Instead we fetch reports as needed in mt_feature_mapping(). This prevents
>>> fetching reports which might cause problems with the device in question.
>>>
>>> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> ---
>>> Added check for MT_CLS_WIN_8 so that the fix is only done for real Win8
>>> devices.
>> I think we are good now. I would just like to have Seth or Andrew giving
>> a tested-by for Windows precision touchpads that can be found on the Dell
>> laptops.
>>
>> If the buttonpad property is set correctly, this is:
>> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> This is working fine on my XPS 13 9343.
>
> Tested-by: Seth Forshee <seth.forshee@canonical.com>
I can confirm that it is working on the Dell XPS 13 and the buttonpad 
property is set correctly. However, I also tried it on an older non 
production precision touchpad and it didn't work. It reports that it 
fails to fetch feature 8 and then doesn't report any input. I'm 
concerned that some of the early PTPs in some Dell and Acer systems 
might not work with this change. I'll test this tomorrow on a few more 
systems to see if the problem is isolated to just this one non 
production touchpad.

[ 2969.222125] usb 1-1: New USB device found, idVendor=06cb, idProduct=2393
[ 2969.222128] usb 1-1: New USB device strings: Mfr=0, Product=2, 
SerialNumber=0
[ 2969.222130] usb 1-1: Product: Synaptics T Pad V 01.31
[ 2974.223578] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
[ 2979.226037] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
[ 2979.226137] input: Synaptics T Pad V 01.31 UNKNOWN as 
/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/0003:06CB:2393.0008/input/input34
[ 2979.226431] hid-multitouch 0003:06CB:2393.0008: 
input,hiddev0,hidraw2: USB HID v1.11 Mouse [Synaptics T Pad V 01.31] on 
usb-0000:00:14.0-1/input0

Feature 8 looks like this:
   FEATURE(8)[FEATURE]
     Field(0)
       Application(Digitizers.TouchPad)
       Usage(2)
         Digitizers.ContactMaximumNumber
         Digitizers.ButtonType
       Logical Minimum(0)
       Logical Maximum(15)
       Physical Minimum(0)
       Physical Maximum(65535)
       Unit Exponent(-4)
       Unit(SI Linear : Seconds)
       Report Size(4)
       Report Count(2)
       Report Offset(0)
       Flags( Variable Absolute )

Andrew

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Mika Westerberg]

On Wed, Oct 07, 2015 at 06:12:42PM -0700, Andrew Duggan wrote:
> I can confirm that it is working on the Dell XPS 13 and the buttonpad
> property is set correctly. However, I also tried it on an older non
> production precision touchpad and it didn't work. It reports that it fails
> to fetch feature 8 and then doesn't report any input. I'm concerned that
> some of the early PTPs in some Dell and Acer systems might not work with
> this change. I'll test this tomorrow on a few more systems to see if the
> problem is isolated to just this one non production touchpad.

Thanks for testing.

> [ 2969.222125] usb 1-1: New USB device found, idVendor=06cb, idProduct=2393
> [ 2969.222128] usb 1-1: New USB device strings: Mfr=0, Product=2,
> SerialNumber=0
> [ 2969.222130] usb 1-1: Product: Synaptics T Pad V 01.31
> [ 2974.223578] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
> [ 2979.226037] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
> [ 2979.226137] input: Synaptics T Pad V 01.31 UNKNOWN as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/0003:06CB:2393.0008/input/input34
> [ 2979.226431] hid-multitouch 0003:06CB:2393.0008: input,hiddev0,hidraw2:
> USB HID v1.11 Mouse [Synaptics T Pad V 01.31] on usb-0000:00:14.0-1/input0

Without the patch is it successfully retrieving feature 8?

It may be that it has HID_QUIRK_NO_INIT_REPORTS set via quirks in usbhid
which causes the driver to fetch the report in mt_feature_mapping() and
previously it did not do that.

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Andrew Duggan]

On 10/08/2015 02:40 AM, Mika Westerberg wrote:
> On Wed, Oct 07, 2015 at 06:12:42PM -0700, Andrew Duggan wrote:
>> I can confirm that it is working on the Dell XPS 13 and the buttonpad
>> property is set correctly. However, I also tried it on an older non
>> production precision touchpad and it didn't work. It reports that it fails
>> to fetch feature 8 and then doesn't report any input. I'm concerned that
>> some of the early PTPs in some Dell and Acer systems might not work with
>> this change. I'll test this tomorrow on a few more systems to see if the
>> problem is isolated to just this one non production touchpad.
> Thanks for testing.
>
>> [ 2969.222125] usb 1-1: New USB device found, idVendor=06cb, idProduct=2393
>> [ 2969.222128] usb 1-1: New USB device strings: Mfr=0, Product=2,
>> SerialNumber=0
>> [ 2969.222130] usb 1-1: Product: Synaptics T Pad V 01.31
>> [ 2974.223578] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
>> [ 2979.226037] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
>> [ 2979.226137] input: Synaptics T Pad V 01.31 UNKNOWN as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/0003:06CB:2393.0008/input/input34
>> [ 2979.226431] hid-multitouch 0003:06CB:2393.0008: input,hiddev0,hidraw2:
>> USB HID v1.11 Mouse [Synaptics T Pad V 01.31] on usb-0000:00:14.0-1/input0
> Without the patch is it successfully retrieving feature 8?
>
> It may be that it has HID_QUIRK_NO_INIT_REPORTS set via quirks in usbhid
> which causes the driver to fetch the report in mt_feature_mapping() and
> previously it did not do that.

No, this touchpad doesn't have HID_QUIRK_NO_INIT_REPORTS set and 
usbhid/hid-core.c is calling usbhid_init_reports() which is retrieving 
feature 8. But, I did some additional testing and I wasn't able to 
reproduce this issue on other systems. The failure occurs on a Dell XPS 
13 9343, but the exact same touchpad and the exact same kernel (I booted 
both systems off an external HD to ensure that the kernel and OS are 
identical) I didn't see a problem. I also tried a different USB PTP 
touchpad which didn't have an issue. Also, I tried returning in 
mt_get_feature() instead of reading the report when the report id is 8. 
When I did that everything worked.

So as of right now, it looks like a single non production USB PTP 
touchpad fails on one particular system. But, all other tests have been 
successful.

Let me know if you have any other suggestions for me to test. But, it 
seems like this failure might be fairly isolated.

Andrew

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Andrew Duggan]

On 10/08/2015 02:40 AM, Mika Westerberg wrote:
> On Wed, Oct 07, 2015 at 06:12:42PM -0700, Andrew Duggan wrote:
>> I can confirm that it is working on the Dell XPS 13 and the buttonpad
>> property is set correctly. However, I also tried it on an older non
>> production precision touchpad and it didn't work. It reports that it fails
>> to fetch feature 8 and then doesn't report any input. I'm concerned that
>> some of the early PTPs in some Dell and Acer systems might not work with
>> this change. I'll test this tomorrow on a few more systems to see if the
>> problem is isolated to just this one non production touchpad.
> Thanks for testing.
>
>> [ 2969.222125] usb 1-1: New USB device found, idVendor=06cb, idProduct=2393
>> [ 2969.222128] usb 1-1: New USB device strings: Mfr=0, Product=2,
>> SerialNumber=0
>> [ 2969.222130] usb 1-1: Product: Synaptics T Pad V 01.31
>> [ 2974.223578] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
>> [ 2979.226037] hid-multitouch 0003:06CB:2393.0008: failed to fetch feature 8
>> [ 2979.226137] input: Synaptics T Pad V 01.31 UNKNOWN as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/0003:06CB:2393.0008/input/input34
>> [ 2979.226431] hid-multitouch 0003:06CB:2393.0008: input,hiddev0,hidraw2:
>> USB HID v1.11 Mouse [Synaptics T Pad V 01.31] on usb-0000:00:14.0-1/input0
> Without the patch is it successfully retrieving feature 8?
>
> It may be that it has HID_QUIRK_NO_INIT_REPORTS set via quirks in usbhid
> which causes the driver to fetch the report in mt_feature_mapping() and
> previously it did not do that.

No, this touchpad doesn't have HID_QUIRK_NO_INIT_REPORTS set and 
usbhid/hid-core.c is calling usbhid_init_reports() which is retrieving 
feature 8. But, I did some additional testing and I wasn't able to 
reproduce this issue on other systems. The failure occurs on a Dell XPS 
13 9343, but the exact same touchpad and the exact same kernel (I booted 
both systems off an external HD to ensure that the kernel and OS are 
identical) I didn't see a problem. I also tried a different USB PTP 
touchpad which didn't have an issue. Also, I tried returning in 
mt_get_feature() instead of reading the report when the report id is 8. 
When I did that everything worked.

So as of right now, it looks like a single non production USB PTP 
touchpad fails on one particular system. But, all other tests have been 
successful.

Let me know if you have any other suggestions for me to test. But, it 
seems like this failure might be fairly isolated.

Andrew

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Mika Westerberg]

On Thu, Oct 08, 2015 at 06:10:23PM -0700, Andrew Duggan wrote:
> No, this touchpad doesn't have HID_QUIRK_NO_INIT_REPORTS set and
> usbhid/hid-core.c is calling usbhid_init_reports() which is retrieving
> feature 8. But, I did some additional testing and I wasn't able to reproduce
> this issue on other systems. The failure occurs on a Dell XPS 13 9343, but
> the exact same touchpad and the exact same kernel (I booted both systems off
> an external HD to ensure that the kernel and OS are identical) I didn't see
> a problem. I also tried a different USB PTP touchpad which didn't have an
> issue. Also, I tried returning in mt_get_feature() instead of reading the
> report when the report id is 8. When I did that everything worked.
> 
> So as of right now, it looks like a single non production USB PTP touchpad
> fails on one particular system. But, all other tests have been successful.
> 
> Let me know if you have any other suggestions for me to test. But, it seems
> like this failure might be fairly isolated.

Thanks a lot for testing all this.

If the failure is limited to a single non-production touchpad then, I
suppose we should be fine. As long as there are no such systems out
there in the wild.

One additional thing we could check in mt_get_feature() is if the device
is connected via I2C and only in that case fetch the feature in
question.

Jiri, Benjamin, what do you think?

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Benjamin Tissoires]

On Oct 09 2015 or thereabouts, Mika Westerberg wrote:
> On Thu, Oct 08, 2015 at 06:10:23PM -0700, Andrew Duggan wrote:
> > No, this touchpad doesn't have HID_QUIRK_NO_INIT_REPORTS set and
> > usbhid/hid-core.c is calling usbhid_init_reports() which is retrieving
> > feature 8. But, I did some additional testing and I wasn't able to reproduce
> > this issue on other systems. The failure occurs on a Dell XPS 13 9343, but
> > the exact same touchpad and the exact same kernel (I booted both systems off
> > an external HD to ensure that the kernel and OS are identical) I didn't see
> > a problem. I also tried a different USB PTP touchpad which didn't have an
> > issue. Also, I tried returning in mt_get_feature() instead of reading the
> > report when the report id is 8. When I did that everything worked.
> > 
> > So as of right now, it looks like a single non production USB PTP touchpad
> > fails on one particular system. But, all other tests have been successful.
> > 
> > Let me know if you have any other suggestions for me to test. But, it seems
> > like this failure might be fairly isolated.
> 
> Thanks a lot for testing all this.
> 
> If the failure is limited to a single non-production touchpad then, I
> suppose we should be fine. As long as there are no such systems out
> there in the wild.
> 
> One additional thing we could check in mt_get_feature() is if the device
> is connected via I2C and only in that case fetch the feature in
> question.
> 
> Jiri, Benjamin, what do you think?

I think we are fine with the current patch. The windows driver does
retrieve the important features during the initialization so with this
patch we are closer than ever than what it does.

If (and only if) we ever encounter a problem later on, we can always
make this patch only for i2c devices, but I would still try to match the
Windows driver as much as possible.

So my reviewed-by still applies.

Cheers,
Benjamin

Re: [PATCH v2] HID: multitouch: Fetch feature reports on demand for Win8 devices

[Jiri Kosina]

On Wed, 7 Oct 2015, Mika Westerberg wrote:

> Some newer Intel Skylake based Dell laptops with Win8 precision touchpad
> fail when initial feature reports are fetched from it. Below is an example
> output with some additional debug included:
> 
>  i2c_hid i2c-DLL0704:01: Fetching the HID descriptor
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=20 00
>  i2c_hid i2c-DLL0704:01: HID Descriptor: 1e 00 00 01 99 02 21 00 24 ...
>  ...
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 38 02 23 00
>  i2c_hid i2c-DLL0704:01: report (len=4): 04 00 08 05
>  i2c_hid i2c-DLL0704:01: report id 13
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 3d 02 23 00
>  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
>  i2c_hid i2c-DLL0704:01: report id 7
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 37 02 23 00
>  i2c_hid i2c-DLL0704:01: report (len=259): 03 01 07 fc 28 fe 84 40 ...
>  i2c_hid i2c-DLL0704:01: report id 4
>  i2c_hid i2c-DLL0704:01: i2c_hid_get_report
>  i2c_hid i2c-DLL0704:01: __i2c_hid_command: cmd=22 00 34 02 23 00
> 
> We manage to fetch few reports but then the touchpad dies:
> 
>  i2c_designware i2c_designware.1: i2c_dw_handle_tx_abort: lost arbitration
>  i2c_hid i2c-DLL0704:01: failed to retrieve report from device.
> 
> it eventually pulls the whole I2C bus low:
> 
>  i2c_designware i2c_designware.1: controller timed out
>  i2c_hid i2c-DLL0704:01: failed to set a report to device.
> 
> Fix this by preventing initial feature report retrieval for Win8 devices.
> Instead we fetch reports as needed in mt_feature_mapping(). This prevents
> fetching reports which might cause problems with the device in question.
> 
> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Added check for MT_CLS_WIN_8 so that the fix is only done for real Win8
> devices.

This is now in hid.git#for-4.4/multitouch. Thanks everybody,

-- 
Jiri Kosina
SUSE Labs