Linux-ARM-Kernel Archive on lore.kernel.org

* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Andrzej Pietrasiewicz @ 2020-05-22 15:35 UTC (permalink / raw)
  To: Dmitry Torokhov, Peter Hutterer
  Cc: Nick Dyer, linux-iio, Benjamin Tissoires, platform-driver-x86,
	ibm-acpi-devel, Laxman Dewangan, Peter Meerwald-Stadler, kernel,
	Fabio Estevam, linux-samsung-soc, Krzysztof Kozlowski,
	Jonathan Hunter, linux-acpi, Kukjin Kim, NXP Linux Team,
	linux-input, Len Brown, Peter Hutterer, Michael Hennerich,
	Sascha Hauer, Sylvain Lemieux, Henrique de Moraes Holschuh,
	Vladimir Zapolskiy, Hans de Goede, Lars-Peter Clausen,
	linux-tegra, linux-arm-kernel, Barry Song, Ferruh Yigit, patches,
	Rafael J . Wysocki, Thierry Reding, Sangwon Jee,
	Pengutronix Kernel Team, Hartmut Knaack, Shawn Guo,
	Jonathan Cameron
In-Reply-To: <20200518024034.GL89269@dtor-ws>

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;
			}
		}

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply