linux-kernel - Re: [PATCH v10 3/3] clk: Add rate constraints to clocks

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]

Date:	Tue, 20 Jan 2015 16:46:55 -0800
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	linux-kernel@...r.kernel.org,
	Mike Turquette <mturquette@...aro.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Jonathan Corbet <corbet@....net>,
	Tony Lindgren <tony@...mide.com>,
	Russell King <linux@....linux.org.uk>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	Emilio López <emilio@...pez.com.ar>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Tero Kristo <t-kristo@...com>,
	Manuel Lauss <manuel.lauss@...il.com>,
	Alex Elder <elder@...aro.org>,
	Matt Porter <mporter@...aro.org>,
	Zhangfei Gao <zhangfei.gao@...aro.org>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Bintian Wang <bintian.wang@...wei.com>,
	Chao Xie <chao.xie@...vell.com>, linux-doc@...r.kernel.org,
	linux-omap@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mips@...ux-mips.org
Subject: Re: [PATCH v10 3/3] clk: Add rate constraints to clocks

It's looking fairly close. Thanks for keeping up with the review
comments.

On 01/20, Tomeu Vizoso wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e867d6a..f241e27 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2143,6 +2280,10 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
>  	else
>  		clk->owner = NULL;
>  
> +	INIT_HLIST_HEAD(&clk->clks);
> +
> +	hw->clk = __clk_create_clk(hw, NULL, NULL);
> +
>  	ret = __clk_init(dev, hw->clk);
>  	if (ret)
>  		return ERR_PTR(ret);

Don't we need to __clk_free_clk() here too?

> @@ -2151,6 +2292,19 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
>  }
>  EXPORT_SYMBOL_GPL(__clk_register);
>  
> +static void __clk_free_clk(struct clk *clk)
> +{
> +	struct clk_core *core = clk->core;
> +
> +	clk_prepare_lock();
> +	hlist_del(&clk->child_node);
> +	clk_prepare_unlock();
> +
> +	kfree(clk);
> +
> +	clk_core_set_rate(core, core->req_rate);

Is it safe to call this during clock registration? I hope that it
will just bail out and do nothing because core->rate ==
core->req_rate. Maybe we can avoid this given my next comment
below.

> +}
> +
>  /**
>   * clk_register - allocate a new clock, register it and return an opaque cookie
>   * @dev: device that is registering this clock
> @@ -2210,12 +2364,14 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  		}
>  	}
>  
> +	INIT_HLIST_HEAD(&clk->clks);
> +
>  	hw->clk = __clk_create_clk(hw, NULL, NULL);
>  	ret = __clk_init(dev, hw->clk);
>  	if (!ret)
>  		return hw->clk;
>  
> -	kfree(hw->clk);
> +	__clk_free_clk(hw->clk);
>  fail_parent_names_copy:
>  	while (--i >= 0)
>  		kfree(clk->parent_names[i]);
> @@ -2421,7 +2577,7 @@ void __clk_put(struct clk *clk)
>  		return;
>  
>  	clk_core_put(clk->core);
> -	kfree(clk);
> +	__clk_free_clk(clk);

This doesn't look right. First we drop the core reference here
with clk_core_put() and then we call __clk_free_clk() which will
go and call clk_core_set_rate() on the clk->core which may or may
not exist anymore. I'd think we want to do these steps:

 1. Unlink clk from clks list
 2. Recalculate rate and set if changed
 3. Drop kref on core with clk_core_put()
 4. kfree the clk

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/