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:
>>>
>>>
>>>>Implement a new force feedback interface, in which all
>>>>non-driver-specific
>>>>operations are separated to a common module. This includes handling
>>>>effect
>>>>type validations, effect timers, locking, etc.
>>>>
>>>
>>>Still looking at it, couple of random points for now...
>>>
>>>
>>>>The code should be built as part of the input module, but
>>>>unfortunately that
>>>>would require renaming input.c, which we don't want to do. So instead
>>>>we make
>>>>INPUT_FF_EFFECTS a bool so that it cannot be built as a module.
>>>>
>>>
>>>I am not opposed to rename input.c, I wonder what pending changes
>>>besides David's header cleanup Andrew had in mind.
>>>
>>>
>>>>@@ -865,6 +865,9 @@ struct input_dev {
>>>> unsigned long sndbit[NBITS(SND_MAX)];
>>>> unsigned long ffbit[NBITS(FF_MAX)];
>>>> unsigned long swbit[NBITS(SW_MAX)];
>>>>+
>>>>+ struct ff_device *ff;
>>>>+ struct mutex ff_lock;
>>>
>>>
>>>I believe that ff_lock should be part of ff_device and be only used to
>>>controll access when uploading/erasing effects. The teardown process
>>>should make sure that device inactive anyway only then remove
>>>ff_device from input_dev; by that time noone should be able to
>>>upload/erase effects. Therefore ff_lock is not needed to protect
>>>dev->ff.
>>>
>>
>>Hmm, I remember testing this by putting a 10 second sleep into the end
>>of input_ff_effect_upload() and dropping the ff_locking when
>>unregistering device. Then while in that sleep I unplugged the device.
>>The dev->ff was indeed removed while the input_ff_effect_upload() was
>>still running.
>>
>>Maybe there was/is some bug in the input device unregistering process
>>that doesn't account for ioctls.
>>
>>Anyway, I'll retest this issue soon.
>>
>
>
> And it will fail, locking is missing many parts of input core. Notice I
> said _should_, not will ;) I was trying to paint how it should work when
> we have proper locking and I don't want to use ff_lock to paper over
> some bugs in the core.
>
Ah, ok.
>>>>===================================================================
>>>>--- 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.
>
>>>Clearing FF bits is pointless here as device is about to disappear;
>>>locking is also not needed because we are guaranteed to be the last
>>>user of the device structure.
>>
>>True, if that guarantee really exists.
>>
> Yes, this is guaranteed.
>
So, now you guarantee it, but it isn't really so? ;)
When we remove locking, timer_del, clear_bit, all that is left is
kfree() and I guess that has to still be run in the input_dev_release().
--
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]
