linux-kernel - Re: [PATCH 1/4] HID: wiiu-drc: Add a driver for this gamepad

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]

Date:   Wed,  5 May 2021 22:33:15 +0000
From:   Jonathan Neuschäfer <j.ne@...teo.net>
To:     Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
Cc:     linux-input@...r.kernel.org, Ash Logan <ash@...quark.com>,
        Jonathan Neuschäfer <j.ne@...teo.net>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] HID: wiiu-drc: Add a driver for this gamepad

Hi,

some mostly trivial remarks and questions of curiosity below, because
I'm not very qualified to review the input subsystem side of things.


On Mon, May 03, 2021 at 01:28:32AM +0200, Emmanuel Gil Peyrot wrote:
> From: Ash Logan <ash@...quark.com>
> 
> This driver is for the DRC (wireless gamepad) when plugged to the DRH of
> the Wii U, a chip exposing it as a USB device.

s/plugged/wirelessly connected/, rather

> 
> This first patch exposes the buttons and sticks of this device, so that
> it can act as a plain game controller.
> 
> Signed-off-by: Ash Logan <ash@...quark.com>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
> ---

Out of curiosity:

Do the HID reports travel over the wireless link from DRC to DRH, or are
they formed in DRH firmware?

Is there a reference of the device-specific HID format? I briefly looked
at https://libdrc.org/docs/index.html but couldn't find it there.


>  drivers/hid/Kconfig        |   7 +
>  drivers/hid/Makefile       |   1 +
>  drivers/hid/hid-ids.h      |   1 +
>  drivers/hid/hid-quirks.c   |   3 +
>  drivers/hid/hid-wiiu-drc.c | 270 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 282 insertions(+)
>  create mode 100644 drivers/hid/hid-wiiu-drc.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4bf263c2d61a..01116c315459 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1105,6 +1105,13 @@ config HID_WACOM
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called wacom.
>  
> +config HID_WIIU_DRC
> +	tristate "Nintendo Wii U gamepad over internal DRH"

                                 gamepad (DRC)

... so it's clearer where the "DRC" name comes from.

> +#if IS_ENABLED(CONFIG_HID_WIIU_DRC)
> +	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIU_DRH) },
> +#endif

Is the DRC connection the only USB function that the DRH provides?


> +++ b/drivers/hid/hid-wiiu-drc.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * HID driver for Nintendo Wii U gamepad, connected via console-internal DRH

                                    gamepad (DRC)


> +static int drc_raw_event(struct hid_device *hdev, struct hid_report *report,
> +	 u8 *data, int len)
> +{
> +	struct drc *drc = hid_get_drvdata(hdev);
> +	int i;
> +	u32 buttons;
> +
> +	if (len != 128)
> +		return 0;

From include/linux/hid.h:

 * raw_event and event should return negative on error, any other value will
 * pass the event on to .event() typically return 0 for success.

Not sure if returning 0 as you do above is appropriate.


> +static bool drc_setup_joypad(struct drc *drc,
> +		struct hid_device *hdev)
> +{
> +	struct input_dev *input_dev;
> +
> +	input_dev = allocate_and_setup(hdev, DEVICE_NAME " Joypad");

"Nintendo Wii U gamepad Joypad" looks a bit sub-optimal, but I'm not
sure about the conventions here.


> +
> +	/* These two buttons are actually TV control and Power. */
> +	set_bit(BTN_Z, input_dev->keybit);
> +	set_bit(BTN_DEAD, input_dev->keybit);

Hmm... from what I've deen the TV control button opens a menu on the
gamepad itself. Does it send the input event in addition to that?
Or is there a mode where it opens the TV menu, and a mode where it
forwards the button press to the Wii U?


> +MODULE_AUTHOR("Ash Logan <ash@...quark.com>");

Since you're submitting the driver, rather than Ash, maybe adjust the
author field here? (totally your choice.)



Thanks,
Jonathan

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)