Linux-Samsung-soc Archive on lore.kernel.org
 help / color / Atom feed
From: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Peter Hutterer <peter.hutterer@who-t.net>
Cc: Hans de Goede <hdegoede@redhat.com>,
	linux-input@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, linux-tegra@vger.kernel.org,
	patches@opensource.cirrus.com,
	ibm-acpi-devel@lists.sourceforge.net,
	platform-driver-x86@vger.kernel.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>, Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Kukjin Kim <kgene@kernel.org>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Vladimir Zapolskiy <vz@mleia.com>,
	Sylvain Lemieux <slemieux.tyco@gmail.com>,
	Laxman Dewangan <ldewangan@nvidia.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Barry Song <baohua@kernel.org>,
	Michael Hennerich <michael.hennerich@analog.com>,
	Nick Dyer <nick@shmanahar.org>, Ferruh Yigit <fery@cypress.com>,
	Sangwon Jee <jeesw@melfas.com>,
	Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>,
	kernel@collabora.com, Peter Hutterer <peter.hutterer@redhat.com>,
	Benjamin Tissoires <btissoir@redhat.com>
Subject: Re: [PATCHv2 0/7] Support inhibiting input devices
Date: Fri, 22 May 2020 17:35:56 +0200
Message-ID: <513f25c0-7125-c564-0090-052d626fe508@collabora.com> (raw)
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

  parent reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200506002746.GB89269@dtor-ws>
2020-05-15 16:49 ` Andrzej Pietrasiewicz
2020-05-15 16:51   ` [PATCHv2 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-05-15 16:52   ` [PATCHv2 4/7] iio: adc: exynos: Use input_device_enabled() Andrzej Pietrasiewicz
2020-05-15 16:52   ` [PATCHv2 6/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-05-15 18:19   ` [PATCHv2 0/7] Support inhibiting input devices Hans de Goede
2020-05-17 22:55     ` Peter Hutterer
2020-05-18  2:40       ` Dmitry Torokhov
2020-05-18  7:36         ` Hans de Goede
2020-05-22 15:35         ` Andrzej Pietrasiewicz [this message]
2020-05-27  6:13           ` Peter Hutterer
2020-05-18 10:48     ` Andrzej Pietrasiewicz
2020-05-18 12:24       ` Hans de Goede
2020-05-18 13:49         ` Andrzej Pietrasiewicz
2020-05-18 14:23           ` Hans de Goede
2020-05-19  9:02             ` Andrzej Pietrasiewicz
2020-05-19  9:36               ` Hans de Goede
2020-05-27  6:34                 ` Dmitry Torokhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=513f25c0-7125-c564-0090-052d626fe508@collabora.com \
    --to=andrzej.p@collabora.com \
    --cc=baohua@kernel.org \
    --cc=btissoir@redhat.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=fery@cypress.com \
    --cc=festevam@gmail.com \
    --cc=hdegoede@redhat.com \
    --cc=ibm-acpi-devel@lists.sourceforge.net \
    --cc=ibm-acpi@hmh.eng.br \
    --cc=jeesw@melfas.com \
    --cc=jic23@kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=kernel@collabora.com \
    --cc=kernel@pengutronix.de \
    --cc=kgene@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=krzk@kernel.org \
    --cc=lars@metafoo.de \
    --cc=ldewangan@nvidia.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=michael.hennerich@analog.com \
    --cc=nick@shmanahar.org \
    --cc=patches@opensource.cirrus.com \
    --cc=peter.hutterer@redhat.com \
    --cc=peter.hutterer@who-t.net \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    --cc=rjw@rjwysocki.net \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=slemieux.tyco@gmail.com \
    --cc=thierry.reding@gmail.com \
    --cc=vz@mleia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Samsung-soc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-samsung-soc/0 linux-samsung-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-samsung-soc linux-samsung-soc/ https://lore.kernel.org/linux-samsung-soc \
		linux-samsung-soc@vger.kernel.org
	public-inbox-index linux-samsung-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-samsung-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git