linux-kernel - Re: [PATCH] clk: Remove global clk traversal on fetch parent index

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: <154542449501.13075.1518070014652024380@swboyd.mtv.corp.google.com>
Date:   Fri, 21 Dec 2018 12:34:55 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Derek Basehore <dbasehore@...omium.org>, mturquette@...libre.com
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        Derek Basehore <dbasehore@...omium.org>
Subject: Re: [PATCH] clk: Remove global clk traversal on fetch parent index

Quoting Derek Basehore (2018-12-20 16:31:00)
> It's not required to traverse the entire clk tree when the parents
> array contains a NULL value. You already have the parent clk_core
> pointer, so you can just compare the parent->name and parent_names[i]
> pointers.

Ok.

> 
> In cases where clk names are never registered, this can be
> a substantial power improvement since a mux having an unregistered
> parent name will traverse the clk tree on every set_rate. This can
> happen hundreds of times a second on CPU clks.
> 
> Change-Id: I85499d2e576249568ff508e424ca8d5009e6e2b1

This should go away.

> Signed-off-by: Derek Basehore <dbasehore@...omium.org>
> ---
>  drivers/clk/clk.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index af011974d4ec..57a95c713286 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1516,9 +1516,18 @@ static int clk_fetch_parent_index(struct clk_core *core,
>         if (!parent)
>                 return -EINVAL;
>  
> -       for (i = 0; i < core->num_parents; i++)
> -               if (clk_core_get_parent_by_index(core, i) == parent)
> +       for (i = 0; i < core->num_parents; i++) {
> +               if (core->parents[i] == parent)
> +                       return i;
> +
> +               if (core->parents[i])
> +                       continue;
> +
> +               if (!strcmp(parent->name, core->parent_names[i])) {
> +                       core->parents[i] = parent;
>                         return i;
> +               }
> +       }
>  

Wow, this looks really familiar. In fact, take a look at commit
da0f0b2c3ad2 ("clk: Correct lookup logic in clk_fetch_parent_index()")
and you'll see that it's almost the same code. The main difference is
that this patch's clk_fetch_parent_index() doesn't call __clk_lookup()
to get a pointer to the clk it already has. And then in commit
470b5e2f97cf ("clk: simplify clk_fetch_parent_index() function") we
consolidated all the logic to simplify things, but it seems that we
became confused that the result of !strcmp(parent, names[i]) is actually
only true when the strings are equal. That was an invalid
transformation. Ouch. But this is a good find!

So this patch is reverting commit 470b5e2f97cf and then optimizing it to
never call __clk_lookup() because it's useless. I'll need to add a
comment and expand on the commit text here so that it's more obvious
what's going on. Otherwise someone will come by and try to consolidate
again and we'll do this all over again.