linux-kernel - Re: [PATCH] Revert "clk: fix __clk_init

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: <154394675320.88331.12449582989130694425@swboyd.mtv.corp.google.com>
Date:   Tue, 04 Dec 2018 10:05:53 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Jerome Brunet <jbrunet@...libre.com>,
        Michael Turquette <mturquette@...libre.com>
Cc:     Jerome Brunet <jbrunet@...libre.com>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: Re: [PATCH] Revert "clk: fix __clk_init_parent() for single parent clocks"

Quoting Jerome Brunet (2018-12-04 08:32:57)
> This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.
> 
> From the original commit message:
>   It turned out a problem because there are some single-parent clocks
>   that implement .get_parent() callback and return non-zero index.
>   The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.
> 
> It is wrong for a clock to return an index >= num_parents. CCF checks
> for this condition in clk_core_get_parent_by_index(). This function sets
> the parent to NULL if index is incoherent, which seems like the only
> sane choice.
> 
> commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks")
> appears to be a work around installed in the core framework for a problem
> that is platform specific, and should probably be fixed in the platform code.

Ouch. I see that I even pointed that out in 2016 but never got a reply
or a fix patch[1].

> 
> Further more, it introduces a problem in a corner case of the mux clock.
> Take mux with multiple parents, but only one is known, the rest being
> undocumented. The register reset has one of unknown parent set.
> 
> Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
>  * get_parent() is called, register is read and give an unknown index.
>    -> the mux is orphaned.
>  * a call to set_rate() will reparent the mux to the only known parent.
> 
> With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
>  * the register is never read.
>  * parent is wrongly assumed to be the only known one.
>    As a consequence, all the calculation deriving from the mux will be
>    wrong
>  * Since we believe the only know parent to be set, set_parent() won't
>    ever be called and the register won't be set with the correct value.

Isn't this the broken bad case all over again? Why register a clk as a
mux and then only say it has one parent?

> 
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---

Is this related to the other patch you sent? Can you send series for
related patches please?

[1] https://lkml.kernel.org/r/20160209181833.GA24167@codeaurora.org