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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/19/20 11:02 AM, Andrzej Pietrasiewicz wrote:
> Hi Hans, Hi Dmitry,
> 
> W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
>> Hi,
> 
> <snip>
> 
>>>>>>
>>>>>> So I wonder what this series actually adds for functionality for
>>>>>> userspace which can not already be achieved this way?
>>>>>>
>>>>>> I also noticed that you keep the device open (do not call the
>>>>>> input_device's close callback) when inhibited and just throw away
>>>>>
>>>>> I'm not sure if I understand you correctly, it is called:
>>>>>
>>>>> +static inline void input_stop(struct input_dev *dev)
>>>>> +{
>>>>> +    if (dev->poller)
>>>>> +        input_dev_poller_stop(dev->poller);
>>>>> +    if (dev->close)
>>>>> +        dev->close(dev);
>>>>>                  ^^^^^^^^^^^^^^^^
>>>>> +static int input_inhibit(struct input_dev *dev)
>>>>> +{
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    mutex_lock(&dev->mutex);
>>>>> +
>>>>> +    if (dev->inhibited)
>>>>> +        goto out;
>>>>> +
>>>>> +    if (dev->users) {
>>>>> +        if (dev->inhibit) {
>>>>> +            ret = dev->inhibit(dev);
>>>>> +            if (ret)
>>>>> +                goto out;
>>>>> +        }
>>>>> +        input_stop(dev);
>>>>>                  ^^^^^^^^^^^^^^^^
>>>>>
>>>>> It will not be called when dev->users is zero, but if it is zero,
>>>>> then nobody has opened the device yet so there is nothing to close.
>>>>
>>>> Ah, I missed that.
>>>>
>>>> So if the device implements the inhibit call back then on
>>>> inhibit it will get both the inhibit and close callback called?
>>>>
>>>
>>> That's right. And conversely, upon uninhibit open() and uninhibit()
>>> callbacks will be invoked. Please note that just as with open()/close(),
>>> providing inhibit()/uninhibit() is optional.
>>
>> Ack.
>>
>>>> And what happens if the last user goes away and the device
>>>> is not inhibited?
>>>
>>> close() is called as usually.
>>
>> But not inhibit, hmm, see below.
>>
>>>> I'm trying to understand here what the difference between the 2
>>>> is / what the goal of having a separate inhibit callback ?
>>>>
>>>
>>> Drivers have very different ideas about what it means to suspend/resume
>>> and open/close. The optional inhibit/uninhibit callbacks are meant for
>>> the drivers to know that it is this particular action going on.
>>
>> So the inhibit() callback triggers the "suspend" behavior ?
>> But shouldn't drivers which are capable of suspending the device
>> always do so on close() ?
>>
>> Since your current proposal also calls close() on inhibit() I
>> really see little difference between an inhibit() and the last
>> user of the device closing it and IMHO unless there is a good
>> reason to actually differentiate the 2 it would be better
>> to only stick with the existing close() and in cases where
>> that does not put the device in a low-power mode yet, fix
>> the existing close() callback to do the low-power mode
>> setting instead of adding a new callback.
>>
>>> For inhibit() there's one more argument: close() does not return a value,
>>> so its meaning is "do some last cleanup" and as such it is not allowed
>>> to fail - whatever its effect is, we must deem it successful. inhibit()
>>> does return a value and so it is allowed to fail.
>>
>> Well, we could make close() return an error and at least in the inhibit()
>> case propagate that to userspace. I wonder if userspace is going to
>> do anything useful with that error though...
>>
>> In my experience errors during cleanup/shutdown are best logged
>> (using dev_err) and otherwise ignored, so that we try to clean up
>> as much possible. Unless the very first step of the shutdown process
>> fails the device is going to be in some twilight zone state anyways
>> at this point we might as well try to cleanup as much as possible.
> 
> What you say makes sense to me.
> @Dmitry?
> 
>>
>>> All in all, it is up to the drivers to decide which callback they
>>> provide. Based on my work so far I would say that there are tens
>>> of simple cases where open() and close() are sufficient, out of total
>>> ~400 users of input_allocate_device():
>>>
>>> $ git grep "input_allocate_device(" | grep -v ^Documentation | \
>>> cut -f1 -d: | sort | uniq | wc
>>>      390     390   13496
>>
>> So can you explain a bit more about the cases where only having
>> open/close is not sufficient?  So far I have the feeling that
>> those are all we need and that we really do not need separate
>> [un]inhibit callbacks.
> 
> My primary concern was not being able to propagate inhibit() error
> to userspace, and then if we have inhibit(), uninhibit() should be
> there for completeness. If propagating the error to userspace can
> be neglected then yes, it seems open/close should be sufficient,
> even more because the real meaning of "open" is "prepare the device
> for generating input events".
> 
> To validate the idea of not introducing inhibit()/uninhibit() callbacks
> to implement device inhibiting/uninhibiting let's look at
> drivers/input/mouse/elan_i2c_core.c (PATCH 7/7):
> 
> static int elan_inhibit(struct input_dev *input)
> {
> [...]
> 
>      ret = mutex_lock_interruptible(&data->sysfs_mutex);
>      if (ret)
>          return ret;
> 
>      disable_irq(client->irq);
> 
>      ret = elan_disable_power(data);
>      if (ret)
>          enable_irq(client->irq);
> [...]
> }
> 
> First, close() does not exist in this driver. Of course this can be
> fixed. Then it doesn't return a value. Then, if either taking the
> mutex or disabling the power fails, the close() is still deemed
> successful. Is it ok?

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

 > Well, we could make close() return an error and at least in the inhibit()
 > case propagate that to userspace. I wonder if userspace is going to
 > do anything useful with that error though...

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

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

Regards,

Hans