linux-kernel - Re: [PATCH 0/4] clk: mvebu: fix clk init order

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: <53032C14.9020502@free-electrons.com>
Date:	Tue, 18 Feb 2014 10:47:00 +0100
From:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
To:	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Emilio López 
	<emilio@...pez.com.ar>
CC:	Jason Cooper <jason@...edaemon.net>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Andrew Lunn <andrew@...n.ch>,
	Mike Turquette <mturquette@...aro.org>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 0/4] clk: mvebu: fix clk init order

On 17/02/2014 19:19, Ezequiel Garcia wrote:
> On Mon, Feb 17, 2014 at 04:59:01PM +0100, Gregory CLEMENT wrote:
> [..]
>>>
>>> Right. If you think it adds a regression, then that's a perfectly valid reasons
>>> for nacking.
>>>
>>> However, I'd like to double-check we have such a regression. I guess you're
>>> talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
>>> driver in the first place:
>>>
>>> void __init mvebu_coreclk_setup(struct device_node *np,
>>> 				const struct coreclk_soc_desc *desc)
>>> {
>>> 	const char *tclk_name = "tclk";
>>> [..] 
>>
>>
>> Here it is just about giving a name to a clock. As in the device tree
>> we only refer to the clock by index, the name don't matter.
>>
> 
> Unless I'm really confused about what's the problem here, the *name* is
> *all* that matters.
> 
> We're having a clock *registration* order issue (which is different from clock
> enable). Let me try to explain the issue, in case it's still not clear.
> 
> I'll stick to the current specific problem but it can apply to any other
> pair of parent/child.
> 
> Some of the mvebu clocks are registered as part of the "core" clock
> group, modeled by the "marvell,armada-370-core-clock" compatible node.
> 
> Another group of mvebu clocks are registered as part of the "gating"
> clock group, modeled by the "marvell,armada-370-gating-clock" compatible
> node.
> 
> By default, all the gating clocks are child of the first registered core
> clock. This clock is named "tclk" by default, and this name can be overloaded
> in the devicetree.
> 
> So far, so good, right?
> 
> The issue we're trying to fix, arises from the mvebu_clk_gating_setup()
> trying to get the name of this "tclk", as a registered clock.
> 
> In other words, the current code needs the tclk to be registered, for it
> will ask his name like this:
> 
> 	const char *default_parent = NULL;
> 
> 	clk = of_clk_get(np, 0);
> 	if (!IS_ERR(clk)) {
> 		default_parent = __clk_get_name(clk);
> 		clk_put(clk);
> 	}
> 
> Once it gets the name, all goes smoothly. Notice how the clock is obtained
> for the sole purpose of getting the name of it, which shows clearly it's the
> *name* that matters.
> 
> The ordering issue happens when the gating clock group is probed, before
> the core clock group. In that case, it's not possible to get the
> "&coreclk 0" (which is wrongly assumed to be registered), and so it's
> not possible to get the name.
> 
> So the root of the problem is that snippet above, which adds a
> completely unneeded registration order requirement. Instead, we should
> be looking for the names: we can hardcode the name or fetch it from the
> devicetree (Emilio has posted patches for both).
> 
> I really don't see why we're not fixing this, instead of adding yet
> another layer of complexity to the problem.

All this have already discussed in the previous emails. And even if Emilio
denied introducing a regression, it was what the code did. See my example
here:
http://article.gmane.org/gmane.linux.kernel/1649439

Now as you both are really annoying with it, what would be an acceptable
version would be the something like the patch attached. There will be still
an issue if old dtb is used with recent kernel, but at least the user will
be warned.

This code only fix the Armada 370 case, a complete solution should modify
the dtsi for Armada XP, Armada 375, Armada 38x, Kirkwood and Dove. It should
also make the outputname mandatory for gate-clk for consistency.


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

View attachment "0001-RFC-clk-mvebu-make-clock-output-names-mandatory.patch" of type "text/x-diff" (4908 bytes)