Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Rafael J. Wysocki]

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

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Bjørn Mork]

"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

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Rafael J. Wysocki]

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

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Rafael J. Wysocki]

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

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Rafael J. Wysocki]

On Wednesday, February 7, 2018 11:44:15 PM CET Rafael J. Wysocki wrote:
> 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.

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(+)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ 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);

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Markus Demleitner]

On Thu, Feb 08, 2018 at 08:12:06PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 7, 2018 11:44:15 PM CET Rafael J. Wysocki wrote:
> > 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.
> 
> 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:]

> 
> 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
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ 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);
> 

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Re: [Bug 197863] Thinkpad X240 resume dramatically slower on kernels 4.13+

[Rafael J. Wysocki]

On Fri, Feb 9, 2018 at 2:43 PM, Markus Demleitner <m@tfiu.de> wrote:
> On Thu, Feb 08, 2018 at 08:12:06PM +0100, Rafael J. Wysocki wrote:
>> On Wednesday, February 7, 2018 11:44:15 PM CET Rafael J. Wysocki wrote:
>> > On Mon, Feb 5, 2018 at 6:06 PM, Rafael J. Wysocki
>> > <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> > > On 2/5/2018 3:14 PM, Bjørn Mork wrote:
>> > >>
>> > >> "Rafael J. Wysocki" <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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.
>>
>> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <m@tfiu.de>
Tested-by: Markus Demleitner <m@tfiu.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ 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);