Bug 199393 - S3 isn't supported and hangs, mem_sleep should mapped to s2idle by default - Dell Venue Pro 7140
Summary: S3 isn't supported and hangs, mem_sleep should mapped to s2idle by default - ...
Status: RESOLVED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Sleep-Wake (show other bugs)
Hardware: Intel Linux
: P1 normal
Assignee: Rafael J. Wysocki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-14 17:43 UTC by Tristian Celestin
Modified: 2018-06-28 08:21 UTC (History)
6 users (show)

See Also:
Kernel Version: 4.16 4.15 4.14 4.13
Subsystem:
Regression: No
Bisected commit-id:


Attachments
FACP of Dell Venue Pro 7140 (9.83 KB, text/plain)
2018-04-14 18:56 UTC, Tristian Celestin
Details
DSDT of Dell Venue Pro 7140 (560.21 KB, text/plain)
2018-04-14 18:59 UTC, Tristian Celestin
Details
FACP of Dell Latitude 5175 (9.82 KB, text/x-csrc)
2018-04-14 19:20 UTC, Tristian Celestin
Details
DSDT of Dell Latitude 5175 (1.06 MB, text/x-csrc)
2018-04-14 19:21 UTC, Tristian Celestin
Details
Omit function bitfield check when deciding to use s2idle by default (1.86 KB, patch)
2018-04-22 21:50 UTC, Tristian Celestin
Details | Diff

Description Tristian Celestin 2018-04-14 17:43:43 UTC
Dell Venue Pro 7140 is a 2-in-1 detachable laptop with an Intel Core-M processor (5th gen "Broadwell"). This machine was originally released with Windows 8.1, and supported "Modern Standby". It is upgradable to Windows 10 and supports "Connected Standby".

The S3 sleep state is not supported. If I perform a S3 suspend, the machine can not be awoken. I can hold the power button to force the machine off, release it, then press it again to boot the machine again.

If I use [deep] as the suspend method in /sys/power/mem_sleep, and setup pm_test using the following commands, the machine wakes back up when I run echo mem > /sys/power/state without an error code:

echo "freezer" > /sys/power/pm_test
echo "devices" > /sys/power/pm_test
echo "platform" > /sys/power/pm_test
echo "processors" > /sys/power/pm_test
echo "core" > /sys/power/pm_test

The machine doesn't wake back up at all when I setup pm_test using the following command:

echo "none" > /sys/power/pm_test

I can only shutdown the machine by holding the power button for ~6 seconds, then booting the machine again with another short press to the power button.

If I perform pm_test commands using [s2idle] as the suspend method in /sys/power/mem_sleep, the following tests pass:

echo "freezer" > /sys/power/pm_test
echo "devices" > /sys/power/pm_test
echo "platform" > /sys/power/pm_test
echo "none" > /sys/power/pm_test

i. e. none of the tests fail for s2idle. Therefore, I think s2idle should be reported as the default suspend method.

These conditions sound like those addressed in a previous bug [1], except I am dealing with a different Dell model. In fact, I also have a Dell Latitude 5175, which is the Venue Pro 7140's successor. When I start the machine, it reports s2idle as the default suspend method (cat /sys/power/mem_sleep). I expect the same of the 7140, but it reports [deep] by default.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=195897
Comment 1 Tristian Celestin 2018-04-14 18:40:05 UTC
I narrowed down what I believe to be the cause of the difference, but I don't know how to proceed.

