Bug 199693 - [bisect 08810a4119aaebf6318f209ec5dd9828e969cba4] (pci runtime) System freeze after resuming from suspend (amdgpu)
Summary: [bisect 08810a4119aaebf6318f209ec5dd9828e969cba4] (pci runtime) System freeze...
Status: CLOSED CODE_FIX
Alias: None
Product: Power Management
Classification: Unclassified
Component: Hibernation/Suspend (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Rafael J. Wysocki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-11 15:23 UTC by Thomas Martitz
Modified: 2018-10-25 10:04 UTC (History)
7 users (show)

See Also:
Kernel Version: v4.17-rc5
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
Patch making resume work (1.80 KB, patch)
2018-05-15 19:16 UTC, Thomas Martitz
Details | Diff
PM / core: Make direct_complete depend on pm_runtime_enabled() initially (608 bytes, patch)
2018-05-18 09:47 UTC, Rafael J. Wysocki
Details | Diff
PM / core: Debug devices with direct_complete set and runtime PM disabled (1.04 KB, patch)
2018-05-19 08:19 UTC, Rafael J. Wysocki
Details | Diff
dmesg (193.59 KB, text/plain)
2018-05-20 21:07 UTC, Thomas Martitz
Details
PM / core: Debug devices with direct_complete set and runtime PM disabled (1.11 KB, patch)
2018-05-21 08:20 UTC, Rafael J. Wysocki
Details | Diff
dmesg (no_pm_callbacks) (75.97 KB, text/plain)
2018-05-21 20:39 UTC, Thomas Martitz
Details
PM / core: Set direct_complete for devices without PM callbacks (961 bytes, patch)
2018-05-21 22:24 UTC, Rafael J. Wysocki
Details | Diff

Description Thomas Martitz 2018-05-11 15:23:38 UTC
See also https://bugs.freedesktop.org/show_bug.cgi?id=106447 and https://bugzilla.kernel.org/show_bug.cgi?id=199609

-----

The system immediately locks up when resuming from suspend. I get to see the mouse cursor and the blue background of KDE's screen lock (but no password entry or anything like that), but cannot do anything.

I can also reproduce on 4.16.7 and 4.17-rc4. This does not happen with amdgpu blacklisted, or with Arch Linux' LTS kernel (4.14.39) though I get other random failures on the LTS kernel.

Unfortunately, the systemd journal does not contain anything after entering suspend so I have no possibility to get at a backtrace.

-----

I did a bisect and git reported this as the culprit:

kugel@thomas-nb:linux.git$ git bisect good
08810a4119aaebf6318f209ec5dd9828e969cba4 is the first bad commit
commit 08810a4119aaebf6318f209ec5dd9828e969cba4
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Wed Oct 25 14:12:29 2017 +0200

    PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags
    
    The motivation for this change is to provide a way to work around
    a problem with the direct-complete mechanism used for avoiding
    system suspend/resume handling for devices in runtime suspend.
    
    The problem is that some middle layer code (the PCI bus type and
    the ACPI PM domain in particular) returns positive values from its
    system suspend ->prepare callbacks regardless of whether the driver's
    ->prepare returns a positive value or 0, which effectively prevents
    drivers from being able to control the direct-complete feature.
    Some drivers need that control, however, and the PCI bus type has
    grown its own flag to deal with this issue, but since it is not
    limited to PCI, it is better to address it by adding driver flags at
    the core level.
    
    To that end, add a driver_flags field to struct dev_pm_info for flags
    that can be set by device drivers at the probe time to inform the PM
    core and/or bus types, PM domains and so on on the capabilities and/or
    preferences of device drivers.  Also add two static inline helpers
    for setting that field and testing it against a given set of flags
    and make the driver core clear it automatically on driver remove
    and probe failures.
    
    Define and document two PM driver flags related to the direct-
    complete feature: NEVER_SKIP and SMART_PREPARE that can be used,
    respectively, to indicate to the PM core that the direct-complete
    mechanism should never be used for the device and to inform the
    middle layer code (bus types, PM domains etc) that it can only
    request the PM core to use the direct-complete mechanism for
    the device (by returning a positive value from its ->prepare
    callback) if it also has been requested by the driver.
    
    While at it, make the core check pm_runtime_suspended() when
    setting power.direct_complete so that it doesn't need to be
    checked by ->prepare callbacks.
    
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Acked-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

:040000 040000 6f18a781ca7ee0501888a66532f0667f2926aeb1 440821a72777285dccc37d3a8254688bf4a24486 M      Documentation
:040000 040000 6aaceba7f5aae9368a1e6e287a1f56cb1326adbf 557c1672f5101aeae16ce6bda4969c42dd3321bb M      drivers
:040000 040000 bdc707f2a476baf517361c46ed28977cb30b6e1b 7c33fb89c953ad06a7b1c8b686d6b6a403aa509b M      include

Interestingly, this commit seems to also affect my wifi. I.e. the good commits (from the susped pov) do not have working wifi, while bad commits have working wifi.

I investigated the commit a bit more, and found that the following patch (which reverts part of said commit) repairs resuming.

I can't tell the consequences, however reading the commit message suggests this part is non-critical:

> While at it, make the core check pm_runtime_suspended() when
> setting power.direct_complete so that it doesn't need to be
> checked by ->prepare callbacks.

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 02a497e7c785..028c14386e5d 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1959,9 +1959,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
         * applies to suspend transitions, however.
         */
        spin_lock_irq(&dev->power.lock);
-       dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
-               pm_runtime_suspended(dev) && ret > 0 &&
-               !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
+       dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;
        spin_unlock_irq(&dev->power.lock);
        return 0;
 }

