Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed

[Jerome Brunet]

On Wed 25 Dec 2019 at 17:34, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:

> The following traceback is seen if a critical clock fails to prepare.
>
> bcm2835-clk 3f101000.cprman: plld: couldn't lock PLL
> ------------[ cut here ]------------
> Enabling unprepared plld_per
> WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:1014 clk_core_enable+0xcc/0x2c0
> ...
> Call trace:
>  clk_core_enable+0xcc/0x2c0
>  __clk_register+0x5c4/0x788
>  devm_clk_hw_register+0x4c/0xb0
>  bcm2835_register_pll_divider+0xc0/0x150
>  bcm2835_clk_probe+0x134/0x1e8
>  platform_drv_probe+0x50/0xa0
>  really_probe+0xd4/0x308
>  driver_probe_device+0x54/0xe8
>  device_driver_attach+0x6c/0x78
>  __driver_attach+0x54/0xd8
> ...
>
> Check return values from clk_core_prepare() and clk_core_enable() and
> bail out if any of those functions returns an error.
>
> Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> Fixes: 99652a469df1 ("clk: migrate the count of orphaned clocks at init")
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
>  drivers/clk/clk.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 6a11239ccde3..772258de2d1f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3426,11 +3426,17 @@ static int __clk_core_init(struct clk_core *core)
>  	if (core->flags & CLK_IS_CRITICAL) {
>  		unsigned long flags;
>  
> -		clk_core_prepare(core);
> +		ret = clk_core_prepare(core);
> +		if (ret)
> +			goto out;
>  
>  		flags = clk_enable_lock();
> -		clk_core_enable(core);
> +		ret = clk_core_enable(core);
>  		clk_enable_unlock(flags);
> +		if (ret) {
> +			clk_core_unprepare(core);
> +			goto out;
> +		}

Hi Guenter,

It looks like it was a mistake to discard the possibility of a failure
here. Thanks for correcting this.

However, we would not want a critical clock to silently fail to
enable. This might lead to unexpected behavior which are generally hard
(and annoying) to debug.

Would you mind adding some kind of warning trace in case this fails ?

Thx

>  	}
>  
>  	clk_core_reparent_orphans_nolock();