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.
>
>> ===================================================================
>> --- 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.
> 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.
> 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.
This too can be removed if it is guaranteed that the device is not
deleted while ioctl is executing.
>> +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?
Yes, a good catch.
>> +
>> +/**
>> + * 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?
Oh, well... the maximum value of s16 is 0x7fff in positive side, -0x8000
in negative side. In input_ff_sum_effect() (apparently the only function
that uses this) the "i = i * gain / 0x7fff" uses the maximum value of
0x7fff.
Then again, I guess it wouldn't really matter if that exception is just
skipped and then detect the small overflow in input_ff_safe_sum().
>
>> +
>> +/**
>> + * 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?
Well, b is guaranteed by the caller to be below the limit. However, if
that input_ff_unsign() stuff is dropped in favor of abs(), b could be
above limit and then if(!a) should be dropped.
> PLease don;t start making any changes yet, I am still looking...
>
Ok.
--
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]
