linux-kernel - Re: [PATCH v4] clk: samsung: Prevent potential endless loop in the PLL set

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] [day] [month] [year] [list]

Message-ID: <20201113073641.GA4405@kozik-lap>
Date:   Fri, 13 Nov 2020 08:36:41 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Sylwester Nawrocki <s.nawrocki@...sung.com>
Cc:     linux-clk@...r.kernel.org, tomasz.figa@...il.com,
        cw00.choi@...sung.com, m.szyprowski@...sung.com, sboyd@...nel.org,
        mturquette@...libre.com, b.zolnierkie@...sung.com,
        linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v4] clk: samsung: Prevent potential endless loop in the
 PLL set_rate ops

On Tue, Nov 10, 2020 at 08:32:26PM +0100, Sylwester Nawrocki wrote:
> The PLL status polling loops in the set_rate callbacks of some PLLs
> have no timeout detection and may become endless loops when something
> goes wrong with the PLL.
> 
> For some PLLs there is already the ktime API based timeout detection,
> but it will not work in all conditions when .set_rate gets called.
> In particular, before the clocksource is initialized or when the
> timekeeping is suspended.
> 
> This patch adds a common helper with the PLL status bit polling and
> timeout detection. For conditions where the timekeeping API should not
> be used a simple readl_relaxed/cpu_relax() busy loop is added with the
> iterations limit derived from measurements of readl_relaxed() execution
> time for various PLL types and Exynos SoCs variants.
> 
> Actual PLL lock time depends on the P divider value, the VCO frequency
> and a constant PLL type specific LOCK_FACTOR and can be calculated as
> 
>  lock_time = Pdiv * LOCK_FACTOR / VCO_freq
> 
> For the ktime API use cases a common timeout value of 20 ms is applied
> for all the PLLs with an assumption that maximum possible value of Pdiv
> is 64, maximum possible LOCK_FACTOR value is 3000 and minimum VCO
> frequency is 24 MHz.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@...sung.com>
> ---
> I'm not sure whether we actually need to implement precise timeouts,
> likely the simple busy loop case would be enough. AFAIK the PLL
> failures happen very rarely, mostly in early code development stage
> for given platform.
> 
> Changes since v3:
>  - dropped udelay() from the PLL status bit polling loop as it didn't
>    work on arm64 at early boot time, before timekeeping was initialized,
>  - use the timekeeping API in cases when it is already initialized and
>    not suspended,
>  - use samsung_pll_lock_wait() also in samsung_pll3xxx_enable() function,
>    now all potential endless loops are removed.
> ---
>  drivers/clk/samsung/clk-pll.c | 147 ++++++++++++++++++++----------------------
>  1 file changed, 71 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index ac70ad7..cefb57e 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -8,14 +8,17 @@
>  
>  #include <linux/errno.h>
>  #include <linux/hrtimer.h>
> +#include <linux/iopoll.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/timekeeping.h>
>  #include <linux/clk-provider.h>
>  #include <linux/io.h>
>  #include "clk.h"
>  #include "clk-pll.h"
>  
> -#define PLL_TIMEOUT_MS		10
> +#define PLL_TIMEOUT_US		20000U
> +#define PLL_TIMEOUT_LOOPS	1000000U
>  
>  struct samsung_clk_pll {
>  	struct clk_hw		hw;
> @@ -63,6 +66,53 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
>  	return rate_table[i - 1].rate;
>  }
>  
> +static bool __early_timeout = true;

Drop the __ prefix and maybe use "pll_early_timeout".
This looks like __ro_after_init.

Best regards,
Krzysztof