[PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Sakari Ailus]

While holding a reference to a device_node it is allowed to put that node
without holding devtree_lock spinlock. Move of_node_put() after releasing
the spinlock.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/of/base.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d4bea3c..a8d7fed 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -286,8 +286,8 @@ struct device_node *of_find_all_nodes(struct device_node *prev)
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np = __of_find_all_nodes(prev);
 	of_node_get(np);
-	of_node_put(prev);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(prev);
 	return np;
 }
 EXPORT_SYMBOL(of_find_all_nodes);
@@ -660,8 +660,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	parent = of_node_get(node->parent);
-	of_node_put(node);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(node);
 	return parent;
 }
 EXPORT_SYMBOL(of_get_next_parent);
@@ -732,8 +732,8 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 		if (of_node_get(next))
 			break;
 	}
-	of_node_put(prev);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(prev);
 	return next;
 }
 EXPORT_SYMBOL(of_get_next_available_child);
@@ -875,8 +875,8 @@ struct device_node *of_find_node_by_name(struct device_node *from,
 		if (np->name && (of_node_cmp(np->name, name) == 0)
 		    && of_node_get(np))
 			break;
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(from);
 	return np;
 }
 EXPORT_SYMBOL(of_find_node_by_name);
@@ -904,8 +904,8 @@ struct device_node *of_find_node_by_type(struct device_node *from,
 		if (np->type && (of_node_cmp(np->type, type) == 0)
 		    && of_node_get(np))
 			break;
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(from);
 	return np;
 }
 EXPORT_SYMBOL(of_find_node_by_type);
@@ -935,8 +935,8 @@ struct device_node *of_find_compatible_node(struct device_node *from,
 		if (__of_device_is_compatible(np, compatible, type, NULL) &&
 		    of_node_get(np))
 			break;
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(from);
 	return np;
 }
 EXPORT_SYMBOL(of_find_compatible_node);
@@ -970,8 +970,8 @@ struct device_node *of_find_node_with_property(struct device_node *from,
 		}
 	}
 out:
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(from);
 	return np;
 }
 EXPORT_SYMBOL(of_find_node_with_property);
@@ -1051,8 +1051,8 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
 			break;
 		}
 	}
-	of_node_put(from);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	of_node_put(from);
 	return np;
 }
 EXPORT_SYMBOL(of_find_matching_node_and_match);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Rob Herring]

On Wed, Feb 22, 2017 at 3:29 AM, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> While holding a reference to a device_node it is allowed to put that node
> without holding devtree_lock spinlock. Move of_node_put() after releasing
> the spinlock.

True, but why do you really care? It is not a heavyweight operation.

Rob

>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/of/base.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c..a8d7fed 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -286,8 +286,8 @@ struct device_node *of_find_all_nodes(struct device_node *prev)
>         raw_spin_lock_irqsave(&devtree_lock, flags);
>         np = __of_find_all_nodes(prev);
>         of_node_get(np);
> -       of_node_put(prev);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(prev);
>         return np;
>  }
>  EXPORT_SYMBOL(of_find_all_nodes);
> @@ -660,8 +660,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
>
>         raw_spin_lock_irqsave(&devtree_lock, flags);
>         parent = of_node_get(node->parent);
> -       of_node_put(node);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(node);
>         return parent;
>  }
>  EXPORT_SYMBOL(of_get_next_parent);
> @@ -732,8 +732,8 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
>                 if (of_node_get(next))
>                         break;
>         }
> -       of_node_put(prev);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(prev);
>         return next;
>  }
>  EXPORT_SYMBOL(of_get_next_available_child);
> @@ -875,8 +875,8 @@ struct device_node *of_find_node_by_name(struct device_node *from,
>                 if (np->name && (of_node_cmp(np->name, name) == 0)
>                     && of_node_get(np))
>                         break;
> -       of_node_put(from);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(from);
>         return np;
>  }
>  EXPORT_SYMBOL(of_find_node_by_name);
> @@ -904,8 +904,8 @@ struct device_node *of_find_node_by_type(struct device_node *from,
>                 if (np->type && (of_node_cmp(np->type, type) == 0)
>                     && of_node_get(np))
>                         break;
> -       of_node_put(from);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(from);
>         return np;
>  }
>  EXPORT_SYMBOL(of_find_node_by_type);
> @@ -935,8 +935,8 @@ struct device_node *of_find_compatible_node(struct device_node *from,
>                 if (__of_device_is_compatible(np, compatible, type, NULL) &&
>                     of_node_get(np))
>                         break;
> -       of_node_put(from);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(from);
>         return np;
>  }
>  EXPORT_SYMBOL(of_find_compatible_node);
> @@ -970,8 +970,8 @@ struct device_node *of_find_node_with_property(struct device_node *from,
>                 }
>         }
>  out:
> -       of_node_put(from);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(from);
>         return np;
>  }
>  EXPORT_SYMBOL(of_find_node_with_property);
> @@ -1051,8 +1051,8 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
>                         break;
>                 }
>         }
> -       of_node_put(from);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +       of_node_put(from);
>         return np;
>  }
>  EXPORT_SYMBOL(of_find_matching_node_and_match);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Frank Rowand]

Adding Rob.

