|
From: Hans de G. <hde...@re...> - 2020-05-19 09:36:49
|
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
|
