[PATCH 0/5] OF: Fixes in preperation of DT overlays

[PATCH 0/5] OF: Fixes in preperation of DT overlays

[Pantelis Antoniou]

This patchset introduces a number of fixes that are required
for the subsequent patches that add DT overlays support.

Most of them are trivial, adding small bits that are missing,
or exporting functions that were private before.

Pantelis Antoniou (5):
  OF: Clear detach flag on attach
  OF: Introduce device tree node flag helpers.
  OF: export of_property_notify
  OF: Export all DT proc update functions
  OF: Introduce utility helper functions

 drivers/of/Makefile |   2 +-
 drivers/of/base.c   |  77 ++++++++--------
 drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h  | 119 ++++++++++++++++++++++++
 4 files changed, 413 insertions(+), 38 deletions(-)
 create mode 100644 drivers/of/util.c

-- 
1.7.12

[PATCH 1/5] OF: Clear detach flag on attach

[Pantelis Antoniou]

When attaching a node always clear the detach flag. Without this change
the sequence detach, attach fails.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7d4c70f..ca10916 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
 	np->allnext = of_allnodes;
 	np->parent->child = np;
 	of_allnodes = np;
+	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	of_add_proc_dt_entry(np);
-- 
1.7.12

Re: [PATCH 1/5] OF: Clear detach flag on attach

[Gerhard Sittig]

On Tue, Nov 05, 2013 at 19:50 +0200, Pantelis Antoniou wrote:
> 
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
>  	np->allnext = of_allnodes;
>  	np->parent->child = np;
>  	of_allnodes = np;
> +	of_node_clear_flag(np, OF_DETACHED);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	of_add_proc_dt_entry(np);

Does this add a call to a routine which only gets introduced in a
subsequent patch (2/5)?  If so, it would break builds during the
series, and thus would hinder bisection.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH 1/5] OF: Clear detach flag on attach

[Pantelis Antoniou]

Hi Gerhard,

On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:

> On Tue, Nov 05, 2013 at 19:50 +0200, Pantelis Antoniou wrote:
>> 
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
>> 	np->allnext = of_allnodes;
>> 	np->parent->child = np;
>> 	of_allnodes = np;
>> +	of_node_clear_flag(np, OF_DETACHED);
>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>> 	of_add_proc_dt_entry(np);
> 
> Does this add a call to a routine which only gets introduced in a
> subsequent patch (2/5)?  If so, it would break builds during the
> series, and thus would hinder bisection.
> 

You're right, I'll re-order on the next series.

> 
> virtually yours
> Gerhard Sittig
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Regards

-- Pantelis

Re: [PATCH 1/5] OF: Clear detach flag on attach

[Alexander Sverdlin]

Hello Pantelis,

On 05/11/13 21:03, ext Pantelis Antoniou wrote:
> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
>>> 	np->allnext = of_allnodes;
>>> 	np->parent->child = np;
>>> 	of_allnodes = np;
>>> +	of_node_clear_flag(np, OF_DETACHED);
>>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>>
>>> 	of_add_proc_dt_entry(np);
>>
>> Does this add a call to a routine which only gets introduced in a
>> subsequent patch (2/5)?  If so, it would break builds during the
>> series, and thus would hinder bisection.
>>
> 
> You're right, I'll re-order on the next series.

Is it necessary at all now, after these fixes:
9e401275 of: fdt: fix memory initialization for expanded DT
0640332e of: Fix missing memory initialization on FDT unflattening
92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening

?

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 1/5] OF: Clear detach flag on attach

[Pantelis Antoniou]

Hi Alexander,

I'm not exactly sure, but I think it is still needed.
Since at that point the tree is attached.

Grant?

Regards

-- Pantelis

On Nov 6, 2013, at 10:46 AM, Alexander Sverdlin wrote:

> Hello Pantelis,
> 
> On 05/11/13 21:03, ext Pantelis Antoniou wrote:
>> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
>>>> 	np->allnext = of_allnodes;
>>>> 	np->parent->child = np;
>>>> 	of_allnodes = np;
>>>> +	of_node_clear_flag(np, OF_DETACHED);
>>>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>>> 
>>>> 	of_add_proc_dt_entry(np);
>>> 
>>> Does this add a call to a routine which only gets introduced in a
>>> subsequent patch (2/5)?  If so, it would break builds during the
>>> series, and thus would hinder bisection.
>>> 
>> 
>> You're right, I'll re-order on the next series.
> 
> Is it necessary at all now, after these fixes:
> 9e401275 of: fdt: fix memory initialization for expanded DT
> 0640332e of: Fix missing memory initialization on FDT unflattening
> 92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
> 
> ?
> 
> -- 
> Best regards,
> Alexander Sverdlin.

Re: [PATCH 1/5] OF: Clear detach flag on attach

[Alexander Sverdlin]

Hi!

On 06/11/13 09:49, ext Pantelis Antoniou wrote:
> I'm not exactly sure, but I think it is still needed.
> Since at that point the tree is attached.

Yes, now I think it's necessary. If you consider multiple detach-attach sequences.
I only thought about first fdt unflattering, which is the case in overlay_proc_release(), I suppose.
So the call to of_node_clear_flag() is superfluous, but doesn't hurt. 

> On Nov 6, 2013, at 10:46 AM, Alexander Sverdlin wrote:
> 
>> Hello Pantelis,
>>
>> On 05/11/13 21:03, ext Pantelis Antoniou wrote:
>>> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
>>>>> --- a/drivers/of/base.c
>>>>> +++ b/drivers/of/base.c
>>>>> @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
>>>>> 	np->allnext = of_allnodes;
>>>>> 	np->parent->child = np;
>>>>> 	of_allnodes = np;
>>>>> +	of_node_clear_flag(np, OF_DETACHED);
>>>>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>>>>
>>>>> 	of_add_proc_dt_entry(np);
>>>>
>>>> Does this add a call to a routine which only gets introduced in a
>>>> subsequent patch (2/5)?  If so, it would break builds during the
>>>> series, and thus would hinder bisection.
>>>>
>>>
>>> You're right, I'll re-order on the next series.
>>
>> Is it necessary at all now, after these fixes:
>> 9e401275 of: fdt: fix memory initialization for expanded DT
>> 0640332e of: Fix missing memory initialization on FDT unflattening
>> 92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
>>
>> ?
>>
>> -- 
>> Best regards,
>> Alexander Sverdlin.
> 
> 
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 1/5] OF: Clear detach flag on attach

[Grant Likely]

On Wed, 6 Nov 2013 10:49:44 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> On Nov 6, 2013, at 10:46 AM, Alexander Sverdlin wrote:
> 
> > Hello Pantelis,
> > 
> > On 05/11/13 21:03, ext Pantelis Antoniou wrote:
> >> On Nov 5, 2013, at 9:43 PM, Gerhard Sittig wrote:
> >>>> --- a/drivers/of/base.c
> >>>> +++ b/drivers/of/base.c
> >>>> @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np)
> >>>> 	np->allnext = of_allnodes;
> >>>> 	np->parent->child = np;
> >>>> 	of_allnodes = np;
> >>>> +	of_node_clear_flag(np, OF_DETACHED);
> >>>> 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >>>> 
> >>>> 	of_add_proc_dt_entry(np);
> >>> 
> >>> Does this add a call to a routine which only gets introduced in a
> >>> subsequent patch (2/5)?  If so, it would break builds during the
> >>> series, and thus would hinder bisection.
> >>> 
> >> 
> >> You're right, I'll re-order on the next series.
> > 
> > Is it necessary at all now, after these fixes:
> > 9e401275 of: fdt: fix memory initialization for expanded DT
> > 0640332e of: Fix missing memory initialization on FDT unflattening
> > 92d31610 of/fdt: Remove duplicate memory clearing on FDT unflattening
>
> Hi Alexander,
> 
> I'm not exactly sure, but I think it is still needed.
> Since at that point the tree is attached.
> 
> Grant?

In one sense it is a little odd because it isn't something that any of
the existing users (of which there are 2) would be affected by. It isn't
a bad idea though. Merged patches 2 & 1.

g.

[PATCH 2/5] OF: Introduce device tree node flag helpers.

[Pantelis Antoniou]

Helper functions for working with device node flags.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 include/linux/of.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..786c4f6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -114,6 +114,26 @@ static inline void of_node_set_flag(struct device_node *n, unsigned long flag)
 	set_bit(flag, &n->_flags);
 }
 
+static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
+{
+	clear_bit(flag, &n->_flags);
+}
+
+static inline int of_property_check_flag(struct property *p, unsigned long flag)
+{
+	return test_bit(flag, &p->_flags);
+}
+
+static inline void of_property_set_flag(struct property *p, unsigned long flag)
+{
+	set_bit(flag, &p->_flags);
+}
+
+static inline void of_property_clear_flag(struct property *p, unsigned long flag)
+{
+	clear_bit(flag, &p->_flags);
+}
+
 extern struct device_node *of_find_all_nodes(struct device_node *prev);
 
 /*
-- 
1.7.12

Re: [PATCH 2/5] OF: Introduce device tree node flag helpers.

[Grant Likely]

On Tue,  5 Nov 2013 19:50:13 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Helper functions for working with device node flags.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

Merged, thanks

g.

> ---
>  include/linux/of.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f95aee3..786c4f6 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -114,6 +114,26 @@ static inline void of_node_set_flag(struct device_node *n, unsigned long flag)
>  	set_bit(flag, &n->_flags);
>  }
>  
> +static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
> +{
> +	clear_bit(flag, &n->_flags);
> +}
> +
> +static inline int of_property_check_flag(struct property *p, unsigned long flag)
> +{
> +	return test_bit(flag, &p->_flags);
> +}
> +
> +static inline void of_property_set_flag(struct property *p, unsigned long flag)
> +{
> +	set_bit(flag, &p->_flags);
> +}
> +
> +static inline void of_property_clear_flag(struct property *p, unsigned long flag)
> +{
> +	clear_bit(flag, &p->_flags);
> +}
> +
>  extern struct device_node *of_find_all_nodes(struct device_node *prev);
>  
>  /*
> -- 
> 1.7.12
> 

[PATCH 3/5] OF: export of_property_notify

[Pantelis Antoniou]

of_property_notify can be utilized by other users too, export it.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  |  8 +-------
 include/linux/of.h | 11 +++++++++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ca10916..0ffc5a9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1424,7 +1424,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
 #if defined(CONFIG_OF_DYNAMIC)
-static int of_property_notify(int action, struct device_node *np,
+int of_property_notify(int action, struct device_node *np,
 			      struct property *prop)
 {
 	struct of_prop_reconfig pr;
@@ -1433,12 +1433,6 @@ static int of_property_notify(int action, struct device_node *np,
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
 }
-#else
-static int of_property_notify(int action, struct device_node *np,
-			      struct property *prop)
-{
-	return 0;
-}
 #endif
 
 /**
diff --git a/include/linux/of.h b/include/linux/of.h
index 786c4f6..6fec255 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -307,6 +307,17 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+#if defined(CONFIG_OF_DYNAMIC)
+extern int of_property_notify(int action, struct device_node *np,
+			      struct property *prop);
+#else
+static inline int of_property_notify(int action, struct device_node *np,
+			      struct property *prop)
+{
+	return 0;
+}
+#endif
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.12

Re: [PATCH 3/5] OF: export of_property_notify

[Matt Porter]

On Tue, Nov 05, 2013 at 07:50:14PM +0200, Pantelis Antoniou wrote:
> of_property_notify can be utilized by other users too, export it.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/base.c  |  8 +-------
>  include/linux/of.h | 11 +++++++++++
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ca10916..0ffc5a9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1424,7 +1424,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>  EXPORT_SYMBOL(of_count_phandle_with_args);
>  
>  #if defined(CONFIG_OF_DYNAMIC)
> -static int of_property_notify(int action, struct device_node *np,
> +int of_property_notify(int action, struct device_node *np,
>  			      struct property *prop)
>  {
>  	struct of_prop_reconfig pr;
> @@ -1433,12 +1433,6 @@ static int of_property_notify(int action, struct device_node *np,
>  	pr.prop = prop;
>  	return of_reconfig_notify(action, &pr);
>  }
> -#else
> -static int of_property_notify(int action, struct device_node *np,
> -			      struct property *prop)
> -{
> -	return 0;
> -}
>  #endif

EXPORT_SYMBOL[_GPL]?

-Matt

Re: [PATCH 3/5] OF: export of_property_notify

[Pantelis Antoniou]

Hi Matt,

On Nov 5, 2013, at 8:12 PM, Matt Porter wrote:

> On Tue, Nov 05, 2013 at 07:50:14PM +0200, Pantelis Antoniou wrote:
>> of_property_notify can be utilized by other users too, export it.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>> drivers/of/base.c  |  8 +-------
>> include/linux/of.h | 11 +++++++++++
>> 2 files changed, 12 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index ca10916..0ffc5a9 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1424,7 +1424,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>> EXPORT_SYMBOL(of_count_phandle_with_args);
>> 
>> #if defined(CONFIG_OF_DYNAMIC)
>> -static int of_property_notify(int action, struct device_node *np,
>> +int of_property_notify(int action, struct device_node *np,
>> 			      struct property *prop)
>> {
>> 	struct of_prop_reconfig pr;
>> @@ -1433,12 +1433,6 @@ static int of_property_notify(int action, struct device_node *np,
>> 	pr.prop = prop;
>> 	return of_reconfig_notify(action, &pr);
>> }
>> -#else
>> -static int of_property_notify(int action, struct device_node *np,
>> -			      struct property *prop)
>> -{
>> -	return 0;
>> -}
>> #endif
> 
> EXPORT_SYMBOL[_GPL]?
> 

Duh, good catch.

Will fix on next revision.

> -Matt

Regards

-- Pantelis

Re: [PATCH 3/5] OF: export of_property_notify

[Grant Likely]

On Tue,  5 Nov 2013 19:50:14 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> of_property_notify can be utilized by other users too, export it.

Please keep this patch in the series with the patch that actually uses
it.

g.

> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/base.c  |  8 +-------
>  include/linux/of.h | 11 +++++++++++
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ca10916..0ffc5a9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1424,7 +1424,7 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
>  EXPORT_SYMBOL(of_count_phandle_with_args);
>  
>  #if defined(CONFIG_OF_DYNAMIC)
> -static int of_property_notify(int action, struct device_node *np,
> +int of_property_notify(int action, struct device_node *np,
>  			      struct property *prop)
>  {
>  	struct of_prop_reconfig pr;
> @@ -1433,12 +1433,6 @@ static int of_property_notify(int action, struct device_node *np,
>  	pr.prop = prop;
>  	return of_reconfig_notify(action, &pr);
>  }
> -#else
> -static int of_property_notify(int action, struct device_node *np,
> -			      struct property *prop)
> -{
> -	return 0;
> -}
>  #endif
>  
>  /**
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 786c4f6..6fec255 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -307,6 +307,17 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
>  extern int of_count_phandle_with_args(const struct device_node *np,
>  	const char *list_name, const char *cells_name);
>  
> +#if defined(CONFIG_OF_DYNAMIC)
> +extern int of_property_notify(int action, struct device_node *np,
> +			      struct property *prop);
> +#else
> +static inline int of_property_notify(int action, struct device_node *np,
> +			      struct property *prop)
> +{
> +	return 0;
> +}
> +#endif
> +
>  extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
>  extern int of_alias_get_id(struct device_node *np, const char *stem);
>  
> -- 
> 1.7.12
> 

Re: [PATCH 3/5] OF: export of_property_notify

[Pantelis Antoniou]

Hi Grant,

On Nov 11, 2013, at 5:06 PM, Grant Likely wrote:

> On Tue,  5 Nov 2013 19:50:14 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> of_property_notify can be utilized by other users too, export it.
> 
> Please keep this patch in the series with the patch that actually uses
> it.
> 

OK.

> g.
> 

Regards

-- Pantelis

[PATCH 4/5] OF: Export all DT proc update functions

[Pantelis Antoniou]

There are other users for the proc DT functions.
Export them.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  | 70 ++++++++++++++++++++++++++++++------------------------
 include/linux/of.h | 29 ++++++++++++++++++++++
 2 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0ffc5a9..c3c5391 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1435,6 +1435,40 @@ int of_property_notify(int action, struct device_node *np,
 }
 #endif
 
+#ifdef CONFIG_PROC_DEVICETREE
+
+void of_add_proc_dt_entry(struct device_node *dn)
+{
+	struct proc_dir_entry *ent;
+
+	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
+	if (ent)
+		proc_device_tree_add_node(dn, ent);
+}
+
+void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop)
+{
+	if (np && prop && np->pde)
+		proc_device_tree_add_prop(np->pde, prop);
+}
+
+void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop)
+{
+	if (np && prop && np->pde)
+		proc_device_tree_remove_prop(np->pde, prop);
+}
+
+void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop)
+{
+	if (np && newprop && oldprop && np->pde)
+		proc_device_tree_update_prop(np->pde, newprop, oldprop);
+}
+
+#endif /* CONFIG_PROC_DEVICETREE */
+
 /**
  * of_add_property - Add a property to a node
  */
