linux-kernel - Re: [PATCH v5 1/2] clk: Add of_clk_get_by_name

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <153576994987.19113.11376893046599589648@swboyd.mtv.corp.google.com>
Date:   Fri, 31 Aug 2018 19:45:49 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Michael Turquette <mturquette@...libre.com>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        Russell King <linux@...linux.org.uk>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Simon Horman <horms@...ge.net.au>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Phil Edworthy <phil.edworthy@...esas.com>
Subject: Re: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function

Quoting Phil Edworthy (2018-08-31 07:07:22)
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 9ab3db8..4adb99e 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -54,30 +54,29 @@ EXPORT_SYMBOL(of_clk_get);
>  
>  static struct clk *__of_clk_get_by_name(struct device_node *np,
>                                         const char *dev_id,
> -                                       const char *name)
> +                                       const char *name,
> +                                       bool optional)
>  {
>         struct clk *clk = ERR_PTR(-ENOENT);
> +       struct device_node *child = np;
> +       int index = 0;
>  
>         /* Walk up the tree of devices looking for a clock that matches */
>         while (np) {
> -               int index = 0;
>  
>                 /*
>                  * For named clocks, first look up the name in the
>                  * "clock-names" property.  If it cannot be found, then
> -                * index will be an error code, and of_clk_get() will fail.
> +                * index will be an error code.
>                  */
>                 if (name)
>                         index = of_property_match_string(np, "clock-names", name);
> -               clk = __of_clk_get(np, index, dev_id, name);
> -               if (!IS_ERR(clk)) {
> -                       break;
> -               } else if (name && index >= 0) {
> -                       if (PTR_ERR(clk) != -EPROBE_DEFER)
> -                               pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
> -                                       np, name ? name : "", index);
> +               if (index >= 0)
> +                       clk = __of_clk_get(np, index, dev_id, name);
> +               if (!IS_ERR(clk))

Was this change necessary? It looks like we can leave it all alone and
keep passing a negative number to __of_clk_get() and have that return an
error pointer which we then return immediately as an error. But, if the
clock is optional and we've passed a name here, shouldn't we treat an
error from of_property_match_string() as success too? This is all
looking pretty fragile so maybe it can be better commented and also more
explicit instead of relying on the reader to jump through all the
function calls to figure out what the return value is in some cases.

>                         return clk;
> -               }
> +               if (name && index >= 0)
> +                       break;

And this causes us to duplicate logic down below because we have to
check it again if it's not optional or some other error condition?

>  
>                 /*
>                  * No matching clock found on this node.  If the parent node
> @@ -89,6 +88,16 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
>                         break;
>         }
>  
> +       /* The clock is not valid, but it could be optional or deferred */
> +       if (optional && PTR_ERR(clk) == -ENOENT) {
> +               clk = NULL;
> +               pr_info("no optional clock %pOF:%s\n", child,
> +                       name ? name : "");

Is this intentionally pr_info?

> +       } else if (name && index >= 0 && PTR_ERR(clk) != -EPROBE_DEFER) {
> +               pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
> +                       child, name, index);
> +       }
> +
>         return clk;
>  }
>