Re: [PATCH] drivers: clk: keystone: Fix parameter judgment in clk_prepare

[Brian Masney <bmasney@redhat.com>]

Hi Ping,

Thanks for the patch!

On Thu, Jan 22, 2026 at 06:11:24PM +0800, ping.gao wrote:
>     The clk may return NULL or an ERR_PTR. Don't
>     treat an ERR_PTR as valid.
> 
>     for example: biu_clk in dwmmc driver request fail, but it's ERR_PTR,
>     not null,it will panic when call clk_prepare
>     log is below:

The patch subject has 'keystone:' that should be dropped. (Along with
the extra tab.)

This is also a nitpick but the extra spaces at the beginning of each
line in the commit message should be removed. Just start it at the
beginning of the line. When the patch is applied, 'git log' will
correctly indent your commit message properly for you.

>     [  438.400868] [7:   binder:436_2: 4998] Unable to handle kernel paging request at virtual address fffffffffffffffe
>     [  438.400877] [7:   binder:436_2: 4998] Mem abort info:
>     [  438.400881] [7:   binder:436_2: 4998]   ESR = 0x0000000096000005
>     [  438.400887] [7:   binder:436_2: 4998]   EC = 0x25: DABT (current EL), IL = 32 bits
>     [  438.400894] [7:   binder:436_2: 4998]   SET = 0, FnV = 0
>     [  438.400899] [7:   binder:436_2: 4998]   EA = 0, S1PTW = 0
>     [  438.400904] [7:   binder:436_2: 4998]   FSC = 0x05: level 1 translation fault
>     ...
>     [  438.409424] [7:   binder:436_2: 4998] Call trace:
>     [  438.409429] [7:   binder:436_2: 4998]  clk_prepare+0x10/0x24
>     [  438.409439] [7:   binder:436_2: 4998]  dw_mci_runtime_resume+0x50/0x2d8 [dw_mmc_samsung cd210e210975263404c28fc89778f369f8398f0c]
>     [  438.409471] [7:   binder:436_2: 4998]  dw_mci_exynos_runtime_resume+0x18/0x58 [dw_mmc_exynos_samsung 2735a594c7c9c9e8c65b0b87523fbf70dcaabfff]
>     [  438.409496] [7:   binder:436_2: 4998]  pm_generic_runtime_resume+0x40/0x58
>     [  438.409506] [7:   binder:436_2: 4998]  pm_runtime_force_resume+0x9c/0x134
>     [  438.409517] [7:   binder:436_2: 4998]  platform_pm_resume+0x40/0x8c
>     [  438.409529] [7:   binder:436_2: 4998]  dpm_run_callback+0x64/0x230
>     [  438.409540] [7:   binder:436_2: 4998]  __device_resume+0x1d8/0x394
>     [  438.409551] [7:   binder:436_2: 4998]  dpm_resume+0x110/0x2b8
>     [  438.409561] [7:   binder:436_2: 4998]  dpm_resume_end+0x1c/0x38
>     [  438.409570] [7:   binder:436_2: 4998]  suspend_devices_and_enter+0x828/0xab0
>     [  438.409582] [7:   binder:436_2: 4998]  pm_suspend+0x334/0x618
>     [  438.409592] [7:   binder:436_2: 4998]  state_store+0x104/0x144
>     [  438.409601] [7:   binder:436_2: 4998]  kobj_attr_store+0x30/0x48
>     [  438.409610] [7:   binder:436_2: 4998]  sysfs_kf_write+0x54/0x6c
>     [  438.409619] [7:   binder:436_2: 4998]  kernfs_fop_write_iter+0x104/0x1a8
>     [  438.409628] [7:   binder:436_2: 4998]  vfs_write+0x24c/0x2f4
>     [  438.409640] [7:   binder:436_2: 4998]  ksys_write+0x78/0xe8
>     [  438.409652] [7:   binder:436_2: 4998]  __arm64_sys_write+0x1c/0x2c
>     [  438.409664] [7:   binder:436_2: 4998]  invoke_syscall+0x58/0x114
>     [  438.409676] [7:   binder:436_2: 4998]  el0_svc_common+0xac/0xe0
>     [  438.409687] [7:   binder:436_2: 4998]  do_el0_svc+0x1c/0x28
>     [  438.409698] [7:   binder:436_2: 4998]  el0_svc+0x38/0x68
>     [  438.409705] [7:   binder:436_2: 4998]  el0t_64_sync_handler+0x68/0xbc
>     [  438.409712] [7:   binder:436_2: 4998]  el0t_64_sync+0x1a8/0x1ac
> 
> Signed-off-by: ping.gao <ping.gao@samsung.com>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..6d62f69323b5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1171,7 +1171,7 @@ static int clk_core_prepare_lock(struct clk_core *core)
>   */
>  int clk_prepare(struct clk *clk)
>  {
> -	if (!clk)
> +	if (IS_ERR_OR_NULL(clk))
>  		return 0;
>  
>  	return clk_core_prepare_lock(clk->core);

With a cleaned up commit message:

Reviewed-by: Brian Masney <bmasney@redhat.com>

Just to help Stephen: It looks like this is what ultimately causes the
problem:

dw_mci_probe() in drivers/mmc/host/dw_mmc.c sets up host->ciu_clk, and
if an error is returned by devm_clk_get(), it leaves the error pointer in
host->ciu_clk, and the probe continues normally. Later
dw_mci_runtime_resume() calls clk_prepare_enable() on host->ciu_clk with
the error pointer.

Additionally, clk_unprepare() already has this IS_ERR_OR_NULL() check.

Stephen: Do you think that clk_enable() should also be updated as well?
I see that clk_disable() also has the IS_ERR_OR_NULL() check.

Brian