[PATCHv2,0/7] Support inhibiting input devices
mbox series

Message ID 20200515164943.28480-1-andrzej.p@collabora.com
Headers show
Series
  • Support inhibiting input devices
Related show

Message

Andrzej Pietrasiewicz May 15, 2020, 4:49 p.m. UTC
Userspace might want to implement a policy to temporarily disregard input
from certain devices, including not treating them as wakeup sources.

An example use case is a laptop, whose keyboard can be folded under the
screen to create tablet-like experience. The user then must hold the laptop
in such a way that it is difficult to avoid pressing the keyboard keys. It
is therefore desirable to temporarily disregard input from the keyboard,
until it is folded back. This obviously is a policy which should be kept
out of the kernel, but the kernel must provide suitable means to implement
such a policy.

Due to interactions with suspend/resume, a helper has been added for drivers
to decide if the device is being used or not (PATCH 1/7) and it has been
applied to relevant drivers (PATCH 2-5/7). Patches 2-5 are only being sent
to relevant mailing lists and maintainers.

PATCH 6/7 adds support for inhibiting input devices, while PATCH 7/7
provides an example how to convert a driver to take advantage of this
new feature. Patch 7/7 is only being sent to input mailing list and
maintainer.

This work is inspired by:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/45c2d7bb398f74adfae0017e20b224152fde3822

and

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4ce0e8a3697edb8fd071110b3af65014512061c7

v1..v2:
- added input_device_enabled() helper and used it in drivers (Dmitry)
- the fact of open() and close() being called in inhibit/uninhibit paths has
been emphasized in the commit message of PATCH 6/7 (Dmitry)

Andrzej Pietrasiewicz (5):
  Input: add input_device_enabled()
  Input: use input_device_enabled()
  ACPI: button: Use input_device_enabled() helper
  iio: adc: exynos: Use input_device_enabled()
  platform/x86: thinkpad_acpi: Use input_device_enabled()

Dmitry Torokhov (1):
  Input: elan_i2c: Support inhibiting

Patrik Fimml (1):
  Input: Add "inhibited" property

 drivers/acpi/button.c                       |   8 +-
 drivers/iio/adc/exynos_adc.c                |  11 +-
 drivers/input/input.c                       | 142 ++++++++++++++++++--
 drivers/input/joystick/xpad.c               |   4 +-
 drivers/input/keyboard/ep93xx_keypad.c      |   2 +-
 drivers/input/keyboard/gpio_keys.c          |   4 +-
 drivers/input/keyboard/imx_keypad.c         |   4 +-
 drivers/input/keyboard/ipaq-micro-keys.c    |   2 +-
 drivers/input/keyboard/lpc32xx-keys.c       |   4 +-
 drivers/input/keyboard/pmic8xxx-keypad.c    |   4 +-
 drivers/input/keyboard/pxa27x_keypad.c      |   2 +-
 drivers/input/keyboard/samsung-keypad.c     |   4 +-
 drivers/input/keyboard/spear-keyboard.c     |   8 +-
 drivers/input/keyboard/st-keyscan.c         |   4 +-
 drivers/input/keyboard/tegra-kbc.c          |   4 +-
 drivers/input/misc/drv260x.c                |   4 +-
 drivers/input/misc/drv2665.c                |   4 +-
 drivers/input/misc/drv2667.c                |   4 +-
 drivers/input/misc/gp2ap002a00f.c           |   4 +-
 drivers/input/misc/kxtj9.c                  |   4 +-
 drivers/input/misc/sirfsoc-onkey.c          |   2 +-
 drivers/input/mouse/elan_i2c_core.c         | 112 +++++++++++----
 drivers/input/mouse/navpoint.c              |   4 +-
 drivers/input/touchscreen/ad7879.c          |   6 +-
 drivers/input/touchscreen/atmel_mxt_ts.c    |   4 +-
 drivers/input/touchscreen/auo-pixcir-ts.c   |   8 +-
 drivers/input/touchscreen/bu21029_ts.c      |   4 +-
 drivers/input/touchscreen/chipone_icn8318.c |   4 +-
 drivers/input/touchscreen/cyttsp_core.c     |   4 +-
 drivers/input/touchscreen/eeti_ts.c         |   4 +-
 drivers/input/touchscreen/ektf2127.c        |   4 +-
 drivers/input/touchscreen/imx6ul_tsc.c      |   4 +-
 drivers/input/touchscreen/ipaq-micro-ts.c   |   2 +-
 drivers/input/touchscreen/iqs5xx.c          |   4 +-
 drivers/input/touchscreen/lpc32xx_ts.c      |   4 +-
 drivers/input/touchscreen/melfas_mip4.c     |   4 +-
 drivers/input/touchscreen/mms114.c          |   6 +-
 drivers/input/touchscreen/pixcir_i2c_ts.c   |   8 +-
 drivers/input/touchscreen/ucb1400_ts.c      |   4 +-
 drivers/input/touchscreen/wm97xx-core.c     |  14 +-
 drivers/input/touchscreen/zforce_ts.c       |   8 +-
 drivers/platform/x86/thinkpad_acpi.c        |   4 +-
 include/linux/input.h                       |  10 ++
 43 files changed, 336 insertions(+), 119 deletions(-)


base-commit: 2ef96a5bb12be62ef75b5828c0aab838ebb29cb8

Comments

Hans de Goede May 15, 2020, 6:19 p.m. UTC | #1
Hi Andrezj,

On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
> Userspace might want to implement a policy to temporarily disregard input
> from certain devices, including not treating them as wakeup sources.
> 
> An example use case is a laptop, whose keyboard can be folded under the
> screen to create tablet-like experience. The user then must hold the laptop
> in such a way that it is difficult to avoid pressing the keyboard keys. It
> is therefore desirable to temporarily disregard input from the keyboard,
> until it is folded back. This obviously is a policy which should be kept
> out of the kernel, but the kernel must provide suitable means to implement
> such a policy.

Actually libinput already binds together (inside libinput) SW_TABLET_MODE
generating evdev nodes and e.g. internal keyboards on devices with 360°
hinges for this reason. libinput simply closes the /dev/input/event#
node when folded and re-opens it when the keyboard should become active
again. Thus not only suppresses events but allows e.g. touchpads to
enter runtime suspend mode which saves power. Typically closing the
/dev/input/event# node will also disable the device as wakeup source.

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

On top of this you add special inhibit and uninhibit callbacks
and implement those for just a few devices. How do these differ
from just closing the device and later opening it again ?

Also using a sysfs property for this is very weird given that the
rest of the evdev interface is using ioctls for everything...

So all in all I see a lot of question marks here and I think we
need to have a detailed discussion about what use-cases this
series tries to enable before moving forward with this.

Regards,

Hans
Peter Hutterer May 17, 2020, 10:55 p.m. UTC | #2
On Fri, May 15, 2020 at 08:19:10PM +0200, Hans de Goede wrote:
> Hi Andrezj,
> 
> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
> > Userspace might want to implement a policy to temporarily disregard input
> > from certain devices, including not treating them as wakeup sources.
> > 
> > An example use case is a laptop, whose keyboard can be folded under the
> > screen to create tablet-like experience. The user then must hold the laptop
> > in such a way that it is difficult to avoid pressing the keyboard keys. It
> > is therefore desirable to temporarily disregard input from the keyboard,
> > until it is folded back. This obviously is a policy which should be kept
> > out of the kernel, but the kernel must provide suitable means to implement
> > such a policy.
> 
> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
> generating evdev nodes and e.g. internal keyboards on devices with 360°
> hinges for this reason. libinput simply closes the /dev/input/event#
> node when folded and re-opens it when the keyboard should become active
> again. Thus not only suppresses events but allows e.g. touchpads to
> enter runtime suspend mode which saves power. Typically closing the
> /dev/input/event# node will also disable the device as wakeup source.
> 
> So I wonder what this series actually adds for functionality for
> userspace which can not already be achieved this way?

Thanks Hans. To expand on this:
libinput has heuristics to guess which input devices (keyboards, touchpads)
are built-in ones. When the tablet mode switch is on, we disable these
devices internally (this is not visible to callers), and re-enable it again
later when the tablet mode switch is off again.

This is done for keyboards and touchpads atm (and I think pointing sticks)
and where the heuristics fail we have extra quirks in place. For example
the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode
but buttons (e.g. volume keys) around the screen send events through the
same event node. So on those devices we don't disable the keyboard.

We've had this code for a few years now and the only changes to it have been
the various device quirks for devices that must not suspend the keyboard,
it's otherwise working as expected.

If we ever have a device where we need to disable parts of the keyboard
only, we could address this with EVIOCSMASK but so far that hasn't been
necessary.

I agree with Hans, right now I don't see the usefulness of this new sysfs
toggle. For it to be really useful you'd have to guarantee that it's
available for 100% of the devices and that's IMO unlikely to happen.

Cheers,
   Peter

> 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.
> 
> On top of this you add special inhibit and uninhibit callbacks
> and implement those for just a few devices. How do these differ
> from just closing the device and later opening it again ?
> 
> Also using a sysfs property for this is very weird given that the
> rest of the evdev interface is using ioctls for everything...
> 
> So all in all I see a lot of question marks here and I think we
> need to have a detailed discussion about what use-cases this
> series tries to enable before moving forward with this.
> 
> Regards,
> 
> Hans
>
Dmitry Torokhov May 18, 2020, 2:40 a.m. UTC | #3
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,
> > 
> > On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
> > > Userspace might want to implement a policy to temporarily disregard input
> > > from certain devices, including not treating them as wakeup sources.
> > > 
> > > An example use case is a laptop, whose keyboard can be folded under the
> > > screen to create tablet-like experience. The user then must hold the laptop
> > > in such a way that it is difficult to avoid pressing the keyboard keys. It
> > > is therefore desirable to temporarily disregard input from the keyboard,
> > > until it is folded back. This obviously is a policy which should be kept
> > > out of the kernel, but the kernel must provide suitable means to implement
> > > such a policy.
> > 
> > Actually libinput already binds together (inside libinput) SW_TABLET_MODE
> > generating evdev nodes and e.g. internal keyboards on devices with 360°
> > hinges for this reason. libinput simply closes the /dev/input/event#
> > node when folded and re-opens it when the keyboard should become active
> > again. Thus not only suppresses events but allows e.g. touchpads to
> > enter runtime suspend mode which saves power. Typically closing the
> > /dev/input/event# node will also disable the device as wakeup source.
> > 
> > So I wonder what this series actually adds for functionality for
> > userspace which can not already be achieved this way?
> 
> Thanks Hans. To expand on this:
> libinput has heuristics to guess which input devices (keyboards, touchpads)
> are built-in ones. When the tablet mode switch is on, we disable these
> devices internally (this is not visible to callers), and re-enable it again
> later when the tablet mode switch is off again.

