linux-kernel - Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

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]

Date:	Mon, 27 Jul 2015 09:25:49 +0200
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel@...inux.com, mturquette@...aro.org, sboyd@...eaurora.org,
	devicetree@...r.kernel.org, geert@...ux-m68k.org,
	s.hauer@...gutronix.de
Subject: Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable,
 disable} framework

On Wed, Jul 22, 2015 at 02:04:13PM +0100, Lee Jones wrote:
> These new API calls will firstly provide a mechanisms to tag a clock as
> critical and secondly allow any knowledgeable driver to (un)gate clocks,
> even if they are marked as critical.
> 
> Suggested-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> Signed-off-by: Lee Jones <lee.jones@...aro.org>
> ---
>  drivers/clk/clk.c            | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h |  2 ++
>  include/linux/clk.h          | 30 +++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 61c3fc5..486b1da 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);
>  
>  /***    private data structures    ***/
>  
> +/**
> + * struct critical -	Provides 'play' over critical clocks.  A clock can be
> + *			marked as critical, meaning that it should not be
> + *			disabled.  However, if a driver which is aware of the
> + *			critical behaviour wants to control it, it can do so
> + *			using clk_enable_critical() and clk_disable_critical().
> + *
> + * @enabled	Is clock critical?  Once set, doesn't change
> + * @leave_on	Self explanatory.  Can be disabled by knowledgeable drivers
> + */
> +struct critical {
> +	bool enabled;
> +	bool leave_on;
> +};
> +
>  struct clk_core {
>  	const char		*name;
>  	const struct clk_ops	*ops;
> @@ -75,6 +90,7 @@ struct clk_core {
>  	struct dentry		*dentry;
>  #endif
>  	struct kref		ref;
> +	struct critical		critical;
>  };
>  
>  struct clk {
> @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)
>  	if (WARN_ON(clk->enable_count == 0))
>  		return;
>  
> +	/* Refuse to turn off a critical clock */
> +	if (clk->enable_count == 1 && clk->critical.leave_on)
> +		return;
> +

I think it should be handled by a separate counting. Otherwise, if you
have two users that marked the clock as critical, and then one of them
disable it...

>  	if (--clk->enable_count > 0)
>  		return;
>  
> @@ -1037,6 +1057,13 @@ void clk_disable(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_disable);
>  
> +void clk_disable_critical(struct clk *clk)
> +{
> +	clk->core->critical.leave_on = false;

.. you just lost the fact that it was critical in the first place.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)