| Message ID | 1249007207-27392-1-git-send-email-mjg@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Matthew, On Fri, Jul 31, 2009 at 03:26:46AM +0100, Matthew Garrett wrote: > Devices occasionally use the keyboard controller to send hardware > notifications that should be handled by the kernel rather than userspace. > These can be handled in kernel by registering an input handler, but the > event will still bubble up to userspace where it may cause confusion. This > patch adds support for the kernel to register filters, allowing it to > indicate that the event should be consumed by the hardware-specific driver > rather than passed to other input handlers. The assumption is that, as this > is hardware specific, there should be no need to have more than one filter - > similarly, if an input device is grabbed, we assume that people know what > they're doing and don't pass it to the filter. > I don't think this is the proper layer to do the filtering, the filter should be done in i8042, not input core. The way you done it would kill any possibility of user assigning KEY_WLAN to a spare key on his or her keyboard and using it to control wireless cards that are not hardwired. I would accept the patch that would add a filter in the form bool (*i8042_fillter)(unsigned char data, unsigned int flags, struct serio *port); I don't think we'd need to chain them into a list or something, one hook should be enough since I don't expect we'll have several platform drivers loaded on one box.
On Mon, Aug 03, 2009 at 10:04:51AM -0700, Dmitry Torokhov wrote: > I don't think this is the proper layer to do the filtering, the filter > should be done in i8042, not input core. The way you done it would kill > any possibility of user assigning KEY_WLAN to a spare key on his or her > keyboard and using it to control wireless cards that are not hardwired. Ok, that seems fine. > I would accept the patch that would add a filter in the form > > bool (*i8042_fillter)(unsigned char data, unsigned int flags, > struct serio *port); > > I don't think we'd need to chain them into a list or something, one hook > should be enough since I don't expect we'll have several platform > drivers loaded on one box. So filter on the raw i8042 data rather than after any processing? Wouldn't this require some level of state machine in the driver using this functionality?
On Tue, Aug 04, 2009 at 11:01:58AM +0100, Matthew Garrett wrote: > On Mon, Aug 03, 2009 at 10:04:51AM -0700, Dmitry Torokhov wrote: > > > I don't think this is the proper layer to do the filtering, the filter > > should be done in i8042, not input core. The way you done it would kill > > any possibility of user assigning KEY_WLAN to a spare key on his or her > > keyboard and using it to control wireless cards that are not hardwired. > > Ok, that seems fine. > > > I would accept the patch that would add a filter in the form > > > > bool (*i8042_fillter)(unsigned char data, unsigned int flags, > > struct serio *port); > > > > I don't think we'd need to chain them into a list or something, one hook > > should be enough since I don't expect we'll have several platform > > drivers loaded on one box. > > So filter on the raw i8042 data rather than after any processing? > Wouldn't this require some level of state machine in the driver using > this functionality? > Yes, but it should not be too complex - with laptops you can be pretty much sure keyboard is in translated set 2 mode.
On Tue, 04 Aug 2009, Dmitry Torokhov wrote: > On Tue, Aug 04, 2009 at 11:01:58AM +0100, Matthew Garrett wrote: > > On Mon, Aug 03, 2009 at 10:04:51AM -0700, Dmitry Torokhov wrote: > > > I don't think this is the proper layer to do the filtering, the filter > > > should be done in i8042, not input core. The way you done it would kill > > > any possibility of user assigning KEY_WLAN to a spare key on his or her > > > keyboard and using it to control wireless cards that are not hardwired. > > > > Ok, that seems fine. > > > > > I would accept the patch that would add a filter in the form > > > > > > bool (*i8042_fillter)(unsigned char data, unsigned int flags, > > > struct serio *port); > > > > > > I don't think we'd need to chain them into a list or something, one hook > > > should be enough since I don't expect we'll have several platform > > > drivers loaded on one box. > > > > So filter on the raw i8042 data rather than after any processing? > > Wouldn't this require some level of state machine in the driver using > > this functionality? > > Yes, but it should not be too complex - with laptops you can be pretty > much sure keyboard is in translated set 2 mode. But don't we have a crapload of implementation weirdness work-arounds in the i8042 drivers, including some for problems with the raw protocol itself? We wouldn't want to duplicate THAT... will the filter be placed somewhere that doesn't have to deal with that kind of bogosity? With laptops, you can be sure that 99% of the time, the i8042 is just firmware running inside the EC, and subject to a LOT MORE bugs and incompetence than an emulated i8042 inside the chipset would be...
On Aug 6, 2009, at 5:49 AM, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote: > On Tue, 04 Aug 2009, Dmitry Torokhov wrote: >> On Tue, Aug 04, 2009 at 11:01:58AM +0100, Matthew Garrett wrote: >>> On Mon, Aug 03, 2009 at 10:04:51AM -0700, Dmitry Torokhov wrote: >>>> I don't think this is the proper layer to do the filtering, the >>>> filter >>>> should be done in i8042, not input core. The way you done it >>>> would kill >>>> any possibility of user assigning KEY_WLAN to a spare key on his >>>> or her >>>> keyboard and using it to control wireless cards that are not >>>> hardwired. >>> >>> Ok, that seems fine. >>> >>>> I would accept the patch that would add a filter in the form >>>> >>>> bool (*i8042_fillter)(unsigned char data, unsigned int flags, >>>> struct serio *port); >>>> >>>> I don't think we'd need to chain them into a list or something, >>>> one hook >>>> should be enough since I don't expect we'll have several platform >>>> drivers loaded on one box. >>> >>> So filter on the raw i8042 data rather than after any processing? >>> Wouldn't this require some level of state machine in the driver >>> using >>> this functionality? >> >> Yes, but it should not be too complex - with laptops you can be >> pretty >> much sure keyboard is in translated set 2 mode. > > But don't we have a crapload of implementation weirdness work- > arounds in the > i8042 drivers, including some for problems with the raw protocol > itself? We > wouldn't want to duplicate THAT... There actually not many quirks there, once we identified all ports on i8042; mostly we need to deal with keys not sending release events. > will the filter be placed somewhere that > doesn't have to deal with that kind of bogosity? The problem with doing this on input core level is that we don't always even have a keycode, nor do we want one. For example my d630 sends some crap through keyboard serio port when it is unhappy with power adapter...
On Thu, 06 Aug 2009, Dmitry Torokhov wrote: > >But don't we have a crapload of implementation weirdness work- > >arounds in the > >i8042 drivers, including some for problems with the raw protocol > >itself? We > >wouldn't want to duplicate THAT... > > There actually not many quirks there, once we identified all ports > on i8042; mostly we need to deal with keys not sending release > events. > > >will the filter be placed somewhere that > >doesn't have to deal with that kind of bogosity? > > The problem with doing this on input core level is that we don't It doesn't have to be on the input core, it could be on i8042, but the hook should be AFTER i8042 did all the processing and cleanup that we want to take place...
diff --git a/drivers/input/input.c b/drivers/input/input.c index 7c237e6..80f1e48 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -88,19 +88,26 @@ static int input_defuzz_abs_event(int value, int old_val, int fuzz) */ static void input_pass_event(struct input_dev *dev, unsigned int type, unsigned int code, int value) -{ - struct input_handle *handle; + +{ struct input_handle *handle; rcu_read_lock(); handle = rcu_dereference(dev->grab); - if (handle) + if (handle) { handle->handler->event(handle, type, code, value); - else - list_for_each_entry_rcu(handle, &dev->h_list, d_node) - if (handle->open) - handle->handler->event(handle, - type, code, value); + goto out; + } + + handle = rcu_dereference(dev->filter); + if (handle && handle->handler->filter(handle, type, code, value)) + goto out; + + list_for_each_entry_rcu(handle, &dev->h_list, d_node) + if (handle->open) + handle->handler->event(handle, + type, code, value); +out: rcu_read_unlock(); } @@ -375,12 +382,15 @@ int input_grab_device(struct input_handle *handle) } EXPORT_SYMBOL(input_grab_device); -static void __input_release_device(struct input_handle *handle) +static void __input_release_device(struct input_handle *handle, bool filter) { struct input_dev *dev = handle->dev; - if (dev->grab == handle) { - rcu_assign_pointer(dev->grab, NULL); + if (handle == (filter ? dev->filter : dev->grab)) { + if (filter) + rcu_assign_pointer(dev->filter, NULL); + else + rcu_assign_pointer(dev->grab, NULL); /* Make sure input_pass_event() notices that grab is gone */ synchronize_rcu(); @@ -404,12 +414,65 @@ void input_release_device(struct input_handle *handle) struct input_dev *dev = handle->dev; mutex_lock(&dev->mutex); - __input_release_device(handle); + __input_release_device(handle, false); mutex_unlock(&dev->mutex); } EXPORT_SYMBOL(input_release_device); /** + * input_filter_device - allow input events to be filtered from higher layers + * @handle: input handle that wants to filter the device + * + * When a device is filtered by an input handle all events generated by + * the device are to this handle. If the filter function returns true then + * the event is discarded rather than being passed to any other input handles, + * otherwise it is passed to them as normal. Grabs will be handled before + * filters, so a grabbed device will not deliver events to a filter function. + */ +int input_filter_device(struct input_handle *handle) +{ + struct input_dev *dev = handle->dev; + int retval; + + retval = mutex_lock_interruptible(&dev->mutex); + if (retval) + return retval; + + if (dev->filter) { + retval = -EBUSY; + goto out; + } + + rcu_assign_pointer(dev->filter, handle); + synchronize_rcu(); + + out: + mutex_unlock(&dev->mutex); + return retval; +} +EXPORT_SYMBOL(input_filter_device); + +/** + * input_unfilter_device - removes a filter from a device + * @handle: input handle that owns the device + * + * Removes the filter from a device so that other input handles can + * start receiving unfiltered input events. Upon release all handlers + * attached to the device have their start() method called so they + * have a change to synchronize device state with the rest of the + * system. + */ +void input_unfilter_device(struct input_handle *handle) +{ + struct input_dev *dev = handle->dev; + + mutex_lock(&dev->mutex); + __input_release_device(handle, true); + mutex_unlock(&dev->mutex); +} +EXPORT_SYMBOL(input_unfilter_device); + +/** * input_open_device - open input device * @handle: handle through which device is being accessed * @@ -482,7 +545,9 @@ void input_close_device(struct input_handle *handle) mutex_lock(&dev->mutex); - __input_release_device(handle); + /* Release both grabs and filters */ + __input_release_device(handle, false); + __input_release_device(handle, true); if (!--dev->users && dev->close) dev->close(dev); diff --git a/include/linux/input.h b/include/linux/input.h index 8b3bc3e..e28f116 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -1118,6 +1118,7 @@ struct input_dev { int (*event)(struct input_dev *dev, unsigned int type, unsigned int code, int value); struct input_handle *grab; + struct input_handle *filter; spinlock_t event_lock; struct mutex mutex; @@ -1218,6 +1219,7 @@ struct input_handler { void *private; void (*event)(struct input_handle *handle, unsigned int type, unsigned int code, int value); + bool (*filter)(struct input_handle *handle, unsigned int type, unsigned int code, int value); int (*connect)(struct input_handler *handler, struct input_dev *dev, const struct input_device_id *id); void (*disconnect)(struct input_handle *handle); void (*start)(struct input_handle *handle); @@ -1295,6 +1297,9 @@ void input_unregister_handle(struct input_handle *); int input_grab_device(struct input_handle *); void input_release_device(struct input_handle *); +int input_filter_device(struct input_handle *); +void input_unfilter_device(struct input_handle *); + int input_open_device(struct input_handle *); void input_close_device(struct input_handle *);
Devices occasionally use the keyboard controller to send hardware notifications that should be handled by the kernel rather than userspace. These can be handled in kernel by registering an input handler, but the event will still bubble up to userspace where it may cause confusion. This patch adds support for the kernel to register filters, allowing it to indicate that the event should be consumed by the hardware-specific driver rather than passed to other input handlers. The assumption is that, as this is hardware specific, there should be no need to have more than one filter - similarly, if an input device is grabbed, we assume that people know what they're doing and don't pass it to the filter. Signed-off-by: Matthew Garrett <mjg@redhat.com> --- drivers/input/input.c | 91 ++++++++++++++++++++++++++++++++++++++++++------- include/linux/input.h | 5 +++ 2 files changed, 83 insertions(+), 13 deletions(-)