| 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
| ||
|
Message-ID: <CAPBBQ-ZYFA82GDz0uf9k=7dmdr+THgP0fqJKvXB_b8a62HMeyA@mail.gmail.com>
Date: Mon, 23 Jan 2023 22:24:53 +0100
From: Philippe Valembois <lephilousophe@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.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
Hello,
Thanks for the review.
I just sent an updated patch taking your comments into account.
Regards,
Philippe Valembois
Le lun. 23 janv. 2023 à 08:57, Benjamin Tissoires
<benjamin.tissoires@...hat.com> a écrit :
>
> 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
>
Powered by blists - more mailing lists