Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1510511 > unrolled thread

Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online calls to hotplugged cpu")]

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-27 19:30 +0200
    Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Thomas Gleixner <tglx@linutronix.de> - 2016-10-27 21:00 +0200
      Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Thomas Gleixner <tglx@linutronix.de> - 2016-10-27 21:30 +0200
        Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-27 22:40 +0200
          Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Thomas Gleixner <tglx@linutronix.de> - 2016-10-27 22:50 +0200
            Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-28 18:00 +0200
              Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Thomas Gleixner <tglx@linutronix.de> - 2016-10-28 21:10 +0200
                Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-11-01 21:50 +0100
                  Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-11-07 13:00 +0100
                    Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Thomas Gleixner <tglx@linutronix.de> - 2016-11-07 14:20 +0100
                      Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-11-07 17:50 +0100
                  Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Feng Tang <feng.tang@intel.com> - 2016-11-09 04:50 +0100
                    Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-09 07:10 +0100
      Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online  calls to hotplugged cpu")] Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-27 21:30 +0200

#1510511 — Re: S3 resume regression [1cf4f629d9d2 ("cpu/hotplug: Move online calls to hotplugged cpu")]

On Tue, Aug 09, 2016 at 08:20:57PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 14, 2016 at 04:29:42PM +0800, Feng Tang wrote:
> > if you only want it to work, you can try an old patch
> > https://bugzilla.kernel.org/attachment.cgi?id=76071 from a similar bug
> > https://bugzilla.kernel.org/show_bug.cgi?id=41932
> > 
> > Alistair Buxton confirmed it work for 3.18 at least
> > https://bugzilla.kernel.org/show_bug.cgi?id=107151#c16
> 
> That patch is a bit too ripe by now. Would need a fresh squeezed one.

Since no one else bothered to refresh the patch I did it myself:

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index f6aae7977824..d73d094a8972 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -657,8 +657,16 @@ static void tick_handle_oneshot_broadcast(struct clock_event_device *dev)
 	 * - There are pending events on sleeping CPUs which were not
 	 * in the event mask
 	 */
-	if (next_event.tv64 != KTIME_MAX)
+	if (next_event.tv64 != KTIME_MAX) {
+		s64 delta = next_event.tv64 - now.tv64;
+
+		if (delta >= 10000000000) {
+			printk(KERN_CRIT "%s(): The delta is big: %lld\n", __func__, delta);
+			next_event.tv64 = now.tv64 + 3000000000;
+		}
+
 		tick_broadcast_set_event(dev, next_cpu, next_event);
+	}
 
 	raw_spin_unlock(&tick_broadcast_lock);

Unfortunately it doesn't do anything for me.

The fortunate thing is that acpi-idle has magically been fixed in the
meantime, so I can at least go back to using that one and have working
S3.

-- 
Ville Syrjälä
Intel OTC

[toc] | [next] | [standalone]

#1510577

[Multipart message — attachments visible in raw view] — view raw

On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> On Tue, Aug 09, 2016 at 08:20:57PM +0300, Ville Syrjälä wrote:
> > On Thu, Jul 14, 2016 at 04:29:42PM +0800, Feng Tang wrote:
> > > if you only want it to work, you can try an old patch
> > > https://bugzilla.kernel.org/attachment.cgi?id=76071 from a similar bug
> > > https://bugzilla.kernel.org/show_bug.cgi?id=41932
> > > 
> > > Alistair Buxton confirmed it work for 3.18 at least
> > > https://bugzilla.kernel.org/show_bug.cgi?id=107151#c16
> > 
> > That patch is a bit too ripe by now. Would need a fresh squeezed one.
> 
> Since no one else bothered to refresh the patch I did it myself:
> 
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index f6aae7977824..d73d094a8972 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -657,8 +657,16 @@ static void tick_handle_oneshot_broadcast(struct clock_event_device *dev)
>  	 * - There are pending events on sleeping CPUs which were not
>  	 * in the event mask
>  	 */
> -	if (next_event.tv64 != KTIME_MAX)
> +	if (next_event.tv64 != KTIME_MAX) {
> +		s64 delta = next_event.tv64 - now.tv64;
> +
> +		if (delta >= 10000000000) {
> +			printk(KERN_CRIT "%s(): The delta is big: %lld\n", __func__, delta);
> +			next_event.tv64 = now.tv64 + 3000000000;
> +		}
> +
>  		tick_broadcast_set_event(dev, next_cpu, next_event);
> +	}
>  
>  	raw_spin_unlock(&tick_broadcast_lock);
> 
> Unfortunately it doesn't do anything for me.

