linux-kernel - Re: [PATCH v2 12/16] clk: avoid circular clock topology

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, 23 Feb 2016 00:32:39 +0100
From:	Joachim Eastwood <manabian@...il.com>
To:	Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:	Stephen Boyd <sboyd@...eaurora.org>,
	Michael Turquette <mturquette@...libre.com>,
	linux-clk@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Subject: Re: [PATCH v2 12/16] clk: avoid circular clock topology

On 22 February 2016 at 03:29, Masahiro Yamada
<yamada.masahiro@...ionext.com> wrote:
> Hi Joachim,
>
>
> 2016-02-22 6:39 GMT+09:00 Joachim Eastwood <manabian@...il.com>:
>> Hi everyone,
>>
>> On 28 December 2015 at 11:10, Masahiro Yamada
>> <yamada.masahiro@...ionext.com> wrote:
>>> Currently, clk_register() never checks a circular parent looping,
>>> but clock providers could register such an insane clock topology.
>>> For example, "clk_a" could have "clk_b" as a parent, and vice versa.
>>> In this case, clk_core_reparent() creates a circular parent list
>>> and __clk_recalc_accuracies() calls itself recursively forever.
>>>
>>> The core infrastructure should be kind enough to bail out, showing
>>> an appropriate error message in such a case.  This helps to easily
>>> find a bug in clock providers.  (uh, I made such a silly mistake
>>> when I was implementing my clock providers first.  I was upset
>>> because the kernel did not respond, without any error message.)
>>>
>>> This commit adds a new helper function, __clk_is_ancestor().  It
>>> returns true if the second argument is a possible ancestor of the
>>> first one.  If a clock core is a possible ancestor of itself, it
>>> would make a loop when it were registered.  That should be detected
>>> as an error.
>>
>> This commit breaks lpc18xx boot in next right now. See
>> http://marc.info/?l=linux-arm-kernel&m=145608597106087&w=2
>>
>> The Clock Generation Unit (CGU) on lpc18xx allow for circular parents
>> in hardware. While it is obliviously not a good idea to configure the
>> clocks in that manner there is nothing that stops you either.
>>
>> Please take a look at the second figure on:
>> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
>> All PLLs can feed clock into the dividers and the dividers can feed
>> clock into the PLLs.
>>
>> The reason why this is made possible in the CGU is because you can
>> then choose where to put your divider; either before the PLL or after.
>>
>
>
> Sorry for breaking your board.

No worries, that is why we have next so we can catch it before it hits
mainline :-)


> I am OK with reverting b58f75aa83fb.
>
>
>
> I guess your hardware could make clock looping for the best flexibility
> but you do not make clock looping in actual use cases.

That's right.


> Maybe, does it make sense to check the parent looping
> in clk_set_parent() or somewhere, not in clk_register()?

I think that would be a nice addition. While the CGU can certainly be
configured with loops it is indeed something that we should prevent
from happening.


regards,
Joachim Eastwood