Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200710151939.4894-1-grant.likely@arm.com>
In-Reply-To: <20200710151939.4894-1-grant.likely@arm.com>
From:   Darren Hart <dvhart@infradead.org>
Date:   Fri, 10 Jul 2020 18:59:42 -0700
Message-ID: <CAJuF2pzvh2G7_2q88a4e=dpB1RATrdF8jsOkpVuuueZLGGbsiQ@mail.gmail.com>
Subject: Re: [PATCH] hid-input: Fix devices that return multiple bytes in
 battery report
To:     Grant Likely <grant.likely@secretlab.ca>
Cc:     LKML <linux-kernel@vger.kernel.org>, linux-input@vger.kernel.org,
        Grant Likely <grant.likely@arm.com>,
        Darren Hart <darren@dvhart.com>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>