linux-kernel - Re: [PATCH] HID: Add support for 146b:0902 Bigben Interactive Kids' 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]

Message-ID: <729f18bc-65b7-9bfd-50ed-fbb1f168500d@hanno.de>
Date:   Tue, 29 May 2018 09:46:24 +0200
From:   Hanno Zulla <abos@...no.de>
To:     Roderick Colenbrander <thunderbird2k@...il.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input <linux-input@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: Add support for 146b:0902 Bigben Interactive Kids'
 gamepad

Hi Roderick,
Hi Benjamin,

thanks for the review!

> In terms of code style, I noticed a lot of C++ style comments which> are not part of the kernel code style instead use C comments. To
check> for potential other issues as well, run your patch through>
'scripts/checkpatch.pl'.

Okay, will check with that.

> I noticed you fixed up reported descriptors a bit to get axes remapped
> in a different way. This is reasonable as the default mappings are
> often not good. However, I would suggest use the mapping functions
> instead (e.g. see hid-sony.c and other drivers). It also allows you to
> properly remap buttons as well. I suspect the current button mapping
> is all over the place as well. Make sure axis and button mapping
> complies to the Linux gamepad spec (see
> Documentation/input/gamepad.rst).

The main reason for the fixup was to cleanup the output part of the
descriptor, as the device's original descriptor seemed to use
nonsensical/undefined "Usage" values.

Those two axis remappings were the only two necessary. The rest of the
mappings are fine and look okay when compared to other drivers that do
not fully adhere to the gamepad spec (e.g. xpad.c also doesn't map my
XBox-Controller's D-Pad to NORTH/EAST/SOUTH/WEST but instead to HAT0 in
the same way as this device does).

I had assumed that using a descriptor fixup and leaving the actual work
to the standard driver was cleaner than meddling with the incoming data
in a mapping function.

Actually, I was researching if I could fix the descriptor so that no
LED/FF-specific code is necessary, but my knowledge of HID was too
limited to achieve that.

> In addition to those comments, please also try to follow the
> submission guidelines

Ah, thank you.

> Also, keep point 9 in mind ("Don't get discouraged - or impatient").

Nah. Don't worry! :-D

Thanks & kind regards,

Hanno