On Tue 01 Oct 2019 at 19:44, Stephen Boyd <sb...@kernel.org> 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.
Function already under the lock seem to be marked with "_nolock"
Maybe one should added for get_phase() ?
Also the debugfs side calls clk_core_get_rate() and
clk_core_get_accuracy(). Both are taking the prepare_lock.
So I don't get why clk_get_phase() should do thing differently from the
others, and not take the lock ?
>
> Cc: Douglas Anderson <diand...@chromium.org>
> Cc: Heiko Stuebner <he...@sntech.de>
> Cc: Jerome Brunet <jbru...@baylibre.com>
> Signed-off-by: Stephen Boyd <sb...@kernel.org>
> ---
>
> Resending because I couldn't find this anywhere.
>
> drivers/clk/clk.c | 44 +++++++++++++++++++++++++++++---------------
> 1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1c677d7f7f53..16add5626dfa 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -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;
>
> return ret;
> }
> @@ -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();
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(clk_get_phase);
>
> @@ -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);
> + if (phase >= 0)
> + seq_printf(s, "%5d", phase);
> + else
> + seq_puts(s, "-----");
> +
> + seq_printf(s, " %6d\n", clk_core_get_scaled_duty_cycle(c, 100000));
> }
>
> static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
> @@ -2921,6 +2935,7 @@ DEFINE_SHOW_ATTRIBUTE(clk_summary);
>
> static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
> {
> + int phase;
> unsigned long min_rate, max_rate;
>
> clk_core_get_boundaries(c, &min_rate, &max_rate);
> @@ -2934,7 +2949,9 @@ static void clk_dump_one(struct seq_file *s, struct
> clk_core *c, int level)
> seq_printf(s, "\"min_rate\": %lu,", min_rate);
> seq_printf(s, "\"max_rate\": %lu,", max_rate);
> seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
> - seq_printf(s, "\"phase\": %d,", clk_core_get_phase(c));
> + phase = clk_core_get_phase(c);
> + if (phase >= 0)
> + seq_printf(s, "\"phase\": %d,", phase);
> seq_printf(s, "\"duty_cycle\": %u",
> clk_core_get_scaled_duty_cycle(c, 100000));
> }
> @@ -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.
> */
> - if (core->ops->get_phase)
> - core->phase = core->ops->get_phase(core->hw);
> - else
> - core->phase = 0;
> + clk_core_get_phase(core);
Should the error be checked here as well ?
>
> /*
> * Set clk's duty cycle.
