PM/Trace: get rid of synchronous resume limit during PM trace

PATCH Applied Fu, Zhonghui 4 participants 9 messages Jan 26, 2015 – Jan 30, 2015

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

Fu, Zhonghui

author

2015-01-26 05:07 UTC #0 lore raw

PATCH 1/1 PM/Trace: get rid of synchronous resume limit during PM trace

Quoted 1 line

From f9c841d1f943d81b5ab0aac7483e794a7f966296 Mon Sep 17 00:00:00 2001

From: Zhonghui Fu <[email protected]>
Date: Mon, 26 Jan 2015 11:27:08 +0800
Subject: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace

There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.

Signed-off-by Zhonghui Fu <[email protected]>

---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)

drivers/base/power/main.c +1 -2

index 9717d5f..5df148b 100644
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
 static void async_resume_noirq(void *data, async_cookie_t cookie)

include/linux/resume-trace.h +0 -7

index f31db23..fd0866e 100644
@@ -7,11 +7,6 @@
 
 extern int pm_trace_enabled;
 
-static inline int pm_trace_is_enabled(void)
-{
-       return pm_trace_enabled;
-}
-
 struct device;
 extern void set_trace_device(struct device *);
 extern void generate_resume_trace(const void *tracedata, unsigned int user);

@@ -24,8 +19,6 @@ extern int show_trace_dev_match(char *buf, size_t size);
 
 #else
 
-static inline int pm_trace_is_enabled(void) { return 0; }
-
 #define TRACE_DEVICE(dev) do { } while (0)
 #define TRACE_RESUME(dev) do { } while (0)

-- 1.7.1

Fu, Zhonghui

author

2015-01-26 07:59 UTC #1 lore raw

This is a incorrect patch , please ignore it.

Thanks,
Zhonghui

Quoted 58 lines · Zhonghui

From f9c841d1f943d81b5ab0aac7483e794a7f966296 Mon Sep 17 00:00:00 2001
From: Zhonghui Fu <[email protected]>
Date: Mon, 26 Jan 2015 11:27:08 +0800
Subject: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace
There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)
drivers/base/power/main.c +1 -2
index 9717d5f..5df148b 100644
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
 static void async_resume_noirq(void *data, async_cookie_t cookie)
include/linux/resume-trace.h +0 -8
index f31db23..fd0866e 100644
@@ -7,11 +7,6 @@
 
 extern int pm_trace_enabled;
 
-static inline int pm_trace_is_enabled(void)
-{
-       return pm_trace_enabled;
-}
-
 struct device;
 extern void set_trace_device(struct device *);
 extern void generate_resume_trace(const void *tracedata, unsigned int user);
@@ -24,8 +19,6 @@ extern int show_trace_dev_match(char *buf, size_t size);
 
 #else
 
-static inline int pm_trace_is_enabled(void) { return 0; }
-
 #define TRACE_DEVICE(dev) do { } while (0)
 #define TRACE_RESUME(dev) do { } while (0)
 
-- 1.7.1

Rafael J. Wysocki

2015-01-26 13:21 UTC #2 lore raw

Quoted 2 lines · Zhonghui

This is a incorrect patch , please ignore it.

Of course it is incorrect.

If it "fixes" anything for you, this means you need to reorder devices in dpm_list on the given system.

Quoted 60 lines · Zhonghui

Quoted 58 lines
From f9c841d1f943d81b5ab0aac7483e794a7f966296 Mon Sep 17 00:00:00 2001
From: Zhonghui Fu <[email protected]>
Date: Mon, 26 Jan 2015 11:27:08 +0800
Subject: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace
There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)
drivers/base/power/main.c +1 -2
index 9717d5f..5df148b 100644
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
 static void async_resume_noirq(void *data, async_cookie_t cookie)
include/linux/resume-trace.h +0 -8
index f31db23..fd0866e 100644
@@ -7,11 +7,6 @@
 
 extern int pm_trace_enabled;
 
-static inline int pm_trace_is_enabled(void)
-{
-       return pm_trace_enabled;
-}
-
 struct device;
 extern void set_trace_device(struct device *);
 extern void generate_resume_trace(const void *tracedata, unsigned int user);
@@ -24,8 +19,6 @@ extern int show_trace_dev_match(char *buf, size_t size);
 
 #else
 
-static inline int pm_trace_is_enabled(void) { return 0; }
-
 #define TRACE_DEVICE(dev) do { } while (0)
 #define TRACE_RESUME(dev) do { } while (0)
 
-- 1.7.1

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Pavel Machek

2015-01-26 10:23 UTC #3 lore raw

Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)

In reply to Fu, Zhonghui

Quoted 22 lines · Zhonghui

Quoted 1 line
From f9c841d1f943d81b5ab0aac7483e794a7f966296 Mon Sep 17 00:00:00 2001
From: Zhonghui Fu <[email protected]>
Date: Mon, 26 Jan 2015 11:27:08 +0800
Subject: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace
There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)
drivers/base/power/main.c +0 -0
index 9717d5f..5df148b 100644

drivers/base/power/main.c static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn · line 522

Quoted 8 lines

drivers/base/power/main.c +1 -2

@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }

Actually... whoever did the original patch was evil person. Changing behaviour when tracing is requested is evil, evil, evil. Git blame tells me

Signed-off-by Chuansheng Liu <[email protected]>

went to the dark side.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Liu, Chuansheng

2015-01-26 10:39 UTC #4 lore raw

RE: Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)

Hello Pavel,

Quoted 48 lines

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Monday, January 26, 2015 6:24 PM
To: Fu, Zhonghui
Cc: Rafael J. Wysocki; Brown, Len; Greg Kroah-Hartman; [email protected]; [email protected]; Liu, Chuansheng
Subject: Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)
Quoted 1 line · Zhonghui
Quoted 1 line
From f9c841d1f943d81b5ab0aac7483e794a7f966296 Mon Sep 17 00:00:00
2001
Quoted 3 lines
From: Zhonghui Fu <[email protected]>
Date: Mon, 26 Jan 2015 11:27:08 +0800
Subject: [PATCH] PM/Trace: get rid of synchronous resume limit during PM
trace
drivers/base/power/main.c static int device_resume_noirq(struct device *dev,
Quoted 18 lines
There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)
drivers/base/power/main.c +0 -0
index 9717d5f..5df148b 100644
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev,
pm_message_t state, bool asyn
drivers/base/power/main.c static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn · line 522
Quoted 8 lines
drivers/base/power/main.c +1 -2
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
Actually... whoever did the original patch was evil person. Changing behaviour when tracing is requested is evil, evil, evil. Git blame tells me

Quoted 3 lines

Signed-off-by Chuansheng Liu <[email protected]>
went to the dark side.

Although I didn't get where is something wrong, but the is_async() is not created by my commit, it is from commit (PM: Start asynchronous resume threads upfront), I just moved it ahead.

And like other phases, I added it into resum/suspend_noirq()...

Pavel Machek

2015-01-26 11:05 UTC #5 lore raw

Re: Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)

Quoted 33 lines · Chuansheng

Hello Pavel,
Quoted 31 lines
drivers/base/power/main.c static int device_resume_noirq(struct device *dev,
Quoted 17 lines
There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)
drivers/base/power/main.c +0 -0
index 9717d5f..5df148b 100644
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev,
pm_message_t state, bool asyn
drivers/base/power/main.c static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn · line 522
Quoted 8 lines
drivers/base/power/main.c +1 -2
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
Actually... whoever did the original patch was evil person. Changing behaviour when tracing is requested is evil, evil, evil. Git blame tells me

Quoted 8 lines

Quoted 3 lines
Signed-off-by Chuansheng Liu <[email protected]>
went to the dark side.
Although I didn't get where is something wrong, but the is_async() is not created by my commit, it is from commit (PM: Start asynchronous resume threads upfront), I just moved it ahead.
And like other phases, I added it into resum/suspend_noirq()...

I see, blame blamed wrong person. It looks like Rafael is evil:

commit 97df8c12995c5bac73e3bfeea4c5be155c1f4401

Author: Rafael J. Wysocki <[email protected]>
Date:   Sat Jan 23 22:25:31 2010 +0100

PM: Start asynchronous resume threads upfront

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Rafael J. Wysocki

2015-01-26 13:18 UTC #6 lore raw

Re: Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)

Quoted 43 lines · Pavel Machek

Quoted 33 lines · Chuansheng
Hello Pavel,
Quoted 31 lines
drivers/base/power/main.c static int device_resume_noirq(struct device *dev,
Quoted 17 lines
There are some kind of dependency between devices in some hardware platforms. So, asynchronous resuming devices may hang system due to wrong resume order. As a result, should not fore synchronously resuming devices during tracing PM events.
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/base/power/main.c    |    3 +--
 include/linux/resume-trace.h |    7 -------
 2 files changed, 1 insertions(+), 9 deletions(-)
drivers/base/power/main.c +0 -0
index 9717d5f..5df148b 100644
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev,
pm_message_t state, bool asyn
drivers/base/power/main.c static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn · line 522
Quoted 8 lines
drivers/base/power/main.c +1 -2
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
Actually... whoever did the original patch was evil person. Changing behaviour when tracing is requested is evil, evil, evil. Git blame tells me
Quoted 8 lines
Quoted 3 lines
Signed-off-by Chuansheng Liu <[email protected]>
went to the dark side.
Although I didn't get where is something wrong, but the is_async() is not created by my commit, it is from commit (PM: Start asynchronous resume threads upfront), I just moved it ahead.
And like other phases, I added it into resum/suspend_noirq()...

Quoted 7 lines

I see, blame blamed wrong person. It looks like Rafael is evil:
commit 97df8c12995c5bac73e3bfeea4c5be155c1f4401
Author: Rafael J. Wysocki <[email protected]>
Date:   Sat Jan 23 22:25:31 2010 +0100
PM: Start asynchronous resume threads upfront

This only means we won't use asyc suspend/resume at all when the RTC-based resume debug is enabled, because it wouldn't make sense (the RTC-based debug requires strict ordering of callbacks between devices or we may find that device A hanged the resume while actually device B that was running in parallel with A did that).

And I shouldn't even need to explain this ... Sad.

Rafael

Pavel Machek

2015-01-26 13:43 UTC #7 lore raw

Re: Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)

Document pm_tracing actually affecting suspend in non-trivial way.

Signed-off-by Pavel Machek <[email protected]>

---

Quoted 35 lines · Rafael J. Wysocki

Quoted 26 lines · Pavel Machek
Quoted 15 lines · Chuansheng
Quoted 15 lines
Review static int device_resume_noirq(struct device *dev,
Quoted 1 line
(quoted diff) +0 -0
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev,
pm_message_t state, bool asyn
Review static int device_resume_noirq(struct device *dev, · line 521
Quoted 8 lines
(quoted diff) +1 -2
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
Actually... whoever did the original patch was evil person. Changing behaviour when tracing is requested is evil, evil, evil. Git blame tells me
Quoted 8 lines
Quoted 3 lines
Signed-off-by Chuansheng Liu <[email protected]>
went to the dark side.
Although I didn't get where is something wrong, but the is_async() is not created by my commit, it is from commit (PM: Start asynchronous resume threads upfront), I just moved it ahead.
And like other phases, I added it into resum/suspend_noirq()...
Quoted 7 lines
I see, blame blamed wrong person. It looks like Rafael is evil:
commit 97df8c12995c5bac73e3bfeea4c5be155c1f4401
Author: Rafael J. Wysocki <[email protected]>
Date:   Sat Jan 23 22:25:31 2010 +0100
PM: Start asynchronous resume threads upfront

Quoted 7 lines

This only means we won't use asyc suspend/resume at all when the RTC-based resume debug is enabled, because it wouldn't make sense (the RTC-based debug requires strict ordering of callbacks between devices or we may find that device A hanged the resume while actually device B that was running in parallel with A did that).
And I shouldn't even need to explain this ... Sad.

Well, I forgot that pm_trace_is_enabled() is the simple, RTC based one, and believe it would be worth a comment...

Documentation/power/s2ram.txt +4 -0

index 1bdfa04..4685aee 100644
@@ -69,6 +69,10 @@ Reason for this is that the RTC is the only reliably available piece of
 hardware during resume operations where a value can be set that will
 survive a reboot.
 
+pm_trace is not compatible with asynchronous suspend, so it turns
+asynchronous suspend off (which may work around timing or
+ordering-sensitive bugs).
+
 Consequence is that after a resume (even if it is successful) your system
 clock will have a value corresponding to the magic number instead of the
 correct date/time! It is therefore advisable to use a program like ntp-date

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Rafael J. Wysocki

Applied

2015-01-30 00:30 UTC #8 lore raw

Re: Change behaviour when tracing ... nasty trap (was Re: [PATCH] PM/Trace: get rid of synchronous resume limit during PM trace)

Quoted 49 lines · Pavel Machek

Document pm_tracing actually affecting suspend in non-trivial way.
Signed-off-by Pavel Machek <[email protected]>
---
Quoted 41 lines · Rafael J. Wysocki
Quoted 26 lines · Pavel Machek
Quoted 15 lines · Chuansheng
Quoted 15 lines
Review static int device_resume_noirq(struct device *dev,
Quoted 1 line
(quoted diff) +0 -0
@@ -517,8 +517,7 @@ static int device_resume_noirq(struct device *dev,
pm_message_t state, bool asyn
Review static int device_resume_noirq(struct device *dev, · line 521
Quoted 8 lines
(quoted diff) +1 -2
 static bool is_async(struct device *dev)
 {
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
+	return dev->power.async_suspend && pm_async_enabled;
 }
 
Actually... whoever did the original patch was evil person. Changing behaviour when tracing is requested is evil, evil, evil. Git blame tells me
Quoted 8 lines
Quoted 3 lines
Signed-off-by Chuansheng Liu <[email protected]>
went to the dark side.
Although I didn't get where is something wrong, but the is_async() is not created by my commit, it is from commit (PM: Start asynchronous resume threads upfront), I just moved it ahead.
And like other phases, I added it into resum/suspend_noirq()...
Quoted 7 lines
I see, blame blamed wrong person. It looks like Rafael is evil:
commit 97df8c12995c5bac73e3bfeea4c5be155c1f4401
Author: Rafael J. Wysocki <[email protected]>
Date:   Sat Jan 23 22:25:31 2010 +0100
PM: Start asynchronous resume threads upfront
This only means we won't use asyc suspend/resume at all when the RTC-based resume debug is enabled, because it wouldn't make sense (the RTC-based debug requires strict ordering of callbacks between devices or we may find that device A hanged the resume while actually device B that was running in parallel with A did that).

Quoted 4 lines

Quoted 1 line
And I shouldn't even need to explain this ... Sad.
Well, I forgot that pm_trace_is_enabled() is the simple, RTC based one, and believe it would be worth a comment...

A comment won't hurt. :-)

Applied, thanks!

Quoted 18 lines

Documentation/power/s2ram.txt +4 -0

index 1bdfa04..4685aee 100644
@@ -69,6 +69,10 @@ Reason for this is that the RTC is the only reliably available piece of
 hardware during resume operations where a value can be set that will
 survive a reboot.
 
+pm_trace is not compatible with asynchronous suspend, so it turns
+asynchronous suspend off (which may work around timing or
+ordering-sensitive bugs).
+
 Consequence is that after a resume (even if it is successful) your system
 clock will have a value corresponding to the magic number instead of the
 correct date/time! It is therefore advisable to use a program like ntp-date

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.