| Message ID | 20171030174050.30375-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 30, 2017 at 06:40:49PM +0100, Hans de Goede wrote: > In some cases it is undesirable for a wakeup button to send input events > to userspace if pressed to wakeup the system (if pressed during suspend). > > A typical example of this is the power-button on laptops / tablets, > sending a KEY_POWER event to userspace when woken up with the power-button > will cause userspace to immediately suspend the system again which is > undesirable. > > For power-buttons attached to a PMIC, or handled by e.g. ACPI, not sending > an input event in this case is take care of by the PMIC / ACPI hardware / > code. But in the case of a GPIO button we need to explicitly suppress the > sending of the input event. > > This commit supports this by adding a suppress_evdev_events_on_wakeup bool > to struct gpio_keys_button, which platform code can set to suppress the > input events for presses of wakeup keys during suspend. I think this is [your] userspace issue. What if I press the button rapidly several times? I know Android actually _wants_ to see KEY_POWER at resume, or its opportunistic suspend will kick in right away. I think ChromeOS is OK with getting KEY_POWER on resume as well. I'd say you need to have a small timeout before you start suspending again. Thanks.
Hi, On 30-10-17 19:17, Dmitry Torokhov wrote: > On Mon, Oct 30, 2017 at 06:40:49PM +0100, Hans de Goede wrote: >> In some cases it is undesirable for a wakeup button to send input events >> to userspace if pressed to wakeup the system (if pressed during suspend). >> >> A typical example of this is the power-button on laptops / tablets, >> sending a KEY_POWER event to userspace when woken up with the power-button >> will cause userspace to immediately suspend the system again which is >> undesirable. >> >> For power-buttons attached to a PMIC, or handled by e.g. ACPI, not sending >> an input event in this case is take care of by the PMIC / ACPI hardware / >> code. But in the case of a GPIO button we need to explicitly suppress the >> sending of the input event. >> >> This commit supports this by adding a suppress_evdev_events_on_wakeup bool >> to struct gpio_keys_button, which platform code can set to suppress the >> input events for presses of wakeup keys during suspend. > > I think this is [your] userspace issue. It would have been nice if you started discussing this when I posted v1 quite some time ago. > What if I press the button > rapidly several times? Why would anyone do that ? But to answer the question it depends on the timing, if you keep pressing it long enough for the resume to complete then the first press after that will cause it to suspend again. > I know Android actually _wants_ to see KEY_POWER > at resume, or its opportunistic suspend will kick in right away. Hmm, that is weird, because I believe that android x86 does work on regular PCs and they do not do that. > I think > ChromeOS is OK with getting KEY_POWER on resume as well. > > I'd say you need to have a small timeout before you start suspending > again. The problem with that is that it is going to be inherently racy. More in general Bay Trail / Cherry Trail devices using gpio_key for their power-button are the only "PC" devices sending a KEY_POWER after wake-up, Bay / Cherry Trail devices which have their power button hooked-up differently such as the Asus Transformer series do not do this. So both for consistency and because a timeout is racy I believe that having the power button not send KEY_POWER after resume is the proper solution here. Note that I've made this configurable and that soc_button_array is the only driver getting this enabled, making the power-button behavior on PC like devices consistent, without impacting any other devices. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 30, 2017 at 09:08:21PM +0100, Hans de Goede wrote: > Hi, > > On 30-10-17 19:17, Dmitry Torokhov wrote: > > On Mon, Oct 30, 2017 at 06:40:49PM +0100, Hans de Goede wrote: > > > In some cases it is undesirable for a wakeup button to send input events > > > to userspace if pressed to wakeup the system (if pressed during suspend). > > > > > > A typical example of this is the power-button on laptops / tablets, > > > sending a KEY_POWER event to userspace when woken up with the power-button > > > will cause userspace to immediately suspend the system again which is > > > undesirable. > > > > > > For power-buttons attached to a PMIC, or handled by e.g. ACPI, not sending > > > an input event in this case is take care of by the PMIC / ACPI hardware / > > > code. But in the case of a GPIO button we need to explicitly suppress the > > > sending of the input event. > > > > > > This commit supports this by adding a suppress_evdev_events_on_wakeup bool > > > to struct gpio_keys_button, which platform code can set to suppress the > > > input events for presses of wakeup keys during suspend. > > > > I think this is [your] userspace issue. > > It would have been nice if you started discussing this when I posted v1 > quite some time ago. The original commits IIRC were about poor quality of buttons on these x86 tablets and the apparent need for debounce on resume path. Or did I mix them up with something else. > > > What if I press the button > > rapidly several times? > > Why would anyone do that ? But to answer the question it depends on Because they can? > the timing, if you keep pressing it long enough for the resume to complete > then the first press after that will cause it to suspend again. > > > I know Android actually _wants_ to see KEY_POWER > > at resume, or its opportunistic suspend will kick in right away. > > Hmm, that is weird, because I believe that android x86 does work > on regular PCs and they do not do that. The commit 0f107573da417c7f5c6d3a0160ebacc3adb019c4 was done specifically so that Pixel C would resume if power button was pressed and immediately released. I.e. before there was inconsistency with driver behavior, depending on how long the button was pressed. I am not sure about android x86 frankly. > > > I think > > ChromeOS is OK with getting KEY_POWER on resume as well. > > > > I'd say you need to have a small timeout before you start suspending > > again. > > The problem with that is that it is going to be inherently racy. Racy in what way? > > More in general Bay Trail / Cherry Trail devices using gpio_key > for their power-button are the only "PC" devices sending a > KEY_POWER after wake-up, Bay / Cherry Trail devices which > have their power button hooked-up differently such as the Asus > Transformer series do not do this. > > So both for consistency and because a timeout is racy I believe > that having the power button not send KEY_POWER after resume is > the proper solution here. > > Note that I've made this configurable and that soc_button_array > is the only driver getting this enabled, making the power-button > behavior on PC like devices consistent, without impacting any > other devices. I'd rather we did not make assumption in the kernel about behavior of userspace we happen to run on. The "PC like" devices change all the time, and one could even say that failure to deliver events by ACPI drivers on resume is a bug. Please teach userspace how to handle events coming during resume phase and what to do about them, and be done with it. One option is to say (in whatever implements your power policy) "we want to ignore next KEY_POWER event for the next NNN msec". You will need it ianyway if you decide to put your userspace onto, let's say, ASUS Flip (an ARM device using gpio-keys for power button and volume up/down buttons on the side of the case). Thanks.
Hi, On 30-10-17 21:48, Dmitry Torokhov wrote: <snip> >> So both for consistency and because a timeout is racy I believe >> that having the power button not send KEY_POWER after resume is >> the proper solution here. >> >> Note that I've made this configurable and that soc_button_array >> is the only driver getting this enabled, making the power-button >> behavior on PC like devices consistent, without impacting any >> other devices. > > I'd rather we did not make assumption in the kernel about behavior of > userspace we happen to run on. The "PC like" devices change all the > time, and one could even say that failure to deliver events by ACPI > drivers on resume is a bug. > > Please teach userspace how to handle events coming during resume phase > and what to do about them, and be done with it. One option is to say (in > whatever implements your power policy) "we want to ignore next KEY_POWER > event for the next NNN msec". You will need it ianyway if you decide to > put your userspace onto, let's say, ASUS Flip (an ARM device using > gpio-keys for power button and volume up/down buttons on the side of the > case). Ok, I've written and submitted patches for this for gnome: https://bugzilla.gnome.org/show_bug.cgi?id=789771 That still leaves the same issue for KDE, XFCE, mate, etc. though, note I've no intention of fixing those. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Nov 01 2017 or thereabouts, Hans de Goede wrote: > Hi, > > On 30-10-17 21:48, Dmitry Torokhov wrote: > > <snip> > > > > So both for consistency and because a timeout is racy I believe > > > that having the power button not send KEY_POWER after resume is > > > the proper solution here. > > > > > > Note that I've made this configurable and that soc_button_array > > > is the only driver getting this enabled, making the power-button > > > behavior on PC like devices consistent, without impacting any > > > other devices. > > > > I'd rather we did not make assumption in the kernel about behavior of > > userspace we happen to run on. The "PC like" devices change all the > > time, and one could even say that failure to deliver events by ACPI > > drivers on resume is a bug. > > > > Please teach userspace how to handle events coming during resume phase > > and what to do about them, and be done with it. One option is to say (in > > whatever implements your power policy) "we want to ignore next KEY_POWER > > event for the next NNN msec". You will need it ianyway if you decide to > > put your userspace onto, let's say, ASUS Flip (an ARM device using > > gpio-keys for power button and volume up/down buttons on the side of the > > case). > > Ok, I've written and submitted patches for this for gnome: > https://bugzilla.gnome.org/show_bug.cgi?id=789771 > > That still leaves the same issue for KDE, XFCE, mate, etc. though, > note I've no intention of fixing those. > Hi Hans, I roughly had a look at the series, and I am wondering, can't we at least take the cleanup patches (1/4 2/4 as far as i can tell)? Cheers, Benjamin > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06-11-17 10:19, Benjamin Tissoires wrote: > On Nov 01 2017 or thereabouts, Hans de Goede wrote: >> Hi, >> >> On 30-10-17 21:48, Dmitry Torokhov wrote: >> >> <snip> >> >>>> So both for consistency and because a timeout is racy I believe >>>> that having the power button not send KEY_POWER after resume is >>>> the proper solution here. >>>> >>>> Note that I've made this configurable and that soc_button_array >>>> is the only driver getting this enabled, making the power-button >>>> behavior on PC like devices consistent, without impacting any >>>> other devices. >>> >>> I'd rather we did not make assumption in the kernel about behavior of >>> userspace we happen to run on. The "PC like" devices change all the >>> time, and one could even say that failure to deliver events by ACPI >>> drivers on resume is a bug. >>> >>> Please teach userspace how to handle events coming during resume phase >>> and what to do about them, and be done with it. One option is to say (in >>> whatever implements your power policy) "we want to ignore next KEY_POWER >>> event for the next NNN msec". You will need it ianyway if you decide to >>> put your userspace onto, let's say, ASUS Flip (an ARM device using >>> gpio-keys for power button and volume up/down buttons on the side of the >>> case). >> >> Ok, I've written and submitted patches for this for gnome: >> https://bugzilla.gnome.org/show_bug.cgi?id=789771 >> >> That still leaves the same issue for KDE, XFCE, mate, etc. though, >> note I've no intention of fixing those. >> > > Hi Hans, > > I roughly had a look at the series, and I am wondering, can't we at > least take the cleanup patches (1/4 2/4 as far as i can tell)? You could take those, but I'm not sure they are worth the churn, without the need for the interrupt handlers to be able to access (more) device-level (rather then button-level) data. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 9b51aa56cc83..3beff91f1a4c 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -373,6 +373,10 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata) return; } + if (bdata->button->suppress_evdev_events_on_wakeup && + bdata->ddata->suspended && state) + return; + if (type == EV_ABS) { if (state) input_event(input, type, button->code, button->value); @@ -400,6 +404,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id) BUG_ON(irq != bdata->irq); + if (bdata->button->suppress_evdev_events_on_wakeup && + bdata->ddata->suspended) + return IRQ_HANDLED; + if (bdata->button->wakeup) { const struct gpio_keys_button *button = bdata->button; @@ -445,6 +453,10 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id) BUG_ON(irq != bdata->irq); + if (bdata->button->suppress_evdev_events_on_wakeup && + bdata->ddata->suspended) + return IRQ_HANDLED; + spin_lock_irqsave(&bdata->lock, flags); if (!bdata->key_pressed) { @@ -894,6 +906,7 @@ static int __maybe_unused gpio_keys_resume(struct device *dev) if (error) return error; + /* Sync state before clearing suspended, so it knows this is a resume */ gpio_keys_report_state(ddata); ddata->suspended = false; return 0; diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h index 0b71024c082c..e05fe0902358 100644 --- a/include/linux/gpio_keys.h +++ b/include/linux/gpio_keys.h @@ -15,6 +15,9 @@ struct device; * @debounce_interval: debounce ticks interval in msecs * @can_disable: %true indicates that userspace is allowed to * disable button via sysfs + * @suppress_evdev_events_on_wakeup: For wake-up source buttons only, if %true + * then no evdev-events will be generated if pressed while + * suspended * @value: axis value for %EV_ABS * @irq: Irq number in case of interrupt keys */ @@ -27,6 +30,7 @@ struct gpio_keys_button { int wakeup; int debounce_interval; bool can_disable; + bool suppress_evdev_events_on_wakeup; int value; unsigned int irq; };
In some cases it is undesirable for a wakeup button to send input events to userspace if pressed to wakeup the system (if pressed during suspend). A typical example of this is the power-button on laptops / tablets, sending a KEY_POWER event to userspace when woken up with the power-button will cause userspace to immediately suspend the system again which is undesirable. For power-buttons attached to a PMIC, or handled by e.g. ACPI, not sending an input event in this case is take care of by the PMIC / ACPI hardware / code. But in the case of a GPIO button we need to explicitly suppress the sending of the input event. This commit supports this by adding a suppress_evdev_events_on_wakeup bool to struct gpio_keys_button, which platform code can set to suppress the input events for presses of wakeup keys during suspend. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -This is a rewrite if my "Input: gpio_keys - Do not report wake button presses as evdev events" patch. -Instead of unconditionally ignoring presses of all wake-up buttons during suspend, this rewrite makes this configurable per button -This version uses a timer to delay clearing the suspended flag for software debouncing, rather then jiffy compare magic Changes in v3: -Get rid of the need to have a timer all-together -Rename the flag from no_wakeup_events to suppress_evdev_events_on_wakeup --- drivers/input/keyboard/gpio_keys.c | 13 +++++++++++++ include/linux/gpio_keys.h | 4 ++++ 2 files changed, 17 insertions(+)