I think that is great that libinput has tried solving this for the
tablet mode, but unfortunately libinput only works for users of
libinput, leaving cases such as:

1. In-kernel input handlers, such as SysRq, VT and others
2. Systems that do not rely on libinput for userspace handing (Android,
Chrome OS)
3. Systems with policies that are more complex than tablet mode only.

Because of libinput's inability to affect the kernel, and the presence
of "always on" input handlers (sysrq, VT keyboard, potentially others),
while libinput may control whether consumers receive events from certain
input devices, it will not allow power savings that an explicit
"inhibit" allows when coming from dedicated power policy manager.

I think pushing policy decisions into a library, and trying to have all
clients agree with it, is much harder and leaks unnecessary knowledge
into quite a few layers. A dedicated power policy manager, that is not
only responsible for input device, but power state of the system as a
whole, is a very viable architecture.

> 
> This is done for keyboards and touchpads atm (and I think pointing sticks)
> and where the heuristics fail we have extra quirks in place. For example
> the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode
> but buttons (e.g. volume keys) around the screen send events through the
> same event node. So on those devices we don't disable the keyboard.
> 
> We've had this code for a few years now and the only changes to it have been
> the various device quirks for devices that must not suspend the keyboard,
> it's otherwise working as expected.
> 
> If we ever have a device where we need to disable parts of the keyboard
> only, we could address this with EVIOCSMASK but so far that hasn't been
> necessary.
> 
> I agree with Hans, right now I don't see the usefulness of this new sysfs
> toggle. For it to be really useful you'd have to guarantee that it's
> available for 100% of the devices and that's IMO unlikely to happen.

The inhibiting of the events works for 100% of input devices, the power
savings work for the ones that implement it. It is responsibility of
folds shipping the systems to make sure drivers they use support inhibit
if they believe it will help their battery life.

> 
> Cheers,
>    Peter
> 
> > 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.

> > 
> > On top of this you add special inhibit and uninhibit callbacks
> > and implement those for just a few devices. How do these differ
> > from just closing the device and later opening it again ?

I believe majority will simply reuse open/close callbacks. In Chrome OS
we have dedicated inhibit/uninhibit, but I would like to allow using
open/close as alternatives.

> > 
> > Also using a sysfs property for this is very weird given that the
> > rest of the evdev interface is using ioctls for everything...

This is not evdev interface, it is at the level above evdev (so that it
can affect all handlers, not only evdev). As such it is not bound by
evdev interface.

Thanks.
Hans de Goede May 18, 2020, 7:36 a.m. UTC | #4
Hi,

On 5/18/20 4:40 AM, Dmitry Torokhov wrote:
> 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,
>>>
>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>>> Userspace might want to implement a policy to temporarily disregard input
>>>> from certain devices, including not treating them as wakeup sources.
>>>>
>>>> An example use case is a laptop, whose keyboard can be folded under the
>>>> screen to create tablet-like experience. The user then must hold the laptop
>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>>> is therefore desirable to temporarily disregard input from the keyboard,
>>>> until it is folded back. This obviously is a policy which should be kept
>>>> out of the kernel, but the kernel must provide suitable means to implement
>>>> such a policy.
>>>
>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>>> hinges for this reason. libinput simply closes the /dev/input/event#
>>> node when folded and re-opens it when the keyboard should become active
>>> again. Thus not only suppresses events but allows e.g. touchpads to
>>> enter runtime suspend mode which saves power. Typically closing the
>>> /dev/input/event# node will also disable the device as wakeup source.
>>>
>>> So I wonder what this series actually adds for functionality for
>>> userspace which can not already be achieved this way?
>>
>> Thanks Hans. To expand on this:
>> libinput has heuristics to guess which input devices (keyboards, touchpads)
>> are built-in ones. When the tablet mode switch is on, we disable these
>> devices internally (this is not visible to callers), and re-enable it again
>> later when the tablet mode switch is off again.
> 
> I think that is great that libinput has tried solving this for the
> tablet mode, but unfortunately libinput only works for users of
> libinput, leaving cases such as:
> 
> 1. In-kernel input handlers, such as SysRq, VT and others
> 2. Systems that do not rely on libinput for userspace handing (Android,
> Chrome OS)
> 3. Systems with policies that are more complex than tablet mode only.
> 
> Because of libinput's inability to affect the kernel, and the presence
> of "always on" input handlers (sysrq, VT keyboard, potentially others),
> while libinput may control whether consumers receive events from certain
> input devices, it will not allow power savings that an explicit
> "inhibit" allows when coming from dedicated power policy manager.

Ok, the sysrq and vt keyboard handlers keeping the device open and thus
make it keep using power is a valid reason for a separate inhibit mechanism.

> I think pushing policy decisions into a library, and trying to have all
> clients agree with it, is much harder and leaks unnecessary knowledge
> into quite a few layers. A dedicated power policy manager, that is not
> only responsible for input device, but power state of the system as a
> whole, is a very viable architecture.

Well AFAIK the kernel-policy has always been to leave policy decisions
up to userspace as much as possible, but this just adds a mechanism to
implement the policy so that is fine.

>> This is done for keyboards and touchpads atm (and I think pointing sticks)
>> and where the heuristics fail we have extra quirks in place. For example
>> the Lenovo Yogas tend to disable the keyboard mechanically in tablet mode
>> but buttons (e.g. volume keys) around the screen send events through the
>> same event node. So on those devices we don't disable the keyboard.
>>
>> We've had this code for a few years now and the only changes to it have been
>> the various device quirks for devices that must not suspend the keyboard,
>> it's otherwise working as expected.
>>
>> If we ever have a device where we need to disable parts of the keyboard
>> only, we could address this with EVIOCSMASK but so far that hasn't been
>> necessary.
>>
>> I agree with Hans, right now I don't see the usefulness of this new sysfs
>> toggle. For it to be really useful you'd have to guarantee that it's
>> available for 100% of the devices and that's IMO unlikely to happen.
> 
> The inhibiting of the events works for 100% of input devices, the power
> savings work for the ones that implement it. It is responsibility of
> folds shipping the systems to make sure drivers they use support inhibit
> if they believe it will help their battery life.
> 
>>
>> Cheers,
>>     Peter
>>
>>> 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.
> 
>>>
>>> On top of this you add special inhibit and uninhibit callbacks
>>> and implement those for just a few devices. How do these differ
>>> from just closing the device and later opening it again ?
> 
> I believe majority will simply reuse open/close callbacks. In Chrome OS
> we have dedicated inhibit/uninhibit, but I would like to allow using
> open/close as alternatives.

Ack, maybe some driver flag to just call close on inhibit and
open on unhibit (also taking input_device.users into account of course) ?

>>> Also using a sysfs property for this is very weird given that the
>>> rest of the evdev interface is using ioctls for everything...
> 
> This is not evdev interface, it is at the level above evdev (so that it
> can affect all handlers, not only evdev). As such it is not bound by
> evdev interface.

Ok I can see how on some systems the process implementing the policy
of when to inhibit would be separate from the process which has the
device open. But in e.g. the libinput case it would be good if
libinput could activate any potential power-savings by setting
the inhibit flag.

The problem with sysfs interfaces is that they typically require
root rights and that they are not really compatible with FD
passing. libinput runs as a normal user, getting a fd to the
/dev/input/event# node passed by systemd-logind.

As said I can see the reason for wanting a sysfs attribute for
this, can we perhaps have both a sysfs interface and an ioctl?

Note both could share the same boolean in the kernel, it would be
up to userspace to not try and write to both. E.g. chrome-os
would use the sysfs attr, libinput would use the ioctl.

Regards,

Hans
Andrzej Pietrasiewicz May 18, 2020, 10:48 a.m. UTC | #5
Hi Hans,

W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
> Hi Andrezj,
> 
> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>> Userspace might want to implement a policy to temporarily disregard input
>> from certain devices, including not treating them as wakeup sources.
>>
>> An example use case is a laptop, whose keyboard can be folded under the
>> screen to create tablet-like experience. The user then must hold the laptop
>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>> is therefore desirable to temporarily disregard input from the keyboard,
>> until it is folded back. This obviously is a policy which should be kept
>> out of the kernel, but the kernel must provide suitable means to implement
>> such a policy.
> 
> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
> generating evdev nodes and e.g. internal keyboards on devices with 360°
> hinges for this reason. libinput simply closes the /dev/input/event#
> node when folded and re-opens it when the keyboard should become active
> again. Thus not only suppresses events but allows e.g. touchpads to
> enter runtime suspend mode which saves power. Typically closing the
> /dev/input/event# node will also disable the device as wakeup source.
> 
> 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.

Andrzej
Hans de Goede May 18, 2020, 12:24 p.m. UTC | #6
Hi,

On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans,
> 
> W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
>> Hi Andrezj,
>>
>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>> Userspace might want to implement a policy to temporarily disregard input
>>> from certain devices, including not treating them as wakeup sources.
>>>
>>> An example use case is a laptop, whose keyboard can be folded under the
>>> screen to create tablet-like experience. The user then must hold the laptop
>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>> is therefore desirable to temporarily disregard input from the keyboard,
>>> until it is folded back. This obviously is a policy which should be kept
>>> out of the kernel, but the kernel must provide suitable means to implement
>>> such a policy.
>>
>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>> hinges for this reason. libinput simply closes the /dev/input/event#
>> node when folded and re-opens it when the keyboard should become active
>> again. Thus not only suppresses events but allows e.g. touchpads to
>> enter runtime suspend mode which saves power. Typically closing the
>> /dev/input/event# node will also disable the device as wakeup source.
>>
>> 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?

