From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Hutterer Subject: Re: [PATCHv2 0/7] Support inhibiting input devices Date: Wed, 27 May 2020 16:13:26 +1000 Message-ID: <20200527061326.GA531660@koala> References: <20200506002746.GB89269@dtor-ws> <20200515164943.28480-1-andrzej.p@collabora.com> <842b95bb-8391-5806-fe65-be64b02de122@redhat.com> <20200517225510.GA205823@koala> <20200518024034.GL89269@dtor-ws> <513f25c0-7125-c564-0090-052d626fe508@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <513f25c0-7125-c564-0090-052d626fe508@collabora.com> Sender: linux-acpi-owner@vger.kernel.org To: Andrzej Pietrasiewicz Cc: Dmitry Torokhov , Hans de Goede , 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" , Len Brown , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Kukjin Kim , Krzysztof Kozlowski , Shawn Guo , Sascha Hauer , Pengutronix List-Id: linux-tegra@vger.kernel.org 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, > > > > > > > > > > > > > > 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EDECAC433E0 for ; Wed, 27 May 2020 06:13:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BDE6B2075A for ; Wed, 27 May 2020 06:13:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=who-t.net header.i=@who-t.net header.b="myNaYPVb"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="04/epPm1" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725819AbgE0GNp (ORCPT ); Wed, 27 May 2020 02:13:45 -0400 Received: from new1-smtp.messagingengine.com ([66.111.4.221]:36275 "EHLO new1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725267AbgE0GNo (ORCPT ); Wed, 27 May 2020 02:13:44 -0400 Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id 136D858015D; Wed, 27 May 2020 02:13:43 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Wed, 27 May 2020 02:13:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=who-t.net; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:content-transfer-encoding:in-reply-to; s=fm3; bh=E b2Zi/zhilnTOPybbmMHYUfE4AbMTuPFBHFWd4pKlM0=; b=myNaYPVbSE/bsgXlo oh4J0u8+RKSeoWguDxEY1sniQ7H1CLC1DUviAOiluwRKnU+HbOGUuZQXNRH92lWn CP5Rzzec9LEJiCeN+Ff9mjBB0DtUDIXmTSEfL8YEB9+17DbglDtoNZZtLAE112b8 khUyKiJoGNVLDnna8guU4qC6viBLzWj+GpxHIo+zB7CnjHOCd0ZQIRhw5/7m9vY/ Yj7C3XWNAC5psU6dvjRIVURiZqOBz92Z36K206eTFf/bq/ifNgTTnjIbzBrZjg3q Tfkj4cn+eXDhKAyatRYD2Vos73K06V6H/Kfehk5y3ehIAXxgp+y+lvwrl/mj/r55 dRbVA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=Eb2Zi/zhilnTOPybbmMHYUfE4AbMTuPFBHFWd4pKl M0=; b=04/epPm1D2wOTZMoutIF3Vy4/qeeSFxcWJMBDeMEbBkgtQKqSRSnSMOuP /gSiDhieoS4U1l/io/57V91qWzaq94E9iqNG5x3+1uzOt1N8PZlrrvKTA5bwuEjK H/RJ21MzqkScKXfnSlFG8bF8IFtXLLyE4rVQD8vm9msqqG+whvogbzeAiN3PwFL3 2CtWwAJEkOcUp0UmJx2OZu777Ztq0MM9E1DeUE8TxovPqN70Sj/y8GztY3jPPxEg Gbb6eZYmGz8SDs5j6BMtrLPmn0BjaZ1obMh0ov1ilOQ1jXwUwoS4Nt26hseAk59H dw1Eo3YDvM6aQ9oVRWwcgHcTa4bjg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedruddvfedguddtudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggugfgjsehtkeertddttddunecuhfhrohhmpefrvght vghrucfjuhhtthgvrhgvrhcuoehpvghtvghrrdhhuhhtthgvrhgvrhesfihhohdqthdrnh gvtheqnecuggftrfgrthhtvghrnhephfegveefhfekgfdukeffgeefkeevueelueeiuedt gfejieeigeekjedugffgtdeknecukfhppeduudejrddvtddrieekrddufedvnecuvehluh hsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphgvthgvrhdrhhhu thhtvghrvghrseifhhhoqdhtrdhnvght X-ME-Proxy: Received: from koala (117-20-68-132.751444.bne.nbn.aussiebb.net [117.20.68.132]) by mail.messagingengine.com (Postfix) with ESMTPA id 7F09C3280060; Wed, 27 May 2020 02:13:30 -0400 (EDT) Date: Wed, 27 May 2020 16:13:26 +1000 From: Peter Hutterer To: Andrzej Pietrasiewicz Cc: Dmitry Torokhov , Hans de Goede , 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" , Len Brown , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Kukjin Kim , Krzysztof Kozlowski , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Vladimir Zapolskiy , Sylvain Lemieux , Laxman Dewangan , Thierry Reding , Jonathan Hunter , Barry Song , Michael Hennerich , Nick Dyer , Ferruh Yigit , Sangwon Jee , Henrique de Moraes Holschuh , kernel@collabora.com, Peter Hutterer , Benjamin Tissoires Subject: Re: [PATCHv2 0/7] Support inhibiting input devices Message-ID: <20200527061326.GA531660@koala> References: <20200506002746.GB89269@dtor-ws> <20200515164943.28480-1-andrzej.p@collabora.com> <842b95bb-8391-5806-fe65-be64b02de122@redhat.com> <20200517225510.GA205823@koala> <20200518024034.GL89269@dtor-ws> <513f25c0-7125-c564-0090-052d626fe508@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <513f25c0-7125-c564-0090-052d626fe508@collabora.com> Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org 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, > > > > > > > > > > > > > > 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BBA32C433E0 for ; Wed, 27 May 2020 06:13:56 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 802C72071A for ; Wed, 27 May 2020 06:13:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ZQKXWhhU"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=who-t.net header.i=@who-t.net header.b="myNaYPVb"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="04/epPm1" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 802C72071A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=who-t.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=WygN9Mtmmko+WMb4CLODuzufz8yKAQzeM2ls1kh5+eg=; b=ZQKXWhhUANLDZo aASWZRuUCP2o+L3spmKRkNsUbwljqKPNPo0+OOpa8qT5wW/zv3AKT8xjMFXjtfKAy+RVNhfxKsZpT Y5TP+h0yyG2X5JOpzcdHSEymO5qdCoF8qNG8tioFXq4HinwdI1vjbB8GM/t/ygy2WFytorf4lKBwZ mEU1dxzKpgZQHSmjKgGw+kRup1O89Jf3bLgK6ZZ9gFWx5RRys1jK5PnzqRFtk5hGJ+Aft1ZhMSWUC BheUauWQZVKrH7oxTf82WtEygod82W8dVeMdA3wyGjlWInRRVQ96F+mh5HaKe3Obyk4baocoSiAqz akXdqxHtjRH076VYkXhQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jdpK7-0005Hm-DK; Wed, 27 May 2020 06:13:55 +0000 Received: from new1-smtp.messagingengine.com ([66.111.4.221]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jdpK4-0005HM-De for linux-arm-kernel@lists.infradead.org; Wed, 27 May 2020 06:13:54 +0000 Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id 136D858015D; Wed, 27 May 2020 02:13:43 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Wed, 27 May 2020 02:13:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=who-t.net; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:content-transfer-encoding:in-reply-to; s=fm3; bh=E b2Zi/zhilnTOPybbmMHYUfE4AbMTuPFBHFWd4pKlM0=; b=myNaYPVbSE/bsgXlo oh4J0u8+RKSeoWguDxEY1sniQ7H1CLC1DUviAOiluwRKnU+HbOGUuZQXNRH92lWn CP5Rzzec9LEJiCeN+Ff9mjBB0DtUDIXmTSEfL8YEB9+17DbglDtoNZZtLAE112b8 khUyKiJoGNVLDnna8guU4qC6viBLzWj+GpxHIo+zB7CnjHOCd0ZQIRhw5/7m9vY/ Yj7C3XWNAC5psU6dvjRIVURiZqOBz92Z36K206eTFf/bq/ifNgTTnjIbzBrZjg3q Tfkj4cn+eXDhKAyatRYD2Vos73K06V6H/Kfehk5y3ehIAXxgp+y+lvwrl/mj/r55 dRbVA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=Eb2Zi/zhilnTOPybbmMHYUfE4AbMTuPFBHFWd4pKl M0=; b=04/epPm1D2wOTZMoutIF3Vy4/qeeSFxcWJMBDeMEbBkgtQKqSRSnSMOuP /gSiDhieoS4U1l/io/57V91qWzaq94E9iqNG5x3+1uzOt1N8PZlrrvKTA5bwuEjK H/RJ21MzqkScKXfnSlFG8bF8IFtXLLyE4rVQD8vm9msqqG+whvogbzeAiN3PwFL3 2CtWwAJEkOcUp0UmJx2OZu777Ztq0MM9E1DeUE8TxovPqN70Sj/y8GztY3jPPxEg Gbb6eZYmGz8SDs5j6BMtrLPmn0BjaZ1obMh0ov1ilOQ1jXwUwoS4Nt26hseAk59H dw1Eo3YDvM6aQ9oVRWwcgHcTa4bjg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedruddvfedguddtudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggugfgjsehtkeertddttddunecuhfhrohhmpefrvght vghrucfjuhhtthgvrhgvrhcuoehpvghtvghrrdhhuhhtthgvrhgvrhesfihhohdqthdrnh gvtheqnecuggftrfgrthhtvghrnhephfegveefhfekgfdukeffgeefkeevueelueeiuedt gfejieeigeekjedugffgtdeknecukfhppeduudejrddvtddrieekrddufedvnecuvehluh hsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphgvthgvrhdrhhhu thhtvghrvghrseifhhhoqdhtrdhnvght X-ME-Proxy: Received: from koala (117-20-68-132.751444.bne.nbn.aussiebb.net [117.20.68.132]) by mail.messagingengine.com (Postfix) with ESMTPA id 7F09C3280060; Wed, 27 May 2020 02:13:30 -0400 (EDT) Date: Wed, 27 May 2020 16:13:26 +1000 From: Peter Hutterer To: Andrzej Pietrasiewicz Subject: Re: [PATCHv2 0/7] Support inhibiting input devices Message-ID: <20200527061326.GA531660@koala> References: <20200506002746.GB89269@dtor-ws> <20200515164943.28480-1-andrzej.p@collabora.com> <842b95bb-8391-5806-fe65-be64b02de122@redhat.com> <20200517225510.GA205823@koala> <20200518024034.GL89269@dtor-ws> <513f25c0-7125-c564-0090-052d626fe508@collabora.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <513f25c0-7125-c564-0090-052d626fe508@collabora.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200526_231352_598400_F54BF6FC X-CRM114-Status: GOOD ( 27.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nick Dyer , linux-iio@vger.kernel.org, Benjamin Tissoires , platform-driver-x86@vger.kernel.org, ibm-acpi-devel@lists.sourceforge.net, Laxman Dewangan , Peter Meerwald-Stadler , kernel@collabora.com, Fabio Estevam , linux-samsung-soc@vger.kernel.org, Krzysztof Kozlowski , Jonathan Hunter , linux-acpi@vger.kernel.org, Kukjin Kim , NXP Linux Team , linux-input@vger.kernel.org, Len Brown , Peter Hutterer , Michael Hennerich , Sascha Hauer , Sylvain Lemieux , Henrique de Moraes Holschuh , Vladimir Zapolskiy , Hans de Goede , Lars-Peter Clausen , linux-tegra@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Barry Song , Ferruh Yigit , patches@opensource.cirrus.com, Dmitry Torokhov , "Rafael J . Wysocki" , Thierry Reding , Sangwon Jee , Pengutronix Kernel Team , Hartmut Knaack , Shawn Guo , Jonathan Cameron Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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=A004: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, > > > > = > = > > = > > > = > > > > 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 =3D=3D 2) { > disposition =3D INPUT_PASS_TO_HANDLERS; > break; > } > = > if (!!test_bit(code, dev->key) !=3D !!value) { > = > __change_bit(code, dev->key); > disposition =3D 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 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 !=3D 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 =3D=3D 2) { > + if (value =3D=3D 2 && test_bit(code, dev->key)) { > disposition =3D 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