Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()

[Doug Anderson]

Hi,

On Tue, Oct 1, 2019 at 10:44 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> We don't check for errors from clk_ops::get_phase() before storing away
> the result into the clk_core::phase member. This can lead to some fairly
> confusing debugfs information if these ops do return an error. Let's
> skip the store when this op fails to fix this. While we're here, move
> the locking outside of clk_core_get_phase() to simplify callers from
> the debugfs side.
>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Heiko Stuebner <heiko@xxxxxxxxx>
> Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> ---
>
> Resending because I couldn't find this anywhere.

It was at:

https://lore.kernel.org/r/155692148370.12939.291938595926908281@xxxxxxxxxxxxxxxxxxxxxxxxxx


> @@ -2640,14 +2640,14 @@ EXPORT_SYMBOL_GPL(clk_set_phase);
>
>  static int clk_core_get_phase(struct clk_core *core)
>  {
> -       int ret;
> +       int ret = 0;
>
> -       clk_prepare_lock();
> +       lockdep_assert_held(&prepare_lock);
>         /* Always try to update cached phase if possible */
>         if (core->ops->get_phase)
> -               core->phase = core->ops->get_phase(core->hw);
> -       ret = core->phase;
> -       clk_prepare_unlock();
> +               ret = core->ops->get_phase(core->hw);
> +       if (ret >= 0)
> +               core->phase = ret;

It doesn't matter much, but if it were me I'd add this under the "if
(core->ops->get_phase)" statement.  Then we don't keep doing a memory
write of 0 to "core->phase" all the time when "core->ops->get_phase"
isn't there.  ...plus (to me) it makes more logical sense.

I'd guess you were trying to make sure that core->phase got set to 0
like the old code did in __clk_core_init().  ...but that really
shouldn't be needed since the clk_core is initted with kzalloc().


> @@ -2661,10 +2661,16 @@ static int clk_core_get_phase(struct clk_core *core)
>   */
>  int clk_get_phase(struct clk *clk)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>
> -       return clk_core_get_phase(clk->core);
> +       clk_prepare_unlock();
> +       ret = clk_core_get_phase(clk->core);
> +       clk_prepare_unlock();

Probably the first of these two should be clk_prepare_lock() unless
you really really wanted the clock to be unlocked.


> @@ -2878,13 +2884,21 @@ static struct hlist_head *orphan_list[] = {
>  static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
>                                  int level)
>  {
> -       seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu %5d %6d\n",
> +       int phase;
> +
> +       seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu ",
>                    level * 3 + 1, "",
>                    30 - level * 3, c->name,
>                    c->enable_count, c->prepare_count, c->protect_count,
> -                  clk_core_get_rate(c), clk_core_get_accuracy(c),
> -                  clk_core_get_phase(c),
> -                  clk_core_get_scaled_duty_cycle(c, 100000));
> +                  clk_core_get_rate(c), clk_core_get_accuracy(c));
> +
> +       phase = clk_core_get_phase(c);

Don't you need a clk_prepare_lock() / clk_prepare_unlock() around this now?


> @@ -3349,10 +3366,7 @@ static int __clk_core_init(struct clk_core *core)
>          * Since a phase is by definition relative to its parent, just
>          * query the current clock phase, or just assume it's in phase.

Maybe update the comment to something like "clk_core_get_phase() will
cache the phase for us".


>          */
> -       if (core->ops->get_phase)
> -               core->phase = core->ops->get_phase(core->hw);
> -       else
> -               core->phase = 0;
> +       clk_core_get_phase(core);