Fix for 1172692 fails to restore hibernation mode with suspend-hybrid

[Dirk F]

The patch provided in upstream bug <https://bugs.freedesktop.org/show_bug.cgi?id=52572> and implemented through 28-suspend-hybrid.patch causes error messages "sh: I/O error" when resuming with suspend-hybrid and hibernate kernel methods.

With suspend-hybrid it also forces the hibernation mode, as shown in /sys/power/disk, to "suspend" instead of restoring the original mode which is what the patch tried to do.

Please see the upstream bug for a review of the provided patch with proposed fixes.

Affects all Ubuntu and derived versions with pm-utils 1.4.1-9fix.ubuntu12.10 or later.

ProblemType: Bug
DistroRelease: Ubuntu 14.04
Package: pm-utils 1.4.1-13ubuntu0.2 [modified: usr/lib/pm-utils/pm-functions]
ProcVersionSignature: Ubuntu 4.2.0-29.34~14.04.1-generic 4.2.8-ckt3
Uname: Linux 4.2.0-29-generic i686
ApportVersion: 2.14.1-0ubuntu3.19
Architecture: i386
Date: Mon Feb 22 16:44:52 2016
InstallationDate: Installed on 2016-02-21 (0 days ago)
InstallationMedia: Lubuntu 14.04.4 LTS "Trusty Tahr" - Release i386 (20160217.1)
PackageArchitecture: all
SourcePackage: pm-utils
UpgradeStatus: No upgrade log present (probably fresh install)

[Jskarvad (jskarvad)]

Created attachment 64764
Add support for in-kernel suspend to both.

From kernel-3.6 there is in-kernel support for suspend to both (AKA hybrid suspend).

Original report with patch from Bojan (also added as attachment):
https://bugzilla.redhat.com/show_bug.cgi?id=843657

[Jskarvad (jskarvad)]

Created attachment 68712
Improved patch to save previous hibernation method

https://bugzilla.redhat.com/show_bug.cgi?id=866487

[Dirk F (fieldhouse)]

Comment on attachment 68712
Improved patch to save previous hibernation method

Review of attachment 68712:
-----------------------------------------------------------------

In do_hibernate() the attempt to save and restore the active mode in /sys/power/disk fails, causing "sh: I/O error" message in pm log (attempting to write something that isn't one of the modes in /sys/power/disk, namely an empty string). Instrumenting the function I found that HIBERNATE_MODE_SAVE was never set.

The characters [] are special in a shell pattern (which is what follows the ## and %% shell variable expansion modifiers) and have to be escaped: \[ \].

The following works as you intended:

do_hibernate()
 {
  [ -n "${HIBERNATE_MODE}" ] && \
  grep -qw "${HIBERNATE_MODE}" /sys/power/disk && \
  #df 2016-02-07 Shell patterns have to be escaped \[ \]! Fixes sh: I/O error when -z $HIBERNATE_MODE_SAVE
  HIBERNATE_MODE_SAVE=$(cat /sys/power/disk) && \
  HIBERNATE_MODE_SAVE="${HIBERNATE_MODE_SAVE##*\[}" && \
  HIBERNATE_MODE_SAVE="${HIBERNATE_MODE_SAVE%%\]*}" && \
  echo -n "${HIBERNATE_MODE}" > /sys/power/disk
  echo -n "disk" > /sys/power/state
  RET=$?
  echo -n "$HIBERNATE_MODE_SAVE" > /sys/power/disk
  return "$RET"
 }

Although you could make the penultimate line as follows I don't recommend it because it would hide any problems like the escaping issue that could cause HIBERNATE_MODE_SAVE to be invalid:

  [ -n "$HIBERNATE_MODE_SAVE" ] && echo -n "$HIBERNATE_MODE_SAVE" > /sys/power/disk

[Dirk F (fieldhouse)]

In comment #2 I wrote:
> Comment on attachment 68712 [details] [review]
>...
> Although you could make the penultimate line as follows I don't recommend it
> because it would hide any problems like the escaping issue that could cause
> HIBERNATE_MODE_SAVE to be invalid:
>
> [ -n "$HIBERNATE_MODE_SAVE" ] && echo -n "$HIBERNATE_MODE_SAVE" >
> /sys/power/disk

In fact there are 2 cases as the code is used now:

1 HIBERNATE_MODE unset => normal hibernate

2 HIBERNATE_MODE = suspend => suspend-hybrid

Given which, I've revised my comment above and propose a new version of the modified do_hibernate() as follows:

 do_hibernate()
 {
  local hibernate_mode_save ret

  [ -n "${HIBERNATE_MODE}" ] && \
   grep -qw "${HIBERNATE_MODE}" /sys/power/disk && \
   hibernate_mode_save=$(cat /sys/power/disk) && \
   hibernate_mode_save="${hibernate_mode_save##*\[}" && \
   hibernate_mode_save="${hibernate_mode_save%%\]*}" && \
   [ "$hibernate_mode_save" != "${HIBERNATE_MODE}" ] || \
   hibernate_mode_save=""
  [ -n "$hibernate_mode_save" ] && \
   echo -n "${HIBERNATE_MODE}" > /sys/power/disk
  echo -n "disk" > /sys/power/state
  ret=$?
  [ -n "$hibernate_mode_save" ] && \
   echo -n "$hibernate_mode_save" > /sys/power/disk
  return $ret
 }

The key points:
- hibernate_mode_save is only set if the current HIBERNATE_MODE is being changed (which only happens, if it does, in the suspend-hybrid case);
- on resume the hibernate mode is only restored if hibernate_mode_save was set.

This fixes:
- the failure to restore the hibernate mode with suspend-hybrid;
- "sh: I/O error" on resume from suspend-hybrid
- "sh: I/O error" on resume from hibernate.

Finally, the pm_functions script uses "echo -n" (from line 318, including the above patch) and local declarations while the comment against function log() implies that the script is aiming for POSIX conformance and yet other functions use non-POSIX local declarations. local and "echo -n" usages are fine for Debian and derived environments. To achieve POSIX conformance such usages would have to be reviewed and modified; "echo -n" could be replaced with printf (the parameters to be echoed in each case being plain text not containing formatting commands); or a shell function echo() can be added based on log().

[Dirk F (fieldhouse)]

In comment #3 I wrote:
>...
> Finally, the pm_functions script uses "echo -n" (from line 318, including
> the above patch) ...

See also bug 91497.

[Dirk F (fieldhouse)]

Created attachment 122010
patch based on review of patch 68712, taken from running pm-functions

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Dirk F (fieldhouse)]

Marked bug 1394388 as duplicate of this. The bug report points out an additional related symptom, as follows.

When issuing the pm-suspend command, the scripts set the HIBERNATE_MODE in /sys/power/disk to "suspend".

There is some shell magic to save the previous setting in variable HIBERNATE_MODE_SAVE, but this fails under the default shell /bin/dash (works correctly with /bin/bash).

Effect: if you use pm-suspend once, the effective HIBERNATE_MODE in /sys/power/disk stays in "suspend", and any subsequent pm-hibernate will also just do a suspend instead of "platform" or "shutdown".

[Martin Pitt (pitti)]

pm-utils hasn't actually been used by the indicator/lid switch/etc. desktop integration since the move to systemd. Thus this only affects Ubuntu 14.04, and manually calling "pm-suspend" in later releases.

[Ajax-a (ajax-a)]

pm-utils hasn't been touched in eight years, none of this is likely to get addressed. Closing bugs and disabling the bz product.