@@ -1462,11 +1496,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	*next = prop;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-#ifdef CONFIG_PROC_DEVICETREE
 	/* try to add to proc as well if it was initialized */
-	if (np->pde)
-		proc_device_tree_add_prop(np->pde, prop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_add_proc_dt_prop_entry(np, prop);
 
 	return 0;
 }
@@ -1508,11 +1539,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (!found)
 		return -ENODEV;
 
-#ifdef CONFIG_PROC_DEVICETREE
-	/* try to remove the proc node as well */
-	if (np->pde)
-		proc_device_tree_remove_prop(np->pde, prop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_remove_proc_dt_prop_entry(np, prop);
 
 	return 0;
 }
@@ -1562,11 +1589,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (!found)
 		return -ENODEV;
 
-#ifdef CONFIG_PROC_DEVICETREE
 	/* try to add to proc as well if it was initialized */
-	if (np->pde)
-		proc_device_tree_update_prop(np->pde, newprop, oldprop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_update_proc_dt_prop_entry(np, newprop, oldprop);
 
 	return 0;
 }
@@ -1602,22 +1626,6 @@ int of_reconfig_notify(unsigned long action, void *p)
 	return notifier_to_errno(rc);
 }
 
-#ifdef CONFIG_PROC_DEVICETREE
-static void of_add_proc_dt_entry(struct device_node *dn)
-{
-	struct proc_dir_entry *ent;
-
-	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
-	if (ent)
-		proc_device_tree_add_node(dn, ent);
-}
-#else
-static void of_add_proc_dt_entry(struct device_node *dn)
-{
-	return;
-}
-#endif
-
 /**
  * of_attach_node - Plug a device node into the tree and global list.
  */
@@ -1643,12 +1651,12 @@ int of_attach_node(struct device_node *np)
 }
 
 #ifdef CONFIG_PROC_DEVICETREE
-static void of_remove_proc_dt_entry(struct device_node *dn)
+void of_remove_proc_dt_entry(struct device_node *dn)
 {
 	proc_remove(dn->pde);
 }
 #else
-static void of_remove_proc_dt_entry(struct device_node *dn)
+void of_remove_proc_dt_entry(struct device_node *dn)
 {
 	return;
 }
diff --git a/include/linux/of.h b/include/linux/of.h
index 6fec255..a56c450 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -318,6 +318,35 @@ static inline int of_property_notify(int action, struct device_node *np,
 }
 #endif
 
+#ifdef CONFIG_PROC_DEVICETREE
+
+extern void of_add_proc_dt_entry(struct device_node *dn);
+extern void of_remove_proc_dt_entry(struct device_node *dn);
+
+extern void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop);
+
+extern void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop);
+
+extern void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop);
+#else
+
+static inline void of_add_proc_dt_entry(struct device_node *dn) { }
+static inline void of_remove_proc_dt_entry(struct device_node *dn) { }
+
+static inline void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop) { }
+
+static inline void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop) { }
+
+static inline void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop) { }
+
+#endif
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.12

Re: [PATCH 4/5] OF: Export all DT proc update functions

[Grant Likely]

On Tue,  5 Nov 2013 19:50:15 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> There are other users for the proc DT functions.
> Export them.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

These are all very much internal DT APIs. There is no way that anything
outside of drivers/of should ever be calling them. (In fact, I want to
be rid of the /proc/devicetree implementation entirely and put it into
sysfs, but that is a separate patch set). If they need to be broken out
into a header file, can they be put into something like
drivers/of/of_private.h?

g.

> ---
>  drivers/of/base.c  | 70 ++++++++++++++++++++++++++++++------------------------
>  include/linux/of.h | 29 ++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 0ffc5a9..c3c5391 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1435,6 +1435,40 @@ int of_property_notify(int action, struct device_node *np,
>  }
>  #endif
>  
> +#ifdef CONFIG_PROC_DEVICETREE
> +
> +void of_add_proc_dt_entry(struct device_node *dn)
> +{
> +	struct proc_dir_entry *ent;
> +
> +	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
> +	if (ent)
> +		proc_device_tree_add_node(dn, ent);
> +}
> +
> +void of_add_proc_dt_prop_entry(struct device_node *np,
> +		struct property *prop)
> +{
> +	if (np && prop && np->pde)
> +		proc_device_tree_add_prop(np->pde, prop);
> +}
> +
> +void of_remove_proc_dt_prop_entry(struct device_node *np,
> +		struct property *prop)
> +{
> +	if (np && prop && np->pde)
> +		proc_device_tree_remove_prop(np->pde, prop);
> +}
> +
> +void of_update_proc_dt_prop_entry(struct device_node *np,
> +		struct property *newprop, struct property *oldprop)
> +{
> +	if (np && newprop && oldprop && np->pde)
> +		proc_device_tree_update_prop(np->pde, newprop, oldprop);
> +}
> +
> +#endif /* CONFIG_PROC_DEVICETREE */
> +
>  /**
>   * of_add_property - Add a property to a node
>   */
> @@ -1462,11 +1496,8 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	*next = prop;
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -#ifdef CONFIG_PROC_DEVICETREE
>  	/* try to add to proc as well if it was initialized */
> -	if (np->pde)
> -		proc_device_tree_add_prop(np->pde, prop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> +	of_add_proc_dt_prop_entry(np, prop);
>  
>  	return 0;
>  }
> @@ -1508,11 +1539,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	if (!found)
>  		return -ENODEV;
>  
> -#ifdef CONFIG_PROC_DEVICETREE
> -	/* try to remove the proc node as well */
> -	if (np->pde)
> -		proc_device_tree_remove_prop(np->pde, prop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> +	of_remove_proc_dt_prop_entry(np, prop);
>  
>  	return 0;
>  }
> @@ -1562,11 +1589,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	if (!found)
>  		return -ENODEV;
>  
> -#ifdef CONFIG_PROC_DEVICETREE
>  	/* try to add to proc as well if it was initialized */
> -	if (np->pde)
> -		proc_device_tree_update_prop(np->pde, newprop, oldprop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> +	of_update_proc_dt_prop_entry(np, newprop, oldprop);
>  
>  	return 0;
>  }
> @@ -1602,22 +1626,6 @@ int of_reconfig_notify(unsigned long action, void *p)
>  	return notifier_to_errno(rc);
>  }
>  
> -#ifdef CONFIG_PROC_DEVICETREE
> -static void of_add_proc_dt_entry(struct device_node *dn)
> -{
> -	struct proc_dir_entry *ent;
> -
> -	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
> -	if (ent)
> -		proc_device_tree_add_node(dn, ent);
> -}
> -#else
> -static void of_add_proc_dt_entry(struct device_node *dn)
> -{
> -	return;
> -}
> -#endif
> -
>  /**
>   * of_attach_node - Plug a device node into the tree and global list.
>   */
> @@ -1643,12 +1651,12 @@ int of_attach_node(struct device_node *np)
>  }
>  
>  #ifdef CONFIG_PROC_DEVICETREE
> -static void of_remove_proc_dt_entry(struct device_node *dn)
> +void of_remove_proc_dt_entry(struct device_node *dn)
>  {
>  	proc_remove(dn->pde);
>  }
>  #else
> -static void of_remove_proc_dt_entry(struct device_node *dn)
> +void of_remove_proc_dt_entry(struct device_node *dn)
>  {
>  	return;
>  }
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 6fec255..a56c450 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -318,6 +318,35 @@ static inline int of_property_notify(int action, struct device_node *np,
>  }
>  #endif
>  
> +#ifdef CONFIG_PROC_DEVICETREE
> +
> +extern void of_add_proc_dt_entry(struct device_node *dn);
> +extern void of_remove_proc_dt_entry(struct device_node *dn);
> +
> +extern void of_add_proc_dt_prop_entry(struct device_node *np,
> +		struct property *prop);
> +
> +extern void of_remove_proc_dt_prop_entry(struct device_node *np,
> +		struct property *prop);
> +
> +extern void of_update_proc_dt_prop_entry(struct device_node *np,
> +		struct property *newprop, struct property *oldprop);
> +#else
> +
> +static inline void of_add_proc_dt_entry(struct device_node *dn) { }
> +static inline void of_remove_proc_dt_entry(struct device_node *dn) { }
> +
> +static inline void of_add_proc_dt_prop_entry(struct device_node *np,
> +		struct property *prop) { }
> +
> +static inline void of_remove_proc_dt_prop_entry(struct device_node *np,
> +		struct property *prop) { }
> +
> +static inline void of_update_proc_dt_prop_entry(struct device_node *np,
> +		struct property *newprop, struct property *oldprop) { }
> +
> +#endif
> +
>  extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
>  extern int of_alias_get_id(struct device_node *np, const char *stem);
>  
> -- 
> 1.7.12
> 

Re: [PATCH 4/5] OF: Export all DT proc update functions

[Pantelis Antoniou]

Hi Grant,

On Nov 11, 2013, at 5:09 PM, Grant Likely wrote:

> On Tue,  5 Nov 2013 19:50:15 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> There are other users for the proc DT functions.
>> Export them.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> These are all very much internal DT APIs. There is no way that anything
> outside of drivers/of should ever be calling them. (In fact, I want to
> be rid of the /proc/devicetree implementation entirely and put it into
> sysfs, but that is a separate patch set). If they need to be broken out
> into a header file, can they be put into something like
> drivers/of/of_private.h?
> 

That's fine by me.

I can move them to of_private.h, no problem there.

> g.
> 

Regards

-- Pantelis

[PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Introduce helper functions for working with the live DT tree.

__of_free_property() frees a dynamically created property
__of_free_tree() recursively frees a device node tree
__of_copy_property() copies a property dynamically
__of_create_empty_node() creates an empty node
__of_find_node_by_full_name() finds the node with the full name
and
of_multi_prop_cmp() performs a multi property compare but without
having to take locks.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/Makefile |   2 +-
 drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h  |  59 ++++++++++++
 3 files changed, 313 insertions(+), 1 deletion(-)
 create mode 100644 drivers/of/util.c

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index efd0510..9bc6d8c 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,4 +1,4 @@
-obj-y = base.o device.o platform.o
+obj-y = base.o device.o platform.o util.o
 obj-$(CONFIG_OF_FLATTREE) += fdt.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o
diff --git a/drivers/of/util.c b/drivers/of/util.c
new file mode 100644
index 0000000..5117e2b
--- /dev/null
+++ b/drivers/of/util.c
@@ -0,0 +1,253 @@
+/*
+ * Utility functions for working with device tree(s)
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+/**
+ * __of_free_property - release the memory of an allocated property
+ * @prop:	Property to release
+ *
+ * Release the memory of an allocated property only after checking
+ * that the property has been marked as OF_DYNAMIC.
+ * Only call on known allocated properties.
+ */
+void __of_free_property(struct property *prop)
+{
+	if (prop == NULL)
+		return;
+
+	if (of_property_check_flag(prop, OF_DYNAMIC)) {
+		kfree(prop->value);
+		kfree(prop->name);
+		kfree(prop);
+	} else {
+		pr_warn("%s: property %p cannot be freed; memory is gone\n",
+				__func__, prop);
+	}
+}
+
+/**
+ * __of_free_tree - release the memory of a device tree node and
+ *		    of all it's children + properties.
+ * @node:	Device Tree node to release
+ *
+ * Release the memory of a device tree node and of all it's children.
+ * Also release the properties and the dead properties.
+ * Only call on detached node trees, and you better be sure that
+ * no pointer exist for any properties. Only safe to do if you 
+ * absolutely control the life cycle of the node.
+ * Also note that the node is not removed from the all_nodes list,
+ * neither from the parent's child list; this should be handled before
+ * calling this function.
+ */
+void __of_free_tree(struct device_node *node)
+{
+	struct property *prop;
+	struct device_node *noden;
+
+	/* sanity check */
+	if (!node)
+		return;
+
+	/* free recursively any children */
+	while ((noden = node->child) != NULL) {
+		node->child = noden->sibling;
+		__of_free_tree(noden);
+	}
+
+	/* free every property already allocated */
+	while ((prop = node->properties) != NULL) {
+		node->properties = prop->next;
+		__of_free_property(prop);
+	}
+
+	/* free dead properties already allocated */
+	while ((prop = node->deadprops) != NULL) {
+		node->deadprops = prop->next;
+		__of_free_property(prop);
+	}
+
+	if (of_node_check_flag(node, OF_DYNAMIC)) {
+		kfree(node->type);
+		kfree(node->name);
+		kfree(node);
+	} else {
+		pr_warn("%s: node %p cannot be freed; memory is gone\n",
+				__func__, node);
+	}
+}
+
+/**
+ * __of_copy_property - Copy a property dynamically.
+ * @prop:	Property to copy
+ * @flags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Copy a property by dynamically allocating the memory of both the
+ * property stucture and the property name & contents. The property's
+ * flags have the OF_DYNAMIC bit set so that we can differentiate between
+ * dynamically allocated properties and not.
+ * Returns the newly allocated property or NULL on out of memory error.
+ */
+struct property *__of_copy_property(const struct property *prop, gfp_t flags)
+{
+	struct property *propn;
+
+	propn = kzalloc(sizeof(*prop), flags);
+	if (propn == NULL)
+		return NULL;
+
+	propn->name = kstrdup(prop->name, flags);
+	if (propn->name == NULL)
+		goto err_fail_name;
+
+	if (prop->length > 0) {
+		propn->value = kmalloc(prop->length, flags);
+		if (propn->value == NULL)
+			goto err_fail_value;
+		memcpy(propn->value, prop->value, prop->length);
+		propn->length = prop->length;
+	}
+
+	/* mark the property as dynamic */
+	of_property_set_flag(propn, OF_DYNAMIC);
+
+	return propn;
+
+err_fail_value:
+	kfree(propn->name);
+err_fail_name:
+	kfree(propn);
+	return NULL;
+}
+
+/**
+ * __of_create_empty_node - Create an empty device node dynamically.
+ * @name:	Name of the new device node
+ * @type:	Type of the new device node
+ * @full_name:	Full name of the new device node
+ * @phandle:	Phandle of the new device node
+ * @flags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Create an empty device tree node, suitable for further modification.
+ * The node data are dynamically allocated and all the node flags
+ * have the OF_DYNAMIC & OF_DETACHED bits set.
+ * Returns the newly allocated node or NULL on out of memory error.
+ */
+struct device_node *__of_create_empty_node(
+		const char *name, const char *type, const char *full_name,
+		phandle phandle, gfp_t flags)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), flags);
+	if (node == NULL)
+		return NULL;
+
+	node->name = kstrdup(name, flags);
+	if (node->name == NULL)
+		goto err_return;
+
+	node->type = kstrdup(type, flags);
+	if (node->type == NULL)
+		goto err_return;
+
+	node->full_name = kstrdup(full_name, flags);
+	if (node->type == NULL)
+		goto err_return;
+
+	node->phandle = phandle;
+	kref_init(&node->kref);
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+
+	return node;
+
+err_return:
+	__of_free_tree(node);
+	return NULL;
+}
+
+/**
+ * __of_find_node_by_full_name - Find a node with the full name recursively
+ * @node:	Root of the tree to perform the search
+ * @full_name:	Full name of the node to find.
+ *
+ * Find a node with the give full name by recursively following any of 
+ * the child node links.
+ * Returns the matching node, or NULL if not found.
+ * Note that the devtree lock is not taken, so this function is only
+ * safe to call on either detached trees, or when devtree lock is already
+ * taken.
+ */
+struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name)
+{
+	struct device_node *child, *found;
+
+	if (node == NULL)
+		return NULL;
+
+	/* check */
+	if (of_node_cmp(node->full_name, full_name) == 0)
+		return node;
+
+	__for_each_child_of_node(node, child) {
+		found = __of_find_node_by_full_name(child, full_name);
+		if (found != NULL)
+			return found;
+	}
+
+	return NULL;
+}
+
+/**
+ * of_multi_prop_cmp - Check if a property matches a value
+ * @prop:	Property to check
+ * @value:	Value to check against
+ *
+ * Check whether a property matches a value, using the standard
+ * of_compat_cmp() test on each string. It is similar to the test
+ * of_device_is_compatible() makes, but it can be performed without
+ * taking the devtree_lock, which is required in some cases.
+ * Returns 0 on a match, -1 on no match.
+ */
+int of_multi_prop_cmp(const struct property *prop, const char *value)
+{
+	const char *cp;
+	int cplen, vlen, l;
+
+	if (prop == NULL || value == NULL)
+		return -1;
+
+	cp = prop->value;
+	cplen = prop->length;
+	vlen = strlen(value);
+
+	while (cplen > 0) {
+		if (of_compat_cmp(cp, value, vlen) == 0)
+			return 0;
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
+
+	return -1;
+}
+
diff --git a/include/linux/of.h b/include/linux/of.h
index a56c450..9d69bd2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -662,4 +662,63 @@ extern void proc_device_tree_update_prop(struct proc_dir_entry *pde,
 					 struct property *oldprop);
 #endif
 
+/**
+ * General utilities for working with live trees.
+ *
+ * All functions with two leading underscores operate
+ * without taking node references, so you either have to
+ * own the devtree lock or work on detached trees only.
+ */
+
+#ifdef CONFIG_OF
+
+/* iterator for internal use; not references, neither affects devtree lock */
+#define __for_each_child_of_node(dn, chld) \
+	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
+
+void __of_free_property(struct property *prop);
+void __of_free_tree(struct device_node *node);
+struct property *__of_copy_property(const struct property *prop, gfp_t flags);
+struct device_node *__of_create_empty_node( const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t flags);
+struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name);
+int of_multi_prop_cmp(const struct property *prop, const char *value);
+
+#else /* !CONFIG_OF */
+
+#define __for_each_child_of_node(dn, chld) \
+	while (0)
+
+static inline void __of_free_property(struct property *prop) { }
+
+static inline void __of_free_tree(struct device_node *node) { }
+
+static inline struct property *__of_copy_property(const struct property *prop,
+		gfp_t flags)
+{
+	return NULL;
+}
+
+static inline struct device_node *__of_create_empty_node( const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t flags)
+{
+	return NULL;
+}
+
+static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name)
+{
+	return NULL;
+}
+
+static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
+{
+	return -1;
+}
+
+#endif	/* !CONFIG_OF */
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Ionut Nicu]

Hi,

First of all, good to see this patch set being submitted again!

We're using an older version of your patch set for some time and
they're working good for us.

On 05.11.2013 18:50, ext Pantelis Antoniou wrote:
> Introduce helper functions for working with the live DT tree.
> 
> __of_free_property() frees a dynamically created property
> __of_free_tree() recursively frees a device node tree
> __of_copy_property() copies a property dynamically
> __of_create_empty_node() creates an empty node
> __of_find_node_by_full_name() finds the node with the full name
> and
> of_multi_prop_cmp() performs a multi property compare but without
> having to take locks.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/Makefile |   2 +-
>  drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h  |  59 ++++++++++++
>  3 files changed, 313 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/of/util.c
> 
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index efd0510..9bc6d8c 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,4 +1,4 @@
> -obj-y = base.o device.o platform.o
> +obj-y = base.o device.o platform.o util.o
>  obj-$(CONFIG_OF_FLATTREE) += fdt.o
>  obj-$(CONFIG_OF_PROMTREE) += pdt.o
>  obj-$(CONFIG_OF_ADDRESS)  += address.o
> diff --git a/drivers/of/util.c b/drivers/of/util.c
> new file mode 100644
> index 0000000..5117e2b
> --- /dev/null
> +++ b/drivers/of/util.c
> @@ -0,0 +1,253 @@
> +/*
> + * Utility functions for working with device tree(s)
> + *
> + * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
> + * Copyright (C) 2012 Texas Instruments Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +
> +/**
> + * __of_free_property - release the memory of an allocated property
> + * @prop:	Property to release
> + *
> + * Release the memory of an allocated property only after checking
> + * that the property has been marked as OF_DYNAMIC.
> + * Only call on known allocated properties.
> + */
> +void __of_free_property(struct property *prop)
> +{
> +	if (prop == NULL)
> +		return;
> +
> +	if (of_property_check_flag(prop, OF_DYNAMIC)) {
> +		kfree(prop->value);
> +		kfree(prop->name);
> +		kfree(prop);
> +	} else {
> +		pr_warn("%s: property %p cannot be freed; memory is gone\n",
> +				__func__, prop);
> +	}
> +}
> +
> +/**
> + * __of_free_tree - release the memory of a device tree node and
> + *		    of all it's children + properties.
> + * @node:	Device Tree node to release
> + *
> + * Release the memory of a device tree node and of all it's children.
> + * Also release the properties and the dead properties.
> + * Only call on detached node trees, and you better be sure that
> + * no pointer exist for any properties. Only safe to do if you 
> + * absolutely control the life cycle of the node.
> + * Also note that the node is not removed from the all_nodes list,
> + * neither from the parent's child list; this should be handled before
> + * calling this function.
> + */
> +void __of_free_tree(struct device_node *node)
> +{
> +	struct property *prop;
> +	struct device_node *noden;
> +
> +	/* sanity check */
> +	if (!node)
> +		return;
> +
> +	/* free recursively any children */
> +	while ((noden = node->child) != NULL) {
> +		node->child = noden->sibling;
> +		__of_free_tree(noden);
> +	}
> +
> +	/* free every property already allocated */
> +	while ((prop = node->properties) != NULL) {
> +		node->properties = prop->next;
> +		__of_free_property(prop);
> +	}
> +
> +	/* free dead properties already allocated */
> +	while ((prop = node->deadprops) != NULL) {
> +		node->deadprops = prop->next;
> +		__of_free_property(prop);
> +	}
> +
> +	if (of_node_check_flag(node, OF_DYNAMIC)) {
> +		kfree(node->type);
> +		kfree(node->name);
> +		kfree(node);
> +	} else {
> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
> +				__func__, node);
> +	}
> +}
> +
> +/**
> + * __of_copy_property - Copy a property dynamically.
> + * @prop:	Property to copy
> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Copy a property by dynamically allocating the memory of both the
> + * property stucture and the property name & contents. The property's
> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> + * dynamically allocated properties and not.
> + * Returns the newly allocated property or NULL on out of memory error.
> + */
> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
> +{
> +	struct property *propn;
> +
> +	propn = kzalloc(sizeof(*prop), flags);
> +	if (propn == NULL)
> +		return NULL;
> +
> +	propn->name = kstrdup(prop->name, flags);
> +	if (propn->name == NULL)
> +		goto err_fail_name;
> +
> +	if (prop->length > 0) {
> +		propn->value = kmalloc(prop->length, flags);
> +		if (propn->value == NULL)
> +			goto err_fail_value;
> +		memcpy(propn->value, prop->value, prop->length);
> +		propn->length = prop->length;
> +	}


I think the prop->length check, should be removed. Properties with length 0, such
as "interrupt-controller;" have length 0 and some parts of the linux kernel actually
rely on their value being not NULL.

For example in drivers/of/irq.c, of_irq_map_raw() checks that a node is an interrupt
controller by calling:

of_get_property(ipar, "interrupt-controller", NULL)

and checking that it returns a non-null value.

We can safely use kmalloc with size 0 which will return ZERO_SIZE_PTR. This will cause
all the tests for non-null values in case of zero length properties to pass.

I've sent you a patch a while ago for this. I'm not sure you had time to review it.

Thanks,
Ionut

> +
> +	/* mark the property as dynamic */
> +	of_property_set_flag(propn, OF_DYNAMIC);
> +
> +	return propn;
> +
> +err_fail_value:
> +	kfree(propn->name);
> +err_fail_name:
> +	kfree(propn);
> +	return NULL;
> +}
> +
> +/**
> + * __of_create_empty_node - Create an empty device node dynamically.
> + * @name:	Name of the new device node
> + * @type:	Type of the new device node
> + * @full_name:	Full name of the new device node
> + * @phandle:	Phandle of the new device node
> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Create an empty device tree node, suitable for further modification.
> + * The node data are dynamically allocated and all the node flags
> + * have the OF_DYNAMIC & OF_DETACHED bits set.
> + * Returns the newly allocated node or NULL on out of memory error.
> + */
> +struct device_node *__of_create_empty_node(
> +		const char *name, const char *type, const char *full_name,
> +		phandle phandle, gfp_t flags)
> +{
> +	struct device_node *node;
> +
> +	node = kzalloc(sizeof(*node), flags);
> +	if (node == NULL)
> +		return NULL;
> +
> +	node->name = kstrdup(name, flags);
> +	if (node->name == NULL)
> +		goto err_return;
> +
> +	node->type = kstrdup(type, flags);
> +	if (node->type == NULL)
> +		goto err_return;
> +
> +	node->full_name = kstrdup(full_name, flags);
> +	if (node->type == NULL)
> +		goto err_return;
> +
> +	node->phandle = phandle;
> +	kref_init(&node->kref);
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +
> +	return node;
> +
> +err_return:
> +	__of_free_tree(node);
> +	return NULL;
> +}
> +
> +/**
> + * __of_find_node_by_full_name - Find a node with the full name recursively
> + * @node:	Root of the tree to perform the search
> + * @full_name:	Full name of the node to find.
> + *
> + * Find a node with the give full name by recursively following any of 
> + * the child node links.
> + * Returns the matching node, or NULL if not found.
> + * Note that the devtree lock is not taken, so this function is only
> + * safe to call on either detached trees, or when devtree lock is already
> + * taken.
> + */
> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
> +		const char *full_name)
> +{
> +	struct device_node *child, *found;
> +
> +	if (node == NULL)
> +		return NULL;
> +
> +	/* check */
> +	if (of_node_cmp(node->full_name, full_name) == 0)
> +		return node;
> +
> +	__for_each_child_of_node(node, child) {
> +		found = __of_find_node_by_full_name(child, full_name);
> +		if (found != NULL)
> +			return found;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * of_multi_prop_cmp - Check if a property matches a value
> + * @prop:	Property to check
> + * @value:	Value to check against
> + *
> + * Check whether a property matches a value, using the standard
> + * of_compat_cmp() test on each string. It is similar to the test
> + * of_device_is_compatible() makes, but it can be performed without
> + * taking the devtree_lock, which is required in some cases.
> + * Returns 0 on a match, -1 on no match.
> + */
> +int of_multi_prop_cmp(const struct property *prop, const char *value)
> +{
> +	const char *cp;
> +	int cplen, vlen, l;
> +
> +	if (prop == NULL || value == NULL)
> +		return -1;
> +
> +	cp = prop->value;
> +	cplen = prop->length;
> +	vlen = strlen(value);
> +
> +	while (cplen > 0) {
> +		if (of_compat_cmp(cp, value, vlen) == 0)
> +			return 0;
> +		l = strlen(cp) + 1;
> +		cp += l;
> +		cplen -= l;
> +	}
> +
> +	return -1;
> +}
> +
> diff --git a/include/linux/of.h b/include/linux/of.h
> index a56c450..9d69bd2 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -662,4 +662,63 @@ extern void proc_device_tree_update_prop(struct proc_dir_entry *pde,
>  					 struct property *oldprop);
>  #endif
>  
> +/**
> + * General utilities for working with live trees.
> + *
> + * All functions with two leading underscores operate
> + * without taking node references, so you either have to
> + * own the devtree lock or work on detached trees only.
> + */
> +
> +#ifdef CONFIG_OF
> +
> +/* iterator for internal use; not references, neither affects devtree lock */
> +#define __for_each_child_of_node(dn, chld) \
> +	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
> +
> +void __of_free_property(struct property *prop);
> +void __of_free_tree(struct device_node *node);
> +struct property *__of_copy_property(const struct property *prop, gfp_t flags);
> +struct device_node *__of_create_empty_node( const char *name,
> +		const char *type, const char *full_name,
> +		phandle phandle, gfp_t flags);
> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
> +		const char *full_name);
> +int of_multi_prop_cmp(const struct property *prop, const char *value);
> +
> +#else /* !CONFIG_OF */
> +
> +#define __for_each_child_of_node(dn, chld) \
> +	while (0)
> +
> +static inline void __of_free_property(struct property *prop) { }
> +
> +static inline void __of_free_tree(struct device_node *node) { }
> +
> +static inline struct property *__of_copy_property(const struct property *prop,
> +		gfp_t flags)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *__of_create_empty_node( const char *name,
> +		const char *type, const char *full_name,
> +		phandle phandle, gfp_t flags)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
> +		const char *full_name)
> +{
> +	return NULL;
> +}
> +
> +static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
> +{
> +	return -1;
> +}
> +
> +#endif	/* !CONFIG_OF */
> +
>  #endif /* _LINUX_OF_H */
> 

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Ionut,

