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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1jd0cbpg77.fsf@starbuckisacylon.baylibre.com>
Date:   Thu, 26 Dec 2019 10:51:56 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Michael Turquette <mturquette@...libre.com>
Cc:     Stephen Boyd <sboyd@...nel.org>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: Don't try to enable critical clocks if prepare failed


On Wed 25 Dec 2019 at 17:34, Guenter Roeck <linux@...ck-us.net> 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@...libre.com>
> Fixes: 99652a469df1 ("clk: migrate the count of orphaned clocks at init")
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
>  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();