Hi Andrzej,
On Fri, May 22, 2020 at 05:35:56PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Hans, hi Dmitry,
>
> W dniu 18.05.2020 o 04:40, Dmitry Torokhov pisze:
> > Hi Hans, Peter,
> >
> > On Mon, May 18, 2020 at 08:55:10AM +1000, Peter Hutterer wrote:
> > > On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
> > > > Hi Andrezj,
> > > >
>
> <snip>
>
> > >
> > > > I also noticed that you keep the device open (do not call the
> > > > input_device's close callback) when inhibited and just throw away
> > > > any events generated. This seems inefficient and may lead to
> > > > the internal state getting out of sync. What if a key is pressed
> > > > while inhibited and then the device is uninhibited while the key
> > > > is still pressed? Now the press event is lost and userspace
> > > > querying the current state will see the pressed key as being
> > > > released.
> >
> > This is a good point. We should look into signalling that some events
> > have been dropped (via EV_SYN/SYN_DROPPED) so that clients are aware of
> > it.
> >
>
> It seems to me that the situation Hans envisions is not possible,
> or will not be possible with a simple change. Let me explain.
>
> For a start, let's recall that the input core prevents consecutive
> events of the same kind (type _and_ code _and_ value) from being
> delivered to handlers. The decision is made in input_get_disposition().
> For EV_KEY it is:
>
> if (is_event_supported(code, dev->keybit, KEY_MAX)) {
>
> /* auto-repeat bypasses state updates */
> if (value == 2) {
> disposition = INPUT_PASS_TO_HANDLERS;
> break;
> }
>
> if (!!test_bit(code, dev->key) != !!value) {
>
> __change_bit(code, dev->key);
> disposition = INPUT_PASS_TO_HANDLERS;
> }
> }
note that this isn't per-process state, userspace can get release events
after open() for keys it never got the press event for. Simple test:
type evtest<enter> and KEY_ENTER up is the first event you'll get.
But otherwise I agree with you that press/release should always be balanced
if input_dev_release_keys() is called on inhibit and with that autorepeat
snippet below. At least I couldn't come up with any combination of multiple
clients opening/closing/inhibiting that resulted in an unwanted release
event after uninhibit.
Cheers,
Peter
> Let's now focus on value != 2 (events other than auto-repeat).
> The disposition changes from the default INPUT_IGNORE_EVENT to
> INPUT_PASS_TO_HANDLERS only when the event in question changes
> the current state: either by releasing a pressed key, or by
> pressing a released key. Subsequent releases of a released key
> or subsequent presses of a pressed key will be ignored.
>
> What Hans points out is the possibility of uninhibiting a device
> while its key is pressed and then releasing the key. First of all,
> during inhibiting input_dev_release_keys() is called, so input_dev's
> internal state will be cleared of all pressed keys. Then the device
> - after being uninhibited - all of a sudden produces a key release
> event. It will be ignored as per the "subsequent releases of a
> released key" case, so the handlers will not be passed an unmatched
> key release event. Assuming that passing an unmatched key release
> event was Hans's concern, in this case it seems impossible.
>
> Now, the value of 2 (auto-repeat) needs some attention. There are two
> cases to consider: the device uses input core's software repeat or it
> uses its own (hardware) repeat.
>
> Let's consider the first case. The timer which generates auto-repeat
> is only started on a key press event and only stopped on a key release
> event. As such, if any auto-repeat was in progress when inhibiting
> happened, it must have been stopped as per input_dev_release_keys().
> Then the key is pressed and held after the device has been inhibited,
> and the device is being uninhibited. Since it uses software auto-repeat,
> no events will be reported by the device until the key is released,
> and, as explained above, the release event will be ignored.
>
> Let's consider the second case. The key is pressed and held after the
> device has been inhibited and the device is being uninhibited. The worst
> thing that can happen is unmatched key repeat events will start coming
> from the device. We must prevent them from reaching the handlers and
> ignore them instead. So I suggest something on the lines of:
>
> if (is_event_supported(code, dev->keybit, KEY_MAX)) {
>
> /* auto-repeat bypasses state updates */
> - if (value == 2) {
> + if (value == 2 && test_bit(code, dev->key)) {
> disposition = INPUT_PASS_TO_HANDLERS;
> break;
> }
>
> The intended meaning is "ignore key repeat events if the key is not
> pressed".
>
> With this small change I believe it is not possible to have neither
> unmatched release nor unmatched repeat being delivered to handlers.
>
> Regards,
>
> Andrzej
