Anssi Hannula wrote:
> Dmitry Torokhov wrote:
>
>>On 6/6/06, Anssi Hannula <[email protected]> wrote:
>>
>>
>>>Dmitry Torokhov wrote:
>>>
>>>>On Monday 05 June 2006 17:11, Anssi Hannula wrote:
>>>>
>>>>
>>>>>Dmitry Torokhov wrote:
>>>>>
>>>>>
>>>>>>On 5/30/06, Anssi Hannula <[email protected]> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
