| Message ID | 20201103070646.18687-1-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Chanwoo Choi |
| Headers | show |
| Series | PM / devfreq: passive: Update frequency when start governor | expand |
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
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 >
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);
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 | 15 +++++++++++++++ 1 file changed, 15 insertions(+)