Messages in this thread | | | From | Doug Anderson <> | Date | Tue, 1 Oct 2019 14:20:50 -0700 | Subject | Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase() |
| |
Hi,
On Tue, Oct 1, 2019 at 10:44 AM 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> > --- > > Resending because I couldn't find this anywhere.
It was at:
https://lore.kernel.org/r/155692148370.12939.291938595926908281@swboyd.mtv.corp.google.com
> @@ -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);
| |