On Nov 6, 2013, at 11:21 AM, Ionut Nicu wrote:

> Hi,
> 
> First of all, good to see this patch set being submitted again!
> 
> We're using an older version of your patch set for some time and
> they're working good for us.
> 

Thanks, that's good to know.

> On 05.11.2013 18:50, ext Pantelis Antoniou wrote:
>> Introduce helper functions for working with the live DT tree.
>> 
>> __of_free_property() frees a dynamically created property
>> __of_free_tree() recursively frees a device node tree
>> __of_copy_property() copies a property dynamically
>> __of_create_empty_node() creates an empty node
>> __of_find_node_by_full_name() finds the node with the full name
>> and
>> of_multi_prop_cmp() performs a multi property compare but without
>> having to take locks.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>> drivers/of/Makefile |   2 +-
>> drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of.h  |  59 ++++++++++++
>> 3 files changed, 313 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/of/util.c
>> 
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index efd0510..9bc6d8c 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,4 +1,4 @@
>> -obj-y = base.o device.o platform.o
>> +obj-y = base.o device.o platform.o util.o
>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>> obj-$(CONFIG_OF_PROMTREE) += pdt.o
>> obj-$(CONFIG_OF_ADDRESS)  += address.o
>> diff --git a/drivers/of/util.c b/drivers/of/util.c
>> new file mode 100644
>> index 0000000..5117e2b
>> --- /dev/null
>> +++ b/drivers/of/util.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + * Utility functions for working with device tree(s)
>> + *
>> + * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
>> + * Copyright (C) 2012 Texas Instruments Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +
>> +/**
>> + * __of_free_property - release the memory of an allocated property
>> + * @prop:	Property to release
>> + *
>> + * Release the memory of an allocated property only after checking
>> + * that the property has been marked as OF_DYNAMIC.
>> + * Only call on known allocated properties.
>> + */
>> +void __of_free_property(struct property *prop)
>> +{
>> +	if (prop == NULL)
>> +		return;
>> +
>> +	if (of_property_check_flag(prop, OF_DYNAMIC)) {
>> +		kfree(prop->value);
>> +		kfree(prop->name);
>> +		kfree(prop);
>> +	} else {
>> +		pr_warn("%s: property %p cannot be freed; memory is gone\n",
>> +				__func__, prop);
>> +	}
>> +}
>> +
>> +/**
>> + * __of_free_tree - release the memory of a device tree node and
>> + *		    of all it's children + properties.
>> + * @node:	Device Tree node to release
>> + *
>> + * Release the memory of a device tree node and of all it's children.
>> + * Also release the properties and the dead properties.
>> + * Only call on detached node trees, and you better be sure that
>> + * no pointer exist for any properties. Only safe to do if you 
>> + * absolutely control the life cycle of the node.
>> + * Also note that the node is not removed from the all_nodes list,
>> + * neither from the parent's child list; this should be handled before
>> + * calling this function.
>> + */
>> +void __of_free_tree(struct device_node *node)
>> +{
>> +	struct property *prop;
>> +	struct device_node *noden;
>> +
>> +	/* sanity check */
>> +	if (!node)
>> +		return;
>> +
>> +	/* free recursively any children */
>> +	while ((noden = node->child) != NULL) {
>> +		node->child = noden->sibling;
>> +		__of_free_tree(noden);
>> +	}
>> +
>> +	/* free every property already allocated */
>> +	while ((prop = node->properties) != NULL) {
>> +		node->properties = prop->next;
>> +		__of_free_property(prop);
>> +	}
>> +
>> +	/* free dead properties already allocated */
>> +	while ((prop = node->deadprops) != NULL) {
>> +		node->deadprops = prop->next;
>> +		__of_free_property(prop);
>> +	}
>> +
>> +	if (of_node_check_flag(node, OF_DYNAMIC)) {
>> +		kfree(node->type);
>> +		kfree(node->name);
>> +		kfree(node);
>> +	} else {
>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>> +				__func__, node);
>> +	}
>> +}
>> +
>> +/**
>> + * __of_copy_property - Copy a property dynamically.
>> + * @prop:	Property to copy
>> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
>> + *
>> + * Copy a property by dynamically allocating the memory of both the
>> + * property stucture and the property name & contents. The property's
>> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
>> + * dynamically allocated properties and not.
>> + * Returns the newly allocated property or NULL on out of memory error.
>> + */
>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
>> +{
>> +	struct property *propn;
>> +
>> +	propn = kzalloc(sizeof(*prop), flags);
>> +	if (propn == NULL)
>> +		return NULL;
>> +
>> +	propn->name = kstrdup(prop->name, flags);
>> +	if (propn->name == NULL)
>> +		goto err_fail_name;
>> +
>> +	if (prop->length > 0) {
>> +		propn->value = kmalloc(prop->length, flags);
>> +		if (propn->value == NULL)
>> +			goto err_fail_value;
>> +		memcpy(propn->value, prop->value, prop->length);
>> +		propn->length = prop->length;
>> +	}
> 
> 
> I think the prop->length check, should be removed. Properties with length 0, such
> as "interrupt-controller;" have length 0 and some parts of the linux kernel actually
> rely on their value being not NULL.
> 
> For example in drivers/of/irq.c, of_irq_map_raw() checks that a node is an interrupt
> controller by calling:
> 
> of_get_property(ipar, "interrupt-controller", NULL)
> 
> and checking that it returns a non-null value.
> 
> We can safely use kmalloc with size 0 which will return ZERO_SIZE_PTR. This will cause
> all the tests for non-null values in case of zero length properties to pass.
> 
> I've sent you a patch a while ago for this. I'm not sure you had time to review it.
> 

I am aware of that, and your patch looks sane.

As I mentioned earlier I'm trying to get this accepted in general term and then we'll
get around fixing any minor problems.

> Thanks,
> Ionut
> 

Regards

-- Pantelis

>> +
>> +	/* mark the property as dynamic */
>> +	of_property_set_flag(propn, OF_DYNAMIC);
>> +
>> +	return propn;
>> +
>> +err_fail_value:
>> +	kfree(propn->name);
>> +err_fail_name:
>> +	kfree(propn);
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * __of_create_empty_node - Create an empty device node dynamically.
>> + * @name:	Name of the new device node
>> + * @type:	Type of the new device node
>> + * @full_name:	Full name of the new device node
>> + * @phandle:	Phandle of the new device node
>> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
>> + *
>> + * Create an empty device tree node, suitable for further modification.
>> + * The node data are dynamically allocated and all the node flags
>> + * have the OF_DYNAMIC & OF_DETACHED bits set.
>> + * Returns the newly allocated node or NULL on out of memory error.
>> + */
>> +struct device_node *__of_create_empty_node(
>> +		const char *name, const char *type, const char *full_name,
>> +		phandle phandle, gfp_t flags)
>> +{
>> +	struct device_node *node;
>> +
>> +	node = kzalloc(sizeof(*node), flags);
>> +	if (node == NULL)
>> +		return NULL;
>> +
>> +	node->name = kstrdup(name, flags);
>> +	if (node->name == NULL)
>> +		goto err_return;
>> +
>> +	node->type = kstrdup(type, flags);
>> +	if (node->type == NULL)
>> +		goto err_return;
>> +
>> +	node->full_name = kstrdup(full_name, flags);
>> +	if (node->type == NULL)
>> +		goto err_return;
>> +
>> +	node->phandle = phandle;
>> +	kref_init(&node->kref);
>> +	of_node_set_flag(node, OF_DYNAMIC);
>> +	of_node_set_flag(node, OF_DETACHED);
>> +
>> +	return node;
>> +
>> +err_return:
>> +	__of_free_tree(node);
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * __of_find_node_by_full_name - Find a node with the full name recursively
>> + * @node:	Root of the tree to perform the search
>> + * @full_name:	Full name of the node to find.
>> + *
>> + * Find a node with the give full name by recursively following any of 
>> + * the child node links.
>> + * Returns the matching node, or NULL if not found.
>> + * Note that the devtree lock is not taken, so this function is only
>> + * safe to call on either detached trees, or when devtree lock is already
>> + * taken.
>> + */
>> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> +		const char *full_name)
>> +{
>> +	struct device_node *child, *found;
>> +
>> +	if (node == NULL)
>> +		return NULL;
>> +
>> +	/* check */
>> +	if (of_node_cmp(node->full_name, full_name) == 0)
>> +		return node;
>> +
>> +	__for_each_child_of_node(node, child) {
>> +		found = __of_find_node_by_full_name(child, full_name);
>> +		if (found != NULL)
>> +			return found;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * of_multi_prop_cmp - Check if a property matches a value
>> + * @prop:	Property to check
>> + * @value:	Value to check against
>> + *
>> + * Check whether a property matches a value, using the standard
>> + * of_compat_cmp() test on each string. It is similar to the test
>> + * of_device_is_compatible() makes, but it can be performed without
>> + * taking the devtree_lock, which is required in some cases.
>> + * Returns 0 on a match, -1 on no match.
>> + */
>> +int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> +	const char *cp;
>> +	int cplen, vlen, l;
>> +
>> +	if (prop == NULL || value == NULL)
>> +		return -1;
>> +
>> +	cp = prop->value;
>> +	cplen = prop->length;
>> +	vlen = strlen(value);
>> +
>> +	while (cplen > 0) {
>> +		if (of_compat_cmp(cp, value, vlen) == 0)
>> +			return 0;
>> +		l = strlen(cp) + 1;
>> +		cp += l;
>> +		cplen -= l;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index a56c450..9d69bd2 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -662,4 +662,63 @@ extern void proc_device_tree_update_prop(struct proc_dir_entry *pde,
>> 					 struct property *oldprop);
>> #endif
>> 
>> +/**
>> + * General utilities for working with live trees.
>> + *
>> + * All functions with two leading underscores operate
>> + * without taking node references, so you either have to
>> + * own the devtree lock or work on detached trees only.
>> + */
>> +
>> +#ifdef CONFIG_OF
>> +
>> +/* iterator for internal use; not references, neither affects devtree lock */
>> +#define __for_each_child_of_node(dn, chld) \
>> +	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
>> +
>> +void __of_free_property(struct property *prop);
>> +void __of_free_tree(struct device_node *node);
>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags);
>> +struct device_node *__of_create_empty_node( const char *name,
>> +		const char *type, const char *full_name,
>> +		phandle phandle, gfp_t flags);
>> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> +		const char *full_name);
>> +int of_multi_prop_cmp(const struct property *prop, const char *value);
>> +
>> +#else /* !CONFIG_OF */
>> +
>> +#define __for_each_child_of_node(dn, chld) \
>> +	while (0)
>> +
>> +static inline void __of_free_property(struct property *prop) { }
>> +
>> +static inline void __of_free_tree(struct device_node *node) { }
>> +
>> +static inline struct property *__of_copy_property(const struct property *prop,
>> +		gfp_t flags)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline struct device_node *__of_create_empty_node( const char *name,
>> +		const char *type, const char *full_name,
>> +		phandle phandle, gfp_t flags)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> +		const char *full_name)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> +	return -1;
>> +}
>> +
>> +#endif	/* !CONFIG_OF */
>> +
>> #endif /* _LINUX_OF_H */
>> 
> 

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Guenter Roeck]

