[Sent mid-August to the devel@xxxxxxxxxx mailing list and to the three
listed ACPICA maintainers. Didn't see it on the mailing list, and didn't
hear back when I asked mailman@xxxxxxxxxx. Now trying linux-acpi. I'm
watching the list so please don't cc me on replies.]
On a current Lenovo ThinkPad X1 Carbon 6th generation (Kaby Lake), the
BIOS announces only "S0i3" ("Windows Modern Standby") support and not S3
(normal suspend) support:
https://wiki.archlinux.org/index.php/Lenovo_ThinkPad_X1_Carbon_(Gen_6)
https://bbs.archlinux.org/viewtopic.php?id=234913
https://forums.lenovo.com/t5/Linux-Discussion/X1-Carbon-Gen-6-cannot-enter-deep-sleep-S3-state-aka-Suspend-to/td-p/3998182
https://delta-xi.net/#056
The information from the BIOS is inaccurate: the hardware does in fact
support S3 as hardware sleep state 5. The primary purpose of this patch
is to correct the wrong information from the BIOS on this laptop (as
identified by DMI). The secondary purpose of this patch is to add an
"acpi_force_s3=5" command-line option that lets users manually specify
the same thing on other machines.
Why is this important? Answer: When the kernel doesn't know that the
laptop supports S3, it falls back to using s2idle for suspend. However,
as the above links show, Linux users see much higher power use on this
laptop with s2idle than with S3. Also, for those of us running Linux
inside a VM in Qubes, wakeup from s2idle seems to fail on this laptop.
Presumably the wakeup bug will be found and fixed at some point, and
perhaps s2idle (and/or S0i3) will someday use as little power as S3, but
at least for the moment it's much better to have traditional S3 support.
Why doesn't the BIOS accurately report S3? The Lenovo answer is simply
that this breaks Windows:
https://forums.lenovo.com/t5/Linux-Discussion/X1-Carbon-Gen-6-cannot-enter-deep-sleep-S3-state-aka-Suspend-to/m-p/3998636/highlight/true#M10573
As the above links also show, Linux users have converged on overriding
the ACPI DSDT for this laptop to enable S3. However, my understanding is
that there's a preference to eliminate ACPI overrides in favor of kernel
patches (for fairly obvious reasons), and that this preference is strong
enough that it doesn't need justification.
I haven't done much Linux kernel programming, so please check carefully
for any coding issues, and please feel free to fix anything that's
wrong. For example, should the DMI information be prioritized over the
command-line option? I don't see a serious argument one way or the
other, and I didn't find any documentation of Linux conventions on this
topic. For the patch the answer is yes, but feel free to fix if it
should be the other way.
What bothers me more is that I haven't found documentation of how this
sort of thing is supposed to be tested. It seems to work on the laptop
where I'm typing this, and some other people have reported success:
https://marc.info/?l=qubes-users&m=153311827308201
https://marc.info/?l=qubes-users&m=153356792316425
https://marc.info/?l=qubes-users&m=153402652424867
But of course what's much more important is whether it accidentally
kicks in on other laptops and non-laptops. For example, what happens if
I screwed up the DMI testing and the code uses hardware sleep state 5 on
other laptops that actually uses a different hardware sleep state? This
would be an obvious thing to test before patch submission, but I didn't
find documentation of any Linux testing mechanism to check this on a
broad range of physical or virtual hardware, or any coding mechanism to
restrict the hardware affected by a patch.
Is this really just something checked by manual code reviews followed
by people screaming if there's a screwup? People with more Linux kernel
experience seem to think that I'm supposed to just go ahead and submit
the patch, so I'm doing that, but I'm still surprised at the apparent
lack of testing infrastructure.
Signed-off-by: "D. J. Bernstein" <djb@xxxxxxxx>
diff -ruw a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c
--- a/drivers/acpi/acpica/hwxface.c 2018-07-31 21:14:22.759000000 +0200
+++ b/drivers/acpi/acpica/hwxface.c 2018-08-01 01:48:01.087000000 +0200
@@ -44,6 +44,7 @@
#define EXPORT_ACPI_INTERFACES
#include <acpi/acpi.h>
+#include <linux/dmi.h>
#include "accommon.h"
#include "acnamesp.h"
@@ -440,6 +441,47 @@
ACPI_EXPORT_SYMBOL(acpi_write_bit_register)
#endif /* !ACPI_REDUCED_HARDWARE */
+
+/* ----- force_s3 support */
+
+/* Some systems silently support sleep type 5 for S3: */
+/* the hardware supports sleep type 5 but S3 is not in the DSDT. */
+/* Setting force_s3 to 5 will override checking the DSDT. */
+static int force_s3 = -1;
+
+/* Systems known to silently support sleep type 5 for S3. */
+static const struct dmi_system_id force_s3_5[] = {
+ {
+ .ident = "ThinkPad X1 Carbon 6th",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon 6th"),
+ },
+ },
+ { },
+};
+
+int __init acpi_force_s3_init(void)
+{
+ if (dmi_check_system(force_s3_5))
+ force_s3 = 5;
+
+ return 0;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_force_s3_init)
+
+/* Let users handle unknown systems by setting acpi_force_s3=5. */
+static int force_s3_setup(char *str)
+{
+ if (str && *str)
+ if (*str >= '0' && *str <= '7')
+ force_s3 = *str - '0';
+ return 1;
+}
+
+__setup("acpi_force_s3=", force_s3_setup);
+
/*******************************************************************************
*
* FUNCTION: acpi_get_sleep_type_data
@@ -492,6 +534,14 @@
return_ACPI_STATUS(AE_BAD_PARAMETER);
}
+ /* Support force_s3 */
+
+ if (force_s3 >= 0 && sleep_state == ACPI_STATE_S3) {
+ *sleep_type_a = force_s3;
+ *sleep_type_b = force_s3;
+ return_ACPI_STATUS(AE_OK);
+ }
+
/* Allocate the evaluation information block */
info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info));
diff -ruw a/drivers/acpi/bus.c b/drivers/acpi/bus.c
--- a/drivers/acpi/bus.c 2018-07-31 21:14:22.762000000 +0200
+++ b/drivers/acpi/bus.c 2018-08-01 02:45:53.527000000 +0200
@@ -1192,6 +1192,7 @@
printk(KERN_INFO PREFIX "Interpreter enabled\n");
/* Initialize sleep structures */
+ acpi_force_s3_init();
acpi_sleep_init();
/*
diff -ruw a/drivers/acpi/internal.h b/drivers/acpi/internal.h
--- a/drivers/acpi/internal.h 2018-07-31 21:14:22.762000000 +0200
+++ b/drivers/acpi/internal.h 2018-08-01 01:48:01.088000000 +0200
@@ -183,6 +183,7 @@
/* External interfaces use first EC only, so remember */
typedef int (*acpi_ec_query_func) (void *data);
+int acpi_force_s3_init(void);
int acpi_ec_init(void);
int acpi_ec_ecdt_probe(void);
int acpi_ec_dsdt_probe(void);
Attachment:
signature.asc
Description: PGP signature