-----


System:
Arch Linux w/ Linux 4.16.7
DMI: HP ZBook 14u G5/83B2, BIOS Q78 Ver. 01.00.05 01/25/2018
Intel Kaby Refresh 8550u
Intel UHD 620
AMD Radeon PRO WX 3100 (I believe this is Polaris, not sure about exact Generation)

lspci:
00:00.0 Host bridge: Intel Corporation Xeon E3-1200 v6/7th Gen Core Processor Host Bridge/DRAM Registers (rev 08)
00:02.0 VGA compatible controller: Intel Corporation UHD Graphics 620 (rev 07)
00:04.0 Signal processing controller: Intel Corporation Xeon E3-1200 v5/E3-1500 v5/6th Gen Core Processor Thermal Subsystem (rev 08)
00:14.0 USB controller: Intel Corporation Sunrise Point-LP USB 3.0 xHCI Controller (rev 21)
00:14.2 Signal processing controller: Intel Corporation Sunrise Point-LP Thermal subsystem (rev 21)
00:15.0 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
00:15.1 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #1 (rev 21)
00:16.0 Communication controller: Intel Corporation Sunrise Point-LP CSME HECI #1 (rev 21)
00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #1 (rev f1)
00:1c.3 PCI bridge: Intel Corporation Device 9d13 (rev f1)
00:1c.4 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #5 (rev f1)
00:1d.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #9 (rev f1)
00:1f.0 ISA bridge: Intel Corporation Device 9d4e (rev 21)
00:1f.2 Memory controller: Intel Corporation Sunrise Point-LP PMC (rev 21)
00:1f.3 Audio device: Intel Corporation Sunrise Point-LP HD Audio (rev 21)
00:1f.4 SMBus: Intel Corporation Sunrise Point-LP SMBus (rev 21)
00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection (4) I219-V (rev 21)
01:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Lexa XT [Radeon PRO WX 3100]
02:00.0 Network controller: Intel Corporation Wireless 8265 / 8275 (rev 78)
3c:00.0 Non-Volatile memory controller: Toshiba America Info Systems Device 0116
Comment 1 Zhang Rui 2018-05-15 07:27:29 UTC

*** This bug has been marked as a duplicate of bug 199609 ***
Comment 2 Thomas Martitz 2018-05-15 09:41:01 UTC
This is not the same as bug 199609 I'm afraid. This bug describes a behavior where the very first resume.

