Re: [patch 03/12] input: new force feedback interface

[Anssi Hannula <anssi.hannula@gmail.com>]

Anssi Hannula wrote:
> Dmitry Torokhov wrote:
> 
>>On 6/6/06, Anssi Hannula <anssi.hannula@gmail.com> wrote:
>>
>>
>>>Dmitry Torokhov wrote:
>>>
>>>>On Monday 05 June 2006 17:11, Anssi Hannula wrote:
>>>>
>>>>
>>>>>Dmitry Torokhov wrote:
>>>>>
>>>>>
>>>>>>On 5/30/06, Anssi Hannula <anssi.hannula@gmail.com> wrote:
>>>>>>
>>>>>>>--- linux-2.6.17-rc4-git12.orig/drivers/input/input.c   2006-05-27
>>>>>>>02:28:57.000000000 +0300
>>>>>>>+++ linux-2.6.17-rc4-git12/drivers/input/input.c        2006-05-27
>>>>>>>02:38:35.000000000 +0300
>>>>>>>@@ -733,6 +733,17 @@ static void input_dev_release(struct cla
>>>>>>>{
>>>>>>>      struct input_dev *dev = to_input_dev(class_dev);
>>>>>>>
>>>>>>>+       if (dev->ff) {
>>>>>>>+               struct ff_device *ff = dev->ff;
>>>>>>>+               clear_bit(EV_FF, dev->evbit);
>>>>>>>+               mutex_lock(&dev->ff_lock);
>>>>>>>+               del_timer_sync(&ff->timer);
>>>>>>
>>>>>>
>>>>>>This is too late. We need to stop timer when device gets unregistered.
>>>>>
>>>>>And what if driver has called input_allocate_device(),
>>>>>input_ff_allocate(), input_ff_register(), but then decides to abort and
>>>>>calls input_dev_release()?   input_unregister_device() would not get
>>>>>called at all.
>>>>>
>>>>
>>>>
>>>>Right, but if device was never registered there is no device node so
>>>
>>>noone
>>>
>>>>could start the timer and deleting it is a noop. Hmm, I think even
>>>
>>>better
>>>
>>>>place would be to stop ff timer when device is closed (i.e. when
>>>
>>>last user
>>>
>>>>closes file handle).
>>>>
>>>
>>>Hmm... actually, they are stopped in flush(), and IIRC that is always
>>>called before deleting input_dev.
>>>
>>
>>flush is called when you close one file handle. If there are more than
>>one process opened event device you only want to stop timer when they
>>all closed ther handles, not when the first one did.
>>
> 
> 
> It doesn't stop the timer explicitely. It only calls the timer
> recalculator function and when all file handles are closed => all
> effects are deleted => no events => input_ff_recalculate_timer() stops
> the timer.
> 
> And IIRC when device is being removed, kernel actually waits for the
> file handles to be flush()ed before kfree()ing the input_dev.
> 
> But hmm, when device is being removed with effects running, and
> input_ff_flush() erases effects and calls input_ff_recalculate_timer(),
> that function schedules the timer to run immediately to stop the
> effects. Therefore we would have a race condition: without locking
> dev->ff could be deleted while input_ff_timer() is still running.
> 
> That is the reason why del_timer_sync() is in input_dev_release(), to
> make sure the input_ff_timer() is no longer running.

I guess we could have an atomic bit timer_is_active which would be set
when the timer is either pending or timer function is running, and then
in device closing or unregistering wait for it to be cleared:

1. Timer is running.
2. input_unregister_device() gets called and blocks until file handles
   are closed
3. The user closes the file handle.
4. input_ff_flush() removes the effects and recalculates timer.
   => timer is set to execute immediately, as there are effects to be
   stopped
5. input_close_device() or input_unregister_device() waits for the timer
   to finish, i.e. timer_is_active to be cleared.

Dmitry, should I make the modifications and send a patch, or do you want
to have more time to look at my patches?

-- 
Anssi Hannula