linux-kernel - Re: [PATCH 1/2] HID: nintendo: fix face button mappings

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] [day] [month] [year] [list]

Message-ID: <CAKcX28X+fqzjW3UHqzhbopjqD=eO7x4TTAjC-3x3w7Po-PgKSQ@mail.gmail.com>
Date:   Thu, 26 May 2022 13:07:01 -0700
From:   Maxwell Fletcher <fletcher0max@...il.com>
To:     Roderick Colenbrander <thunderbird2k@...il.com>
Cc:     "Daniel J. Ogorchock" <djogorchock@...il.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] HID: nintendo: fix face button mappings

Hi Roderick,

Thank you so much for your time; this has all been very educational
for me. I'll be following the work on eBPF.

Best,
Max





On Sat, May 14, 2022 at 8:33 PM Roderick Colenbrander
<thunderbird2k@...il.com> wrote:
>
> Hi Maxwell,
>
> I don't think it is desired to have such kernel module parameters as
> it essentially adds new kernel APIs, which have to be maintained
> unless truly needed.
>
> However, you may have seen the work Benjamin is doing around eBPF for
> HID. I'm no expert on it yet, but it could probably allow you to do a
> similar kind of fixup without having to modify the kernel module.
>
> Thanks,
> Roderick
>
> On Sat, May 14, 2022 at 5:57 PM Maxwell Fletcher <fletcher0max@...il.com> wrote:
> >
> > Hi Roderick,
> >
> > Thanks for the explanation. It makes sense that the mappings were never meant to mirror Xbox controllers. Would it still be possible to merge a patch that adds an opt-in module parameter that changes the mappings, similar to the one in the second part of this patch?
> >
> > Thanks,
> > Max
> >
> > On Fri, May 13, 2022 at 12:58 PM Roderick Colenbrander <thunderbird2k@...il.com> wrote:
> >>
> >> Hi Max,
> >>
> >> Thanks for your patch, however I must say the patch is not correct for
> >> 2 reasons.
> >>
> >> Over the years different controllers have different layouts. The
> >> standard which this driver (as well as others such as
> >> hid-sony/hid-playstation) follow is the Linux gamepad standard (see
> >> Documentation/input/gamepad.rst). It stays away of the debate what is
> >> A/B/X/Y. It talks about North/west/.., (yes they are macros which map
> >> to A/B/X/Y). In case of the Switch it does mean things are flipped,
> >> but it was not meant to represent an Xbox controller. (Technically one
> >> could argue that the Xbox controller should be flipped as it was the
> >> SNES controller back in the days which introduced X/Y and the Switch
> >> is still consistent with that.)
> >>
> >> Second, even if the patch was right it would be tricky to merge. The
> >> problem is that a changed mapping breaks user spaces and in general
> >> can't do this unless there is a really good reason. It just would
> >> break existing applications and libraries (often e.g. SDL)
> >>
> >> Thanks,
> >> Roderick
> >>
> >> On Wed, May 11, 2022 at 8:12 PM Max Fletcher <fletcher0max@...il.com> wrote:
> >> >
> >> > Previously, A and B would match the Xbox layout, but X and Y were incorrectly swapped. This corrects it so that X and Y match the Xbox layout.
> >> >
> >> > Signed-off-by: Max Fletcher <fletcher0max@...il.com>
> >> > ---
> >> >  drivers/hid/hid-nintendo.c | 10 +++++-----
> >> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> >> > index 2204de889739..7735971ede3f 100644
> >> > --- a/drivers/hid/hid-nintendo.c
> >> > +++ b/drivers/hid/hid-nintendo.c
> >> > @@ -1351,10 +1351,10 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
> >> >                 input_report_key(dev, BTN_START, btns & JC_BTN_PLUS);
> >> >                 input_report_key(dev, BTN_THUMBR, btns & JC_BTN_RSTICK);
> >> >                 input_report_key(dev, BTN_MODE, btns & JC_BTN_HOME);
> >> > -               input_report_key(dev, BTN_WEST, btns & JC_BTN_Y);
> >> > -               input_report_key(dev, BTN_NORTH, btns & JC_BTN_X);
> >> > -               input_report_key(dev, BTN_EAST, btns & JC_BTN_A);
> >> > -               input_report_key(dev, BTN_SOUTH, btns & JC_BTN_B);
> >> > +               input_report_key(dev, BTN_X, btns & JC_BTN_Y);
> >> > +               input_report_key(dev, BTN_Y, btns & JC_BTN_X);
> >> > +               input_report_key(dev, BTN_B, btns & JC_BTN_A);
> >> > +               input_report_key(dev, BTN_A, btns & JC_BTN_B);
> >> >         }
> >> >
> >> >         input_sync(dev);
> >> > @@ -1578,7 +1578,7 @@ static const unsigned int joycon_button_inputs_l[] = {
> >> >
> >> >  static const unsigned int joycon_button_inputs_r[] = {
> >> >         BTN_START, BTN_MODE, BTN_THUMBR,
> >> > -       BTN_SOUTH, BTN_EAST, BTN_NORTH, BTN_WEST,
> >> > +       BTN_A, BTN_B, BTN_Y, BTN_X,
> >> >         BTN_TR, BTN_TR2,
> >> >         0 /* 0 signals end of array */
> >> >  };
> >> > --
> >> > 2.35.3
> >> >