linux-kernel - Re: [PATCH] clk: provide public clk_is

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: <5251BD09.3050900@gmail.com>
Date:	Sun, 06 Oct 2013 21:42:01 +0200
From:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To:	Andrew Lunn <andrew@...n.ch>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Russell King <linux@....linux.org.uk>,
	Jason Cooper <jason@...edaemon.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	linux-kernel@...r.kernel.org,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Grant Likely <grant.likely@...aro.org>,
	Mike Turquette <mturquette@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] clk: provide public clk_is_enabled function

On 10/06/2013 06:30 PM, Andrew Lunn wrote:
> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
>>>
>>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote:
>>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
>>>>> To determine if a clk has been previously enabled, provide a public
>>>>> clk_is_enabled function. This is especially helpful to check the state
>>>>> of clk-gate without actually changing the state of the gate.
>>>> I wonder what you want to do with the return value.
>>>>
>>>> When doing
>>>>
>>>> 	if (clk_is_enabled(someclk))
>>>> 		do_something();
>>>>
>>>> you cannot in general know if the clock is still on when you start to
>>>> do_something.
>>>
>>> Hi Uwe
>>>
>>> At least in the use case Sebastian needs it for, we don't need an "in
>>> general" solution. It is used early boot time to see if the boot
>>> loader left the clock running.
>>
>> Wait, unless I'm missing something, the clk_is_enabled() call
>> _won't_ determine whether the clock is enabled in hardware
>> (whether the boot loader created or left this condition), instead
>> it only determines whether clk_enable() was called previously and
>> thus the clock _shall_ be enabled.
>
> Nope, you are wrong.
>
> static int clk_gate_is_enabled(struct clk_hw *hw)
> {
> 	u32 reg;
> 	struct clk_gate *gate = to_clk_gate(hw);
>
> 	reg = clk_readl(gate->reg);
>
> 	/* if a set bit disables this clk, flip it before masking */
> 	if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> 	   reg ^= BIT(gate->bit_idx);
>
> 	   reg &= BIT(gate->bit_idx);
>
> 	   return reg ? 1 : 0;
> }
>
> It reads the hardware state.
>
>> AFAIK the kernel's CCF support is "self contained" and does not
>> consider any data or state that was "inherited" from boot staged
>> before the kernel.  That's why the "disable unused" step disables
>> everything that wasn't acquired _in the kernel_ regardless of
>> what the boot loader may have done or what is enabled at reset.
>
> Not quite true. It uses the is_enabled(), which gets the real hardware
> state, to turn off clocks which are unused but on. It will not turn
> off clock which are already off. So it is inheriting some state from
> the boot loader, in that it knows if the bootloader turned it
> on. However this is not propagated into prepare/enable status.
>
>>> The other user of the clock is the
>>> ethernet driver, which we know cannot change it yet, because driver
>>> probing has not started yet.
>>
>> I understand that the situation here is, that the ethernet driver
>> hasn't probed yet, but the clock driver did.  You are in early setup
>> code and want to (check and) fetch data from the hardware which the
>> ethernet driver later needs.
>
> Nearly, but not quite. If there is an enabled DT node for the device,
> and that node does not have a valid local-mac-address property in the
> node, the bootloader should of programmed the MAC address into the
> device. If it has done that, the clock should be running, because as
> soon as you turn the clock off, it forgets the MAC address. Thus, if
> we find an enabled device in DT, without a valid local-mac-address,
> and the clock is off, we have a bootloader bug, which we want to
> report.
>
>> What's wrong with an explicit enable/disable around the data
>> acquisition?
>
> It avoids the CPU locking hard, but will not get us a valid MAC
> address, which is the point of the exercise.

While I agree to all Andew explained above, I somehow have the strong
feeling that an clk_is_enabled will just be abused where possible. We
already have two ppl complaining about it - even though a quick look at
clk/core.c should have cleared out most of it.

Maybe we should just enable the clock, get the (possibly bogus) MAC
and disable it again. We loose one possible FW_BUG report and overwrite
an invalid local-mac-address property with another invalid one.

Sebastian
--
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/