lkml.org 
[lkml]   [2006]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 03/11] input: new force feedback interface
    Andrew Morton wrote:
    > 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 module handles effect type
    >>validations, effect timers, locking, etc.
    >>
    >>As a result, support is added for gain and envelope for memoryless devices,
    >>periodic => rumble conversion for memoryless devices and rumble => periodic
    >>conversion for devices with periodic support instead of rumble support. Also
    >>the effect memory of devices is not emptied if the root user opens and closes
    >>the device while another user is using effects. This module also obsoletes
    >>some flawed locking and timer code in few ff drivers.
    >>
    >>The module is named ff-effects. If INPUT_FF_EFFECTS is enabled, the force
    >>feedback drivers and interfaces (evdev) will be depending on it.
    >>
    >>Userspace interface is left unaltered.
    >>
    >
    >
    > Nice-looking patches.
    >

    Thanks for looking and providing helpful comments :)

    >>+#define spin_ff_cond_lock(_ff, _flags) \
    >>+ do { \
    >>+ if (!_ff->driver->playback) \
    >>+ spin_lock_irqsave(&_ff->atomiclock, _flags); \
    >>+ } while (0);
    >>+
    >>+#define spin_ff_cond_unlock(_ff, _flags) \
    >>+ do { \
    >>+ if (!_ff->driver->playback) \
    >>+ spin_unlock_irqrestore(&_ff->atomiclock, _flags); \
    >>+ } while (0);
    >
    >
    > Making these static inline functions would deuglify them a bit.
    >

    I put them like that because spin_lock_irqsave is a macro that uses the
    local variable "flags".
    But yes, I can probably pass that parameter to an inline function as a
    pointer. I'll change that.


    >>+static int input_ff_effect_access(struct input_dev *dev, int id, int override)
    >>+{
    >>+ struct ff_device *ff = dev->ff;
    >>+ if (id < dev->ff_effects_max && id >= 0 && test_bit(FF_EFFECT_USED, ff->effects[id].flags))
    >
    >
    > Kernel does have an 80-columns rule, but input seems to have always spurned it.
    >
    >
    >>+static int input_ff_envelope_time(struct ff_effect_status *effect, struct ff_envelope *envelope, unsigned long *event_time)
    >>+{
    >>+ unsigned long fade_start;
    >>+ if (!envelope)
    >>+ return 0;
    >>+
    >>+ if (envelope->attack_length && time_after(effect->play_at + msecs_to_jiffies(envelope->attack_length), effect->adj_at)) {
    >
    >
    > Try using an 80-column wondow for a while ;)

    Okay, I'll make the lines shorter.

    >
    >
    >>+ return value;
    >>+ }
    >>+
    >>+ difference = abs(value) - envelope_level;
    >>+
    >>+ debug("difference = %d", difference);
    >>+ debug("time_from_level = 0x%x", time_from_level);
    >>+ debug("time_of_envelope = 0x%x", time_of_envelope);
    >>+ if (difference < 0)
    >>+ difference = -((-difference) * time_from_level / time_of_envelope);
    >>+ else
    >>+ difference = difference * time_from_level / time_of_envelope;
    >
    >
    > You've checked there's no possibility of divide-by-zero here?
    >

    Yes, the "time_of_envelope" is set a few lines above from
    "envelope->attack_length" or "envelope->fade_length", and there is an if
    block that checks they're non-zero.

    >>+
    >>+static int input_ff_safe_sum(int a, int b, int limit) {
    >
    >
    > The opening brace goes in column zero, please.
    >
    >
    >>+ int c;
    >>+ if (!a)
    >>+ return b;
    >>+ c = a + b;
    >>+ if (c > limit)
    >>+ return limit;
    >>+ return c;
    >>+}
    >>+
    >>+static s8 input_ff_s8_sum(int a, int b) {
    >
    >
    > dittoes.

    Okay, will fix.

    >
    >>+ int c;
    >>+ c = input_ff_safe_sum(a, b, 0x7f);
    >>+ if (c < -0x80)
    >>+ return -0x80;
    >>+ return c;
    >>+}
    >>
    >>...
    >>
    >>+static void input_ff_timer(unsigned long timer_data)
    >>+{
    >>+ struct input_dev *dev = (struct input_dev *) timer_data;
    >>+ struct ff_device *ff = dev->ff;
    >>+ struct ff_effect effect;
    >>+ int i;
    >>+ unsigned long flags;
    >>+ int effects_pending;
    >>+ unsigned long effect_handled[NBITS(FF_EFFECTS_MAX)];
    >
    >
    > DECLARE_BITMAP would be more usual. (Yes, it should have been called
    > DEFINE_BITMAP).
    >

    Ok.

    >>+ int effect_type;
    >>+ int safety;
    >>+
    >>+ debug("timer: updating effects");
    >>+
    >>+ spin_lock_irqsave(&ff->atomiclock, flags);
    >>+
    >>+ memset(effect_handled, 0, sizeof(effect_handled));
    >
    >
    > You could take the lock after the memset.
    >

    Ok.

    >>+int input_ff_erase(struct input_dev *dev, int id)
    >>+{
    >>+ struct ff_device *ff;
    >>+ unsigned long flags = 0;
    >>+ int ret;
    >>+ if (!test_bit(EV_FF, dev->evbit))
    >>+ return -EINVAL;
    >>+ mutex_lock(&dev->ff_lock);
    >>+ ff = dev->ff;
    >>+ if (!ff) {
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return -ENODEV;
    >>+ }
    >>+ spin_ff_cond_lock(ff, flags);
    >>+ ret = _input_ff_erase(dev, id, current->pid == 0);
    >>+ spin_ff_cond_unlock(ff, flags);
    >>+
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return ret;
    >>+}
    >
    >
    > Perhaps you meant `current->uid == 0' here. There's no way in which pid
    > 0 will call this code.

    Right, a silly mistake.

    > What's happening here anyway? Why does this code need to know about pids?
    >
    > Checking for uid==0 woud be a fishy thing to do as well.

    User ID 0 is allowed to delete effects of other users. Pids are used to
    keep a track of what process owns what effects. This is the same
    behaviour as before.

    There is a problem with this, though:
    When a process closes any fd to this device, all pid-matching effects
    are deleted whether the process has another fd using the device or not.

    One solution would probably be to add some handle parameter to
    input_ff_upload() and input_ff_erase(), and then in
    evdev_ioctl_handler() pass an id unique to this fd. Then effects would
    be fd-specific, not pid-specific. I think the uid == 0 thing can also be
    dropped... I don't think the root user needs ability to override user
    effects (it can delete them anyway, just kill the user process owning
    the effects).

    WDYT?

    >
    >>+static int input_ff_flush(struct input_dev *dev, struct file *file)
    >>+{
    >>+ struct ff_device *ff;
    >>+ unsigned long flags = 0;
    >>+ int i;
    >>+ debug("flushing now");
    >>+ mutex_lock(&dev->ff_lock);
    >>+ ff = dev->ff;
    >>+ if (!ff) {
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return -ENODEV;
    >>+ }
    >>+ spin_ff_cond_lock(ff, flags);
    >>+ for (i = 0; i < dev->ff_effects_max; i++) {
    >>+ _input_ff_erase(dev, i, 0);
    >>+ }
    >
    >
    > Unneeded braces.
    >

    Will remove.

    >>+ spin_ff_cond_unlock(ff, flags);
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return 0;
    >>+}
    >>+
    >>+
    >>+ ff->effects[id].flags[0] = 0;
    >>+ ff->effects[id].effect = *effect;
    >>+
    >>+ if (ff->driver->playback) {
    >>+ if (!test_bit(effect->type, ff->flags))
    >>+ input_ff_convert_effect(dev, effect);
    >>+ ret = ff->driver->upload(dev, effect, NULL);
    >>+ if (!ret)
    >>+ set_bit(FF_EFFECT_USED, ff->effects[id].flags);
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return ret;
    >>+ }
    >>+ set_bit(FF_EFFECT_USED, ff->effects[id].flags);
    >>+
    >>+ } else {
    >>+ id = effect->id;
    >>+
    >>+ ret = input_ff_effect_access(dev, id, 1);
    >>+ if (ret) {
    >>+ spin_ff_cond_unlock(ff, flags);
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return ret;
    >>+ }
    >>+
    >>+ if (effect->type != ff->effects[id].effect.type ||
    >>+ (effect->type == FF_PERIODIC && effect->u.periodic.waveform !=
    >>+ ff->effects[id].effect.u.periodic.waveform)) {
    >>+ spin_ff_cond_unlock(ff, flags);
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return -EINVAL;
    >>+ }
    >>+
    >>+ if (ff->driver->playback) {
    >>+ if (!test_bit(effect->type, ff->flags))
    >>+ input_ff_convert_effect(dev, effect);
    >>+ ret = ff->driver->upload(dev, effect, &ff->effects[id].effect);
    >>+ ff->effects[id].effect = *effect;
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return ret;
    >
    >
    > I think we're missing a spin_ff_cond_unlock() here?

    Well, spin_ff_cond_unlock() checks for ff->driver->playback and it is
    already tested in the if block above.

    >
    >>+ }
    >>+ ff->effects[id].effect = *effect;
    >>+ clear_bit(FF_EFFECT_PLAYING, ff->effects[id].flags);
    >>+
    >>+ }
    >>+
    >>+ spin_unlock_irqrestore(&ff->atomiclock, flags);
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return ret;
    >>+}
    >
    >
    > And here we have spin_unlock_irqrestore() instead of spin_ff_cond_unlock().

    If there is no need to unlock, one of the above "if
    (ff->driver->playback)" would be true and the function would've already
    returned before this point.

    > It would be best to convert this function to have a single return point.
    > That tends to prevent problems like this from happening, and from creeping
    > in later on.

    I agree that the locking is too confusing in input_ff_event() and
    input_ff_upload(), even if it is correct.

    I'll modify the function to have a single return point, or maybe split
    the function for the two different locking paths, which then call common
    functions without the need to cond_lock. I guess that could be done for
    all cond_locking functions.

    >
    >>+int input_ff_allocate(struct input_dev *dev)
    >>+{
    >>+ debug("allocating device");
    >>+ mutex_lock(&dev->ff_lock);
    >>+ if (dev->ff)
    >>+ printk(KERN_ERR "ff-effects: allocating to non-NULL pointer\n");
    >>+ dev->ff = kzalloc(sizeof(*dev->ff), GFP_KERNEL);
    >>+ if (!dev->ff) {
    >>+ mutex_unlock(&dev->ff_lock);
    >>+ return -ENOMEM;
    >>+ }
    >>+ spin_lock_init(&dev->ff->atomiclock);
    >>+ init_timer(&dev->ff->timer);
    >>+ dev->ff->timer.function = input_ff_timer;
    >>+ dev->ff->timer.data = (unsigned long) dev;
    >>+ dev->ff->event = input_ff_event;
    >
    >
    > setup_timer()
    >

    Will change.

    >>+ mutex_unlock(&dev->ff_lock);
    >>+ debug("ff allocated");
    >>+ return 0;
    >>+}
    >>+
    >>
    >>...
    >>
    >>Index: linux-2.6.17-rc4-git1/drivers/input/Kconfig
    >>===================================================================
    >>--- linux-2.6.17-rc4-git1.orig/drivers/input/Kconfig 2006-03-20 07:53:29.000000000 +0200
    >>+++ linux-2.6.17-rc4-git1/drivers/input/Kconfig 2006-05-14 02:28:42.000000000 +0300
    >>@@ -24,6 +24,14 @@ config INPUT
    >>
    >> if INPUT
    >>
    >>+config INPUT_FF_EFFECTS
    >>+ tristate "Force feedback effects"
    >>+ help
    >>+ Say Y here if you want to be able to play force feedback effects.
    >>+
    >>+ To compile this driver as a module, choose M here: the
    >>+ module will be called ff-effects.
    >
    >
    > hm. I'd have expected more dependencies than this.
    >

    Only INPUT.

    >> comment "Userland interfaces"
    >>
    >> config INPUT_MOUSEDEV
    >>@@ -110,6 +118,7 @@ config INPUT_TSDEV_SCREEN_Y
    >>
    >> config INPUT_EVDEV
    >> tristate "Event interface"
    >>+ depends on INPUT_FF_EFFECTS || INPUT_FF_EFFECTS=n
    >
    >
    > Isn't that always true?
    >

    This disallows building evdev as builtin and ff-effects as module.

    >>+
    >>+struct ff_effect_status {
    >>+ pid_t owner;
    >
    >
    > This code is almost devoid of comments. Those which it does have tend to
    > cover little low-level implementation details. But it's the *big* things
    > which a reader is not able to learn from the implementation, and which
    > should be commented. Like: why on earth does this code need to know about
    > pids?

    Okay, I'll try to add some.

    >
    >>+#if defined(CONFIG_INPUT_FF_EFFECTS_MODULE) || defined(CONFIG_INPUT_FF_EFFECTS)
    >
    >
    > No, we shouldn't use CONFIG_FOO_MODULE. We just don't know at compile-time
    > whether the user will later compile and insert a particular module.
    >

    Right... so maybe we should just make ff-effects a bool instead of
    tristate or put it in the input module?

    >>+ mutex_lock(&dev->ff_lock);
    >>+ del_timer_sync(&ff->timer);
    >>+ dev->flush = NULL;
    >>+ dev->ff = NULL;
    >>+ kfree(ff);
    >>+ mutex_unlock(&dev->ff_lock);
    >
    >
    > The kfree can be moved outside the lock.
    >

    Indeed.

    BTW, what is the best way to send corrected patches for this patchset?
    Probably as a reply to the individual patches?


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

    \
     
     \ /
      Last update: 2006-05-22 18:13    [W:4.608 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site