Hi Pantelis,

On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:

>
> As I mentioned earlier I'm trying to get this accepted in general term and then we'll
> get around fixing any minor problems.
>

I think it is quite likely at this point that your series will be accepted.

However, from my experience with larger submissions like this one, it is quite common
that the maintainers ask for all feedback to be addressed before that happens -
especially for feedback like this, where there is common understanding that some
changes to the code are necessary.

Maybe the device tree maintainers could chime in at this point and let us all know
what will need to happen to get the series accepted.

Thanks,
Guenter

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Guenter,

On Nov 6, 2013, at 4:53 PM, Guenter Roeck wrote:

> Hi Pantelis,
> 
> On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
> 
>> 
>> As I mentioned earlier I'm trying to get this accepted in general term and then we'll
>> get around fixing any minor problems.
>> 
> 
> I think it is quite likely at this point that your series will be accepted.
> 

Heh, maybe.

> However, from my experience with larger submissions like this one, it is quite common
> that the maintainers ask for all feedback to be addressed before that happens -
> especially for feedback like this, where there is common understanding that some
> changes to the code are necessary.
> 

Yep, I will have to spin out new versions addressing everything.

> Maybe the device tree maintainers could chime in at this point and let us all know
> what will need to happen to get the series accepted.
> 

That would be nice indeed.

> Thanks,
> Guenter
> 
> 

Regards

-- Pantelis

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Grant Likely]

On Wed, 06 Nov 2013 06:53:13 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Pantelis,
> 
> On 11/06/2013 01:34 AM, Pantelis Antoniou wrote:
> 
> >
> > As I mentioned earlier I'm trying to get this accepted in general term and then we'll
> > get around fixing any minor problems.
> >
> 
> I think it is quite likely at this point that your series will be accepted.
> 
> However, from my experience with larger submissions like this one, it is quite common
> that the maintainers ask for all feedback to be addressed before that happens -
> especially for feedback like this, where there is common understanding that some
> changes to the code are necessary.
> 
> Maybe the device tree maintainers could chime in at this point and let us all know
> what will need to happen to get the series accepted.

Since it is a big series, I would like to be able to "front-load" the
series of changes with straighforward changes that are easy to merge.
There are some exceptions though, such as when new APIs with users later
in the series. I don't really want to pull in a new API separately from
the user most of the time.

g.

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Grant Likely]

On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Introduce helper functions for working with the live DT tree.
> 
> __of_free_property() frees a dynamically created property
> __of_free_tree() recursively frees a device node tree
> __of_copy_property() copies a property dynamically
> __of_create_empty_node() creates an empty node
> __of_find_node_by_full_name() finds the node with the full name
> and
> of_multi_prop_cmp() performs a multi property compare but without
> having to take locks.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

So, this all looks like private stuff, or stuff that belongs in
drivers/of/base.c. Can you move stuff around. I've made more comments
below.

g.

> ---
>  drivers/of/Makefile |   2 +-
>  drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h  |  59 ++++++++++++
>  3 files changed, 313 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/of/util.c
> 
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index efd0510..9bc6d8c 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,4 +1,4 @@
> -obj-y = base.o device.o platform.o
> +obj-y = base.o device.o platform.o util.o
>  obj-$(CONFIG_OF_FLATTREE) += fdt.o
>  obj-$(CONFIG_OF_PROMTREE) += pdt.o
>  obj-$(CONFIG_OF_ADDRESS)  += address.o
> diff --git a/drivers/of/util.c b/drivers/of/util.c
> new file mode 100644
> index 0000000..5117e2b
> --- /dev/null
> +++ b/drivers/of/util.c
> @@ -0,0 +1,253 @@
> +/*
> + * Utility functions for working with device tree(s)
> + *
> + * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
> + * Copyright (C) 2012 Texas Instruments Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +
> +/**
> + * __of_free_property - release the memory of an allocated property
> + * @prop:	Property to release
> + *
> + * Release the memory of an allocated property only after checking
> + * that the property has been marked as OF_DYNAMIC.
> + * Only call on known allocated properties.
> + */
> +void __of_free_property(struct property *prop)
> +{
> +	if (prop == NULL)
> +		return;
> +
> +	if (of_property_check_flag(prop, OF_DYNAMIC)) {
> +		kfree(prop->value);
> +		kfree(prop->name);
> +		kfree(prop);
> +	} else {
> +		pr_warn("%s: property %p cannot be freed; memory is gone\n",
> +				__func__, prop);
> +	}
> +}
> +
> +/**
> + * __of_free_tree - release the memory of a device tree node and
> + *		    of all it's children + properties.
> + * @node:	Device Tree node to release
> + *
> + * Release the memory of a device tree node and of all it's children.
> + * Also release the properties and the dead properties.
> + * Only call on detached node trees, and you better be sure that
> + * no pointer exist for any properties. Only safe to do if you 
> + * absolutely control the life cycle of the node.
> + * Also note that the node is not removed from the all_nodes list,
> + * neither from the parent's child list; this should be handled before
> + * calling this function.
> + */
> +void __of_free_tree(struct device_node *node)
> +{
> +	struct property *prop;
> +	struct device_node *noden;
> +
> +	/* sanity check */
> +	if (!node)
> +		return;
> +
> +	/* free recursively any children */
> +	while ((noden = node->child) != NULL) {
> +		node->child = noden->sibling;
> +		__of_free_tree(noden);
> +	}
> +
> +	/* free every property already allocated */
> +	while ((prop = node->properties) != NULL) {
> +		node->properties = prop->next;
> +		__of_free_property(prop);
> +	}
> +
> +	/* free dead properties already allocated */
> +	while ((prop = node->deadprops) != NULL) {
> +		node->deadprops = prop->next;
> +		__of_free_property(prop);
> +	}
> +
> +	if (of_node_check_flag(node, OF_DYNAMIC)) {
> +		kfree(node->type);
> +		kfree(node->name);
> +		kfree(node);
> +	} else {
> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
> +				__func__, node);
> +	}
> +}

All of the above is potentially dangerous. There is no way to determine
if anything still holds a reference to a node. The proper way to handle
removal of properties is to have a release method when the last
of_node_put is called.

> +
> +/**
> + * __of_copy_property - Copy a property dynamically.
> + * @prop:	Property to copy
> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Copy a property by dynamically allocating the memory of both the
> + * property stucture and the property name & contents. The property's
> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> + * dynamically allocated properties and not.
> + * Returns the newly allocated property or NULL on out of memory error.
> + */

What do you intend the use-case to be for this function? Will the
duplicated property be immediately modified? If so, what happens if the
property needs to be grown in size?

> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
> +{
> +	struct property *propn;
> +
> +	propn = kzalloc(sizeof(*prop), flags);
> +	if (propn == NULL)
> +		return NULL;
> +
> +	propn->name = kstrdup(prop->name, flags);
> +	if (propn->name == NULL)
> +		goto err_fail_name;
> +
> +	if (prop->length > 0) {
> +		propn->value = kmalloc(prop->length, flags);
> +		if (propn->value == NULL)
> +			goto err_fail_value;
> +		memcpy(propn->value, prop->value, prop->length);
> +		propn->length = prop->length;
> +	}
> +
> +	/* mark the property as dynamic */
> +	of_property_set_flag(propn, OF_DYNAMIC);
> +
> +	return propn;
> +
> +err_fail_value:
> +	kfree(propn->name);
> +err_fail_name:
> +	kfree(propn);
> +	return NULL;
> +}
> +
> +/**
> + * __of_create_empty_node - Create an empty device node dynamically.
> + * @name:	Name of the new device node
> + * @type:	Type of the new device node
> + * @full_name:	Full name of the new device node
> + * @phandle:	Phandle of the new device node
> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Create an empty device tree node, suitable for further modification.
> + * The node data are dynamically allocated and all the node flags
> + * have the OF_DYNAMIC & OF_DETACHED bits set.
> + * Returns the newly allocated node or NULL on out of memory error.
> + */
> +struct device_node *__of_create_empty_node(
> +		const char *name, const char *type, const char *full_name,
> +		phandle phandle, gfp_t flags)

I would like to see a user of this function in the core DT paths that
allocate nodes. It will make for less chance of breakage if the fdt and
pdt paths change something, but this function isn't updated.

g.

> +{
> +	struct device_node *node;
> +
> +	node = kzalloc(sizeof(*node), flags);
> +	if (node == NULL)
> +		return NULL;
> +
> +	node->name = kstrdup(name, flags);
> +	if (node->name == NULL)
> +		goto err_return;
> +
> +	node->type = kstrdup(type, flags);
> +	if (node->type == NULL)
> +		goto err_return;
> +
> +	node->full_name = kstrdup(full_name, flags);
> +	if (node->type == NULL)
> +		goto err_return;

Again, who do you expect the user of this function to be? If it is part
of unflattening an overlay tree, is there a reason that the passed in
names cannot be used directly instead of kmallocing them?

> +
> +	node->phandle = phandle;
> +	kref_init(&node->kref);
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +
> +	return node;
> +
> +err_return:
> +	__of_free_tree(node);
> +	return NULL;
> +}
> +
> +/**
> + * __of_find_node_by_full_name - Find a node with the full name recursively
> + * @node:	Root of the tree to perform the search
> + * @full_name:	Full name of the node to find.
> + *
> + * Find a node with the give full name by recursively following any of 
> + * the child node links.
> + * Returns the matching node, or NULL if not found.
> + * Note that the devtree lock is not taken, so this function is only
> + * safe to call on either detached trees, or when devtree lock is already
> + * taken.
> + */
> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
> +		const char *full_name)

Sounds like something that should be in drivers/of/base.c

> +{
> +	struct device_node *child, *found;
> +
> +	if (node == NULL)
> +		return NULL;
> +
> +	/* check */
> +	if (of_node_cmp(node->full_name, full_name) == 0)
> +		return node;
> +
> +	__for_each_child_of_node(node, child) {
> +		found = __of_find_node_by_full_name(child, full_name);
> +		if (found != NULL)
> +			return found;
> +	}

I'm not a huge fan of recursive calls. Why doesn't a slightly modified
of_fund_node_by_name() work here?

I agree that of_find_node_by_name is not particularly elegant and it
would be good to have something more efficient, but it works and
following the same method would be consistent.

> +
> +	return NULL;
> +}
> +
> +/**
> + * of_multi_prop_cmp - Check if a property matches a value
> + * @prop:	Property to check
> + * @value:	Value to check against
> + *
> + * Check whether a property matches a value, using the standard
> + * of_compat_cmp() test on each string. It is similar to the test

of_compat_cmp() is only for compatible strings, and it purely to paper
over the way different OFW implementations work. Don't use the same
function. Don't use it for any property other than compatible because
that will just encourage bad behaviour.

> + * of_device_is_compatible() makes, but it can be performed without
> + * taking the devtree_lock, which is required in some cases.
> + * Returns 0 on a match, -1 on no match.

That's what __of_device_is_compatible() is for. It is a version of the
function that doesn't take the lock.

> + */
> +int of_multi_prop_cmp(const struct property *prop, const char *value)
> +{
> +	const char *cp;
> +	int cplen, vlen, l;
> +
> +	if (prop == NULL || value == NULL)
> +		return -1;
> +
> +	cp = prop->value;
> +	cplen = prop->length;
> +	vlen = strlen(value);
> +
> +	while (cplen > 0) {
> +		if (of_compat_cmp(cp, value, vlen) == 0)
> +			return 0;
> +		l = strlen(cp) + 1;
> +		cp += l;
> +		cplen -= l;
> +	}
> +
> +	return -1;
> +}
> +
> diff --git a/include/linux/of.h b/include/linux/of.h
> index a56c450..9d69bd2 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -662,4 +662,63 @@ extern void proc_device_tree_update_prop(struct proc_dir_entry *pde,
>  					 struct property *oldprop);
>  #endif
>  
> +/**
> + * General utilities for working with live trees.
> + *
> + * All functions with two leading underscores operate
> + * without taking node references, so you either have to
> + * own the devtree lock or work on detached trees only.
> + */
> +
> +#ifdef CONFIG_OF
> +
> +/* iterator for internal use; not references, neither affects devtree lock */
> +#define __for_each_child_of_node(dn, chld) \
> +	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
> +
> +void __of_free_property(struct property *prop);
> +void __of_free_tree(struct device_node *node);
> +struct property *__of_copy_property(const struct property *prop, gfp_t flags);
> +struct device_node *__of_create_empty_node( const char *name,
> +		const char *type, const char *full_name,
> +		phandle phandle, gfp_t flags);
> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
> +		const char *full_name);
> +int of_multi_prop_cmp(const struct property *prop, const char *value);
> +
> +#else /* !CONFIG_OF */
> +
> +#define __for_each_child_of_node(dn, chld) \
> +	while (0)
> +
> +static inline void __of_free_property(struct property *prop) { }
> +
> +static inline void __of_free_tree(struct device_node *node) { }
> +
> +static inline struct property *__of_copy_property(const struct property *prop,
> +		gfp_t flags)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *__of_create_empty_node( const char *name,
> +		const char *type, const char *full_name,
> +		phandle phandle, gfp_t flags)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
> +		const char *full_name)
> +{
> +	return NULL;
> +}
> +
> +static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
> +{
> +	return -1;
> +}
> +
> +#endif	/* !CONFIG_OF */
> +
>  #endif /* _LINUX_OF_H */
> -- 
> 1.7.12
> 

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Grant,

