| 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
| ||
|
Date: Fri, 14 Aug 2020 09:46:15 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Sylwester Nawrocki <s.nawrocki@...sung.com>,
linux-clk@...r.kernel.org
Cc: tomasz.figa@...il.com, sboyd@...nel.org, mturquette@...libre.com,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
b.zolnierkie@...sung.com, m.szyprowski@...sung.com
Subject: Re: [PATCH v3] clk: samsung: Prevent potential endless loop in the
PLL set_rate ops
Hi Sylwester,
On 8/13/20 6:55 PM, Sylwester Nawrocki wrote:
> In the .set_rate callback for some PLLs there is a loop polling state
> of the PLL lock bit and it may become an endless loop when something
> goes wrong with the PLL. For some PLLs there is already code for polling
> with a timeout but it uses the ktime API, which doesn't work in some
> conditions when the set_rate op is called, in particular during
> initialization of the clk provider before the clocksource initialization
> has completed. Hence the ktime API cannot be used to reliably detect
> the PLL locking timeout.
>
> This patch adds a common helper function for busy waiting on the PLL lock
> bit with timeout detection.
>
> 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
>
> Common timeout value of 10 ms is used 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>
> ---
> Changes for v3:
> - use busy-loop with udelay() instead of ktime API
> Changes for v2:
> - use common readl_relaxed_poll_timeout_atomic() macro
> ---
> drivers/clk/samsung/clk-pll.c | 94 ++++++++++++++++---------------------------
> 1 file changed, 34 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index ac70ad7..c83d261 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -15,7 +15,7 @@
> #include "clk.h"
> #include "clk-pll.h"
>
> -#define PLL_TIMEOUT_MS 10
> +#define PLL_TIMEOUT_US 10000U
>
> struct samsung_clk_pll {
> struct clk_hw hw;
> @@ -63,6 +63,25 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
> return rate_table[i - 1].rate;
> }
>
> +static int samsung_pll_lock_wait(struct samsung_clk_pll *pll,
> + unsigned int reg_mask)
> +{
> + int i;
> +
> + /* Wait until the PLL is in steady locked state */
> + for (i = 0; i < PLL_TIMEOUT_US / 2; i++) {
> + if (readl_relaxed(pll->con_reg) & reg_mask)
> + return 0;
> +
> + udelay(2);
> + cpu_relax();
> + }
> +
> + pr_err("Could not lock PLL %s\n", clk_hw_get_name(&pll->hw));
> +
> + return -ETIMEDOUT;
> +}
> +
> static int samsung_pll3xxx_enable(struct clk_hw *hw)
> {
> struct samsung_clk_pll *pll = to_clk_pll(hw);
> @@ -241,12 +260,9 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate,
> writel_relaxed(tmp, pll->con_reg);
>
> /* Wait until the PLL is locked if it is enabled. */
> - if (tmp & BIT(pll->enable_offs)) {
> - do {
> - cpu_relax();
> - tmp = readl_relaxed(pll->con_reg);
> - } while (!(tmp & BIT(pll->lock_offs)));
> - }
> + if (tmp & BIT(pll->enable_offs))
> + return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
> +
> return 0;
> }
>
> @@ -318,7 +334,7 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
> unsigned long parent_rate)
> {
> struct samsung_clk_pll *pll = to_clk_pll(hw);
> - u32 tmp, pll_con0, pll_con1;
> + u32 pll_con0, pll_con1;
> const struct samsung_pll_rate_table *rate;
>
> rate = samsung_get_pll_settings(pll, drate);
> @@ -356,13 +372,8 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
> pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
> writel_relaxed(pll_con1, pll->con_reg + 4);
>
> - /* wait_lock_time */
> - if (pll_con0 & BIT(pll->enable_offs)) {
> - do {
> - cpu_relax();
> - tmp = readl_relaxed(pll->con_reg);
> - } while (!(tmp & BIT(pll->lock_offs)));
> - }
> + if (pll_con0 & BIT(pll->enable_offs))
> + return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
>
> return 0;
> }
> @@ -437,7 +448,6 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long drate,
> struct samsung_clk_pll *pll = to_clk_pll(hw);
> const struct samsung_pll_rate_table *rate;
> u32 con0, con1;
> - ktime_t start;
>
> /* Get required rate settings from table */
> rate = samsung_get_pll_settings(pll, drate);
> @@ -489,20 +499,7 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long drate,
> writel_relaxed(con0, pll->con_reg);
>
> /* Wait for locking. */
> - start = ktime_get();
> - while (!(readl_relaxed(pll->con_reg) & PLL45XX_LOCKED)) {
> - ktime_t delta = ktime_sub(ktime_get(), start);
> -
> - if (ktime_to_ms(delta) > PLL_TIMEOUT_MS) {
> - pr_err("%s: could not lock PLL %s\n",
> - __func__, clk_hw_get_name(hw));
> - return -EFAULT;
> - }
> -
> - cpu_relax();
> - }
> -
> - return 0;
> + return samsung_pll_lock_wait(pll, PLL45XX_LOCKED);
> }
>
> static const struct clk_ops samsung_pll45xx_clk_ops = {
> @@ -588,7 +585,6 @@ static int samsung_pll46xx_set_rate(struct clk_hw *hw, unsigned long drate,
> struct samsung_clk_pll *pll = to_clk_pll(hw);
> const struct samsung_pll_rate_table *rate;
> u32 con0, con1, lock;
> - ktime_t start;
>
> /* Get required rate settings from table */
> rate = samsung_get_pll_settings(pll, drate);
> @@ -648,20 +644,7 @@ static int samsung_pll46xx_set_rate(struct clk_hw *hw, unsigned long drate,
> writel_relaxed(con1, pll->con_reg + 0x4);
>
> /* Wait for locking. */
> - start = ktime_get();
> - while (!(readl_relaxed(pll->con_reg) & PLL46XX_LOCKED)) {
> - ktime_t delta = ktime_sub(ktime_get(), start);
> -
> - if (ktime_to_ms(delta) > PLL_TIMEOUT_MS) {
> - pr_err("%s: could not lock PLL %s\n",
> - __func__, clk_hw_get_name(hw));
> - return -EFAULT;
> - }
> -
> - cpu_relax();
> - }
> -
> - return 0;
> + return samsung_pll_lock_wait(pll, PLL46XX_LOCKED);
> }
>
> static const struct clk_ops samsung_pll46xx_clk_ops = {
> @@ -1035,14 +1018,9 @@ static int samsung_pll2550xx_set_rate(struct clk_hw *hw, unsigned long drate,
> (rate->sdiv << PLL2550XX_S_SHIFT);
> writel_relaxed(tmp, pll->con_reg);
>
> - /* wait_lock_time */
> - do {
> - cpu_relax();
> - tmp = readl_relaxed(pll->con_reg);
> - } while (!(tmp & (PLL2550XX_LOCK_STAT_MASK
> - << PLL2550XX_LOCK_STAT_SHIFT)));
> -
> - return 0;
> + /* Wait for locking. */
> + return samsung_pll_lock_wait(pll,
> + PLL2550XX_LOCK_STAT_MASK << PLL2550XX_LOCK_STAT_SHIFT);
> }
>
> static const struct clk_ops samsung_pll2550xx_clk_ops = {
> @@ -1132,13 +1110,9 @@ static int samsung_pll2650x_set_rate(struct clk_hw *hw, unsigned long drate,
> con1 |= ((rate->kdiv & PLL2650X_K_MASK) << PLL2650X_K_SHIFT);
> writel_relaxed(con1, pll->con_reg + 4);
>
> - do {
> - cpu_relax();
> - con0 = readl_relaxed(pll->con_reg);
> - } while (!(con0 & (PLL2650X_LOCK_STAT_MASK
> - << PLL2650X_LOCK_STAT_SHIFT)));
> -
> - return 0;
> + /* Wait for locking. */
> + return samsung_pll_lock_wait(pll,
> + PLL2650X_LOCK_STAT_MASK << PLL2650X_LOCK_STAT_SHIFT);
> }
>
> static const struct clk_ops samsung_pll2650x_clk_ops = {
> --
> 2.7.4
>
>
>
Thanks.
Acked-by: Chanwoo Choi <cw00.choi@...sung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Powered by blists - more mailing lists