| Message ID | 20200605173335.13753-8-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Support inhibiting input devices | expand |
Hi, On 6/5/20 7:33 PM, Andrzej Pietrasiewicz wrote: > From: Patrik Fimml <patrikf@chromium.org> > > Userspace might want to implement a policy to temporarily disregard input > from certain devices, including not treating them as wakeup sources. > > An example use case is a laptop, whose keyboard can be folded under the > screen to create tablet-like experience. The user then must hold the laptop > in such a way that it is difficult to avoid pressing the keyboard keys. It > is therefore desirable to temporarily disregard input from the keyboard, > until it is folded back. This obviously is a policy which should be kept > out of the kernel, but the kernel must provide suitable means to implement > such a policy. > > This patch adds a sysfs interface for exactly this purpose. > > To implement the said interface it adds an "inhibited" property to struct > input_dev, and effectively creates four states a device can be in: closed > uninhibited, closed inhibited, open uninhibited, open inhibited. It also > defers calling driver's ->open() and ->close() to until they are actually > needed, e.g. it makes no sense to prepare the underlying device for > generating events (->open()) if the device is inhibited. > > uninhibit > closed <------------ closed > uninhibited ------------> inhibited > | ^ inhibit | ^ > 1st | | 1st | | > open | | open | | > | | | | > | | last | | last > | | close | | close > v | uninhibit v | > open <------------ open > uninhibited ------------> inhibited > > The top inhibit/uninhibit transition happens when users == 0. > The bottom inhibit/uninhibit transition happens when users > 0. > The left open/close transition happens when !inhibited. > The right open/close transition happens when inhibited. > Due to all transitions being serialized with dev->mutex, it is impossible > to have "diagonal" transitions between closed uninhibited and open > inhibited or between open uninhibited and closed inhibited. > > No new callbacks are added to drivers, because their open() and close() > serve exactly the purpose to tell the driver to start/stop providing > events to the input core. Consequently, open() and close() - if provided > - are called in both inhibit and uninhibit paths. > > Signed-off-by: Patrik Fimml <patrikf@chromium.org> > Co-developed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/input/input.c | 115 +++++++++++++++++++++++++++++++++++++++--- > include/linux/input.h | 12 ++++- > 2 files changed, 118 insertions(+), 9 deletions(-) > > diff --git a/drivers/input/input.c b/drivers/input/input.c > index 41377bfa142d..4110b5797219 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,7 +620,7 @@ 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. It seems the comment which is part of the context here may need some updating because of the inhibit changes ? Otherwise this patch and the rest of the series looks good to me now. Thank you very much for your work on this. Regards, Hans > @@ -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) > >
This is a quick respin of v3, with just two small changes, please see the changelog below. Userspace might want to implement a policy to temporarily disregard input from certain devices. An example use case is a convertible laptop, whose keyboard can be folded under the screen to create tablet-like experience. The user then must hold the laptop in such a way that it is difficult to avoid pressing the keyboard keys. It is therefore desirable to temporarily disregard input from the keyboard, until it is folded back. This obviously is a policy which should be kept out of the kernel, but the kernel must provide suitable means to implement such a policy. Due to interactions with suspend/resume, a helper has been added for drivers to decide if the device is being used or not (PATCH 1/7) and it has been applied to relevant drivers (PATCH 2,4,5,6/7). PATCH 7/7 adds support for inhibiting input devices. This work is inspired by: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/45c2d7bb398f74adfae0017e20b224152fde3822 and https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4ce0e8a3697edb8fd071110b3af65014512061c7 In this respin the elan_i2c patch is dropped and converting it will be addressed later. v3..v4: - updated the comment in input_open_device() (Hans) - used more straightforward locking pattern in adc/exynos (Michał) v2..v3: - ignored autorepeat events in input_get_disposition() if a key is not pressed (Hans) - dropped inhibit()/uninhibit() driver callbacks (Hans) - split ACPI button patch into taking the lock and using the helper (Rafael) - dropped the elan_i2c conversion - fixed typos in exynos adc v1..v2: - added input_device_enabled() helper and used it in drivers (Dmitry) - the fact of open() and close() being called in inhibit/uninhibit paths has been emphasized in the commit message of PATCH 6/7 (Dmitry) Andrzej Pietrasiewicz (6): Input: add input_device_enabled() Input: use input_device_enabled() ACPI: button: Access input device's users under appropriate mutex ACPI: button: Use input_device_enabled() helper iio: adc: exynos: Use input_device_enabled() platform/x86: thinkpad_acpi: Use input_device_enabled() Patrik Fimml (1): Input: Add "inhibited" property drivers/acpi/button.c | 7 +- drivers/iio/adc/exynos_adc.c | 6 +- drivers/input/input.c | 125 ++++++++++++++++++-- drivers/input/joystick/xpad.c | 4 +- drivers/input/keyboard/ep93xx_keypad.c | 2 +- drivers/input/keyboard/gpio_keys.c | 4 +- drivers/input/keyboard/imx_keypad.c | 4 +- drivers/input/keyboard/ipaq-micro-keys.c | 2 +- drivers/input/keyboard/lpc32xx-keys.c | 4 +- drivers/input/keyboard/pmic8xxx-keypad.c | 4 +- drivers/input/keyboard/pxa27x_keypad.c | 2 +- drivers/input/keyboard/samsung-keypad.c | 4 +- drivers/input/keyboard/spear-keyboard.c | 8 +- drivers/input/keyboard/st-keyscan.c | 4 +- drivers/input/keyboard/tegra-kbc.c | 4 +- drivers/input/misc/drv260x.c | 4 +- drivers/input/misc/drv2665.c | 4 +- drivers/input/misc/drv2667.c | 4 +- drivers/input/misc/gp2ap002a00f.c | 4 +- drivers/input/misc/kxtj9.c | 4 +- drivers/input/misc/sirfsoc-onkey.c | 2 +- drivers/input/mouse/navpoint.c | 4 +- drivers/input/touchscreen/ad7879.c | 6 +- drivers/input/touchscreen/atmel_mxt_ts.c | 4 +- drivers/input/touchscreen/auo-pixcir-ts.c | 8 +- drivers/input/touchscreen/bu21029_ts.c | 4 +- drivers/input/touchscreen/chipone_icn8318.c | 4 +- drivers/input/touchscreen/cyttsp_core.c | 4 +- drivers/input/touchscreen/eeti_ts.c | 4 +- drivers/input/touchscreen/ektf2127.c | 4 +- drivers/input/touchscreen/imx6ul_tsc.c | 4 +- drivers/input/touchscreen/ipaq-micro-ts.c | 2 +- drivers/input/touchscreen/iqs5xx.c | 4 +- drivers/input/touchscreen/lpc32xx_ts.c | 4 +- drivers/input/touchscreen/melfas_mip4.c | 4 +- drivers/input/touchscreen/mms114.c | 6 +- drivers/input/touchscreen/pixcir_i2c_ts.c | 8 +- drivers/input/touchscreen/ucb1400_ts.c | 4 +- drivers/input/touchscreen/wm97xx-core.c | 14 ++- drivers/input/touchscreen/zforce_ts.c | 8 +- drivers/platform/x86/thinkpad_acpi.c | 4 +- include/linux/input.h | 14 ++- 42 files changed, 228 insertions(+), 96 deletions(-) base-commit: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
Hi All, On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: > This is a quick respin of v3, with just two small changes, please see > the changelog below. > > Userspace might want to implement a policy to temporarily disregard input > from certain devices. > > An example use case is a convertible laptop, whose keyboard can be folded > under the screen to create tablet-like experience. The user then must hold > the laptop in such a way that it is difficult to avoid pressing the keyboard > keys. It is therefore desirable to temporarily disregard input from the > keyboard, until it is folded back. This obviously is a policy which should > be kept out of the kernel, but the kernel must provide suitable means to > implement such a policy. First of all sorry to start a somewhat new discussion about this while this patch set is also somewhat far along in the review process, but I believe what I discuss below needs to be taken into account. Yesterday I have been looking into why an Asus T101HA would not stay suspended when the LID is closed. The cause is that the USB HID multi-touch touchpad in the base of the device starts sending events when the screen gets close to the touchpad (so when the LID is fully closed) and these events are causing a wakeup from suspend. HID multi-touch devices do have a way to tell them to fully stop sending events, also disabling the USB remote wakeup the device is doing. The question is when to tell it to not send events though ... So now I've been thinking about how to fix this and I believe that there is some interaction between this problem and this patch-set. The problem I'm seeing on the T101HA is about wakeups, so the question which I want to discuss is: 1. How does inhibiting interact with enabling / disabling the device as a wakeup source ? 2. Since we have now made inhibiting equal open/close how does open/close interact with a device being a wakeup source ? And my own initial (to be discussed) answers to these questions: 1. It seems to me that when a device is inhibited it should not be a wakeup source, so where possible a input-device-driver should disable a device's wakeup capabilities on suspend if inhibited 2. This one is trickier I don't think we have really clearly specified any behavior here. The default behavior of most drivers seems to be using something like this in their suspend callback: if (device_may_wakeup(dev)) enable_irq_wake(data->irq); else if (input->users) foo_stop_receiving_events(data); Since this is what most drivers seem to do I believe we should keep this as is and that we should just clearly document that if the input_device has users (has been opened) or not does not matter for its wakeup behavior. Combining these 2 answers leads to this new pseudo code template for an input-device's suspend method: /* * If inhibited we have already disabled events and * we do NOT want to setup the device as wake source. */ if (input->inhibited) return 0; if (device_may_wakeup(dev)) enable_irq_wake(data->irq); else if (input->users) foo_stop_receiving_events(data); ### A different, but related issue is how to make devices actually use the new inhibit support on the builtin keyboard + touchpad when say the lid is closed. Arguably this is an userspace problem, but it is a tricky one. Currently on most modern Linux distributions suspend-on-lid-close is handled by systemd-logind and most modern desktop-environments are happy to have logind handle this for them. But most knowledge about input devices and e.g. heurisitics to decide if a touchpad is internal or external are part of libinput. Now we could have libinput use the new inhibit support (1), but then when the lid closes we get race between whatever process is using libinput trying to inhibit the touchpad (which must be done before to suspend to disable it as wakeup source) and logind trying to suspend the system. One solution here would be to move the setting of the inhibit sysfs attr into logind, but that requires adding a whole bunch of extra knowledge to logind which does not really belong there IMHO. I've been thinking a bit about this and to me it seems that the kernel is in the ideal position to automatically inhibit some devices when some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The issue here is to chose on which devices to enable this. I believe that the auto inhibit on some switches mechanism is best done inside the kernel (disabled by default) and then we can have a sysfs attr called auto_inhibit_ev_sw_mask which can be set to e.g. (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to inhibit both when the lid is closed or when switched to tablet mode. This could then be combined with a userspace utility run from an udev rule which makes the actual decision what auto_inhibit_ev_sw_mask should be set for a given input device. This will put the mechanism for what we want inside the kernel and leaves the policy on which switches we want this for out of the kernel. Note adding this new auto_inhibit_ev_sw_mask sysfs attr falls somewhat outside the context of this patchset and could be done as a follow up to this patch-set. But I do believe that we need to figure out how (non ChromeOS) userspace can / will use the new inhibit interface before merging it. Regards, Hans 1) There are issues here with libinput not running as root and this being a root only sysfs interface but lets ignore those for now, note that the auto_inhibit_ev_sw_mask also neatly solves this problem
On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: > > This is a quick respin of v3, with just two small changes, please see > > the changelog below. > > > > Userspace might want to implement a policy to temporarily disregard input > > from certain devices. > > > > An example use case is a convertible laptop, whose keyboard can be folded > > under the screen to create tablet-like experience. The user then must hold > > the laptop in such a way that it is difficult to avoid pressing the keyboard > > keys. It is therefore desirable to temporarily disregard input from the > > keyboard, until it is folded back. This obviously is a policy which should > > be kept out of the kernel, but the kernel must provide suitable means to > > implement such a policy. > > First of all sorry to start a somewhat new discussion about this > while this patch set is also somewhat far along in the review process, > but I believe what I discuss below needs to be taken into account. > > Yesterday I have been looking into why an Asus T101HA would not stay > suspended when the LID is closed. The cause is that the USB HID multi-touch > touchpad in the base of the device starts sending events when the screen > gets close to the touchpad (so when the LID is fully closed) and these > events are causing a wakeup from suspend. HID multi-touch devices > do have a way to tell them to fully stop sending events, also disabling > the USB remote wakeup the device is doing. The question is when to tell > it to not send events though ... > > So now I've been thinking about how to fix this and I believe that there > is some interaction between this problem and this patch-set. > > The problem I'm seeing on the T101HA is about wakeups, so the question > which I want to discuss is: > > 1. How does inhibiting interact with enabling / > disabling the device as a wakeup source ? > > 2. Since we have now made inhibiting equal open/close how does open/close > interact with a device being a wakeup source ? > > And my own initial (to be discussed) answers to these questions: > > 1. It seems to me that when a device is inhibited it should not be a > wakeup source, so where possible a input-device-driver should disable > a device's wakeup capabilities on suspend if inhibited If "inhibit" means "do not generate any events going forward", then this must also cover wakeup events, so I agree. > 2. This one is trickier I don't think we have really clearly specified > any behavior here. The default behavior of most drivers seems to be > using something like this in their suspend callback: > > if (device_may_wakeup(dev)) > enable_irq_wake(data->irq); > else if (input->users) > foo_stop_receiving_events(data); > > Since this is what most drivers seem to do I believe we should keep > this as is and that we should just clearly document that if the > input_device has users (has been opened) or not does not matter > for its wakeup behavior. > > Combining these 2 answers leads to this new pseudo code template > for an input-device's suspend method: > > /* > * If inhibited we have already disabled events and > * we do NOT want to setup the device as wake source. > */ > if (input->inhibited) > return 0; > > if (device_may_wakeup(dev)) > enable_irq_wake(data->irq); > else if (input->users) > foo_stop_receiving_events(data); > > ### Sounds reasonable to me. > A different, but related issue is how to make devices actually use the > new inhibit support on the builtin keyboard + touchpad when say the lid > is closed. Arguably this is an userspace problem, but it is a tricky > one. Currently on most modern Linux distributions suspend-on-lid-close > is handled by systemd-logind and most modern desktop-environments are > happy to have logind handle this for them. > > But most knowledge about input devices and e.g. heurisitics to decide > if a touchpad is internal or external are part of libinput. Now we could > have libinput use the new inhibit support (1), but then when the lid > closes we get race between whatever process is using libinput trying > to inhibit the touchpad (which must be done before to suspend to disable > it as wakeup source) and logind trying to suspend the system. > > One solution here would be to move the setting of the inhibit sysfs > attr into logind, but that requires adding a whole bunch of extra > knowledge to logind which does not really belong there IMHO. > > I've been thinking a bit about this and to me it seems that the kernel > is in the ideal position to automatically inhibit some devices when > some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The > issue here is to chose on which devices to enable this. I believe > that the auto inhibit on some switches mechanism is best done inside > the kernel (disabled by default) and then we can have a sysfs > attr called auto_inhibit_ev_sw_mask which can be set to e.g. > (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever > the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to > inhibit both when the lid is closed or when switched to tablet mode. I agree that the kernel is the right place to handle this, but it requires some extra knowledge about dependencies between devices. It'd be kind of like power resources in ACPI, so for each state of a "master" device (in principle, there may be more states of it than just two) there would be a list of "dependent" intput devices that need to be inhibited when the "master" device goes into that state. > This could then be combined with a userspace utility run from an > udev rule which makes the actual decision what auto_inhibit_ev_sw_mask > should be set for a given input device. > > This will put the mechanism for what we want inside the kernel and > leaves the policy on which switches we want this for out of the > kernel. > > Note adding this new auto_inhibit_ev_sw_mask sysfs attr falls > somewhat outside the context of this patchset and could be done > as a follow up to this patch-set. But I do believe that we need to > figure out how (non ChromeOS) userspace can / will use the new inhibit > interface before merging it.
Hi All, W dniu 10.06.2020 o 12:38, Rafael J. Wysocki pisze: > On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >>> This is a quick respin of v3, with just two small changes, please see >>> the changelog below. >>> >>> Userspace might want to implement a policy to temporarily disregard input >>> from certain devices. >>> >>> An example use case is a convertible laptop, whose keyboard can be folded >>> under the screen to create tablet-like experience. The user then must hold >>> the laptop in such a way that it is difficult to avoid pressing the keyboard >>> keys. It is therefore desirable to temporarily disregard input from the >>> keyboard, until it is folded back. This obviously is a policy which should >>> be kept out of the kernel, but the kernel must provide suitable means to >>> implement such a policy. >> >> First of all sorry to start a somewhat new discussion about this >> while this patch set is also somewhat far along in the review process, >> but I believe what I discuss below needs to be taken into account. >> >> Yesterday I have been looking into why an Asus T101HA would not stay >> suspended when the LID is closed. The cause is that the USB HID multi-touch >> touchpad in the base of the device starts sending events when the screen >> gets close to the touchpad (so when the LID is fully closed) and these >> events are causing a wakeup from suspend. HID multi-touch devices >> do have a way to tell them to fully stop sending events, also disabling >> the USB remote wakeup the device is doing. The question is when to tell >> it to not send events though ... >> >> So now I've been thinking about how to fix this and I believe that there >> is some interaction between this problem and this patch-set. >> >> The problem I'm seeing on the T101HA is about wakeups, so the question >> which I want to discuss is: >> >> 1. How does inhibiting interact with enabling / >> disabling the device as a wakeup source ? >> >> 2. Since we have now made inhibiting equal open/close how does open/close >> interact with a device being a wakeup source ? >> >> And my own initial (to be discussed) answers to these questions: >> >> 1. It seems to me that when a device is inhibited it should not be a >> wakeup source, so where possible a input-device-driver should disable >> a device's wakeup capabilities on suspend if inhibited > > If "inhibit" means "do not generate any events going forward", then > this must also cover wakeup events, so I agree. I agree, too. > >> 2. This one is trickier I don't think we have really clearly specified >> any behavior here. The default behavior of most drivers seems to be >> using something like this in their suspend callback: >> >> if (device_may_wakeup(dev)) >> enable_irq_wake(data->irq); >> else if (input->users) >> foo_stop_receiving_events(data); >> >> Since this is what most drivers seem to do I believe we should keep >> this as is and that we should just clearly document that if the >> input_device has users (has been opened) or not does not matter >> for its wakeup behavior. >> >> Combining these 2 answers leads to this new pseudo code template >> for an input-device's suspend method: >> >> /* >> * If inhibited we have already disabled events and >> * we do NOT want to setup the device as wake source. >> */ >> if (input->inhibited) >> return 0; Right, if a device is inhibited it shouldn't become a wakeup source, because that would contradict the purpose of being inhibited. >> >> if (device_may_wakeup(dev)) >> enable_irq_wake(data->irq); What would it mean to become a wakeup source if there are no users, or nobody has ever opened the device? There are no interested input handlers (users) so what's the point of becoming a wakeup source? Why would the system need to wake up? >> else if (input->users) >> foo_stop_receiving_events(data); >> >> ### > > Sounds reasonable to me. > >> A different, but related issue is how to make devices actually use the >> new inhibit support on the builtin keyboard + touchpad when say the lid >> is closed. Arguably this is an userspace problem, but it is a tricky >> one. Currently on most modern Linux distributions suspend-on-lid-close >> is handled by systemd-logind and most modern desktop-environments are >> happy to have logind handle this for them. >> >> But most knowledge about input devices and e.g. heurisitics to decide >> if a touchpad is internal or external are part of libinput. Now we could >> have libinput use the new inhibit support (1), but then when the lid >> closes we get race between whatever process is using libinput trying >> to inhibit the touchpad (which must be done before to suspend to disable >> it as wakeup source) and logind trying to suspend the system. >> >> One solution here would be to move the setting of the inhibit sysfs >> attr into logind, but that requires adding a whole bunch of extra >> knowledge to logind which does not really belong there IMHO. >> >> I've been thinking a bit about this and to me it seems that the kernel >> is in the ideal position to automatically inhibit some devices when >> some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The >> issue here is to chose on which devices to enable this. I believe >> that the auto inhibit on some switches mechanism is best done inside >> the kernel (disabled by default) and then we can have a sysfs >> attr called auto_inhibit_ev_sw_mask which can be set to e.g. >> (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever >> the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to >> inhibit both when the lid is closed or when switched to tablet mode. > > I agree that the kernel is the right place to handle this, but it > requires some extra knowledge about dependencies between devices. > > It'd be kind of like power resources in ACPI, so for each state of a > "master" device (in principle, there may be more states of it than > just two) there would be a list of "dependent" intput devices that > need to be inhibited when the "master" device goes into that state. > >> This could then be combined with a userspace utility run from an >> udev rule which makes the actual decision what auto_inhibit_ev_sw_mask >> should be set for a given input device. >> >> This will put the mechanism for what we want inside the kernel and >> leaves the policy on which switches we want this for out of the >> kernel. >> >> Note adding this new auto_inhibit_ev_sw_mask sysfs attr falls >> somewhat outside the context of this patchset and could be done >> as a follow up to this patch-set. Yes, please ;) But I do believe that we need to >> figure out how (non ChromeOS) userspace can / will use the new inhibit >> interface before merging it. Of course. Regards, Andrzej
Hi, On 6/10/20 3:12 PM, Andrzej Pietrasiewicz wrote: > Hi All, > > W dniu 10.06.2020 o 12:38, Rafael J. Wysocki pisze: >> On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi All, >>> >>> On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >>>> This is a quick respin of v3, with just two small changes, please see >>>> the changelog below. >>>> >>>> Userspace might want to implement a policy to temporarily disregard input >>>> from certain devices. >>>> >>>> An example use case is a convertible laptop, whose keyboard can be folded >>>> under the screen to create tablet-like experience. The user then must hold >>>> the laptop in such a way that it is difficult to avoid pressing the keyboard >>>> keys. It is therefore desirable to temporarily disregard input from the >>>> keyboard, until it is folded back. This obviously is a policy which should >>>> be kept out of the kernel, but the kernel must provide suitable means to >>>> implement such a policy. >>> >>> First of all sorry to start a somewhat new discussion about this >>> while this patch set is also somewhat far along in the review process, >>> but I believe what I discuss below needs to be taken into account. >>> >>> Yesterday I have been looking into why an Asus T101HA would not stay >>> suspended when the LID is closed. The cause is that the USB HID multi-touch >>> touchpad in the base of the device starts sending events when the screen >>> gets close to the touchpad (so when the LID is fully closed) and these >>> events are causing a wakeup from suspend. HID multi-touch devices >>> do have a way to tell them to fully stop sending events, also disabling >>> the USB remote wakeup the device is doing. The question is when to tell >>> it to not send events though ... >>> >>> So now I've been thinking about how to fix this and I believe that there >>> is some interaction between this problem and this patch-set. >>> >>> The problem I'm seeing on the T101HA is about wakeups, so the question >>> which I want to discuss is: >>> >>> 1. How does inhibiting interact with enabling / >>> disabling the device as a wakeup source ? >>> >>> 2. Since we have now made inhibiting equal open/close how does open/close >>> interact with a device being a wakeup source ? >>> >>> And my own initial (to be discussed) answers to these questions: >>> >>> 1. It seems to me that when a device is inhibited it should not be a >>> wakeup source, so where possible a input-device-driver should disable >>> a device's wakeup capabilities on suspend if inhibited >> >> If "inhibit" means "do not generate any events going forward", then >> this must also cover wakeup events, so I agree. > > I agree, too. > >> >>> 2. This one is trickier I don't think we have really clearly specified >>> any behavior here. The default behavior of most drivers seems to be >>> using something like this in their suspend callback: >>> >>> if (device_may_wakeup(dev)) >>> enable_irq_wake(data->irq); >>> else if (input->users) >>> foo_stop_receiving_events(data); >>> >>> Since this is what most drivers seem to do I believe we should keep >>> this as is and that we should just clearly document that if the >>> input_device has users (has been opened) or not does not matter >>> for its wakeup behavior. >>> >>> Combining these 2 answers leads to this new pseudo code template >>> for an input-device's suspend method: >>> >>> /* >>> * If inhibited we have already disabled events and >>> * we do NOT want to setup the device as wake source. >>> */ >>> if (input->inhibited) >>> return 0; > > Right, if a device is inhibited it shouldn't become a wakeup source, > because that would contradict the purpose of being inhibited. Ack. Note I do think that we need to document this (and more in general the answer to both questions from above) clearly so that going forward if there are any questions about how this is supposed to work we can just point to the docs. Can you do a follow-up patch, or include a patch in your next version which documents this (once we agree on what "this" exactly is) ? >>> >>> if (device_may_wakeup(dev)) >>> enable_irq_wake(data->irq); > > What would it mean to become a wakeup source if there are no users, > or nobody has ever opened the device? There are no interested > input handlers (users) so what's the point of becoming a wakeup > source? Why would the system need to wake up? Well this is what we have been doing so far, so arguably we need to keep doing it to avoid regressions / breaking our ABI. Lets for example take a laptop, where when suspended the power-button is the only valid wakeup-source and this is running good old slackware with fvwm2 or windowmaker as "desktop environment", then likely no process will have the power-button input evdev node open. Still we should wakeup the laptop on the power-button press, otherwise it will never wakeup. Note I agree with you that the way this works is not ideal, I just do not think that we can change it. >>> else if (input->users) >>> foo_stop_receiving_events(data); >>> >>> ### <snip> Regards, Hans
Hi Hans, W dniu 10.06.2020 o 15:21, Hans de Goede pisze: > Hi, > > On 6/10/20 3:12 PM, Andrzej Pietrasiewicz wrote: >> Hi All, >> >> W dniu 10.06.2020 o 12:38, Rafael J. Wysocki pisze: >>> On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi All, >>>> >>>> On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >>>>> This is a quick respin of v3, with just two small changes, please see >>>>> the changelog below. >>>>> >>>>> Userspace might want to implement a policy to temporarily disregard input >>>>> from certain devices. >>>>> >>>>> An example use case is a convertible laptop, whose keyboard can be folded >>>>> under the screen to create tablet-like experience. The user then must hold >>>>> the laptop in such a way that it is difficult to avoid pressing the keyboard >>>>> keys. It is therefore desirable to temporarily disregard input from the >>>>> keyboard, until it is folded back. This obviously is a policy which should >>>>> be kept out of the kernel, but the kernel must provide suitable means to >>>>> implement such a policy. >>>> >>>> First of all sorry to start a somewhat new discussion about this >>>> while this patch set is also somewhat far along in the review process, >>>> but I believe what I discuss below needs to be taken into account. >>>> >>>> Yesterday I have been looking into why an Asus T101HA would not stay >>>> suspended when the LID is closed. The cause is that the USB HID multi-touch >>>> touchpad in the base of the device starts sending events when the screen >>>> gets close to the touchpad (so when the LID is fully closed) and these >>>> events are causing a wakeup from suspend. HID multi-touch devices >>>> do have a way to tell them to fully stop sending events, also disabling >>>> the USB remote wakeup the device is doing. The question is when to tell >>>> it to not send events though ... >>>> >>>> So now I've been thinking about how to fix this and I believe that there >>>> is some interaction between this problem and this patch-set. >>>> >>>> The problem I'm seeing on the T101HA is about wakeups, so the question >>>> which I want to discuss is: >>>> >>>> 1. How does inhibiting interact with enabling / >>>> disabling the device as a wakeup source ? >>>> >>>> 2. Since we have now made inhibiting equal open/close how does open/close >>>> interact with a device being a wakeup source ? >>>> >>>> And my own initial (to be discussed) answers to these questions: >>>> >>>> 1. It seems to me that when a device is inhibited it should not be a >>>> wakeup source, so where possible a input-device-driver should disable >>>> a device's wakeup capabilities on suspend if inhibited >>> >>> If "inhibit" means "do not generate any events going forward", then >>> this must also cover wakeup events, so I agree. >> >> I agree, too. >> >>> >>>> 2. This one is trickier I don't think we have really clearly specified >>>> any behavior here. The default behavior of most drivers seems to be >>>> using something like this in their suspend callback: >>>> >>>> if (device_may_wakeup(dev)) >>>> enable_irq_wake(data->irq); >>>> else if (input->users) >>>> foo_stop_receiving_events(data); >>>> >>>> Since this is what most drivers seem to do I believe we should keep >>>> this as is and that we should just clearly document that if the >>>> input_device has users (has been opened) or not does not matter >>>> for its wakeup behavior. >>>> >>>> Combining these 2 answers leads to this new pseudo code template >>>> for an input-device's suspend method: >>>> >>>> /* >>>> * If inhibited we have already disabled events and >>>> * we do NOT want to setup the device as wake source. >>>> */ >>>> if (input->inhibited) >>>> return 0; >> >> Right, if a device is inhibited it shouldn't become a wakeup source, >> because that would contradict the purpose of being inhibited. > > Ack. Note I do think that we need to document this (and more > in general the answer to both questions from above) clearly so > that going forward if there are any questions about how this is > supposed to work we can just point to the docs. > > Can you do a follow-up patch, or include a patch in your next > version which documents this (once we agree on what "this" > exactly is) ? Sure I can. Just need to know when "this" becomes stable enough ;) If this series otherwise looks mature enough I would opt for a follow-up patch. > >>>> >>>> if (device_may_wakeup(dev)) >>>> enable_irq_wake(data->irq); >> >> What would it mean to become a wakeup source if there are no users, >> or nobody has ever opened the device? There are no interested >> input handlers (users) so what's the point of becoming a wakeup >> source? Why would the system need to wake up? > > Well this is what we have been doing so far, so arguably we > need to keep doing it to avoid regressions / breaking our ABI. > > Lets for example take a laptop, where when suspended the > power-button is the only valid wakeup-source and this is > running good old slackware with fvwm2 or windowmaker as > "desktop environment", then likely no process will have > the power-button input evdev node open. Still we should > wakeup the laptop on the power-button press, otherwise > it will never wakeup. > True, thanks for explaining. > Note I agree with you that the way this works is not > ideal, I just do not think that we can change it. > Regards, Andrzej
Hi, On 6/10/20 12:38 PM, Rafael J. Wysocki wrote: > On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: <snip> >> A different, but related issue is how to make devices actually use the >> new inhibit support on the builtin keyboard + touchpad when say the lid >> is closed. Arguably this is an userspace problem, but it is a tricky >> one. Currently on most modern Linux distributions suspend-on-lid-close >> is handled by systemd-logind and most modern desktop-environments are >> happy to have logind handle this for them. >> >> But most knowledge about input devices and e.g. heurisitics to decide >> if a touchpad is internal or external are part of libinput. Now we could >> have libinput use the new inhibit support (1), but then when the lid >> closes we get race between whatever process is using libinput trying >> to inhibit the touchpad (which must be done before to suspend to disable >> it as wakeup source) and logind trying to suspend the system. >> >> One solution here would be to move the setting of the inhibit sysfs >> attr into logind, but that requires adding a whole bunch of extra >> knowledge to logind which does not really belong there IMHO. >> >> I've been thinking a bit about this and to me it seems that the kernel >> is in the ideal position to automatically inhibit some devices when >> some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The >> issue here is to chose on which devices to enable this. I believe >> that the auto inhibit on some switches mechanism is best done inside >> the kernel (disabled by default) and then we can have a sysfs >> attr called auto_inhibit_ev_sw_mask which can be set to e.g. >> (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever >> the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to >> inhibit both when the lid is closed or when switched to tablet mode. > > I agree that the kernel is the right place to handle this, but it > requires some extra knowledge about dependencies between devices. > > It'd be kind of like power resources in ACPI, so for each state of a > "master" device (in principle, there may be more states of it than > just two) there would be a list of "dependent" intput devices that > need to be inhibited when the "master" device goes into that state. So a big part of the reason to punt the decision on which input devices to enable this auto-inhibit is that we don't really have information about those relationsships / device-links you are suggesting here. libinput is already doing inhibiting inside userspace for e.g. the tablet-mode switch but it relies on heuristics + quirk tables to decide which keyboards should be inhibited and which not. E.g. for a 360 degree hinges 2-in-1 we want to disable the builtin keyboard, when folded into in tablet mode, but not any external ones. Mostly the builtin kbd will be PS2 but I have one such 2-in-1 here in my home office with a USB kbd ... In general of the master devices there will be only 1, there will be only 1 lid switch and only 1 tablet-mode switch. So my idea with the auto_inhibit_ev_sw_mask, is for it to be a per input-device setting. So using your terms, all input devices with the (1 << SW_LID) bit set in their auto_inhibit_ev_sw_mask will be dependents of the (master) device which actually is reporting the SW_LID bit. The idea here is for this to work the same as how the rfkill code from net/rfkill/input.c works, except instead of binding e.g. KEY_WLAN to toggling the sw-state of rfkill devices with a type of RFKILL_TYPE_WLAN. This will bind SW_LID to inhibiting input devices with the SW_LID bit set in their auto_inhibit_ev_sw_mask. Regards, Hans
On Wed, Jun 10, 2020 at 3:21 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 6/10/20 3:12 PM, Andrzej Pietrasiewicz wrote: > > Hi All, > > [cut] > > What would it mean to become a wakeup source if there are no users, > > or nobody has ever opened the device? There are no interested > > input handlers (users) so what's the point of becoming a wakeup > > source? Why would the system need to wake up? > > Well this is what we have been doing so far, so arguably we > need to keep doing it to avoid regressions / breaking our ABI. > > Lets for example take a laptop, where when suspended the > power-button is the only valid wakeup-source and this is > running good old slackware with fvwm2 or windowmaker as > "desktop environment", then likely no process will have > the power-button input evdev node open. Still we should > wakeup the laptop on the power-button press, otherwise > it will never wakeup. > > Note I agree with you that the way this works is not > ideal, I just do not think that we can change it. Please note that "no users" merely means that user space is not interested in receiving and processing the events from that device. If it is configured for system wakeup, it doesn't matter whether or not user space will consume the related events. Thanks!
On Wed, Jun 10, 2020 at 12:38:30PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi All, > > > > On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: > > > This is a quick respin of v3, with just two small changes, please see > > > the changelog below. > > > > > > Userspace might want to implement a policy to temporarily disregard input > > > from certain devices. > > > > > > An example use case is a convertible laptop, whose keyboard can be folded > > > under the screen to create tablet-like experience. The user then must hold > > > the laptop in such a way that it is difficult to avoid pressing the keyboard > > > keys. It is therefore desirable to temporarily disregard input from the > > > keyboard, until it is folded back. This obviously is a policy which should > > > be kept out of the kernel, but the kernel must provide suitable means to > > > implement such a policy. > > > > First of all sorry to start a somewhat new discussion about this > > while this patch set is also somewhat far along in the review process, > > but I believe what I discuss below needs to be taken into account. > > > > Yesterday I have been looking into why an Asus T101HA would not stay > > suspended when the LID is closed. The cause is that the USB HID multi-touch > > touchpad in the base of the device starts sending events when the screen > > gets close to the touchpad (so when the LID is fully closed) and these > > events are causing a wakeup from suspend. HID multi-touch devices > > do have a way to tell them to fully stop sending events, also disabling > > the USB remote wakeup the device is doing. The question is when to tell > > it to not send events though ... > > > > So now I've been thinking about how to fix this and I believe that there > > is some interaction between this problem and this patch-set. > > > > The problem I'm seeing on the T101HA is about wakeups, so the question > > which I want to discuss is: > > > > 1. How does inhibiting interact with enabling / > > disabling the device as a wakeup source ? One should not affect the other. > > > > 2. Since we have now made inhibiting equal open/close how does open/close > > interact with a device being a wakeup source ? One did not affect another, and it should not. > > > > And my own initial (to be discussed) answers to these questions: > > > > 1. It seems to me that when a device is inhibited it should not be a > > wakeup source, so where possible a input-device-driver should disable > > a device's wakeup capabilities on suspend if inhibited > > If "inhibit" means "do not generate any events going forward", then > this must also cover wakeup events, so I agree. Why? These are separate concepts. Do we disable wake on lan when bringing network interface down? Do we update power/wakeup when device is inhibited? Do we restore it afterwards? Do we un-inhibit if we reenable wakeup after device is inhibited? Do we return error? How? Inhibit works on logical level, i.e. if I have several input interfaces on the same hardware device, I cam inhibit one leaving others intact. This does not mean that the device should stop generating wakeup events. We can't even guarantee this for composite devices. > > > 2. This one is trickier I don't think we have really clearly specified > > any behavior here. The default behavior of most drivers seems to be > > using something like this in their suspend callback: > > > > if (device_may_wakeup(dev)) > > enable_irq_wake(data->irq); > > else if (input->users) > > foo_stop_receiving_events(data); > > > > Since this is what most drivers seem to do I believe we should keep > > this as is and that we should just clearly document that if the > > input_device has users (has been opened) or not does not matter > > for its wakeup behavior. > > > > Combining these 2 answers leads to this new pseudo code template > > for an input-device's suspend method: > > > > /* > > * If inhibited we have already disabled events and > > * we do NOT want to setup the device as wake source. > > */ > > if (input->inhibited) > > return 0; > > > > if (device_may_wakeup(dev)) > > enable_irq_wake(data->irq); > > else if (input->users) > > foo_stop_receiving_events(data); > > > > ### > > Sounds reasonable to me. However it will not work. For many input devices connected to i2c we declare interrupt as wakeup interrupt, and the driver does not need to issue enable_irq_wake() and disable_irq_wake(). The wakeup handling is happening in driver core, which is not aware of input-specific inhibit (nor should it be). I need to ping Mark about the patch adding the similar handling to SPI. > > > A different, but related issue is how to make devices actually use the > > new inhibit support on the builtin keyboard + touchpad when say the lid > > is closed. Arguably this is an userspace problem, but it is a tricky > > one. Currently on most modern Linux distributions suspend-on-lid-close > > is handled by systemd-logind and most modern desktop-environments are > > happy to have logind handle this for them. > > > > But most knowledge about input devices and e.g. heurisitics to decide > > if a touchpad is internal or external are part of libinput. Now we could > > have libinput use the new inhibit support (1), but then when the lid > > closes we get race between whatever process is using libinput trying > > to inhibit the touchpad (which must be done before to suspend to disable > > it as wakeup source) and logind trying to suspend the system. > > > > One solution here would be to move the setting of the inhibit sysfs > > attr into logind, but that requires adding a whole bunch of extra > > knowledge to logind which does not really belong there IMHO. You do not need to push the knowledge into logind, you just need to communicate to logind what devices can be wakeup sources and which ones should not. Chrome OS uses udev tags/properties for that. > > > > I've been thinking a bit about this and to me it seems that the kernel > > is in the ideal position to automatically inhibit some devices when > > some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The > > issue here is to chose on which devices to enable this. I believe > > that the auto inhibit on some switches mechanism is best done inside > > the kernel (disabled by default) and then we can have a sysfs > > attr called auto_inhibit_ev_sw_mask which can be set to e.g. > > (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever > > the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to > > inhibit both when the lid is closed or when switched to tablet mode. This is a policy and should be kept out of the kernel. Yes, we had it implemented with rfkill input handler, but it caused quite a few issues. As far as I know it is not being used anymore and we should not try with SW_LID->inhibit either. I know it is faster to patch the kernel than to roll out proper userspace because everyone updates kernel regularly, but it does not mean it is the right solution. Thanks.
Hi, On 6/10/20 8:28 PM, Dmitry Torokhov wrote: > On Wed, Jun 10, 2020 at 12:38:30PM +0200, Rafael J. Wysocki wrote: >> On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi All, >>> >>> On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >>>> This is a quick respin of v3, with just two small changes, please see >>>> the changelog below. >>>> >>>> Userspace might want to implement a policy to temporarily disregard input >>>> from certain devices. >>>> >>>> An example use case is a convertible laptop, whose keyboard can be folded >>>> under the screen to create tablet-like experience. The user then must hold >>>> the laptop in such a way that it is difficult to avoid pressing the keyboard >>>> keys. It is therefore desirable to temporarily disregard input from the >>>> keyboard, until it is folded back. This obviously is a policy which should >>>> be kept out of the kernel, but the kernel must provide suitable means to >>>> implement such a policy. >>> >>> First of all sorry to start a somewhat new discussion about this >>> while this patch set is also somewhat far along in the review process, >>> but I believe what I discuss below needs to be taken into account. >>> >>> Yesterday I have been looking into why an Asus T101HA would not stay >>> suspended when the LID is closed. The cause is that the USB HID multi-touch >>> touchpad in the base of the device starts sending events when the screen >>> gets close to the touchpad (so when the LID is fully closed) and these >>> events are causing a wakeup from suspend. HID multi-touch devices >>> do have a way to tell them to fully stop sending events, also disabling >>> the USB remote wakeup the device is doing. The question is when to tell >>> it to not send events though ... >>> >>> So now I've been thinking about how to fix this and I believe that there >>> is some interaction between this problem and this patch-set. >>> >>> The problem I'm seeing on the T101HA is about wakeups, so the question >>> which I want to discuss is: >>> >>> 1. How does inhibiting interact with enabling / >>> disabling the device as a wakeup source ? > > One should not affect the other. > >>> >>> 2. Since we have now made inhibiting equal open/close how does open/close >>> interact with a device being a wakeup source ? > > One did not affect another, and it should not. > >>> >>> And my own initial (to be discussed) answers to these questions: >>> >>> 1. It seems to me that when a device is inhibited it should not be a >>> wakeup source, so where possible a input-device-driver should disable >>> a device's wakeup capabilities on suspend if inhibited >> >> If "inhibit" means "do not generate any events going forward", then >> this must also cover wakeup events, so I agree. > > Why? These are separate concepts. Do we disable wake on lan when > bringing network interface down? Do we update power/wakeup when device > is inhibited? Do we restore it afterwards? Do we un-inhibit if we > reenable wakeup after device is inhibited? Do we return error? How? > > Inhibit works on logical level, i.e. if I have several input interfaces > on the same hardware device, I cam inhibit one leaving others intact. > This does not mean that the device should stop generating wakeup events. > We can't even guarantee this for composite devices. After thinking more about this I believe you are right and we should keep these as 2 separate, completely independent settings. Especially since the wakeup setting typically is a setting of the parent device, where as the inhibit is done on the actual input-dev. ### Some quick background info on my original thoughts here, as mentioned I started thinking about this because of spurious wakeups from suspend when the lid of an asus t101ha is "touching" its touchpad. The HID multi-touch protocol has a setting where we can ask the device to stop sending events. So even though the kbd + touchpad are a single composite USB device, we can disable wakeup (in a way) for just the touchpad at the hid-multitouch level. So I was thinking maybe adding a separate wakeup setting to the input device itself for this. But thinking more about it, when the lid is closed we can just disable wakeup on the entire USB device, since the keyboard is covered by the lid too. And then on suspend the hid-multitouch driver can detect that its parent (or parents parent in the case of USB) has wakeup disabled and also tell the device to stop scanning for fingers to save some power. We probably also need a close and open callbacks add the HID-driver level, so that if there are no touchpad users we can also use the same option to put the HID multi-touch device in a low power mode where it does not scan for fingers. <snip> >>> A different, but related issue is how to make devices actually use the >>> new inhibit support on the builtin keyboard + touchpad when say the lid >>> is closed. Arguably this is an userspace problem, but it is a tricky >>> one. Currently on most modern Linux distributions suspend-on-lid-close >>> is handled by systemd-logind and most modern desktop-environments are >>> happy to have logind handle this for them. >>> >>> But most knowledge about input devices and e.g. heurisitics to decide >>> if a touchpad is internal or external are part of libinput. Now we could >>> have libinput use the new inhibit support (1), but then when the lid >>> closes we get race between whatever process is using libinput trying >>> to inhibit the touchpad (which must be done before to suspend to disable >>> it as wakeup source) and logind trying to suspend the system. >>> >>> One solution here would be to move the setting of the inhibit sysfs >>> attr into logind, but that requires adding a whole bunch of extra >>> knowledge to logind which does not really belong there IMHO. > > You do not need to push the knowledge into logind, you just need to > communicate to logind what devices can be wakeup sources and which ones > should not. Chrome OS uses udev tags/properties for that. True, I did not think of doing the tag thingie + letting logind do the inhibit on LID close based on that. logind could also disable wakeup (to save power while suspended) on devices which are tagged for it to do that (should probably be a separate tag from the inhibit tag). >>> I've been thinking a bit about this and to me it seems that the kernel >>> is in the ideal position to automatically inhibit some devices when >>> some EV_SW transitions from 0->1 (and uninhibit again on 1->0). The >>> issue here is to chose on which devices to enable this. I believe >>> that the auto inhibit on some switches mechanism is best done inside >>> the kernel (disabled by default) and then we can have a sysfs >>> attr called auto_inhibit_ev_sw_mask which can be set to e.g. >>> (1 << SW_LID) to make the kernel auto-inhibit the input-device whenever >>> the lid is closed, or to ((1 << SW_LID) | (1 << SW_TABLET_MODE)) to >>> inhibit both when the lid is closed or when switched to tablet mode. > > This is a policy and should be kept out of the kernel. Yes, we had it > implemented with rfkill input handler, but it caused quite a few issues. > As far as I know it is not being used anymore and we should not try with > SW_LID->inhibit either. > > I know it is faster to patch the kernel than to roll out proper > userspace because everyone updates kernel regularly, but it does not > mean it is the right solution. Agreed, I just could not come up with a clean userspace solution, but using udev+hwdb to set a tag for logind instead of having the write to a new auto_inhibit_ev_sw_mask will work nicely. So I think this is all resolved now (or at least we have a plan for it). Regards, Hans
Hi, On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: > This is a quick respin of v3, with just two small changes, please see > the changelog below. > > Userspace might want to implement a policy to temporarily disregard input > from certain devices. > > An example use case is a convertible laptop, whose keyboard can be folded > under the screen to create tablet-like experience. The user then must hold > the laptop in such a way that it is difficult to avoid pressing the keyboard > keys. It is therefore desirable to temporarily disregard input from the > keyboard, until it is folded back. This obviously is a policy which should > be kept out of the kernel, but the kernel must provide suitable means to > implement such a policy. > > Due to interactions with suspend/resume, a helper has been added for drivers > to decide if the device is being used or not (PATCH 1/7) and it has been > applied to relevant drivers (PATCH 2,4,5,6/7). > > PATCH 7/7 adds support for inhibiting input devices. > > This work is inspired by: > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/45c2d7bb398f74adfae0017e20b224152fde3822 > > and > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4ce0e8a3697edb8fd071110b3af65014512061c7 > > In this respin the elan_i2c patch is dropped and converting it will be > addressed later. > > v3..v4: > - updated the comment in input_open_device() (Hans) > - used more straightforward locking pattern in adc/exynos (Michał) > > v2..v3: > - ignored autorepeat events in input_get_disposition() if a key is not > pressed (Hans) > - dropped inhibit()/uninhibit() driver callbacks (Hans) > - split ACPI button patch into taking the lock and using the helper (Rafael) > - dropped the elan_i2c conversion > - fixed typos in exynos adc > > v1..v2: > - added input_device_enabled() helper and used it in drivers (Dmitry) > - the fact of open() and close() being called in inhibit/uninhibit paths has > been emphasized in the commit message of PATCH 6/7 (Dmitry) > > Andrzej Pietrasiewicz (6): > Input: add input_device_enabled() > Input: use input_device_enabled() > ACPI: button: Access input device's users under appropriate mutex > ACPI: button: Use input_device_enabled() helper > iio: adc: exynos: Use input_device_enabled() > platform/x86: thinkpad_acpi: Use input_device_enabled() > > Patrik Fimml (1): > Input: Add "inhibited" property The entire series looks good to me: Acked-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans
Hi, On 6/10/20 3:41 PM, Andrzej Pietrasiewicz wrote: > Hi Hans, > > W dniu 10.06.2020 o 15:21, Hans de Goede pisze: >> Hi, >> >> On 6/10/20 3:12 PM, Andrzej Pietrasiewicz wrote: >>> Hi All, >>> >>> W dniu 10.06.2020 o 12:38, Rafael J. Wysocki pisze: >>>> On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >>>>> >>>>> Hi All, >>>>> >>>>> On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >>>>>> This is a quick respin of v3, with just two small changes, please see >>>>>> the changelog below. >>>>>> >>>>>> Userspace might want to implement a policy to temporarily disregard input >>>>>> from certain devices. >>>>>> >>>>>> An example use case is a convertible laptop, whose keyboard can be folded >>>>>> under the screen to create tablet-like experience. The user then must hold >>>>>> the laptop in such a way that it is difficult to avoid pressing the keyboard >>>>>> keys. It is therefore desirable to temporarily disregard input from the >>>>>> keyboard, until it is folded back. This obviously is a policy which should >>>>>> be kept out of the kernel, but the kernel must provide suitable means to >>>>>> implement such a policy. >>>>> >>>>> First of all sorry to start a somewhat new discussion about this >>>>> while this patch set is also somewhat far along in the review process, >>>>> but I believe what I discuss below needs to be taken into account. >>>>> >>>>> Yesterday I have been looking into why an Asus T101HA would not stay >>>>> suspended when the LID is closed. The cause is that the USB HID multi-touch >>>>> touchpad in the base of the device starts sending events when the screen >>>>> gets close to the touchpad (so when the LID is fully closed) and these >>>>> events are causing a wakeup from suspend. HID multi-touch devices >>>>> do have a way to tell them to fully stop sending events, also disabling >>>>> the USB remote wakeup the device is doing. The question is when to tell >>>>> it to not send events though ... >>>>> >>>>> So now I've been thinking about how to fix this and I believe that there >>>>> is some interaction between this problem and this patch-set. >>>>> >>>>> The problem I'm seeing on the T101HA is about wakeups, so the question >>>>> which I want to discuss is: >>>>> >>>>> 1. How does inhibiting interact with enabling / >>>>> disabling the device as a wakeup source ? >>>>> >>>>> 2. Since we have now made inhibiting equal open/close how does open/close >>>>> interact with a device being a wakeup source ? >>>>> >>>>> And my own initial (to be discussed) answers to these questions: >>>>> >>>>> 1. It seems to me that when a device is inhibited it should not be a >>>>> wakeup source, so where possible a input-device-driver should disable >>>>> a device's wakeup capabilities on suspend if inhibited >>>> >>>> If "inhibit" means "do not generate any events going forward", then >>>> this must also cover wakeup events, so I agree. >>> >>> I agree, too. >>> >>>> >>>>> 2. This one is trickier I don't think we have really clearly specified >>>>> any behavior here. The default behavior of most drivers seems to be >>>>> using something like this in their suspend callback: >>>>> >>>>> if (device_may_wakeup(dev)) >>>>> enable_irq_wake(data->irq); >>>>> else if (input->users) >>>>> foo_stop_receiving_events(data); >>>>> >>>>> Since this is what most drivers seem to do I believe we should keep >>>>> this as is and that we should just clearly document that if the >>>>> input_device has users (has been opened) or not does not matter >>>>> for its wakeup behavior. >>>>> >>>>> Combining these 2 answers leads to this new pseudo code template >>>>> for an input-device's suspend method: >>>>> >>>>> /* >>>>> * If inhibited we have already disabled events and >>>>> * we do NOT want to setup the device as wake source. >>>>> */ >>>>> if (input->inhibited) >>>>> return 0; >>> >>> Right, if a device is inhibited it shouldn't become a wakeup source, >>> because that would contradict the purpose of being inhibited. >> >> Ack. Note I do think that we need to document this (and more >> in general the answer to both questions from above) clearly so >> that going forward if there are any questions about how this is >> supposed to work we can just point to the docs. >> >> Can you do a follow-up patch, or include a patch in your next >> version which documents this (once we agree on what "this" >> exactly is) ? > > Sure I can. Just need to know when "this" becomes stable enough ;) > If this series otherwise looks mature enough I would opt for a > follow-up patch. FWIW after my flip-flop to agreeing with Dmitry that the 2 (inhibit vs wakeup) should be completely orthogonal this new policy is stable/mature from my pov (and consistent with how we handle wakeup vs input_dev->users). I still think it would be good to do a follow-up documentation patch documenting that these (and esp. inhibit) are orthogonal. This will mean for example that if a device is inhibit but still wakeup enabled and the device's close method silences the devices, that it needs to be unsilenced in suspend. This might be worth mentioning in the docs even though drivers which silence the device on close should already unsilence the device on suspend when it is wakeup-enabled. Note maybe we should give it a couple of days for others to give their opinion before you submit the follow-up documentation patch. Regards, Hans
Hi Hans, W dniu 12.06.2020 o 10:30, Hans de Goede pisze: > Hi, > > On 6/10/20 3:41 PM, Andrzej Pietrasiewicz wrote: >> Hi Hans, >> >> W dniu 10.06.2020 o 15:21, Hans de Goede pisze: >>> Hi, >>> >>> On 6/10/20 3:12 PM, Andrzej Pietrasiewicz wrote: >>>> Hi All, >>>> >>>> W dniu 10.06.2020 o 12:38, Rafael J. Wysocki pisze: >>>>> On Wed, Jun 10, 2020 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> >>>>>> Hi All, >>>>>> >>>>>> On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >>>>>>> This is a quick respin of v3, with just two small changes, please see >>>>>>> the changelog below. >>>>>>> >>>>>>> Userspace might want to implement a policy to temporarily disregard input >>>>>>> from certain devices. >>>>>>> >>>>>>> An example use case is a convertible laptop, whose keyboard can be folded >>>>>>> under the screen to create tablet-like experience. The user then must hold >>>>>>> the laptop in such a way that it is difficult to avoid pressing the keyboard >>>>>>> keys. It is therefore desirable to temporarily disregard input from the >>>>>>> keyboard, until it is folded back. This obviously is a policy which should >>>>>>> be kept out of the kernel, but the kernel must provide suitable means to >>>>>>> implement such a policy. >>>>>> >>>>>> First of all sorry to start a somewhat new discussion about this >>>>>> while this patch set is also somewhat far along in the review process, >>>>>> but I believe what I discuss below needs to be taken into account. >>>>>> >>>>>> Yesterday I have been looking into why an Asus T101HA would not stay >>>>>> suspended when the LID is closed. The cause is that the USB HID multi-touch >>>>>> touchpad in the base of the device starts sending events when the screen >>>>>> gets close to the touchpad (so when the LID is fully closed) and these >>>>>> events are causing a wakeup from suspend. HID multi-touch devices >>>>>> do have a way to tell them to fully stop sending events, also disabling >>>>>> the USB remote wakeup the device is doing. The question is when to tell >>>>>> it to not send events though ... >>>>>> >>>>>> So now I've been thinking about how to fix this and I believe that there >>>>>> is some interaction between this problem and this patch-set. >>>>>> >>>>>> The problem I'm seeing on the T101HA is about wakeups, so the question >>>>>> which I want to discuss is: >>>>>> >>>>>> 1. How does inhibiting interact with enabling / >>>>>> disabling the device as a wakeup source ? >>>>>> >>>>>> 2. Since we have now made inhibiting equal open/close how does open/close >>>>>> interact with a device being a wakeup source ? >>>>>> >>>>>> And my own initial (to be discussed) answers to these questions: >>>>>> >>>>>> 1. It seems to me that when a device is inhibited it should not be a >>>>>> wakeup source, so where possible a input-device-driver should disable >>>>>> a device's wakeup capabilities on suspend if inhibited >>>>> >>>>> If "inhibit" means "do not generate any events going forward", then >>>>> this must also cover wakeup events, so I agree. >>>> >>>> I agree, too. >>>> >>>>> >>>>>> 2. This one is trickier I don't think we have really clearly specified >>>>>> any behavior here. The default behavior of most drivers seems to be >>>>>> using something like this in their suspend callback: >>>>>> >>>>>> if (device_may_wakeup(dev)) >>>>>> enable_irq_wake(data->irq); >>>>>> else if (input->users) >>>>>> foo_stop_receiving_events(data); >>>>>> >>>>>> Since this is what most drivers seem to do I believe we should keep >>>>>> this as is and that we should just clearly document that if the >>>>>> input_device has users (has been opened) or not does not matter >>>>>> for its wakeup behavior. >>>>>> >>>>>> Combining these 2 answers leads to this new pseudo code template >>>>>> for an input-device's suspend method: >>>>>> >>>>>> /* >>>>>> * If inhibited we have already disabled events and >>>>>> * we do NOT want to setup the device as wake source. >>>>>> */ >>>>>> if (input->inhibited) >>>>>> return 0; >>>> >>>> Right, if a device is inhibited it shouldn't become a wakeup source, >>>> because that would contradict the purpose of being inhibited. >>> >>> Ack. Note I do think that we need to document this (and more >>> in general the answer to both questions from above) clearly so >>> that going forward if there are any questions about how this is >>> supposed to work we can just point to the docs. >>> >>> Can you do a follow-up patch, or include a patch in your next >>> version which documents this (once we agree on what "this" >>> exactly is) ? >> >> Sure I can. Just need to know when "this" becomes stable enough ;) >> If this series otherwise looks mature enough I would opt for a >> follow-up patch. > > FWIW after my flip-flop to agreeing with Dmitry that the 2 > (inhibit vs wakeup) should be completely orthogonal this new > policy is stable/mature from my pov (and consistent with how > we handle wakeup vs input_dev->users). > > I still think it would be good to do a follow-up documentation > patch documenting that these (and esp. inhibit) are orthogonal. > > This will mean for example that if a device is inhibit but > still wakeup enabled and the device's close method silences > the devices, that it needs to be unsilenced in suspend. > This might be worth mentioning in the docs even though > drivers which silence the device on close should already > unsilence the device on suspend when it is wakeup-enabled. > > Note maybe we should give it a couple of days for others to > give their opinion before you submit the follow-up documentation > patch. > True. I will send something after the weekend. Andrzej
Hi Dmitry, W dniu 12.06.2020 o 10:17, Hans de Goede pisze: > Hi, > > On 6/8/20 1:22 PM, Andrzej Pietrasiewicz wrote: >> This is a quick respin of v3, with just two small changes, please see >> the changelog below. >> >> Userspace might want to implement a policy to temporarily disregard input >> from certain devices. >> <snip> >> v3..v4: >> - updated the comment in input_open_device() (Hans) >> - used more straightforward locking pattern in adc/exynos (Michał) >> >> v2..v3: >> - ignored autorepeat events in input_get_disposition() if a key is not >> pressed (Hans) >> - dropped inhibit()/uninhibit() driver callbacks (Hans) >> - split ACPI button patch into taking the lock and using the helper (Rafael) >> - dropped the elan_i2c conversion >> - fixed typos in exynos adc >> >> v1..v2: >> - added input_device_enabled() helper and used it in drivers (Dmitry) >> - the fact of open() and close() being called in inhibit/uninhibit paths has >> been emphasized in the commit message of PATCH 6/7 (Dmitry) <snip> > > The entire series looks good to me: > > Acked-by: Hans de Goede <hdegoede@redhat.com> What are the prospects of this series being merged? Regards, Andrzej
diff --git a/drivers/input/input.c b/drivers/input/input.c index 41377bfa142d..4110b5797219 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,7 +620,7 @@ 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. @@ -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)