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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 May 2018 15:57:56 -0700
From:   Roderick Colenbrander <thunderbird2k@...il.com>
To:     Hanno Zulla <abos@...no.de>
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 Hanno,

A few suggestions for your patch based on a quick review (had limited time).

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'.

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).

Thanks,
Roderick

On Wed, May 23, 2018 at 8:57 AM, Hanno Zulla <abos@...no.de> wrote:
> Hello,
>
> this is a driver for the "BigBen Interactive Kid-friendly Wired
> Controller PS3OFMINIPAD SONY" [1] with USB id 146b:0902. It was
> originally sold as a PS3 accessory and serves as a very nice gamepad for
> Retropie.
>
> With the help of serialusb [2], it was possible to analyze the protocol
> used by the gamepad and fix the missing entries for the descriptor.
>
>
> This is my *first* driver, so please review accordingly.
>
>
> Three things where I'm not sure and hope to hear from you about:
>
> - is that the correct use of hid_validate_values() in bigben_probe()?
>
> - is the error handling in bigben_probe() correct?
>
> - how do I properly test possible suspend/resume scenarios?
>
> Thanks,
>
> Hanno
>
>
> [1]
> <https://www.bigben-interactive.co.uk/bigben-products/gaming-accessories/wired-mini-controller-ps3>
>
> [2] <https://github.com/matlo/serialusb>