And what happens if the last user goes away and the device
is not inhibited?

I'm trying to understand here what the difference between the 2
is / what the goal of having a separate inhibit callback ?

IOW is there something which we want to do on close when
the close is being done to inhibit the device, which we do
not want to do on a normal close ?

Regards,

Hans
Andrzej Pietrasiewicz May 18, 2020, 1:49 p.m. UTC | #7
Hi Hans,

W dniu 18.05.2020 o 14:24, Hans de Goede pisze:
> Hi,
> 
> On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote:
>> Hi Hans,
>>
>> W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
>>> Hi Andrezj,
>>>
>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>>> Userspace might want to implement a policy to temporarily disregard input
>>>> from certain devices, including not treating them as wakeup sources.
>>>>
>>>> An example use case is a laptop, whose keyboard can be folded under the
>>>> screen to create tablet-like experience. The user then must hold the laptop
>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>>> is therefore desirable to temporarily disregard input from the keyboard,
>>>> until it is folded back. This obviously is a policy which should be kept
>>>> out of the kernel, but the kernel must provide suitable means to implement
>>>> such a policy.
>>>
>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>>> hinges for this reason. libinput simply closes the /dev/input/event#
>>> node when folded and re-opens it when the keyboard should become active
>>> again. Thus not only suppresses events but allows e.g. touchpads to
>>> enter runtime suspend mode which saves power. Typically closing the
>>> /dev/input/event# node will also disable the device as wakeup source.
>>>
>>> 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.

> And what happens if the last user goes away and the device
> is not inhibited?

close() is called as usually.

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

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.

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

Andrzej
Hans de Goede May 18, 2020, 2:23 p.m. UTC | #8
Hi,

On 5/18/20 3:49 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans,
> 
> W dniu 18.05.2020 o 14:24, Hans de Goede pisze:
>> Hi,
>>
>> On 5/18/20 12:48 PM, Andrzej Pietrasiewicz wrote:
>>> Hi Hans,
>>>
>>> W dniu 15.05.2020 o 20:19, Hans de Goede pisze:
>>>> Hi Andrezj,
>>>>
>>>> On 5/15/20 6:49 PM, Andrzej Pietrasiewicz wrote:
>>>>> Userspace might want to implement a policy to temporarily disregard input
>>>>> from certain devices, including not treating them as wakeup sources.
>>>>>
>>>>> An example use case is a laptop, whose keyboard can be folded under the
>>>>> screen to create tablet-like experience. The user then must hold the laptop
>>>>> in such a way that it is difficult to avoid pressing the keyboard keys. It
>>>>> is therefore desirable to temporarily disregard input from the keyboard,
>>>>> until it is folded back. This obviously is a policy which should be kept
>>>>> out of the kernel, but the kernel must provide suitable means to implement
>>>>> such a policy.
>>>>
>>>> Actually libinput already binds together (inside libinput) SW_TABLET_MODE
>>>> generating evdev nodes and e.g. internal keyboards on devices with 360°
>>>> hinges for this reason. libinput simply closes the /dev/input/event#
>>>> node when folded and re-opens it when the keyboard should become active
>>>> again. Thus not only suppresses events but allows e.g. touchpads to
>>>> enter runtime suspend mode which saves power. Typically closing the
>>>> /dev/input/event# node will also disable the device as wakeup source.
>>>>
>>>> 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.

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

Regards,

Hans
Andrzej Pietrasiewicz May 19, 2020, 9:02 a.m. UTC | #9
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?
@Dmitry?

Regards,

Andrzej
Hans de Goede May 19, 2020, 9:36 a.m. UTC | #10
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
Andrzej Pietrasiewicz May 22, 2020, 3:35 p.m. UTC | #11
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
Peter Hutterer May 27, 2020, 6:13 a.m. UTC | #12
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,
> > > > 
> 
> <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;
> 			}
> 		}

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<enter> 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
Dmitry Torokhov May 27, 2020, 6:34 a.m. UTC | #13
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.

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

No, please, no flag days and no changing close() to return error, it
makes no sense for close().

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

Well, we have the dedicated callbacks in Chrome OS, so when I did the
patch I could even handle Ctrl-C, so why not? But it indeed can easily
be dropped in favor of straight mutex_lock().

Thanks.
Andrzej Pietrasiewicz June 2, 2020, 4:56 p.m. UTC | #14
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
Dmitry Torokhov June 2, 2020, 5:52 p.m. UTC | #15
Hi Andrzej,

On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Dmitry,
> 
> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
> > 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.

Yes, that is correct. If a driver wants really fine-grained control, it
will provide inhibit (or both inhibit and close), otherwise it will rely
on close in place of inhibit.

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

I agree.

> 
> Then what does opening (with input_open_device()) an inhibited
> device mean? Should it succeed or should it fail?

It should succeed.

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

So what you are trying to say is that you see inhibit as something that
is done in addition to what happens in close. But what exactly do you
want to do in inhibit, in addition to what close is doing?

In my view, if we want to have a dedicated inhibit callback, then it
will do everything that close does, they both are aware of each other
and can sort out the state transitions between them. For drivers that do
not have dedicated inhibit/uninhibit, we can use open and close
handlers, and have input core sort out when each should be called. That
means that we should not call dev->open() in input_open_device() when
device is inhibited (and same for dev->close() in input_close_device).
And when uninhibiting, we should not call dev->open() when there are no
users for the device, and no dev->close() when inhibiting with no users.

Do you see any problems with this approach?

Thanks.
Andrzej Pietrasiewicz June 2, 2020, 6:50 p.m. UTC | #16
Hi Dmitry,

W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:
> Hi Andrzej,
> 
> On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
>> Hi Dmitry,
>>
>> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
>>> 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.
> 
> Yes, that is correct. If a driver wants really fine-grained control, it
> will provide inhibit (or both inhibit and close), otherwise it will rely
> on close in place of inhibit.
> 
>>
>> 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.
> 
> I agree.
> 
>>
>> Then what does opening (with input_open_device()) an inhibited
>> device mean? Should it succeed or should it fail?
> 
> It should succeed.
> 
>> 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).
> 
> So what you are trying to say is that you see inhibit as something that
> is done in addition to what happens in close. But what exactly do you
> want to do in inhibit, in addition to what close is doing?

See below (*).

> 
> In my view, if we want to have a dedicated inhibit callback, then it
> will do everything that close does, they both are aware of each other
> and can sort out the state transitions between them. For drivers that do
> not have dedicated inhibit/uninhibit, we can use open and close
> handlers, and have input core sort out when each should be called. That
> means that we should not call dev->open() in input_open_device() when
> device is inhibited (and same for dev->close() in input_close_device).
> And when uninhibiting, we should not call dev->open() when there are no
> users for the device, and no dev->close() when inhibiting with no users.
> 
> Do you see any problems with this approach?

My concern is that if e.g. both ->open() and ->uninhibit() are provided,
then in certain circumstances ->open() won't be called:

1. users == 0
2. inhibit happens
3. input_open_device() happens, ->open() not called
4. uninhibit happens
5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.

They way I understand your answer is that we implicitly impose requirements
on drivers which choose to implement e.g. both ->open() and ->uninhibit():
in such a case ->uninhibit() should be doing exactly the same things as
->open() does. Which leads to a conclusion that in practice no drivers
should choose to implement both, otherwise they must be aware that
->uninhibit() can be sometimes called instead of ->open(). Then ->open()
becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
Or, maybe, then ->inhibit() can be a superset of ->close() and
->uninhibit() a superset of ->open().

If such an approach is ok with you, it is ok with me, too.

(*)
Calling both ->inhibit() and ->close() (if they are provided) allows
drivers to go fancy and fail inhibiting (which is impossible using
only ->close() as it does not return a value, but ->inhibit() by design
does). Then ->uninhibit() is mostly for symmetry.

Regards,

Andrzej
Hans de Goede June 2, 2020, 8:19 p.m. UTC | #17
Hi,

On 6/2/20 8:50 PM, Andrzej Pietrasiewicz wrote:
> Hi Dmitry,
> 
> W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:
>> Hi Andrzej,
>>
>> On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
>>> Hi Dmitry,
>>>
>>> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
>>>> 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.
>>
>> Yes, that is correct. If a driver wants really fine-grained control, it
>> will provide inhibit (or both inhibit and close), otherwise it will rely
>> on close in place of inhibit.
>>
>>>
>>> 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.
>>
>> I agree.
>>
>>>
>>> Then what does opening (with input_open_device()) an inhibited
>>> device mean? Should it succeed or should it fail?
>>
>> It should succeed.
>>
>>> 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).
>>
>> So what you are trying to say is that you see inhibit as something that
>> is done in addition to what happens in close. But what exactly do you
>> want to do in inhibit, in addition to what close is doing?
> 
> See below (*).
> 
>>
>> In my view, if we want to have a dedicated inhibit callback, then it
>> will do everything that close does, they both are aware of each other
>> and can sort out the state transitions between them. For drivers that do
>> not have dedicated inhibit/uninhibit, we can use open and close
>> handlers, and have input core sort out when each should be called. That
>> means that we should not call dev->open() in input_open_device() when
>> device is inhibited (and same for dev->close() in input_close_device).
>> And when uninhibiting, we should not call dev->open() when there are no
>> users for the device, and no dev->close() when inhibiting with no users.
>>
>> Do you see any problems with this approach?
> 
> My concern is that if e.g. both ->open() and ->uninhibit() are provided,
> then in certain circumstances ->open() won't be called:
> 
> 1. users == 0
> 2. inhibit happens
> 3. input_open_device() happens, ->open() not called
> 4. uninhibit happens
> 5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.
> 
> They way I understand your answer is that we implicitly impose requirements
> on drivers which choose to implement e.g. both ->open() and ->uninhibit():
> in such a case ->uninhibit() should be doing exactly the same things as
> ->open() does. Which leads to a conclusion that in practice no drivers
> should choose to implement both, otherwise they must be aware that
> ->uninhibit() can be sometimes called instead of ->open(). Then ->open()
> becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
> Or, maybe, then ->inhibit() can be a superset of ->close() and
> ->uninhibit() a superset of ->open().
> 
> If such an approach is ok with you, it is ok with me, too.
> 
> (*)
> Calling both ->inhibit() and ->close() (if they are provided) allows
> drivers to go fancy and fail inhibiting (which is impossible using
> only ->close() as it does not return a value, but ->inhibit() by design
> does). Then ->uninhibit() is mostly for symmetry.

