diff mbox series

[v2,5/5] PM / devfreq: passive: Update frequency when start governor

Message ID 20220507150145.531864-6-cw00.choi@samsung.com (mailing list archive)
State Superseded, archived
Headers show
Series PM / devfreq: Add cpu based scaling support to passive governor | expand

Commit Message

Chanwoo Choi May 7, 2022, 3:01 p.m. UTC 
If the parent device changes the their frequency before registering
the passive device, the passive device cannot receive the notification
from parent device and then the passive device cannot be able to
set the proper frequency according to the frequency of parent device.

So, when start the passive governor, update the frequency
according to the frequency of parent device.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/governor_passive.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Chen-Yu Tsai May 9, 2022, 4:25 a.m. UTC | #1 
Hi,

On Sun, May 08, 2022 at 12:01:45AM +0900, Chanwoo Choi wrote:
> If the parent device changes the their frequency before registering
> the passive device, the passive device cannot receive the notification
> from parent device and then the passive device cannot be able to
> set the proper frequency according to the frequency of parent device.
> 
> So, when start the passive governor, update the frequency
> according to the frequency of parent device.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Link: https://lore.kernel.org/r/20220507150145.531864-6-cw00.choi@samsung.com
> ---
>  drivers/devfreq/governor_passive.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index b34dbe750c0a..74d26c193fdb 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -412,6 +412,23 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>  		if (!p_data->this)
>  			p_data->this = devfreq;
>  
> +		/*
> +		 * If the parent device changes the their frequency before
> +		 * registering the passive device, the passive device cannot
> +		 * receive the notification from parent device and then the
> +		 * passive device cannot be able to set the proper frequency
> +		 * according to the frequency of parent device.
> +		 *
> +		 * When start the passive governor, update the frequency
> +		 * according to the frequency of parent device.
> +		 */
> +		mutex_lock(&devfreq->lock);
> +		ret = devfreq_update_target(devfreq, parent->previous_freq);

This crashes when parent is NULL, in the case where parent is cpufreq.
This is the case with the MTK ccifreq driver, which produces the panic
and backtrace below [1].

I made a fix for a previous version of this patch:

    https://github.com/wens/linux/commit/f85c1834dd07388abb57a00200c80f7440823a03

BTW, could you CC me on future revisions? I'm not subscribed to the
linux-pm mailing list.


Regards
ChenYu

[1]

