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

[Marek Szyprowski]

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@xxxxxxxxxxx>

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland