[systemd-devel] [PATCH] Add support for Intel Rapid Start

[Bastien Nocera]

On Sun, 2013-10-13 at 22:40 +0200, Zbigniew Jędrzejewski-Szmek wrote:
> On Sun, Oct 13, 2013 at 09:50:59PM +0200, Bastien Nocera wrote:
> > 
> > Instead of using the kernel's hybrid sleep, use the firmware for
> > laptops that support Intel Rapid Start, as explained in:
> > http://mjg59.dreamwidth.org/26022.html
> > and implemented in:
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/drivers/platform/x86/intel-rst.c
> > ---
> >  src/shared/sleep-config.c | 28 ++++++++++++++++++++++++++--
> >  src/shared/sleep-config.h |  1 +
> >  src/sleep/sleep.c         |  3 +++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c
> > index d068bfc..c5551fc 100644
> > --- a/src/shared/sleep-config.c
> > +++ b/src/shared/sleep-config.c
> > @@ -250,6 +250,26 @@ static bool enough_memory_for_hibernation(void) {
> >          return r;
> >  }
> >  
> > +/* Check whether Intel Rapid Start Technology is supported:
> > + * http://mjg59.dreamwidth.org/26022.html */
> > +bool has_rapid_start(void) {
> > +        int r;
> > +        _cleanup_free_ char *p = NULL;
> > +
> > +        r = read_one_line_file("/sys/bus/acpi/devices/INT3392:00/wakeup_events", &p);
> > +        if (r < 0)
> > +                return false;
> > +
> > +        /* We only support rapid start if we have both values set:
> > +         * 1: Wake to enter hibernation when the wakeup timer expires
> > +         * 2: Wake to enter hibernation when the battery reaches a
> > +         * critical level */
> > +        if (p[0] == '3' && p[1] == '\0')
> > +                return true;
> > +
> > +        return false;
> > +}
> > +
> This part looks ok.

I've made the changes that Matthew suggested locally (and I learnt
glob(3), cool).

> >  int can_sleep(const char *verb) {
> >          _cleanup_strv_free_ char **modes = NULL, **states = NULL;
> >          int r;
> > @@ -262,8 +282,12 @@ int can_sleep(const char *verb) {
> >          if (r < 0)
> >                  return false;
> >  
> > -        if (!can_sleep_state(states) || !can_sleep_disk(modes))
> > +        if (!can_sleep_state(states) || !can_sleep_disk(modes) || !has_rapid_start())
> >                  return false;
> This disables sleep support for everyone who doesn't have rapid start...

Yeah, that's spurious, removed.

> > -        return streq(verb, "suspend") || enough_memory_for_hibernation();
> > +        if streq(verb, "suspend")
> > +                return true;
> > +        if (streq (verb, "hybrid-sleep"))
> > +                return enough_memory_for_hibernation() || has_rapid_start();
> > +        return enough_memory_for_hibernation();
> Does this compile?

It does, and I see where you think there's a problem. Fixed locally.

> > @@ -211,6 +211,9 @@ int main(int argc, char *argv[]) {
> >          if (r <= 0)
> >                  goto finish;
> >  
> > +        if (has_rapid_start () && streq(arg_verb, "hybrid-sleep"))
> > +                arg_verb = "suspend";
> > +
> >          r = parse_sleep_config(arg_verb, &modes, &states);
> >          if (r < 0)
> >                  goto finish;
> So, how exactly does this work? We request suspend from the kernel, and get
> something functionally equivalent to hybrid-sleep instead? I think it would
> be better to do something mjg suggested in the other mail: to configure this
> explicitly by writting appropriate values when hybrid sleep has been requested.

We just suspend to memory, and the firmware will handle saving to disk
if and when battery is low, or after X amount of time.

IMO, it makes no sense to use the kernel provided hybrid-sleep when
Intel Fast Start is there. It has all the inconveniences of hibernation
(slow when you want to suspend, brittle as you might lose everything
when selecting the wrong kernel in your grub/gummiboot menu, and as the
kernel interacts with the drivers in a way that some of them don't like,
and ugly, as there's no "plymouth"-y UI for it).

A new suspend mode as Matthew suggested ("firmware-hybrid-sleep"?) would
still require extending systemd, but in a much less invasive way.

> > > +        r = read_one_line_file("/sys/bus/acpi/devices/INT3392:00/wakeup_events", &p);
> > /sys/bus/acpi/drivers/intel_rapid_start/*/wakeup_events would be better,
> > if there's any way to use wildcards here.
> There's a bunch of glob-related functions, like glob_exists or glob_extend
> in util.h.

Used, thanks.

I've posted the updated patch here:
https://gist.github.com/hadess/6968197

And I'll wait on the result of Matthew's discussions to follow up.

Cheers