lkml.org 
[lkml]   [2020]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
From
Date
Quoting Doug Anderson (2019-10-01 14:20:50)
> 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().

Ok. I bail out early with return 0 now.

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

Thanks.

>
>
> > @@ -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?

Not really, we already hold the lock when this function is called so
locking it again is not useful.

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

Ok.

\
 
 \ /
  Last update: 2020-01-05 08:54    [W:0.339 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site