linux-kernel - Re: [PATCH 1/1] HID: evision: Add preliminary support for EVision keyboards

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-hwJJwUQJueutZ5z_4qwBb1Y4+-9h3ta0Xran=s6pJ_e3itQ@mail.gmail.com>
Date:   Mon, 23 Jan 2023 08:57:39 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Philippe Valembois <lephilousophe@...il.com>
Cc:     Jiri Kosina <jikos@...nel.org>, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org
Subject: Re: [PATCH 1/1] HID: evision: Add preliminary support for EVision keyboards

On Sat, Dec 24, 2022 at 1:23 PM Philippe Valembois
<lephilousophe@...il.com> wrote:
>
> From: Philippe Valembois <lephilousophe@...rs.noreply.github.com>

Jiri, I have a doubt. Do we accept emails from users.noreply.github.com?

>
> For now only supports one model and only filters out bogus reports sent
> when the keyboard has been configured through hidraw.
> Without this, as events are not released, soft repeat floods userspace
> with unknown key events.
>
> Signed-off-by: Philippe Valembois <lephilousophe@...rs.noreply.github.com>
> ---
>  drivers/hid/Kconfig       |  7 ++++
>  drivers/hid/Makefile      |  1 +
>  drivers/hid/hid-evision.c | 79 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/hid/hid-evision.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index e2a5d30c8..1320ea75c 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -329,6 +329,13 @@ config HID_ELO
>         Support for the ELO USB 4000/4500 touchscreens. Note that this is for
>         different devices than those handled by CONFIG_TOUCHSCREEN_USB_ELO.
>
> +config HID_EVISION
> +       tristate "EVision Keyboards Support"
> +       depends on USB_HID

AFAICT, the driver only uses pure HID API, so you should be able to
depend on HID, not just USB_HID.

> +       help
> +       Support for some EVision keyboards. Note that this is needed only when
> +       applying customization using userspace programs.
> +
>  config HID_EZKEY
>         tristate "Ezkey BTC 8193 keyboard"
>         default !EXPERT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index e8014c1a2..bd01571dd 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_HID_EMS_FF)      += hid-emsff.o
>  obj-$(CONFIG_HID_ELAN)         += hid-elan.o
>  obj-$(CONFIG_HID_ELECOM)       += hid-elecom.o
>  obj-$(CONFIG_HID_ELO)          += hid-elo.o
> +obj-$(CONFIG_HID_EVISION)      += hid-evision.o
>  obj-$(CONFIG_HID_EZKEY)                += hid-ezkey.o
>  obj-$(CONFIG_HID_FT260)                += hid-ft260.o
>  obj-$(CONFIG_HID_GEMBIRD)      += hid-gembird.o
> diff --git a/drivers/hid/hid-evision.c b/drivers/hid/hid-evision.c
> new file mode 100644
> index 000000000..6ea331575
> --- /dev/null
> +++ b/drivers/hid/hid-evision.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  HID driver for EVision devices
> + *  For now, only ignore bogus consumer reports
> + *  sent after the keyboard has been configured
> + *
> + *  Copyright (c) 2022 Philippe Valembois
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>

Outside of hid_is_usb(), you are not using anything USB related, so
this can be dropped

> +
> +
> +#define USB_VENDOR_ID_EVISION       0x320f
> +#define USB_DEVICE_ID_EVISION_ICL01 0x5041

We tend to add those variables in drivers/hid/hid-ids.h

> +
> +static int evision_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> +               struct hid_field *field, struct hid_usage *usage,
> +               unsigned long **bit, int *max)
> +{
> +       if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER)
> +               return 0;
> +
> +       // Ignore key down event

No C++ comments style please, use /* */ instead

> +       if ((usage->hid & HID_USAGE) >> 8 == 0x05)
> +               return -1;
> +       // Ignore key up event

Same (and for any other // below).

> +       if ((usage->hid & HID_USAGE) >> 8 == 0x06)
> +               return -1;
> +
> +       switch (usage->hid & HID_USAGE) {
> +       // Ignore configuration saved event
> +       case 0x0401: return -1;
> +       // Ignore reset event
> +       case 0x0402: return -1;
> +       }
> +       return 0;
> +}
> +
> +static int evision_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +       int ret;
> +
> +       if (!hid_is_usb(hdev))
> +               return -EINVAL;

This can be dropped...

> +
> +       ret = hid_parse(hdev);
> +       if (ret) {
> +               hid_err(hdev, "EVision hid parse failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +       if (ret) {
> +               hid_err(hdev, "EVision hw start failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;

... which means the probe is the default one, meaning it can also be
dropped from the patch :)

> +}
> +
> +static const struct hid_device_id evision_devices[] = {
> +       { HID_USB_DEVICE(USB_VENDOR_ID_EVISION, USB_DEVICE_ID_EVISION_ICL01) },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(hid, evision_devices);
> +
> +static struct hid_driver evision_driver = {
> +       .name = "evision",
> +       .id_table = evision_devices,
> +       .input_mapping = evision_input_mapping,
> +       .probe = evision_probe,

Just for completeness, remove that .probe line and your driver will
behave the same and be smaller :)

> +};
> +module_hid_driver(evision_driver);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.38.2
>

Cheers,
Benjamin