[PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Benjamin Tissoires]

Some old mice have a tendency to not accept the high resolution multiplier.
They reply with a -EPIPE which was previously ignored.

Force the call to resolution multiplier to be synchronous and actually
check for the answer. If this fails, consider the mouse like a normal one.

Fixes: 2dc702c991e377 ("HID: input: use the Resolution Multiplier for
       high-resolution scrolling")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1700071
Reported-and-tested-by: James Feeney <james@nurealm.net>
Cc: stable@vger.kernel.org  # v5.0+
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c  |  7 +++-
 drivers/hid/hid-input.c | 81 +++++++++++++++++++++++++----------------
 include/linux/hid.h     |  2 +-
 3 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 76464aabd20c..92387fc0bf18 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1637,7 +1637,7 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
  * Implement a generic .request() callback, using .raw_request()
  * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
  */
-void __hid_request(struct hid_device *hid, struct hid_report *report,
+int __hid_request(struct hid_device *hid, struct hid_report *report,
 		int reqtype)
 {
 	char *buf;
@@ -1646,7 +1646,7 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
 
 	buf = hid_alloc_report_buf(report, GFP_KERNEL);
 	if (!buf)
-		return;
+		return -ENOMEM;
 
 	len = hid_report_len(report);
 
@@ -1663,8 +1663,11 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
 	if (reqtype == HID_REQ_GET_REPORT)
 		hid_input_report(hid, report->type, buf, ret, 0);
 
+	ret = 0;
+
 out:
 	kfree(buf);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__hid_request);
 
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 1fce0076e7dc..fadf76f0a386 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1542,52 +1542,71 @@ static void hidinput_close(struct input_dev *dev)
 	hid_hw_close(hid);
 }
 
-static void hidinput_change_resolution_multipliers(struct hid_device *hid)
+static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
+		struct hid_report *report, bool use_logical_max)
 {
-	struct hid_report_enum *rep_enum;
-	struct hid_report *rep;
 	struct hid_usage *usage;
+	bool update_needed = false;
 	int i, j;
 
-	rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
-	list_for_each_entry(rep, &rep_enum->report_list, list) {
-		bool update_needed = false;
+	if (report->maxfield == 0)
+		return false;
 
-		if (rep->maxfield == 0)
-			continue;
+	/*
+	 * If we have more than one feature within this report we
+	 * need to fill in the bits from the others before we can
+	 * overwrite the ones for the Resolution Multiplier.
+	 */
+	if (report->maxfield > 1) {
+		hid_hw_request(hid, report, HID_REQ_GET_REPORT);
+		hid_hw_wait(hid);
+	}
 
-		/*
-		 * If we have more than one feature within this report we
-		 * need to fill in the bits from the others before we can
-		 * overwrite the ones for the Resolution Multiplier.
+	for (i = 0; i < report->maxfield; i++) {
+		__s32 value = use_logical_max ?
+			      report->field[i]->logical_maximum :
+			      report->field[i]->logical_minimum;
+
+		/* There is no good reason for a Resolution
+		 * Multiplier to have a count other than 1.
+		 * Ignore that case.
 		 */
-		if (rep->maxfield > 1) {
-			hid_hw_request(hid, rep, HID_REQ_GET_REPORT);
-			hid_hw_wait(hid);
-		}
+		if (report->field[i]->report_count != 1)
+			continue;
 
-		for (i = 0; i < rep->maxfield; i++) {
-			__s32 logical_max = rep->field[i]->logical_maximum;
+		for (j = 0; j < report->field[i]->maxusage; j++) {
+			usage = &report->field[i]->usage[j];
 
-			/* There is no good reason for a Resolution
-			 * Multiplier to have a count other than 1.
-			 * Ignore that case.
-			 */
-			if (rep->field[i]->report_count != 1)
+			if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
 				continue;
 
-			for (j = 0; j < rep->field[i]->maxusage; j++) {
-				usage = &rep->field[i]->usage[j];
+			*report->field[i]->value = value;
+			update_needed = true;
+		}
+	}
+
+	return update_needed;
+}
 
-				if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
-					continue;
+static void hidinput_change_resolution_multipliers(struct hid_device *hid)
+{
+	struct hid_report_enum *rep_enum;
+	struct hid_report *rep;
+	int ret;
 
-				*rep->field[i]->value = logical_max;
-				update_needed = true;
+	rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
+	list_for_each_entry(rep, &rep_enum->report_list, list) {
+		bool update_needed = __hidinput_change_resolution_multipliers(hid,
+								     rep, true);
+
+		if (update_needed) {
+			ret = __hid_request(hid, rep, HID_REQ_SET_REPORT);
+			if (ret) {
+				__hidinput_change_resolution_multipliers(hid,
+								    rep, false);
+				return;
 			}
 		}
-		if (update_needed)
-			hid_hw_request(hid, rep, HID_REQ_SET_REPORT);
 	}
 
 	/* refresh our structs */
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ac0c70b4ce10..5a24ebfb5b47 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -894,7 +894,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
 unsigned int hidinput_count_leds(struct hid_device *hid);
 __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
 void hid_output_report(struct hid_report *report, __u8 *data);
-void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
+int __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
 u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device,
-- 
2.19.2

[PATCH 2/2] HID: input: fix assignment of .value

[Benjamin Tissoires]

The value field is actually an array of .maxfield. We should assign the
correct number to the correct usage.

Not that we never encounter a device that requires this ATM, but better
have the proper code path.

Fixes: 2dc702c991e377 ("HID: input: use the Resolution Multiplier for
       high-resolution scrolling")
Cc: stable@vger.kernel.org  # v5.0+
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index fadf76f0a386..6dd0294e1133 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1580,7 +1580,7 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
 			if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
 				continue;
 
-			*report->field[i]->value = value;
+			report->field[i]->value[j] = value;
 			update_needed = true;
 		}
 	}
-- 
2.19.2

Re: [PATCH 2/2] HID: input: fix assignment of .value

[James Feeney]

On 4/23/19 11:21 AM, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 2dc702c991e3 HID: input: use the Resolution Multiplier for high-resolution scrolling.
> 
> The bot has tested the following trees: v5.0.9.
> 
> v5.0.9: Failed to apply! Possible dependencies:
>     724f54bc0063 ("HID: input: make sure the wheel high resolution multiplier is set")
> 
> 
> How should we proceed with this patch?
> 
> --
> Thanks,
> Sasha
> 

This patch will only apply *after* the application of the first patch, since they "overlap".

Yes, that is probably not the best circumstance.  I think Benjamin wanted to keep the issue in patch 2/2 distinct.

Was the first patch applied before attempting application of the second patch?

James

Re: [PATCH 2/2] HID: input: fix assignment of .value

[Sasha Levin]

On Tue, Apr 23, 2019 at 11:54:07AM -0600, James Feeney wrote:
>On 4/23/19 11:21 AM, Sasha Levin wrote:
>> Hi,
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: 2dc702c991e3 HID: input: use the Resolution Multiplier for high-resolution scrolling.
>>
>> The bot has tested the following trees: v5.0.9.
>>
>> v5.0.9: Failed to apply! Possible dependencies:
>>     724f54bc0063 ("HID: input: make sure the wheel high resolution multiplier is set")
>>
>>
>> How should we proceed with this patch?
>>
>> --
>> Thanks,
>> Sasha
>>
>
>This patch will only apply *after* the application of the first patch, since they "overlap".
>
>Yes, that is probably not the best circumstance.  I think Benjamin wanted to keep the issue in patch 2/2 distinct.
>
>Was the first patch applied before attempting application of the second patch?

My script messed up and listed it as a dependency (even though it's not
upstream) rather than applying it. I'll look into it, sorry for the
noise.

--
Thanks,
Sasha

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Benjamin Tissoires]

On Tue, Apr 23, 2019 at 5:46 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Some old mice have a tendency to not accept the high resolution multiplier.
> They reply with a -EPIPE which was previously ignored.
>
> Force the call to resolution multiplier to be synchronous and actually
> check for the answer. If this fails, consider the mouse like a normal one.
>
> Fixes: 2dc702c991e377 ("HID: input: use the Resolution Multiplier for
>        high-resolution scrolling")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1700071
> Reported-and-tested-by: James Feeney <james@nurealm.net>
> Cc: stable@vger.kernel.org  # v5.0+
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---

Given that this basically breaks a basic functionality of many
Microsoft mice, I went ahead and applied this series to
for-5.1/upstream-fixes

Cheers,
Benjamin

>  drivers/hid/hid-core.c  |  7 +++-
>  drivers/hid/hid-input.c | 81 +++++++++++++++++++++++++----------------
>  include/linux/hid.h     |  2 +-
>  3 files changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 76464aabd20c..92387fc0bf18 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1637,7 +1637,7 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>   * Implement a generic .request() callback, using .raw_request()
>   * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>   */
> -void __hid_request(struct hid_device *hid, struct hid_report *report,
> +int __hid_request(struct hid_device *hid, struct hid_report *report,
>                 int reqtype)
>  {
>         char *buf;
> @@ -1646,7 +1646,7 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
>
>         buf = hid_alloc_report_buf(report, GFP_KERNEL);
>         if (!buf)
> -               return;
> +               return -ENOMEM;
>
>         len = hid_report_len(report);
>
> @@ -1663,8 +1663,11 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
>         if (reqtype == HID_REQ_GET_REPORT)
>                 hid_input_report(hid, report->type, buf, ret, 0);
>
> +       ret = 0;
> +
>  out:
>         kfree(buf);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(__hid_request);
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 1fce0076e7dc..fadf76f0a386 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1542,52 +1542,71 @@ static void hidinput_close(struct input_dev *dev)
>         hid_hw_close(hid);
>  }
>
> -static void hidinput_change_resolution_multipliers(struct hid_device *hid)
> +static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
> +               struct hid_report *report, bool use_logical_max)
>  {
> -       struct hid_report_enum *rep_enum;
> -       struct hid_report *rep;
>         struct hid_usage *usage;
> +       bool update_needed = false;
>         int i, j;
>
> -       rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
> -       list_for_each_entry(rep, &rep_enum->report_list, list) {
> -               bool update_needed = false;
> +       if (report->maxfield == 0)
> +               return false;
>
> -               if (rep->maxfield == 0)
> -                       continue;
> +       /*
> +        * If we have more than one feature within this report we
> +        * need to fill in the bits from the others before we can
> +        * overwrite the ones for the Resolution Multiplier.
> +        */
> +       if (report->maxfield > 1) {
> +               hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> +               hid_hw_wait(hid);
> +       }
>
> -               /*
> -                * If we have more than one feature within this report we
> -                * need to fill in the bits from the others before we can
> -                * overwrite the ones for the Resolution Multiplier.
> +       for (i = 0; i < report->maxfield; i++) {
> +               __s32 value = use_logical_max ?
> +                             report->field[i]->logical_maximum :
> +                             report->field[i]->logical_minimum;
> +
> +               /* There is no good reason for a Resolution
> +                * Multiplier to have a count other than 1.
> +                * Ignore that case.
>                  */
> -               if (rep->maxfield > 1) {
> -                       hid_hw_request(hid, rep, HID_REQ_GET_REPORT);
> -                       hid_hw_wait(hid);
> -               }
> +               if (report->field[i]->report_count != 1)
> +                       continue;
>
> -               for (i = 0; i < rep->maxfield; i++) {
> -                       __s32 logical_max = rep->field[i]->logical_maximum;
> +               for (j = 0; j < report->field[i]->maxusage; j++) {
> +                       usage = &report->field[i]->usage[j];
>
> -                       /* There is no good reason for a Resolution
> -                        * Multiplier to have a count other than 1.
> -                        * Ignore that case.
> -                        */
> -                       if (rep->field[i]->report_count != 1)
> +                       if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
>                                 continue;
>
> -                       for (j = 0; j < rep->field[i]->maxusage; j++) {
> -                               usage = &rep->field[i]->usage[j];
> +                       *report->field[i]->value = value;
> +                       update_needed = true;
> +               }
> +       }
> +
> +       return update_needed;
> +}
>
> -                               if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
> -                                       continue;
> +static void hidinput_change_resolution_multipliers(struct hid_device *hid)
> +{
> +       struct hid_report_enum *rep_enum;
> +       struct hid_report *rep;
> +       int ret;
>
> -                               *rep->field[i]->value = logical_max;
> -                               update_needed = true;
> +       rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
> +       list_for_each_entry(rep, &rep_enum->report_list, list) {
> +               bool update_needed = __hidinput_change_resolution_multipliers(hid,
> +                                                                    rep, true);
> +
> +               if (update_needed) {
> +                       ret = __hid_request(hid, rep, HID_REQ_SET_REPORT);
> +                       if (ret) {
> +                               __hidinput_change_resolution_multipliers(hid,
> +                                                                   rep, false);
> +                               return;
>                         }
>                 }
> -               if (update_needed)
> -                       hid_hw_request(hid, rep, HID_REQ_SET_REPORT);
>         }
>
>         /* refresh our structs */
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ac0c70b4ce10..5a24ebfb5b47 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -894,7 +894,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>  unsigned int hidinput_count_leds(struct hid_device *hid);
>  __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>  void hid_output_report(struct hid_report *report, __u8 *data);
> -void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
> +int __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>  u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>  struct hid_device *hid_allocate_device(void);
>  struct hid_report *hid_register_report(struct hid_device *device,
> --
> 2.19.2
>

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[James Feeney]

Hey Benjamin

On 4/24/19 7:30 AM, Benjamin Tissoires wrote:
> Given that this basically breaks a basic functionality of many
> Microsoft mice, I went ahead and applied this series to
> for-5.1/upstream-fixes

Is there some reason that both patches should not be applied immediately, to the 5.0 series?

Or - likely I am uninformed - are the 5.1 patches applied as a set separate from the 5.0 series revisions?

Thanks
James

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Benjamin Tissoires]

On Wed, Apr 24, 2019 at 5:42 PM James Feeney <james@nurealm.net> wrote:
>
> Hey Benjamin
>
> On 4/24/19 7:30 AM, Benjamin Tissoires wrote:
> > Given that this basically breaks a basic functionality of many
> > Microsoft mice, I went ahead and applied this series to
> > for-5.1/upstream-fixes
>
> Is there some reason that both patches should not be applied immediately, to the 5.0 series?
>
> Or - likely I am uninformed - are the 5.1 patches applied as a set separate from the 5.0 series revisions?

For a patch to be picked up by stable, it first needs to go in Linus'
tree. Currently we are working on 5.1, so any stable patches need to
go in 5.1 first. Then, once they hit Linus' tree, the stable team will
pick them and backport them in the appropriate stable tree.

But distributions can backport them as they wish, so you can make a
request to your distribution to include them ASAP. They are officially
upstream, though yet to be sent to Linus.

Cheers,
Benjamin

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[James Feeney]

Hey Everyone

On 4/24/19 10:41 AM, Benjamin Tissoires wrote:
>>> For a patch to be picked up by stable, it first needs to go in Linus'
>>> tree. Currently we are working on 5.1, so any stable patches need to
>>> go in 5.1 first. Then, once they hit Linus' tree, the stable team will
>>> pick them and backport them in the appropriate stable tree.

Hmm - so, I just booted linux 5.1.9, and this patch set is *still* missing from the kernel.

Is there anything that we can do about this?

James

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Greg KH]

On Fri, Jun 14, 2019 at 04:09:35PM -0600, James Feeney wrote:
> Hey Everyone
> 
> On 4/24/19 10:41 AM, Benjamin Tissoires wrote:
> >>> For a patch to be picked up by stable, it first needs to go in Linus'
> >>> tree. Currently we are working on 5.1, so any stable patches need to
> >>> go in 5.1 first. Then, once they hit Linus' tree, the stable team will
> >>> pick them and backport them in the appropriate stable tree.
> 
> Hmm - so, I just booted linux 5.1.9, and this patch set is *still* missing from the kernel.
> 
> Is there anything that we can do about this?

What is the git commit id of the patch in Linus's tree?

As I said before, it can not be backported until it shows up there
first.

thanks,

greg k-h

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Thomas Backlund]

Den 15-06-2019 kl. 08:50, skrev Greg KH:
> On Fri, Jun 14, 2019 at 04:09:35PM -0600, James Feeney wrote:
>> Hey Everyone
>>
>> On 4/24/19 10:41 AM, Benjamin Tissoires wrote:
>>>>> For a patch to be picked up by stable, it first needs to go in Linus'
>>>>> tree. Currently we are working on 5.1, so any stable patches need to
>>>>> go in 5.1 first. Then, once they hit Linus' tree, the stable team will
>>>>> pick them and backport them in the appropriate stable tree.
>>
>> Hmm - so, I just booted linux 5.1.9, and this patch set is *still* missing from the kernel.
>>
>> Is there anything that we can do about this?
> 
> What is the git commit id of the patch in Linus's tree?
> 
> As I said before, it can not be backported until it shows up there
> first.
> 

That would be:
d43c17ead879ba7c076dc2f5fd80cd76047c9ff4

and

39b3c3a5fbc5d744114e497d35bf0c12f798c134

--
Thomas

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Thomas Backlund]

Den 15-06-2019 kl. 08:50, skrev Greg KH:
> On Fri, Jun 14, 2019 at 04:09:35PM -0600, James Feeney wrote:
>> Hey Everyone
>>
>> On 4/24/19 10:41 AM, Benjamin Tissoires wrote:
>>>>> For a patch to be picked up by stable, it first needs to go in Linus'
>>>>> tree. Currently we are working on 5.1, so any stable patches need to
>>>>> go in 5.1 first. Then, once they hit Linus' tree, the stable team will
>>>>> pick them and backport them in the appropriate stable tree.
>>
>> Hmm - so, I just booted linux 5.1.9, and this patch set is *still* missing from the kernel.
>>
>> Is there anything that we can do about this?
> 
> What is the git commit id of the patch in Linus's tree?
> 
> As I said before, it can not be backported until it shows up there
> first.
> 

That would be:
d43c17ead879ba7c076dc2f5fd80cd76047c9ff4

and

39b3c3a5fbc5d744114e497d35bf0c12f798c134

Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set

[Greg KH]

On Sat, Jun 15, 2019 at 12:03:04PM +0300, Thomas Backlund wrote:
> Den 15-06-2019 kl. 08:50, skrev Greg KH:
> > On Fri, Jun 14, 2019 at 04:09:35PM -0600, James Feeney wrote:
> > > Hey Everyone
> > > 
> > > On 4/24/19 10:41 AM, Benjamin Tissoires wrote:
> > > > > > For a patch to be picked up by stable, it first needs to go in Linus'
> > > > > > tree. Currently we are working on 5.1, so any stable patches need to
> > > > > > go in 5.1 first. Then, once they hit Linus' tree, the stable team will
> > > > > > pick them and backport them in the appropriate stable tree.
> > > 
> > > Hmm - so, I just booted linux 5.1.9, and this patch set is *still* missing from the kernel.
> > > 
> > > Is there anything that we can do about this?
> > 
> > What is the git commit id of the patch in Linus's tree?
> > 
> > As I said before, it can not be backported until it shows up there
> > first.
> > 
> 
> That would be:
> d43c17ead879ba7c076dc2f5fd80cd76047c9ff4
> 
> and
> 
> 39b3c3a5fbc5d744114e497d35bf0c12f798c134

Thanks, now queued up.

greg k-h