However, if I make a fix (I'll post it later this day hopefully) for this bug, then I start seeing bug 199609 too.

In fact, amdgpu developers asked me to file a bug report against the kernel while bug 199609 was moved to amdgpu.
Comment 3 Chen Yu 2018-05-15 15:26:35 UTC
It looks like some pci devices(graphic) are not safe to be kept in runtime suspended when doing suspend/resume.
Add Rafael too.
Comment 4 Thomas Martitz 2018-05-15 19:14:50 UTC
Not necessarily, I'd say. Using the attached patch (I posted this to linux-pm, too). Makes it work.

Also, I think it's another dev that affects amdgpu somehow, restricting the above patch to amdgpu-bound devices does not fix the issue.
Comment 5 Thomas Martitz 2018-05-15 19:16:15 UTC
Created attachment 275999 [details]
Patch making resume work
Comment 6 Rafael J. Wysocki 2018-05-18 08:51:22 UTC
This is a good finding, thanks!

The problem is, though, that the change in comment #5 should not affect any functionality in theory.

What is does is to prevent the initial value of power.direct_complete from depending on the pm_runtime_suspended(dev) return value at the time of its evaluation, which is in device_prepare().  However, later on, __device_suspend() will evaluate pm_runtime_status_suspended(dev) and it will clear power.direct_complete if that returns "false" without invoking any callbacks.  Moreover, it will clear power.direct_complete for the device's parent and suppliers in that case.

Now, the execution of device_prepare() takes place in the parents-first order, whereas the execution of __device_suspend() occurs in the children-first order, so no PM callbacks should be executed for the device itself and its parent, and any of its suppliers before setting power.direct_complete in device_prepare() and clearing it in __device_suspend() if the device is not runtime-suspended at that point.

Overall, the patch from comment #5 simply removes an optimization and while it can be applied in principle, it would be good to understand why it helps in the first place.
Comment 7 Rafael J. Wysocki 2018-05-18 09:47:39 UTC
Created attachment 276039 [details]
PM / core: Make direct_complete depend on pm_runtime_enabled() initially

Please check if this patch is still sufficient to make the resume work on your system.
Comment 8 Thomas Martitz 2018-05-18 21:08:57 UTC
Huge thanks for your efforts. Your explaination sheds some light to this (for me anyway) Unfortunately, your proposed patch in comment #7 does not make resume work on my system.
Comment 9 Rafael J. Wysocki 2018-05-18 21:34:40 UTC
(In reply to Thomas Martitz from comment #8)
> Huge thanks for your efforts. Your explaination sheds some light to this
> (for me anyway) Unfortunately, your proposed patch in comment #7 does not
> make resume work on my system.

But that confirms my suspicion. :-)

What appears to be happening is that there is a device in your system with runtime PM disabled and with runtime PM status set to "suspended".  If you remove the pm_runtime_enabled() component from the initial value of power.direct_complete for that device (which also effectively happens in the patch from comment #5), power.direct_complete will be set from it and that will effectively prevent any suspend/resume callbacks from being invoked for it and the device will stay functional throughout the suspend.  It will not resume properly, but that may not be a problem if it hasn't suspended in the first place.

Now, amdgpu probably depends on that device in some way, so keeping that device alive throughout the cycle makes amdgpu happy.  This likely means that amdgpu has an ordering issue exposed by the core change you've found.

Of course, setting power.direct_complete for devices with runtime PM disabled doesn't make any sense whatever, so the underlying bug needs to be fixed, so we'll need to find the offending device and deal with it.

I'll cut a patch for that in the next couple of days, but you may try to do it yourself now that you know what to look for. :-)
Comment 10 Rafael J. Wysocki 2018-05-19 08:19:05 UTC
Created attachment 276051 [details]
PM / core: Debug devices with direct_complete set and runtime PM disabled

Something like this.

It makes the change made by the patch from comment #5, but in addition to that it prints messages for devices having power.direct_complete set and runtime PM disabled (that should never happen) and aborts suspend in the next phase if at least one device like that is found.

Please apply and try to suspend.  The suspend should be aborted (if I'm not mistaken), so attach a dmesg log generated after that.
Comment 11 Thomas Martitz 2018-05-20 21:07:36 UTC
Created attachment 276075 [details]
dmesg

Here's the output. Please note for how many devices your debug statement is printed, even though it shouldn't happen.
Comment 12 Rafael J. Wysocki 2018-05-21 08:20:14 UTC
Created attachment 276081 [details]
PM / core: Debug devices with direct_complete set and runtime PM disabled

OK, thanks!

The majority of these devices don't have any PM callbacks, however, so it actually is safe to set direct_complete for them even without enabled runtime PM.

Please test this patch the same as the previous one.
Comment 13 Thomas Martitz 2018-05-21 20:39:32 UTC
Created attachment 276109 [details]
dmesg (no_pm_callbacks)

Hm, with that patch no debug message is printed. I understand that this is not your expectation?

In contrast do your previous patch, suspend was *not* aborted with this one and resume works (i guess it shouldn't be since my patch is included and no dev seems to match the criteria)
Comment 14 Rafael J. Wysocki 2018-05-21 22:05:56 UTC
(In reply to Thomas Martitz from comment #13)
> Created attachment 276109 [details]
> dmesg (no_pm_callbacks)
> 
> Hm, with that patch no debug message is printed. I understand that this is
> not your expectation?

It is one of the outcomes I took into consideration. :-)

> In contrast do your previous patch, suspend was *not* aborted with this one
> and resume works (i guess it shouldn't be since my patch is included and no
> dev seems to match the criteria)

OK, so it looks like something depends on a device without PM callbacks.  Interesting ...
Comment 15 Rafael J. Wysocki 2018-05-21 22:24:01 UTC
Created attachment 276117 [details]
PM / core: Set direct_complete for devices without PM callbacks

Please test this patch.

It should fix the resume issue for you and also it restores the direct_complete optimization for parents of devices without PM callbacks.
Comment 16 Thomas Martitz 2018-05-21 22:28:53 UTC
Indeed, your patch lets resume work. Thanks! \o/
Comment 17 Rafael J. Wysocki 2018-05-25 07:53:58 UTC
Final patch merged as commit c62ec4610c40bcc44f2d3d5ed1c312737279e2f3.

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