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

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

[Stephen Boyd]

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.

 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);
 
 	/*
 	 * Set clk's duty cycle.
-- 
Sent by a computer through tubes

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

[Doug Anderson]

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);

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

[Stephen Boyd]

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.

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

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

[Stephen Boyd]

(Sorry I'm way behind on emails)

Quoting Jerome Brunet (2019-10-02 01:31:46)
> 
> 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.

Yes both are taking the lock again when we're already holding the lock.
It is wasteful. I'll send another patch with the series to make those
calls in debugfs use the nolock variants. That will open up the question
of how we sometimes recalc rates and other times don't depending on if
the nolock or lock variant of the get_rate() API is used.

> 
> So I don't get why clk_get_phase() should do thing differently from the
> others, and not take the lock ?

Got it.

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

What error?

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

[Stephen Boyd]

Quoting Stephen Boyd (2020-01-04 23:50:49)
> 
> Quoting Jerome Brunet (2019-10-02 01:31:46)
> > >
> > > +     clk_core_get_phase(core);
> > 
> > Should the error be checked here as well ?
> 
> What error?
> 

Ah the error when clk_ops::get_phase() returns an error? I guess we
should just silently ignore it to maintain the previous behavior? Or we
can bail out of clk registration. Seems low risk to do that in another
patch.

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

[Jerome Brunet]

On Sun 05 Jan 2020 at 08:55, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Stephen Boyd (2020-01-04 23:50:49)
>> 
>> Quoting Jerome Brunet (2019-10-02 01:31:46)
>> > >
>> > > +     clk_core_get_phase(core);
>> > 
>> > Should the error be checked here as well ?
>> 
>> What error?
>> 
>
> Ah the error when clk_ops::get_phase() returns an error? I guess we
> should just silently ignore it to maintain the previous behavior?

Indeed, that's the previous behavior so we can keep it.
I'm just not a fan of silently ignoring errors. These choices tend to
come back to haunt us ...

> Or we
> can bail out of clk registration. Seems low risk to do that in another
> patch.

Why not, or at least a warning so we get a hint that something is wrong.