linux-kernel - Re: [PATCH v9 5/6] clk: Add floor and ceiling constraints to clock rates

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: <5407A6B9.1080606@codeaurora.org>
Date:	Wed, 03 Sep 2014 16:39:37 -0700
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	Mike Turquette <mturquette@...aro.org>
CC:	Stephen Warren <swarren@...dotorg.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>,
	linux-kernel@...r.kernel.org, tomasz.figa@...il.com, rabin@....in,
	Thierry Reding <thierry.reding@...il.com>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v9 5/6] clk: Add floor and ceiling constraints to clock
 rates

On 09/03/14 08:33, Tomeu Vizoso wrote:
> Adds a way for clock consumers to set maximum and minimum rates. This can be
> used for thermal drivers to set ceiling rates, or by misc. drivers to set
> floor rates to assure a minimum performance level.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> Tested-by: Heiko Stuebner <heiko@...ech.de>
>
> ---
>
> v9: * Apply first all the floor constraints, then the ceiling constraints.
>     * WARN on ceiling constraints below the current floor, for a given user clk
>
> v5: * Move the storage of constraints to the per-user clk struct, as suggested
>       by Stephen Warren.
> ---
>  drivers/clk/clk.c           | 43 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.h           |  1 +
>  drivers/clk/clkdev.c        |  2 +-
>  include/linux/clk-private.h |  5 +++++
>  include/linux/clk.h         | 18 ++++++++++++++++++
>  5 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 61a3492..3a961c6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -560,6 +560,8 @@ struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev,
>  	clk->dev_id = dev;
>  	clk->con_id = con;
>  
> +	hlist_add_head(&clk->child_node, &clk_core->per_user_clks);
> +

How is this safe with another thread that may be traversing the list? Or
even two threads calling clk_get_parent() at the same time?

>  	return clk;
>  }
>  
> @@ -1625,6 +1627,7 @@ static void clk_change_rate(struct clk_core *clk)
>  int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
>  {
>  	struct clk_core *top, *fail_clk;
> +	struct clk *clk_user;
>  	int ret = 0;
>  
>  	if (!clk)
> @@ -1633,6 +1636,15 @@ int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
>  	/* prevent racing with updates to the clock topology */
>  	clk_prepare_lock();
>  
> +	hlist_for_each_entry(clk_user, &clk->per_user_clks, child_node) {
> +		rate = max(rate, clk_user->floor_constraint);
> +	}
> +
> +	hlist_for_each_entry(clk_user, &clk->per_user_clks, child_node) {
> +		if (clk_user->ceiling_constraint > 0)
> +			rate = min(rate, clk_user->ceiling_constraint);
> +	}
> +
>  	/* bail early if nothing to do */
>  	if (rate == clk_provider_get_rate(clk))
>  		goto out;
> @@ -1699,6 +1711,29 @@ int clk_set_rate(struct clk *clk_user, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_rate);
>  
> +int clk_set_floor_rate(struct clk *clk_user, unsigned long rate)
> +{
> +	struct clk_core *clk = clk_to_clk_core(clk_user);
> +
> +	clk_user->floor_constraint = rate;
> +	return clk_provider_set_rate(clk, clk_provider_get_rate(clk));

It would be nice if this was also locked around so that the
floor_constraint or ceiling_constraint doesn't change while another
thread is iterating the list. I guess we'll get by though because
eventually things will settle and either this thread here will set the
"final" rate, or the other thread in clk_provider_set_rate() will have
already set the final rate. It just seems wrong to not hold the lock
while updating what is supposed to be protected by the prepare lock.

> +}
> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
> +
> +int clk_set_ceiling_rate(struct clk *clk_user, unsigned long rate)
> +{
> +	struct clk_core *clk = clk_to_clk_core(clk_user);
> +
> +	WARN(rate > 0 && rate < clk_user->floor_constraint,
> +	     "clk %s dev %s con %s: new ceiling %lu lower than existing floor %lu\n",
> +	     __clk_get_name(clk), clk_user->dev_id, clk_user->con_id, rate,
> +	     clk_user->floor_constraint);
> +
> +	clk_user->ceiling_constraint = rate;
> +	return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
> +}
> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);

Maybe I'm late to this patch series given that Mike applied it, but I
wonder why we wouldn't just have one API that takes a min and a max,
i.e. clk_set_rate_range(clk, min, max)? Then clk_set_rate() is a small
wrapper on top that just sets min and max to the same value.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/