[PATCH] clk: emit warning if fail to get parent clk

[PATCH] clk: emit warning if fail to get parent clk

[Jun Nie]

Emit warning if fail to get parent clk to expose potential issue earlier.
For example, clk_hw_get_rate() will return 0 for a clock without parent core
while parent number is not zero. This cause opp always think it is switching
frequency from 0 to some other frequency. Crash may happen if we switch
from high frequency to low frequency and lower CPU voltage before clk rate
switching.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/clk/clk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1a27e99ccb17..78b21b888e56 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -424,6 +424,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
 {
 	struct clk_parent_map *entry = &core->parents[index];
 	struct clk_core *parent = ERR_PTR(-ENOENT);
+	int emit_warn = 0;
 
 	if (entry->hw) {
 		parent = entry->hw->core;
@@ -443,6 +444,12 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
 	/* Only cache it if it's not an error */
 	if (!IS_ERR(parent))
 		entry->core = parent;
+	else if (parent != ERR_PTR(-EPROBE_DEFER))
+		emit_warn = 1;
+
+	if (emit_warn || (!parent && core->num_parents))
+		pr_warn("Fail to get indexed %d parent for clk %s.",
+			index, core->name);
 }
 
 static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
-- 
2.17.1

Re: [PATCH] clk: emit warning if fail to get parent clk

[Stephen Boyd]

Quoting Jun Nie (2020-09-28 01:47:44)
> Emit warning if fail to get parent clk to expose potential issue earlier.
> For example, clk_hw_get_rate() will return 0 for a clock without parent core
> while parent number is not zero. This cause opp always think it is switching
> frequency from 0 to some other frequency. Crash may happen if we switch
> from high frequency to low frequency and lower CPU voltage before clk rate
> switching.

Thanks for the background reasoning. It's good to know what the problem
is. Is there any way to change OPP so it can handle this scenario
better?

> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/clk/clk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1a27e99ccb17..78b21b888e56 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -424,6 +424,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
>  {
>         struct clk_parent_map *entry = &core->parents[index];
>         struct clk_core *parent = ERR_PTR(-ENOENT);
> +       int emit_warn = 0;
>  
>         if (entry->hw) {
>                 parent = entry->hw->core;
> @@ -443,6 +444,12 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
>         /* Only cache it if it's not an error */
>         if (!IS_ERR(parent))
>                 entry->core = parent;
> +       else if (parent != ERR_PTR(-EPROBE_DEFER))
> +               emit_warn = 1;
> +
> +       if (emit_warn || (!parent && core->num_parents))
> +               pr_warn("Fail to get indexed %d parent for clk %s.",
> +                       index, core->name);

How do we know that this error isn't because the parent hasn't been
probed yet?

Re: [PATCH] clk: emit warning if fail to get parent clk

[Jun Nie]

Stephen Boyd <sboyd@kernel.org> 于2020年10月8日周四 上午7:53写道：
>
> Quoting Jun Nie (2020-09-28 01:47:44)
> > Emit warning if fail to get parent clk to expose potential issue earlier.
> > For example, clk_hw_get_rate() will return 0 for a clock without parent core
> > while parent number is not zero. This cause opp always think it is switching
> > frequency from 0 to some other frequency. Crash may happen if we switch
> > from high frequency to low frequency and lower CPU voltage before clk rate
> > switching.
>
> Thanks for the background reasoning. It's good to know what the problem
> is. Is there any way to change OPP so it can handle this scenario
> better?

Maybe we can save latest CPU voltage and lower the voltage per opp voltage
requirement instead of clk rate requirement.
>
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> >  drivers/clk/clk.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 1a27e99ccb17..78b21b888e56 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -424,6 +424,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
> >  {
> >         struct clk_parent_map *entry = &core->parents[index];
> >         struct clk_core *parent = ERR_PTR(-ENOENT);
> > +       int emit_warn = 0;
> >
> >         if (entry->hw) {
> >                 parent = entry->hw->core;
> > @@ -443,6 +444,12 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
> >         /* Only cache it if it's not an error */
> >         if (!IS_ERR(parent))
> >                 entry->core = parent;
> > +       else if (parent != ERR_PTR(-EPROBE_DEFER))
> > +               emit_warn = 1;
> > +
> > +       if (emit_warn || (!parent && core->num_parents))
> > +               pr_warn("Fail to get indexed %d parent for clk %s.",
> > +                       index, core->name);
>
> How do we know that this error isn't because the parent hasn't been
> probed yet?

Yes, this is chance that failure to probe parent also cause the error.
But I had thought there may
be unnecessary error message before the successful probing, though I
did not see it. If you think
it does not a matter, next version can emit warning on all error.

Jun

Re: [PATCH] clk: emit warning if fail to get parent clk

[Stephen Boyd]

Quoting Jun Nie (2020-10-08 20:08:13)
> Stephen Boyd <sboyd@kernel.org> 于2020年10月8日周四 上午7:53写道：
> >
> > How do we know that this error isn't because the parent hasn't been
> > probed yet?
> 
> Yes, this is chance that failure to probe parent also cause the error.
> But I had thought there may
> be unnecessary error message before the successful probing, though I
> did not see it. If you think
> it does not a matter, next version can emit warning on all error.
> 

I suspect this warning can't be emitted here because we don't know if
the parent hasn't probed or if something else is wrong. It would be
better to fix OPP from what I can tell.