Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] clk: Don't cache errors from clk_ops::get_phase()
@ 2019-10-01 17:44 Stephen Boyd
  2019-10-01 21:20 ` Doug Anderson
  2019-10-02  8:31 ` Jerome Brunet
  0 siblings, 2 replies; 7+ messages in thread
From: Stephen Boyd @ 2019-10-01 17:44 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, Douglas Anderson, Heiko Stuebner, Jerome Brunet

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
  2019-10-01 17:44 [PATCH] clk: Don't cache errors from clk_ops::get_phase() Stephen Boyd
@ 2019-10-01 21:20 ` Doug Anderson
  2020-01-05  7:53   ` Stephen Boyd
  2019-10-02  8:31 ` Jerome Brunet
  1 sibling, 1 reply; 7+ messages in thread
From: Doug Anderson @ 2019-10-01 21:20 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, LKML, linux-clk, Heiko Stuebner, Jerome Brunet

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
  2019-10-01 17:44 [PATCH] clk: Don't cache errors from clk_ops::get_phase() Stephen Boyd
  2019-10-01 21:20 ` Doug Anderson
@ 2019-10-02  8:31 ` Jerome Brunet
  2020-01-05  7:50   ` Stephen Boyd
  1 sibling, 1 reply; 7+ messages in thread
From: Jerome Brunet @ 2019-10-02  8:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, linux-kernel, linux-clk, Douglas Anderson,
	Heiko Stuebner, 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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
  2019-10-02  8:31 ` Jerome Brunet
@ 2020-01-05  7:50   ` Stephen Boyd
  2020-01-05  7:55     ` Stephen Boyd
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2020-01-05  7:50 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Michael Turquette, linux-kernel, linux-clk, Douglas Anderson,
	Heiko Stuebner, Jerome Brunet

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
  2019-10-01 21:20 ` Doug Anderson
@ 2020-01-05  7:53   ` Stephen Boyd
  0 siblings, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2020-01-05  7:53 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Michael Turquette, LKML, linux-clk, Heiko Stuebner, Jerome Brunet

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
  2020-01-05  7:50   ` Stephen Boyd
@ 2020-01-05  7:55     ` Stephen Boyd
  2020-01-07  9:44       ` Jerome Brunet
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2020-01-05  7:55 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Michael Turquette, linux-kernel, linux-clk, Douglas Anderson,
	Heiko Stuebner, Jerome Brunet

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
  2020-01-05  7:55     ` Stephen Boyd
@ 2020-01-07  9:44       ` Jerome Brunet
  0 siblings, 0 replies; 7+ messages in thread
From: Jerome Brunet @ 2020-01-07  9:44 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, linux-kernel, linux-clk, Douglas Anderson,
	Heiko Stuebner


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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 17:44 [PATCH] clk: Don't cache errors from clk_ops::get_phase() Stephen Boyd
2019-10-01 21:20 ` Doug Anderson
2020-01-05  7:53   ` Stephen Boyd
2019-10-02  8:31 ` Jerome Brunet
2020-01-05  7:50   ` Stephen Boyd
2020-01-05  7:55     ` Stephen Boyd
2020-01-07  9:44       ` Jerome Brunet

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git