* Re: [PATCH] devfreq: Suspend all devices on system shutdown
2019-01-25 13:54 ` [PATCH] devfreq: Suspend all devices on system shutdown Marek Szyprowski
@ 2019-01-28 1:17 ` Chanwoo Choi
2019-01-28 8:05 ` MyungJoo Ham
2019-03-06 23:10 ` Pavel Machek
2 siblings, 0 replies; 6+ messages in thread
From: Chanwoo Choi @ 2019-01-28 1:17 UTC (permalink / raw)
To: Marek Szyprowski, linux-kernel, linux-pm, linux-samsung-soc
Cc: MyungJoo Ham, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Lukasz Luba, Markus Reichl
Hi,
On 19. 1. 25. 오후 10:54, 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.
>
> 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(+)
>
> 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);
>
Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] devfreq: Suspend all devices on system shutdown
2019-01-25 13:54 ` [PATCH] devfreq: Suspend all devices on system shutdown Marek Szyprowski
2019-01-28 1:17 ` Chanwoo Choi
@ 2019-01-28 8:05 ` MyungJoo Ham
2019-01-28 11:57 ` Marek Szyprowski
2019-03-06 23:10 ` Pavel Machek
2 siblings, 1 reply; 6+ messages in thread
From: MyungJoo Ham @ 2019-01-28 8:05 UTC (permalink / raw)
To: Marek Szyprowski, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org, linux-samsung-soc@vger.kernel.org
Cc: Chanwoo Choi, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Lukasz Luba, Markus Reichl
>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)
...
Cheers,
MyungJoo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] devfreq: Suspend all devices on system shutdown
2019-01-28 8:05 ` MyungJoo Ham
@ 2019-01-28 11:57 ` Marek Szyprowski
0 siblings, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2019-01-28 11:57 UTC (permalink / raw)
To: myungjoo.ham, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org, linux-samsung-soc@vger.kernel.org
Cc: Chanwoo Choi, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Lukasz Luba, Markus Reichl
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] devfreq: Suspend all devices on system shutdown
2019-01-25 13:54 ` [PATCH] devfreq: Suspend all devices on system shutdown Marek Szyprowski
2019-01-28 1:17 ` Chanwoo Choi
2019-01-28 8:05 ` MyungJoo Ham
@ 2019-03-06 23:10 ` Pavel Machek
2019-03-07 1:27 ` Chanwoo Choi
2 siblings, 1 reply; 6+ messages in thread
From: Pavel Machek @ 2019-03-06 23:10 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, linux-pm, linux-samsung-soc, MyungJoo Ham,
Chanwoo Choi, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Lukasz Luba, Markus Reichl
[-- Attachment #1: Type: text/plain, Size: 829 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] devfreq: Suspend all devices on system shutdown
2019-03-06 23:10 ` Pavel Machek
@ 2019-03-07 1:27 ` Chanwoo Choi
0 siblings, 0 replies; 6+ messages in thread
From: Chanwoo Choi @ 2019-03-07 1:27 UTC (permalink / raw)
To: Pavel Machek, Marek Szyprowski
Cc: linux-kernel, linux-pm, linux-samsung-soc, MyungJoo Ham,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Lukasz Luba,
Markus Reichl
Hi Pavel,
On 19. 3. 7. 오전 8:10, Pavel Machek wrote:
> 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.
This patch executes the devfreq_suspend() for all devfreq devices.
The devfreq_suspend() does the following:
1. (mandatory) stop the polling of governor.
2. (optional) If devfreq device specifies the 'opp-suspend'
property in DT, the devfreq changes the frequency by using
the devfreq->suspend_freq.
If the user of devfreq device doesn't want to change the frequency
during the system shutdown, just don't add the 'opp-suspend' property
in DT. And the devfreq_suspend() will just stop the polling for governor.
Even if on battery-powered devices, I think it is not problem
to stop the polling of governor during the system shutdown.
In my case, I don't know what is dangerous.
Could you explain it more detailed.
>
> Could we explicitely do it only for devices that need it?
> Pavel
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread