Messages in this thread | | | From | Jerome Brunet <> | Subject | Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase() | Date | Wed, 02 Oct 2019 10:31:46 +0200 |
| |
On Tue 01 Oct 2019 at 19:44, Stephen Boyd <sboyd@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 <dianders@chromium.org> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Signed-off-by: Stephen Boyd <sboyd@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.
| |