Re: [Linux-stable-mirror] [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+
On Sat, Feb 03, 2018 at 07:25:54PM +0100, Markus Demleitner wrote:
On Tue, Jan 30, 2018 at 07:32:04AM +0100, Greg KH wrote:
On Mon, Jan 29, 2018 at 07:21:09PM +0100, Markus Demleitner wrote:
A while ago I opened bug #197863 in the kernel bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=197863
Essentially, xhci_hcd on a resume from RAM takes several seconds on a Thinkpad X240 that is equipped with a Sierra LTE modem after an upgrade to 4.13:
[...]
Now, interestingly, there are quick resumes in 4.15.0, too, now and then. In that case, things look pretty much like on 4.12.2. Here's a 4.15.0 fast resume example:
[ 873.127570] usb 1-4: device firmware changed [ 873.277351] usb 1-8: reset high-speed USB device number 4 using xhci_hcd [ 873.515141] restoring control 00000000-0000-0000-0000-000000000101/0/2 [ 873.583238] OOM killer enabled. [ 873.583240] Restarting tasks ... [ 873.583339] usb 1-4: USB disconnect, device number 10 [ 873.586356] done. [ 873.604420] PM: suspend exit [ 873.737283] usb 1-4: new high-speed USB device number 11 using xhci_hcd [ 880.867398] usb 1-4: device descriptor read/64, error -110 [ 881.175558] usb 1-4: New USB device found, idVendor=1199, idProduct=a001 [ 881.175563] usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 881.175566] usb 1-4: Product: Sierra Wireless EM7345 4G LTE [ 881.175568] usb 1-4: Manufacturer: Sierra Wireless Inc. [ 881.175570] usb 1-4: SerialNumber: 013937002544516
Any guess what might be behind this?
Any chance you can run 'git bisect' to find the offending commit for this issue?
It's 662591461c4b9a1e3b9b159dbf37648a585ebaae. To my eyes, it even looks plausible that it's causing the problematic behaviour, but since I can't say I understand what I'd be doing if I dabbled with the change, I've refrained from guessing how to fix it.
I'm happy to try patches, though.
Ok, thanks. I've added the authors of this patch to the email here, perhaps they have an idea of what is going on?
greg k-h
Greg KH gregkh@linuxfoundation.org writes:
On Sat, Feb 03, 2018 at 07:25:54PM +0100, Markus Demleitner wrote:
It's 662591461c4b9a1e3b9b159dbf37648a585ebaae. To my eyes, it even looks plausible that it's causing the problematic behaviour, but since I can't say I understand what I'd be doing if I dabbled with the change, I've refrained from guessing how to fix it.
I'm happy to try patches, though.
Ok, thanks. I've added the authors of this patch to the email here, perhaps they have an idea of what is going on?
This thing made me curious enough to dive into code I don't understand, as I have experienced the annoying crazy fan behaviour in resume a few times on my X1 Carbon 4th gen.
Maybe I missed something, but it looks like commit
c3a696b6e8f8 ("ACPI / EC: Use busy polling mode when GPE is not enabled")
introduced suspend/resume busy polling for the "boot EC" unintentionally?
The patch moved acpi_ec_leave_noirq() and acpi_ec_leave_noirq() functions outside the #ifdef CONFIG_PM_SLEEP, so they could be reused while installing handlers. But when doing that the
if (ec == first_ec)
conditions on suspend/resume were silently dropped. I assume the intention might have been to move those intto acpi_ec_suspend_noirq() and acpi_ec_resume_noirq() instead? But that didn't happen AFAICS.
Or did I misunderstand this completely? Not unlikely given that I have zero clue about what this code is doing...
But I do wonder if the attached (completely untested!!) patch makes things any better?
Bjørn
On 2/4/2018 9:28 PM, Bjørn Mork wrote:
Greg KH gregkh@linuxfoundation.org writes:
On Sat, Feb 03, 2018 at 07:25:54PM +0100, Markus Demleitner wrote:
It's 662591461c4b9a1e3b9b159dbf37648a585ebaae. To my eyes, it even looks plausible that it's causing the problematic behaviour, but since I can't say I understand what I'd be doing if I dabbled with the change, I've refrained from guessing how to fix it.
I'm happy to try patches, though.
Ok, thanks. I've added the authors of this patch to the email here, perhaps they have an idea of what is going on?
This thing made me curious enough to dive into code I don't understand, as I have experienced the annoying crazy fan behaviour in resume a few times on my X1 Carbon 4th gen.
Maybe I missed something, but it looks like commit
c3a696b6e8f8 ("ACPI / EC: Use busy polling mode when GPE is not enabled")
introduced suspend/resume busy polling for the "boot EC" unintentionally?
The patch moved acpi_ec_leave_noirq() and acpi_ec_leave_noirq() functions outside the #ifdef CONFIG_PM_SLEEP, so they could be reused while installing handlers. But when doing that the
if (ec == first_ec)conditions on suspend/resume were silently dropped. I assume the intention might have been to move those intto acpi_ec_suspend_noirq() and acpi_ec_resume_noirq() instead? But that didn't happen AFAICS.
Or did I misunderstand this completely? Not unlikely given that I have zero clue about what this code is doing...
But I do wonder if the attached (completely untested!!) patch makes things any better?
I don't think so, the macro is needed too.
I'll queue up a full revert of 662591461c4b9a1e3b.
Thanks, Rafael
"Rafael J. Wysocki" rafael.j.wysocki@intel.com writes:
On 2/4/2018 9:28 PM, Bjørn Mork wrote:
But I do wonder if the attached (completely untested!!) patch makes things any better?
I don't think so, the macro is needed too.
Doh! Obviously. Don't know how I managed to miss that.
I'll queue up a full revert of 662591461c4b9a1e3b.
Still with the additional exception for "ec == first_ec"?
Bjørn
On 2/5/2018 3:14 PM, Bjørn Mork wrote:
"Rafael J. Wysocki" rafael.j.wysocki@intel.com writes:
On 2/4/2018 9:28 PM, Bjørn Mork wrote:
But I do wonder if the attached (completely untested!!) patch makes things any better?
I don't think so, the macro is needed too.
Doh! Obviously. Don't know how I managed to miss that.
I'll queue up a full revert of 662591461c4b9a1e3b.
Still with the additional exception for "ec == first_ec"?
No, just a full revert for now.
The above can be fixed on top of that.
Thanks, Rafael
On Mon, Feb 5, 2018 at 6:06 PM, Rafael J. Wysocki rafael.j.wysocki@intel.com wrote:
On 2/5/2018 3:14 PM, Bjørn Mork wrote:
"Rafael J. Wysocki" rafael.j.wysocki@intel.com writes:
On 2/4/2018 9:28 PM, Bjørn Mork wrote:
But I do wonder if the attached (completely untested!!) patch makes things any better?
I don't think so, the macro is needed too.
Doh! Obviously. Don't know how I managed to miss that.
I'll queue up a full revert of 662591461c4b9a1e3b.
Still with the additional exception for "ec == first_ec"?
No, just a full revert for now.
That doesn't work, because we made some changes on top of this commit.
I'll send a patch to try tomorrow.
Thanks, Rafael
linux-stable-mirror@lists.linaro.org
-
Bjørn Mork -
Greg KH -
Rafael J. Wysocki -
Rafael J. Wysocki