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

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

[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()")
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.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;
-- 
2.27.0

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

[Dmitry Osipenko]

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@linaro.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:

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

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

[Sam Protsenko]

Hi,

On Thu, 10 Dec 2020 at 22:59, Dmitry Osipenko <digetx@gmail.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@linaro.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