The acpi driver file in platform/acpi/sleep.c has a method, lps0_device_attach(...) that determines whether a device should be set to s2idle by default. The relevant block is:

       if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {                                                         
                char bitmask = *(char *)out_obj->buffer.pointer;                                                    
                                                                                                                                                                                                                              
                if ((bitmask & ACPI_LPS0_PLATFORM_MASK) == ACPI_LPS0_PLATFORM_MASK ||                               
                    (bitmask & ACPI_LPS0_SCREEN_MASK) == ACPI_LPS0_SCREEN_MASK) {                                   
                        lps0_dsm_func_mask = bitmask;                                                               
                        lps0_device_handle = adev->handle;                                                          
                        /*                                                                                          
                         * Use suspend-to-idle by default if the default                                            
                         * suspend mode was not set from the command line.                                          
                         */                                                                                         
                        if (mem_sleep_default > PM_SUSPEND_MEM)                                                     
                                mem_sleep_current = PM_SUSPEND_TO_IDLE;                                             
                }

I placed a bunch of acpi_handle_debug statements prior to the bitmask check:

                acpi_handle_debug(adev->handle, "bitmask is 0x%x\n", bitmask);                                      
                acpi_handle_debug(adev->handle, "ACPI_LPS0_PLATFORM_MASK is 0x%x\n", ACPI_LPS0_PLATFORM_MASK);      
                acpi_handle_debug(adev->handle, "bitmask & ACPI_LPS0_PLATFORM_MASK is 0x%x\n", bitmask & ACPI_LPS0_P
LATFORM_MASK);                                                                                                      
                acpi_handle_debug(adev->handle, "ACPI_LPS0_SCREEN_MASK is %x\n", ACPI_LPS0_SCREEN_MASK);            
                acpi_handle_debug(adev->handle, "bitmask & ACPI_LPS0_SCREEN_MASK is 0x%x\n", bitmask & ACPI_LPS0_SCR
EEN_MASK);                                                                                                          
                                                                                                                    
                if ((bitmask & ACPI_LPS0_PLATFORM_MASK) == ACPI_LPS0_PLATFORM_MASK ...

and received the following output:

[    0.000000] ACPI: LAPIC_NMI (acpi_id[0x01] low level lint[0x24])
[    0.000000] ACPI: LAPIC_NMI (acpi_id[0x02] dfl res lint[0x1])
[    0.000000] ACPI: LAPIC_NMI (acpi_id[0x03] dfl edge lint[0xc6])
[    0.000000] ACPI: LAPIC_NMI (acpi_id[0x04] dfl dfl lint[0x44])
[    0.060484] acpiphp: ACPI Hot Plug PCI Controller Driver version: 0.5
[    0.116014] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
[    0.116014] acpi PNP0A08:00: _OSC failed (AE_ERROR); disabling ASPM
[    0.139573] acpi:920: ACPI: \_SB_.PEPD: bitmask is 0x7
[    0.139575] acpi:921: ACPI: \_SB_.PEPD: ACPI_LPS0_PLATFORM_MASK is 0x60
[    0.139577] acpi:922: ACPI: \_SB_.PEPD: bitmask & ACPI_LPS0_PLATFORM_MASK is 0x0
[    0.139579] acpi:923: ACPI: \_SB_.PEPD: ACPI_LPS0_SCREEN_MASK is 18
[    0.139580] acpi:924: ACPI: \_SB_.PEPD: bitmask & ACPI_LPS0_SCREEN_MASK is 0x0
[    0.139582] acpi:939: ACPI: \_SB_.PEPD: _DSM function mask: 0x7
[    0.139584] acpi:754: ACPI: \: _DSM function 1 eval failed
[    0.139585] acpi:757: ACPI: \: out_obj is NULL
[    0.203942] clocksource: acpi_pm: mask: 0xffffff max_cycles: 0xffffff, max_idle_ns: 2085701024 ns
Comment 2 Tristian Celestin 2018-04-14 18:56:07 UTC
Created attachment 275365 [details]
FACP of Dell Venue Pro 7140

This FACP confirms that the platform supports s2idle (Low Power S0 Idle).
Comment 3 Tristian Celestin 2018-04-14 18:59:03 UTC
Created attachment 275367 [details]
DSDT of Dell Venue Pro 7140

This DSDT is provided by the version A15 firmware found here [1].

[1] http://www.dell.com/support/home/us/en/19/drivers/driversdetails?driverId=8NW7F
Comment 4 Tristian Celestin 2018-04-14 19:20:14 UTC
Created attachment 275369 [details]
FACP of Dell Latitude 5175

Added the FACP of the Dell Latitude 5175 as a point of comparison. s2idle is recognized as the default suspend method on this machine.
Comment 5 Tristian Celestin 2018-04-14 19:21:40 UTC
Created attachment 275371 [details]
DSDT of Dell Latitude 5175

Added DSDT of Dell Latitude 5175 as a point of comparison. s2idle is recognized as the default suspend method on this machine.
Comment 6 Tristian Celestin 2018-04-15 13:38:45 UTC
I think I understand why the following line in the 7140's dmesg appears:

[    0.139573] acpi:920: ACPI: \_SB_.PEPD: bitmask is 0x7


The _DSM in the 7140 returns the value when ARG2 is set to 0. On line 13760:

               If ((Arg0 == ToUUID ("c4eb40a0-6cd2-11e2-bcfd-0800200c9a66")))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x07
                        })
                    }