On 02/22/17 01:29, Sakari Ailus wrote:
> While holding a reference to a device_node it is allowed to put that node
> without holding devtree_lock spinlock. Move of_node_put() after releasing
> the spinlock.

Please explain why the change fixes or improves the code.  In other words,
what is the point of the change?

Please use getmaintainer to find who to send the patch to.

-Frank


> 
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/of/base.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c..a8d7fed 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -286,8 +286,8 @@ struct device_node *of_find_all_nodes(struct device_node *prev)
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	np = __of_find_all_nodes(prev);
>  	of_node_get(np);
> -	of_node_put(prev);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(prev);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_all_nodes);

< snip >

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Sakari Ailus]

Hi Frank,

Frank Rowand wrote:
> Adding Rob.
>
> On 02/22/17 01:29, Sakari Ailus wrote:
>> While holding a reference to a device_node it is allowed to put that node
>> without holding devtree_lock spinlock. Move of_node_put() after releasing
>> the spinlock.
>
> Please explain why the change fixes or improves the code.  In other words,
> what is the point of the change?

The spinlock is simply not needed, that's all. I'd say generally moving 
code that does not require serialising out of a serialised section does 
improve the code, sometimes also the readability of the code, especially 
if it doesn't get more complicated as a result.

>
> Please use getmaintainer to find who to send the patch to.

I'll do that the next time.

-- 
Kind regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Frank Rowand]

On 02/22/17 01:29, Sakari Ailus wrote:
> While holding a reference to a device_node it is allowed to put that node
> without holding devtree_lock spinlock. Move of_node_put() after releasing
> the spinlock.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

I would rather leave this code the way it is.

The change is a micro-optimization that is not going to have any real
impact on performance.

The change also makes the code less clear and readable.  (Not
significantly, but very slightly).  THe code in the current form
slightly emphasizes the balancing of gets and puts.  I know that
this is nit picking, but so be it.

Thus the change is on balance code churn.

-Frank

> ---
>  drivers/of/base.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c..a8d7fed 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -286,8 +286,8 @@ struct device_node *of_find_all_nodes(struct device_node *prev)
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	np = __of_find_all_nodes(prev);
>  	of_node_get(np);
> -	of_node_put(prev);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(prev);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_all_nodes);
> @@ -660,8 +660,8 @@ struct device_node *of_get_next_parent(struct device_node *node)
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	parent = of_node_get(node->parent);
> -	of_node_put(node);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(node);
>  	return parent;
>  }
>  EXPORT_SYMBOL(of_get_next_parent);
> @@ -732,8 +732,8 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
>  		if (of_node_get(next))
>  			break;
>  	}
> -	of_node_put(prev);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(prev);
>  	return next;
>  }
>  EXPORT_SYMBOL(of_get_next_available_child);
> @@ -875,8 +875,8 @@ struct device_node *of_find_node_by_name(struct device_node *from,
>  		if (np->name && (of_node_cmp(np->name, name) == 0)
>  		    && of_node_get(np))
>  			break;
> -	of_node_put(from);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(from);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_node_by_name);
> @@ -904,8 +904,8 @@ struct device_node *of_find_node_by_type(struct device_node *from,
>  		if (np->type && (of_node_cmp(np->type, type) == 0)
>  		    && of_node_get(np))
>  			break;
> -	of_node_put(from);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(from);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_node_by_type);
> @@ -935,8 +935,8 @@ struct device_node *of_find_compatible_node(struct device_node *from,
>  		if (__of_device_is_compatible(np, compatible, type, NULL) &&
>  		    of_node_get(np))
>  			break;
> -	of_node_put(from);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(from);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_compatible_node);
> @@ -970,8 +970,8 @@ struct device_node *of_find_node_with_property(struct device_node *from,
>  		}
>  	}
>  out:
> -	of_node_put(from);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(from);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_node_with_property);
> @@ -1051,8 +1051,8 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
>  			break;
>  		}
>  	}
> -	of_node_put(from);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	of_node_put(from);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_matching_node_and_match);
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Rob Herring]

On Mon, Feb 27, 2017 at 8:54 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 02/22/17 01:29, Sakari Ailus wrote:
>> While holding a reference to a device_node it is allowed to put that node
>> without holding devtree_lock spinlock. Move of_node_put() after releasing
>> the spinlock.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>
> I would rather leave this code the way it is.
>
> The change is a micro-optimization that is not going to have any real
> impact on performance.

I have the same feelings.

>
> The change also makes the code less clear and readable.  (Not
> significantly, but very slightly).  THe code in the current form
> slightly emphasizes the balancing of gets and puts.  I know that
> this is nit picking, but so be it.
>
> Thus the change is on balance code churn.
>
> -Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] devicetree: of_node_put() does not require holding devtree_lock

[Sakari Ailus]

Hi Rob and Frank,

Rob Herring wrote:
> On Mon, Feb 27, 2017 at 8:54 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 02/22/17 01:29, Sakari Ailus wrote:
>>> While holding a reference to a device_node it is allowed to put that node
>>> without holding devtree_lock spinlock. Move of_node_put() after releasing
>>> the spinlock.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>
>> I would rather leave this code the way it is.
>>
>> The change is a micro-optimization that is not going to have any real
>> impact on performance.
>
> I have the same feelings.

Fair enough. Feel free to ignore the patch then.

-- 
Kind regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html