GNOME Bugzilla – Bug 789771
[PATCH] media-keys: suppress KEY_POWER events during resume
Last modified: 2017-11-30 21:25:15 UTC
Created attachment 362760 [details] [review] [PATCH 1/2] media-keys: Add DisablePowerButton and EnablePowerButton dbus methods Some devices send a KEY_POWER event when woken up from suspend with the power-button, causing g-s-d to immediately re-suspend the tablet. These 2 patches make g-s-d ignore any KEY_POWER events arriving between suspend and 1 second after resume. Note: 1) I tried using a delay of 0.5 seconds after resume, that turns out to no always be enough. 2) One could argue that the KEY_POWER event should never reach userspace if it was the cause of a wake-up from suspend. I've tried fixing this in the kernel but I got a very clear and definitive NO to that approach form the kernel's input subsys maintainer, see: https://patchwork.kernel.org/patch/10032961/
Created attachment 362761 [details] [review] [PATCH 2/2] power: Suppress power-button presses during suspend
Review of attachment 362760 [details] [review]: I'd prefer to not add more DBus ping ponging unless absolutely necessary. In this case we even already have a logind proxy instance in this plugin so we can just connect its "g-signal" signal to catch PrepareForSleep and do the inhibition flag toggling right there
Review of attachment 362761 [details] [review]: let's keep this self-contained in media-keys
Hi, (In reply to Rui Matos from comment #2) > Review of attachment 362760 [details] [review] [review]: > > I'd prefer to not add more DBus ping ponging unless absolutely necessary. > > In this case we even already have a logind proxy instance in this plugin so > we can just connect its "g-signal" signal to catch PrepareForSleep and do > the inhibition flag toggling right there I completely understand. So I was looking into making the requested changing and did a search for "PrepareForSleep" in gsd-power-manager.c, which found me this: /* We take a delay inhibitor here, which causes logind to send a * PrepareForSleep signal, which gives us a chance to lock the screen * and do some other preparations. */ ... g_dbus_proxy_call_with_unix_fd_list (manager->priv->logind_proxy, "Inhibit", g_variant_new ("(ssss)", "sleep", g_get_user_name (), "GNOME needs to lock "delay"), 0, G_MAXINT, NULL, NULL, inhibit_suspend_done, manager); I'm not sure if PrepareForSleep really is only send to listeners who have taken a suspend inhibitor, but regardless in order to do this in a race free manner we would need to take a suspend inhibitor in media-keys too to make sure the power_button_disabled bool gets set before suspending. I can add similar handling for this to the media-keys plugin so that we can use PrepareForSleep in a race-free manner there. But I'm not sure if you prefer that over my original dbus ping-pong method... Either way let me know and I'll prepare a new version. Regards, Hans
You're right, we need a sleep delay inhibitor in media-keys to make this race free. Still better IMHO than the alternative of adding a plugin inter-dependency.
Created attachment 363836 [details] [review] [PATCH] media-keys: Suppress power-button presses during suspend Here is a new version of the patch, with the changes we've discussed.
Review of attachment 363836 [details] [review]: with that cleanup addressed, looks fine ::: plugins/media-keys/gsd-media-keys-manager.c @@ +2965,3 @@ + manager->priv->inhibit_suspend_taken = FALSE; + } + we should also remove the reenable timer source here if it's set
Wouldn't it be better for systemd to deal with that stuff? Because I guess that the code there would need to have a similar work-around.
(In reply to Bastien Nocera from comment #8) > Wouldn't it be better for systemd to deal with that stuff? Because I guess > that the code there would need to have a similar work-around. For power-button presses GNOME is disabling systemd's handling so that it can take a different action depending on gsettings and hardware-type. And I don't think that ignoring say suspend requests for 2 seconds after resume in general is going to fly with the systemd people, and we would then need to add similar handling for any other actions which may result from the power-button.
OK, so systemd will have to have a similar work-around applied if it wants to handle suspending with the power button from the console. How about putting the quirk in libinput though? I'm not sure whether it has enough knowledge of what's happening to handle it but we have a few layers from the button itself to gnome-settings-daemon: - the kernel's input subsystem (that's already a nack) - libinput - Xorg and/or mutter/gnome-shell - gnome-settings-daemon Any other layers we could put this in?
Created attachment 364211 [details] [review] [PATCH] media-keys: Suppress power-button presses during suspend Rui, here is an updated patch adding the requested timer cleanup. I don't have gnome commit rights, can you commit this please?
(In reply to Bastien Nocera from comment #10) > OK, so systemd will have to have a similar work-around applied if it wants > to handle suspending with the power button from the console. Yes, but this problem only manifests itself on tablets (and mobile phones) I'm not sure if suspending from the console is really going to be a use-case there though. > How about putting the quirk in libinput though? I'm not sure whether it has > enough knowledge of what's happening to handle it but we have a few layers > from the button itself to gnome-settings-daemon: > - the kernel's input subsystem (that's already a nack) > - libinput libinput is not aware of suspend/resume > - Xorg and/or mutter/gnome-shell Right, so that would mean 2 different code paths for the Xorg (mutter not involved) and Wayland code-paths that does not seem like a good solution. > - gnome-settings-daemon So this seems the best option. > Any other layers we could put this in? Not that I can see.
(In reply to Hans de Goede from comment #12) > (In reply to Bastien Nocera from comment #10) > > - Xorg and/or mutter/gnome-shell > > Right, so that would mean 2 different code paths for the Xorg (mutter not > involved) and Wayland code-paths that does not seem like a good solution. gnome-shell is involved, it's what sends gnome-settings-daemon those keys...
(In reply to Bastien Nocera from comment #13) > (In reply to Hans de Goede from comment #12) > > (In reply to Bastien Nocera from comment #10) > > > - Xorg and/or mutter/gnome-shell > > > > Right, so that would mean 2 different code paths for the Xorg (mutter not > > involved) and Wayland code-paths that does not seem like a good solution. > > gnome-shell is involved, it's what sends gnome-settings-daemon those keys... Are you sure? I would expect this to be a passive-grab by g-s-d-media-keys on X11, on X11 gnome-shell is just a window-manager. Also even if gnome-shell is involved it does not seem the right layer to fix this, this is a problem with power-button handling and all the power-button handling code is in g-s-d-media-keys.
(In reply to Hans de Goede from comment #14) > > gnome-shell is involved, it's what sends gnome-settings-daemon those keys... > > Are you sure? I would expect this to be a passive-grab by g-s-d-media-keys > on X11, on X11 gnome-shell is just a window-manager. g-s-d uses a private dbus protocol with gnome-shell these days so it's gnome-shell that sets the X key grab and just relays back to g-s-d. > Also even if gnome-shell is involved it does not seem the right layer to fix > this, this is a problem with power-button handling and all the power-button > handling code is in g-s-d-media-keys. I tend to agree. Doing this in gnome-shell means adding specific knowledge about the power key there which would need extending the above mentioned dbus api. Either that, or move the whole power button handling to gnome-shell...
(In reply to Hans de Goede from comment #14) > (In reply to Bastien Nocera from comment #13) > > (In reply to Hans de Goede from comment #12) > > > (In reply to Bastien Nocera from comment #10) > > > > - Xorg and/or mutter/gnome-shell > > > > > > Right, so that would mean 2 different code paths for the Xorg (mutter not > > > involved) and Wayland code-paths that does not seem like a good solution. > > > > gnome-shell is involved, it's what sends gnome-settings-daemon those keys... > > Are you sure? I would expect this to be a passive-grab by g-s-d-media-keys > on X11, on X11 gnome-shell is just a window-manager. I'm certain, we've used a single codepath for desktop-wide key capture since 2012. Looks for "GrabAccelerator" in g-s-d and gnome-shell. > Also even if gnome-shell is involved it does not seem the right layer to fix > this, this is a problem with power-button handling and all the power-button > handling code is in g-s-d-media-keys. The right layer to fix this is the kernel having a proper API. Anything on top is just taping over problems there.
I'll add that gsd-power is an utterly brittle mess of a state machine, and that the test suite has been busted for I don't know how many years, so I'd really be happy if we didn't make this even more unmaintainable.
I agree that it would be better for from a GNOME pov for the kernel to not send the KEY_POWER on wakeup by power-button, but that is not going to happen. As for the g-s-d-power state-machine, this patch does not touch the g-s-d-power state-machine at all, it only touches media-keys and changes when media-keys will respond to KEY_POWER (and ask g-s-d-power to suspend because of a power button press), so the g-s-d-power state-machine is not touched at all. I still believe that this is the best to fix this, and this does really need fixing, so please lets move forward with the latest version of the patch as I've posted it.
Review of attachment 364211 [details] [review]: ok, I'll push this
Rui, Thank you for merging this, can we get this cherry picked into the 3.26 branch too (assuming there will be another 3.26.x release) ? Regards, Hans
Hello, I tried out the patch and it turned out not to work on my system for two reasons: 1. The timeout of 1s is too fast. It takes around 7s on my system until the key event comes in. 2. The patch only disables the power button. The same behavior can be observed with suspend, hibernate, and standby keys. (hibernate in my case) for 2.: I suggest to add the disabled check also to do_power_action in addition to do_power_button_action Marko
Hello Marko, 7 seconds is really long, too long to set as a timeout IMHO. Can you explain your use-case / your hardware in a bit more detail. You say your hardware has a hibernate key? Maybe you have other wakeup sources / other keys to wakeup the system with too and the solution is to simply not use the hibernate key to wake the system? I wrote this patch for (Intel based) tablets which can only be woken by their power-button when suspended. Regards, Hans
can we tag devices that send spurious power events, and just ignore the next event unconditionally on systems affected instead of using a timeout on all systems ?
Hi, >>7 seconds is really long, too long to set as a timeout IMHO. Can you explain >>your use-case / your hardware in a bit more detail. You say your hardware has a >>hibernate key? It's a minnowboard max board. The hibernate key is emulated by a IR remote connected via usb. The same I could observe with a lenovo thinkpad and a wireless (usb) keyboard from microsoft, when the standby key is pressed to wake up the system. To the 7secs: I'm not sure. Maybe the usb devices need to be enumerated again after resume. >>Maybe you have other wakeup sources / other keys to wakeup the system with too >>and the solution is to simply not use the hibernate key to wake the system? I >>wrote this patch for (Intel based) tablets which can only be woken by their >>power-button when suspended. Well, yeah. There is a power button on the board, gpios would work as well. But I put the board into a case. There should be nothing connected with the device but the remote control. >> can we tag devices that send spurious power events, and just ignore the next event unconditionally on systems affected instead of using a timeout on all systems ? I guess the issue can be seen with any usb based keyboard with a power or standby key.
when the KEY_POWER event comes in, is the timestamp of the event from 7 seconds ago? if so, maybe could filter out events that happen during suspend by comparing timestamp of when we resumed.