Messages in this thread | | | Date | Tue, 06 Jun 2006 14:23:38 +0300 | From | Anssi Hannula <> | Subject | Re: [patch 03/12] input: new force feedback interface |
| |
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: >>> >>> >>>>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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |