Thinkpad X240 resume dramatically slower on kernels 4.13+

Bug 197863 New Greg KH 7 participants 10 messages Feb 4, 2018 – Feb 9, 2018 5 messages not in the archive

Subsystems (3)

Stable branch USB ACPI

lore

Share & tools

lore

Greg KH

author maintainer

2018-02-04 06:18 UTC #5 lore raw

In reply to a message not in the archive

Quoted 35 lines · Markus Demleitner

Quoted 8 lines · Greg KH
Quoted 7 lines · Markus Demleitner
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:
[...]
Quoted 20 lines
Quoted 20 lines
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
Quoted 4 lines
Quoted 1 line
Any guess what might be behind this?
Any chance you can run 'git bisect' to find the offending commit for this issue?

Quoted 6 lines

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

Bjørn Mork

2018-02-04 20:28 UTC #6 lore raw

Quoted 11 lines · Greg KH

Quoted 6 lines · Markus Demleitner
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

From 82b8f437a243854a3f1d3c82f85520fd2b788881 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <[email protected]>
Date: Sun, 4 Feb 2018 21:15:36 +0100
Subject: [PATCH] Revert "ACPI / EC: Drop EC noirq hooks to fix a regression"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This reverts commit 662591461c4b9a1e3b9b159dbf37648a585ebaae and re-introduce the conditions dropped by commit c3a696b6e8f8 ("ACPI / EC: Use busy polling mode when GPE is not enabled")

Fixes c3a696b6e8f8 ("ACPI / EC: Use busy polling mode when GPE is not enabled")

Signed-off-by Bjørn Mork <[email protected]>

---
 drivers/acpi/ec.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/acpi/ec.c +20 -0

index d9f38c645e4a..24a772f66847 100644
@@ -1905,6 +1905,26 @@ int __init acpi_ec_ecdt_probe(void)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	if (ec == first_ec)
+		acpi_ec_enter_noirq(ec);
+	return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	if (ec == first_ec)
+		acpi_ec_leave_noirq(ec);
+	return 0;
+}
+
 static int acpi_ec_suspend(struct device *dev)
 {
 	struct acpi_ec *ec =

Signature · 1 line

2.11.0

Rafael J. Wysocki

2018-02-05 10:55 UTC #7 lore raw

Quoted 31 lines · Bjørn Mork

Quoted 10 lines · Greg KH
Quoted 6 lines · Markus Demleitner
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.

Quoted 5 lines

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bjørn Mork

2018-02-05 14:14 UTC #8 lore raw

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines · Bjørn Mork
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.

Quoted 1 line

I'll queue up a full revert of 662591461c4b9a1e3b.

Still with the additional exception for "ec == first_ec"?

Bjørn
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2018-02-05 17:06 UTC #9 lore raw

Quoted 11 lines · Bjørn Mork

Quoted 5 lines · Rafael J. Wysocki
Quoted 2 lines · Bjørn Mork
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.
Quoted 1 line
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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

maintainer

2018-02-07 22:44 UTC #10 lore raw

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Bjørn Mork
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines · Bjørn Mork
But I do wonder if the attached (completely untested!!) patch makes things any better?
I don't think so, the macro is needed too.

Quoted 8 lines

Quoted 6 lines
Doh! Obviously. Don't know how I managed to miss that.
Quoted 1 line
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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2018-02-08 19:13 UTC #11 lore raw

Quoted 17 lines · CET Rafael J. Wysocki

Quoted 15 lines · Rafael J. Wysocki
Quoted 14 lines · Bjørn Mork
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines · Bjørn Mork
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.
Quoted 1 line
I'll queue up a full revert of 662591461c4b9a1e3b.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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.

Below is a patch for the mainline (should be applicable to 4.15.y) to test. Please let me know if it improves things for you.

The corresponding fix for -stable would still be a full revert of 662591461c4b9a1e3b, but it needs to be fixed in the mainline first.

---
 drivers/acpi/ec.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/acpi/ec.c +6 -0

Index: linux-pm/drivers/acpi/ec.c
===================================================================
@@ -1927,6 +1927,9 @@ static int acpi_ec_suspend_noirq(struct
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE);
 
+	if (acpi_sleep_no_ec_events())
+		acpi_ec_enter_noirq(ec);
+
 	return 0;
 }

