| 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: <Yw8+cx+0QyHe6K95@gmail.com>
Date: Wed, 31 Aug 2022 12:56:51 +0200
From: Marcus Folkesson <marcus.folkesson@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: Jiri Kosina <jikos@...nel.org>,
"open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] HID: Add driver for VRC-2 Car Controller
Hi,
On Wed, Aug 31, 2022 at 11:45:21AM +0200, Benjamin Tissoires wrote:
> On Tue, Aug 30, 2022 at 9:46 PM Marcus Folkesson
> <marcus.folkesson@...il.com> wrote:
> >
> > VRC-2 is 2-axis controller often used in car simulators.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@...il.com>
> > ---
> > MAINTAINERS | 6 +++
> > drivers/hid/Kconfig | 9 ++++
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-vrc2.c | 100 +++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 116 insertions(+)
> > create mode 100644 drivers/hid/hid-vrc2.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 216b9f021f72..33010f93c993 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8919,6 +8919,12 @@ F: drivers/hid/hid-sensor-*
> > F: drivers/iio/*/hid-*
> > F: include/linux/hid-sensor-*
> >
> > +HID VRC-2 CAR CONTROLLER DRIVER
> > +M: Marcus Folkesson <marcus.folkesson@...il.com>
> > +L: linux-input@...r.kernel.org
> > +S: Maintained
> > +F: drivers/hid/hid-vrc2.c
> > +
> > HID WACOM DRIVER
> > M: Ping Cheng <ping.cheng@...om.com>
> > M: Jason Gerecke <jason.gerecke@...om.com>
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index d8313d36086c..518e0a5a7852 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -481,6 +481,15 @@ config HID_VIEWSONIC
> > help
> > Support for ViewSonic/Signotec PD1011 signature pad.
> >
> > +config HID_VRC2
> > + tristate "VRC-2 Car Controller"
> > + depends on HID
> > + help
> > + Support for VRC-2 2-axis Car Controller
>
> maybe "Support for VRC-2 which is a 2-axis controller often used in
> car simulators." to be a little bit more explicit.
Will change, thank you.
>
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called hid-vrc2.
> > +
> > config HID_XIAOMI
> > tristate "Xiaomi"
> > depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 85d50ab352ee..32ab85b1a776 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -137,6 +137,7 @@ obj-$(CONFIG_HID_XINMO) += hid-xinmo.o
> > obj-$(CONFIG_HID_ZEROPLUS) += hid-zpff.o
> > obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
> > obj-$(CONFIG_HID_VIEWSONIC) += hid-viewsonic.o
> > +obj-$(CONFIG_HID_VRC2) += hid-vrc2.o
> >
> > wacom-objs := wacom_wac.o wacom_sys.o
> > obj-$(CONFIG_HID_WACOM) += wacom.o
> > diff --git a/drivers/hid/hid-vrc2.c b/drivers/hid/hid-vrc2.c
> > new file mode 100644
> > index 000000000000..43c98607b837
> > --- /dev/null
> > +++ b/drivers/hid/hid-vrc2.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * HID driver for VRC-2 2-axis Car controller
> > + *
> > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@...il.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +
> > +/*
> > + * VID/PID are probably "borrowed", so keep them locally and
> > + * do not populate hid-ids.h with those.
> > + */
> > +#define USB_VENDOR_ID_VRC2 (0x07c0)
> > +#define USB_DEVICE_ID_VRC2 (0x1125)
> > +
> > +static __u8 vrc2_rdesc_fixed[] = {
> > + 0x05, 0x01, // Usage Page (Generic Desktop Ctrls)
> > + 0x09, 0x04, // Usage (Joystick)
> > + 0xA1, 0x01, // Collection (Application)
> > + 0x09, 0x01, // Usage (Pointer)
> > + 0xA1, 0x00, // Collection (Physical)
> > + 0x09, 0x30, // Usage (X)
> > + 0x09, 0x31, // Usage (Y)
> > + 0x15, 0x00, // Logical Minimum (0)
> > + 0x26, 0xFF, 0x07, // Logical Maximum (2047)
> > + 0x35, 0x00, // Physical Minimum (0)
> > + 0x46, 0xFF, 0x00, // Physical Maximum (255)
> > + 0x75, 0x10, // Report Size (16)
> > + 0x95, 0x02, // Report Count (2)
> > + 0x81, 0x02, // Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> > + 0xC0, // End Collection
> > + 0x75, 0x08, // Report Size (8)
> > + 0x95, 0x03, // Report Count (3)
> > + 0x81, 0x03, // Input (Cnst,Var,Abs)
> > + 0xC0, // End Collection
> > +};
> > +
> > +static __u8 *vrc2_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > + unsigned int *rsize)
> > +{
> > + if (*rsize == 23)
> > + return rdesc;
>
> This test is not required. probe() returned -ENODEV for this
> interface, so we are sure we have the correct one when we are there.
Got it.
>
> > +
> > + hid_info(hdev, "fixing up VRC-2 report descriptor\n");
> > + *rsize = sizeof(vrc2_rdesc_fixed);
> > + return vrc2_rdesc_fixed;
> > +}
> > +
> > +static int vrc2_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > + int ret;
> > +
> > + /*
> > + * The device gives us 2 separate USB endpoints.
> > + * One of those (the one with report descriptor size of 23) is just bogus so ignore it
> > + */
> > + if (hdev->dev_rsize == 23)
> > + return -ENODEV;
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev, "parse failed\n");
> > + return ret;
> > + }
> > +
> > + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > + if (ret) {
> > + hid_err(hdev, "hw start failed\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void vrc2_remove(struct hid_device *hdev)
> > +{
> > + hid_hw_stop(hdev);
> > +}
>
> I am pretty sure you can remove that vrc2_remove(), hid-core should be
> able to call hid_hw_stop() for you.
>
Hrm. Looking at this description:
/**
* hid_hw_start - start underlying HW
* @hdev: hid device
* @connect_mask: which outputs to connect, see HID_CONNECT_*
*
* Call this in probe function *after* hid_parse. This will setup HW
* buffers and start the device (if not defeirred to device open).
* hid_hw_stop must be called if this was successful.
*/
and
/**
* hid_hw_stop - stop underlying HW
* @hdev: hid device
*
* This is usually called from remove function or from probe when something
* failed and hid_hw_start was called already.
*/
Most of the HID drivers actually do call hid_hw_stop() in the their .remove.
hid_device_remove(struct device *dev)
Seems to do this for you though.
Maybe we should clean it up to avoid more drivers to follow this
pattern?
> > +
> > +static const struct hid_device_id vrc2_devices[] = {
> > + { HID_USB_DEVICE(USB_VENDOR_ID_VRC2, USB_DEVICE_ID_VRC2) },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(hid, vrc2_devices);
> > +
> > +static struct hid_driver vrc2_driver = {
> > + .name = "vrc2",
> > + .id_table = vrc2_devices,
> > + .report_fixup = vrc2_report_fixup,
> > + .probe = vrc2_probe,
> > + .remove = vrc2_remove,
> > +};
> > +module_hid_driver(vrc2_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@...il.com>");
> > +MODULE_DESCRIPTION("HID driver for VRC-2 2-axis Car controller");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.37.1
> >
>
> Cheers,
> Benjamin
>
Best regards,
Marcus Folkesson
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists