devfreq: Suspend all devices on system shutdown

Message ID 20190125135403.10228-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • devfreq: Suspend all devices on system shutdown
Related show

Commit Message

Marek Szyprowski Jan. 25, 2019, 1:54 p.m.
This way devfreq core ensures that all its devices will be set to safe
operation points before reboot operation. There are board on which some
aggressive power saving operation points are behind the capabilities of
the bootloader to properly reset the hardware and boot the board. This
way one can avoid board crash early after reboot.

Similar pattern is used in CPUfreq subsystem.

Reported-by: Markus Reichl <m.reichl@fivetechno.de>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/devfreq/devfreq.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.17.1

Comments

Marek Szyprowski Jan. 28, 2019, 11:57 a.m. | #1
Hi MyungJoo,

On 2019-01-28 09:05, MyungJoo Ham wrote:
>> This way devfreq core ensures that all its devices will be set to safe

>> operation points before reboot operation. There are board on which some

>> aggressive power saving operation points are behind the capabilities of

>> the bootloader to properly reset the hardware and boot the board. This

>> way one can avoid board crash early after reboot.

>>

>> Similar pattern is used in CPUfreq subsystem.

>>

>> Reported-by: Markus Reichl <m.reichl@fivetechno.de>

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

> You are invoking ALL devfreq suspend callbacks at shutdown

> with this commit.

>

> Can you make it invoke only devices explicitly saying their needs

> to handle "SHUTDOWN" event?

>

> For example, we can add a flag at struct devfreq_dev_profile:

> "uint32_t requirement"

> , where

> 0x1: need to operate at the initial frequency for suspend

> 0x2: need to operate at the initial frequency for shutdown

> 0x4: it forgets its status at resume, reconfigure frequency at resume.

> (or reverse 0x1's semantics for the backward compatibility)


Frankly speaking this looks like an over-engineering. Switching to safe
frequency during reboot shouldn't have any negative side-effects. IMHO
such flags can be always added later, once there will be a real use case
for them.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Pavel Machek March 6, 2019, 11:10 p.m. | #2
On Fri 2019-01-25 14:54:03, Marek Szyprowski wrote:
> This way devfreq core ensures that all its devices will be set to safe

> operation points before reboot operation. There are board on which some

> aggressive power saving operation points are behind the capabilities of

> the bootloader to properly reset the hardware and boot the board. This

> way one can avoid board crash early after reboot.

> 

> Similar pattern is used in CPUfreq subsystem.


This looks somehow dangerous to me. I guess this will break someone's
shutdown, and on battery-powered devices, that's quite bad thing to
do.

Could we explicitely do it only for devices that need it?
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Patch

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0ae3de76833b..f6aba8344c56 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -23,6 +23,7 @@ 
 #include <linux/devfreq.h>
 #include <linux/workqueue.h>
 #include <linux/platform_device.h>
+#include <linux/syscore_ops.h>
 #include <linux/list.h>
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
@@ -1422,6 +1423,10 @@  static struct attribute *devfreq_attrs[] = {
 };
 ATTRIBUTE_GROUPS(devfreq);
 
+static struct syscore_ops devfreq_syscore_ops = {
+	.shutdown = devfreq_suspend,
+};
+
 static int __init devfreq_init(void)
 {
 	devfreq_class = class_create(THIS_MODULE, "devfreq");
@@ -1438,6 +1443,8 @@  static int __init devfreq_init(void)
 	}
 	devfreq_class->dev_groups = devfreq_groups;
 
+	register_syscore_ops(&devfreq_syscore_ops);
+
 	return 0;
 }
 subsys_initcall(devfreq_init);