On Nov 11, 2013, at 5:37 PM, Grant Likely wrote:

> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Introduce helper functions for working with the live DT tree.
>> 
>> __of_free_property() frees a dynamically created property
>> __of_free_tree() recursively frees a device node tree
>> __of_copy_property() copies a property dynamically
>> __of_create_empty_node() creates an empty node
>> __of_find_node_by_full_name() finds the node with the full name
>> and
>> of_multi_prop_cmp() performs a multi property compare but without
>> having to take locks.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> So, this all looks like private stuff, or stuff that belongs in
> drivers/of/base.c. Can you move stuff around. I've made more comments
> below.
> 

Placement is no big issue;

> g.
> 

[snip]

>> +	} else {
>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>> +				__func__, node);
>> +	}
>> +}
> 
> All of the above is potentially dangerous. There is no way to determine
> if anything still holds a reference to a node. The proper way to handle
> removal of properties is to have a release method when the last
> of_node_put is called.
> 

This is safe, and expected to be called only on a dynamically created tree,
that's what all the checks against OF_DYNAMIC guard against.

It is not ever meant to be called on an arbitrary tree, created by unflattening
a blob.

Perhaps we could have a switch to control whether an unflattened tree is 
created dynamically and then freeing/releasing will work.

kobject-ifcation will require it anyway, don't you agree? 

>> +
>> +/**
>> + * __of_copy_property - Copy a property dynamically.
>> + * @prop:	Property to copy
>> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
>> + *
>> + * Copy a property by dynamically allocating the memory of both the
>> + * property stucture and the property name & contents. The property's
>> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
>> + * dynamically allocated properties and not.
>> + * Returns the newly allocated property or NULL on out of memory error.
>> + */
> 
> What do you intend the use-case to be for this function? Will the
> duplicated property be immediately modified? If so, what happens if the
> property needs to be grown in size?
> 

No, the property will no be modified. If it needs to grow it will be moved to 
deadprops (since we don't track refs to props) and a new one will be allocated.

>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
>> +{
>> +	struct property *propn;
>> +
>> +	propn = kzalloc(sizeof(*prop), flags);
>> +	if (propn == NULL)
>> +		return NULL;
>> +
>> +	propn->name = kstrdup(prop->name, flags);
>> +	if (propn->name == NULL)
>> +		goto err_fail_name;
>> +
>> +	if (prop->length > 0) {
>> +		propn->value = kmalloc(prop->length, flags);
>> +		if (propn->value == NULL)
>> +			goto err_fail_value;
>> +		memcpy(propn->value, prop->value, prop->length);
>> +		propn->length = prop->length;
>> +	}
>> +
>> +	/* mark the property as dynamic */
>> +	of_property_set_flag(propn, OF_DYNAMIC);
>> +
>> +	return propn;
>> +
>> +err_fail_value:
>> +	kfree(propn->name);
>> +err_fail_name:
>> +	kfree(propn);
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * __of_create_empty_node - Create an empty device node dynamically.
>> + * @name:	Name of the new device node
>> + * @type:	Type of the new device node
>> + * @full_name:	Full name of the new device node
>> + * @phandle:	Phandle of the new device node
>> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
>> + *
>> + * Create an empty device tree node, suitable for further modification.
>> + * The node data are dynamically allocated and all the node flags
>> + * have the OF_DYNAMIC & OF_DETACHED bits set.
>> + * Returns the newly allocated node or NULL on out of memory error.
>> + */
>> +struct device_node *__of_create_empty_node(
>> +		const char *name, const char *type, const char *full_name,
>> +		phandle phandle, gfp_t flags)
> 
> I would like to see a user of this function in the core DT paths that
> allocate nodes. It will make for less chance of breakage if the fdt and
> pdt paths change something, but this function isn't updated.
> 

Hmm, where do you think it can be used? Perhaps the unflatten parts?

> g.
> 
>> +{
>> +	struct device_node *node;
>> +
>> +	node = kzalloc(sizeof(*node), flags);
>> +	if (node == NULL)
>> +		return NULL;
>> +
>> +	node->name = kstrdup(name, flags);
>> +	if (node->name == NULL)
>> +		goto err_return;
>> +
>> +	node->type = kstrdup(type, flags);
>> +	if (node->type == NULL)
>> +		goto err_return;
>> +
>> +	node->full_name = kstrdup(full_name, flags);
>> +	if (node->type == NULL)
>> +		goto err_return;
> 
> Again, who do you expect the user of this function to be? If it is part
> of unflattening an overlay tree, is there a reason that the passed in
> names cannot be used directly instead of kmallocing them?
> 

I want to be able to get rid of the blob eventually; I don't need to keep
dragging it around.

>> +
>> +	node->phandle = phandle;
>> +	kref_init(&node->kref);
>> +	of_node_set_flag(node, OF_DYNAMIC);
>> +	of_node_set_flag(node, OF_DETACHED);
>> +
>> +	return node;
>> +
>> +err_return:
>> +	__of_free_tree(node);
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * __of_find_node_by_full_name - Find a node with the full name recursively
>> + * @node:	Root of the tree to perform the search
>> + * @full_name:	Full name of the node to find.
>> + *
>> + * Find a node with the give full name by recursively following any of 
>> + * the child node links.
>> + * Returns the matching node, or NULL if not found.
>> + * Note that the devtree lock is not taken, so this function is only
>> + * safe to call on either detached trees, or when devtree lock is already
>> + * taken.
>> + */
>> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> +		const char *full_name)
> 
> Sounds like something that should be in drivers/of/base.c
> 

Yes.

>> +{
>> +	struct device_node *child, *found;
>> +
>> +	if (node == NULL)
>> +		return NULL;
>> +
>> +	/* check */
>> +	if (of_node_cmp(node->full_name, full_name) == 0)
>> +		return node;
>> +
>> +	__for_each_child_of_node(node, child) {
>> +		found = __of_find_node_by_full_name(child, full_name);
>> +		if (found != NULL)
>> +			return found;
>> +	}
> 
> I'm not a huge fan of recursive calls. Why doesn't a slightly modified
> of_fund_node_by_name() work here?
> 
> I agree that of_find_node_by_name is not particularly elegant and it
> would be good to have something more efficient, but it works and
> following the same method would be consistent.
> 

of_find_node_by_name is not iterating on the passed tree and it's subnodes,
it is a global search starting from:

	np = from ? from->allnext : of_allnodes;

This is just broken, since it depends on unflattening creating nodes in the
allnodes list in sequence. I.e. that child nodes are after the parent node.
This assumption breaks down when doing dynamic insertions of nodes.
A detached tree in not linked on of_allnodes anyway.

>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * of_multi_prop_cmp - Check if a property matches a value
>> + * @prop:	Property to check
>> + * @value:	Value to check against
>> + *
>> + * Check whether a property matches a value, using the standard
>> + * of_compat_cmp() test on each string. It is similar to the test
> 
> of_compat_cmp() is only for compatible strings, and it purely to paper
> over the way different OFW implementations work. Don't use the same
> function. Don't use it for any property other than compatible because
> that will just encourage bad behaviour.
> 

OK

>> + * of_device_is_compatible() makes, but it can be performed without
>> + * taking the devtree_lock, which is required in some cases.
>> + * Returns 0 on a match, -1 on no match.
> 
> That's what __of_device_is_compatible() is for. It is a version of the
> function that doesn't take the lock.

I see. The original version of the patches started before this was
available. Will remove.

> 
>> + */
>> +int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> +	const char *cp;
>> +

[snip]

Regards

-- Pantelis

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Grant Likely]

On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Grant,
> 
> On Nov 11, 2013, at 5:37 PM, Grant Likely wrote:
> 
> > On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >> Introduce helper functions for working with the live DT tree.
> >> 
> >> __of_free_property() frees a dynamically created property
> >> __of_free_tree() recursively frees a device node tree
> >> __of_copy_property() copies a property dynamically
> >> __of_create_empty_node() creates an empty node
> >> __of_find_node_by_full_name() finds the node with the full name
> >> and
> >> of_multi_prop_cmp() performs a multi property compare but without
> >> having to take locks.
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> > 
> > So, this all looks like private stuff, or stuff that belongs in
> > drivers/of/base.c. Can you move stuff around. I've made more comments
> > below.
> > 
> 
> Placement is no big issue;
> 
> > g.
> > 
> 
> [snip]
> 
> >> +	} else {
> >> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
> >> +				__func__, node);
> >> +	}
> >> +}
> > 
> > All of the above is potentially dangerous. There is no way to determine
> > if anything still holds a reference to a node. The proper way to handle
> > removal of properties is to have a release method when the last
> > of_node_put is called.
> > 
> 
> This is safe, and expected to be called only on a dynamically created tree,
> that's what all the checks against OF_DYNAMIC guard against.
> 
> It is not ever meant to be called on an arbitrary tree, created by unflattening
> a blob.

I am talking about when being used on a dynamic tree. The problem is
when a driver or other code holds a reference to a dynamic nodes, but
doesn't release it correctly. The memory must not be freed until all of
the references are relased. OF_DYNAMIC doesn't actually help in that
case, and it is the reason for of_node_get()/of_node_put()

> Perhaps we could have a switch to control whether an unflattened tree is 
> created dynamically and then freeing/releasing will work.
> 
> kobject-ifcation will require it anyway, don't you agree? 

Yes. Kobjectifcation will also take care of the release method.

