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

[Jerome Brunet]

On Tue 01 Oct 2019 at 19:44, 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.

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 <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.
>
>  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.