Unable to handle kernel read from unreadable memory at virtual address 0000000000000420
Mem abort info:
ESR = 0x0000000096000005
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x05: level 1 translation fault
Data abort info:
ISV = 0, ISS = 0x00000005
CM = 0, WnR = 0
[0000000000000420] user address but active_mm is swapper
Internal error: Oops: 96000005 [#1] PREEMPT SMP
Modules linked in:
CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc5-next-20220505-09393-g38dc825c1d73 #155 b348fdb8d61a403eef7a9c5857bc02a261fcb213
Hardware name: Google juniper sku16 board (DT)
pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devfreq_passive_event_handler (drivers/devfreq/governor_passive.c:426)
lr : devfreq_passive_event_handler (drivers/devfreq/governor_passive.c:426)
sp : ffffffc00808ba80
x29: ffffffc00808ba80 x28: 0000000000000000 x27: ffffffe99bb90458
x26: 0000000000000010 x25: ffffff80c1843848 x24: ffffff80c1843810
x23: ffffffe99babf3f5 x22: ffffffe99c278190 x21: ffffff80c0924d80
x20: ffffff80c1843800 x19: 0000000000000000 x18: 0000000000000000
x17: 0000000065516d0e x16: 00000000fc90660b x15: 0000000000000018
x14: 0000000000000000 x13: ffffffffff000000 x12: 0000000000000038
x11: 0101010101010101 x10: 8000000000000000 x9 : ffffffe99acb8458
x8 : 0065766973000000 x7 : 0000000000000080 x6 : 0000000000000000
x5 : 8000000000000000 x4 : 0000000000000000 x3 : ffffff80c1843810
x2 : ffffff80c0228000 x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
devfreq_passive_event_handler (drivers/devfreq/governor_passive.c:426)
devfreq_add_device (drivers/devfreq/devfreq.c:932)
devm_devfreq_add_device (drivers/devfreq/devfreq.c:1028)
mtk_ccifreq_probe (drivers/devfreq/mtk-cci-devfreq.c:366)
platform_probe (drivers/base/platform.c:1398)
really_probe (drivers/base/dd.c:542 drivers/base/dd.c:621 drivers/base/dd.c:566)
__driver_probe_device (drivers/base/dd.c:752)
driver_probe_device (drivers/base/dd.c:782)
__driver_attach (drivers/base/dd.c:1143 drivers/base/dd.c:1094)
bus_for_each_dev (drivers/base/bus.c:301)
driver_attach (drivers/base/dd.c:1160)
bus_add_driver (drivers/base/bus.c:619)
driver_register (drivers/base/driver.c:240)
__platform_driver_register (drivers/base/platform.c:866)
mtk_ccifreq_platdrv_init (drivers/devfreq/mtk-cci-devfreq.c:468)
do_one_initcall (init/main.c:1301)
kernel_init_freeable (init/main.c:1375 init/main.c:1392 init/main.c:1411 init/main.c:1618)
kernel_init (init/main.c:1511)
ret_from_fork (arch/arm64/kernel/entry.S:868)
Code: f9000eb4 91004298 aa1803e0 940979d4 (f9421261)
All code
========
   0:	f9000eb4 	str	x20, [x21, #24]
   4:	91004298 	add	x24, x20, #0x10
   8:	aa1803e0 	mov	x0, x24
   c:	940979d4 	bl	0x25e75c
  10:*	f9421261 	ldr	x1, [x19, #1056]		<-- trapping instruction

Code starting with the faulting instruction
===========================================
   0:	f9421261 	ldr	x1, [x19, #1056]
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Oops: Fatal exception
SMP: stopping secondary CPUs
Kernel Offset: 0x2992c00000 from 0xffffffc008000000
PHYS_OFFSET: 0x40000000
CPU features: 0x000,00324811,00001086
Memory Limit: none
PANIC in EL3.

> +		if (ret < 0)
> +			dev_warn(&devfreq->dev,
> +			"failed to update devfreq using passive governor\n");
> +		mutex_unlock(&devfreq->lock);
> +
>  		if (p_data->parent_type == DEVFREQ_PARENT_DEV)
>  			ret = devfreq_passive_register_notifier(devfreq);
>  		else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> 
> -- 
> 2.25.1
>
Chanwoo Choi May 9, 2022, 10:29 a.m. UTC | #2 
Hi,

On 22. 5. 9. 13:25, Chen-Yu Tsai wrote:
> Hi,
> 
> On Sun, May 08, 2022 at 12:01:45AM +0900, Chanwoo Choi wrote:
>> If the parent device changes the their frequency before registering
>> the passive device, the passive device cannot receive the notification
>> from parent device and then the passive device cannot be able to
>> set the proper frequency according to the frequency of parent device.
>>
>> So, when start the passive governor, update the frequency
>> according to the frequency of parent device.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Link: https://lore.kernel.org/r/20220507150145.531864-6-cw00.choi@samsung.com
>> ---
>>   drivers/devfreq/governor_passive.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index b34dbe750c0a..74d26c193fdb 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -412,6 +412,23 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>>   		if (!p_data->this)
>>   			p_data->this = devfreq;
>>   
>> +		/*
>> +		 * If the parent device changes the their frequency before
>> +		 * registering the passive device, the passive device cannot
>> +		 * receive the notification from parent device and then the
>> +		 * passive device cannot be able to set the proper frequency
>> +		 * according to the frequency of parent device.
>> +		 *
>> +		 * When start the passive governor, update the frequency
>> +		 * according to the frequency of parent device.
>> +		 */
>> +		mutex_lock(&devfreq->lock);
>> +		ret = devfreq_update_target(devfreq, parent->previous_freq);
> 
> This crashes when parent is NULL, in the case where parent is cpufreq.
> This is the case with the MTK ccifreq driver, which produces the panic
> and backtrace below [1].
> 
> I made a fix for a previous version of this patch:
> 
>      https://github.com/wens/linux/commit/f85c1834dd07388abb57a00200c80f7440823a03
> 
> BTW, could you CC me on future revisions? I'm not subscribed to the
> linux-pm mailing list.

OK. I'll.

> 
> 
> Regards
> ChenYu
> 
> [1]
> 
> Unable to handle kernel read from unreadable memory at virtual address 0000000000000420
> Mem abort info:
> ESR = 0x0000000096000005
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x05: level 1 translation fault
> Data abort info:
> ISV = 0, ISS = 0x00000005
> CM = 0, WnR = 0
> [0000000000000420] user address but active_mm is swapper
> Internal error: Oops: 96000005 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc5-next-20220505-09393-g38dc825c1d73 #155 b348fdb8d61a403eef7a9c5857bc02a261fcb213
> Hardware name: Google juniper sku16 board (DT)
> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : devfreq_passive_event_handler (drivers/devfreq/governor_passive.c:426)
> lr : devfreq_passive_event_handler (drivers/devfreq/governor_passive.c:426)
> sp : ffffffc00808ba80
> x29: ffffffc00808ba80 x28: 0000000000000000 x27: ffffffe99bb90458
> x26: 0000000000000010 x25: ffffff80c1843848 x24: ffffff80c1843810
> x23: ffffffe99babf3f5 x22: ffffffe99c278190 x21: ffffff80c0924d80
> x20: ffffff80c1843800 x19: 0000000000000000 x18: 0000000000000000
> x17: 0000000065516d0e x16: 00000000fc90660b x15: 0000000000000018
> x14: 0000000000000000 x13: ffffffffff000000 x12: 0000000000000038
> x11: 0101010101010101 x10: 8000000000000000 x9 : ffffffe99acb8458
> x8 : 0065766973000000 x7 : 0000000000000080 x6 : 0000000000000000
> x5 : 8000000000000000 x4 : 0000000000000000 x3 : ffffff80c1843810
> x2 : ffffff80c0228000 x1 : 0000000000000000 x0 : 0000000000000000
> Call trace:
> devfreq_passive_event_handler (drivers/devfreq/governor_passive.c:426)
> devfreq_add_device (drivers/devfreq/devfreq.c:932)
> devm_devfreq_add_device (drivers/devfreq/devfreq.c:1028)
> mtk_ccifreq_probe (drivers/devfreq/mtk-cci-devfreq.c:366)
> platform_probe (drivers/base/platform.c:1398)
> really_probe (drivers/base/dd.c:542 drivers/base/dd.c:621 drivers/base/dd.c:566)
> __driver_probe_device (drivers/base/dd.c:752)
> driver_probe_device (drivers/base/dd.c:782)
> __driver_attach (drivers/base/dd.c:1143 drivers/base/dd.c:1094)
> bus_for_each_dev (drivers/base/bus.c:301)
> driver_attach (drivers/base/dd.c:1160)
> bus_add_driver (drivers/base/bus.c:619)
> driver_register (drivers/base/driver.c:240)
> __platform_driver_register (drivers/base/platform.c:866)
> mtk_ccifreq_platdrv_init (drivers/devfreq/mtk-cci-devfreq.c:468)
> do_one_initcall (init/main.c:1301)
> kernel_init_freeable (init/main.c:1375 init/main.c:1392 init/main.c:1411 init/main.c:1618)
> kernel_init (init/main.c:1511)
> ret_from_fork (arch/arm64/kernel/entry.S:868)
> Code: f9000eb4 91004298 aa1803e0 940979d4 (f9421261)
> All code
> ========
>     0:	f9000eb4 	str	x20, [x21, #24]
>     4:	91004298 	add	x24, x20, #0x10
>     8:	aa1803e0 	mov	x0, x24
>     c:	940979d4 	bl	0x25e75c
>    10:*	f9421261 	ldr	x1, [x19, #1056]		<-- trapping instruction
> 
> Code starting with the faulting instruction
> ===========================================
>     0:	f9421261 	ldr	x1, [x19, #1056]
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Oops: Fatal exception
> SMP: stopping secondary CPUs
> Kernel Offset: 0x2992c00000 from 0xffffffc008000000
> PHYS_OFFSET: 0x40000000
> CPU features: 0x000,00324811,00001086
> Memory Limit: none
> PANIC in EL3.
> 
>> +		if (ret < 0)
>> +			dev_warn(&devfreq->dev,
>> +			"failed to update devfreq using passive governor\n");
>> +		mutex_unlock(&devfreq->lock);
>> +
>>   		if (p_data->parent_type == DEVFREQ_PARENT_DEV)
>>   			ret = devfreq_passive_register_notifier(devfreq);
>>   		else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
>>
>> -- 
>> 2.25.1
>>

Thanks for the testing. I'll drop the last patch
and then send next version.
diff mbox series

Patch

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index b34dbe750c0a..74d26c193fdb 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -412,6 +412,23 @@  static int devfreq_passive_event_handler(struct devfreq *devfreq,
 		if (!p_data->this)
 			p_data->this = devfreq;
 
+		/*
+		 * If the parent device changes the their frequency before
+		 * registering the passive device, the passive device cannot
+		 * receive the notification from parent device and then the
+		 * passive device cannot be able to set the proper frequency
+		 * according to the frequency of parent device.
+		 *
+		 * When start the passive governor, update the frequency
+		 * according to the frequency of parent device.
+		 */
+		mutex_lock(&devfreq->lock);
+		ret = devfreq_update_target(devfreq, parent->previous_freq);
+		if (ret < 0)
+			dev_warn(&devfreq->dev,
+			"failed to update devfreq using passive governor\n");
+		mutex_unlock(&devfreq->lock);
+
 		if (p_data->parent_type == DEVFREQ_PARENT_DEV)
 			ret = devfreq_passive_register_notifier(devfreq);
 		else if (p_data->parent_type == CPUFREQ_PARENT_DEV)