And I'm not surprised, because the original patch forced a 5 seconds event
in the broadcast device on resume, aside of limiting the reprogramming.

What that old patch did, was:

1) Make sure that the broadcast device is actually armed at resume.

   That might cause the HPET to resume proper.

2) Force a max. 3 seconds rearm when the targeted expiry time is > than 10
   seconds

   That might make sure that lower C-States are never entered.
 
> The fortunate thing is that acpi-idle has magically been fixed in the
> meantime, so I can at least go back to using that one and have working
> S3.

What's the lowest C-State with acpi-idle and what's the lowest one with
intel_idle?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1510592

[Multipart message — attachments visible in raw view] — view raw

On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> On Thu, Oct 27, 2016 at 08:48:57PM +0200, Thomas Gleixner wrote:
> > What that old patch did, was:
> > 
> > 1) Make sure that the broadcast device is actually armed at resume.
> > 
> >    That might cause the HPET to resume proper.
> > 
> > 2) Force a max. 3 seconds rearm when the targeted expiry time is > than 10
> >    seconds
> > 
> >    That might make sure that lower C-States are never entered.
> 
> Doh. I lost the other hunk somewhere. Let's try that again... And indeed
> with the other hunk in tow the machine would appear to resume properly.

So it would be interesting whether that hunk in resume_broadcast() is
sufficient.
 
> > What's the lowest C-State with acpi-idle and what's the lowest one with
> > intel_idle?
> 
> acpi_idle
> /sys/devices/system/cpu/cpu0/cpuidle/state3/desc:ACPI FFH INTEL MWAIT 0x30
> /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0
> /sys/devices/system/cpu/cpu0/cpuidle/state3/latency:100
> /sys/devices/system/cpu/cpu0/cpuidle/state3/name:C3
> /sys/devices/system/cpu/cpu0/cpuidle/state3/power:0
> /sys/devices/system/cpu/cpu0/cpuidle/state3/residency:200
> /sys/devices/system/cpu/cpu0/cpuidle/state3/time:5677316
> /sys/devices/system/cpu/cpu0/cpuidle/state3/usage:5920
> 
> intel_idle:
> /sys/devices/system/cpu/cpu0/cpuidle/state3/desc:MWAIT 0x30
> /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0
> /sys/devices/system/cpu/cpu0/cpuidle/state3/latency:100
> /sys/devices/system/cpu/cpu0/cpuidle/state3/name:C4-ATM
> /sys/devices/system/cpu/cpu0/cpuidle/state3/power:0
> /sys/devices/system/cpu/cpu0/cpuidle/state3/residency:400
> /sys/devices/system/cpu/cpu0/cpuidle/state3/time:7146705
> /sys/devices/system/cpu/cpu0/cpuidle/state3/usage:6826

Does the machine work, when you limit intel idle to C3, which would then
match acpi idle ?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1510637

On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > On Thu, Oct 27, 2016 at 08:48:57PM +0200, Thomas Gleixner wrote:
> > > What that old patch did, was:
> > > 
> > > 1) Make sure that the broadcast device is actually armed at resume.
> > > 
> > >    That might cause the HPET to resume proper.
> > > 
> > > 2) Force a max. 3 seconds rearm when the targeted expiry time is > than 10
> > >    seconds
> > > 
> > >    That might make sure that lower C-States are never entered.
> > 
> > Doh. I lost the other hunk somewhere. Let's try that again... And indeed
> > with the other hunk in tow the machine would appear to resume properly.
> 
> So it would be interesting whether that hunk in resume_broadcast() is
> sufficient.

So far it looks like the answer is yes.

Looks to be about 5 seconds slower than acpi-idle in resuming, but
I suppose that's not all that surprising ;)

>  
> > > What's the lowest C-State with acpi-idle and what's the lowest one with
> > > intel_idle?
> > 
> > acpi_idle
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/desc:ACPI FFH INTEL MWAIT 0x30
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/latency:100
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/name:C3
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/power:0
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/residency:200
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/time:5677316
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/usage:5920
> > 
> > intel_idle:
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/desc:MWAIT 0x30
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/latency:100
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/name:C4-ATM
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/power:0
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/residency:400
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/time:7146705
> > /sys/devices/system/cpu/cpu0/cpuidle/state3/usage:6826
> 
> Does the machine work, when you limit intel idle to C3, which would then
> match acpi idle ?

I'm pretty sure I had tested all of these, but I just double checked
to make sure. There's no C3 with intel_idle so I limited to C2, but
that did not help.

Isn't it possible that ACPI C3 is in fact C4? I thought ACPI C-states
are always numbered non-sparsely, and in this case ACPI C3 could be
anything from C3 to C11 (if the processor actually supported such
states obviously). Actually now that I look at the descriptions for
the states in sysfs, it says "MWAIT 0x30" for state3 on both drivers,
which I presume means it's in fact C4 for both.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1510661

[Multipart message — attachments visible in raw view] — view raw

On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> > So it would be interesting whether that hunk in resume_broadcast() is
> > sufficient.
> 
> So far it looks like the answer is yes.
> 
> Looks to be about 5 seconds slower than acpi-idle in resuming, but
> I suppose that's not all that surprising ;)

Well, set it to 1msec then. If that works reliably then we really can do
that unconditionally. There is no harm in firing a useless timer during
resume once.

> > Does the machine work, when you limit intel idle to C3, which would then
> > match acpi idle ?
> 
> I'm pretty sure I had tested all of these, but I just double checked
> to make sure. There's no C3 with intel_idle so I limited to C2, but
> that did not help.
> 
> Isn't it possible that ACPI C3 is in fact C4? I thought ACPI C-states
> are always numbered non-sparsely, and in this case ACPI C3 could be
> anything from C3 to C11 (if the processor actually supported such
> states obviously). Actually now that I look at the descriptions for
> the states in sysfs, it says "MWAIT 0x30" for state3 on both drivers,
> which I presume means it's in fact C4 for both.

Indeed.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1511200

On Thu, Oct 27, 2016 at 10:41:18PM +0200, Thomas Gleixner wrote:
> On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> > > So it would be interesting whether that hunk in resume_broadcast() is
> > > sufficient.
> > 
> > So far it looks like the answer is yes.
> > 
> > Looks to be about 5 seconds slower than acpi-idle in resuming, but
> > I suppose that's not all that surprising ;)
> 
> Well, set it to 1msec then. If that works reliably then we really can do
> that unconditionally. There is no harm in firing a useless timer during
> resume once.

I narrowed down the required timeout, and looks like 25ms is the
minimum that works. With 24ms I already started to have failures. So
maybe just bump it up by an order of magnitude to 250ms for some
safety margin?

In any case I think I'll leave the machine running S3 cycles over the
weekend with the 25ms timeout just to see if it will eventually fail.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1511364

[Multipart message — attachments visible in raw view] — view raw

On Fri, 28 Oct 2016, Ville Syrjälä wrote:
> On Thu, Oct 27, 2016 at 10:41:18PM +0200, Thomas Gleixner wrote:
> > On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > > On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> > > > So it would be interesting whether that hunk in resume_broadcast() is
> > > > sufficient.
> > > 
> > > So far it looks like the answer is yes.
> > > 
> > > Looks to be about 5 seconds slower than acpi-idle in resuming, but
> > > I suppose that's not all that surprising ;)
> > 
> > Well, set it to 1msec then. If that works reliably then we really can do
> > that unconditionally. There is no harm in firing a useless timer during
> > resume once.
> 
> I narrowed down the required timeout, and looks like 25ms is the
> minimum that works. With 24ms I already started to have failures. So
> maybe just bump it up by an order of magnitude to 250ms for some
> safety margin?

Sure, but what puzzles me is that we need a timeout that big. What happens
between broadcast_resume() and broadcast_resume() + 25ms?

IOW, what is the event/resume function which we need to bridge. We should
really try to track than down.

You might try to enable function tracing and do a tracing_off() when that
25ms timeout fires.

Something like 

	stop_trace = true;

in broadcast_resume() and then in the broadcast timer function:

	if (stop_trace) {
		stop_trace = false;
		tracing_off();
	}

Then when the machine is up read the trace, compress and upload it
somewhere or send it in private mail if it's not that big.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1513497

On Fri, Oct 28, 2016 at 08:58:41PM +0200, Thomas Gleixner wrote:
> On Fri, 28 Oct 2016, Ville Syrjälä wrote:
> > On Thu, Oct 27, 2016 at 10:41:18PM +0200, Thomas Gleixner wrote:
> > > On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > > > On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> > > > > So it would be interesting whether that hunk in resume_broadcast() is
> > > > > sufficient.
> > > > 
> > > > So far it looks like the answer is yes.
> > > > 
> > > > Looks to be about 5 seconds slower than acpi-idle in resuming, but
> > > > I suppose that's not all that surprising ;)
> > > 
> > > Well, set it to 1msec then. If that works reliably then we really can do
> > > that unconditionally. There is no harm in firing a useless timer during
> > > resume once.
> > 
> > I narrowed down the required timeout, and looks like 25ms is the
> > minimum that works. With 24ms I already started to have failures. So
> > maybe just bump it up by an order of magnitude to 250ms for some
> > safety margin?

I left the thing running for the weekend and it failed 26 out of 16057
times with the 25ms timeout. Looks like it takes ~5 minutes to resume
when it fails, but eventually it does come back.

> 
> Sure, but what puzzles me is that we need a timeout that big. What happens
> between broadcast_resume() and broadcast_resume() + 25ms?
> 
> IOW, what is the event/resume function which we need to bridge. We should
> really try to track than down.

My hunch would be that SMM trap in the DSDT/SSDT since that's where
things ended up last time I was tracing these resume problems. Though I
can't recall if that was just with acpi-idle or if intel_idle landed in
the same spot as well.

I guess I can try to repeat that test tomorrow, or I'll try your function
tracer method if the other thing fails.

> 
> You might try to enable function tracing and do a tracing_off() when that
> 25ms timeout fires.
> 
> Something like 
> 
> 	stop_trace = true;
> 
> in broadcast_resume() and then in the broadcast timer function:
> 
> 	if (stop_trace) {
> 		stop_trace = false;
> 		tracing_off();
> 	}
> 
> Then when the machine is up read the trace, compress and upload it
> somewhere or send it in private mail if it's not that big.
> 
> Thanks,
> 
> 	tglx


-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1516040

On Tue, Nov 01, 2016 at 10:47:37PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 28, 2016 at 08:58:41PM +0200, Thomas Gleixner wrote:
> > On Fri, 28 Oct 2016, Ville Syrjälä wrote:
> > > On Thu, Oct 27, 2016 at 10:41:18PM +0200, Thomas Gleixner wrote:
> > > > On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > > > > On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> > > > > > So it would be interesting whether that hunk in resume_broadcast() is
> > > > > > sufficient.
> > > > > 
> > > > > So far it looks like the answer is yes.
> > > > > 
> > > > > Looks to be about 5 seconds slower than acpi-idle in resuming, but
> > > > > I suppose that's not all that surprising ;)
> > > > 
> > > > Well, set it to 1msec then. If that works reliably then we really can do
> > > > that unconditionally. There is no harm in firing a useless timer during
> > > > resume once.
> > > 
> > > I narrowed down the required timeout, and looks like 25ms is the
> > > minimum that works. With 24ms I already started to have failures. So
> > > maybe just bump it up by an order of magnitude to 250ms for some
> > > safety margin?
> 
> I left the thing running for the weekend and it failed 26 out of 16057
> times with the 25ms timeout. Looks like it takes ~5 minutes to resume
> when it fails, but eventually it does come back.
> 
> > 
> > Sure, but what puzzles me is that we need a timeout that big. What happens
> > between broadcast_resume() and broadcast_resume() + 25ms?
> > 
> > IOW, what is the event/resume function which we need to bridge. We should
> > really try to track than down.
> 
> My hunch would be that SMM trap in the DSDT/SSDT since that's where
> things ended up last time I was tracing these resume problems. Though I
> can't recall if that was just with acpi-idle or if intel_idle landed in
> the same spot as well.
> 
> I guess I can try to repeat that test tomorrow, or I'll try your function
> tracer method if the other thing fails.

I didn't manage to find a lot of time to play around with this, but it
definitely looks like the SMM trap is the problem here. I repeated my
pm_trace experiemnts and when it gets stuck it is trying to execute the
_WAK ACPI method which is where the SMM trap happens.

Maybe the SMM code was written with the expectation of a periodic tick
or something like that?

> 
> > 
> > You might try to enable function tracing and do a tracing_off() when that
> > 25ms timeout fires.
> > 
> > Something like 
> > 
> > 	stop_trace = true;
> > 
> > in broadcast_resume() and then in the broadcast timer function:
> > 
> > 	if (stop_trace) {
> > 		stop_trace = false;
> > 		tracing_off();
> > 	}
> > 
> > Then when the machine is up read the trace, compress and upload it
> > somewhere or send it in private mail if it's not that big.
> > 
> > Thanks,
> > 
> > 	tglx
> 
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1516163

[Multipart message — attachments visible in raw view] — view raw

On Mon, 7 Nov 2016, Ville Syrjälä wrote:
> I didn't manage to find a lot of time to play around with this, but it
> definitely looks like the SMM trap is the problem here. I repeated my
> pm_trace experiemnts and when it gets stuck it is trying to execute the
> _WAK ACPI method which is where the SMM trap happens.
> 
> Maybe the SMM code was written with the expectation of a periodic tick
> or something like that?

Can you try the untested hack below, please? It should confirm that.

Thanks,

	tglx

8<---------------
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -269,6 +269,17 @@ acpi_status acpi_hw_legacy_wake_prep(u8
 	return_ACPI_STATUS(status);
 }
 
+static const ktime_t time10ms = { .tv64 = 10 * NSEC_PER_MSEC };
+
+static enum hrtimer_restart acpi_hw_legacy_tmr(struct hrtimer *tmr)
+{
+	hrtimer_forward_now(tmr, time10ms);
+
+	return HRTIMER_RESTART;
+}
+
+
+
 /*******************************************************************************
  *
  * FUNCTION:    acpi_hw_legacy_wake
@@ -284,6 +295,7 @@ acpi_status acpi_hw_legacy_wake_prep(u8
 
 acpi_status acpi_hw_legacy_wake(u8 sleep_state)
 {
+	struct hrtimer timer;
 	acpi_status status;
 
 	ACPI_FUNCTION_TRACE(hw_legacy_wake);
@@ -311,12 +323,18 @@ acpi_status acpi_hw_legacy_wake(u8 sleep
 		return_ACPI_STATUS(status);
 	}
 
+	hrtimer_init_on_stack(&timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	timer.function = acpi_hw_legacy_tmr;
+	hrtimer_start(&timer, time10ms, HRTIMER_MODE_REL);
+
 	/*
 	 * Now we can execute _WAK, etc. Some machines require that the GPEs
 	 * are enabled before the wake methods are executed.
 	 */
 	acpi_hw_execute_sleep_method(METHOD_PATHNAME__WAK, sleep_state);
 