All the complications discussed above are exactly why I still
believe that there should be only open and close.

If error propagation on inhibit is considered as something
really important to have then we can make the input driver close
callback return an error (*), note I'm talking about the
driver close callback here, not the system call.

If the close callback is called for actually closing the fd
referring to the input node, then the new error return code
can be ignored, as we already do for errors on close atm
since the driver close callback returns void.

I still have not seen a very convincing argument for having
separate inhibit and close callbacks and as the messy discussion
above shows, having 2 such very similar yet subtly different
calls seems like a bad idea...

Regards,

Hans


*) This will require a flag day where "return 0" is added
to all current close handlers
Andrzej Pietrasiewicz June 3, 2020, 1:07 p.m. UTC | #18
Hi Hans, hi Dmitry,

W dniu 02.06.2020 o 22:19, Hans de Goede pisze:
> Hi,
> 
> On 6/2/20 8:50 PM, Andrzej Pietrasiewicz wrote:
>> Hi Dmitry,
>>
>> W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:
>>> Hi Andrzej,
>>>
>>> On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
>>>> Hi Dmitry,
>>>>
>>>> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
>>>>> 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.
>>>
>>> Yes, that is correct. If a driver wants really fine-grained control, it
>>> will provide inhibit (or both inhibit and close), otherwise it will rely
>>> on close in place of inhibit.
>>>
>>>>
>>>> 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.
>>>
>>> I agree.
>>>
>>>>
>>>> Then what does opening (with input_open_device()) an inhibited
>>>> device mean? Should it succeed or should it fail?
>>>
>>> It should succeed.
>>>
>>>> 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).
>>>
>>> So what you are trying to say is that you see inhibit as something that
>>> is done in addition to what happens in close. But what exactly do you
>>> want to do in inhibit, in addition to what close is doing?
>>
>> See below (*).
>>
>>>
>>> In my view, if we want to have a dedicated inhibit callback, then it
>>> will do everything that close does, they both are aware of each other
>>> and can sort out the state transitions between them. For drivers that do
>>> not have dedicated inhibit/uninhibit, we can use open and close
>>> handlers, and have input core sort out when each should be called. That
>>> means that we should not call dev->open() in input_open_device() when
>>> device is inhibited (and same for dev->close() in input_close_device).
>>> And when uninhibiting, we should not call dev->open() when there are no
>>> users for the device, and no dev->close() when inhibiting with no users.
>>>
>>> Do you see any problems with this approach?
>>
>> My concern is that if e.g. both ->open() and ->uninhibit() are provided,
>> then in certain circumstances ->open() won't be called:
>>
>> 1. users == 0
>> 2. inhibit happens
>> 3. input_open_device() happens, ->open() not called
>> 4. uninhibit happens
>> 5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.
>>
>> They way I understand your answer is that we implicitly impose requirements
>> on drivers which choose to implement e.g. both ->open() and ->uninhibit():
>> in such a case ->uninhibit() should be doing exactly the same things as
>> ->open() does. Which leads to a conclusion that in practice no drivers
>> should choose to implement both, otherwise they must be aware that
>> ->uninhibit() can be sometimes called instead of ->open(). Then ->open()
>> becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
>> Or, maybe, then ->inhibit() can be a superset of ->close() and
>> ->uninhibit() a superset of ->open().
>>
>> If such an approach is ok with you, it is ok with me, too.
>>
>> (*)
>> Calling both ->inhibit() and ->close() (if they are provided) allows
>> drivers to go fancy and fail inhibiting (which is impossible using
>> only ->close() as it does not return a value, but ->inhibit() by design
>> does). Then ->uninhibit() is mostly for symmetry.
> 
> All the complications discussed above are exactly why I still
> believe that there should be only open and close.
> 
> If error propagation on inhibit is considered as something
> really important to have then we can make the input driver close
> callback return an error (*), note I'm talking about the
> driver close callback here, not the system call.
> 
> If the close callback is called for actually closing the fd
> referring to the input node, then the new error return code
> can be ignored, as we already do for errors on close atm
> since the driver close callback returns void.
> 
> I still have not seen a very convincing argument for having
> separate inhibit and close callbacks and as the messy discussion
> above shows, having 2 such very similar yet subtly different
> calls seems like a bad idea...
> 
> Regards,
> 
> Hans
> 
> 
> *) This will require a flag day where "return 0" is added
> to all current close handlers
> 

I'm taking one step back and looking at the ->open() and ->close()
driver callbacks. They are called from input_open_device() and
input_close_device(), respectively:

input_open_device():
"This function should be called by input handlers when they
want to start receive events from given input device."

->open() callback:
"this method is called when the very first user calls
input_open_device(). The driver must prepare the device to start
generating events (start polling thread, request an IRQ, submit
URB, etc.)"

input_close_device():
"This function should be called by input handlers when they
want to stop receive events from given input device."

->close() callback:
"this method is called when the very last user calls
input_close_device()"

It seems to me that the callback names do not reflect their
purpose: their meaning is not to "open" or to "close" but to
give drivers a chance to control when they start or stop
providing events to the input core.

What would you say about changing the callbacks' names?
I'd envsion: ->provide_events() instead of ->open() and
->stop_events() instead of ->close(). Of course drivers can
exploit the fact of knowing that nobody wants any events
from them and do whatever they consider appropriate, for
example go into a low power mode - but the latter is beyond
the scope of the input subsystem and is driver-specific.

