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

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

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.

-- 
Anssi Hannula