Messages in this thread | | | From | Jerome Brunet <> | Subject | Re: [PATCH v2 1/4] clk: Don't cache errors from clk_ops::get_phase() | Date | Thu, 06 Feb 2020 09:26:06 +0100 |
| |
On Thu 06 Feb 2020 at 00:27, 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. > > 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> > --- > drivers/clk/clk.c | 48 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 16 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index d529ad67805c..26213e82f5f9 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2660,12 +2660,14 @@ static int clk_core_get_phase(struct clk_core *core) > { > int ret; > > - clk_prepare_lock();
Should the function name get the "_nolock" suffix then ?
> + lockdep_assert_held(&prepare_lock); > + if (!core->ops->get_phase) > + return 0; > + > /* 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; > } > @@ -2679,10 +2681,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_lock(); > + ret = clk_core_get_phase(clk->core); > + clk_prepare_unlock(); > + > + return ret; > } > EXPORT_SYMBOL_GPL(clk_get_phase); > > @@ -2896,13 +2904,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, > @@ -2939,6 +2955,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); > @@ -2952,7 +2969,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)); > } > @@ -3434,14 +3453,11 @@ static int __clk_core_init(struct clk_core *core) > core->accuracy = 0; > > /* > - * Set clk's phase. > + * Set clk's phase by clk_core_get_phase() caching the phase. > * 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); > > /* > * Set clk's duty cycle.
| |