Re: [RFC/RFT 1/5] Input: implement proper locking in input core

Jeff Garzik
2007-07-24T05:36:17Z

Dmitry Torokhov wrote:
> +static void input_repeat_key(unsigned long data)
> +{
> + struct input_dev *dev = (void *) data;
>
> - change_bit(code, dev->key);
> + spin_lock_irq(&dev->event_lock);
[...]
> +void input_inject_event(struct input_handle *handle,
> + unsigned int type, unsigned int code, int value)
> {
> - struct input_dev *dev = (void *) data;
> + struct input_dev *dev = handle->dev;
> + struct input_handle *grab;
>
> - if (!test_bit(dev->repeat_key, dev->key))
> - return;
> + if (is_event_supported(type, dev->evbit, EV_MAX)) {
> + spin_lock_irq(&dev->event_lock);
>
> - input_event(dev, EV_KEY, dev->repeat_key, 2);
> - input_sync(dev);
> + grab = rcu_dereference(dev->grab);
> + if (!grab || grab == handle)
> + input_handle_event(dev, type, code, value);
>
> - if (dev->rep[REP_PERIOD])
> - mod_timer(&dev->timer, jiffies + msecs_to_jiffies(dev->rep[REP_PERIOD]));
> + spin_unlock_irq(&dev->event_lock);
> + }
> }
> +EXPORT_SYMBOL(input_inject_event);
[...]
> + spin_lock_irq(&dev->event_lock);
> +
> + /*
> + * Simulate keyup events for all pressed keys so that handlers
> + * are not left with "stuck" keys. The driver may continue
> + * generate events even after we done here but they will not
> + * reach any handlers.
> + */
> + if (is_event_supported(EV_KEY, dev->evbit, EV_MAX)) {
> + for (code = 0; code <= KEY_MAX; code++) {
> + if (is_event_supported(code, dev->keybit, KEY_MAX) &&
> + test_bit(code, dev->key)) {
> + input_pass_event(dev, EV_KEY, code, 0);
> + }
> + }
> + input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
> + }
> +
> + list_for_each_entry(handle, &dev->h_list, d_node)
> + handle->open = 0;
> +
> + spin_unlock_irq(&dev->event_lock);

spin_lock_irq() should generally be avoided.

In cases like the first case -- input_repeat_key() -- you are making
incorrect assumptions about the state of interrupts. The other cases
are probably ok, but in general spin_lock_irq() has a long history of
being very fragile and quite often wrong.

Use spin_lock_irqsave() to be safe. Definitely in input_repeat_key(),
but I strongly recommend removing spin_lock_irq() from all your patches
here.

Jeff