Bug 52572 - Please add support for in-kernel suspend to both
Summary: Please add support for in-kernel suspend to both
Status: NEW
Alias: None
Product: pm-utils
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Richard Hughes
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-27 07:25 UTC by Jaroslav Škarvada
Modified: 2016-02-28 01:15 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add support for in-kernel suspend to both. (687 bytes, patch)
2012-07-27 07:25 UTC, Jaroslav Škarvada
Details | Splinter Review
Improved patch to save previous hibernation method (938 bytes, patch)
2012-10-17 14:39 UTC, Jaroslav Škarvada
Details | Splinter Review
patch based on review of patch 68712, taken from running pm-functions (1.12 KB, patch)
2016-02-28 01:15 UTC, /df
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Škarvada 2012-07-27 07:25:40 UTC
Created attachment 64764 [details] [review]
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
Comment 1 Jaroslav Škarvada 2012-10-17 14:39:31 UTC
Created attachment 68712 [details] [review]
Improved patch to save previous hibernation method

https://bugzilla.redhat.com/show_bug.cgi?id=866487
Comment 2 /df 2016-02-07 15:29:53 UTC
Comment on attachment 68712 [details] [review]
Improved patch to save previous hibernation method

Review of attachment 68712 [details] [review]:
-----------------------------------------------------------------

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
Comment 3 /df 2016-02-22 16:38:16 UTC
In comment #2 I wrote:
> Comment on attachment 68712 [details] [review] [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().
Comment 4 /df 2016-02-22 20:38:39 UTC
In comment #3 I wrote:
>...
> Finally, the pm_functions script uses "echo -n" (from line 318, including
> the above patch) ...

See also bug 91497.
Comment 5 /df 2016-02-28 01:15:09 UTC
Created attachment 122010 [details] [review]
patch based on review of patch 68712, taken from running pm-functions


bug/show.html.tmpl processed on Aug 24, 2016 at 01:31:19.
(provided by the Example extension).