linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hid-input: Fix devices that return multiple bytes in battery report
@ 2020-07-10 15:19 Grant Likely
  2020-07-11  1:59 ` Darren Hart
  2020-07-16  0:27 ` Sasha Levin
  0 siblings, 2 replies; 4+ messages in thread
From: Grant Likely @ 2020-07-10 15:19 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, Grant Likely, Grant Likely,
	Darren Hart, Jiri Kosina, Benjamin Tissoires, stable

Some devices, particularly the 3DConnexion Spacemouse wireless 3D
controllers, return more than just the battery capacity in the battery
report. The Spacemouse devices return an additional byte with a device
specific field. However, hidinput_query_battery_capacity() only
requests a 2 byte transfer.

When a spacemouse is connected via USB (direct wire, no wireless dongle)
and it returns a 3 byte report instead of the assumed 2 byte battery
report the larger transfer confuses and frightens the USB subsystem
which chooses to ignore the transfer. Then after 2 seconds assume the
device has stopped responding and reset it. This can be reproduced
easily by using a wired connection with a wireless spacemouse. The
Spacemouse will enter a loop of resetting every 2 seconds which can be
observed in dmesg.

This patch solves the problem by increasing the transfer request to 4
bytes instead of 2. The fix isn't particularly elegant, but it is simple
and safe to backport to stable kernels. A further patch will follow to
more elegantly handle battery reports that contain additional data.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Darren Hart <darren@dvhart.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/hid/hid-input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dea9cc65bf80..e8641ce677e4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -350,13 +350,13 @@ static int hidinput_query_battery_capacity(struct hid_device *dev)
 	u8 *buf;
 	int ret;
 
-	buf = kmalloc(2, GFP_KERNEL);
+	buf = kmalloc(4, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
+	ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 4,
 				 dev->battery_report_type, HID_REQ_GET_REPORT);
-	if (ret != 2) {
+	if (ret < 2) {
 		kfree(buf);
 		return -ENODATA;
 	}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] hid-input: Fix devices that return multiple bytes in battery report
  2020-07-10 15:19 [PATCH] hid-input: Fix devices that return multiple bytes in battery report Grant Likely
@ 2020-07-11  1:59 ` Darren Hart
  2020-07-14 13:17   ` Jiri Kosina
  2020-07-16  0:27 ` Sasha Levin
  1 sibling, 1 reply; 4+ messages in thread
From: Darren Hart @ 2020-07-11  1:59 UTC (permalink / raw)
  To: Grant Likely
  Cc: LKML, linux-input, Grant Likely, Darren Hart, Jiri Kosina,
	Benjamin Tissoires, stable

On Fri, Jul 10, 2020 at 8:19 AM Grant Likely <grant.likely@secretlab.ca> wrote:
>
> Some devices, particularly the 3DConnexion Spacemouse wireless 3D
> controllers, return more than just the battery capacity in the battery
> report. The Spacemouse devices return an additional byte with a device
> specific field. However, hidinput_query_battery_capacity() only
> requests a 2 byte transfer.
>
> When a spacemouse is connected via USB (direct wire, no wireless dongle)
> and it returns a 3 byte report instead of the assumed 2 byte battery
> report the larger transfer confuses and frightens the USB subsystem
> which chooses to ignore the transfer. Then after 2 seconds assume the
> device has stopped responding and reset it. This can be reproduced
> easily by using a wired connection with a wireless spacemouse. The
> Spacemouse will enter a loop of resetting every 2 seconds which can be
> observed in dmesg.
>
> This patch solves the problem by increasing the transfer request to 4
> bytes instead of 2. The fix isn't particularly elegant, but it is simple
> and safe to backport to stable kernels. A further patch will follow to
> more elegantly handle battery reports that contain additional data.
>

Applied and tested on 5.8.0-rc4+ (aa0c9086b40c) with a 3Dconnexion
SpaceMouse Wireless (tested connected via USB). Observed the same
behavior Grant reports before the patch. After the patch, the device stays
connected successfully.

Tested-by: Darren Hart <dvhart@infradead.org>

Thanks Grant!

> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Darren Hart <darren@dvhart.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/hid/hid-input.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index dea9cc65bf80..e8641ce677e4 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -350,13 +350,13 @@ static int hidinput_query_battery_capacity(struct hid_device *dev)
>         u8 *buf;
>         int ret;
>
> -       buf = kmalloc(2, GFP_KERNEL);
> +       buf = kmalloc(4, GFP_KERNEL);
>         if (!buf)
>                 return -ENOMEM;
>
> -       ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
> +       ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 4,
>                                  dev->battery_report_type, HID_REQ_GET_REPORT);
> -       if (ret != 2) {
> +       if (ret < 2) {
>                 kfree(buf);
>                 return -ENODATA;
>         }
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hid-input: Fix devices that return multiple bytes in battery report
  2020-07-11  1:59 ` Darren Hart
