| Message ID | 20200608112211.12125-8-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support inhibiting input devices | expand |
Hi Andrzej, On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote: > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev, > case EV_KEY: > if (is_event_supported(code, dev->keybit, KEY_MAX)) { > > - /* auto-repeat bypasses state updates */ > - if (value == 2) { > + /* > + * auto-repeat bypasses state updates but repeat > + * events are ignored if the key is not pressed > + */ > + if (value == 2 && test_bit(code, dev->key)) { > disposition = INPUT_PASS_TO_HANDLERS; > break; > } Is this chunk really part of inhibit support? I'd think we cancel autorepeat timer when we are releasing a key, no? Thanks.
Hi Dmitry, W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze: > Hi Andrzej, > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote: >> @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev, >> case EV_KEY: >> if (is_event_supported(code, dev->keybit, KEY_MAX)) { >> >> - /* auto-repeat bypasses state updates */ >> - if (value == 2) { >> + /* >> + * auto-repeat bypasses state updates but repeat >> + * events are ignored if the key is not pressed >> + */ >> + if (value == 2 && test_bit(code, dev->key)) { >> disposition = INPUT_PASS_TO_HANDLERS; >> break; >> } > > Is this chunk really part of inhibit support? I'd think we cancel > autorepeat timer when we are releasing a key, no? > When I look at it now it seems to me the chunk might be redundant. But let me explain what I had in mind when adding it. It is a matter of what we do with input events generated while a device is inhibited. If ->open()/->close() are not provided by the driver then inhibiting amounts to merely ignoring input events from a device while it remains active. What else can you do if the driver does not provide a method to prepare the device for generating events/ to stop generating events? In this special case a user might trigger a repeated event while the device is inhibited, then the user keeps holding the key down and the device is uninhibited. Do we pass anything to handlers then? In my opinion we should not. Such an event is "illegal" in a sense that it was generated at a time when nobody wanted any events from the device. Hence the test to let only those auto-repeat events through for which a key is actually pressed. However, what I see now is that if a device is inhibited, no key will ever reach neither the "1" nor "2" state because of the "if" in the very beginning of input_handle_event(). Regards, Andrzej
On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote: > Hi Dmitry, > > W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze: > > Hi Andrzej, > > > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote: > > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev, > > > case EV_KEY: > > > if (is_event_supported(code, dev->keybit, KEY_MAX)) { > > > - /* auto-repeat bypasses state updates */ > > > - if (value == 2) { > > > + /* > > > + * auto-repeat bypasses state updates but repeat > > > + * events are ignored if the key is not pressed > > > + */ > > > + if (value == 2 && test_bit(code, dev->key)) { > > > disposition = INPUT_PASS_TO_HANDLERS; > > > break; > > > } > > > > Is this chunk really part of inhibit support? I'd think we cancel > > autorepeat timer when we are releasing a key, no? > > > > When I look at it now it seems to me the chunk might be redundant. > But let me explain what I had in mind when adding it. > > It is a matter of what we do with input events generated while a > device is inhibited. If ->open()/->close() are not provided by the > driver then inhibiting amounts to merely ignoring input events from > a device while it remains active. What else can you do if the driver > does not provide a method to prepare the device for generating events/ > to stop generating events? > > In this special case a user might trigger a repeated event while the > device is inhibited, then the user keeps holding the key down and the > device is uninhibited. Do we pass anything to handlers then? > > In my opinion we should not. Such an event is "illegal" in a sense that it > was generated at a time when nobody wanted any events from the device. > Hence the test to let only those auto-repeat events through for which > a key is actually pressed. > > However, what I see now is that if a device is inhibited, no key > will ever reach neither the "1" nor "2" state because of the "if" > in the very beginning of input_handle_event(). OK, then let's drop it for now. We can revisit if we see that a problem. Thanks.
On Tue, Oct 06, 2020 at 06:11:02PM -0700, Dmitry Torokhov wrote: > On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote: > > Hi Dmitry, > > > > W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze: > > > Hi Andrzej, > > > > > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote: > > > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev, > > > > case EV_KEY: > > > > if (is_event_supported(code, dev->keybit, KEY_MAX)) { > > > > - /* auto-repeat bypasses state updates */ > > > > - if (value == 2) { > > > > + /* > > > > + * auto-repeat bypasses state updates but repeat > > > > + * events are ignored if the key is not pressed > > > > + */ > > > > + if (value == 2 && test_bit(code, dev->key)) { > > > > disposition = INPUT_PASS_TO_HANDLERS; > > > > break; > > > > } > > > > > > Is this chunk really part of inhibit support? I'd think we cancel > > > autorepeat timer when we are releasing a key, no? > > > > > > > When I look at it now it seems to me the chunk might be redundant. > > But let me explain what I had in mind when adding it. > > > > It is a matter of what we do with input events generated while a > > device is inhibited. If ->open()/->close() are not provided by the > > driver then inhibiting amounts to merely ignoring input events from > > a device while it remains active. What else can you do if the driver > > does not provide a method to prepare the device for generating events/ > > to stop generating events? > > > > In this special case a user might trigger a repeated event while the > > device is inhibited, then the user keeps holding the key down and the > > device is uninhibited. Do we pass anything to handlers then? > > > > In my opinion we should not. Such an event is "illegal" in a sense that it > > was generated at a time when nobody wanted any events from the device. > > Hence the test to let only those auto-repeat events through for which > > a key is actually pressed. > > > > However, what I see now is that if a device is inhibited, no key > > will ever reach neither the "1" nor "2" state because of the "if" > > in the very beginning of input_handle_event(). > > OK, then let's drop it for now. We can revisit if we see that a problem. And by that I mean that I will drop it myself, no need to resend. I will be applying this shortly. Thanks.
On Tue, Oct 06, 2020 at 06:12:49PM -0700, Dmitry Torokhov wrote: > On Tue, Oct 06, 2020 at 06:11:02PM -0700, Dmitry Torokhov wrote: > > On Tue, Oct 06, 2020 at 03:04:28PM +0200, Andrzej Pietrasiewicz wrote: > > > Hi Dmitry, > > > > > > W dniu 05.10.2020 o 20:10, Dmitry Torokhov pisze: > > > > Hi Andrzej, > > > > > > > > On Mon, Jun 08, 2020 at 01:22:11PM +0200, Andrzej Pietrasiewicz wrote: > > > > > @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev, > > > > > case EV_KEY: > > > > > if (is_event_supported(code, dev->keybit, KEY_MAX)) { > > > > > - /* auto-repeat bypasses state updates */ > > > > > - if (value == 2) { > > > > > + /* > > > > > + * auto-repeat bypasses state updates but repeat > > > > > + * events are ignored if the key is not pressed > > > > > + */ > > > > > + if (value == 2 && test_bit(code, dev->key)) { > > > > > disposition = INPUT_PASS_TO_HANDLERS; > > > > > break; > > > > > } > > > > > > > > Is this chunk really part of inhibit support? I'd think we cancel > > > > autorepeat timer when we are releasing a key, no? > > > > > > > > > > When I look at it now it seems to me the chunk might be redundant. > > > But let me explain what I had in mind when adding it. > > > > > > It is a matter of what we do with input events generated while a > > > device is inhibited. If ->open()/->close() are not provided by the > > > driver then inhibiting amounts to merely ignoring input events from > > > a device while it remains active. What else can you do if the driver > > > does not provide a method to prepare the device for generating events/ > > > to stop generating events? > > > > > > In this special case a user might trigger a repeated event while the > > > device is inhibited, then the user keeps holding the key down and the > > > device is uninhibited. Do we pass anything to handlers then? > > > > > > In my opinion we should not. Such an event is "illegal" in a sense that it > > > was generated at a time when nobody wanted any events from the device. > > > Hence the test to let only those auto-repeat events through for which > > > a key is actually pressed. > > > > > > However, what I see now is that if a device is inhibited, no key > > > will ever reach neither the "1" nor "2" state because of the "if" > > > in the very beginning of input_handle_event(). > > > > OK, then let's drop it for now. We can revisit if we see that a problem. > > And by that I mean that I will drop it myself, no need to resend. I will > be applying this shortly. Well, "shortly" was just a tad optimistic, but I did apply it ;) Thanks.
diff --git a/drivers/input/input.c b/drivers/input/input.c index 41377bfa142d..f624b09a1f00 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -284,8 +284,11 @@ static int input_get_disposition(struct input_dev *dev, case EV_KEY: if (is_event_supported(code, dev->keybit, KEY_MAX)) { - /* auto-repeat bypasses state updates */ - if (value == 2) { + /* + * auto-repeat bypasses state updates but repeat + * events are ignored if the key is not pressed + */ + if (value == 2 && test_bit(code, dev->key)) { disposition = INPUT_PASS_TO_HANDLERS; break; } @@ -367,8 +370,13 @@ static int input_get_disposition(struct input_dev *dev, static void input_handle_event(struct input_dev *dev, unsigned int type, unsigned int code, int value) { - int disposition = input_get_disposition(dev, type, code, &value); + int disposition; + + /* filter-out events from inhibited devices */ + if (dev->inhibited) + return; + disposition = input_get_disposition(dev, type, code, &value); if (disposition != INPUT_IGNORE_EVENT && type != EV_SYN) add_input_randomness(type, code, value); @@ -612,10 +620,10 @@ int input_open_device(struct input_handle *handle) handle->open++; - if (dev->users++) { + if (dev->users++ || dev->inhibited) { /* - * Device is already opened, so we can exit immediately and - * report success. + * Device is already opened and/or inhibited, + * so we can exit immediately and report success. */ goto out; } @@ -675,10 +683,9 @@ void input_close_device(struct input_handle *handle) __input_release_device(handle); - if (!--dev->users) { + if (!dev->inhibited && !--dev->users) { if (dev->poller) input_dev_poller_stop(dev->poller); - if (dev->close) dev->close(dev); } @@ -1416,12 +1423,49 @@ static ssize_t input_dev_show_properties(struct device *dev, } static DEVICE_ATTR(properties, S_IRUGO, input_dev_show_properties, NULL); +static int input_inhibit_device(struct input_dev *dev); +static int input_uninhibit_device(struct input_dev *dev); + +static ssize_t inhibited_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct input_dev *input_dev = to_input_dev(dev); + + return scnprintf(buf, PAGE_SIZE, "%d\n", input_dev->inhibited); +} + +static ssize_t inhibited_store(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t len) +{ + struct input_dev *input_dev = to_input_dev(dev); + ssize_t rv; + bool inhibited; + + if (strtobool(buf, &inhibited)) + return -EINVAL; + + if (inhibited) + rv = input_inhibit_device(input_dev); + else + rv = input_uninhibit_device(input_dev); + + if (rv != 0) + return rv; + + return len; +} + +static DEVICE_ATTR_RW(inhibited); + static struct attribute *input_dev_attrs[] = { &dev_attr_name.attr, &dev_attr_phys.attr, &dev_attr_uniq.attr, &dev_attr_modalias.attr, &dev_attr_properties.attr, + &dev_attr_inhibited.attr, NULL }; @@ -1703,6 +1747,63 @@ void input_reset_device(struct input_dev *dev) } EXPORT_SYMBOL(input_reset_device); +static int input_inhibit_device(struct input_dev *dev) +{ + int ret = 0; + + mutex_lock(&dev->mutex); + + if (dev->inhibited) + goto out; + + if (dev->users) { + if (dev->close) + dev->close(dev); + if (dev->poller) + input_dev_poller_stop(dev->poller); + } + + spin_lock_irq(&dev->event_lock); + input_dev_release_keys(dev); + input_dev_toggle(dev, false); + spin_unlock_irq(&dev->event_lock); + + dev->inhibited = true; + +out: + mutex_unlock(&dev->mutex); + return ret; +} + +static int input_uninhibit_device(struct input_dev *dev) +{ + int ret = 0; + + mutex_lock(&dev->mutex); + + if (!dev->inhibited) + goto out; + + if (dev->users) { + if (dev->open) { + ret = dev->open(dev); + if (ret) + goto out; + } + if (dev->poller) + input_dev_poller_start(dev->poller); + } + + dev->inhibited = false; + spin_lock_irq(&dev->event_lock); + input_dev_toggle(dev, true); + spin_unlock_irq(&dev->event_lock); + +out: + mutex_unlock(&dev->mutex); + return ret; +} + #ifdef CONFIG_PM_SLEEP static int input_dev_suspend(struct device *dev) { @@ -2131,7 +2232,7 @@ bool input_device_enabled(struct input_dev *dev) { lockdep_assert_held(&dev->mutex); - return dev->users > 0; + return !dev->inhibited && dev->users > 0; } EXPORT_SYMBOL_GPL(input_device_enabled); diff --git a/include/linux/input.h b/include/linux/input.h index eda4587dba67..0354b298d874 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -90,9 +90,11 @@ enum input_clock_type { * @open: this method is called when the very first user calls * input_open_device(). The driver must prepare the device * to start generating events (start polling thread, - * request an IRQ, submit URB, etc.) + * request an IRQ, submit URB, etc.). The meaning of open() is + * to start providing events to the input core. * @close: this method is called when the very last user calls - * input_close_device(). + * input_close_device(). The meaning of close() is to stop + * providing events to the input core. * @flush: purges the device. Most commonly used to get rid of force * feedback effects loaded into the device when disconnecting * from it @@ -127,6 +129,10 @@ enum input_clock_type { * and needs not be explicitly unregistered or freed. * @timestamp: storage for a timestamp set by input_set_timestamp called * by a driver + * @inhibited: indicates that the input device is inhibited. If that is + * the case then input core ignores any events generated by the device. + * Device's close() is called when it is being inhibited and its open() + * is called when it is being uninhibited. */ struct input_dev { const char *name; @@ -201,6 +207,8 @@ struct input_dev { bool devres_managed; ktime_t timestamp[INPUT_CLK_MAX]; + + bool inhibited; }; #define to_input_dev(d) container_of(d, struct input_dev, dev)