Linux-Samsung-soc Archive on lore.kernel.org
 help / color / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Andrzej Pietrasiewicz <andrzej.p@collabora.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
Cc: "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>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	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, 19 May 2020 11:36:34 +0200
Message-ID: <aa2ce2ab-e5bc-9cb4-8b53-c1ef9348b646@redhat.com> (raw)
In-Reply-To: <2d224833-3a7e-bc7c-af15-1f803f466697@collabora.com>

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...
>>
>> 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?
> 
>>
>>> All in all, it is up to the drivers to decide which callback they
>>> provide. Based on my work so far I would say that there are tens
>>> of simple cases where open() and close() are sufficient, out of total
>>> ~400 users of input_allocate_device():
>>>
>>> $ git grep "input_allocate_device(" | grep -v ^Documentation | \
>>> cut -f1 -d: | sort | uniq | wc
>>>      390     390   13496
>>
>> So can you explain a bit more about the cases where only having
>> open/close is not sufficient?  So far I have the feeling that
>> those are all we need and that we really do not need separate
>> [un]inhibit callbacks.
> 
> My primary concern was not being able to propagate inhibit() error
> to userspace, and then if we have inhibit(), uninhibit() should be
> there for completeness. If propagating the error to userspace can
> be neglected then yes, it seems open/close should be sufficient,
> even more because the real meaning of "open" is "prepare the device
> for generating input events".
> 
> To validate the idea of not introducing inhibit()/uninhibit() callbacks
> to implement device inhibiting/uninhibiting let's look at
> drivers/input/mouse/elan_i2c_core.c (PATCH 7/7):
> 
> static int elan_inhibit(struct input_dev *input)
> {
> [...]
> 
>      ret = mutex_lock_interruptible(&data->sysfs_mutex);
>      if (ret)
>          return ret;
> 
>      disable_irq(client->irq);
> 
>      ret = elan_disable_power(data);
>      if (ret)
>          enable_irq(client->irq);
> [...]
> }
> 
> First, close() does not exist in this driver. Of course this can be
> fixed. Then it doesn't return a value. Then, if either taking the
> mutex or disabling the power fails, the close() is still deemed
> successful. Is it ok?

Note I also mentioned another solution for the error propagation,
which would require a big "flag day" commit adding "return 0"
to all existing close callbacks, but otherwise should work for your
purposes:

 > 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...

And I guess we could log an error that close failed in the old close() path
where we cannot propagate the error.

Also why the mutex_lock_interruptible() ?  If you change that to
a normal mutex_lock() you loose one of the possible 2 error cases and
I doubt anyone is going to do a CTRL-C of the process doing the
inhibiting (or that that process starts a timer using a signal
to ensure the inhibit does not take to long or some such).

Regards,

Hans


  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 [this message]
2020-05-27  6:34                 ` Dmitry Torokhov
2020-06-02 16:56                   ` Andrzej Pietrasiewicz
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=aa2ce2ab-e5bc-9cb4-8b53-c1ef9348b646@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=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=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