linux-kernel - Re: [PATCH 1/2] of: property: Get rid of code duplication in port getting

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] [day] [month] [year] [list]

Message-ID: <CAPLW+4nLi2236-EL0d=X9bzv11LiBJd2c5NY5aY4qZHL3rUCgw@mail.gmail.com>
Date:   Fri, 11 Dec 2020 15:26:54 +0200
From:   Sam Protsenko <semen.protsenko@...aro.org>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        John Stultz <john.stultz@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH 1/2] of: property: Get rid of code duplication in port getting

Hi,

On Thu, 10 Dec 2020 at 22:59, Dmitry Osipenko <digetx@...il.com> wrote:
>
> 10.12.2020 23:29, Sam Protsenko пишет:
> > Both of_graph_is_present() and of_graph_get_next_endpoint() functions
> > share common piece of code for obtaining the graph port. Extract it into
> > separate static function to get rid of code duplication and avoid
> > possible coding errors in future.
> >
> > Fixes: 4ec0a44ba8d7 ("of_graph: add of_graph_is_present()")
>
> The "fixes" tag should be used only for bug-fixes and there is no bug
> fixed in this patch.
>
> > Signed-off-by: Sam Protsenko <semen.protsenko@...aro.org>
> > ---
> >  drivers/of/property.c | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 408a7b5f06a9..da111fcf37ac 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -30,13 +30,13 @@
> >  #include "of_private.h"
> >
> >  /**
> > - * of_graph_is_present() - check graph's presence
> > + * of_graph_get_port - find the "port" node in a given node
> >   * @node: pointer to device_node containing graph port
> >   *
> > - * Return: True if @node has a port or ports (with a port) sub-node,
> > - * false otherwise.
> > + * Return: A 'port' node pointer with refcount incremented if found or NULL
> > + * otherwise. The caller has to use of_node_put() on it when done.
> >   */
> > -bool of_graph_is_present(const struct device_node *node)
> > +static struct device_node *of_graph_get_port(const struct device_node *node)
> >  {
> >       struct device_node *ports, *port;
> >
> > @@ -46,8 +46,22 @@ bool of_graph_is_present(const struct device_node *node)
> >
> >       port = of_get_child_by_name(node, "port");
> >       of_node_put(ports);
> > -     of_node_put(port);
> >
> > +     return port;
> > +}
> > +
> > +/**
> > + * of_graph_is_present() - check graph's presence
> > + * @node: pointer to device_node containing graph port
> > + *
> > + * Return: True if @node has a port or ports (with a port) sub-node,
> > + * false otherwise.
> > + */
> > +bool of_graph_is_present(const struct device_node *node)
> > +{
> > +     struct device_node *port = of_graph_get_port(node);
> > +
> > +     of_node_put(port);
> >       return !!port;
> >  }
> >  EXPORT_SYMBOL(of_graph_is_present);
> > @@ -631,15 +645,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> >        * parent port node.
> >        */
> >       if (!prev) {
> > -             struct device_node *node;
> > -
> > -             node = of_get_child_by_name(parent, "ports");
> > -             if (node)
> > -                     parent = node;
> > -
> > -             port = of_get_child_by_name(parent, "port");
> > -             of_node_put(node);
> > -
> > +             port = of_graph_get_port(parent);
> >               if (!port) {
> >                       pr_err("graph: no port node found in %pOF\n", parent);
> >                       return NULL;
> >
>
> This repeats the problem which was made once before:
>

You are right. Inlining is probably the best solution here. Let's drop
this patch and keep everything as is. Thanks for catching this!

> https://lore.kernel.org/patchwork/patch/1266028/#1461493