Re: [PATCH] HID: apple: Fix stuck function keys when using FN

[JoÃo Moreno]

Hi Benjamin,

No worries, also pretty busy over here. Didn't mean to press.

On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> Hi JoÃo,
>
> On Sun, Jun 30, 2019 at 10:15 PM JoÃo Moreno <mail@xxxxxxxxxxxxxx> wrote:
> >
> > Hi Jiri & Benjamin,
> >
> > Let me know if you need something else to get this patch moving forward. This
> > fixes an issue I hit daily, it would be great to get it fixed.
>
> Sorry for the delay, I am very busy with internal corporate stuff, and
> I tried setting up a new CI system at home, and instead of spending a
> couple of ours, I am down to 2 weeks of hard work, without possibility
> to switch to the new right now :(
> Anyway.
>
> >
> > Thanks.
> >
> > On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@xxxxxxxxxxxxxx> wrote:
> > >
> > > This fixes an issue in which key down events for function keys would be
> > > repeatedly emitted even after the user has raised the physical key. For
> > > example, the driver fails to emit the F5 key up event when going through
> > > the following steps:
> > > - fnmode=1: hold FN, hold F5, release FN, release F5
> > > - fnmode=2: hold F5, hold FN, release F5, release FN
>
> Ouch :/
>

Right?!

> > >
> > > The repeated F5 key down events can be easily verified using xev.
> > >
> > > Signed-off-by: Joao Moreno <mail@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/hid/hid-apple.c | 21 +++++++++++----------
> > >  1 file changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> > > index 1cb41992aaa1..81867a6fa047 100644
> > > --- a/drivers/hid/hid-apple.c
> > > +++ b/drivers/hid/hid-apple.c
> > > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
> > >                 trans = apple_find_translation (table, usage->code);
> > >
> > >                 if (trans) {
> > > -                       if (test_bit(usage->code, asc->pressed_fn))
> > > -                               do_translate = 1;
> > > -                       else if (trans->flags & APPLE_FLAG_FKEY)
> > > -                               do_translate = (fnmode == 2 && asc->fn_on) ||
> > > -                                       (fnmode == 1 && !asc->fn_on);
> > > +                       int fn_on = value ? asc->fn_on :
> > > +                               test_bit(usage->code, asc->pressed_fn);
> > > +
> > > +                       if (!value)
> > > +                               clear_bit(usage->code, asc->pressed_fn);
> > > +                       else if (asc->fn_on)
> > > +                               set_bit(usage->code, asc->pressed_fn);
>
> I have the feeling that this is not the correct fix here.
>
> I might be wrong, but the following sequence might also mess up the
> driver state, depending on how the reports are emitted:
> - hold FN, hold F4, hold F5, release F4, release FN, release F5
>

I believe this should be fine. Following the code:

- hold FN, sets asc->fn_on to true
- hold F4, in the trans block fn_on will be true and we'll set the F4
bit in the bitmap
- hold F5, in the trans block fn_on will be true and we'll set the F5 bit
- release F4, in the trans block fn_on will be true (because of the bitmap) and
we'll clear the F4 bit
- release FN, asc->fn_on will be false, but it doesn't matter since...
- release F5, in the trans block we'll look into the bitmap (instead
of asc->fn_on),
so fn_on will be true and we'll clear the F5 bit

I tested it in practice using my changes:

Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is
pressed, seems like a hardware limitation. But F6 does work. So, when I execute
these events in that order, everything works as it should: xev reports
the following:

KeyPress F4
KeyPress F6
KeyRelease F4
KeyRelease F6

> The reason is that the driver only considers you have one key pressed
> with the modifier, and as the code changed its state based on the last
> value.
>

I believe the bitmap takes care of storing the FN state per key press. The
trick I did was to check on the global `asc->fn_on` state only when a key
is pressed, but check on the bitmap instead when it's released.

Let me know what you think. Am I missing something here?

Cheers,
JoÃo.

> IMO a better fix would:
>
> - keep the existing `trans` mapping lookout
> - whenever a `trans` mapping gets found:
>   * get both translated and non-translated currently reported values
> (`test_bit(keycode, input_dev->key)`)
>   * if one of them is set to true, then consider the keycode to be the
> one of the key (no matter fn_on)
>     -> deal with `value` with the corrected keycode
>   * if the key was not pressed:
>     -> chose the keycode based on `fn_on` and `fnmode` states
>     and report the key press event
>
> This should remove the nasty pressed_fn state which depends on the
> other pressed keys.
>
> Cheers,
> Benjamin
>
> > > +
> > > +                       if (trans->flags & APPLE_FLAG_FKEY)
> > > +                               do_translate = (fnmode == 2 && fn_on) ||
> > > +                                       (fnmode == 1 && !fn_on);
> > >                         else
> > >                                 do_translate = asc->fn_on;
> > >
> > >                         if (do_translate) {
> > > -                               if (value)
> > > -                                       set_bit(usage->code, asc->pressed_fn);
> > > -                               else
> > > -                                       clear_bit(usage->code, asc->pressed_fn);
> > > -
> > >                                 input_event(input, usage->type, trans->to,
> > >                                                 value);
> > >
> > > --
> > > 2.19.1
> > >