@@ -1934,6 +1937,9 @@ static int acpi_ec_resume_noirq(struct d
 {
 	struct acpi_ec *ec = acpi_driver_data(to_acpi_device(dev));
 
+	if (acpi_sleep_no_ec_events())
+		acpi_ec_leave_noirq(ec);
+
 	if (ec_no_wakeup && test_bit(EC_FLAGS_STARTED, &ec->flags) &&
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE);

Markus Demleitner

2018-02-09 13:43 UTC #12 lore raw

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · CET Rafael J. Wysocki
Quoted 18 lines · Rafael J. Wysocki
Quoted 16 lines · Bjørn Mork
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines · Bjørn Mork
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.
Quoted 1 line
I'll queue up a full revert of 662591461c4b9a1e3b.
Still with the additional exception for "ec == first_ec"?

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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.
Below is a patch for the mainline (should be applicable to 4.15.y) to test. Please let me know if it improves things for you.

It pretty certainly does (patch applied on top of 4.15.0 from tarball). Five out of five wakeup cycles were fast, and the cell modem is still properly initialised -- the message went out via the resumed modem.

Thanks!

-- Markus

[patch retained for reference:]

Quoted 33 lines

The corresponding fix for -stable would still be a full revert of 662591461c4b9a1e3b, but it needs to be fixed in the mainline first.

---
 drivers/acpi/ec.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/acpi/ec.c
===================================================================

drivers/acpi/ec.c +6 -0

@@ -1927,6 +1927,9 @@ static int acpi_ec_suspend_noirq(struct
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE);
 
+	if (acpi_sleep_no_ec_events())
+		acpi_ec_enter_noirq(ec);
+
 	return 0;
 }

@@ -1934,6 +1937,9 @@ static int acpi_ec_resume_noirq(struct d
 {
 	struct acpi_ec *ec = acpi_driver_data(to_acpi_device(dev));
 
+	if (acpi_sleep_no_ec_events())
+		acpi_ec_leave_noirq(ec);
+
 	if (ec_no_wakeup && test_bit(EC_FLAGS_STARTED, &ec->flags) &&
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE);

Rafael J. Wysocki

2018-02-09 21:26 UTC #13 lore raw

Quoted 28 lines · Markus Demleitner

Quoted 27 lines · Rafael J. Wysocki
Quoted 25 lines · CET Rafael J. Wysocki
Quoted 11 lines · Rafael J. Wysocki
Quoted 10 lines · Bjørn Mork
Quoted 7 lines · Rafael J. Wysocki
Quoted 2 lines · Bjørn Mork
But I do wonder if the attached (completely untested!!) patch makes things any better?
I don't think so, the macro is needed too.
Quoted 8 lines
Quoted 6 lines
Doh! Obviously. Don't know how I managed to miss that.
Quoted 1 line
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.

Quoted 7 lines

Quoted 2 lines
Below is a patch for the mainline (should be applicable to 4.15.y) to test. Please let me know if it improves things for you.
It pretty certainly does (patch applied on top of 4.15.0 from tarball). Five out of five wakeup cycles were fast, and the cell modem is still properly initialised -- the message went out via the resumed modem.

OK, let's do it, then.

Thanks for testing!
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2018-02-09 21:55 UTC #14 lore raw

PATCH 1/1 ACPI / EC: Restore polling during noirq suspend/resume phases

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit 662591461c4b (ACPI / EC: Drop EC noirq hooks to fix a regression) modified the ACPI EC driver so that it doesn't switch over to busy polling mode during noirq stages of system suspend and resume in an attempt to fix an issue resulting from that behavior.

However, that modification introduced a system resume regression on Thinkpad X240, so make the EC driver switch over to the polling mode during noirq stages of system suspend and resume again, which effectively reverts the problematic commit.

Fixes 662591461c4b (ACPI / EC: Drop EC noirq hooks to fix a regression)

Link https://bugzilla.kernel.org/show_bug.cgi?id=197863

Reported-by Markus Demleitner <[email protected]>

Tested-by Markus Demleitner <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/ec.c |    6 ++++++
 1 file changed, 6 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

drivers/acpi/ec.c +6 -0

Index: linux-pm/drivers/acpi/ec.c
===================================================================
@@ -1927,6 +1927,9 @@ static int acpi_ec_suspend_noirq(struct
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE);
 
+	if (acpi_sleep_no_ec_events())
+		acpi_ec_enter_noirq(ec);
+
 	return 0;
 }

@@ -1934,6 +1937,9 @@ static int acpi_ec_resume_noirq(struct d
 {
 	struct acpi_ec *ec = acpi_driver_data(to_acpi_device(dev));
 
+	if (acpi_sleep_no_ec_events())
+		acpi_ec_leave_noirq(ec);
+
 	if (ec_no_wakeup && test_bit(EC_FLAGS_STARTED, &ec->flags) &&
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE);