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

[Chanwoo Choi <cwchoi00@gmail.com>]

On 22. 5. 13. 07:34, Marek Szyprowski wrote:
> Hi Chanwoo,
> 
> On 11.05.2022 11:35, Chanwoo Choi wrote:
>> From: Saravana Kannan <skannan@codeaurora.org>
>>
>> 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@chromium.org>
>> Tested-by: Johnson Wang <johnson.wang@mediatek.com>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> [Sibi: Integrated cpu-freqmap governor into passive_governor]
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> [Chanwoo: Fix conflict with latest code and cleanup code]
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> 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