> 
> >> +
> >> +/**
> >> + * __of_copy_property - Copy a property dynamically.
> >> + * @prop:	Property to copy
> >> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
> >> + *
> >> + * Copy a property by dynamically allocating the memory of both the
> >> + * property stucture and the property name & contents. The property's
> >> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> >> + * dynamically allocated properties and not.
> >> + * Returns the newly allocated property or NULL on out of memory error.
> >> + */
> > 
> > What do you intend the use-case to be for this function? Will the
> > duplicated property be immediately modified? If so, what happens if the
> > property needs to be grown in size?
> > 
> 
> No, the property will no be modified. If it needs to grow it will be moved to 
> deadprops (since we don't track refs to props) and a new one will be allocated.
> 
> >> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
> >> +{
> >> +	struct property *propn;
> >> +
> >> +	propn = kzalloc(sizeof(*prop), flags);
> >> +	if (propn == NULL)
> >> +		return NULL;
> >> +
> >> +	propn->name = kstrdup(prop->name, flags);
> >> +	if (propn->name == NULL)
> >> +		goto err_fail_name;
> >> +
> >> +	if (prop->length > 0) {
> >> +		propn->value = kmalloc(prop->length, flags);
> >> +		if (propn->value == NULL)
> >> +			goto err_fail_value;
> >> +		memcpy(propn->value, prop->value, prop->length);
> >> +		propn->length = prop->length;
> >> +	}
> >> +
> >> +	/* mark the property as dynamic */
> >> +	of_property_set_flag(propn, OF_DYNAMIC);
> >> +
> >> +	return propn;
> >> +
> >> +err_fail_value:
> >> +	kfree(propn->name);
> >> +err_fail_name:
> >> +	kfree(propn);
> >> +	return NULL;
> >> +}
> >> +
> >> +/**
> >> + * __of_create_empty_node - Create an empty device node dynamically.
> >> + * @name:	Name of the new device node
> >> + * @type:	Type of the new device node
> >> + * @full_name:	Full name of the new device node
> >> + * @phandle:	Phandle of the new device node
> >> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
> >> + *
> >> + * Create an empty device tree node, suitable for further modification.
> >> + * The node data are dynamically allocated and all the node flags
> >> + * have the OF_DYNAMIC & OF_DETACHED bits set.
> >> + * Returns the newly allocated node or NULL on out of memory error.
> >> + */
> >> +struct device_node *__of_create_empty_node(
> >> +		const char *name, const char *type, const char *full_name,
> >> +		phandle phandle, gfp_t flags)
> > 
> > I would like to see a user of this function in the core DT paths that
> > allocate nodes. It will make for less chance of breakage if the fdt and
> > pdt paths change something, but this function isn't updated.
> > 
> 
> Hmm, where do you think it can be used? Perhaps the unflatten parts?

Yes. In unflattening the tree, fdt.c and in extracting the trea from
real OFW (pdt.c).

> 
> > g.
> > 
> >> +{
> >> +	struct device_node *node;
> >> +
> >> +	node = kzalloc(sizeof(*node), flags);
> >> +	if (node == NULL)
> >> +		return NULL;
> >> +
> >> +	node->name = kstrdup(name, flags);
> >> +	if (node->name == NULL)
> >> +		goto err_return;
> >> +
> >> +	node->type = kstrdup(type, flags);
> >> +	if (node->type == NULL)
> >> +		goto err_return;
> >> +
> >> +	node->full_name = kstrdup(full_name, flags);
> >> +	if (node->type == NULL)
> >> +		goto err_return;
> > 
> > Again, who do you expect the user of this function to be? If it is part
> > of unflattening an overlay tree, is there a reason that the passed in
> > names cannot be used directly instead of kmallocing them?
> > 
> 
> I want to be able to get rid of the blob eventually; I don't need to keep
> dragging it around.

Why? It really doesn't hurt and it means data does not need to be
copied.

> 
> >> +
> >> +	node->phandle = phandle;
> >> +	kref_init(&node->kref);
> >> +	of_node_set_flag(node, OF_DYNAMIC);
> >> +	of_node_set_flag(node, OF_DETACHED);
> >> +
> >> +	return node;
> >> +
> >> +err_return:
> >> +	__of_free_tree(node);
> >> +	return NULL;
> >> +}
> >> +
> >> +/**
> >> + * __of_find_node_by_full_name - Find a node with the full name recursively
> >> + * @node:	Root of the tree to perform the search
> >> + * @full_name:	Full name of the node to find.
> >> + *
> >> + * Find a node with the give full name by recursively following any of 
> >> + * the child node links.
> >> + * Returns the matching node, or NULL if not found.
> >> + * Note that the devtree lock is not taken, so this function is only
> >> + * safe to call on either detached trees, or when devtree lock is already
> >> + * taken.
> >> + */
> >> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >> +		const char *full_name)
> > 
> > Sounds like something that should be in drivers/of/base.c
> > 
> 
> Yes.
> 
> >> +{
> >> +	struct device_node *child, *found;
> >> +
> >> +	if (node == NULL)
> >> +		return NULL;
> >> +
> >> +	/* check */
> >> +	if (of_node_cmp(node->full_name, full_name) == 0)
> >> +		return node;
> >> +
> >> +	__for_each_child_of_node(node, child) {
> >> +		found = __of_find_node_by_full_name(child, full_name);
> >> +		if (found != NULL)
> >> +			return found;
> >> +	}
> > 
> > I'm not a huge fan of recursive calls. Why doesn't a slightly modified
> > of_fund_node_by_name() work here?
> > 
> > I agree that of_find_node_by_name is not particularly elegant and it
> > would be good to have something more efficient, but it works and
> > following the same method would be consistent.
> > 
> 
> of_find_node_by_name is not iterating on the passed tree and it's subnodes,
> it is a global search starting from:
> 
> 	np = from ? from->allnext : of_allnodes;
> 
> This is just broken, since it depends on unflattening creating nodes in the
> allnodes list in sequence. I.e. that child nodes are after the parent node.
> This assumption breaks down when doing dynamic insertions of nodes.
> A detached tree in not linked on of_allnodes anyway.

It would be good to have a root-of-tree structure for both the core tree
and the overlay trees so that a common iterator can be implemented.

g.

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Grant,

On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:

> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Hi Grant,
>> 
>> On Nov 11, 2013, at 5:37 PM, Grant Likely wrote:
>> 
>>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>> Introduce helper functions for working with the live DT tree.
>>>> 
>>>> __of_free_property() frees a dynamically created property
>>>> __of_free_tree() recursively frees a device node tree
>>>> __of_copy_property() copies a property dynamically
>>>> __of_create_empty_node() creates an empty node
>>>> __of_find_node_by_full_name() finds the node with the full name
>>>> and
>>>> of_multi_prop_cmp() performs a multi property compare but without
>>>> having to take locks.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>> 
>>> So, this all looks like private stuff, or stuff that belongs in
>>> drivers/of/base.c. Can you move stuff around. I've made more comments
>>> below.
>>> 
>> 
>> Placement is no big issue;
>> 
>>> g.
>>> 
>> 
>> [snip]
>> 
>>>> +	} else {
>>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>>>> +				__func__, node);
>>>> +	}
>>>> +}
>>> 
>>> All of the above is potentially dangerous. There is no way to determine
>>> if anything still holds a reference to a node. The proper way to handle
>>> removal of properties is to have a release method when the last
>>> of_node_put is called.
>>> 
>> 
>> This is safe, and expected to be called only on a dynamically created tree,
>> that's what all the checks against OF_DYNAMIC guard against.
>> 
>> It is not ever meant to be called on an arbitrary tree, created by unflattening
>> a blob.
> 
> I am talking about when being used on a dynamic tree. The problem is
> when a driver or other code holds a reference to a dynamic nodes, but
> doesn't release it correctly. The memory must not be freed until all of
> the references are relased. OF_DYNAMIC doesn't actually help in that
> case, and it is the reason for of_node_get()/of_node_put()
> 

I know, but even that is not enough. of_node_get()/of_node_put() handles the
case of references to the nodes, but not what happens with references to
properties. deadprops is mitigating the problem somewhat, but if we're going
to go to all the trouble of kobjectification let's do the props as well.

of_get_property could be modified to return a devm_kmalloced copy of the real
property and that would deal with most of the callers. Of course for
the small sized scalar data we can avoid the copy.

By using the devm_* interface we also avoid having to mess too much with the callers.

I.e. what about something like devm_of_get_property()?

>> Perhaps we could have a switch to control whether an unflattened tree is 
>> created dynamically and then freeing/releasing will work.
>> 
>> kobject-ifcation will require it anyway, don't you agree? 
> 
> Yes. Kobjectifcation will also take care of the release method.
> 
>> 
>>>> +
>>>> +/**
>>>> + * __of_copy_property - Copy a property dynamically.
>>>> + * @prop:	Property to copy
>>>> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
>>>> + *
>>>> + * Copy a property by dynamically allocating the memory of both the
>>>> + * property stucture and the property name & contents. The property's
>>>> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
>>>> + * dynamically allocated properties and not.
>>>> + * Returns the newly allocated property or NULL on out of memory error.
>>>> + */
>>> 
>>> What do you intend the use-case to be for this function? Will the
>>> duplicated property be immediately modified? If so, what happens if the
>>> property needs to be grown in size?
>>> 
>> 
>> No, the property will no be modified. If it needs to grow it will be moved to 
>> deadprops (since we don't track refs to props) and a new one will be allocated.
>> 
>>>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
>>>> +{
>>>> +	struct property *propn;
>>>> +
>>>> +	propn = kzalloc(sizeof(*prop), flags);
>>>> +	if (propn == NULL)
>>>> +		return NULL;
>>>> +
>>>> +	propn->name = kstrdup(prop->name, flags);
>>>> +	if (propn->name == NULL)
>>>> +		goto err_fail_name;
>>>> +
>>>> +	if (prop->length > 0) {
>>>> +		propn->value = kmalloc(prop->length, flags);
>>>> +		if (propn->value == NULL)
>>>> +			goto err_fail_value;
>>>> +		memcpy(propn->value, prop->value, prop->length);
>>>> +		propn->length = prop->length;
>>>> +	}
>>>> +
>>>> +	/* mark the property as dynamic */
>>>> +	of_property_set_flag(propn, OF_DYNAMIC);
>>>> +
>>>> +	return propn;
>>>> +
>>>> +err_fail_value:
>>>> +	kfree(propn->name);
>>>> +err_fail_name:
>>>> +	kfree(propn);
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * __of_create_empty_node - Create an empty device node dynamically.
>>>> + * @name:	Name of the new device node
>>>> + * @type:	Type of the new device node
>>>> + * @full_name:	Full name of the new device node
>>>> + * @phandle:	Phandle of the new device node
>>>> + * @flags:	Allocation flags (typically pass GFP_KERNEL)
>>>> + *
>>>> + * Create an empty device tree node, suitable for further modification.
>>>> + * The node data are dynamically allocated and all the node flags
>>>> + * have the OF_DYNAMIC & OF_DETACHED bits set.
>>>> + * Returns the newly allocated node or NULL on out of memory error.
>>>> + */
>>>> +struct device_node *__of_create_empty_node(
>>>> +		const char *name, const char *type, const char *full_name,
>>>> +		phandle phandle, gfp_t flags)
>>> 
>>> I would like to see a user of this function in the core DT paths that
>>> allocate nodes. It will make for less chance of breakage if the fdt and
>>> pdt paths change something, but this function isn't updated.
>>> 
>> 
>> Hmm, where do you think it can be used? Perhaps the unflatten parts?
> 
> Yes. In unflattening the tree, fdt.c and in extracting the trea from
> real OFW (pdt.c).
> 
>> 
>>> g.
>>> 
>>>> +{
>>>> +	struct device_node *node;
>>>> +
>>>> +	node = kzalloc(sizeof(*node), flags);
>>>> +	if (node == NULL)
>>>> +		return NULL;
>>>> +
>>>> +	node->name = kstrdup(name, flags);
>>>> +	if (node->name == NULL)
>>>> +		goto err_return;
>>>> +
>>>> +	node->type = kstrdup(type, flags);
>>>> +	if (node->type == NULL)
>>>> +		goto err_return;
>>>> +
>>>> +	node->full_name = kstrdup(full_name, flags);
>>>> +	if (node->type == NULL)
>>>> +		goto err_return;
>>> 
>>> Again, who do you expect the user of this function to be? If it is part
>>> of unflattening an overlay tree, is there a reason that the passed in
>>> names cannot be used directly instead of kmallocing them?
>>> 
>> 
>> I want to be able to get rid of the blob eventually; I don't need to keep
>> dragging it around.
> 
> Why? It really doesn't hurt and it means data does not need to be
> copied.

Copying data lead to less problems that having to drag that blob around. 
That's just preference, so not a big issue.

> 
>> 
>>>> +
>>>> +	node->phandle = phandle;
>>>> +	kref_init(&node->kref);
>>>> +	of_node_set_flag(node, OF_DYNAMIC);
>>>> +	of_node_set_flag(node, OF_DETACHED);
>>>> +
>>>> +	return node;
>>>> +
>>>> +err_return:
>>>> +	__of_free_tree(node);
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * __of_find_node_by_full_name - Find a node with the full name recursively
>>>> + * @node:	Root of the tree to perform the search
>>>> + * @full_name:	Full name of the node to find.
>>>> + *
>>>> + * Find a node with the give full name by recursively following any of 
>>>> + * the child node links.
>>>> + * Returns the matching node, or NULL if not found.
>>>> + * Note that the devtree lock is not taken, so this function is only
>>>> + * safe to call on either detached trees, or when devtree lock is already
>>>> + * taken.
>>>> + */
>>>> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
>>>> +		const char *full_name)
>>> 
>>> Sounds like something that should be in drivers/of/base.c
>>> 
>> 
>> Yes.
>> 
>>>> +{
>>>> +	struct device_node *child, *found;
>>>> +
>>>> +	if (node == NULL)
>>>> +		return NULL;
>>>> +
>>>> +	/* check */
>>>> +	if (of_node_cmp(node->full_name, full_name) == 0)
>>>> +		return node;
>>>> +
>>>> +	__for_each_child_of_node(node, child) {
>>>> +		found = __of_find_node_by_full_name(child, full_name);
>>>> +		if (found != NULL)
>>>> +			return found;
>>>> +	}
>>> 
>>> I'm not a huge fan of recursive calls. Why doesn't a slightly modified
>>> of_fund_node_by_name() work here?
>>> 
>>> I agree that of_find_node_by_name is not particularly elegant and it
>>> would be good to have something more efficient, but it works and
>>> following the same method would be consistent.
>>> 
>> 
>> of_find_node_by_name is not iterating on the passed tree and it's subnodes,
>> it is a global search starting from:
>> 
>> 	np = from ? from->allnext : of_allnodes;
>> 
>> This is just broken, since it depends on unflattening creating nodes in the
>> allnodes list in sequence. I.e. that child nodes are after the parent node.
>> This assumption breaks down when doing dynamic insertions of nodes.
>> A detached tree in not linked on of_allnodes anyway.
> 
> It would be good to have a root-of-tree structure for both the core tree
> and the overlay trees so that a common iterator can be implemented.
> 

I don't know. The overlay trees while being built are not affecting the
kernel in any way until they're applied. They're just a bunch of private
data.

Consider the case of an overlay tree that's not applied, but contains bad 
data, for example a name of a node clashes with one of the live tree. 
Linking it into the kernel's real live tree can lead to problems, like
shadowing the real node.

> g.
> 

Regards

-- Pantelis

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Grant Likely]

On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
> > On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >>>> +	} else {
> >>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
> >>>> +				__func__, node);
> >>>> +	}
> >>>> +}
> >>> 
> >>> All of the above is potentially dangerous. There is no way to determine
> >>> if anything still holds a reference to a node. The proper way to handle
> >>> removal of properties is to have a release method when the last
> >>> of_node_put is called.
> >>> 
> >> 
> >> This is safe, and expected to be called only on a dynamically created tree,
> >> that's what all the checks against OF_DYNAMIC guard against.
> >> 
> >> It is not ever meant to be called on an arbitrary tree, created by unflattening
> >> a blob.
> > 
> > I am talking about when being used on a dynamic tree. The problem is
> > when a driver or other code holds a reference to a dynamic nodes, but
> > doesn't release it correctly. The memory must not be freed until all of
> > the references are relased. OF_DYNAMIC doesn't actually help in that
> > case, and it is the reason for of_node_get()/of_node_put()
> > 
> 
> I know, but even that is not enough. of_node_get()/of_node_put() handles the
> case of references to the nodes, but not what happens with references to
> properties. deadprops is mitigating the problem somewhat, but if we're going
> to go to all the trouble of kobjectification let's do the props as well.
> 
> of_get_property could be modified to return a devm_kmalloced copy of the real
> property and that would deal with most of the callers. Of course for
> the small sized scalar data we can avoid the copy.
> 
> By using the devm_* interface we also avoid having to mess too much with the callers.
> 
> I.e. what about something like devm_of_get_property()?

Reference counting is already a horrible pain to keep correct. I don't
see a better way to handle it in the dynamic case, so we're stuck with
it, but I don't want to make it any harder. Adding ref counting to
properties will make it harder than it already is to get the code right.
I'm absolutely fine with a little bit of wasted memory in the form of
deadprops when the alternative is so horrible. References at the node
level is enough granularity.

I don't think kduping the property is the solution either. I strongly
suspect that will be far more expensive than the deadprop solution.

> >>>> +{
> >>>> +	struct device_node *node;
> >>>> +
> >>>> +	node = kzalloc(sizeof(*node), flags);
> >>>> +	if (node == NULL)
> >>>> +		return NULL;
> >>>> +
> >>>> +	node->name = kstrdup(name, flags);
> >>>> +	if (node->name == NULL)
> >>>> +		goto err_return;
> >>>> +
> >>>> +	node->type = kstrdup(type, flags);
> >>>> +	if (node->type == NULL)
> >>>> +		goto err_return;
> >>>> +
> >>>> +	node->full_name = kstrdup(full_name, flags);
> >>>> +	if (node->type == NULL)
> >>>> +		goto err_return;
> >>> 
> >>> Again, who do you expect the user of this function to be? If it is part
> >>> of unflattening an overlay tree, is there a reason that the passed in
> >>> names cannot be used directly instead of kmallocing them?
> >>> 
> >> 
> >> I want to be able to get rid of the blob eventually; I don't need to keep
> >> dragging it around.
> > 
> > Why? It really doesn't hurt and it means data does not need to be
> > copied.
> 
> Copying data lead to less problems that having to drag that blob around. 
> That's just preference, so not a big issue.

Can you elaborate? What problems do you foresee being created by keeping
the blob?

g.

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Grant,

On Nov 14, 2013, at 2:44 AM, Grant Likely wrote:

> On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
>>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>>>> +	} else {
>>>>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>>>>>> +				__func__, node);
>>>>>> +	}
>>>>>> +}
>>>>> 
>>>>> All of the above is potentially dangerous. There is no way to determine
>>>>> if anything still holds a reference to a node. The proper way to handle
>>>>> removal of properties is to have a release method when the last
>>>>> of_node_put is called.
>>>>> 
>>>> 
>>>> This is safe, and expected to be called only on a dynamically created tree,
>>>> that's what all the checks against OF_DYNAMIC guard against.
>>>> 
>>>> It is not ever meant to be called on an arbitrary tree, created by unflattening
>>>> a blob.
>>> 
>>> I am talking about when being used on a dynamic tree. The problem is
>>> when a driver or other code holds a reference to a dynamic nodes, but
>>> doesn't release it correctly. The memory must not be freed until all of
>>> the references are relased. OF_DYNAMIC doesn't actually help in that
>>> case, and it is the reason for of_node_get()/of_node_put()
>>> 
>> 
>> I know, but even that is not enough. of_node_get()/of_node_put() handles the
>> case of references to the nodes, but not what happens with references to
>> properties. deadprops is mitigating the problem somewhat, but if we're going
>> to go to all the trouble of kobjectification let's do the props as well.
>> 
>> of_get_property could be modified to return a devm_kmalloced copy of the real
>> property and that would deal with most of the callers. Of course for
>> the small sized scalar data we can avoid the copy.
>> 
>> By using the devm_* interface we also avoid having to mess too much with the callers.
>> 
>> I.e. what about something like devm_of_get_property()?
> 
> Reference counting is already a horrible pain to keep correct. I don't
> see a better way to handle it in the dynamic case, so we're stuck with
> it, but I don't want to make it any harder. Adding ref counting to
> properties will make it harder than it already is to get the code right.
> I'm absolutely fine with a little bit of wasted memory in the form of
> deadprops when the alternative is so horrible. References at the node
> level is enough granularity.
> 
> I don't think kduping the property is the solution either. I strongly
> suspect that will be far more expensive than the deadprop solution.
> 

As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property()
makes sense for new drivers though? What do you think? Perhaps copying to a
user supplied buffer as well?

>>>>>> +{
>>>>>> +	struct device_node *node;
>>>>>> +
>>>>>> +	node = kzalloc(sizeof(*node), flags);
>>>>>> +	if (node == NULL)
>>>>>> +		return NULL;
>>>>>> +
>>>>>> +	node->name = kstrdup(name, flags);
>>>>>> +	if (node->name == NULL)
>>>>>> +		goto err_return;
>>>>>> +
>>>>>> +	node->type = kstrdup(type, flags);
>>>>>> +	if (node->type == NULL)
>>>>>> +		goto err_return;
>>>>>> +
>>>>>> +	node->full_name = kstrdup(full_name, flags);
>>>>>> +	if (node->type == NULL)
>>>>>> +		goto err_return;
>>>>> 
>>>>> Again, who do you expect the user of this function to be? If it is part
>>>>> of unflattening an overlay tree, is there a reason that the passed in
>>>>> names cannot be used directly instead of kmallocing them?
>>>>> 
>>>> 
>>>> I want to be able to get rid of the blob eventually; I don't need to keep
>>>> dragging it around.
>>> 
>>> Why? It really doesn't hurt and it means data does not need to be
>>> copied.
>> 
>> Copying data lead to less problems that having to drag that blob around. 
>> That's just preference, so not a big issue.
> 
> Can you elaborate? What problems do you foresee being created by keeping
> the blob?

It's a kind of drag. That means you get handed a device_node pointer you are not
able to free it without having the blob along with the container/accessor of it.
I.e. For the normal case where the blob comes from a request_firmware() call
You have to keep the firmware structure around.

Depending on what other method you're going to use tends to make the code a little
bit messier. 

> 
> g.

Regards

-- Pantelis

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Grant Likely]

On Thu, 14 Nov 2013 10:51:05 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Grant,
> 
> On Nov 14, 2013, at 2:44 AM, Grant Likely wrote:
> 
> > On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
> >>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >>>>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >>>>>> +	} else {
> >>>>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
> >>>>>> +				__func__, node);
> >>>>>> +	}
> >>>>>> +}
> >>>>> 
> >>>>> All of the above is potentially dangerous. There is no way to determine
> >>>>> if anything still holds a reference to a node. The proper way to handle
> >>>>> removal of properties is to have a release method when the last
> >>>>> of_node_put is called.
> >>>>> 
> >>>> 
> >>>> This is safe, and expected to be called only on a dynamically created tree,
> >>>> that's what all the checks against OF_DYNAMIC guard against.
> >>>> 
> >>>> It is not ever meant to be called on an arbitrary tree, created by unflattening
> >>>> a blob.
> >>> 
> >>> I am talking about when being used on a dynamic tree. The problem is
> >>> when a driver or other code holds a reference to a dynamic nodes, but
> >>> doesn't release it correctly. The memory must not be freed until all of
> >>> the references are relased. OF_DYNAMIC doesn't actually help in that
> >>> case, and it is the reason for of_node_get()/of_node_put()
> >>> 
> >> 
> >> I know, but even that is not enough. of_node_get()/of_node_put() handles the
> >> case of references to the nodes, but not what happens with references to
> >> properties. deadprops is mitigating the problem somewhat, but if we're going
> >> to go to all the trouble of kobjectification let's do the props as well.
> >> 
> >> of_get_property could be modified to return a devm_kmalloced copy of the real
> >> property and that would deal with most of the callers. Of course for
> >> the small sized scalar data we can avoid the copy.
> >> 
> >> By using the devm_* interface we also avoid having to mess too much with the callers.
> >> 
> >> I.e. what about something like devm_of_get_property()?
> > 
> > Reference counting is already a horrible pain to keep correct. I don't
> > see a better way to handle it in the dynamic case, so we're stuck with
> > it, but I don't want to make it any harder. Adding ref counting to
> > properties will make it harder than it already is to get the code right.
> > I'm absolutely fine with a little bit of wasted memory in the form of
> > deadprops when the alternative is so horrible. References at the node
> > level is enough granularity.
> > 
> > I don't think kduping the property is the solution either. I strongly
> > suspect that will be far more expensive than the deadprop solution.
> > 
> 
> As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property()
> makes sense for new drivers though? What do you think? Perhaps copying to a
> user supplied buffer as well?

I still don't think it is necessary. The device lifetime should always
be shorter than the node lifetime.

> It's a kind of drag. That means you get handed a device_node pointer you are not
> able to free it without having the blob along with the container/accessor of it.
> I.e. For the normal case where the blob comes from a request_firmware() call
> You have to keep the firmware structure around.
> 
> Depending on what other method you're going to use tends to make the code a little
> bit messier. 

Understood. Stick with keeping the blob around for now. It can be
reworkd in the future if necessary since there are no associated
userspace ABI issues.

g.

Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Grant,

On Nov 15, 2013, at 8:27 AM, Grant Likely wrote:

> On Thu, 14 Nov 2013 10:51:05 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Hi Grant,
>> 
>> On Nov 14, 2013, at 2:44 AM, Grant Likely wrote:
>> 
>>> On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
>>>>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>>>>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>>>>>> +	} else {
>>>>>>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>>>>>>>> +				__func__, node);
>>>>>>>> +	}
>>>>>>>> +}
>>>>>>> 
>>>>>>> All of the above is potentially dangerous. There is no way to determine
>>>>>>> if anything still holds a reference to a node. The proper way to handle
>>>>>>> removal of properties is to have a release method when the last
>>>>>>> of_node_put is called.
>>>>>>> 
>>>>>> 
>>>>>> This is safe, and expected to be called only on a dynamically created tree,
>>>>>> that's what all the checks against OF_DYNAMIC guard against.
>>>>>> 
>>>>>> It is not ever meant to be called on an arbitrary tree, created by unflattening
>>>>>> a blob.
>>>>> 
>>>>> I am talking about when being used on a dynamic tree. The problem is
>>>>> when a driver or other code holds a reference to a dynamic nodes, but
>>>>> doesn't release it correctly. The memory must not be freed until all of
>>>>> the references are relased. OF_DYNAMIC doesn't actually help in that
>>>>> case, and it is the reason for of_node_get()/of_node_put()
>>>>> 
>>>> 
>>>> I know, but even that is not enough. of_node_get()/of_node_put() handles the
>>>> case of references to the nodes, but not what happens with references to
>>>> properties. deadprops is mitigating the problem somewhat, but if we're going
>>>> to go to all the trouble of kobjectification let's do the props as well.
>>>> 
>>>> of_get_property could be modified to return a devm_kmalloced copy of the real
>>>> property and that would deal with most of the callers. Of course for
>>>> the small sized scalar data we can avoid the copy.
>>>> 
>>>> By using the devm_* interface we also avoid having to mess too much with the callers.
>>>> 
>>>> I.e. what about something like devm_of_get_property()?
>>> 
>>> Reference counting is already a horrible pain to keep correct. I don't
>>> see a better way to handle it in the dynamic case, so we're stuck with
>>> it, but I don't want to make it any harder. Adding ref counting to
>>> properties will make it harder than it already is to get the code right.
>>> I'm absolutely fine with a little bit of wasted memory in the form of
>>> deadprops when the alternative is so horrible. References at the node
>>> level is enough granularity.
>>> 
>>> I don't think kduping the property is the solution either. I strongly
>>> suspect that will be far more expensive than the deadprop solution.
>>> 
>> 
>> As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property()
>> makes sense for new drivers though? What do you think? Perhaps copying to a
>> user supplied buffer as well?
> 
> I still don't think it is necessary. The device lifetime should always
> be shorter than the node lifetime.
> 
>> It's a kind of drag. That means you get handed a device_node pointer you are not
>> able to free it without having the blob along with the container/accessor of it.
>> I.e. For the normal case where the blob comes from a request_firmware() call
>> You have to keep the firmware structure around.
>> 
>> Depending on what other method you're going to use tends to make the code a little
>> bit messier. 
> 
> Understood. Stick with keeping the blob around for now. It can be
> reworkd in the future if necessary since there are no associated
> userspace ABI issues.
> 
> g.

OK, will do.

Regards

-- Pantelis

Re: [PATCH 0/5] OF: Fixes in preperation of DT overlays

[Alexander Sverdlin]

Hi!

On 05/11/13 18:50, ext Pantelis Antoniou wrote:
> This patchset introduces a number of fixes that are required
> for the subsequent patches that add DT overlays support.
> 
> Most of them are trivial, adding small bits that are missing,
> or exporting functions that were private before.
> 
> Pantelis Antoniou (5):
>   OF: Clear detach flag on attach
>   OF: Introduce device tree node flag helpers.
>   OF: export of_property_notify
>   OF: Export all DT proc update functions
>   OF: Introduce utility helper functions

All patches there we use as they are since July on our MIPS64 platforms. So, for all 5 patches:
Tested-by: Alexander Sverdlin <alexander.sverdlin@nsn.com>
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nsn.com>

>  drivers/of/Makefile |   2 +-
>  drivers/of/base.c   |  77 ++++++++--------
>  drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h  | 119 ++++++++++++++++++++++++
>  4 files changed, 413 insertions(+), 38 deletions(-)
>  create mode 100644 drivers/of/util.c
> 

-- 
Best regards,
Alexander Sverdlin.