| Message ID | 20220507150145.531864-6-cw00.choi@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | PM / devfreq: Add cpu based scaling support to passive governor | expand |
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 >
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 --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)
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(+)