After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 613130 - suspends when switching VTs when lid is closed
suspends when switching VTs when lid is closed
Status: RESOLVED FIXED
Product: gnome-power-manager
Classification: Deprecated
Component: gnome-power-manager
2.29.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME Power Manager Maintainer(s)
GNOME Power Manager Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2010-03-17 10:20 UTC by Martin Pitt
Modified: 2010-03-22 11:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gpm debug output (11.74 KB, text/x-log)
2010-03-17 10:20 UTC, Martin Pitt
  Details
Fix wrong suspends with docked laptops (3.12 KB, patch)
2010-03-17 13:36 UTC, Martin Pitt
none Details | Review

Description Martin Pitt 2010-03-17 10:20:10 UTC
Created attachment 156345 [details]
gpm debug output

Reported at https://launchpad.net/bugs/515465:

------------- 8< ------------------
I have configured g-p-m to go into suspend to ram when closing the LID on a Laptop running Lucid..

If the system is in a docking station and started with the lid closed, the following problem occurs:
Now when switching from X to a virtual console and then back to X the laptop goes into suspend.
------------- 8< ------------------

I can replicate this behaviour on my machine. I started upower and gpm in debug mode and caught the logs.

upower side is easy. It does not get/send any events in between the first VT switch and gpm triggering suspend:

TI:11:00:17     FI:up-daemon.c  FN:up_daemon_set_pmutils_powersave,431
 - excuting command: /usr/sbin/pm-powersave false
TI:11:01:16     FI:up-daemon.c  FN:up_daemon_deferred_sleep,609
 - between AboutToSleep() and /usr/sbin/pm-suspend was 0.000007s

The switch from X to VT1 was done at 11:01:00, and the switch back from VT1 to X at 11:01:10. So it's not an upower bug.

On the first VT switch, gpm gets
TI:11:01:02     TH:0x16034f0    FI:egg-console-kit.c    FN:egg_console_kit_active_changed_cb,239
 - emitting active: 0
TI:11:01:02     TH:0x16034f0    FI:gpm-manager.c        FN:gpm_manager_console_kit_active_changed_cb,1722
 - console now inactive

On the switch back we get

TI:11:01:12     TH:0x16034f0    FI:egg-console-kit.c    FN:egg_console_kit_active_changed_cb,239
 - emitting active: 1
TI:11:01:12     TH:0x16034f0    FI:gpm-manager.c        FN:gpm_manager_console_kit_active_changed_cb,1722
 - console now active
TI:11:01:12     TH:0x16034f0    FI:gpm-manager.c        FN:gpm_manager_console_kit_active_changed_cb,1741
 - Performing AC policy as become active when lid down
TI:11:01:12     TH:0x16034f0    FI:gpm-manager.c        FN:gpm_manager_perform_policy,577
 - action: /apps/gnome-power-manager/buttons/lid_ac set to suspend (The lid has been found closed on ac power.)
TI:11:01:12     TH:0x16034f0    FI:gpm-manager.c        FN:gpm_manager_action_suspend,525
 - suspending, reason: The lid has been found closed on ac power.

I attach the full log for reference.
Comment 1 Martin Pitt 2010-03-17 11:38:27 UTC
Ah, so this logic was introduced a while ago in

  http://git.gnome.org/browse/gnome-power-manager/commit/?id=2afc1080ed12aba7cee0d6bf75b5a25363449c1d

as a proposed fix for https://bugzilla.redhat.com/show_bug.cgi?id=497262.

This was already done half a year ago, so I wonder why I noticed that just recently. It might have been shadowed by other bugs which were recently fixed, but the logic matches this bug behaviour quite clearly, in gpm_manager_console_kit_active_changed_cb().

So for RH#497262 I see two cases for the timing:

Case (A):
 1. user session ends
 2. lid gets closed
 3. gdm session starts with gpm

Case (B):
 1. user session ends
 2. gdm session starts with gpm
 3. lid gets closed

I don't think that 2afc10 fixes case (B). Here, gdm's gpm will just get a lid event and suspend through gpm_manager_lid_button_pressed(). This should have worked with or without 2afc10, since it's unrelated to CK events. The test in https://bugzilla.redhat.com/show_bug.cgi?id=497262#c4 tests precisely this case, but are we sure that it didn't work before already? (unless something was broken with gpm_manager_lid_button_pressed(), of course).

However, case (A) matches the original bug description in the RH bug, and I don't believe that this commit fixes it at all, since gpm_manager_console_kit_active_changed_cb() isn't called at gpm startup. If we'd check the lid status at startup of gpm, we would reintroduce bug 585228 ("immediately suspends on startup when lid is closed").

Thus RH#497262 case (A) and bug #585228 are mutually excluding each other, i. e. we can't DTRT for both docked laptops and the "close lid while no gpm is running" race if we just look at the status of the lid. For this case we could perhaps look at xrandr to see if there is an external active monitor.

So, in conclusion, if we revert 2afc10, we are back at a situation where changes of the active console never execute the lid policy (which are really not related to each other) and fix this bug. It should not change case (A) at all (since that needs to be checked at gpm startup, not in CK callbacks), and it should not change case (B) since that's handled through lid events, not CK callbacks.

Opinions?
Comment 2 Martin Pitt 2010-03-17 11:45:16 UTC
> I don't think that 2afc10 fixes case (B). Here, gdm's gpm will just get a lid
event and suspend through gpm_manager_lid_button_pressed().

Ah, here's my thinko. It does get a lid event, but it ignores it because it does not (yet) have the active console.

So to fix this case, we could record the lid event in gpm_manager_button_pressed_cb() and run the policy once we get the active console back. But that would again cause double-suspend issues with several gpm's running (user A suspends, resumes, switches to user B, and this would suddenly suspend). So we'd additionally need to reset the "caught ignored lid event" flag if gpm detects a suspend.
Comment 3 Martin Pitt 2010-03-17 11:54:50 UTC
(In reply to comment #2)
> > I don't think that 2afc10 fixes case (B). Here, gdm's gpm will just get a lid
> event and suspend through gpm_manager_lid_button_pressed().
> 
> Ah, here's my thinko. It does get a lid event, but it ignores it because it
> does not (yet) have the active console.

And the thinko-thinko: Above comment was true for Matthias' test case in https://bugzilla.redhat.com/show_bug.cgi?id=497262#c4 (where gpm is not on the active console). But it's a different situation than in the original bug description there, since if you log out and gdm session starts, its gpm is on an active console. So that's back to comment 1, a case which should always have worked.
Comment 4 Martin Pitt 2010-03-17 13:36:49 UTC
Created attachment 156355 [details] [review]
Fix wrong suspends with docked laptops

This patch essentially reverts commit 2afc10, see previous comments and https://bugzilla.redhat.com/show_bug.cgi?id=497262 for a rationale.

I tested both the unpatched ("current") and patched g-p-m with the following test case combinations:

1. Docked (lid always closed), starting gdm (with gpm session):
current: no suspend (correct)
with patch: no suspend (correct)

2. Docked (lid always closed), logging in (with user gpm in session):
current: no suspend (correct)
with patch: no suspend (correct)

3. Docked (lid always closed), logging out (back to gdm session):
current: no suspend (correct)
with patch: no suspend (correct)

4. Docked (lid always closed), log in two user session, switch from one to the other:
current: suspend (BUG)
with patch: no suspend (correct)

5. Docked (lid always closed), boot up to gdm or into an user session, switch to text VT (getty) and back:
current: suspend (BUG)
with patch: no suspend (correct)

6. Undocked (boot with lid open), starting gdm (with gpm session):
current: no suspend (correct)
with patch: no suspend (correct)

7. Undocked (boot with lid open), close lid (in gdm or any user sesssion):
current: suspend (correct)
with patch: suspend (correct)

8. Undocked (boot with lid open), close lid while user session is down and gdm is not yet started:
current: no suspend (BUG)
with patch: no suspend (BUG)

9. Undocked (boot with lid open), killall gnome-power-manager; sleep 10; kill -15 -1, and close lid during the sleep (same case as before, but easier to hit):
current: no suspend (BUG)
with patch: no suspend (BUG)

10. Undocked (boot with lid open), switch to text VT, sleep 10; chvt 7 (whereever gdm is), close lid during sleep, and wait 10 s:
current: attempts to suspend, gets an error dialog "cannot suspend" (attempt might be considered desirable, but is against the "only foreground events" policy)
with patch: no suspend (might be considered a bug, but consistent with "only foreground events" policy
-----

So the interesting cases are:

 - 4. That's this bug, and the patch fixes it

 - 8./9. That's the original description in RH#497262 and Case (A) in the discussion above. The behaviour isn't changed by either 2afc10 or this patch, and is a really hard problem to solve (unless we want to introduce a default policy into upower when no gpm is running)

 - 10. That's comment 4 in RH#497262 and not a realistic scenario IMHO. If no gpm has the foreground console (i. e. we are in a bash on a VT) we shouldn't try to craft a gpm response. I think sticking to our "ignore events when not having the foreground console" is a consistent and robust policy.

Case (B) from above isn't all that interesting, it's the same as "regular" suspend in a session (several test cases here).
Comment 5 Martin Pitt 2010-03-22 11:58:07 UTC
Discussed with Richard on IRC, and got approval to push this.