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>,
	Hans de Goede <hdegoede@redhat.com>
Cc: 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: Tue, 2 Jun 2020 18:56:40 +0200
Message-ID: <88f939cd-1518-d516-59f2-8f627a6a70d2@collabora.com> (raw)
In-Reply-To: <20200527063430.GJ89269@dtor-ws>

Hi Dmitry,

W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
> On Tue, May 19, 2020 at 11:36:34AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 5/19/20 11:02 AM, Andrzej Pietrasiewicz wrote:
>>> Hi Hans, Hi Dmitry,
>>>
>>> W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
>>>> Hi,
>>>
>>> <snip>
>>>
>>>>>>>>
>>>>>>>> So I wonder what this series actually adds for functionality for
>>>>>>>> userspace which can not already be achieved this way?
>>>>>>>>
>>>>>>>> I also noticed that you keep the device open (do not call the
>>>>>>>> input_device's close callback) when inhibited and just throw away
>>>>>>>
>>>>>>> I'm not sure if I understand you correctly, it is called:
>>>>>>>
>>>>>>> +static inline void input_stop(struct input_dev *dev)
>>>>>>> +{
>>>>>>> +    if (dev->poller)
>>>>>>> +        input_dev_poller_stop(dev->poller);
>>>>>>> +    if (dev->close)
>>>>>>> +        dev->close(dev);
>>>>>>>                   ^^^^^^^^^^^^^^^^
>>>>>>> +static int input_inhibit(struct input_dev *dev)
>>>>>>> +{
>>>>>>> +    int ret = 0;
>>>>>>> +
>>>>>>> +    mutex_lock(&dev->mutex);
>>>>>>> +
>>>>>>> +    if (dev->inhibited)
>>>>>>> +        goto out;
>>>>>>> +
>>>>>>> +    if (dev->users) {
>>>>>>> +        if (dev->inhibit) {
>>>>>>> +            ret = dev->inhibit(dev);
>>>>>>> +            if (ret)
>>>>>>> +                goto out;
>>>>>>> +        }
>>>>>>> +        input_stop(dev);
>>>>>>>                   ^^^^^^^^^^^^^^^^
>>>>>>>
>>>>>>> It will not be called when dev->users is zero, but if it is zero,
>>>>>>> then nobody has opened the device yet so there is nothing to close.
>>>>>>
>>>>>> Ah, I missed that.
>>>>>>
>>>>>> So if the device implements the inhibit call back then on
>>>>>> inhibit it will get both the inhibit and close callback called?
>>>>>>
>>>>>
>>>>> That's right. And conversely, upon uninhibit open() and uninhibit()
>>>>> callbacks will be invoked. Please note that just as with open()/close(),
>>>>> providing inhibit()/uninhibit() is optional.
>>>>
>>>> Ack.
>>>>
>>>>>> And what happens if the last user goes away and the device
>>>>>> is not inhibited?
>>>>>
>>>>> close() is called as usually.
>>>>
>>>> But not inhibit, hmm, see below.
>>>>
>>>>>> I'm trying to understand here what the difference between the 2
>>>>>> is / what the goal of having a separate inhibit callback ?
>>>>>>
>>>>>
>>>>> Drivers have very different ideas about what it means to suspend/resume
>>>>> and open/close. The optional inhibit/uninhibit callbacks are meant for
>>>>> the drivers to know that it is this particular action going on.
>>>>
>>>> So the inhibit() callback triggers the "suspend" behavior ?
>>>> But shouldn't drivers which are capable of suspending the device
>>>> always do so on close() ?
>>>>
>>>> Since your current proposal also calls close() on inhibit() I
>>>> really see little difference between an inhibit() and the last
>>>> user of the device closing it and IMHO unless there is a good
>>>> reason to actually differentiate the 2 it would be better
>>>> to only stick with the existing close() and in cases where
>>>> that does not put the device in a low-power mode yet, fix
>>>> the existing close() callback to do the low-power mode
>>>> setting instead of adding a new callback.
>>>>
>>>>> For inhibit() there's one more argument: close() does not return a value,
>>>>> so its meaning is "do some last cleanup" and as such it is not allowed
>>>>> to fail - whatever its effect is, we must deem it successful. inhibit()
>>>>> does return a value and so it is allowed to fail.
>>>>
>>>> Well, we could make close() return an error and at least in the inhibit()
>>>> case propagate that to userspace. I wonder if userspace is going to
>>>> do anything useful with that error though...
> 
> It really can't do anything. Have you ever seen userspace handling
> errors from close()? And what can be done? A program is terminating, but
> the kernel says "no, you closing input device failed, you have to
> continue running indefinitely..."
> 
>>>>
>>>> In my experience errors during cleanup/shutdown are best logged
>>>> (using dev_err) and otherwise ignored, so that we try to clean up
>>>> as much possible. Unless the very first step of the shutdown process
>>>> fails the device is going to be in some twilight zone state anyways
>>>> at this point we might as well try to cleanup as much as possible.
>>>
>>> What you say makes sense to me.
>>> @Dmitry?
> 
> I will note here, that inhibit is closer to suspend() than to close(),
> and we do report errors for suspend(). Therefore we could conceivably
> try to handle errors if driver really wants to be fancy. But I think
> majority of cases will be quite happy with using close() and simply
> logging errors, as Hans said.
> 
> That said, I think the way we should handle inhibit/uninhibit, is that
> if we have the callback defined, then we call it, and only call open and
> close if uninhibit or inhibit are _not_ defined.
> 

If I understand you correctly you suggest to call either inhibit,
if provided or close, if inhibit is not provided, but not both,
that is, if both are provided then on the inhibit path only
inhibit is called. And, consequently, you suggest to call either
uninhibit or open, but not both. The rest of my mail makes this
assumption, so kindly confirm if I understand you correctly.

In my opinion this idea will not work.

The first question is should we be able to inhibit a device
which is not opened? In my opinion we should, in order to be
able to inhibit a device in anticipation without needing to
open it first.

Then what does opening (with input_open_device()) an inhibited
device mean? Should it succeed or should it fail? If it is not
the first opening then effectively it boils down to increasing
device's and handle's counters, so we can allow it to succeed.
If, however, the device is being opened for the first time,
the ->open() method wants to be called, but that somehow
contradicts the device's inhibited state. So a logical thing
to do is to either fail input_open_device() or postpone ->open()
invocation to the moment of uninhibiting - and the latter is
what the patches in this series currently do.

Failing input_open_device() because of the inhibited state is
not the right thing to do. Let me explain. Suppose that a device
is already inhibited and then a new matching handler appears
in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
any character devices (only evdev.c, joydev.c and mousedev.c do),
so for them it makes no sense to delay calling input_open_device()
and it is called in handler's ->connect(). If input_open_device()
now fails, we have lost the only chance for this ->connect() to
succeed.

Summarizing, IMO the uninhibit path should be calling both
->open() and ->uninhibit() (if provided), and conversely, the inhibit
path should be calling both ->inhibit() and ->close() (if provided).

What's your opinion?

Regards,

Andrzej

  reply index

Thread overview: 73+ 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
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
2020-06-02 16:56                   ` Andrzej Pietrasiewicz [this message]
2020-06-02 17:52                     ` Dmitry Torokhov
2020-06-02 18:50                       ` Andrzej Pietrasiewicz
2020-06-02 20:19                         ` Hans de Goede
2020-06-03 13:07                           ` Andrzej Pietrasiewicz
2020-06-03 17:38                             ` Hans de Goede
2020-06-03 17:54                               ` Andrzej Pietrasiewicz
2020-06-03 19:37                                 ` Hans de Goede
2020-06-04  7:28                                   ` Dmitry Torokhov
2020-06-05 17:33                                     ` [PATCH v3 " Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 2/7] Input: use input_device_enabled() Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 3/7] ACPI: button: Access input device's users under appropriate mutex Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 4/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 5/7] iio: adc: exynos: Use input_device_enabled() Andrzej Pietrasiewicz
2020-06-05 19:49                                         ` Michał Mirosław
2020-06-05 17:33                                       ` [PATCH v3 6/7] platform/x86: thinkpad_acpi: " Andrzej Pietrasiewicz
2020-06-05 17:33                                       ` [PATCH v3 7/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-06-05 17:41                                         ` Hans de Goede
2020-06-08 11:22                                           ` [PATCH v4 0/7] Support inhibiting input devices Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 1/7] Input: add input_device_enabled() Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 2/7] Input: use input_device_enabled() Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 3/7] ACPI: button: Access input device's users under appropriate mutex Andrzej Pietrasiewicz
2020-06-24 15:00                                               ` Rafael J. Wysocki
2020-06-25  5:23                                                 ` Dmitry Torokhov
2020-06-25 10:55                                                   ` Rafael J. Wysocki
2020-06-08 11:22                                             ` [PATCH v4 4/7] ACPI: button: Use input_device_enabled() helper Andrzej Pietrasiewicz
2020-06-25  5:24                                               ` Dmitry Torokhov
2020-06-08 11:22                                             ` [PATCH v4 5/7] iio: adc: exynos: Use input_device_enabled() Andrzej Pietrasiewicz
2020-06-10  1:28                                               ` Michał Mirosław
2020-06-10  7:52                                                 ` [FIXED PATCH " Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 6/7] platform/x86: thinkpad_acpi: " Andrzej Pietrasiewicz
2020-06-08 11:22                                             ` [PATCH v4 7/7] Input: Add "inhibited" property Andrzej Pietrasiewicz
2020-06-10  9:49                                             ` [PATCH v4 0/7] Support inhibiting input devices Hans de Goede
2020-06-10 10:38                                               ` Rafael J. Wysocki
2020-06-10 13:12                                                 ` Andrzej Pietrasiewicz
2020-06-10 13:21                                                   ` Hans de Goede
2020-06-10 13:41                                                     ` Andrzej Pietrasiewicz
2020-06-12  8:30                                                       ` Hans de Goede
2020-06-12  8:47                                                         ` Andrzej Pietrasiewicz
2020-06-16 17:29                                                         ` [PATCH] Input: document inhibiting Andrzej Pietrasiewicz
2020-06-16 17:38                                                           ` Randy Dunlap
2020-06-17  7:44                                                           ` Hans de Goede
2020-06-17 10:18                                                             ` [PATCH v2] " Andrzej Pietrasiewicz
2020-06-17 10:21                                                               ` Hans de Goede
2020-06-17 16:52                                                               ` Randy Dunlap
2020-06-23 13:35                                                               ` Pavel Machek
2020-06-10 14:01                                                     ` [PATCH v4 0/7] Support inhibiting input devices Rafael J. Wysocki
2020-06-10 13:52                                                 ` Hans de Goede
2020-06-10 18:28                                                 ` Dmitry Torokhov
2020-06-12  8:14                                                   ` Hans de Goede
2020-06-12  8:17                                             ` Hans de Goede
2020-08-03 14:40                                               ` Andrzej Pietrasiewicz
2020-06-07 20:24                                       ` [PATCH v3 " Pavel Machek
2020-06-08  5:37                                         ` Dmitry Torokhov
2020-06-08  9:28                                           ` Andrzej Pietrasiewicz

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=88f939cd-1518-d516-59f2-8f627a6a70d2@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=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