linux-kernel - Re: [PATCH v6 1/2] HID: logitech: Add MX Master over Bluetooth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO-hwJJ18L3nDMQ_OqCZPyA+FFk3QsnbY=dYSNkELWzTSAnQLA@mail.gmail.com>
Date:   Fri, 18 Oct 2019 17:36:05 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Mazin Rezk <mnrzk@...tonmail.com>
Cc:     "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "jikos@...nel.org" <jikos@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Filipe Laíns <lains@...hlinux.org>
Subject: Re: [PATCH v6 1/2] HID: logitech: Add MX Master over Bluetooth

Hi Mazin,

On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk <mnrzk@...tonmail.com> wrote:
>
> This patch adds support for the MX Master (b01e and b012) and also adds
> foundational code for other Bluetooth LE HID++ devices to be added.
>
> Some devices do not support short reports and thus have a quirk
> (HIDPP_QUIRK_MISSING_SHORT_REPORTS) that forces short reports to be sent as
> long reports. Since all Bluetooth LE HID++ devices seem to act this way,
> HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.
>
> To allow for some space for future quirks, I changed the comment that
> defines the bits reserved for classes from 2...20 to 2..15.
>
> Signed-off-by: Mazin Rezk <mnrzk@...tonmail.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 0179f7ed77e5..997b1056850a 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -59,7 +59,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
>  #define HIDPP_QUIRK_CLASS_G920                 BIT(3)
>  #define HIDPP_QUIRK_CLASS_K750                 BIT(4)
>
> -/* bits 2..20 are reserved for classes */
> +/* bits 2..15 are reserved for classes */
> +#define HIDPP_QUIRK_MISSING_SHORT_REPORTS      BIT(20)
>  /* #define HIDPP_QUIRK_CONNECT_EVENTS          BIT(21) disabled */
>  #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS       BIT(22)
>  #define HIDPP_QUIRK_NO_HIDINPUT                        BIT(23)
> @@ -81,6 +82,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
>                                          HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
>                                          HIDPP_QUIRK_HI_RES_SCROLL_X2121)
>
> +#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
> +
>  #define HIDPP_QUIRK_DELAYED_INIT               HIDPP_QUIRK_NO_HIDINPUT
>
>  #define HIDPP_CAPABILITY_HIDPP10_BATTERY       BIT(0)
> @@ -340,6 +343,12 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
>         struct hidpp_report *message;
>         int ret, max_count;
>
> +       /* Force long reports on devices that do not support short reports */
> +       if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
> +           report_id == REPORT_ID_HIDPP_SHORT)
> +               report_id = REPORT_ID_HIDPP_LONG;

Wouldn't it be faster to just store which report needs to be put here
in struct hidpp_device?

> +
> +
>         switch (report_id) {
>         case REPORT_ID_HIDPP_SHORT:
>                 max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
> @@ -3482,6 +3491,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
>
>  static bool hidpp_validate_device(struct hid_device *hdev)
>  {
> +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +       /* Skip the short report check if the device does not support it */
> +       if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
> +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> +                                            HIDPP_REPORT_LONG_LENGTH, false);
> +

I just merged Andrey's report detection, which means you will need to
update this hunk:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-5.4/upstream-fixes&id=905d754c53a522aacf806ea1d3e7c929148c1910

The good thing, is that now you can simply auto-detect if the short
report is missing. If the returned report_length is null, you know
that the report is missing (and thus you can remember to set the
quirk/which report id is needed).

Cheers,
Benjamin

>         return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
>                                      HIDPP_REPORT_SHORT_LENGTH, false) &&
>                hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> @@ -3773,6 +3788,13 @@ static const struct hid_device_id hidpp_devices[] = {
>         { /* MX5500 keyboard over Bluetooth */
>           HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
>           .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
> +       { /* MX Master mouse over Bluetooth */
> +         HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
> +                        HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
> +         .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
> +                        HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
>         {}
>  };
>
> --
> 2.23.0
>