+	hrtimer_cancel(&timer);
+
 	/*
 	 * Some BIOS code assumes that WAK_STS will be cleared on resume
 	 * and use it to determine whether the system is rebooting or

[toc] | [prev] | [next] | [standalone]

#1516310

On Mon, Nov 07, 2016 at 02:07:43PM +0100, Thomas Gleixner wrote:
> On Mon, 7 Nov 2016, Ville Syrjälä wrote:
> > I didn't manage to find a lot of time to play around with this, but it
> > definitely looks like the SMM trap is the problem here. I repeated my
> > pm_trace experiemnts and when it gets stuck it is trying to execute the
> > _WAK ACPI method which is where the SMM trap happens.
> > 
> > Maybe the SMM code was written with the expectation of a periodic tick
> > or something like that?
> 
> Can you try the untested hack below, please? It should confirm that.
> 
> Thanks,
> 
> 	tglx
> 
> 8<---------------
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -269,6 +269,17 @@ acpi_status acpi_hw_legacy_wake_prep(u8
>  	return_ACPI_STATUS(status);
>  }
>  
> +static const ktime_t time10ms = { .tv64 = 10 * NSEC_PER_MSEC };
> +
> +static enum hrtimer_restart acpi_hw_legacy_tmr(struct hrtimer *tmr)
> +{
> +	hrtimer_forward_now(tmr, time10ms);
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +
> +
>  /*******************************************************************************
>   *
>   * FUNCTION:    acpi_hw_legacy_wake
> @@ -284,6 +295,7 @@ acpi_status acpi_hw_legacy_wake_prep(u8
>  
>  acpi_status acpi_hw_legacy_wake(u8 sleep_state)
>  {
> +	struct hrtimer timer;
>  	acpi_status status;
>  
>  	ACPI_FUNCTION_TRACE(hw_legacy_wake);
> @@ -311,12 +323,18 @@ acpi_status acpi_hw_legacy_wake(u8 sleep
>  		return_ACPI_STATUS(status);
>  	}
>  
> +	hrtimer_init_on_stack(&timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	timer.function = acpi_hw_legacy_tmr;
> +	hrtimer_start(&timer, time10ms, HRTIMER_MODE_REL);
> +
>  	/*
>  	 * Now we can execute _WAK, etc. Some machines require that the GPEs
>  	 * are enabled before the wake methods are executed.
>  	 */
>  	acpi_hw_execute_sleep_method(METHOD_PATHNAME__WAK, sleep_state);
>  
> +	hrtimer_cancel(&timer);
> +
>  	/*
>  	 * Some BIOS code assumes that WAK_STS will be cleared on resume
>  	 * and use it to determine whether the system is rebooting or

Doesn't really seem to help. I did get a few random successes, but
mostly it just fails.

diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index 3c9c10bd49e9..950319b619f1 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -44,6 +44,8 @@
 
 #include <acpi/acpi.h>
 #include <linux/acpi.h>
+#include <linux/pm-trace.h>
+#include <linux/delay.h>
 #include "accommon.h"
 
 #define _COMPONENT          ACPI_HARDWARE
@@ -275,6 +277,8 @@ static enum hrtimer_restart acpi_hw_legacy_tmr(struct hrtimer *tmr)
 {
 	hrtimer_forward_now(tmr, time10ms);
 
+	TRACE_RESUME(0);
+
 	return HRTIMER_RESTART;
 }
 
@@ -327,14 +331,17 @@ acpi_status acpi_hw_legacy_wake(u8 sleep_state)
 	timer.function = acpi_hw_legacy_tmr;
 	hrtimer_start(&timer, time10ms, HRTIMER_MODE_REL);
 
+	TRACE_RESUME(0);
 	/*
 	 * Now we can execute _WAK, etc. Some machines require that the GPEs
 	 * are enabled before the wake methods are executed.
 	 */
 	acpi_hw_execute_sleep_method(METHOD_PATHNAME__WAK, sleep_state);
+	TRACE_RESUME(0);
 
 	hrtimer_cancel(&timer);
 
