[PATCH/RFC 0/3] of/overlay: Update aliases when added or removed

[PATCH/RFC 0/3] of/overlay: Update aliases when added or removed

[Geert Uytterhoeven]

	Hi,

Currently the list of aliases is not updated when a DT overlay that adds
an alias is loaded or unloaded. This break drivers (e.g. serial) that
rely on of_alias_get_id(). This RFC patch series fixes that.

This is definitely not a final solution to be applied, as (1) it doesn't
fix all possible cases, and as (2) there's an unresolved issue w.r.t.
object lifetime. More about this in the last patch.

But it's Good Enough For My Use Case(TM), which is enabling/disabling
serial ports on expansion headers by (un)loading DTBOs.

Thanks for your comments!

Geert Uytterhoeven (3):
  [RFC] of: Extract of_alias_create()
  [RFC] of: Add of_alias_destroy()
  [RFC] of/dynamic: Update list of aliases on aliases changes

 drivers/of/base.c    | 72 +++++++++++++++++++++++++++++++++-------------------
 drivers/of/dynamic.c | 24 ++++++++++++++++++
 include/linux/of.h   |  3 +++
 3 files changed, 73 insertions(+), 26 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH/RFC 1/3] of: Extract of_alias_create()

[Geert Uytterhoeven]

Extract the code to create and add an alias from of_alias_scan() into
its own function of_alias_create().

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 drivers/of/base.c  | 58 ++++++++++++++++++++++++++++++------------------------
 include/linux/of.h |  2 ++
 2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d48ff7391fa77d86..390f9e2b7b65d54b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1891,6 +1891,37 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
 		 ap->alias, ap->stem, ap->id, of_node_full_name(np));
 }
 
+void of_alias_create(struct property *pp,
+		     void * (*dt_alloc)(u64 size, u64 align))
+{
+	const char *start = pp->name;
+	const char *end = start + strlen(start);
+	struct device_node *np;
+	struct alias_prop *ap;
+	int id, len;
+
+	np = of_find_node_by_path(pp->value);
+	if (!np)
+		return;
+
+	/* walk the alias backwards to extract the id and work out
+	 * the 'stem' string */
+	while (isdigit(*(end-1)) && end > start)
+		end--;
+	len = end - start;
+
+	if (kstrtoint(end, 10, &id) < 0)
+		return;
+
+	/* Allocate an alias_prop with enough space for the stem */
+	ap = dt_alloc(sizeof(*ap) + len + 1, 4);
+	if (!ap)
+		return;
+	memset(ap, 0, sizeof(*ap) + len + 1);
+	ap->alias = start;
+	of_alias_add(ap, np, id, start, len);
+}
+
 /**
  * of_alias_scan - Scan all properties of the 'aliases' node
  *
@@ -1925,38 +1956,13 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		return;
 
 	for_each_property_of_node(of_aliases, pp) {
-		const char *start = pp->name;
-		const char *end = start + strlen(start);
-		struct device_node *np;
-		struct alias_prop *ap;
-		int id, len;
-
 		/* Skip those we do not want to proceed */
 		if (!strcmp(pp->name, "name") ||
 		    !strcmp(pp->name, "phandle") ||
 		    !strcmp(pp->name, "linux,phandle"))
 			continue;
 
-		np = of_find_node_by_path(pp->value);
-		if (!np)
-			continue;
-
-		/* walk the alias backwards to extract the id and work out
-		 * the 'stem' string */
-		while (isdigit(*(end-1)) && end > start)
-			end--;
-		len = end - start;
-
-		if (kstrtoint(end, 10, &id) < 0)
-			continue;
-
-		/* Allocate an alias_prop with enough space for the stem */
-		ap = dt_alloc(sizeof(*ap) + len + 1, 4);
-		if (!ap)
-			continue;
-		memset(ap, 0, sizeof(*ap) + len + 1);
-		ap->alias = start;
-		of_alias_add(ap, np, id, start, len);
+		of_alias_create(pp, dt_alloc);
 	}
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index c7715d0b56344fe7..0852625e4cfb3dfe 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -333,6 +333,8 @@ 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);
 
+extern void of_alias_create(struct property *pp,
+			    void * (*dt_alloc)(u64 size, u64 align));
 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);
 extern int of_alias_get_highest_id(const char *stem);
-- 
1.9.1

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

[PATCH/RFC 2/3] of: Add of_alias_destroy()

[Geert Uytterhoeven]

Add a helper function to find an alias, remove it from the list of
aliases, and destroy it.

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 drivers/of/base.c  | 14 ++++++++++++++
 include/linux/of.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 390f9e2b7b65d54b..6aafee0cfb27bd5c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1922,6 +1922,20 @@ void of_alias_create(struct property *pp,
 	of_alias_add(ap, np, id, start, len);
 }
 