With such a naming change in mind let's consider inhibiting.
We want to be able to control when to disregard events from
a given device. It makes sense to do it at device level, otherwise
such an operation would have to be invoked in all associated
handlers (those that have an open handle associating them with
the device in question). But of course we can do better than
merely ignoring the events received: we can tell the drivers
that we don't want any events from them, and later, at uninhibit
time, tell them to start providing the events again. Conceptually,
the two operations (provide or don't provide envents) are exactly
the same thing we want to be happening at input_open_device() and
input_close_device() time. To me, changing the names of
->open() and ->close() exposes this fact very well.

Consequently, ->inhibit() and ->uninhibit() won't be needed,
and drivers which already implement ->provide_events() (formerly
->open()) and ->stop_events() (formerly ->close()) will receive
full inhibit/uninhibit support for free (subject to how well they
implement ->provide_events()/->stop_events()). Unless we can come
up with what the drivers might be doing on top of ->stop_events()
and ->provide_events() when inhibiting/uninhibiting, but it seems
to me we can't. Can we?

Optionally ->close() (only the callback, not input_close_device())
can be made return a value, just as Hans suggests. The value
can be ignored in input_close_device() but used in input_inhibit().
No strong opinion here, though. (btw it seems to me that
input_inhibit() should be renamed to input_inhibit_device()).

Regards,

Andrzej
Hans de Goede June 3, 2020, 5:38 p.m. UTC | #19
Hi,

On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans, hi Dmitry,

<snip>

> I'm taking one step back and looking at the ->open() and ->close()
> driver callbacks. They are called from input_open_device() and
> input_close_device(), respectively:
> 
> input_open_device():
> "This function should be called by input handlers when they
> want to start receive events from given input device."
> 
> ->open() callback:
> "this method is called when the very first user calls
> input_open_device(). The driver must prepare the device to start
> generating events (start polling thread, request an IRQ, submit
> URB, etc.)"
> 
> input_close_device():
> "This function should be called by input handlers when they
> want to stop receive events from given input device."
> 
> ->close() callback:
> "this method is called when the very last user calls
> input_close_device()"
> 
> It seems to me that the callback names do not reflect their
> purpose: their meaning is not to "open" or to "close" but to
> give drivers a chance to control when they start or stop
> providing events to the input core.
> 
> What would you say about changing the callbacks' names?
> I'd envsion: ->provide_events() instead of ->open() and
> ->stop_events() instead of ->close(). Of course drivers can
> exploit the fact of knowing that nobody wants any events
> from them and do whatever they consider appropriate, for
> example go into a low power mode - but the latter is beyond
> the scope of the input subsystem and is driver-specific.

I don't have much of an opinion on changing the names,
to me open/close have always means start/stop receiving
events. This follows the everything is a file philosophy,
e.g. you can also not really "open" a serial port,
yet opening /dev/ttyS0 will activate the receive IRQ
of the UART, etc. So maybe we just need to make the
docs clearer rather then do the rename?  Doing the
rename is certainly going to cause a lot of churn.

Anyways as said, I don't have much of an opinion,
so I'll leave commenting (more) on this to Dmitry.

> With such a naming change in mind let's consider inhibiting.
> We want to be able to control when to disregard events from
> a given device. It makes sense to do it at device level, otherwise
> such an operation would have to be invoked in all associated
> handlers (those that have an open handle associating them with
> the device in question). But of course we can do better than
> merely ignoring the events received: we can tell the drivers
> that we don't want any events from them, and later, at uninhibit
> time, tell them to start providing the events again. Conceptually,
> the two operations (provide or don't provide envents) are exactly
> the same thing we want to be happening at input_open_device() and
> input_close_device() time. To me, changing the names of
> ->open() and ->close() exposes this fact very well.
> 
> Consequently, ->inhibit() and ->uninhibit() won't be needed,
> and drivers which already implement ->provide_events() (formerly
> ->open()) and ->stop_events() (formerly ->close()) will receive
> full inhibit/uninhibit support for free (subject to how well they
> implement ->provide_events()/->stop_events()). Unless we can come
> up with what the drivers might be doing on top of ->stop_events()
> and ->provide_events() when inhibiting/uninhibiting, but it seems
> to me we can't. Can we?

Right. I'm happy that you've come to see that both on open/close
and on inhibit/uninhibit we want to "start receiving events" and
"stop receiving events", so that we only need one set of callbacks.

> Optionally ->close() (only the callback, not input_close_device())
> can be made return a value, just as Hans suggests. The value
> can be ignored in input_close_device() but used in input_inhibit().
> No strong opinion here, though. (btw it seems to me that
> input_inhibit() should be renamed to input_inhibit_device()).

Ack.

Regards,

Hans
Andrzej Pietrasiewicz June 3, 2020, 5:54 p.m. UTC | #20
W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
> Hi,
> 
> On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
>> Hi Hans, hi Dmitry,
> 
> <snip>
> 
>> I'm taking one step back and looking at the ->open() and ->close()
>> driver callbacks. They are called from input_open_device() and
>> input_close_device(), respectively:
>>
>> input_open_device():
>> "This function should be called by input handlers when they
>> want to start receive events from given input device."
>>
>> ->open() callback:
>> "this method is called when the very first user calls
>> input_open_device(). The driver must prepare the device to start
>> generating events (start polling thread, request an IRQ, submit
>> URB, etc.)"
>>
>> input_close_device():
>> "This function should be called by input handlers when they
>> want to stop receive events from given input device."
>>
>> ->close() callback:
>> "this method is called when the very last user calls
>> input_close_device()"
>>
>> It seems to me that the callback names do not reflect their
>> purpose: their meaning is not to "open" or to "close" but to
>> give drivers a chance to control when they start or stop
>> providing events to the input core.
>>
>> What would you say about changing the callbacks' names?
>> I'd envsion: ->provide_events() instead of ->open() and
>> ->stop_events() instead of ->close(). Of course drivers can
>> exploit the fact of knowing that nobody wants any events
>> from them and do whatever they consider appropriate, for
>> example go into a low power mode - but the latter is beyond
>> the scope of the input subsystem and is driver-specific.
> 
> I don't have much of an opinion on changing the names,
> to me open/close have always means start/stop receiving
> events. This follows the everything is a file philosophy,
> e.g. you can also not really "open" a serial port,
> yet opening /dev/ttyS0 will activate the receive IRQ
> of the UART, etc. So maybe we just need to make the
> docs clearer rather then do the rename?  Doing the
> rename is certainly going to cause a lot of churn.

Right, I can see now that the suggestion to change names is
too far fetched. (I feel that release() would be better
than close(), though). But it exposes the message I wanted to
pass.

> 
> Anyways as said, I don't have much of an opinion,
> so I'll leave commenting (more) on this to Dmitry.
> 
>> With such a naming change in mind let's consider inhibiting.
>> We want to be able to control when to disregard events from
>> a given device. It makes sense to do it at device level, otherwise
>> such an operation would have to be invoked in all associated
>> handlers (those that have an open handle associating them with
>> the device in question). But of course we can do better than
>> merely ignoring the events received: we can tell the drivers
>> that we don't want any events from them, and later, at uninhibit
>> time, tell them to start providing the events again. Conceptually,
>> the two operations (provide or don't provide envents) are exactly
>> the same thing we want to be happening at input_open_device() and
>> input_close_device() time. To me, changing the names of
>> ->open() and ->close() exposes this fact very well.
>>
>> Consequently, ->inhibit() and ->uninhibit() won't be needed,
>> and drivers which already implement ->provide_events() (formerly
>> ->open()) and ->stop_events() (formerly ->close()) will receive
>> full inhibit/uninhibit support for free (subject to how well they
>> implement ->provide_events()/->stop_events()). Unless we can come
>> up with what the drivers might be doing on top of ->stop_events()
>> and ->provide_events() when inhibiting/uninhibiting, but it seems
>> to me we can't. Can we?
> 
> Right. I'm happy that you've come to see that both on open/close
> and on inhibit/uninhibit we want to "start receiving events" and
> "stop receiving events", so that we only need one set of callbacks.
> 

Yeah, that's my conclusion - at least on a conceptual level.

That said, what I can imagine is an existing driver (e.g. elan_i2c)
which does not implement neither open() nor close(), but does have
suspend() and resume(). Then it is maybe a bit easier to add inhibit()
and uninhibit() /they would be similar to suspend and resume/ instead
of open() and close(): If only open() and close() are possible, then
the probe function needs to be extended to "close" the device before it
gets registered, because from the moment it is registered it might be
opened right away. And the device must be available earlier during the
course of probe to query some parameters through i2c:

+static int elan_reactivate(struct elan_tp_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	ret = elan_enable_power(data);
+	if (ret)
+		dev_err(dev, "failed to restore power: %d\n", ret);
+
+	ret = elan_initialize(data);
+	if (ret)
+		dev_err(dev, "failed to re-initialize touchpad: %d\n", ret);
+
+	return ret;
+}
+
+static int elan_open(struct input_dev *input)
+{
+	struct elan_tp_data *data = input_get_drvdata(input);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	dev_dbg(&client->dev, "uninhibiting\n");
+
+	ret = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (ret)
+		return ret;
+
+	ret = elan_reactivate(data);
+	if (ret == 0)
+		enable_irq(client->irq);
+
+	mutex_unlock(&data->sysfs_mutex);
+
+	return ret;
+}
+
+static int elan_inhibit(struct input_dev *input)
+{
+	struct elan_tp_data *data = input_get_drvdata(input);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	dev_dbg(&client->dev, "closing\n");
+
+	/*
+	 * We are taking the mutex to make sure sysfs operations are
+	 * complete before we attempt to bring the device into low[er]
+	 * power mode.
+	 */
+	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);
+
+	mutex_unlock(&data->sysfs_mutex);
+
+	return ret;
+}
+
+static void elan_close(struct input_dev *input)
+{
+	elan_inhibit(input);
+}
+
  static int elan_query_device_info(struct elan_tp_data *data)
  {
  	int error;
  	u16 ic_type;

  	error = data->ops->get_version(data->client, false, &data->fw_version);
  	if (error)
  		return error;

  	error = data->ops->get_checksum(data->client, false,
  					&data->fw_checksum);
  	if (error)
  		return error;

  	error = data->ops->get_version(data->client, true, &data->iap_version);
  	if (error)
  		return error;
@@ -1071,34 +1141,36 @@ static int elan_setup_trackpoint_input_device(struct 
elan_tp_data *data)

  static int elan_setup_input_device(struct elan_tp_data *data)
  {
  	struct device *dev = &data->client->dev;
  	struct input_dev *input;
  	unsigned int max_width = max(data->width_x, data->width_y);
  	unsigned int min_width = min(data->width_x, data->width_y);
  	int error;

  	input = devm_input_allocate_device(dev);
  	if (!input)
  		return -ENOMEM;

  	input->name = "Elan Touchpad";
  	input->id.bustype = BUS_I2C;
  	input->id.vendor = ELAN_VENDOR_ID;
  	input->id.product = data->product_id;
+	input->open = elan_open;
+	input->close = elan_close;
  	input_set_drvdata(input, data);

  	error = input_mt_init_slots(input, ETP_MAX_FINGERS,
  				    INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED);
  	if (error) {
  		dev_err(dev, "failed to initialize MT slots: %d\n", error);
  		return error;
  	}

  	__set_bit(EV_ABS, input->evbit);
  	__set_bit(INPUT_PROP_POINTER, input->propbit);
  	if (data->clickpad) {
  		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
  	} else {
  		__set_bit(BTN_RIGHT, input->keybit);
  		if (data->middle_button)
  			__set_bit(BTN_MIDDLE, input->keybit);
@@ -1253,34 +1325,40 @@ static int elan_probe(struct i2c_client *client,
  	if (!irqflags)
  		irqflags = IRQF_TRIGGER_FALLING;

  	error = devm_request_threaded_irq(dev, client->irq, NULL, elan_isr,
  					  irqflags | IRQF_ONESHOT,
  					  client->name, data);
  	if (error) {
  		dev_err(dev, "cannot register irq=%d\n", client->irq);
  		return error;
  	}

  	error = devm_device_add_groups(dev, elan_sysfs_groups);
  	if (error) {
  		dev_err(dev, "failed to create sysfs attributes: %d\n", error);
  		return error;
  	}

+	error = elan_inhibit(data->input);
+	if (error) {
+		dev_err(dev, "failed to inhibit input device before registering: %d\n", error);
+		return error;
+	}
+
  	error = input_register_device(data->input);
  	if (error) {
  		dev_err(dev, "failed to register input device: %d\n", error);
  		return error;
  	}

  	if (data->tp_input) {
  		error = input_register_device(data->tp_input);
  		if (error) {
  			dev_err(&client->dev,
  				"failed to register TrackPoint input device: %d\n",
  				error);
  			return error;
  		}
  	}

  	/*
@@ -1294,72 +1372,71 @@ static int elan_probe(struct i2c_client *client,
  }

  static int __maybe_unused elan_suspend(struct device *dev)
  {
  	struct i2c_client *client = to_i2c_client(dev);
  	struct elan_tp_data *data = i2c_get_clientdata(client);
  	int ret;

  	/*
  	 * We are taking the mutex to make sure sysfs operations are
  	 * complete before we attempt to bring the device into low[er]
  	 * power mode.
  	 */
  	ret = mutex_lock_interruptible(&data->sysfs_mutex);
  	if (ret)
  		return ret;

-	disable_irq(client->irq);
+	mutex_lock(&data->input->mutex);
+	if (input_device_enabled(data->input)) {
+		disable_irq(client->irq);

-	if (device_may_wakeup(dev)) {
-		ret = elan_sleep(data);
-		/* Enable wake from IRQ */
-		data->irq_wake = (enable_irq_wake(client->irq) == 0);
-	} else {
-		ret = elan_disable_power(data);
+		if (device_may_wakeup(dev)) {
+			ret = elan_sleep(data);
+			/* Enable wake from IRQ */
+			data->irq_wake = (enable_irq_wake(client->irq) == 0);
+		} else {
+			ret = elan_disable_power(data);
+		}
  	}
+	mutex_unlock(&data->input->mutex);

  	mutex_unlock(&data->sysfs_mutex);
  	return ret;
  }

  static int __maybe_unused elan_resume(struct device *dev)
  {
  	struct i2c_client *client = to_i2c_client(dev);
  	struct elan_tp_data *data = i2c_get_clientdata(client);
-	int error;
+	int ret = 0;

-	if (device_may_wakeup(dev) && data->irq_wake) {
-		disable_irq_wake(client->irq);
-		data->irq_wake = false;
-	}
+	mutex_lock(&data->input->mutex);
+	if (input_device_enabled(data->input)) {
+		if (data->irq_wake) {
+			disable_irq_wake(client->irq);
+			data->irq_wake = false;
+		}

-	error = elan_enable_power(data);
-	if (error) {
-		dev_err(dev, "power up when resuming failed: %d\n", error);
-		goto err;
+		ret = elan_reactivate(data);
+		enable_irq(data->client->irq);
  	}
+	mutex_unlock(&data->input->mutex);

-	error = elan_initialize(data);
-	if (error)
-		dev_err(dev, "initialize when resuming failed: %d\n", error);
-
-err:
-	enable_irq(data->client->irq);
-	return error;
+	return ret;
  }

Regards,

Andrzej
Hans de Goede June 3, 2020, 7:37 p.m. UTC | #21
Hi,

On 6/3/20 7:54 PM, Andrzej Pietrasiewicz wrote:
> W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
>> Hi,
>>
>> On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
>>> Hi Hans, hi Dmitry,
>>
>> <snip>
>>
>>> I'm taking one step back and looking at the ->open() and ->close()
>>> driver callbacks. They are called from input_open_device() and
>>> input_close_device(), respectively:
>>>
>>> input_open_device():
>>> "This function should be called by input handlers when they
>>> want to start receive events from given input device."
>>>
>>> ->open() callback:
>>> "this method is called when the very first user calls
>>> input_open_device(). The driver must prepare the device to start
>>> generating events (start polling thread, request an IRQ, submit
>>> URB, etc.)"
>>>
>>> input_close_device():
>>> "This function should be called by input handlers when they
>>> want to stop receive events from given input device."
>>>
>>> ->close() callback:
>>> "this method is called when the very last user calls
>>> input_close_device()"
>>>
>>> It seems to me that the callback names do not reflect their
>>> purpose: their meaning is not to "open" or to "close" but to
>>> give drivers a chance to control when they start or stop
>>> providing events to the input core.
>>>
>>> What would you say about changing the callbacks' names?
>>> I'd envsion: ->provide_events() instead of ->open() and
>>> ->stop_events() instead of ->close(). Of course drivers can
>>> exploit the fact of knowing that nobody wants any events
>>> from them and do whatever they consider appropriate, for
>>> example go into a low power mode - but the latter is beyond
>>> the scope of the input subsystem and is driver-specific.
>>
>> I don't have much of an opinion on changing the names,
>> to me open/close have always means start/stop receiving
>> events. This follows the everything is a file philosophy,
>> e.g. you can also not really "open" a serial port,
>> yet opening /dev/ttyS0 will activate the receive IRQ
>> of the UART, etc. So maybe we just need to make the
>> docs clearer rather then do the rename?  Doing the
>> rename is certainly going to cause a lot of churn.
> 
> Right, I can see now that the suggestion to change names is
> too far fetched. (I feel that release() would be better
> than close(), though). But it exposes the message I wanted to
> pass.
> 
>>
>> Anyways as said, I don't have much of an opinion,
>> so I'll leave commenting (more) on this to Dmitry.
>>
>>> With such a naming change in mind let's consider inhibiting.
>>> We want to be able to control when to disregard events from
>>> a given device. It makes sense to do it at device level, otherwise
>>> such an operation would have to be invoked in all associated
>>> handlers (those that have an open handle associating them with
>>> the device in question). But of course we can do better than
>>> merely ignoring the events received: we can tell the drivers
>>> that we don't want any events from them, and later, at uninhibit
>>> time, tell them to start providing the events again. Conceptually,
>>> the two operations (provide or don't provide envents) are exactly
>>> the same thing we want to be happening at input_open_device() and
>>> input_close_device() time. To me, changing the names of
>>> ->open() and ->close() exposes this fact very well.
>>>
>>> Consequently, ->inhibit() and ->uninhibit() won't be needed,
>>> and drivers which already implement ->provide_events() (formerly
>>> ->open()) and ->stop_events() (formerly ->close()) will receive
>>> full inhibit/uninhibit support for free (subject to how well they
>>> implement ->provide_events()/->stop_events()). Unless we can come
>>> up with what the drivers might be doing on top of ->stop_events()
>>> and ->provide_events() when inhibiting/uninhibiting, but it seems
>>> to me we can't. Can we?
>>
>> Right. I'm happy that you've come to see that both on open/close
>> and on inhibit/uninhibit we want to "start receiving events" and
>> "stop receiving events", so that we only need one set of callbacks.
>>
> 
> Yeah, that's my conclusion - at least on a conceptual level.
> 
> That said, what I can imagine is an existing driver (e.g. elan_i2c)
> which does not implement neither open() nor close(), but does have
> suspend() and resume(). Then it is maybe a bit easier to add inhibit()
> and uninhibit() /they would be similar to suspend and resume/ instead
> of open() and close(): If only open() and close() are possible, then
> the probe function needs to be extended to "close" the device before it
> gets registered, because from the moment it is registered it might be
> opened right away.

The probe only needs to "close" it if for some reason it
starts directly sending events in most cases the driver
must actively do something to get it to send events.

So in most cases this should be pretty straight forward,
as for having to do some init / power-on during probe
and then power-off at the end of the probe. Yes sometimes
something like that might be necessary.

Looking at your suggested elan_i2c changes I think they
look fine. I have the feeling that with some refactoring
they can be made a bit cleaner (I did not look a the
changes in too much detail) but overall I think they
look ok.

Note you may also want to look at using the runtime
suspend framework for this, doing a pm_runtime_get_sync()
in open() and then letting (runtime) suspend do the power
off if you set a reasonable timeout for autosuspend after
the last user is gone then that will also avoid an
unnecessary suspend / resume cycle between probe()
exiting and the first open() call and this avoids the
need to do a poweroff() at the end of probe(), the
runtime-pm framework will autosuspend the device after
the timeout expires.

Regards,

Hans


> And the device must be available earlier during the
> course of probe to query some parameters through i2c:
> 
> +static int elan_reactivate(struct elan_tp_data *data)
> +{
> +    struct device *dev = &data->client->dev;
> +    int ret;
> +
> +    ret = elan_enable_power(data);
> +    if (ret)
> +        dev_err(dev, "failed to restore power: %d\n", ret);
> +
> +    ret = elan_initialize(data);
> +    if (ret)
> +        dev_err(dev, "failed to re-initialize touchpad: %d\n", ret);
> +
> +    return ret;
> +}
> +
> +static int elan_open(struct input_dev *input)
> +{
> +    struct elan_tp_data *data = input_get_drvdata(input);
> +    struct i2c_client *client = data->client;
> +    int ret;
> +
> +    dev_dbg(&client->dev, "uninhibiting\n");
> +
> +    ret = mutex_lock_interruptible(&data->sysfs_mutex);
> +    if (ret)
> +        return ret;
> +
> +    ret = elan_reactivate(data);
> +    if (ret == 0)
> +        enable_irq(client->irq);
> +
> +    mutex_unlock(&data->sysfs_mutex);
> +
> +    return ret;
> +}
> +
> +static int elan_inhibit(struct input_dev *input)
> +{
> +    struct elan_tp_data *data = input_get_drvdata(input);
> +    struct i2c_client *client = data->client;
> +    int ret;
> +
> +    dev_dbg(&client->dev, "closing\n");
> +
> +    /*
> +     * We are taking the mutex to make sure sysfs operations are
> +     * complete before we attempt to bring the device into low[er]
> +     * power mode.
> +     */
> +    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);
> +
> +    mutex_unlock(&data->sysfs_mutex);
> +
> +    return ret;
> +}
> +
> +static void elan_close(struct input_dev *input)
> +{
> +    elan_inhibit(input);
> +}
> +
>   static int elan_query_device_info(struct elan_tp_data *data)
>   {
>       int error;
>       u16 ic_type;
> 
>       error = data->ops->get_version(data->client, false, &data->fw_version);
>       if (error)
>           return error;
> 
>       error = data->ops->get_checksum(data->client, false,
>                       &data->fw_checksum);
>       if (error)
>           return error;
> 
>       error = data->ops->get_version(data->client, true, &data->iap_version);
>       if (error)
>           return error;
> @@ -1071,34 +1141,36 @@ static int elan_setup_trackpoint_input_device(struct elan_tp_data *data)
> 
>   static int elan_setup_input_device(struct elan_tp_data *data)
>   {
>       struct device *dev = &data->client->dev;
>       struct input_dev *input;
>       unsigned int max_width = max(data->width_x, data->width_y);
>       unsigned int min_width = min(data->width_x, data->width_y);
>       int error;
> 
>       input = devm_input_allocate_device(dev);
>       if (!input)
>           return -ENOMEM;
> 
>       input->name = "Elan Touchpad";
>       input->id.bustype = BUS_I2C;
>       input->id.vendor = ELAN_VENDOR_ID;
>       input->id.product = data->product_id;
> +    input->open = elan_open;
> +    input->close = elan_close;
>       input_set_drvdata(input, data);
> 
>       error = input_mt_init_slots(input, ETP_MAX_FINGERS,
>                       INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED);
>       if (error) {
>           dev_err(dev, "failed to initialize MT slots: %d\n", error);
>           return error;
>       }
> 
>       __set_bit(EV_ABS, input->evbit);
>       __set_bit(INPUT_PROP_POINTER, input->propbit);
>       if (data->clickpad) {
>           __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
>       } else {
>           __set_bit(BTN_RIGHT, input->keybit);
>           if (data->middle_button)
>               __set_bit(BTN_MIDDLE, input->keybit);
> @@ -1253,34 +1325,40 @@ static int elan_probe(struct i2c_client *client,
>       if (!irqflags)
>           irqflags = IRQF_TRIGGER_FALLING;
> 
>       error = devm_request_threaded_irq(dev, client->irq, NULL, elan_isr,
>                         irqflags | IRQF_ONESHOT,
>                         client->name, data);
>       if (error) {
>           dev_err(dev, "cannot register irq=%d\n", client->irq);
>           return error;
>       }
> 
>       error = devm_device_add_groups(dev, elan_sysfs_groups);
>       if (error) {
>           dev_err(dev, "failed to create sysfs attributes: %d\n", error);
>           return error;
>       }
> 
> +    error = elan_inhibit(data->input);
> +    if (error) {
> +        dev_err(dev, "failed to inhibit input device before registering: %d\n", error);
> +        return error;
> +    }
> +
>       error = input_register_device(data->input);
>       if (error) {
>           dev_err(dev, "failed to register input device: %d\n", error);
>           return error;
>       }
> 
>       if (data->tp_input) {
>           error = input_register_device(data->tp_input);
>           if (error) {
>               dev_err(&client->dev,
>                   "failed to register TrackPoint input device: %d\n",
>                   error);
>               return error;
>           }
>       }
> 
>       /*
> @@ -1294,72 +1372,71 @@ static int elan_probe(struct i2c_client *client,
>   }
> 
>   static int __maybe_unused elan_suspend(struct device *dev)
>   {
>       struct i2c_client *client = to_i2c_client(dev);
>       struct elan_tp_data *data = i2c_get_clientdata(client);
>       int ret;
> 
>       /*
>        * We are taking the mutex to make sure sysfs operations are
>        * complete before we attempt to bring the device into low[er]
>        * power mode.
>        */
>       ret = mutex_lock_interruptible(&data->sysfs_mutex);
>       if (ret)
>           return ret;
> 
> -    disable_irq(client->irq);
> +    mutex_lock(&data->input->mutex);
> +    if (input_device_enabled(data->input)) {
> +        disable_irq(client->irq);
> 
> -    if (device_may_wakeup(dev)) {
> -        ret = elan_sleep(data);
> -        /* Enable wake from IRQ */
> -        data->irq_wake = (enable_irq_wake(client->irq) == 0);
> -    } else {
> -        ret = elan_disable_power(data);
> +        if (device_may_wakeup(dev)) {
> +            ret = elan_sleep(data);
> +            /* Enable wake from IRQ */
> +            data->irq_wake = (enable_irq_wake(client->irq) == 0);
> +        } else {
> +            ret = elan_disable_power(data);
> +        }
>       }
> +    mutex_unlock(&data->input->mutex);
> 
>       mutex_unlock(&data->sysfs_mutex);
>       return ret;
>   }
> 
>   static int __maybe_unused elan_resume(struct device *dev)
>   {
>       struct i2c_client *client = to_i2c_client(dev);
>       struct elan_tp_data *data = i2c_get_clientdata(client);
> -    int error;
> +    int ret = 0;
> 
> -    if (device_may_wakeup(dev) && data->irq_wake) {
> -        disable_irq_wake(client->irq);
> -        data->irq_wake = false;
> -    }
> +    mutex_lock(&data->input->mutex);
> +    if (input_device_enabled(data->input)) {
> +        if (data->irq_wake) {
> +            disable_irq_wake(client->irq);
> +            data->irq_wake = false;
> +        }
> 
> -    error = elan_enable_power(data);
> -    if (error) {
> -        dev_err(dev, "power up when resuming failed: %d\n", error);
> -        goto err;
> +        ret = elan_reactivate(data);
> +        enable_irq(data->client->irq);
>       }
> +    mutex_unlock(&data->input->mutex);
> 
> -    error = elan_initialize(data);
> -    if (error)
> -        dev_err(dev, "initialize when resuming failed: %d\n", error);
> -
> -err:
> -    enable_irq(data->client->irq);
> -    return error;
> +    return ret;
>   }
> 
> Regards,
> 
> Andrzej
>
Dmitry Torokhov June 4, 2020, 7:28 a.m. UTC | #22
Hi Hans, Andrzej,

On Wed, Jun 03, 2020 at 09:37:10PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 6/3/20 7:54 PM, Andrzej Pietrasiewicz wrote:
> > W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
> > > Hi,
> > > 
> > > On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
> > > > Hi Hans, hi Dmitry,
> > > 
> > > <snip>
> > > 
> > > > I'm taking one step back and looking at the ->open() and ->close()
> > > > driver callbacks. They are called from input_open_device() and
> > > > input_close_device(), respectively:
> > > > 
> > > > input_open_device():
> > > > "This function should be called by input handlers when they
> > > > want to start receive events from given input device."
> > > > 
> > > > ->open() callback:
> > > > "this method is called when the very first user calls
> > > > input_open_device(). The driver must prepare the device to start
> > > > generating events (start polling thread, request an IRQ, submit
> > > > URB, etc.)"
> > > > 
> > > > input_close_device():
> > > > "This function should be called by input handlers when they
> > > > want to stop receive events from given input device."
> > > > 
> > > > ->close() callback:
> > > > "this method is called when the very last user calls
> > > > input_close_device()"
> > > > 
> > > > It seems to me that the callback names do not reflect their
> > > > purpose: their meaning is not to "open" or to "close" but to
> > > > give drivers a chance to control when they start or stop
> > > > providing events to the input core.
> > > > 
> > > > What would you say about changing the callbacks' names?
> > > > I'd envsion: ->provide_events() instead of ->open() and
> > > > ->stop_events() instead of ->close(). Of course drivers can
> > > > exploit the fact of knowing that nobody wants any events
> > > > from them and do whatever they consider appropriate, for
> > > > example go into a low power mode - but the latter is beyond
> > > > the scope of the input subsystem and is driver-specific.
> > > 
> > > I don't have much of an opinion on changing the names,
> > > to me open/close have always means start/stop receiving
> > > events. This follows the everything is a file philosophy,
> > > e.g. you can also not really "open" a serial port,
> > > yet opening /dev/ttyS0 will activate the receive IRQ
> > > of the UART, etc. So maybe we just need to make the
> > > docs clearer rather then do the rename?  Doing the
> > > rename is certainly going to cause a lot of churn.
> > 
> > Right, I can see now that the suggestion to change names is
> > too far fetched. (I feel that release() would be better
> > than close(), though). But it exposes the message I wanted to
> > pass.

release() usually means that the object is destroyedm, i.e this action,
unlike close() is irrevocable.

Let's leave the names as is, and adjust kerneldoc comments as needed.

> > 
> > > 
> > > Anyways as said, I don't have much of an opinion,
> > > so I'll leave commenting (more) on this to Dmitry.
> > > 
> > > > With such a naming change in mind let's consider inhibiting.
> > > > We want to be able to control when to disregard events from
> > > > a given device. It makes sense to do it at device level, otherwise
> > > > such an operation would have to be invoked in all associated
> > > > handlers (those that have an open handle associating them with
> > > > the device in question). But of course we can do better than
> > > > merely ignoring the events received: we can tell the drivers
> > > > that we don't want any events from them, and later, at uninhibit
> > > > time, tell them to start providing the events again. Conceptually,
> > > > the two operations (provide or don't provide envents) are exactly
> > > > the same thing we want to be happening at input_open_device() and
> > > > input_close_device() time. To me, changing the names of
> > > > ->open() and ->close() exposes this fact very well.
> > > > 
> > > > Consequently, ->inhibit() and ->uninhibit() won't be needed,
> > > > and drivers which already implement ->provide_events() (formerly
> > > > ->open()) and ->stop_events() (formerly ->close()) will receive
> > > > full inhibit/uninhibit support for free (subject to how well they
> > > > implement ->provide_events()/->stop_events()). Unless we can come
> > > > up with what the drivers might be doing on top of ->stop_events()
> > > > and ->provide_events() when inhibiting/uninhibiting, but it seems
> > > > to me we can't. Can we?
> > > 
> > > Right. I'm happy that you've come to see that both on open/close
> > > and on inhibit/uninhibit we want to "start receiving events" and
> > > "stop receiving events", so that we only need one set of callbacks.
> > > 
> > 
> > Yeah, that's my conclusion - at least on a conceptual level.
> > 
> > That said, what I can imagine is an existing driver (e.g. elan_i2c)
> > which does not implement neither open() nor close(), but does have
> > suspend() and resume(). Then it is maybe a bit easier to add inhibit()
> > and uninhibit() /they would be similar to suspend and resume/ instead
> > of open() and close(): If only open() and close() are possible, then
> > the probe function needs to be extended to "close" the device before it
> > gets registered, because from the moment it is registered it might be
> > opened right away.
> 
> The probe only needs to "close" it if for some reason it
> starts directly sending events in most cases the driver
> must actively do something to get it to send events.
> 
> So in most cases this should be pretty straight forward,
> as for having to do some init / power-on during probe
> and then power-off at the end of the probe. Yes sometimes
> something like that might be necessary.
> 
> Looking at your suggested elan_i2c changes I think they
> look fine. I have the feeling that with some refactoring
> they can be made a bit cleaner (I did not look a the
> changes in too much detail) but overall I think they
> look ok.
> 
> Note you may also want to look at using the runtime
> suspend framework for this, doing a pm_runtime_get_sync()
> in open() and then letting (runtime) suspend do the power
> off if you set a reasonable timeout for autosuspend after
> the last user is gone then that will also avoid an
> unnecessary suspend / resume cycle between probe()
> exiting and the first open() call and this avoids the
> need to do a poweroff() at the end of probe(), the
> runtime-pm framework will autosuspend the device after
> the timeout expires.

Yes, plugging into runtime PM would be nice, as as it currently stands
the driver will be broken with regard to trying access sysfs for
firmware update/calibration/etc if device happens to be inhibited.

The version of the driver in Chrome OS tree is similarly broken, but
because we control both kernel and the rest of the stack we know that we
do not poke at sysfs when device is inhibited. It will not be acceptable
for mainline (and that is one of reasons why elan_i2c does not have
open/close methods at the moment).

Thanks.
Andrzej Pietrasiewicz June 5, 2020, 5:33 p.m. UTC | #23
Userspace might want to implement a policy to temporarily disregard input
from certain devices.

An example use case is a convertible laptop, whose keyboard can be folded
under the screen to create tablet-like experience. The user then must hold
the laptop in such a way that it is difficult to avoid pressing the keyboard
keys. It is therefore desirable to temporarily disregard input from the
keyboard, until it is folded back. This obviously is a policy which should
be kept out of the kernel, but the kernel must provide suitable means to
implement such a policy.

Due to interactions with suspend/resume, a helper has been added for drivers
to decide if the device is being used or not (PATCH 1/7) and it has been
applied to relevant drivers (PATCH 2,4,5,6/7).

PATCH 7/7 adds support for inhibiting input devices.

This work is inspired by:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/45c2d7bb398f74adfae0017e20b224152fde3822

and

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4ce0e8a3697edb8fd071110b3af65014512061c7

In this respin the elan_i2c patch is dropped and converting it will be
addressed later.

v2..v3:
- ignored autorepeat events in input_get_disposition() if a key is not
pressed (Hans)
- dropped inhibit()/uninhibit() driver callbacks (Hans)
- split ACPI button patch into taking the lock and using the helper (Rafael)
- dropped the elan_i2c conversion
- fixed typos in exynos adc

v1..v2:
- added input_device_enabled() helper and used it in drivers (Dmitry)
- the fact of open() and close() being called in inhibit/uninhibit paths has
been emphasized in the commit message of PATCH 6/7 (Dmitry)

Andrzej Pietrasiewicz (6):
  Input: add input_device_enabled()
  Input: use input_device_enabled()
  ACPI: button: Access input device's users under appropriate mutex
  ACPI: button: Use input_device_enabled() helper
  iio: adc: exynos: Use input_device_enabled()
  platform/x86: thinkpad_acpi: Use input_device_enabled()

Patrik Fimml (1):
  Input: Add "inhibited" property

 drivers/acpi/button.c                       |   7 +-
 drivers/iio/adc/exynos_adc.c                |  11 +-
 drivers/input/input.c                       | 121 +++++++++++++++++++-
 drivers/input/joystick/xpad.c               |   4 +-
 drivers/input/keyboard/ep93xx_keypad.c      |   2 +-
 drivers/input/keyboard/gpio_keys.c          |   4 +-
 drivers/input/keyboard/imx_keypad.c         |   4 +-
 drivers/input/keyboard/ipaq-micro-keys.c    |   2 +-
 drivers/input/keyboard/lpc32xx-keys.c       |   4 +-
 drivers/input/keyboard/pmic8xxx-keypad.c    |   4 +-
 drivers/input/keyboard/pxa27x_keypad.c      |   2 +-
 drivers/input/keyboard/samsung-keypad.c     |   4 +-
 drivers/input/keyboard/spear-keyboard.c     |   8 +-
 drivers/input/keyboard/st-keyscan.c         |   4 +-
 drivers/input/keyboard/tegra-kbc.c          |   4 +-
 drivers/input/misc/drv260x.c                |   4 +-
 drivers/input/misc/drv2665.c                |   4 +-
 drivers/input/misc/drv2667.c                |   4 +-
 drivers/input/misc/gp2ap002a00f.c           |   4 +-
 drivers/input/misc/kxtj9.c                  |   4 +-
 drivers/input/misc/sirfsoc-onkey.c          |   2 +-
 drivers/input/mouse/navpoint.c              |   4 +-
 drivers/input/touchscreen/ad7879.c          |   6 +-
 drivers/input/touchscreen/atmel_mxt_ts.c    |   4 +-
 drivers/input/touchscreen/auo-pixcir-ts.c   |   8 +-
 drivers/input/touchscreen/bu21029_ts.c      |   4 +-
 drivers/input/touchscreen/chipone_icn8318.c |   4 +-
 drivers/input/touchscreen/cyttsp_core.c     |   4 +-
 drivers/input/touchscreen/eeti_ts.c         |   4 +-
 drivers/input/touchscreen/ektf2127.c        |   4 +-
 drivers/input/touchscreen/imx6ul_tsc.c      |   4 +-
 drivers/input/touchscreen/ipaq-micro-ts.c   |   2 +-
 drivers/input/touchscreen/iqs5xx.c          |   4 +-
 drivers/input/touchscreen/lpc32xx_ts.c      |   4 +-
 drivers/input/touchscreen/melfas_mip4.c     |   4 +-
 drivers/input/touchscreen/mms114.c          |   6 +-
 drivers/input/touchscreen/pixcir_i2c_ts.c   |   8 +-
 drivers/input/touchscreen/ucb1400_ts.c      |   4 +-
 drivers/input/touchscreen/wm97xx-core.c     |  14 ++-
 drivers/input/touchscreen/zforce_ts.c       |   8 +-
 drivers/platform/x86/thinkpad_acpi.c        |   4 +-
 include/linux/input.h                       |  14 ++-
 42 files changed, 230 insertions(+), 95 deletions(-)


base-commit: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
Pavel Machek June 7, 2020, 8:24 p.m. UTC | #24
On Fri 2020-06-05 19:33:28, Andrzej Pietrasiewicz wrote:
> Userspace might want to implement a policy to temporarily disregard input
> from certain devices.

Wow, you certainly cc a lot of lists.

> An example use case is a convertible laptop, whose keyboard can be folded
> under the screen to create tablet-like experience. The user then must hold
> the laptop in such a way that it is difficult to avoid pressing the keyboard
> keys. It is therefore desirable to temporarily disregard input from the
> keyboard, until it is folded back. This obviously is a policy which should
> be kept out of the kernel, but the kernel must provide suitable means to
> implement such a policy.
> 
> Due to interactions with suspend/resume, a helper has been added for drivers
> to decide if the device is being used or not (PATCH 1/7) and it has been
> applied to relevant drivers (PATCH 2,4,5,6/7).

But is that a right way to implement it?

We want this for cellphones, too -- touchscreen should be disabled
while the device is locked in the pocket -- but we really want the
touchscreen hardware to be powered down in that case (because it keeps
SoC busy and eats a _lot_ of electricity).

But simplistic "receive an event and then drop it if device is
inhibited" does not allow that...

Best regards,
								Pavel
Dmitry Torokhov June 8, 2020, 5:37 a.m. UTC | #25
On Sun, Jun 07, 2020 at 10:24:14PM +0200, Pavel Machek wrote:
> On Fri 2020-06-05 19:33:28, Andrzej Pietrasiewicz wrote:
> > Userspace might want to implement a policy to temporarily disregard input
> > from certain devices.
> 
> Wow, you certainly cc a lot of lists.
> 
> > An example use case is a convertible laptop, whose keyboard can be folded
> > under the screen to create tablet-like experience. The user then must hold
> > the laptop in such a way that it is difficult to avoid pressing the keyboard
> > keys. It is therefore desirable to temporarily disregard input from the
> > keyboard, until it is folded back. This obviously is a policy which should
> > be kept out of the kernel, but the kernel must provide suitable means to
> > implement such a policy.
> > 
> > Due to interactions with suspend/resume, a helper has been added for drivers
> > to decide if the device is being used or not (PATCH 1/7) and it has been
> > applied to relevant drivers (PATCH 2,4,5,6/7).
> 
> But is that a right way to implement it?
> 
> We want this for cellphones, too -- touchscreen should be disabled
> while the device is locked in the pocket -- but we really want the
> touchscreen hardware to be powered down in that case (because it keeps
> SoC busy and eats a _lot_ of electricity).
> 
> But simplistic "receive an event and then drop it if device is
> inhibited" does not allow that...

I do not think you read the entirety of this patch series...

Thanks.
Andrzej Pietrasiewicz June 8, 2020, 9:28 a.m. UTC | #26
Hi Pavel,

W dniu 08.06.2020 o 07:37, Dmitry Torokhov pisze:
> On Sun, Jun 07, 2020 at 10:24:14PM +0200, Pavel Machek wrote:
>> On Fri 2020-06-05 19:33:28, Andrzej Pietrasiewicz wrote:
>>> Userspace might want to implement a policy to temporarily disregard input
>>> from certain devices.
>>
>> Wow, you certainly cc a lot of lists.
>>
>>> An example use case is a convertible laptop, whose keyboard can be folded
>>> under the screen to create tablet-like experience. The user then must hold
>>> the laptop in such a way that it is difficult to avoid pressing the keyboard
>>> keys. It is therefore desirable to temporarily disregard input from the
>>> keyboard, until it is folded back. This obviously is a policy which should
>>> be kept out of the kernel, but the kernel must provide suitable means to
>>> implement such a policy.
>>>
>>> Due to interactions with suspend/resume, a helper has been added for drivers
>>> to decide if the device is being used or not (PATCH 1/7) and it has been
>>> applied to relevant drivers (PATCH 2,4,5,6/7).
>>
>> But is that a right way to implement it?
>>
>> We want this for cellphones, too -- touchscreen should be disabled
>> while the device is locked in the pocket -- but we really want the
>> touchscreen hardware to be powered down in that case (because it keeps
>> SoC busy and eats a _lot_ of electricity).
>>
>> But simplistic "receive an event and then drop it if device is
>> inhibited" does not allow that...
> 
> I do not think you read the entirety of this patch series...
> 

Yeah, kindly read the whole thread. Long story short: Inhibiting _is_ about
ignoring events from inhibited devices. Obviously we can do better than
just that. Indeed, the open() and close() callbacks (which are called at
uninhibiting/inhibiting) mean "start providing events" and "stop providing
events", respectively. How that translates into driver operation is highly
driver-specific and cannot be handled at the input subsystem level, but it
is the place where power savings can be realized: whenever the driver knows
that nobody wants events from it it can do whatever it considers appropriate,
including transitioning the device into low power mode, for example using
PM runtime.

Regards,

Andrzej