+	TRACE_RESUME(0);
 	/*
 	 * Some BIOS code assumes that WAK_STS will be cleared on resume
 	 * and use it to determine whether the system is rebooting or

Tossing that on top shows the trace before the _WAK being the last one
executed. Adding an msleep() before the _WAK does make the trace from
the timer handler show up so at least the timer seems to be ticking
up until some point.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1517750

On Wed, Nov 02, 2016 at 04:47:37AM +0800, Ville Syrjälä wrote:
> On Fri, Oct 28, 2016 at 08:58:41PM +0200, Thomas Gleixner wrote:
> > On Fri, 28 Oct 2016, Ville Syrjälä wrote:
> > > On Thu, Oct 27, 2016 at 10:41:18PM +0200, Thomas Gleixner wrote:
> > > > On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > > > > On Thu, Oct 27, 2016 at 09:25:05PM +0200, Thomas Gleixner wrote:
> > > > > > So it would be interesting whether that hunk in resume_broadcast() is
> > > > > > sufficient.
> > > > > 
> > > > > So far it looks like the answer is yes.
> > > > > 
> > > > > Looks to be about 5 seconds slower than acpi-idle in resuming, but
> > > > > I suppose that's not all that surprising ;)
> > > > 
> > > > Well, set it to 1msec then. If that works reliably then we really can do
> > > > that unconditionally. There is no harm in firing a useless timer during
> > > > resume once.
> > > 
> > > I narrowed down the required timeout, and looks like 25ms is the
> > > minimum that works. With 24ms I already started to have failures. So
> > > maybe just bump it up by an order of magnitude to 250ms for some
> > > safety margin?
> 
> I left the thing running for the weekend and it failed 26 out of 16057
> times with the 25ms timeout. Looks like it takes ~5 minutes to resume
> when it fails, but eventually it does come back.
> 

Just came back from a travel. Yes, the 5 minutes delay may be due to the
expiration of the HPET timer, counting from 0 to 0xffffffff for a 13M
frequencey HPET takes about 300 seconds. After resume, it seems nobody
arms it so my old patch forces to arm one event. 

Thanks,
Feng

[toc] | [prev] | [next] | [standalone]

#1517784

On Tue, Nov 8, 2016 at 7:54 PM, Feng Tang <feng.tang@intel.com> wrote:
> On Wed, Nov 02, 2016 at 04:47:37AM +0800, Ville Syrjälä wrote:
>>
>> I left the thing running for the weekend and it failed 26 out of 16057
>> times with the 25ms timeout. Looks like it takes ~5 minutes to resume
>> when it fails, but eventually it does come back.
>
> Just came back from a travel. Yes, the 5 minutes delay may be due to the
> expiration of the HPET timer, counting from 0 to 0xffffffff for a 13M
> frequencey HPET takes about 300 seconds. After resume, it seems nobody
> arms it so my old patch forces to arm one event.

Ville, what happens if you disable HPET? Can you force the TSC with
"clocksource=tsc" or "tsc=reliable". Does resume work reliably then?

Or is this one of the CPU's where tsc just doesn't work?

              Linus

[toc] | [prev] | [next] | [standalone]

#1510597

On Thu, Oct 27, 2016 at 08:48:57PM +0200, Thomas Gleixner wrote:
> On Thu, 27 Oct 2016, Ville Syrjälä wrote:
> > On Tue, Aug 09, 2016 at 08:20:57PM +0300, Ville Syrjälä wrote:
> > > On Thu, Jul 14, 2016 at 04:29:42PM +0800, Feng Tang wrote:
> > > > if you only want it to work, you can try an old patch
> > > > https://bugzilla.kernel.org/attachment.cgi?id=76071 from a similar bug
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=41932
> > > > 
> > > > Alistair Buxton confirmed it work for 3.18 at least
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=107151#c16
> > > 
> > > That patch is a bit too ripe by now. Would need a fresh squeezed one.
> > 
> > Since no one else bothered to refresh the patch I did it myself:
> > 
> > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> > index f6aae7977824..d73d094a8972 100644
> > --- a/kernel/time/tick-broadcast.c
> > +++ b/kernel/time/tick-broadcast.c
> > @@ -657,8 +657,16 @@ static void tick_handle_oneshot_broadcast(struct clock_event_device *dev)
> >  	 * - There are pending events on sleeping CPUs which were not
> >  	 * in the event mask
> >  	 */
> > -	if (next_event.tv64 != KTIME_MAX)
> > +	if (next_event.tv64 != KTIME_MAX) {
> > +		s64 delta = next_event.tv64 - now.tv64;
> > +
> > +		if (delta >= 10000000000) {
> > +			printk(KERN_CRIT "%s(): The delta is big: %lld\n", __func__, delta);
> > +			next_event.tv64 = now.tv64 + 3000000000;
> > +		}
> > +
> >  		tick_broadcast_set_event(dev, next_cpu, next_event);
> > +	}
> >  
> >  	raw_spin_unlock(&tick_broadcast_lock);
> > 
> > Unfortunately it doesn't do anything for me.
> 
> And I'm not surprised, because the original patch forced a 5 seconds event
> in the broadcast device on resume, aside of limiting the reprogramming.
> 
> What that old patch did, was:
> 
> 1) Make sure that the broadcast device is actually armed at resume.
> 
>    That might cause the HPET to resume proper.
> 
> 2) Force a max. 3 seconds rearm when the targeted expiry time is > than 10
>    seconds
> 
>    That might make sure that lower C-States are never entered.