+void of_alias_destroy(const char *name, void (*dt_free)(void *))
+{
+	struct alias_prop *ap;
+
+	list_for_each_entry(ap, &aliases_lookup, link) {
+		if (strcmp(ap->alias, name))
+			continue;
+
+		list_del(&ap->link);
+		dt_free(ap);
+		return;
+	}
+}
+
 /**
  * of_alias_scan - Scan all properties of the 'aliases' node
  *
diff --git a/include/linux/of.h b/include/linux/of.h
index 0852625e4cfb3dfe..1807a95a00a84a99 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -335,6 +335,7 @@ extern int of_count_phandle_with_args(const struct device_node *np,
 
 extern void of_alias_create(struct property *pp,
 			    void * (*dt_alloc)(u64 size, u64 align));
+extern void of_alias_destroy(const char *name, void (*dt_free)(void *));
 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);
 extern int of_alias_get_highest_id(const char *stem);
-- 
1.9.1

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

[PATCH/RFC 3/3] of/dynamic: Update list of aliases on aliases changes

[Geert Uytterhoeven]

Currently the list of aliases is not updated when an overlay that
modifies /aliases is added or removed. This breaks drivers (e.g. serial)
that rely on of_alias_get_id().

Update the list of aliases when a property of the /aliases node is
added, removed, or updated.

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
  - Should the OF core handle this itself, by registering a notifier
    using of_reconfig_notifier_register()?
  - Adding or destroying the /aliases node itself should be handled,
    too.
  - Is it safe to deallocate struct alias_prop using kfree()? It may
    have been allocated using early_init_dt_alloc_memory_arch() /
    memblock_alloc(). What's the alternative? Leaking memory?
---
 drivers/of/dynamic.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1901f8870591fe30..65dfb26f879c3a9a 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -502,6 +502,16 @@ static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 	}
 }
 
+static void *alias_alloc(u64 size, u64 align)
+{
+	return kzalloc(size, GFP_KERNEL);
+}
+
+static void alias_free(void *p)
+{
+	kfree(p);
+}
+
 static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool revert)
 {
 	struct of_reconfig_data rd;
@@ -513,6 +523,20 @@ static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool reve
 		ce = &ce_inverted;
 	}
 
+	if (ce->np == of_aliases) {
+		switch (ce->action) {
+		case OF_RECONFIG_ADD_PROPERTY:
+			of_alias_create(ce->prop, alias_alloc);
+			break;
+		case OF_RECONFIG_REMOVE_PROPERTY:
+			of_alias_destroy(ce->prop->name, alias_free);
+			break;
+		case OF_RECONFIG_UPDATE_PROPERTY:
+			of_alias_destroy(ce->old_prop->name, alias_free);
+			of_alias_create(ce->prop, alias_alloc);
+			break;
+		}
+	}
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
 	case OF_RECONFIG_DETACH_NODE:
-- 
1.9.1

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

Re: [PATCH/RFC 3/3] of/dynamic: Update list of aliases on aliases changes

[Grant Likely]

On Tue, 30 Jun 2015 16:51:16 +0200
, Geert Uytterhoeven <geert+renesas@glider.be>
 wrote:
> Currently the list of aliases is not updated when an overlay that
> modifies /aliases is added or removed. This breaks drivers (e.g. serial)
> that rely on of_alias_get_id().
> 
> Update the list of aliases when a property of the /aliases node is
> added, removed, or updated.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>   - Should the OF core handle this itself, by registering a notifier
>     using of_reconfig_notifier_register()?

Yes. Let's not add new hooks.

>   - Adding or destroying the /aliases node itself should be handled,
>     too.

Yes

>   - Is it safe to deallocate struct alias_prop using kfree()? It may
>     have been allocated using early_init_dt_alloc_memory_arch() /
>     memblock_alloc(). What's the alternative? Leaking memory?

Properties are not refcounted, so yes we leak memory. The memory remains
owned by the aliases node, but because the aliases node is never freed,
neither are any of the properties. Solving this isn't easy because it
would require adding refcounting *everywhere* that properties are
accessed. I think we have to just live with it until someone clever can
some up with a solution.

g.

> ---
>  drivers/of/dynamic.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 1901f8870591fe30..65dfb26f879c3a9a 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -502,6 +502,16 @@ static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
>  	}
>  }
>  
> +static void *alias_alloc(u64 size, u64 align)
> +{
> +	return kzalloc(size, GFP_KERNEL);
> +}
> +
> +static void alias_free(void *p)
> +{
> +	kfree(p);
> +}
> +
>  static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool revert)
>  {
>  	struct of_reconfig_data rd;
> @@ -513,6 +523,20 @@ static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool reve
>  		ce = &ce_inverted;
>  	}
>  
> +	if (ce->np == of_aliases) {
> +		switch (ce->action) {
> +		case OF_RECONFIG_ADD_PROPERTY:
> +			of_alias_create(ce->prop, alias_alloc);
> +			break;
> +		case OF_RECONFIG_REMOVE_PROPERTY:
> +			of_alias_destroy(ce->prop->name, alias_free);
> +			break;
> +		case OF_RECONFIG_UPDATE_PROPERTY:
> +			of_alias_destroy(ce->old_prop->name, alias_free);
> +			of_alias_create(ce->prop, alias_alloc);
> +			break;
> +		}
> +	}
>  	switch (ce->action) {
>  	case OF_RECONFIG_ATTACH_NODE:
>  	case OF_RECONFIG_DETACH_NODE:
> -- 
> 1.9.1
> 

Re: [PATCH/RFC 3/3] of/dynamic: Update list of aliases on aliases changes

[Geert Uytterhoeven]

Hi Grant,

On Tue, Jun 30, 2015 at 7:21 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, 30 Jun 2015 16:51:16 +0200
> , Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>  wrote:
>> Currently the list of aliases is not updated when an overlay that
>> modifies /aliases is added or removed. This breaks drivers (e.g. serial)
>> that rely on of_alias_get_id().
>>
>> Update the list of aliases when a property of the /aliases node is
>> added, removed, or updated.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>

>>   - Is it safe to deallocate struct alias_prop using kfree()? It may
>>     have been allocated using early_init_dt_alloc_memory_arch() /
>>     memblock_alloc(). What's the alternative? Leaking memory?
>
> Properties are not refcounted, so yes we leak memory. The memory remains
> owned by the aliases node, but because the aliases node is never freed,
> neither are any of the properties. Solving this isn't easy because it
> would require adding refcounting *everywhere* that properties are
> accessed. I think we have to just live with it until someone clever can
> some up with a solution.

Please note that struct alias_prop is not a property, but a list_head.
Hence it's not added to the deadprops of a node, and it isn't owned by
anyone after removal from the aliases_lookup list.
I can create a new dead_aliases list for that, if that's what needed...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/3] of/overlay: Update aliases when added or removed

[Grant Likely]

On Tue, 30 Jun 2015 16:51:13 +0200
, Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
 wrote:
> 	Hi,
> 
> Currently the list of aliases is not updated when a DT overlay that adds
> an alias is loaded or unloaded. This break drivers (e.g. serial) that
> rely on of_alias_get_id(). This RFC patch series fixes that.
> 
> This is definitely not a final solution to be applied, as (1) it doesn't
> fix all possible cases, and as (2) there's an unresolved issue w.r.t.
> object lifetime. More about this in the last patch.
> 
> But it's Good Enough For My Use Case(TM), which is enabling/disabling
> serial ports on expansion headers by (un)loading DTBOs.
> 
> Thanks for your comments!
> 
> Geert Uytterhoeven (3):
>   [RFC] of: Extract of_alias_create()
>   [RFC] of: Add of_alias_destroy()
>   [RFC] of/dynamic: Update list of aliases on aliases changes

Looks good to me. I've made comments on patch 3. Also, you'll need to
include unittests before I can merge it.

g.

> 
>  drivers/of/base.c    | 72 +++++++++++++++++++++++++++++++++-------------------
>  drivers/of/dynamic.c | 24 ++++++++++++++++++
>  include/linux/of.h   |  3 +++
>  3 files changed, 73 insertions(+), 26 deletions(-)
> 
> -- 
> 1.9.1
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> 							    -- Linus Torvalds

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

Re: [PATCH/RFC 0/3] of/overlay: Update aliases when added or removed

[Robert Abel]

Hi Geert, Hi Grant,

On 30 Jun 2015 Grant Likely wroke
> Looks good to me. I've made comments on patch 3. Also, you'll need to include unittests before I can merge it.
> [...]
>>   - Should the OF core handle this itself, by registering a notifier
>>     using of_reconfig_notifier_register()?
>Yes. Let's not add new hooks.

Any news on this? Were the patches applied to any public tree or are
there follow-up patches that address the issue using
of_reconfig_notifier_register?

I'm off-list, please CC me.

Regards,

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

Re: [PATCH/RFC 0/3] of/overlay: Update aliases when added or removed

[Geert Uytterhoeven]

Hi Robert,

On Wed, Jul 29, 2015 at 1:52 PM, Robert Abel
<rabel-Ejy783gw450hGw5VS8l+XCM2BslAju9D@public.gmane.org> wrote:
> On 30 Jun 2015 Grant Likely wroke
>> Looks good to me. I've made comments on patch 3. Also, you'll need to include unittests before I can merge it.
>> [...]
>>>   - Should the OF core handle this itself, by registering a notifier
>>>     using of_reconfig_notifier_register()?
>>Yes. Let's not add new hooks.
>
> Any news on this? Were the patches applied to any public tree or are
> there follow-up patches that address the issue using
> of_reconfig_notifier_register?

In the mean time I made some improvements (e.g. but removed aliases
on a list instead of freeing them, which causes crashes when using the
wrong deallocator), but I haven't implemented the notifier yet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html