@ 2020-07-14 13:17   ` Jiri Kosina
  0 siblings, 0 replies; 4+ messages in thread
From: Jiri Kosina @ 2020-07-14 13:17 UTC (permalink / raw)
  To: Darren Hart
  Cc: Grant Likely, LKML, linux-input, Grant Likely, Darren Hart,
	Benjamin Tissoires, stable

On Fri, 10 Jul 2020, Darren Hart wrote:

> > Some devices, particularly the 3DConnexion Spacemouse wireless 3D
> > controllers, return more than just the battery capacity in the battery
> > report. The Spacemouse devices return an additional byte with a device
> > specific field. However, hidinput_query_battery_capacity() only
> > requests a 2 byte transfer.
> >
> > When a spacemouse is connected via USB (direct wire, no wireless dongle)
> > and it returns a 3 byte report instead of the assumed 2 byte battery
> > report the larger transfer confuses and frightens the USB subsystem
> > which chooses to ignore the transfer. Then after 2 seconds assume the
> > device has stopped responding and reset it. This can be reproduced
> > easily by using a wired connection with a wireless spacemouse. The
> > Spacemouse will enter a loop of resetting every 2 seconds which can be
> > observed in dmesg.
> >
> > This patch solves the problem by increasing the transfer request to 4
> > bytes instead of 2. The fix isn't particularly elegant, but it is simple
> > and safe to backport to stable kernels. A further patch will follow to
> > more elegantly handle battery reports that contain additional data.
> >
> 
> Applied and tested on 5.8.0-rc4+ (aa0c9086b40c) with a 3Dconnexion
> SpaceMouse Wireless (tested connected via USB). Observed the same
> behavior Grant reports before the patch. After the patch, the device stays
> connected successfully.
> 
> Tested-by: Darren Hart <dvhart@infradead.org>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hid-input: Fix devices that return multiple bytes in battery report
  2020-07-10 15:19 [PATCH] hid-input: Fix devices that return multiple bytes in battery report Grant Likely
  2020-07-11  1:59 ` Darren Hart
@ 2020-07-16  0:27 ` Sasha Levin
  1 sibling, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2020-07-16  0:27 UTC (permalink / raw)
  To: Sasha Levin, Grant Likely
  Cc: linux-kernel, linux-input, Darren Hart, Jiri Kosina,
	Benjamin Tissoires, stable, stable

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.7.8, v5.4.51, v4.19.132, v4.14.188, v4.9.230, v4.4.230.

v5.7.8: Build OK!
v5.4.51: Build OK!
v4.19.132: Build OK!
v4.14.188: Build OK!
v4.9.230: Failed to apply! Possible dependencies:
    581c4484769e6 ("HID: input: map digitizer battery usage")

v4.4.230: Failed to apply! Possible dependencies:
    581c4484769e6 ("HID: input: map digitizer battery usage")
    5d9374cf5f66e ("HID: input: ignore the battery in OKLICK Laser BTmouse")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-07-16  0:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-10 15:19 [PATCH] hid-input: Fix devices that return multiple bytes in battery report Grant Likely
2020-07-11  1:59 ` Darren Hart
2020-07-14 13:17   ` Jiri Kosina
2020-07-16  0:27 ` Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).