linux-kernel - Re: [PATCH v5 08/10] clk: fix CLK_SET_RATE

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: <1522398050.3871.12.camel@baylibre.com>
Date:   Fri, 30 Mar 2018 10:20:50 +0200
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Stephen Boyd <sboyd@...eaurora.org>,
        Michael Turquette <mturquette@...libre.com>
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        Russell King <linux@...linux.org.uk>,
        Linus Walleij <linus.walleij@...aro.org>,
        Quentin Schulz <quentin.schulz@...e-electrons.com>,
        Kevin Hilman <khilman@...libre.com>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [PATCH v5 08/10] clk: fix CLK_SET_RATE_GATE with clock rate
 protection

On Fri, 2017-12-01 at 22:51 +0100, Jerome Brunet wrote:
> Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
> clock tree

Hi Mike, Stephen,

Gentle ping.
This patch has been waiting for while now.
As far as I know, the only blocking point to merging this patch is the qcom mmc
driver. The clocks used in this driver rely on the broken implementation of
CLK_SET_RATE_GATE, effectively ignoring the flag.

Since the flag is ignored, removing it from this particular clock won't make any
difference. Can we just do that until a better fix is available for this qcom
mmc clock ?

--- A bit of history ----

I managed to have a run on kci - based on v4.12-rc6:
https://kernelci.org/boot/all/job/khilman/branch/to-build/kernel/v4.12-rc6-10-ge
a373ddef830/

There was no build regression but kci did find one boot regression on qcom
platforms:
* qcom-apq8064-cm-qs600
* qcom-apq8064-ifc6410

it seems the problem is coming from the clock used by the mmc driver
(drivers/mmc/host/mmci.c)

the driver does following sequence:
* get_clk
* prepare_enable
* get_rate
* set_rate
* ...

with clock SDCx_clk (qcom_apq8064.dtsi:1037). This clock has CLK_SET_RATE_PARENT
flag so it will transmit the request to its parent.
The parent of this clock is SDCx_src which has the CLK_SET_RATE_GATE flag.

So obviously, is CLK_SET_RATE_GATE was enforced, the sequence used by mmci
driver would never had worked.

This particular driver rely on the fact that the clock can while the clock is
on. Either it actually works and we can remove CLK_SET_RATE_GATE until a better
solution comes.
OR, it does not work which means nobody has been using this driver for a long
time.

> 
> Acked-by: Linus Walleij <linus.walleij@...aro.org>
> Tested-by: Quentin Schulz <quentin.schulz@...e-electrons.com>
> Tested-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> Acked-by: Michael Turquette <mturquette@...libre.com>
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---
>  drivers/clk/clk.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f6fe5e5595ca..1af843ae20ff 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -605,6 +605,9 @@ static void clk_core_unprepare(struct clk_core *core)
>  	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
>  		return;
>  
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_unprotect(core);
> +
>  	if (--core->prepare_count > 0)
>  		return;
>  
> @@ -679,6 +682,16 @@ static int clk_core_prepare(struct clk_core *core)
>  
>  	core->prepare_count++;
>  
> +	/*
> +	 * CLK_SET_RATE_GATE is a special case of clock protection
> +	 * Instead of a consumer claiming exclusive rate control, it is
> +	 * actually the provider which prevents any consumer from making any
> +	 * operation which could result in a rate change or rate glitch while
> +	 * the clock is prepared.
> +	 */
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_protect(core);
> +
>  	return 0;
>  unprepare:
>  	clk_core_unprepare(core->parent);
> @@ -1780,9 +1793,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>  	if (clk_core_rate_is_protected(core))
>  		return -EBUSY;
>  
> -	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> -		return -EBUSY;
> -
>  	/* calculate new rates and get the topmost changed clock */
>  	top = clk_calc_new_rates(core, req_rate);
>  	if (!top)