Re: [PATCH v4 2/4] PM / devfreq: Add cpu based scaling support to passive governor

[Chanwoo Choi]

On 22. 5. 13. 07:34, Marek Szyprowski wrote:
> Hi Chanwoo,
>
> On 11.05.2022 11:35, Chanwoo Choi wrote:
> > From: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
> >
> > Many CPU architectures have caches that can scale independent of the
> > CPUs. Frequency scaling of the caches is necessary to make sure that the
> > cache is not a performance bottleneck that leads to poor performance and
> > power. The same idea applies for RAM/DDR.
> >
> > To achieve this, this patch adds support for cpu based scaling to the
> > passive governor. This is accomplished by taking the current frequency
> > of each CPU frequency domain and then adjust the frequency of the cache
> > (or any devfreq device) based on the frequency of the CPUs. It listens
> > to CPU frequency transition notifiers to keep itself up to date on the
> > current CPU frequency.
> >
> > To decide the frequency of the device, the governor does one of the
> > following:
> > * Derives the optimal devfreq device opp from required-opps property of
> >     the parent cpu opp_table.
> >
> > * Scales the device frequency in proportion to the CPU frequency. So, if
> >     the CPUs are running at their max frequency, the device runs at its
> >     max frequency. If the CPUs are running at their min frequency, the
> >     device runs at its min frequency. It is interpolated for frequencies
> >     in between.
> >
> > Tested-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > Tested-by: Johnson Wang <johnson.wang@xxxxxxxxxxxx>
> > Signed-off-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
> > [Sibi: Integrated cpu-freqmap governor into passive_governor]
> > Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
> > [Chanwoo: Fix conflict with latest code and cleanup code]
> > Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>
> This patch landed in today's linux next-20220512 as commit 2ab415d4e4e6
> ("PM / devfreq: Add cpu based scaling support to passive governor").
>
> It triggers the following NULL pointer dereference on Exynos based boards:
>
> exynos-bus: new bus device registered: soc:bus-leftbus (100000 KHz ~
> 200000 KHz)
> exynos-bus: new bus device registered: soc:bus-rightbus (100000 KHz ~
> 200000 KHz)
> exynos-bus: new bus device registered: soc:bus-display (160000 KHz ~
> 200000 KHz)
> exynos-bus: new bus device registered: soc:bus-fsys (100000 KHz ~ 134000
> KHz)
> exynos-bus: new bus device registered: soc:bus-peri ( 50000 KHz ~ 100000
> KHz)
> exynos-bus: new bus device registered: soc:bus-mfc (100000 KHz ~ 200000 KHz)
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 0000003c
> [0000003c] *pgd=00000000
> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.18.0-rc6-next-20220512 #5014
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: devfreq_wq devfreq_monitor
> PC is at __mutex_lock+0x48/0x948
> LR is at lock_is_held_type+0x104/0x1a4
> pc : [<c0b93098>]    lr : [<c0b8f2ec>]    psr: 60000053
> sp : f0889dc0  ip : 600000d3  fp : c1dca624
> r10: c1dca44c  r9 : 00000000  r8 : c1984104
> r7 : c1d7f000  r6 : 00000000  r5 : 00000001  r4 : 00000008
> r3 : 00000000  r2 : 00000000  r1 : 2de44000  r0 : 00000000
> Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 4000404a  DAC: 00000051
> Register r0 information: NULL pointer
> Register r1 information: non-paged memory
> Register r2 information: NULL pointer
> Register r3 information: NULL pointer
> Register r4 information: non-paged memory
> Register r5 information: non-paged memory
> Register r6 information: NULL pointer
> Register r7 information: slab task_struct start c1d7f000 pointer offset 0
> Register r8 information: non-slab/vmalloc memory
> Register r9 information: NULL pointer
> Register r10 information: slab kmalloc-2k start c1dca000 pointer offset
> 1100 size 2048
> Register r11 information: slab kmalloc-2k start c1dca000 pointer offset
> 1572 size 2048
> Register r12 information: non-paged memory
> Process kworker/u8:0 (pid: 8, stack limit = 0x(ptrval))
> Stack: (0xf0889dc0 to 0xf088a000)
> ...
>    __mutex_lock from mutex_lock_nested+0x1c/0x24
>    mutex_lock_nested from devfreq_passive_notifier_call+0x24/0x90
>    devfreq_passive_notifier_call from srcu_notifier_call_chain+0x98/0x114
>    srcu_notifier_call_chain from devfreq_set_target+0x6c/0x304
>    devfreq_set_target from devfreq_update_target+0x98/0xe8
>    devfreq_update_target from devfreq_monitor+0x28/0x1c0
>    devfreq_monitor from process_one_work+0x288/0x774
>    process_one_work from worker_thread+0x44/0x504
>    worker_thread from kthread+0xf4/0x128
>    kthread from ret_from_fork+0x14/0x2c
> Exception stack(0xf0889fb0 to 0xf0889ff8)
> ...
> ---[ end trace 0000000000000000 ]---
>
> The issue is caused by the lack of setting devfreq_passive_data->this
> pointer in devfreq_passive_register_notifier. However, after adding:
>
> @@ -395,6 +395,9 @@ static int devfreq_passive_register_notifier(struct
> devfreq *devfreq)
>           if (!parent)
>                   return -EPROBE_DEFER;
>
> +       if (!p_data->this)
> +               p_data->this = devfreq;
> +
>           nb->notifier_call = devfreq_passive_notifier_call;
>           return devfreq_register_notifier(parent, nb,
> DEVFREQ_TRANSITION_NOTIFIER);
>    }
>
> the NULL pointer dereference is gone, but I see the following warnings
> on Odroid U3 board, which were not present before this patch:
>
> devfreq soc:bus-acp: failed to update devfreq using passive governor
> devfreq soc:bus-c2c: failed to update devfreq using passive governor
> devfreq soc:bus-acp: failed to update devfreq using passive governor
> devfreq soc:bus-c2c: failed to update devfreq using passive governor

Hi Marek,

Thanks for the report. I'll fix it.

--
Best Regards,
Samsung Electronics
Chanwoo Choi