Re: [PATCHv2 0/7] Support inhibiting input devices

Andrzej Pietrasiewicz
2020-05-18T13:49:22Z

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