linux-kernel - Re: [PATCH] clk: Provide future parent in clk notification

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <159293933210.62212.706350398043250620@swboyd.mtv.corp.google.com>
Date:   Tue, 23 Jun 2020 12:08:52 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Ikjoon Jang <ikjn@...omium.org>, linux-clk@...r.kernel.org
Cc:     Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, Ikjoon Jang <ikjn@...omium.org>
Subject: Re: [PATCH] clk: Provide future parent in clk notification

Quoting Ikjoon Jang (2020-06-15 22:52:23)
> Current clk notification handlers cannot know its new parent in
> PRE_RATE_CHANGE event. This patch simply adds parent clk to
> clk_notifier_data so the child clk is now able to know its future
> parent prior to reparenting.

Yes, but why is that important?

> 
> Change-Id: I099a784d5302a93951bdc6254d85f8df8c770462

Please remove these.

> Signed-off-by: Ikjoon Jang <ikjn@...omium.org>
> ---
>  drivers/clk/clk.c   | 30 +++++++++++++++++-------------
>  include/linux/clk.h |  9 ++++++---
>  2 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 3f588ed06ce3..62c4e7b50ae5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1846,7 +1849,7 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
>   * take on the rate of its parent.
>   */
>  static int __clk_speculate_rates(struct clk_core *core,
> -                                unsigned long parent_rate)
> +                                struct clk_core *parent)
>  {
>         struct clk_core *child;
>         unsigned long new_rate;
> @@ -1854,11 +1857,12 @@ static int __clk_speculate_rates(struct clk_core *core,
>  
>         lockdep_assert_held(&prepare_lock);
>  
> -       new_rate = clk_recalc(core, parent_rate);
> +       new_rate = clk_recalc(core, parent ? parent->rate : 0);
>  
>         /* abort rate change if a driver returns NOTIFY_BAD or NOTIFY_STOP */
>         if (core->notifier_count)
> -               ret = __clk_notify(core, PRE_RATE_CHANGE, core->rate, new_rate);
> +               ret = __clk_notify(core, parent, PRE_RATE_CHANGE,
> +                                  core->rate, new_rate);
>  
>         if (ret & NOTIFY_STOP_MASK) {
>                 pr_debug("%s: clk notifier callback for clock %s aborted with error %d\n",
> @@ -1867,7 +1871,7 @@ static int __clk_speculate_rates(struct clk_core *core,
>         }
>  
>         hlist_for_each_entry(child, &core->children, child_node) {
> -               ret = __clk_speculate_rates(child, new_rate);
> +               ret = __clk_speculate_rates(child, core);

How does this work? core->rate isn't assigned yet when we're speculating
rates down the tree to the leaves. So that clk_recalc() in the above
hunk would need to save the rate away, which is wrong because it isn't
changed yet, for this line to make sense.

Given that I had to read this for a few minutes to figure this out it
seems that trying to combine the parent and the rate as arguments is
actually more complicated than adding another parameter. Please just add
another argument.

>                 if (ret & NOTIFY_STOP_MASK)
>                         break;
>         }