Re: [PATCH] PM / devfreq: passive: Update frequency when start governor

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Marek,

Thanks for bug reporting. I'll check and fix it. 

Best Regards,
Chanwoo Choi

On 11/9/20 6:22 PM, Marek Szyprowski wrote:
> Hi Chanwoo,
> 
> On 03.11.2020 08:06, 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>
> 
> This patch landed in linux next-20201109 as commit 9b38a4f6b664 ("PM / 
> devfreq: passive: Update frequency when start governor"). Sadly it 
> causes the following warning on various Exynos boards with default 
> exynos_defconfig build:
> 
> exynos-bus: new bus device registered: soc:bus_dmc ( 50000 KHz ~ 400000 KHz)
> exynos-bus: new bus device registered: soc:bus_leftbus ( 50000 KHz ~ 
> 200000 KHz)
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 22 at drivers/devfreq/devfreq.c:411 
> devfreq_update_target+0xdc/0xec
> Modules linked in:
> CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.10.0-rc2-next-20201109 #1898
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events deferred_probe_work_func
> [<c0111648>] (unwind_backtrace) from [<c010d000>] (show_stack+0x10/0x14)
> [<c010d000>] (show_stack) from [<c0b224a0>] (dump_stack+0xb4/0xd4)
> [<c0b224a0>] (dump_stack) from [<c0126844>] (__warn+0xd8/0x11c)
> [<c0126844>] (__warn) from [<c0126938>] (warn_slowpath_fmt+0xb0/0xb8)
> [<c0126938>] (warn_slowpath_fmt) from [<c08ae39c>] 
> (devfreq_update_target+0xdc/0xec)
> [<c08ae39c>] (devfreq_update_target) from [<c08afa0c>] 
> (devfreq_passive_event_handler+0x80/0xc4)
> [<c08afa0c>] (devfreq_passive_event_handler) from [<c08ade70>] 
> (devfreq_add_device+0x4f8/0x5a0)
> [<c08ade70>] (devfreq_add_device) from [<c08adf60>] 
> (devm_devfreq_add_device+0x48/0x84)
> [<c08adf60>] (devm_devfreq_add_device) from [<c08b01c8>] 
> (exynos_bus_probe+0x2ac/0x5c8)
> [<c08b01c8>] (exynos_bus_probe) from [<c0692a70>] 
> (platform_drv_probe+0x6c/0xa4)
> [<c0692a70>] (platform_drv_probe) from [<c0690020>] 
> (really_probe+0x200/0x4fc)
> [<c0690020>] (really_probe) from [<c06904e4>] 
> (driver_probe_device+0x78/0x1fc)
> [<c06904e4>] (driver_probe_device) from [<c068de7c>] 
> (bus_for_each_drv+0x74/0xb8)
> [<c068de7c>] (bus_for_each_drv) from [<c068fd80>] 
> (__device_attach+0xe4/0x17c)
> [<c068fd80>] (__device_attach) from [<c068ee40>] 
> (bus_probe_device+0x88/0x90)
> [<c068ee40>] (bus_probe_device) from [<c068f358>] 
> (deferred_probe_work_func+0x3c/0xd0)
> [<c068f358>] (deferred_probe_work_func) from [<c0148a70>] 
> (process_one_work+0x234/0x7e4)
> [<c0148a70>] (process_one_work) from [<c0149064>] (worker_thread+0x44/0x51c)
> [<c0149064>] (worker_thread) from [<c014fdc8>] (kthread+0x158/0x1a0)
> [<c014fdc8>] (kthread) from [<c010011c>] (ret_from_fork+0x14/0x38)
> Exception stack(0xc1e27fb0 to 0xc1e27ff8)
> 7fa0:                                     00000000 00000000 00000000 
> 00000000
> 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
> 00000000
> 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> irq event stamp: 12147
> hardirqs last  enabled at (12171): [<c01a0358>] console_unlock+0x430/0x6b0
> dwc2 12480000.hsotg: new device is high-speed
> hardirqs last disabled at (12176): [<c01a0350>] console_unlock+0x428/0x6b0
> softirqs last  enabled at (12168): [<c01017a8>] __do_softirq+0x528/0x674
> softirqs last disabled at (12155): [<c012fdb0>] irq_exit+0x1dc/0x1e8
> ---[ end trace 89c1007ec29b9250 ]---
> 
> Reverting it on top of the linux-next fixes the warning, but I assume 
> that the proper fix would require locking changes during the devfreq 
> registration.
> 
>> ---
>>   drivers/devfreq/governor_passive.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index 63332e4a65ae..375aa636027c 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -141,6 +141,21 @@ 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.
>> +		 */
>> +		ret = devfreq_update_target(devfreq, parent->previous_freq);
>> +		if (ret < 0)
>> +			dev_warn(&devfreq->dev,
>> +			"failed to update devfreq using passive governor\n");
>> +
>>   		nb->notifier_call = devfreq_passive_notifier_call;
>>   		ret = devfreq_register_notifier(parent, nb,
>>   					DEVFREQ_TRANSITION_NOTIFIER);
> 
> Best regards
>