Messages in this thread | | | Date | Tue, 24 Jul 2007 01:35:54 -0400 | From | Jeff Garzik <> | Subject | Re: [RFC/RFT 1/5] Input: implement proper locking in input core |
| |
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
- 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/
| |