Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 9 May 2022 12:25:59 +0800
From:   Chen-Yu Tsai <wenst@chromium.org>
To:     Chanwoo Choi <cwchoi00@gmail.com>
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        cw00.choi@samsung.com, myungjoo.ham@samsung.com,
        kyungmin.park@samsung.com, johnson.wang@mediatek.com,
        mka@chromium.org, jia-wei.chang@mediatek.com,
        andrew-sh.cheng@mediatek.com, hsinyi@chromium.org,
        sibis@codeaurora.org, saravanak@google.com
Subject: Re: [PATCH v2 5/5] PM / devfreq: passive: Update frequency when
 start governor
Message-ID: <YniX1w+oI1eOCmCx@google.com>
References: <20220507150145.531864-1-cw00.choi@samsung.com>
 <20220507150145.531864-6-cw00.choi@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220507150145.531864-6-cw00.choi@samsung.com>
Precedence: bulk

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
>