Re: [PATCH] clk: Fix falling back to legacy parent string matching

[Stephen Boyd]

Quoting Stephen Boyd (2019-08-01 10:12:09)
> Calls to clk_core_get() will return ERR_PTR(-EINVAL) if we've started
> migrating a clk driver to use the DT based style of specifying parents
> but we haven't made any DT updates yet. This happens when we pass a
> non-NULL value as the 'name' argument of of_parse_clkspec(). That
> function returns -EINVAL in such a situation, instead of -ENOENT like we
> expected. The return value comes back up to clk_core_fill_parent_index()
> which proceeds to skip calling clk_core_lookup() because the error
> pointer isn't equal to -ENOENT, it's -EINVAL.
> 
> Furthermore, we'll blindly overwrite the error pointer returned by
> clk_core_get() with NULL when there isn't a legacy .name member
> specified in the parent map. This isn't too bad right now because we
> don't really care to differentiate NULL from an error, but in the future
> we should only try to do a legacy lookup if we know we might find
> something so that DT lookups that fail don't try to lookup based on
> strings when there isn't any string to match, hiding the error.
> 
> Fix both these problems so that clk provider drivers can use the new
> style of parent mapping without having to also update their DT at the
> same time. This patch is based on an earlier patch from Taniya Das which
> checked for -EINVAL in addition to -ENOENT return values.
> 
> Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> Cc: Taniya Das <tdas@xxxxxxxxxxxxxx>
> Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> Reported-by: Taniya Das <tdas@xxxxxxxxxxxxxx>
> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> ---
>  drivers/clk/clk.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..6587a70c271c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -355,8 +355,9 @@ static struct clk_core *clk_core_lookup(const char *name)
>   *      };
>   *
>   * Returns: -ENOENT when the provider can't be found or the clk doesn't
> - * exist in the provider. -EINVAL when the name can't be found. NULL when the
> - * provider knows about the clk but it isn't provided on this system.
> + * exist in the provider or the name can't be found in the DT node or
> + * in a clkdev lookup. NULL when the provider knows about the clk but it
> + * isn't provided on this system.
>   * A valid clk_core pointer when the clk can be found in the provider.
>   */
>  static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
> @@ -374,9 +375,9 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
>         /*
>          * If the DT search above couldn't find the provider or the provider
>          * didn't know about this clk, fallback to looking up via clkdev based
> -        * clk_lookups
> +        * clk_lookups.
>          */
> -       if (PTR_ERR(hw) == -ENOENT && name)
> +       if (IS_ERR(hw) && name)
>                 hw = clk_find_hw(dev_id, name);

I thought about this some more. I think we need to call
of_parse_clkspec() in clk_core_get() and only fallback to looking up in
clkdev if we can't parse the DT clock specifier. Otherwise, we'll have a
situation where the DT parsing may fail to find the clock because it
hasn't been registered yet, so it returns -EPROBE_DEFER, but then we'll
overwrite that error value with -ENOENT because clk_find_hw() will be
called.

I'll resend this with a better approach that should still fix the
original problem while making it possible for this scenario to return
errors from the clk provider lookup.

>  
>         if (IS_ERR(hw))
> @@ -401,7 +402,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
>                         parent = ERR_PTR(-EPROBE_DEFER);
>         } else {
>                 parent = clk_core_get(core, index);
> -               if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
> +               if (IS_ERR(parent) && entry->name)
>                         parent = clk_core_lookup(entry->name);
>         }