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

["Dmitry Torokhov" <dmitry.torokhov@gmail.com>]

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.



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

I wonder if ff should be released right at unregister time...

> +               dev->flush = NULL;
> +               dev->ff = NULL;
> +               mutex_unlock(&dev->ff_lock);
> +               kfree(ff);
> +       }
> +
>        kfree(dev);
>        module_put(THIS_MODULE);
>  }

> +static inline int input_ff_safe_lock(struct input_dev *dev)
> +{
> +       mutex_lock(&dev->ff_lock);
> +       if (dev->ff)
> +               return 0;
> +
> +       mutex_unlock(&dev->ff_lock);
> +       return 1;
> +}

This needs to go away. Users should check whether a device supports FF
and if it is then it is device's responsibility to keep it there until
untregister time. We don't expect FF capabilities to flip/flop on a
live device.

> +static void input_ff_calc_timer(struct ff_device *ff)
> +{
> +       int i;
> +       int events = 0;
> +       unsigned long next_time = 0;
...
> +
> +               if (time_after(jiffies, event_time)) {
> +                       event_time = jiffies;

Should it be next_time = jiffies? We want to schedule thetimer ASAP, right?

> +
> +/**
> + * abs() with -0x8000 => 0x7fff exception
> + */
> +static inline u16 input_ff_unsign(s16 value)
> +{
> +       if (value == -0x8000)
> +               return 0x7fff;
> +
> +       return (value < 0 ? -value : value);
> +}

Why is it needed?

> +
> +/**
> + * Safe sum
> + * @a: Integer to sum
> + * @b: Integer to sum
> + * @limit: The sum limit
> + *
> + * If @a+@b is above @limit, return @limit
> + */
> +static int input_ff_safe_sum(int a, int b, int limit)
> +{
> +       int c;
> +       if (!a)
> +               return b;
> +       c = a + b;
> +       if (c > limit)
> +               return limit;
> +       return c;
> +}

As it was mentioned the result will not be limited if a == 0. Is it intended?

PLease don;t start making any changes yet, I am still looking...

-- 
Dmitry