while the _DSM in the 5175 returns a different value. On line 27255:

                If ((Arg0 == ToUUID ("c4eb40a0-6cd2-11e2-bcfd-0800200c9a66")))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x7F
                        })
                    }
Comment 7 Tristian Celestin 2018-04-18 01:11:07 UTC
After coming across [1], I understand what the return values in the ACPI snippets mean: they indicate which functions are supported by the _DSM for PNP0D80. Per the spec, there are 7 functions in total. The 7140 only supports the first 3, so 0x07 (0b0000111) is returned.

The Linux driver platform/acpi/sleep.c will only support a default sleep state of s2idle if functions 3 & 4 are supported, or if functions 5 & 6 are supported. How come the presence of functions 1 & 2 are not considered enough?

[1] http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
Comment 8 Mario Limonciello 2018-04-19 17:53:27 UTC
Tristian,

You did an excellent job collecting and organizing all of these details and drawing the conclusions you did.

My *guess* would be that originally Windows 8.1 didn't have a support for calling the screen on/off or deep resiliency enter/exit arguments to that _DSM method and the BIOS was not modified to introduce them when Windows 10 was released.

When the support for choosing to default to s2idle was selected it was based upon heuristics of devices that existed at the time.  See this commit for more details:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/drivers/acpi/sleep.c?h=linux-next&id=8110dd281e155e5010ffd657bba4742ebef7a93f

Later there were more systems found to be problematic so it was adjusted:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/drivers/acpi/sleep.c?h=linux-next&id=29a5a6d7082427371519ae1e186d9e35612801fb

Based on the evidence you've shared it *might* make sense to adjust the heuristics to not take this extra bitmaask into account to choose the defaults.  Rafael will need to comment on this.  Feel free to produce a patch that adjusts this and send it to the linux-acpi mailing list if Rafael doesn't comment on this bug soon.
Comment 9 Tristian Celestin 2018-04-22 21:50:11 UTC
Created attachment 275499 [details]
Omit function bitfield check when deciding to use s2idle by default

I've attached a patch to disable the heuristic when deciding to enable s2idle.
Comment 10 Mario Limonciello 2018-04-23 13:57:06 UTC
@Tristian,

Can you please submit this to the linux-acpi mailing list?  Also I believe those two bitmask #defines are only used by this function comparison.  If they're no longer used, the #define should also be removed when you submit the patch.

Thanks
Comment 11 Tristian Celestin 2018-04-26 02:20:39 UTC
I've removed the bitmask #defines from the patch and sent it to the linux-acpi mailing list.
Comment 12 Zhang Rui 2018-05-08 03:23:54 UTC
well, to me, the first question is
does Dell suppose to have S3 support on this platform or not?
we have a Dell machine that has broken S3, which we thought it is a BIOS bug, and then later, we found S3 starts to work well in the latest 4.17-rc release.

So, I have the same concern on this platform, we should blacklist S3 on this platform only if
1. we're sure the BIOS does not support S3 properly
2. we're sure Dell does not have the plan to fix the BIOS.
Comment 13 Mario Limonciello 2018-05-08 13:39:29 UTC
@Zhang Rui,

On this particular platform (Venue Pro 7140) there was not a dedicated validation of S3 as the platform shipped in connected standby/modern standby with Windows and did not ship with Linux.

There are no plans to change this right now from Dell BIOS side.
Comment 14 Zhang Rui 2018-06-28 08:21:05 UTC
I see.

https://patchwork.kernel.org/patch/10465883/
Bug marked as resolved as the patch has already been sent to public.
I think the patch is in Rafael' queue.
Will close this bug report after the patch merged, or reopen it if the patch is rejected.

Note You need to log in before you can comment on or make changes to this bug.