linux-kernel - Re: [RFC PATCH 01/13] of: Remove comments that state the obvious

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 27 Oct 2016 07:18:01 -0500
From:   Rob Herring <robh+dt@...nel.org>
To:     Frank Rowand <frowand.list@...il.com>
Cc:     Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Pantelis Antoniou <panto@...oniou-consulting.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 01/13] of: Remove comments that state the obvious

On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@...il.com> wrote:
> From: Frank Rowand <frank.rowand@...sony.com>
>
> Remove comments that state the obvious, to reduce clutter

I'm probably not the best reviewer, have you ever seen a comment in my code. :)

>
> Signed-off-by: Frank Rowand <frank.rowand@...sony.com>
> ---
>  drivers/of/resolver.c | 31 ++-----------------------------
>  1 file changed, 2 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 46325d6394cf..4ff0220d7aa2 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c

> @@ -75,8 +73,6 @@ static phandle of_get_tree_max_phandle(void)
>
>  /*
>   * Adjust a subtree's phandle values by a given delta.
> - * Makes sure not to just adjust the device node's phandle value,
> - * but modify the phandle properties values as well.

What's missing here is the why? Why do we adjust phandle values?

>   */
>  static void __of_adjust_tree_phandles(struct device_node *node,
>                 int phandle_delta)
> @@ -85,32 +81,25 @@ static void __of_adjust_tree_phandles(struct device_node *node,
>         struct property *prop;
>         phandle phandle;
>
> -       /* first adjust the node's phandle direct value */

Seems somewhat useful.

>         if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>                 node->phandle += phandle_delta;
>
> -       /* now adjust phandle & linux,phandle values */

Seems somewhat useful.

>         for_each_property_of_node(node, prop) {
>
> -               /* only look for these two */
>                 if (of_prop_cmp(prop->name, "phandle") != 0 &&
>                     of_prop_cmp(prop->name, "linux,phandle") != 0)
>                         continue;
>
> -               /* must be big enough */
>                 if (prop->length < 4)
>                         continue;
>
> -               /* read phandle value */
>                 phandle = be32_to_cpup(prop->value);
> -               if (phandle == OF_PHANDLE_ILLEGAL)      /* unresolved */
> +               if (phandle == OF_PHANDLE_ILLEGAL)
>                         continue;
>
> -               /* adjust */
>                 *(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>         }
>
> -       /* now do the children recursively */
>         for_each_child_of_node(node, child)
>                 __of_adjust_tree_phandles(child, phandle_delta);
>  }

> @@ -254,7 +239,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
>
>                 for (i = 0; i < count; i++) {
>                         off = be32_to_cpu(((__be32 *)rprop->value)[i]);
> -                       /* make sure the offset doesn't overstep (even wrap) */

Seems somewhat useful.

>                         if (off >= sprop->length ||
>                                         (off + 4) > sprop->length) {
>                                 pr_err("%s: Illegal property '%s' @%s\n",

> @@ -349,10 +328,8 @@ int of_resolve_phandles(struct device_node *resolve)
>         resolve_sym = NULL;
>         resolve_fix = NULL;
>
> -       /* this may fail (if no fixups are required) */

Seem somewhat useful.

>         root_sym = of_find_node_by_path("/__symbols__");
>
> -       /* locate the symbols & fixups nodes on resolve */
>         for_each_child_of_node(resolve, child) {
>
>                 if (!resolve_sym &&