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

[Dmitry Torokhov <dtor_core@ameritech.net>]

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.
 
> > 
> >> ===================================================================
> >> --- 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).

> > 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.

-- 
Dmitry