Doh. I lost the other hunk somewhere. Let's try that again... And indeed
with the other hunk in tow the machine would appear to resume properly.

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index f6aae7977824..e2173aeeb00c 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -507,8 +507,12 @@ void tick_resume_broadcast(void)
 				tick_broadcast_start_periodic(bc);
 			break;
 		case TICKDEV_MODE_ONESHOT:
-			if (!cpumask_empty(tick_broadcast_mask))
+			if (!cpumask_empty(tick_broadcast_mask)) {
 				tick_resume_broadcast_oneshot(bc);
+				clockevents_program_event(bc,
+							  ktime_add_ns(ktime_get(), 5 * NSEC_PER_SEC),
+							  1);
+			}
 			break;
 		}
 	}
@@ -657,8 +661,16 @@ static void tick_handle_oneshot_broadcast(struct clock_event_device *dev)
 	 * - There are pending events on sleeping CPUs which were not
 	 * in the event mask
 	 */
-	if (next_event.tv64 != KTIME_MAX)
+	if (next_event.tv64 != KTIME_MAX) {
+		s64 delta = next_event.tv64 - now.tv64;
+
+		if (delta >= 10000000000) {
+			printk(KERN_CRIT "%s(): The delta is big: %lld\n", __func__, delta);
+			next_event.tv64 = now.tv64 + 3000000000;
+		}
+
 		tick_broadcast_set_event(dev, next_cpu, next_event);
+	}
 
 	raw_spin_unlock(&tick_broadcast_lock);

>  
> > The fortunate thing is that acpi-idle has magically been fixed in the
> > meantime, so I can at least go back to using that one and have working
> > S3.
> 
> What's the lowest C-State with acpi-idle and what's the lowest one with
> intel_idle?

acpi_idle
/sys/devices/system/cpu/cpu0/cpuidle/state3/desc:ACPI FFH INTEL MWAIT 0x30
/sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0
/sys/devices/system/cpu/cpu0/cpuidle/state3/latency:100
/sys/devices/system/cpu/cpu0/cpuidle/state3/name:C3
/sys/devices/system/cpu/cpu0/cpuidle/state3/power:0
/sys/devices/system/cpu/cpu0/cpuidle/state3/residency:200
/sys/devices/system/cpu/cpu0/cpuidle/state3/time:5677316
/sys/devices/system/cpu/cpu0/cpuidle/state3/usage:5920

intel_idle:
/sys/devices/system/cpu/cpu0/cpuidle/state3/desc:MWAIT 0x30
/sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0
/sys/devices/system/cpu/cpu0/cpuidle/state3/latency:100
/sys/devices/system/cpu/cpu0/cpuidle/state3/name:C4-ATM
/sys/devices/system/cpu/cpu0/cpuidle/state3/power:0
/sys/devices/system/cpu/cpu0/cpuidle/state3/residency:400
/sys/devices/system/cpu/cpu0/cpuidle/state3/time:7146705
/sys/devices/system/cpu/cpu0/cpuidle/state3/usage:6826

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web