From: Frank Rowand <[email protected]>
Add checks to (1) overlay apply process and (2) memory freeing
triggered by overlay release. The checks are intended to detect
possible memory leaks and invalid overlays.
The checks revealed bugs in existing code. Fixed the bugs.
While fixing bugs, noted other issues, which are fixed in
separate patches.
***** Powerpc folks: I was not able to test the patches that
***** directly impact Powerpc systems that use dynamic
***** devicetree. Please review that code carefully and
***** test. The specific patches are: 03/16, 04/16, 07/16
FPGA folks:
I made the validation checks that should result in an
invalid live devicetree report "ERROR" and cause the overlay apply
to fail.
I made the memory leak validation tests report "WARNING" and allow
the overlay apply to complete successfully. Please let me know
if you encounter the warnings. There are at least two paths
forward to deal with the cases that trigger the warning: (1) change
the warning to an error and fail the overlay apply, or (2) find a
way to detect the potential memory leaks and free the memory
appropriately.
ALL people:
The validations do _not_ address another major concern I have with
releasing overlays, which is use after free errors.
Frank Rowand (16):
of: overlay: add tests to validate kfrees from overlay removal
of: overlay: add missing of_node_put() after add new node to changeset
of: overlay: add missing of_node_get() in __of_attach_node_sysfs
powerpc/pseries: add of_node_put() in dlpar_detach_node()
of: overlay: use prop add changeset entry for property in new nodes
of: overlay: do not duplicate properties from overlay for new nodes
of: dynamic: change type of of_{at,de}tach_node() to void
of: overlay: reorder fields in struct fragment
of: overlay: validate overlay properties #address-cells and
#size-cells
of: overlay: make all pr_debug() and pr_err() messages unique
of: overlay: test case of two fragments adding same node
of: overlay: check prevents multiple fragments add or delete same node
of: overlay: check prevents multiple fragments touching same property
of: unittest: remove unused of_unittest_apply_overlay() argument
of: unittest: initialize args before calling of_irq_parse_one()
of: unittest: find overlays[] entry by name instead of index
arch/powerpc/platforms/pseries/dlpar.c | 15 +-
arch/powerpc/platforms/pseries/reconfig.c | 6 +-
drivers/of/dynamic.c | 41 +++-
drivers/of/kobj.c | 4 +-
drivers/of/overlay.c | 271 ++++++++++++++++-----
drivers/of/unittest-data/Makefile | 2 +
.../of/unittest-data/overlay_bad_add_dup_node.dts | 28 +++
.../of/unittest-data/overlay_bad_add_dup_prop.dts | 24 ++
drivers/of/unittest-data/overlay_base.dts | 1 +
drivers/of/unittest.c | 43 +++-
include/linux/of.h | 19 +-
11 files changed, 353 insertions(+), 101 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_node.dts
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
The refcount of a newly added overlay node decrements to one
(instead of zero) when the overlay changeset is destroyed. This
change will cause the final decrement be to zero.
After applying this patch, new validation warnings will be
reported from the devicetree unittest during boot due to
a pre-existing devicetree bug. The warnings will be similar to:
OF: ERROR: memory leak of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest4 before free overlay changeset
This pre-existing devicetree bug will also trigger a WARN_ONCE() from
refcount_sub_and_test_checked() when an overlay changeset is
destroyed without having first been applied. This scenario occurs
when an error in the overlay is detected during the overlay changeset
creation:
WARNING: CPU: 0 PID: 1 at lib/refcount.c:187 refcount_sub_and_test_checked+0xa8/0xbc
refcount_t: underflow; use-after-free.
(unwind_backtrace) from (show_stack+0x10/0x14)
(show_stack) from (dump_stack+0x6c/0x8c)
(dump_stack) from (__warn+0xdc/0x104)
(__warn) from (warn_slowpath_fmt+0x44/0x6c)
(warn_slowpath_fmt) from (refcount_sub_and_test_checked+0xa8/0xbc)
(refcount_sub_and_test_checked) from (kobject_put+0x24/0x208)
(kobject_put) from (of_changeset_destroy+0x2c/0xb4)
(of_changeset_destroy) from (free_overlay_changeset+0x1c/0x9c)
(free_overlay_changeset) from (of_overlay_remove+0x284/0x2cc)
(of_overlay_remove) from (of_unittest_apply_revert_overlay_check.constprop.4+0xf8/0x1e8)
(of_unittest_apply_revert_overlay_check.constprop.4) from (of_unittest_overlay+0x960/0xed8)
(of_unittest_overlay) from (of_unittest+0x1cc4/0x2138)
(of_unittest) from (do_one_initcall+0x4c/0x28c)
(do_one_initcall) from (kernel_init_freeable+0x29c/0x378)
(kernel_init_freeable) from (kernel_init+0x8/0x110)
(kernel_init) from (ret_from_fork+0x14/0x2c)
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 1176cb4b6e4e..32cfee68f2e3 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -379,7 +379,9 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
if (ret)
return ret;
- return build_changeset_next_level(ovcs, tchild, node);
+ ret = build_changeset_next_level(ovcs, tchild, node);
+ of_node_put(tchild);
+ return ret;
}
if (node->phandle && tchild->phandle)
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Argument unittest_nr is not used in of_unittest_apply_overlay(),
remove it.
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/unittest.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index efd9c947f192..6d80f474c8f2 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1419,8 +1419,7 @@ static void of_unittest_destroy_tracked_overlays(void)
} while (defers > 0);
}
-static int __init of_unittest_apply_overlay(int overlay_nr, int unittest_nr,
- int *overlay_id)
+static int __init of_unittest_apply_overlay(int overlay_nr, int *overlay_id)
{
const char *overlay_name;
@@ -1453,7 +1452,7 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
}
ovcs_id = 0;
- ret = of_unittest_apply_overlay(overlay_nr, unittest_nr, &ovcs_id);
+ ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
if (ret != 0) {
/* of_unittest_apply_overlay already called unittest() */
return ret;
@@ -1489,7 +1488,7 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
/* apply the overlay */
ovcs_id = 0;
- ret = of_unittest_apply_overlay(overlay_nr, unittest_nr, &ovcs_id);
+ ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
if (ret != 0) {
/* of_unittest_apply_overlay already called unittest() */
return ret;
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Add test case of two fragments updating the same property. After
adding the test case, the system hangs at end of boot, after
after slub stack dumps from kfree() in crypto modprobe code.
Multiple overlay fragments adding, modifying, or deleting the same
property is not supported. Add check to detect the attempt and fail
the overlay apply.
After applying this patch, the devicetree unittest messages will
include:
OF: overlay: ERROR: multiple overlay fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/rpm_avail
...
### dt-test ### end of unittest - 212 passed, 0 failed
The check to detect two fragments updating the same property is
folded into the patch that created the test case to maintain
bisectability.
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 118 ++++++++++++++-------
drivers/of/unittest-data/Makefile | 1 +
.../of/unittest-data/overlay_bad_add_dup_prop.dts | 24 +++++
drivers/of/unittest-data/overlay_base.dts | 1 +
drivers/of/unittest.c | 5 +
5 files changed, 112 insertions(+), 37 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 5376ae166caf..640435534675 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -514,52 +514,96 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
return 0;
}
+static int find_dup_cset_node_entry(struct overlay_changeset *ovcs,
+ struct of_changeset_entry *ce_1)
+{
+ struct of_changeset_entry *ce_2;
+ char *fn_1, *fn_2;
+ int node_path_match;
+
+ if (ce_1->action != OF_RECONFIG_ATTACH_NODE &&
+ ce_1->action != OF_RECONFIG_DETACH_NODE)
+ return 0;
+
+ ce_2 = ce_1;
+ list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
+ if ((ce_2->action == OF_RECONFIG_ATTACH_NODE ||
+ ce_2->action == OF_RECONFIG_DETACH_NODE) &&
+ !of_node_cmp(ce_1->np->full_name, ce_2->np->full_name)) {
+
+ fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
+ fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
+ node_path_match = !strcmp(fn_1, fn_2);
+ kfree(fn_1);
+ kfree(fn_2);
+ if (node_path_match) {
+ pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
+ ce_1->np);
+ return -EINVAL;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int find_dup_cset_prop(struct overlay_changeset *ovcs,
+ struct of_changeset_entry *ce_1)
+{
+ struct of_changeset_entry *ce_2;
+ char *fn_1, *fn_2;
+ int node_path_match;
+
+ if (ce_1->action != OF_RECONFIG_ADD_PROPERTY &&
+ ce_1->action != OF_RECONFIG_REMOVE_PROPERTY &&
+ ce_1->action != OF_RECONFIG_UPDATE_PROPERTY)
+ return 0;
+
+ ce_2 = ce_1;
+ list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
+ if ((ce_2->action == OF_RECONFIG_ADD_PROPERTY ||
+ ce_2->action == OF_RECONFIG_REMOVE_PROPERTY ||
+ ce_2->action == OF_RECONFIG_UPDATE_PROPERTY) &&
+ !of_node_cmp(ce_1->np->full_name, ce_2->np->full_name)) {
+
+ fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
+ fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
+ node_path_match = !strcmp(fn_1, fn_2);
+ kfree(fn_1);
+ kfree(fn_2);
+ if (node_path_match &&
+ !of_prop_cmp(ce_1->prop->name, ce_2->prop->name)) {
+ pr_err("ERROR: multiple overlay fragments add, update, and/or delete property %pOF/%s\n",
+ ce_1->np, ce_1->prop->name);
+ return -EINVAL;
+ }
+ }
+ }
+
+ return 0;
+}
+
/**
- * check_changeset_dup_add_node() - changeset validation: duplicate add node
+ * changeset_dup_entry_check() - check for duplicate entries
* @ovcs: Overlay changeset
*
- * Check changeset @ovcs->cset for multiple add node entries for the same
- * node.
+ * Check changeset @ovcs->cset for multiple {add or delete} node entries for
+ * the same node or duplicate {add, delete, or update} properties entries
+ * for the same property.
*
- * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
- * invalid overlay in @ovcs->fragments[].
+ * Returns 0 on success, or -EINVAL if duplicate changeset entry found.
*/
-static int check_changeset_dup_add_node(struct overlay_changeset *ovcs)
+static int changeset_dup_entry_check(struct overlay_changeset *ovcs)
{
- struct of_changeset_entry *ce_1, *ce_2;
- char *fn_1, *fn_2;
- int name_match;
+ struct of_changeset_entry *ce_1;
+ int dup_entry = 0;
list_for_each_entry(ce_1, &ovcs->cset.entries, node) {
-
- if (ce_1->action == OF_RECONFIG_ATTACH_NODE ||
- ce_1->action == OF_RECONFIG_DETACH_NODE) {
-
- ce_2 = ce_1;
- list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
- if (ce_2->action == OF_RECONFIG_ATTACH_NODE ||
- ce_2->action == OF_RECONFIG_DETACH_NODE) {
- /* inexpensive name compare */
- if (!of_node_cmp(ce_1->np->full_name,
- ce_2->np->full_name)) {
- /* expensive full path name compare */
- fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
- fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
- name_match = !strcmp(fn_1, fn_2);
- kfree(fn_1);
- kfree(fn_2);
- if (name_match) {
- pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
- ce_1->np);
- return -EINVAL;
- }
- }
- }
- }
- }
+ dup_entry |= find_dup_cset_node_entry(ovcs, ce_1);
+ dup_entry |= find_dup_cset_prop(ovcs, ce_1);
}
- return 0;
+ return dup_entry ? -EINVAL : 0;
}
/**
@@ -617,7 +661,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
}
}
- return check_changeset_dup_add_node(ovcs);
+ return changeset_dup_entry_check(ovcs);
}
/*
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 166dbdbfd1c5..9b6807065827 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \
overlay_13.dtb.o \
overlay_15.dtb.o \
overlay_bad_add_dup_node.dtb.o \
+ overlay_bad_add_dup_prop.dtb.o \
overlay_bad_phandle.dtb.o \
overlay_bad_symbol.dtb.o \
overlay_base.dtb.o
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
new file mode 100644
index 000000000000..c190da54f175
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/*
+ * &electric_1/motor-1 and &spin_ctrl_1 are the same node:
+ * /testcase-data-2/substation@100/motor-1
+ *
+ * Thus the property "rpm_avail" in each fragment will
+ * result in an attempt to update the same property twice.
+ * This will result in an error and the overlay apply
+ * will fail.
+ */
+
+&electric_1 {
+
+ motor-1 {
+ rpm_avail = < 100 >;
+ };
+};
+
+&spin_ctrl_1 {
+ rpm_avail = < 100 200 >;
+};
diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
index 820b79ca378a..99ab9d12d00b 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -30,6 +30,7 @@
spin_ctrl_1: motor-1 {
compatible = "ot,ferris-wheel-motor";
spin = "clockwise";
+ rpm_avail = < 50 >;
};
spin_ctrl_2: motor-8 {
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 471b8eb6e842..efd9c947f192 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2148,6 +2148,7 @@ struct overlay_info {
OVERLAY_INFO_EXTERN(overlay_13);
OVERLAY_INFO_EXTERN(overlay_15);
OVERLAY_INFO_EXTERN(overlay_bad_add_dup_node);
+OVERLAY_INFO_EXTERN(overlay_bad_add_dup_prop);
OVERLAY_INFO_EXTERN(overlay_bad_phandle);
OVERLAY_INFO_EXTERN(overlay_bad_symbol);
@@ -2171,6 +2172,7 @@ struct overlay_info {
OVERLAY_INFO(overlay_13, 0),
OVERLAY_INFO(overlay_15, 0),
OVERLAY_INFO(overlay_bad_add_dup_node, -EINVAL),
+ OVERLAY_INFO(overlay_bad_add_dup_prop, -EINVAL),
OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
OVERLAY_INFO(overlay_bad_symbol, -EINVAL),
{}
@@ -2418,6 +2420,9 @@ static __init void of_unittest_overlay_high_level(void)
unittest(overlay_data_apply("overlay_bad_add_dup_node", NULL),
"Adding overlay 'overlay_bad_add_dup_node' failed\n");
+ unittest(overlay_data_apply("overlay_bad_add_dup_prop", NULL),
+ "Adding overlay 'overlay_bad_add_dup_prop' failed\n");
+
unittest(overlay_data_apply("overlay_bad_phandle", NULL),
"Adding overlay 'overlay_bad_phandle' failed\n");
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Multiple overlay fragments adding or deleting the same node is not
supported. Replace code comment of such, with check to detect the
attempt and fail the overlay apply.
Devicetree unittest where multiple fragments added the same node was
added in the previous patch in the series. After applying this patch
the unittest messages will no longer include:
Duplicate name in motor-1, renamed to "controller#1"
OF: overlay: of_overlay_apply() err=0
### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node
### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed
...
### dt-test ### end of unittest - 210 passed, 1 failed
but will instead include:
OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller
...
### dt-test ### end of unittest - 211 passed, 0 failed
Signed-off-by: Frank Rowand <[email protected]>
---
checkpatch errors "line over 80 characters" are ok, they will be
fixed two patches later in this series
drivers/of/overlay.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 49 insertions(+), 9 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f89383331b88..5376ae166caf 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -398,14 +398,6 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
* a live devicetree created from Open Firmware.
*
* NOTE_2: Multiple mods of created nodes not supported.
- * If more than one fragment contains a node that does not already exist
- * in the live tree, then for each fragment of_changeset_attach_node()
- * will add a changeset entry to add the node. When the changeset is
- * applied, __of_attach_node() will attach the node twice (once for
- * each fragment). At this point the device tree will be corrupted.
- *
- * TODO: add integrity check to ensure that multiple fragments do not
- * create the same node.
*
* Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
* invalid @overlay.
@@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
}
/**
+ * check_changeset_dup_add_node() - changeset validation: duplicate add node
+ * @ovcs: Overlay changeset
+ *
+ * Check changeset @ovcs->cset for multiple add node entries for the same
+ * node.
+ *
+ * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
+ * invalid overlay in @ovcs->fragments[].
+ */
+static int check_changeset_dup_add_node(struct overlay_changeset *ovcs)
+{
+ struct of_changeset_entry *ce_1, *ce_2;
+ char *fn_1, *fn_2;
+ int name_match;
+
+ list_for_each_entry(ce_1, &ovcs->cset.entries, node) {
+
+ if (ce_1->action == OF_RECONFIG_ATTACH_NODE ||
+ ce_1->action == OF_RECONFIG_DETACH_NODE) {
+
+ ce_2 = ce_1;
+ list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
+ if (ce_2->action == OF_RECONFIG_ATTACH_NODE ||
+ ce_2->action == OF_RECONFIG_DETACH_NODE) {
+ /* inexpensive name compare */
+ if (!of_node_cmp(ce_1->np->full_name,
+ ce_2->np->full_name)) {
+ /* expensive full path name compare */
+ fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
+ fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
+ name_match = !strcmp(fn_1, fn_2);
+ kfree(fn_1);
+ kfree(fn_2);
+ if (name_match) {
+ pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
+ ce_1->np);
+ return -EINVAL;
+ }
+ }
+ }
+ }
+ }
+ }
+
+ return 0;
+}
+
+/**
* build_changeset() - populate overlay changeset in @ovcs from @ovcs->fragments
* @ovcs: Overlay changeset
*
@@ -577,7 +617,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
}
}
- return 0;
+ return check_changeset_dup_add_node(ovcs);
}
/*
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Make overlay.c debug and error messages unique so that they can be
unambiguously found by grep.
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index e6fb3ffe9d93..f89383331b88 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -513,7 +513,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
for_each_property_of_node(overlay_symbols_node, prop) {
ret = add_changeset_property(ovcs, target, prop, 1);
if (ret) {
- pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
+ pr_debug("Failed to apply symbols prop @%pOF/%s, err=%d\n",
target->np, prop->name, ret);
return ret;
}
@@ -557,7 +557,8 @@ static int build_changeset(struct overlay_changeset *ovcs)
ret = build_changeset_next_level(ovcs, &target,
fragment->overlay);
if (ret) {
- pr_debug("apply failed '%pOF'\n", fragment->target);
+ pr_debug("fragment apply failed '%pOF'\n",
+ fragment->target);
return ret;
}
}
@@ -570,7 +571,8 @@ static int build_changeset(struct overlay_changeset *ovcs)
ret = build_changeset_symbols_node(ovcs, &target,
fragment->overlay);
if (ret) {
- pr_debug("apply failed '%pOF'\n", fragment->target);
+ pr_debug("symbols fragment apply failed '%pOF'\n",
+ fragment->target);
return ret;
}
}
@@ -879,7 +881,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
ret = __of_changeset_apply_notify(&ovcs->cset);
if (ret)
- pr_err("overlay changeset entry notify error %d\n", ret);
+ pr_err("overlay apply changeset entry notify error %d\n", ret);
/* notify failure is not fatal, continue */
list_add_tail(&ovcs->ovcs_list, &ovcs_list);
@@ -1138,7 +1140,7 @@ int of_overlay_remove(int *ovcs_id)
ret = __of_changeset_revert_notify(&ovcs->cset);
if (ret)
- pr_err("overlay changeset entry notify error %d\n", ret);
+ pr_err("overlay remove changeset entry notify error %d\n", ret);
/* notify failure is not fatal, continue */
*ovcs_id = 0;
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Order the fields of struct fragment in the same order as
struct of_overlay_notify_data. The order in struct fragment is
not significant. If both structs are ordered the same then when
examining the data in a debugger or dump the human involved does
not have to remember which context they are examining.
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c113186e222c..29c33a5c533f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -49,8 +49,8 @@ struct target {
* @overlay: pointer to the __overlay__ node
*/
struct fragment {
- struct device_node *target;
struct device_node *overlay;
+ struct device_node *target;
};
/**
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Multiple overlay fragments adding or deleting the same node is not
supported. An attempt to do so results in an incorrect devicetree.
The node name will be munged for the second add.
After adding this patch, the unittest messages will show:
Duplicate name in motor-1, renamed to "controller#1"
OF: overlay: of_overlay_apply() err=0
### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node
### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed
...
### dt-test ### end of unittest - 210 passed, 1 failed
The incorrect (munged) node name "controller#1" can be seen in the
/proc filesystem:
$ pwd
/proc/device-tree/testcase-data-2/substation@100/motor-1
$ ls
compatible controller controller#1 name phandle spin
$ ls controller
power_bus
$ ls controller#1
power_bus_emergency
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/unittest-data/Makefile | 1 +
.../of/unittest-data/overlay_bad_add_dup_node.dts | 28 ++++++++++++++++++++++
drivers/of/unittest.c | 5 ++++
3 files changed, 34 insertions(+)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_node.dts
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 013d85e694c6..166dbdbfd1c5 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \
overlay_12.dtb.o \
overlay_13.dtb.o \
overlay_15.dtb.o \
+ overlay_bad_add_dup_node.dtb.o \
overlay_bad_phandle.dtb.o \
overlay_bad_symbol.dtb.o \
overlay_base.dtb.o
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_node.dts b/drivers/of/unittest-data/overlay_bad_add_dup_node.dts
new file mode 100644
index 000000000000..145dfc3b1024
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_node.dts
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/*
+ * &electric_1/motor-1 and &spin_ctrl_1 are the same node:
+ * /testcase-data-2/substation@100/motor-1
+ *
+ * Thus the new node "controller" in each fragment will
+ * result in an attempt to add the same node twice.
+ * This will result in an error and the overlay apply
+ * will fail.
+ */
+
+&electric_1 {
+
+ motor-1 {
+ controller {
+ power_bus = < 0x1 0x2 >;
+ };
+ };
+};
+
+&spin_ctrl_1 {
+ controller {
+ power_bus_emergency = < 0x101 0x102 >;
+ };
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 722537e14848..471b8eb6e842 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2147,6 +2147,7 @@ struct overlay_info {
OVERLAY_INFO_EXTERN(overlay_12);
OVERLAY_INFO_EXTERN(overlay_13);
OVERLAY_INFO_EXTERN(overlay_15);
+OVERLAY_INFO_EXTERN(overlay_bad_add_dup_node);
OVERLAY_INFO_EXTERN(overlay_bad_phandle);
OVERLAY_INFO_EXTERN(overlay_bad_symbol);
@@ -2169,6 +2170,7 @@ struct overlay_info {
OVERLAY_INFO(overlay_12, 0),
OVERLAY_INFO(overlay_13, 0),
OVERLAY_INFO(overlay_15, 0),
+ OVERLAY_INFO(overlay_bad_add_dup_node, -EINVAL),
OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
OVERLAY_INFO(overlay_bad_symbol, -EINVAL),
{}
@@ -2413,6 +2415,9 @@ static __init void of_unittest_overlay_high_level(void)
unittest(overlay_data_apply("overlay", NULL),
"Adding overlay 'overlay' failed\n");
+ unittest(overlay_data_apply("overlay_bad_add_dup_node", NULL),
+ "Adding overlay 'overlay_bad_add_dup_node' failed\n");
+
unittest(overlay_data_apply("overlay_bad_phandle", NULL),
"Adding overlay 'overlay_bad_phandle' failed\n");
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
If overlay properties #address-cells or #size-cells are already in
the live devicetree for any given node, then the values in the
overlay must match the values in the live tree.
If the properties are already in the live tree then there is no
need to create a changeset entry to add them since they must
have the same value. This reduces the memory used by the
changeset and eliminates a possible memory leak. This is
verified by 12 fewer warnings during the devicetree unittest,
as the possible memory leak warnings about #address-cells and
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 29c33a5c533f..e6fb3ffe9d93 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
* @target may be either in the live devicetree or in a new subtree that
* is contained in the changeset.
*
- * Some special properties are not updated (no error returned).
+ * Some special properties are not added or updated (no error returned):
+ * "name", "phandle", "linux,phandle".
+ *
+ * Properties "#address-cells" and "#size-cells" are not updated if they
+ * are already in the live tree, but if present in the live tree, the values
+ * in the overlay must match the values in the live tree.
*
* Update of property in symbols node is not allowed.
*
@@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
{
struct property *new_prop = NULL, *prop;
int ret = 0;
+ bool check_for_non_overlay_node = false;
if (!of_prop_cmp(overlay_prop->name, "name") ||
!of_prop_cmp(overlay_prop->name, "phandle") ||
@@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
if (!new_prop)
return -ENOMEM;
- if (!prop)
+ if (!prop) {
+
+ check_for_non_overlay_node = true;
ret = of_changeset_add_property(&ovcs->cset, target->np,
new_prop);
- else
+
+ } else if (!of_prop_cmp(prop->name, "#address-cells")) {
+
+ if (prop->length != 4 || new_prop->length != 4 ||
+ *(u32 *)prop->value != *(u32 *)new_prop->value)
+ pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
+ target->np);
+
+ } else if (!of_prop_cmp(prop->name, "#size-cells")) {
+
+ if (prop->length != 4 || new_prop->length != 4 ||
+ *(u32 *)prop->value != *(u32 *)new_prop->value)
+ pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
+ target->np);
+
+ } else {
+
+ check_for_non_overlay_node = true;
ret = of_changeset_update_property(&ovcs->cset, target->np,
new_prop);
+ }
+
+ if (check_for_non_overlay_node &&
+ !of_node_check_flag(target->np, OF_OVERLAY))
+ pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
+ __func__, target->np, new_prop->name);
+
if (ret) {
kfree(new_prop->name);
kfree(new_prop->value);
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
of_attach_node() and of_detach_node() always return zero, so
their return value is meaningless. Change their type to void
and fix all callers to ignore return value.
Signed-off-by: Frank Rowand <[email protected]>
---
Powerpc files not tested
arch/powerpc/platforms/pseries/dlpar.c | 13 ++-----------
arch/powerpc/platforms/pseries/reconfig.c | 6 +-----
drivers/of/dynamic.c | 9 ++-------
include/linux/of.h | 4 ++--
4 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index e3010b14aea5..0027eea94a8b 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -244,15 +244,9 @@ struct device_node *dlpar_configure_connector(__be32 drc_index,
int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
{
- int rc;
-
dn->parent = parent;
- rc = of_attach_node(dn);
- if (rc) {
- printk(KERN_ERR "Failed to add device node %pOF\n", dn);
- return rc;
- }
+ of_attach_node(dn);
return 0;
}
@@ -260,7 +254,6 @@ int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
int dlpar_detach_node(struct device_node *dn)
{
struct device_node *child;
- int rc;
child = of_get_next_child(dn, NULL);
while (child) {
@@ -268,9 +261,7 @@ int dlpar_detach_node(struct device_node *dn)
child = of_get_next_child(dn, child);
}
- rc = of_detach_node(dn);
- if (rc)
- return rc;
+ of_detach_node(dn);
of_node_put(dn);
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 0e0208117e77..0b72098da454 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -47,11 +47,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
goto out_err;
}
- err = of_attach_node(np);
- if (err) {
- printk(KERN_ERR "Failed to add device node %s\n", path);
- goto out_err;
- }
+ of_attach_node(np);
of_node_put(np->parent);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 275c0d7e2268..5f7c99b9de0d 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -224,7 +224,7 @@ static void __of_attach_node(struct device_node *np)
/**
* of_attach_node() - Plug a device node into the tree and global list.
*/
-int of_attach_node(struct device_node *np)
+void of_attach_node(struct device_node *np)
{
struct of_reconfig_data rd;
unsigned long flags;
@@ -241,8 +241,6 @@ int of_attach_node(struct device_node *np)
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
-
- return 0;
}
void __of_detach_node(struct device_node *np)
@@ -273,11 +271,10 @@ void __of_detach_node(struct device_node *np)
/**
* of_detach_node() - "Unplug" a node from the device tree.
*/
-int of_detach_node(struct device_node *np)
+void of_detach_node(struct device_node *np)
{
struct of_reconfig_data rd;
unsigned long flags;
- int rc = 0;
memset(&rd, 0, sizeof(rd));
rd.dn = np;
@@ -291,8 +288,6 @@ int of_detach_node(struct device_node *np)
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
-
- return rc;
}
EXPORT_SYMBOL_GPL(of_detach_node);
diff --git a/include/linux/of.h b/include/linux/of.h
index aa1dafaec6ae..72c593455019 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -406,8 +406,8 @@ extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
#define OF_RECONFIG_REMOVE_PROPERTY 0x0004
#define OF_RECONFIG_UPDATE_PROPERTY 0x0005
-extern int of_attach_node(struct device_node *);
-extern int of_detach_node(struct device_node *);
+extern void of_attach_node(struct device_node *np);
+extern void of_detach_node(struct device_node *np);
#define of_match_ptr(_ptr) (_ptr)
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Callers of of_irq_parse_one() blindly use the pointer args.np
without checking whether of_irq_parse_one() had an error and
thus did not set the value of args.np. Initialize args to
zero so that using the format "%pOF" to show the value of
args.np will show "(null)" when of_irq_parse_one() has an
error and does not set args.np instead of trying to
dereference a random value.
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/unittest.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 6d80f474c8f2..b61a33f30a56 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -780,7 +780,7 @@ static void __init of_unittest_parse_interrupts(void)
for (i = 0; i < 4; i++) {
bool passed = true;
- args.args_count = 0;
+ memset(&args, 0, sizeof(args));
rc = of_irq_parse_one(np, i, &args);
passed &= !rc;
@@ -801,7 +801,7 @@ static void __init of_unittest_parse_interrupts(void)
for (i = 0; i < 4; i++) {
bool passed = true;
- args.args_count = 0;
+ memset(&args, 0, sizeof(args));
rc = of_irq_parse_one(np, i, &args);
/* Test the values from tests-phandle.dtsi */
@@ -854,6 +854,7 @@ static void __init of_unittest_parse_interrupts_extended(void)
for (i = 0; i < 7; i++) {
bool passed = true;
+ memset(&args, 0, sizeof(args));
rc = of_irq_parse_one(np, i, &args);
/* Test the values from tests-phandle.dtsi */
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
One accessor of overlays[] was using a hard coded index value to
find the correct array entry instead of searching for the entry
containing the correct name.
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/unittest.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index b61a33f30a56..4d4ba4ddba9b 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2152,7 +2152,7 @@ struct overlay_info {
OVERLAY_INFO_EXTERN(overlay_bad_phandle);
OVERLAY_INFO_EXTERN(overlay_bad_symbol);
-/* order of entries is hard-coded into users of overlays[] */
+/* entries found by name */
static struct overlay_info overlays[] = {
OVERLAY_INFO(overlay_base, -9999),
OVERLAY_INFO(overlay, 0),
@@ -2175,7 +2175,8 @@ struct overlay_info {
OVERLAY_INFO(overlay_bad_add_dup_prop, -EINVAL),
OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
OVERLAY_INFO(overlay_bad_symbol, -EINVAL),
- {}
+ /* end marker */
+ {.dtb_begin = NULL, .dtb_end = NULL, .expected_result = 0, .name = NULL}
};
static struct device_node *overlay_base_root;
@@ -2205,6 +2206,19 @@ void __init unittest_unflatten_overlay_base(void)
u32 data_size;
void *new_fdt;
u32 size;
+ int found = 0;
+ const char *overlay_name = "overlay_base";
+
+ for (info = overlays; info && info->name; info++) {
+ if (!strcmp(overlay_name, info->name)) {
+ found = 1;
+ break;
+ }
+ }
+ if (!found) {
+ pr_err("no overlay data for %s\n", overlay_name);
+ return;
+ }
info = &overlays[0];
@@ -2252,11 +2266,10 @@ static int __init overlay_data_apply(const char *overlay_name, int *overlay_id)
{
struct overlay_info *info;
int found = 0;
- int k;
int ret;
u32 size;
- for (k = 0, info = overlays; info && info->name; info++, k++) {
+ for (info = overlays; info && info->name; info++) {
if (!strcmp(overlay_name, info->name)) {
found = 1;
break;
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
The changeset entry 'update property' was used for new properties in
an overlay instead of 'add property'.
The decision of whether to use 'update property' was based on whether
the property already exists in the subtree where the node is being
spliced into. At the top level of creating a changeset describing the
overlay, the target node is in the live devicetree, so checking whether
the property exists in the target node returns the correct result.
As soon as the changeset creation algorithm recurses into a new node,
the target is no longer in the live devicetree, but is instead in the
detached overlay tree, thus all properties are incorrectly found to
already exist in the target.
This fix will expose another devicetree bug that will be fixed
in the following patch in the series.
When this patch is applied the errors reported by the devictree
unittest will change, and the unittest results will change from:
### dt-test ### end of unittest - 210 passed, 0 failed
to
### dt-test ### end of unittest - 203 passed, 7 failed
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 112 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 74 insertions(+), 38 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 32cfee68f2e3..0b0904f44bc7 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -24,6 +24,26 @@
#include "of_private.h"
/**
+ * struct target - info about current target node as recursing through overlay
+ * @np: node where current level of overlay will be applied
+ * @in_livetree: @np is a node in the live devicetree
+ *
+ * Used in the algorithm to create the portion of a changeset that describes
+ * an overlay fragment, which is a devicetree subtree. Initially @np is a node
+ * in the live devicetree where the overlay subtree is targeted to be grafted
+ * into. When recursing to the next level of the overlay subtree, the target
+ * also recurses to the next level of the live devicetree, as long as overlay
+ * subtree node also exists in the live devicetree. When a node in the overlay
+ * subtree does not exist at the same level in the live devicetree, target->np
+ * points to a newly allocated node, and all subsequent targets in the subtree
+ * will be newly allocated nodes.
+ */
+struct target {
+ struct device_node *np;
+ bool in_livetree;
+};
+
+/**
* struct fragment - info about fragment nodes in overlay expanded device tree
* @target: target of the overlay operation
* @overlay: pointer to the __overlay__ node
@@ -72,8 +92,7 @@ static int devicetree_corrupt(void)
}
static int build_changeset_next_level(struct overlay_changeset *ovcs,
- struct device_node *target_node,
- const struct device_node *overlay_node);
+ struct target *target, const struct device_node *overlay_node);
/*
* of_resolve_phandles() finds the largest phandle in the live tree.
@@ -257,14 +276,17 @@ static struct property *dup_and_fixup_symbol_prop(
/**
* add_changeset_property() - add @overlay_prop to overlay changeset
* @ovcs: overlay changeset
- * @target_node: where to place @overlay_prop in live tree
+ * @target: where @overlay_prop will be placed
* @overlay_prop: property to add or update, from overlay tree
* @is_symbols_prop: 1 if @overlay_prop is from node "/__symbols__"
*
- * If @overlay_prop does not already exist in @target_node, add changeset entry
- * to add @overlay_prop in @target_node, else add changeset entry to update
+ * If @overlay_prop does not already exist in live devicetree, add changeset
+ * entry to add @overlay_prop in @target, else add changeset entry to update
* value of @overlay_prop.
*
+ * @target may be either in the live devicetree or in a new subtree that
+ * is contained in the changeset.
+ *
* Some special properties are not updated (no error returned).
*
* Update of property in symbols node is not allowed.
@@ -273,20 +295,22 @@ static struct property *dup_and_fixup_symbol_prop(
* invalid @overlay.
*/
static int add_changeset_property(struct overlay_changeset *ovcs,
- struct device_node *target_node,
- struct property *overlay_prop,
+ struct target *target, struct property *overlay_prop,
bool is_symbols_prop)
{
struct property *new_prop = NULL, *prop;
int ret = 0;
- prop = of_find_property(target_node, overlay_prop->name, NULL);
-
if (!of_prop_cmp(overlay_prop->name, "name") ||
!of_prop_cmp(overlay_prop->name, "phandle") ||
!of_prop_cmp(overlay_prop->name, "linux,phandle"))
return 0;
+ if (target->in_livetree)
+ prop = of_find_property(target->np, overlay_prop->name, NULL);
+ else
+ prop = NULL;
+
if (is_symbols_prop) {
if (prop)
return -EINVAL;
@@ -299,10 +323,10 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
return -ENOMEM;
if (!prop)
- ret = of_changeset_add_property(&ovcs->cset, target_node,
+ ret = of_changeset_add_property(&ovcs->cset, target->np,
new_prop);
else
- ret = of_changeset_update_property(&ovcs->cset, target_node,
+ ret = of_changeset_update_property(&ovcs->cset, target->np,
new_prop);
if (ret) {
@@ -315,14 +339,14 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
/**
* add_changeset_node() - add @node (and children) to overlay changeset
- * @ovcs: overlay changeset
- * @target_node: where to place @node in live tree
- * @node: node from within overlay device tree fragment
+ * @ovcs: overlay changeset
+ * @target: where @overlay_prop will be placed in live tree or changeset
+ * @node: node from within overlay device tree fragment
*
- * If @node does not already exist in @target_node, add changeset entry
- * to add @node in @target_node.
+ * If @node does not already exist in @target, add changeset entry
+ * to add @node in @target.
*
- * If @node already exists in @target_node, and the existing node has
+ * If @node already exists in @target, and the existing node has
* a phandle, the overlay node is not allowed to have a phandle.
*
* If @node has child nodes, add the children recursively via
@@ -355,15 +379,16 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
* invalid @overlay.
*/
static int add_changeset_node(struct overlay_changeset *ovcs,
- struct device_node *target_node, struct device_node *node)
+ struct target *target, struct device_node *node)
{
const char *node_kbasename;
struct device_node *tchild;
+ struct target target_child;
int ret = 0;
node_kbasename = kbasename(node->full_name);
- for_each_child_of_node(target_node, tchild)
+ for_each_child_of_node(target->np, tchild)
if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
break;
@@ -372,22 +397,28 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
if (!tchild)
return -ENOMEM;
- tchild->parent = target_node;
+ tchild->parent = target->np;
of_node_set_flag(tchild, OF_OVERLAY);
ret = of_changeset_attach_node(&ovcs->cset, tchild);
if (ret)
return ret;
- ret = build_changeset_next_level(ovcs, tchild, node);
+ target_child.np = tchild;
+ target_child.in_livetree = false;
+
+ ret = build_changeset_next_level(ovcs, &target_child, node);
of_node_put(tchild);
return ret;
}
- if (node->phandle && tchild->phandle)
+ if (node->phandle && tchild->phandle) {
ret = -EINVAL;
- else
- ret = build_changeset_next_level(ovcs, tchild, node);
+ } else {
+ target_child.np = tchild;
+ target_child.in_livetree = target->in_livetree;
+ ret = build_changeset_next_level(ovcs, &target_child, node);
+ }
of_node_put(tchild);
return ret;
@@ -396,7 +427,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
/**
* build_changeset_next_level() - add level of overlay changeset
* @ovcs: overlay changeset
- * @target_node: where to place @overlay_node in live tree
+ * @target: where to place @overlay_node in live tree
* @overlay_node: node from within an overlay device tree fragment
*
* Add the properties (if any) and nodes (if any) from @overlay_node to the
@@ -409,27 +440,26 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
* invalid @overlay_node.
*/
static int build_changeset_next_level(struct overlay_changeset *ovcs,
- struct device_node *target_node,
- const struct device_node *overlay_node)
+ struct target *target, const struct device_node *overlay_node)
{
struct device_node *child;
struct property *prop;
int ret;
for_each_property_of_node(overlay_node, prop) {
- ret = add_changeset_property(ovcs, target_node, prop, 0);
+ ret = add_changeset_property(ovcs, target, prop, 0);
if (ret) {
pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
- target_node, prop->name, ret);
+ target->np, prop->name, ret);
return ret;
}
}
for_each_child_of_node(overlay_node, child) {
- ret = add_changeset_node(ovcs, target_node, child);
+ ret = add_changeset_node(ovcs, target, child);
if (ret) {
pr_debug("Failed to apply node @%pOF/%s, err=%d\n",
- target_node, child->name, ret);
+ target->np, child->name, ret);
of_node_put(child);
return ret;
}
@@ -442,17 +472,17 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
* Add the properties from __overlay__ node to the @ovcs->cset changeset.
*/
static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
- struct device_node *target_node,
+ struct target *target,
const struct device_node *overlay_symbols_node)
{
struct property *prop;
int ret;
for_each_property_of_node(overlay_symbols_node, prop) {
- ret = add_changeset_property(ovcs, target_node, prop, 1);
+ ret = add_changeset_property(ovcs, target, prop, 1);
if (ret) {
pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
- target_node, prop->name, ret);
+ target->np, prop->name, ret);
return ret;
}
}
@@ -475,6 +505,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
static int build_changeset(struct overlay_changeset *ovcs)
{
struct fragment *fragment;
+ struct target target;
int fragments_count, i, ret;
/*
@@ -489,7 +520,9 @@ static int build_changeset(struct overlay_changeset *ovcs)
for (i = 0; i < fragments_count; i++) {
fragment = &ovcs->fragments[i];
- ret = build_changeset_next_level(ovcs, fragment->target,
+ target.np = fragment->target;
+ target.in_livetree = true;
+ ret = build_changeset_next_level(ovcs, &target,
fragment->overlay);
if (ret) {
pr_debug("apply failed '%pOF'\n", fragment->target);
@@ -499,7 +532,10 @@ static int build_changeset(struct overlay_changeset *ovcs)
if (ovcs->symbols_fragment) {
fragment = &ovcs->fragments[ovcs->count - 1];
- ret = build_changeset_symbols_node(ovcs, fragment->target,
+
+ target.np = fragment->target;
+ target.in_livetree = true;
+ ret = build_changeset_symbols_node(ovcs, &target,
fragment->overlay);
if (ret) {
pr_debug("apply failed '%pOF'\n", fragment->target);
@@ -517,7 +553,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
* 1) "target" property containing the phandle of the target
* 2) "target-path" property containing the path of the target
*/
-static struct device_node *find_target_node(struct device_node *info_node)
+static struct device_node *find_target(struct device_node *info_node)
{
struct device_node *node;
const char *path;
@@ -623,7 +659,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
fragment = &fragments[cnt];
fragment->overlay = overlay_node;
- fragment->target = find_target_node(node);
+ fragment->target = find_target(node);
if (!fragment->target) {
of_node_put(fragment->overlay);
ret = -EINVAL;
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
There is a matching of_node_put() in __of_detach_node_sysfs()
Remove misleading comment from function header comment for
of_detach_node().
This patch may result in memory leaks from code that directly calls
the dynamic node add and delete functions directly instead of
using changesets.
Signed-off-by: Frank Rowand <[email protected]>
---
This patch should result in powerpc systems that dynamically
allocate a node, then later deallocate the node to have a
memory leak when the node is deallocated.
The next patch in the series will fix the leak.
drivers/of/dynamic.c | 3 ---
drivers/of/kobj.c | 4 +++-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index b04ee021a891..275c0d7e2268 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -272,9 +272,6 @@ void __of_detach_node(struct device_node *np)
/**
* of_detach_node() - "Unplug" a node from the device tree.
- *
- * The caller must hold a reference to the node. The memory associated with
- * the node is not freed until its refcount goes to zero.
*/
int of_detach_node(struct device_node *np)
{
diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 7a0a18980b98..c72eef988041 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -133,6 +133,9 @@ int __of_attach_node_sysfs(struct device_node *np)
}
if (!name)
return -ENOMEM;
+
+ of_node_get(np);
+
rc = kobject_add(&np->kobj, parent, "%s", name);
kfree(name);
if (rc)
@@ -159,6 +162,5 @@ void __of_detach_node_sysfs(struct device_node *np)
kobject_del(&np->kobj);
}
- /* finally remove the kobj_init ref */
of_node_put(np);
}
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
Add checks:
- attempted kfree due to refcount reaching zero before overlay
is removed
- properties linked to an overlay node when the node is removed
- node refcount > one during node removal in a changeset destroy,
if the node was created by the changeset
After applying this patch, several validation warnings will be
reported from the devicetree unittest during boot due to
pre-existing devicetree bugs. The warnings will be similar to:
OF: ERROR: of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest11/test-unittest111 contains unexpected properties
OF: ERROR: memory leak - destroy cset entry: attach overlay node /testcase-data-2/substation@100/hvac-medium-2 with refcount 2
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
drivers/of/overlay.c | 1 +
include/linux/of.h | 15 ++++++++++-----
3 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index f4f8ed9b5454..b04ee021a891 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -330,6 +330,25 @@ void of_node_release(struct kobject *kobj)
if (!of_node_check_flag(node, OF_DYNAMIC))
return;
+ if (of_node_check_flag(node, OF_OVERLAY)) {
+
+ if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
+ /* premature refcount of zero, do not free memory */
+ pr_err("ERROR: memory leak %s() overlay node %pOF before free overlay changeset\n",
+ __func__, node);
+ return;
+ }
+
+ /*
+ * If node->properties non-empty then properties were added
+ * to this node either by different overlay that has not
+ * yet been removed, or by a non-overlay mechanism.
+ */
+ if (node->properties)
+ pr_err("ERROR: %s() overlay node %pOF contains unexpected properties\n",
+ __func__, node);
+ }
+
property_list_free(node->properties);
property_list_free(node->deadprops);
@@ -434,6 +453,16 @@ struct device_node *__of_node_dup(const struct device_node *np,
static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
{
+ if (ce->action == OF_RECONFIG_ATTACH_NODE &&
+ of_node_check_flag(ce->np, OF_OVERLAY)) {
+ if (kref_read(&ce->np->kobj.kref) > 1) {
+ pr_err("ERROR: memory leak - destroy cset entry: attach overlay node %pOF with refcount %d\n",
+ ce->np, kref_read(&ce->np->kobj.kref));
+ } else {
+ of_node_set_flag(ce->np, OF_OVERLAY_FREE_CSET);
+ }
+ }
+
of_node_put(ce->np);
list_del(&ce->node);
kfree(ce);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index eda57ef12fd0..1176cb4b6e4e 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -373,6 +373,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
return -ENOMEM;
tchild->parent = target_node;
+ of_node_set_flag(tchild, OF_OVERLAY);
ret = of_changeset_attach_node(&ovcs->cset, tchild);
if (ret)
diff --git a/include/linux/of.h b/include/linux/of.h
index 4d25e4f952d9..aa1dafaec6ae 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -138,11 +138,16 @@ static inline void of_node_put(struct device_node *node) { }
extern struct device_node *of_stdout;
extern raw_spinlock_t devtree_lock;
-/* flag descriptions (need to be visible even when !CONFIG_OF) */
-#define OF_DYNAMIC 1 /* node and properties were allocated via kmalloc */
-#define OF_DETACHED 2 /* node has been detached from the device tree */
-#define OF_POPULATED 3 /* device already created for the node */
-#define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children of this node */
+/*
+ * struct device_node flag descriptions
+ * (need to be visible even when !CONFIG_OF)
+ */
+#define OF_DYNAMIC 1 /* (and properties) allocated via kmalloc */
+#define OF_DETACHED 2 /* detached from the device tree */
+#define OF_POPULATED 3 /* device already created */
+#define OF_POPULATED_BUS 4 /* platform bus created for children */
+#define OF_OVERLAY 5 /* allocated for an overlay */
+#define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */
#define OF_BAD_ADDR ((u64)-1)
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
When allocating a new node, add_changeset_node() was duplicating the
properties from the respective node in the overlay instead of
allocating a node with no properties.
When this patch is applied the errors reported by the devictree
unittest from patch "of: overlay: add tests to validate kfrees from
overlay removal" will no longer occur. These error messages are of
the form:
"OF: ERROR: ..."
and the unittest results will change from:
### dt-test ### end of unittest - 203 passed, 7 failed
to
### dt-test ### end of unittest - 210 passed, 0 failed
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/of/overlay.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 0b0904f44bc7..c113186e222c 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -393,7 +393,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
break;
if (!tchild) {
- tchild = __of_node_dup(node, node_kbasename);
+ tchild = __of_node_dup(NULL, node_kbasename);
if (!tchild)
return -ENOMEM;
--
Frank Rowand <[email protected]>
From: Frank Rowand <[email protected]>
"of: overlay: add missing of_node_get() in __of_attach_node_sysfs"
added a missing of_node_get() to __of_attach_node_sysfs(). This
results in a refcount imbalance for nodes attached with
dlpar_attach_node(). The calling sequence from dlpar_attach_node()
to __of_attach_node_sysfs() is:
dlpar_attach_node()
of_attach_node()
__of_attach_node_sysfs()
Signed-off-by: Frank Rowand <[email protected]>
---
***** UNTESTED. I need people with the affected PowerPC systems
***** (systems that dynamically allocate and deallocate
***** devicetree nodes) to test this patch.
arch/powerpc/platforms/pseries/dlpar.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index a0b20c03f078..e3010b14aea5 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -272,6 +272,8 @@ int dlpar_detach_node(struct device_node *dn)
if (rc)
return rc;
+ of_node_put(dn);
+
return 0;
}
--
Frank Rowand <[email protected]>
On 10/04/2018 09:12 PM, [email protected] wrote:
> From: Frank Rowand <[email protected]>
>
> Callers of of_irq_parse_one() blindly use the pointer args.np
> without checking whether of_irq_parse_one() had an error and
> thus did not set the value of args.np. Initialize args to
> zero so that using the format "%pOF" to show the value of
> args.np will show "(null)" when of_irq_parse_one() has an
> error and does not set args.np instead of trying to
> dereference a random value.
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Frank Rowand <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
The same problem exists when of_parse_phandle_with_args() reports an error.
Guenter
> ---
> drivers/of/unittest.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 6d80f474c8f2..b61a33f30a56 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -780,7 +780,7 @@ static void __init of_unittest_parse_interrupts(void)
> for (i = 0; i < 4; i++) {
> bool passed = true;
>
> - args.args_count = 0;
> + memset(&args, 0, sizeof(args));
> rc = of_irq_parse_one(np, i, &args);
>
> passed &= !rc;
> @@ -801,7 +801,7 @@ static void __init of_unittest_parse_interrupts(void)
> for (i = 0; i < 4; i++) {
> bool passed = true;
>
> - args.args_count = 0;
> + memset(&args, 0, sizeof(args));
> rc = of_irq_parse_one(np, i, &args);
>
> /* Test the values from tests-phandle.dtsi */
> @@ -854,6 +854,7 @@ static void __init of_unittest_parse_interrupts_extended(void)
> for (i = 0; i < 7; i++) {
> bool passed = true;
>
> + memset(&args, 0, sizeof(args));
> rc = of_irq_parse_one(np, i, &args);
>
> /* Test the values from tests-phandle.dtsi */
>
On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>
> From: Frank Rowand <[email protected]>
>
> Callers of of_irq_parse_one() blindly use the pointer args.np
> without checking whether of_irq_parse_one() had an error and
> thus did not set the value of args.np. Initialize args to
> zero so that using the format "%pOF" to show the value of
> args.np will show "(null)" when of_irq_parse_one() has an
> error and does not set args.np instead of trying to
> dereference a random value.
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> drivers/of/unittest.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Does this need to be part of this series?
Rob
On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>
> From: Frank Rowand <[email protected]>
>
> If overlay properties #address-cells or #size-cells are already in
> the live devicetree for any given node, then the values in the
> overlay must match the values in the live tree.
>
> If the properties are already in the live tree then there is no
> need to create a changeset entry to add them since they must
> have the same value. This reduces the memory used by the
> changeset and eliminates a possible memory leak. This is
> verified by 12 fewer warnings during the devicetree unittest,
> as the possible memory leak warnings about #address-cells and
and...?
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 29c33a5c533f..e6fb3ffe9d93 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> * @target may be either in the live devicetree or in a new subtree that
> * is contained in the changeset.
> *
> - * Some special properties are not updated (no error returned).
> + * Some special properties are not added or updated (no error returned):
> + * "name", "phandle", "linux,phandle".
> + *
> + * Properties "#address-cells" and "#size-cells" are not updated if they
> + * are already in the live tree, but if present in the live tree, the values
> + * in the overlay must match the values in the live tree.
Perhaps this should be generalized to apply to any property? We can't
really deal with property values changing on the fly anyways.
> *
> * Update of property in symbols node is not allowed.
> *
> @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> {
> struct property *new_prop = NULL, *prop;
> int ret = 0;
> + bool check_for_non_overlay_node = false;
>
> if (!of_prop_cmp(overlay_prop->name, "name") ||
> !of_prop_cmp(overlay_prop->name, "phandle") ||
> @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> if (!new_prop)
> return -ENOMEM;
>
> - if (!prop)
> + if (!prop) {
> +
Remove the extra blank lines.
> + check_for_non_overlay_node = true;
> ret = of_changeset_add_property(&ovcs->cset, target->np,
> new_prop);
> - else
> +
> + } else if (!of_prop_cmp(prop->name, "#address-cells")) {
> +
> + if (prop->length != 4 || new_prop->length != 4 ||
> + *(u32 *)prop->value != *(u32 *)new_prop->value)
Technically these are __be32 types. This could use a helper (of_prop_val_eq).
I'm not sure we really need to validate the length here as dtc does
that (but yes, not everything is from dtc).
> + pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
> + target->np);
> +
> + } else if (!of_prop_cmp(prop->name, "#size-cells")) {
> +
> + if (prop->length != 4 || new_prop->length != 4 ||
> + *(u32 *)prop->value != *(u32 *)new_prop->value)
> + pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
> + target->np);
> +
> + } else {
> +
> + check_for_non_overlay_node = true;
> ret = of_changeset_update_property(&ovcs->cset, target->np,
> new_prop);
>
> + }
> +
> + if (check_for_non_overlay_node &&
> + !of_node_check_flag(target->np, OF_OVERLAY))
> + pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
> + __func__, target->np, new_prop->name);
> +
> if (ret) {
> kfree(new_prop->name);
> kfree(new_prop->value);
> --
> Frank Rowand <[email protected]>
>
On 10/05/18 08:07, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>>
>> From: Frank Rowand <[email protected]>
>>
>> If overlay properties #address-cells or #size-cells are already in
>> the live devicetree for any given node, then the values in the
>> overlay must match the values in the live tree.
>>
>> If the properties are already in the live tree then there is no
>> need to create a changeset entry to add them since they must
>> have the same value. This reduces the memory used by the
>> changeset and eliminates a possible memory leak. This is
>> verified by 12 fewer warnings during the devicetree unittest,
>> as the possible memory leak warnings about #address-cells and
>
> and...?
#size-cells no longer occur.
(Thanks for catching that.)
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 29c33a5c533f..e6fb3ffe9d93 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
>> * @target may be either in the live devicetree or in a new subtree that
>> * is contained in the changeset.
>> *
>> - * Some special properties are not updated (no error returned).
>> + * Some special properties are not added or updated (no error returned):
>> + * "name", "phandle", "linux,phandle".
>> + *
>> + * Properties "#address-cells" and "#size-cells" are not updated if they
>> + * are already in the live tree, but if present in the live tree, the values
>> + * in the overlay must match the values in the live tree.
>
> Perhaps this should be generalized to apply to any property? We can't
> really deal with property values changing on the fly anyways.
That is a bigger discussion. I'd prefer to not hold up this series for that
question to be resolved. It will be easy enough to generalize in an add-on
patch later.
>> *
>> * Update of property in symbols node is not allowed.
>> *
>> @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>> {
>> struct property *new_prop = NULL, *prop;
>> int ret = 0;
>> + bool check_for_non_overlay_node = false;
>>
>> if (!of_prop_cmp(overlay_prop->name, "name") ||
>> !of_prop_cmp(overlay_prop->name, "phandle") ||
>> @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>> if (!new_prop)
>> return -ENOMEM;
>>
>> - if (!prop)
>> + if (!prop) {
>> +
>
> Remove the extra blank lines.
Will do.
>
>> + check_for_non_overlay_node = true;
>> ret = of_changeset_add_property(&ovcs->cset, target->np,
>> new_prop);
>> - else
>> +
>> + } else if (!of_prop_cmp(prop->name, "#address-cells")) {
>> +
>> + if (prop->length != 4 || new_prop->length != 4 ||
>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
>
> Technically these are __be32 types. This could use a helper (of_prop_val_eq).
These are in a unpacked form, so cpu byte order, not FDT byte order.
>
> I'm not sure we really need to validate the length here as dtc does
> that (but yes, not everything is from dtc).
Since I'm accessing 4 bytes of the values, I need to be sure the lengths
are at least 4. For #address-cells and #size-cells the property is
specified as four bytes, so I could simplify the code for the specific case.
If this gets extended to any arbitrary property then a new of_prop_val_eq()
would check that the lengths are equal and the values (of size length) are
also equal.
>
>> + pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
>> + target->np);
>> +
>> + } else if (!of_prop_cmp(prop->name, "#size-cells")) {
>> +
>> + if (prop->length != 4 || new_prop->length != 4 ||
>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
>> + pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
>> + target->np);
>> +
>> + } else {
>> +
>> + check_for_non_overlay_node = true;
>> ret = of_changeset_update_property(&ovcs->cset, target->np,
>> new_prop);
>>
>> + }
>> +
>> + if (check_for_non_overlay_node &&
>> + !of_node_check_flag(target->np, OF_OVERLAY))
>> + pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
>> + __func__, target->np, new_prop->name);
>> +
>> if (ret) {
>> kfree(new_prop->name);
>> kfree(new_prop->value);
>> --
>> Frank Rowand <[email protected]>
>>
>
On 10/05/18 07:53, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>>
>> From: Frank Rowand <[email protected]>
>>
>> Callers of of_irq_parse_one() blindly use the pointer args.np
>> without checking whether of_irq_parse_one() had an error and
>> thus did not set the value of args.np. Initialize args to
>> zero so that using the format "%pOF" to show the value of
>> args.np will show "(null)" when of_irq_parse_one() has an
>> error and does not set args.np instead of trying to
>> dereference a random value.
>>
>> Reported-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> drivers/of/unittest.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Does this need to be part of this series?
I do not know if it could be independently applied before the
rest of the series (I have not tested that). I included it
in the series because the series has so many other changes to
unittest.c.
If you want me to break this out, I will remove it from this
series and resend it after the rest of the series has been
pulled to mainline (and rebase on the new mainline).
This patch is not fixing a known failure case - the current
test data does not trigger the problem. The recent patch
from Guenter that you already accepted fixes the known failure
case, so this patch is not urgent. The same is true about the
other cases Guenter pointed out that this patch should fix.
-Frank
On Fri, Oct 5, 2018 at 1:53 PM Frank Rowand <[email protected]> wrote:
>
> On 10/05/18 08:07, Rob Herring wrote:
> > On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
> >>
> >> From: Frank Rowand <[email protected]>
> >>
> >> If overlay properties #address-cells or #size-cells are already in
> >> the live devicetree for any given node, then the values in the
> >> overlay must match the values in the live tree.
> >>
> >> If the properties are already in the live tree then there is no
> >> need to create a changeset entry to add them since they must
> >> have the same value. This reduces the memory used by the
> >> changeset and eliminates a possible memory leak. This is
> >> verified by 12 fewer warnings during the devicetree unittest,
> >> as the possible memory leak warnings about #address-cells and
> >
> > and...?
>
> #size-cells no longer occur.
>
> (Thanks for catching that.)
>
>
> >>
> >> Signed-off-by: Frank Rowand <[email protected]>
> >> ---
> >> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 35 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >> index 29c33a5c533f..e6fb3ffe9d93 100644
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> >> * @target may be either in the live devicetree or in a new subtree that
> >> * is contained in the changeset.
> >> *
> >> - * Some special properties are not updated (no error returned).
> >> + * Some special properties are not added or updated (no error returned):
> >> + * "name", "phandle", "linux,phandle".
> >> + *
> >> + * Properties "#address-cells" and "#size-cells" are not updated if they
> >> + * are already in the live tree, but if present in the live tree, the values
> >> + * in the overlay must match the values in the live tree.
> >
> > Perhaps this should be generalized to apply to any property? We can't
> > really deal with property values changing on the fly anyways.
>
> That is a bigger discussion. I'd prefer to not hold up this series for that
> question to be resolved. It will be easy enough to generalize in an add-on
> patch later.
Fair enough.
> >> + if (prop->length != 4 || new_prop->length != 4 ||
> >> + *(u32 *)prop->value != *(u32 *)new_prop->value)
> >
> > Technically these are __be32 types. This could use a helper (of_prop_val_eq).
>
> These are in a unpacked form, so cpu byte order, not FDT byte order.
You sure about that? Unpacking doesn't change the order. It can't
because the type is unknown. The value of 'value' is the address of
the data in the FDT.
> > I'm not sure we really need to validate the length here as dtc does
> > that (but yes, not everything is from dtc).
>
> Since I'm accessing 4 bytes of the values, I need to be sure the lengths
> are at least 4. For #address-cells and #size-cells the property is
> specified as four bytes, so I could simplify the code for the specific case.
>
> If this gets extended to any arbitrary property then a new of_prop_val_eq()
> would check that the lengths are equal and the values (of size length) are
> also equal.
Right, that's what I was thinking. Check lengths are equal and then
you can just do a memcmp().
Rob
On 10/05/18 06:26, Guenter Roeck wrote:
> On 10/04/2018 09:12 PM, [email protected] wrote:
>> From: Frank Rowand <[email protected]>
>>
>> Callers of of_irq_parse_one() blindly use the pointer args.np
>> without checking whether of_irq_parse_one() had an error and
>> thus did not set the value of args.np. Initialize args to
>> zero so that using the format "%pOF" to show the value of
>> args.np will show "(null)" when of_irq_parse_one() has an
>> error and does not set args.np instead of trying to
>> dereference a random value.
>>
>> Reported-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Frank Rowand <[email protected]>
>
> Reviewed-by: Guenter Roeck <[email protected]>
>
> The same problem exists when of_parse_phandle_with_args() reports an error.
Thanks, I'll add a fix for that.
-Frank
>
> Guenter
>
>> ---
>> Â drivers/of/unittest.c | 5 +++--
>> Â 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 6d80f474c8f2..b61a33f30a56 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -780,7 +780,7 @@ static void __init of_unittest_parse_interrupts(void)
>> Â Â Â Â Â for (i = 0; i < 4; i++) {
>> Â Â Â Â Â Â Â Â Â bool passed = true;
>> Â -Â Â Â Â Â Â Â args.args_count = 0;
>> +Â Â Â Â Â Â Â memset(&args, 0, sizeof(args));
>> Â Â Â Â Â Â Â Â Â rc = of_irq_parse_one(np, i, &args);
>> Â Â Â Â Â Â Â Â Â Â passed &= !rc;
>> @@ -801,7 +801,7 @@ static void __init of_unittest_parse_interrupts(void)
>> Â Â Â Â Â for (i = 0; i < 4; i++) {
>> Â Â Â Â Â Â Â Â Â bool passed = true;
>> Â -Â Â Â Â Â Â Â args.args_count = 0;
>> +Â Â Â Â Â Â Â memset(&args, 0, sizeof(args));
>> Â Â Â Â Â Â Â Â Â rc = of_irq_parse_one(np, i, &args);
>> Â Â Â Â Â Â Â Â Â Â /* Test the values from tests-phandle.dtsi */
>> @@ -854,6 +854,7 @@ static void __init of_unittest_parse_interrupts_extended(void)
>> Â Â Â Â Â for (i = 0; i < 7; i++) {
>> Â Â Â Â Â Â Â Â Â bool passed = true;
>> Â +Â Â Â Â Â Â Â memset(&args, 0, sizeof(args));
>> Â Â Â Â Â Â Â Â Â rc = of_irq_parse_one(np, i, &args);
>> Â Â Â Â Â Â Â Â Â Â /* Test the values from tests-phandle.dtsi */
>>
>
>
On 10/05/18 12:04, Rob Herring wrote:
> On Fri, Oct 5, 2018 at 1:53 PM Frank Rowand <[email protected]> wrote:
>>
>> On 10/05/18 08:07, Rob Herring wrote:
>>> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>>>>
>>>> From: Frank Rowand <[email protected]>
>>>>
>>>> If overlay properties #address-cells or #size-cells are already in
>>>> the live devicetree for any given node, then the values in the
>>>> overlay must match the values in the live tree.
>>>>
>>>> If the properties are already in the live tree then there is no
>>>> need to create a changeset entry to add them since they must
>>>> have the same value. This reduces the memory used by the
>>>> changeset and eliminates a possible memory leak. This is
>>>> verified by 12 fewer warnings during the devicetree unittest,
>>>> as the possible memory leak warnings about #address-cells and
>>>
>>> and...?
>>
>> #size-cells no longer occur.
>>
>> (Thanks for catching that.)
>>
>>
>>>>
>>>> Signed-off-by: Frank Rowand <[email protected]>
>>>> ---
>>>> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index 29c33a5c533f..e6fb3ffe9d93 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
>>>> * @target may be either in the live devicetree or in a new subtree that
>>>> * is contained in the changeset.
>>>> *
>>>> - * Some special properties are not updated (no error returned).
>>>> + * Some special properties are not added or updated (no error returned):
>>>> + * "name", "phandle", "linux,phandle".
>>>> + *
>>>> + * Properties "#address-cells" and "#size-cells" are not updated if they
>>>> + * are already in the live tree, but if present in the live tree, the values
>>>> + * in the overlay must match the values in the live tree.
>>>
>>> Perhaps this should be generalized to apply to any property? We can't
>>> really deal with property values changing on the fly anyways.
>>
>> That is a bigger discussion. I'd prefer to not hold up this series for that
>> question to be resolved. It will be easy enough to generalize in an add-on
>> patch later.
>
> Fair enough.
>
>>>> + if (prop->length != 4 || new_prop->length != 4 ||
>>>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
>>>
>>> Technically these are __be32 types. This could use a helper (of_prop_val_eq).
>>
>> These are in a unpacked form, so cpu byte order, not FDT byte order.
>
> You sure about that? Unpacking doesn't change the order. It can't
> because the type is unknown. The value of 'value' is the address of
> the data in the FDT.
Aargh. You are totally right.
>>> I'm not sure we really need to validate the length here as dtc does
>>> that (but yes, not everything is from dtc).
>>
>> Since I'm accessing 4 bytes of the values, I need to be sure the lengths
>> are at least 4. For #address-cells and #size-cells the property is
>> specified as four bytes, so I could simplify the code for the specific case.
>>
>> If this gets extended to any arbitrary property then a new of_prop_val_eq()
>> would check that the lengths are equal and the values (of size length) are
>> also equal.
>
> Right, that's what I was thinking. Check lengths are equal and then
> you can just do a memcmp().
Based on all of this it seems better that I create of_prop_val_eq(), as you
suggested, and change to use that.
>
> Rob
>
On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>
> From: Frank Rowand <[email protected]>
>
> If overlay properties #address-cells or #size-cells are already in
> the live devicetree for any given node, then the values in the
> overlay must match the values in the live tree.
Hi Frank,
I'm starting some FPGA testing on this patchset applied to v4.19-rc7.
That applied cleanly; if that's not the best base to test against,
please let me know.
On a very simple overlay, I'm seeing this patch's warning catching
things other than #address-cells or #size-cells. I'm just getting
started looking at this, will spend time understanding this better and
I'll test other overlays. The warnings were:
Applying dtbo: socfpga_overlay.dtb
[ 33.117881] fpga_manager fpga0: writing soc_system.rbf to Altera
SOCFPGA FPGA Manager
[ 33.575223] OF: overlay: WARNING: add_changeset_property(), memory
leak will occur if overlay removed. Property:
/soc/base-fpga-region/firmware-name
[ 33.588584] OF: overlay: WARNING: add_changeset_property(), memory
leak will occur if overlay removed. Property:
/soc/base-fpga-region/fpga-bridges
[ 33.601856] OF: overlay: WARNING: add_changeset_property(), memory
leak will occur if overlay removed. Property:
/soc/base-fpga-region/ranges
Here's part of that overlay including the properties it's complaining about:
/dts-v1/;
/plugin/;
/ {
fragment@0 {
target = <&base_fpga_region>;
#address-cells = <1>;
#size-cells = <1>;
__overlay__ {
#address-cells = <1>;
#size-cells = <1>;
firmware-name = "soc_system.rbf";
fpga-bridges = <&fpga_bridge1>;
ranges = <0x20000 0xff200000 0x100000>,
<0x0 0xc0000000 0x20000000>;
gpio@10040 {
so on...
By the way, I didn't get any warnings when I subsequently removed this overlay.
Alan
>
> If the properties are already in the live tree then there is no
> need to create a changeset entry to add them since they must
> have the same value. This reduces the memory used by the
> changeset and eliminates a possible memory leak. This is
> verified by 12 fewer warnings during the devicetree unittest,
> as the possible memory leak warnings about #address-cells and
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 29c33a5c533f..e6fb3ffe9d93 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> * @target may be either in the live devicetree or in a new subtree that
> * is contained in the changeset.
> *
> - * Some special properties are not updated (no error returned).
> + * Some special properties are not added or updated (no error returned):
> + * "name", "phandle", "linux,phandle".
> + *
> + * Properties "#address-cells" and "#size-cells" are not updated if they
> + * are already in the live tree, but if present in the live tree, the values
> + * in the overlay must match the values in the live tree.
> *
> * Update of property in symbols node is not allowed.
> *
> @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> {
> struct property *new_prop = NULL, *prop;
> int ret = 0;
> + bool check_for_non_overlay_node = false;
>
> if (!of_prop_cmp(overlay_prop->name, "name") ||
> !of_prop_cmp(overlay_prop->name, "phandle") ||
> @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> if (!new_prop)
> return -ENOMEM;
>
> - if (!prop)
> + if (!prop) {
> +
> + check_for_non_overlay_node = true;
> ret = of_changeset_add_property(&ovcs->cset, target->np,
> new_prop);
> - else
> +
> + } else if (!of_prop_cmp(prop->name, "#address-cells")) {
> +
> + if (prop->length != 4 || new_prop->length != 4 ||
> + *(u32 *)prop->value != *(u32 *)new_prop->value)
> + pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
> + target->np);
> +
> + } else if (!of_prop_cmp(prop->name, "#size-cells")) {
> +
> + if (prop->length != 4 || new_prop->length != 4 ||
> + *(u32 *)prop->value != *(u32 *)new_prop->value)
> + pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
> + target->np);
> +
> + } else {
> +
> + check_for_non_overlay_node = true;
> ret = of_changeset_update_property(&ovcs->cset, target->np,
> new_prop);
>
> + }
> +
> + if (check_for_non_overlay_node &&
> + !of_node_check_flag(target->np, OF_OVERLAY))
> + pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
> + __func__, target->np, new_prop->name);
> +
> if (ret) {
> kfree(new_prop->name);
> kfree(new_prop->value);
> --
> Frank Rowand <[email protected]>
>
On Mon, Oct 8, 2018 at 10:57 AM Alan Tull <[email protected]> wrote:
>
> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
> >
> > From: Frank Rowand <[email protected]>
> >
> > If overlay properties #address-cells or #size-cells are already in
> > the live devicetree for any given node, then the values in the
> > overlay must match the values in the live tree.
>
> Hi Frank,
>
> I'm starting some FPGA testing on this patchset applied to v4.19-rc7.
> That applied cleanly; if that's not the best base to test against,
> please let me know.
>
> On a very simple overlay, I'm seeing this patch's warning catching
> things other than #address-cells or #size-cells.
What it's warning about are new properties being added to an existing
node. So !prop is true and !of_node_check_flag(target->np,
OF_OVERLAY) also is true. Is that a potential memory leak as you are
warning? If so, your code is working as planned and you'll just need
to document that also in the header.
> I'm just getting
> started looking at this, will spend time understanding this better and
> I'll test other overlays. The warnings were:
>
> Applying dtbo: socfpga_overlay.dtb
> [ 33.117881] fpga_manager fpga0: writing soc_system.rbf to Altera
> SOCFPGA FPGA Manager
> [ 33.575223] OF: overlay: WARNING: add_changeset_property(), memory
> leak will occur if overlay removed. Property:
> /soc/base-fpga-region/firmware-name
> [ 33.588584] OF: overlay: WARNING: add_changeset_property(), memory
> leak will occur if overlay removed. Property:
> /soc/base-fpga-region/fpga-bridges
> [ 33.601856] OF: overlay: WARNING: add_changeset_property(), memory
> leak will occur if overlay removed. Property:
> /soc/base-fpga-region/ranges
>
> Here's part of that overlay including the properties it's complaining about:
>
> /dts-v1/;
> /plugin/;
> / {
> fragment@0 {
> target = <&base_fpga_region>;
> #address-cells = <1>;
> #size-cells = <1>;
> __overlay__ {
> #address-cells = <1>;
> #size-cells = <1>;
>
> firmware-name = "soc_system.rbf";
> fpga-bridges = <&fpga_bridge1>;
> ranges = <0x20000 0xff200000 0x100000>,
> <0x0 0xc0000000 0x20000000>;
>
> gpio@10040 {
> so on...
>
> By the way, I didn't get any warnings when I subsequently removed this overlay.
>
> Alan
>
> >
> > If the properties are already in the live tree then there is no
> > need to create a changeset entry to add them since they must
> > have the same value. This reduces the memory used by the
> > changeset and eliminates a possible memory leak. This is
> > verified by 12 fewer warnings during the devicetree unittest,
> > as the possible memory leak warnings about #address-cells and
> >
> > Signed-off-by: Frank Rowand <[email protected]>
> > ---
> > drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> > 1 file changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 29c33a5c533f..e6fb3ffe9d93 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> > * @target may be either in the live devicetree or in a new subtree that
> > * is contained in the changeset.
> > *
> > - * Some special properties are not updated (no error returned).
> > + * Some special properties are not added or updated (no error returned):
> > + * "name", "phandle", "linux,phandle".
> > + *
> > + * Properties "#address-cells" and "#size-cells" are not updated if they
> > + * are already in the live tree, but if present in the live tree, the values
> > + * in the overlay must match the values in the live tree.
> > *
> > * Update of property in symbols node is not allowed.
> > *
> > @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> > {
> > struct property *new_prop = NULL, *prop;
> > int ret = 0;
> > + bool check_for_non_overlay_node = false;
> >
> > if (!of_prop_cmp(overlay_prop->name, "name") ||
> > !of_prop_cmp(overlay_prop->name, "phandle") ||
> > @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> > if (!new_prop)
> > return -ENOMEM;
> >
> > - if (!prop)
> > + if (!prop) {
> > +
> > + check_for_non_overlay_node = true;
> > ret = of_changeset_add_property(&ovcs->cset, target->np,
> > new_prop);
> > - else
> > +
> > + } else if (!of_prop_cmp(prop->name, "#address-cells")) {
> > +
> > + if (prop->length != 4 || new_prop->length != 4 ||
> > + *(u32 *)prop->value != *(u32 *)new_prop->value)
> > + pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
> > + target->np);
> > +
> > + } else if (!of_prop_cmp(prop->name, "#size-cells")) {
> > +
> > + if (prop->length != 4 || new_prop->length != 4 ||
> > + *(u32 *)prop->value != *(u32 *)new_prop->value)
> > + pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
> > + target->np);
> > +
> > + } else {
> > +
> > + check_for_non_overlay_node = true;
> > ret = of_changeset_update_property(&ovcs->cset, target->np,
> > new_prop);
> >
> > + }
> > +
> > + if (check_for_non_overlay_node &&
> > + !of_node_check_flag(target->np, OF_OVERLAY))
> > + pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
> > + __func__, target->np, new_prop->name);
> > +
> > if (ret) {
> > kfree(new_prop->name);
> > kfree(new_prop->value);
> > --
> > Frank Rowand <[email protected]>
> >
On 10/08/18 11:46, Alan Tull wrote:
> On Mon, Oct 8, 2018 at 10:57 AM Alan Tull <[email protected]> wrote:
>>
>> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>>>
>>> From: Frank Rowand <[email protected]>
>>>
>>> If overlay properties #address-cells or #size-cells are already in
>>> the live devicetree for any given node, then the values in the
>>> overlay must match the values in the live tree.
>>
>> Hi Frank,
>>
>> I'm starting some FPGA testing on this patchset applied to v4.19-rc7.
>> That applied cleanly; if that's not the best base to test against,
>> please let me know.
I would expect -rc7 to be ok to test against. I'm doing the development
of it on -rc1.
Thanks for the testing.
>> On a very simple overlay, I'm seeing this patch's warning catching
>> things other than #address-cells or #size-cells.
#address-cells and #size-cells escape the warning for properties on an
existing (non-overlay) node if the existing node already contains them
as a special case. Those two properties are needed in the overlay to
avoid dtc compiler warnings. If the same properties already exist in
the base devicetree and have the same values as in the overlay then
there is no need to add property update changeset entries in the overlay
changeset. Since there will not be changeset entries for those two
properties, there will be no memory leak when the changeset is removed.
The special casing of #address-cells and #size-cells is part of the
fix patches that are a result of the validation patches. Thus a little
bit less memory leaking than we have today.
> What it's warning about are new properties being added to an existing
> node. So !prop is true and !of_node_check_flag(target->np,
> OF_OVERLAY) also is true. Is that a potential memory leak as you are
> warning? If so, your code is working as planned and you'll just need
> to document that also in the header.
Yes, you are accurately describing what the check is catching.
The memory leak (on release) is because the memory allocated for overlay
properties is released when the reference count of the node they are
attached is decremented to zero, but only if the node is a dynamic flagged
node (as overlays are). The memory allocated for the overlay properties
will not be freed in this case because the node is not a dynamic node.
>> I'm just getting
>> started looking at this, will spend time understanding this better and
>> I'll test other overlays. The warnings were:
>>
>> Applying dtbo: socfpga_overlay.dtb
>> [ 33.117881] fpga_manager fpga0: writing soc_system.rbf to Altera
>> SOCFPGA FPGA Manager
>> [ 33.575223] OF: overlay: WARNING: add_changeset_property(), memory
>> leak will occur if overlay removed. Property:
>> /soc/base-fpga-region/firmware-name
>> [ 33.588584] OF: overlay: WARNING: add_changeset_property(), memory
>> leak will occur if overlay removed. Property:
>> /soc/base-fpga-region/fpga-bridges
>> [ 33.601856] OF: overlay: WARNING: add_changeset_property(), memory
>> leak will occur if overlay removed. Property:
>> /soc/base-fpga-region/ranges
Are there properties in /soc/base-fpga-region/ in the base devicetree?
If not, then that node could be removed from the base devicetree and first created
in an overlay.
If so, is it possible to add an additional level of node, /soc/base-fpga-region/foo,
which would contain the properties that are warned about above? Then the properties
would be children of an overlay node and the memory would be freed on overlay
release.
This is not actually a suggestion that should be implemented right now, just trying
to understand the possible alternatives, because this would result in an arbitrary
fake level in the tree (which I don't like).
My intent is to leave these validation checks as warnings while we figure out the
best way to solve the underlying memory leak issue. Note that some of the
validation checks result in errors and cause an overlay apply to fail. If I
did those checks correctly, they should only catch cases where the live tree
after applying the overlay was a "corrupt" tree instead of the desired changes.
I expect that Plumbers will be a good place to explore these things.
>> Here's part of that overlay including the properties it's complaining about:
>>
>> /dts-v1/;
>> /plugin/;
>> / {
>> fragment@0 {
>> target = <&base_fpga_region>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> __overlay__ {
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> firmware-name = "soc_system.rbf";
>> fpga-bridges = <&fpga_bridge1>;
>> ranges = <0x20000 0xff200000 0x100000>,
>> <0x0 0xc0000000 0x20000000>;
>>
>> gpio@10040 {
>> so on...
>>
>> By the way, I didn't get any warnings when I subsequently removed this overlay.
Yes, I did not add any check that could catch this at release time.
-Frank
>> Alan
>>
>>>
>>> If the properties are already in the live tree then there is no
>>> need to create a changeset entry to add them since they must
>>> have the same value. This reduces the memory used by the
>>> changeset and eliminates a possible memory leak. This is
>>> verified by 12 fewer warnings during the devicetree unittest,
>>> as the possible memory leak warnings about #address-cells and
>>>
>>> Signed-off-by: Frank Rowand <[email protected]>
>>> ---
>>> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 29c33a5c533f..e6fb3ffe9d93 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
>>> * @target may be either in the live devicetree or in a new subtree that
>>> * is contained in the changeset.
>>> *
>>> - * Some special properties are not updated (no error returned).
>>> + * Some special properties are not added or updated (no error returned):
>>> + * "name", "phandle", "linux,phandle".
>>> + *
>>> + * Properties "#address-cells" and "#size-cells" are not updated if they
>>> + * are already in the live tree, but if present in the live tree, the values
>>> + * in the overlay must match the values in the live tree.
>>> *
>>> * Update of property in symbols node is not allowed.
>>> *
>>> @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>> {
>>> struct property *new_prop = NULL, *prop;
>>> int ret = 0;
>>> + bool check_for_non_overlay_node = false;
>>>
>>> if (!of_prop_cmp(overlay_prop->name, "name") ||
>>> !of_prop_cmp(overlay_prop->name, "phandle") ||
>>> @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>> if (!new_prop)
>>> return -ENOMEM;
>>>
>>> - if (!prop)
>>> + if (!prop) {
>>> +
>>> + check_for_non_overlay_node = true;
>>> ret = of_changeset_add_property(&ovcs->cset, target->np,
>>> new_prop);
>>> - else
>>> +
>>> + } else if (!of_prop_cmp(prop->name, "#address-cells")) {
>>> +
>>> + if (prop->length != 4 || new_prop->length != 4 ||
>>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
>>> + pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
>>> + target->np);
>>> +
>>> + } else if (!of_prop_cmp(prop->name, "#size-cells")) {
>>> +
>>> + if (prop->length != 4 || new_prop->length != 4 ||
>>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
>>> + pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
>>> + target->np);
>>> +
>>> + } else {
>>> +
>>> + check_for_non_overlay_node = true;
>>> ret = of_changeset_update_property(&ovcs->cset, target->np,
>>> new_prop);
>>>
>>> + }
>>> +
>>> + if (check_for_non_overlay_node &&
>>> + !of_node_check_flag(target->np, OF_OVERLAY))
>>> + pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
>>> + __func__, target->np, new_prop->name);
>>> +
>>> if (ret) {
>>> kfree(new_prop->name);
>>> kfree(new_prop->value);
>>> --
>>> Frank Rowand <[email protected]>
>>>
>
On Mon, Oct 8, 2018 at 7:02 PM Frank Rowand <[email protected]> wrote:
>
> On 10/08/18 11:46, Alan Tull wrote:
> > On Mon, Oct 8, 2018 at 10:57 AM Alan Tull <[email protected]> wrote:
> >>
> >> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
> >>>
> >>> From: Frank Rowand <[email protected]>
> >>>
> >>> If overlay properties #address-cells or #size-cells are already in
> >>> the live devicetree for any given node, then the values in the
> >>> overlay must match the values in the live tree.
> >>
> >> Hi Frank,
> >>
> >> I'm starting some FPGA testing on this patchset applied to v4.19-rc7.
> >> That applied cleanly; if that's not the best base to test against,
> >> please let me know.
>
> I would expect -rc7 to be ok to test against. I'm doing the development
> of it on -rc1.
>
> Thanks for the testing.
>
I'll try to be quick about it, but I expect it will take a few days to
hit everything and explain it here.
>
> >> On a very simple overlay, I'm seeing this patch's warning catching
> >> things other than #address-cells or #size-cells.
>
> #address-cells and #size-cells escape the warning for properties on an
> existing (non-overlay) node if the existing node already contains them
> as a special case. Those two properties are needed in the overlay to
> avoid dtc compiler warnings. If the same properties already exist in
> the base devicetree and have the same values as in the overlay then
> there is no need to add property update changeset entries in the overlay
> changeset. Since there will not be changeset entries for those two
> properties, there will be no memory leak when the changeset is removed.
>
> The special casing of #address-cells and #size-cells is part of the
> fix patches that are a result of the validation patches. Thus a little
> bit less memory leaking than we have today.
Makes sense. Thanks for the explanation.
>
>
> > What it's warning about are new properties being added to an existing
> > node. So !prop is true and !of_node_check_flag(target->np,
> > OF_OVERLAY) also is true. Is that a potential memory leak as you are
> > warning? If so, your code is working as planned and you'll just need
> > to document that also in the header.
>
> Yes, you are accurately describing what the check is catching.
>
> The memory leak (on release) is because the memory allocated for overlay
> properties is released when the reference count of the node they are
> attached is decremented to zero, but only if the node is a dynamic flagged
> node (as overlays are). The memory allocated for the overlay properties
> will not be freed in this case because the node is not a dynamic node.
>
>
> >> I'm just getting
> >> started looking at this, will spend time understanding this better and
> >> I'll test other overlays. The warnings were:
> >>
> >> Applying dtbo: socfpga_overlay.dtb
> >> [ 33.117881] fpga_manager fpga0: writing soc_system.rbf to Altera
> >> SOCFPGA FPGA Manager
> >> [ 33.575223] OF: overlay: WARNING: add_changeset_property(), memory
> >> leak will occur if overlay removed. Property:
> >> /soc/base-fpga-region/firmware-name
> >> [ 33.588584] OF: overlay: WARNING: add_changeset_property(), memory
> >> leak will occur if overlay removed. Property:
> >> /soc/base-fpga-region/fpga-bridges
> >> [ 33.601856] OF: overlay: WARNING: add_changeset_property(), memory
> >> leak will occur if overlay removed. Property:
> >> /soc/base-fpga-region/ranges
>
> Are there properties in /soc/base-fpga-region/ in the base devicetree?
It looks like this:
base_fpga_region {
#address-cells = <0x1>;
#size-cells = <0x1>;
compatible = "fpga-region";
fpga-mgr = <&fpga_mgr>;
};
It is used as a target for applying DT overlays for FPGA reprogramming.
>
> If not, then that node could be removed from the base devicetree and first created
> in an overlay.
I'll give it a try and report back!
> If so, is it possible to add an additional level of node, /soc/base-fpga-region/foo,
> which would contain the properties that are warned about above? Then the properties
> would be children of an overlay node and the memory would be freed on overlay
> release.
I expect that would not be hard. The of-fpga-region.c driver would
only need small changes to know where to expect the properties it is
looking for.
And when the child nodes in the overlay are added to the live tree,
they would be under the added fake level. That shouldn't be a problem
either, it would just be different.
> This is not actually a suggestion that should be implemented right now, just trying
> to understand the possible alternatives, because this would result in an arbitrary
> fake level in the tree (which I don't like).
Yeah I don't love it either, but for the sake of discussion, it's
worth the mention. I'm open for suggestions here definitely. But yes
adding the fake level might make the DT code implementation more
straightforward at the cost of something that isn't great in the DT
itself. We can keep scratching our heads a bit.
>
> My intent is to leave these validation checks as warnings while we figure out the
> best way to solve the underlying memory leak issue.
That is helpful. Applying overlays may be a bit noisier for a while
until this gets ironed out.
> Note that some of the
> validation checks result in errors and cause an overlay apply to fail.
Yes, the errors I hit were where the overlay tried to change
#address-cells. I am thinking that we probably don't need to do that.
What we were doing was mapping multiple bridges (between the CPU and
FPGA) into different address spaces. We probably don't have to do it
that way. I'll try to bring in an example without making it
unnecessarily involved:
dts-v1/;
/plugin/;
/ {
fragment@0 {
target-path = "/soc/base_fpga_region";
#address-cells = <1>;
#size-cells = <1>;
__overlay__ {
ranges = <0x00000000 0x00000000 0xc0000000
0x00040000>, //h2f bridge
<0x00000001 0x00000000 0xff200000
0x00001000>; //lwh2f bridge
so on...
The h2f = hps (cpu) to fpga bridge = for DMA
The lwh2f = lightweight cpu to fpga = strongly ordered bridge for
register access
This will raise ERROR messages, but if I change it to the following,
it seems happier.
ranges = <0xc0000000 0xc0000000 0x00040000>,
<0x00000000 0xff200000 0x00001000>;
Maybe we should be using dma-ranges for part of this anyway.
> If I
> did those checks correctly, they should only catch cases where the live tree
> after applying the overlay was a "corrupt" tree instead of the desired changes.
>
> I expect that Plumbers will be a good place to explore these things.
>
>
> >> Here's part of that overlay including the properties it's complaining about:
> >>
> >> /dts-v1/;
> >> /plugin/;
> >> / {
> >> fragment@0 {
> >> target = <&base_fpga_region>;
> >> #address-cells = <1>;
> >> #size-cells = <1>;
> >> __overlay__ {
> >> #address-cells = <1>;
> >> #size-cells = <1>;
> >>
> >> firmware-name = "soc_system.rbf";
> >> fpga-bridges = <&fpga_bridge1>;
> >> ranges = <0x20000 0xff200000 0x100000>,
> >> <0x0 0xc0000000 0x20000000>;
> >>
> >> gpio@10040 {
> >> so on...
> >>
> >> By the way, I didn't get any warnings when I subsequently removed this overlay.
>
> Yes, I did not add any check that could catch this at release time.
>
> -Frank
>
>
> >> Alan
> >>
> >>>
> >>> If the properties are already in the live tree then there is no
> >>> need to create a changeset entry to add them since they must
> >>> have the same value. This reduces the memory used by the
> >>> changeset and eliminates a possible memory leak. This is
> >>> verified by 12 fewer warnings during the devicetree unittest,
> >>> as the possible memory leak warnings about #address-cells and
> >>>
> >>> Signed-off-by: Frank Rowand <[email protected]>
> >>> ---
> >>> drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> >>> 1 file changed, 35 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >>> index 29c33a5c533f..e6fb3ffe9d93 100644
> >>> --- a/drivers/of/overlay.c
> >>> +++ b/drivers/of/overlay.c
> >>> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> >>> * @target may be either in the live devicetree or in a new subtree that
> >>> * is contained in the changeset.
> >>> *
> >>> - * Some special properties are not updated (no error returned).
> >>> + * Some special properties are not added or updated (no error returned):
> >>> + * "name", "phandle", "linux,phandle".
> >>> + *
> >>> + * Properties "#address-cells" and "#size-cells" are not updated if they
> >>> + * are already in the live tree, but if present in the live tree, the values
> >>> + * in the overlay must match the values in the live tree.
> >>> *
> >>> * Update of property in symbols node is not allowed.
> >>> *
> >>> @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> >>> {
> >>> struct property *new_prop = NULL, *prop;
> >>> int ret = 0;
> >>> + bool check_for_non_overlay_node = false;
> >>>
> >>> if (!of_prop_cmp(overlay_prop->name, "name") ||
> >>> !of_prop_cmp(overlay_prop->name, "phandle") ||
> >>> @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> >>> if (!new_prop)
> >>> return -ENOMEM;
> >>>
> >>> - if (!prop)
> >>> + if (!prop) {
> >>> +
> >>> + check_for_non_overlay_node = true;
> >>> ret = of_changeset_add_property(&ovcs->cset, target->np,
> >>> new_prop);
> >>> - else
> >>> +
> >>> + } else if (!of_prop_cmp(prop->name, "#address-cells")) {
> >>> +
> >>> + if (prop->length != 4 || new_prop->length != 4 ||
> >>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
> >>> + pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
> >>> + target->np);
> >>> +
> >>> + } else if (!of_prop_cmp(prop->name, "#size-cells")) {
> >>> +
> >>> + if (prop->length != 4 || new_prop->length != 4 ||
> >>> + *(u32 *)prop->value != *(u32 *)new_prop->value)
> >>> + pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
> >>> + target->np);
> >>> +
> >>> + } else {
> >>> +
> >>> + check_for_non_overlay_node = true;
> >>> ret = of_changeset_update_property(&ovcs->cset, target->np,
> >>> new_prop);
> >>>
> >>> + }
> >>> +
> >>> + if (check_for_non_overlay_node &&
> >>> + !of_node_check_flag(target->np, OF_OVERLAY))
> >>> + pr_err("WARNING: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
> >>> + __func__, target->np, new_prop->name);
> >>> +
> >>> if (ret) {
> >>> kfree(new_prop->name);
> >>> kfree(new_prop->value);
> >>> --
> >>> Frank Rowand <[email protected]>
> >>>
> >
>
On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>
> From: Frank Rowand <[email protected]>
>
Hi Frank,
> The changeset entry 'update property' was used for new properties in
> an overlay instead of 'add property'.
>
> The decision of whether to use 'update property' was based on whether
> the property already exists in the subtree where the node is being
> spliced into. At the top level of creating a changeset describing the
> overlay, the target node is in the live devicetree, so checking whether
> the property exists in the target node returns the correct result.
> As soon as the changeset creation algorithm recurses into a new node,
> the target is no longer in the live devicetree, but is instead in the
> detached overlay tree, thus all properties are incorrectly found to
> already exist in the target.
When I applied an overlay (that added a few gpio controllers, etc),
the node names for nodes added from the overlay end up NULL. It
seems related to this patch and the next one. I haven't completely
root caused this but if I back out to before this patch, the situation
is fixed.
root@arria10:~/unit_tests# ls /sys/bus/platform/drivers/altera_gpio/
bind ff200010.<NULL> ff200020.<NULL> ff200030.<NULL>
uevent unbind
root@arria10:~/unit_tests# ls /sys/bus/platform/drivers/altera_freeze_br/
bind ff200450.<NULL> uevent unbind
Alan
>
> This fix will expose another devicetree bug that will be fixed
> in the following patch in the series.
>
> When this patch is applied the errors reported by the devictree
> unittest will change, and the unittest results will change from:
>
> ### dt-test ### end of unittest - 210 passed, 0 failed
>
> to
>
> ### dt-test ### end of unittest - 203 passed, 7 failed
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> drivers/of/overlay.c | 112 ++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 74 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 32cfee68f2e3..0b0904f44bc7 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -24,6 +24,26 @@
> #include "of_private.h"
>
> /**
> + * struct target - info about current target node as recursing through overlay
> + * @np: node where current level of overlay will be applied
> + * @in_livetree: @np is a node in the live devicetree
> + *
> + * Used in the algorithm to create the portion of a changeset that describes
> + * an overlay fragment, which is a devicetree subtree. Initially @np is a node
> + * in the live devicetree where the overlay subtree is targeted to be grafted
> + * into. When recursing to the next level of the overlay subtree, the target
> + * also recurses to the next level of the live devicetree, as long as overlay
> + * subtree node also exists in the live devicetree. When a node in the overlay
> + * subtree does not exist at the same level in the live devicetree, target->np
> + * points to a newly allocated node, and all subsequent targets in the subtree
> + * will be newly allocated nodes.
> + */
> +struct target {
> + struct device_node *np;
> + bool in_livetree;
> +};
> +
> +/**
> * struct fragment - info about fragment nodes in overlay expanded device tree
> * @target: target of the overlay operation
> * @overlay: pointer to the __overlay__ node
> @@ -72,8 +92,7 @@ static int devicetree_corrupt(void)
> }
>
> static int build_changeset_next_level(struct overlay_changeset *ovcs,
> - struct device_node *target_node,
> - const struct device_node *overlay_node);
> + struct target *target, const struct device_node *overlay_node);
>
> /*
> * of_resolve_phandles() finds the largest phandle in the live tree.
> @@ -257,14 +276,17 @@ static struct property *dup_and_fixup_symbol_prop(
> /**
> * add_changeset_property() - add @overlay_prop to overlay changeset
> * @ovcs: overlay changeset
> - * @target_node: where to place @overlay_prop in live tree
> + * @target: where @overlay_prop will be placed
> * @overlay_prop: property to add or update, from overlay tree
> * @is_symbols_prop: 1 if @overlay_prop is from node "/__symbols__"
> *
> - * If @overlay_prop does not already exist in @target_node, add changeset entry
> - * to add @overlay_prop in @target_node, else add changeset entry to update
> + * If @overlay_prop does not already exist in live devicetree, add changeset
> + * entry to add @overlay_prop in @target, else add changeset entry to update
> * value of @overlay_prop.
> *
> + * @target may be either in the live devicetree or in a new subtree that
> + * is contained in the changeset.
> + *
> * Some special properties are not updated (no error returned).
> *
> * Update of property in symbols node is not allowed.
> @@ -273,20 +295,22 @@ static struct property *dup_and_fixup_symbol_prop(
> * invalid @overlay.
> */
> static int add_changeset_property(struct overlay_changeset *ovcs,
> - struct device_node *target_node,
> - struct property *overlay_prop,
> + struct target *target, struct property *overlay_prop,
> bool is_symbols_prop)
> {
> struct property *new_prop = NULL, *prop;
> int ret = 0;
>
> - prop = of_find_property(target_node, overlay_prop->name, NULL);
> -
> if (!of_prop_cmp(overlay_prop->name, "name") ||
> !of_prop_cmp(overlay_prop->name, "phandle") ||
> !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> return 0;
>
> + if (target->in_livetree)
> + prop = of_find_property(target->np, overlay_prop->name, NULL);
> + else
> + prop = NULL;
> +
> if (is_symbols_prop) {
> if (prop)
> return -EINVAL;
> @@ -299,10 +323,10 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> return -ENOMEM;
>
> if (!prop)
> - ret = of_changeset_add_property(&ovcs->cset, target_node,
> + ret = of_changeset_add_property(&ovcs->cset, target->np,
> new_prop);
> else
> - ret = of_changeset_update_property(&ovcs->cset, target_node,
> + ret = of_changeset_update_property(&ovcs->cset, target->np,
> new_prop);
>
> if (ret) {
> @@ -315,14 +339,14 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>
> /**
> * add_changeset_node() - add @node (and children) to overlay changeset
> - * @ovcs: overlay changeset
> - * @target_node: where to place @node in live tree
> - * @node: node from within overlay device tree fragment
> + * @ovcs: overlay changeset
> + * @target: where @overlay_prop will be placed in live tree or changeset
> + * @node: node from within overlay device tree fragment
> *
> - * If @node does not already exist in @target_node, add changeset entry
> - * to add @node in @target_node.
> + * If @node does not already exist in @target, add changeset entry
> + * to add @node in @target.
> *
> - * If @node already exists in @target_node, and the existing node has
> + * If @node already exists in @target, and the existing node has
> * a phandle, the overlay node is not allowed to have a phandle.
> *
> * If @node has child nodes, add the children recursively via
> @@ -355,15 +379,16 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> * invalid @overlay.
> */
> static int add_changeset_node(struct overlay_changeset *ovcs,
> - struct device_node *target_node, struct device_node *node)
> + struct target *target, struct device_node *node)
> {
> const char *node_kbasename;
> struct device_node *tchild;
> + struct target target_child;
> int ret = 0;
>
> node_kbasename = kbasename(node->full_name);
>
> - for_each_child_of_node(target_node, tchild)
> + for_each_child_of_node(target->np, tchild)
> if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
> break;
>
> @@ -372,22 +397,28 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
> if (!tchild)
> return -ENOMEM;
>
> - tchild->parent = target_node;
> + tchild->parent = target->np;
> of_node_set_flag(tchild, OF_OVERLAY);
>
> ret = of_changeset_attach_node(&ovcs->cset, tchild);
> if (ret)
> return ret;
>
> - ret = build_changeset_next_level(ovcs, tchild, node);
> + target_child.np = tchild;
> + target_child.in_livetree = false;
> +
> + ret = build_changeset_next_level(ovcs, &target_child, node);
> of_node_put(tchild);
> return ret;
> }
>
> - if (node->phandle && tchild->phandle)
> + if (node->phandle && tchild->phandle) {
> ret = -EINVAL;
> - else
> - ret = build_changeset_next_level(ovcs, tchild, node);
> + } else {
> + target_child.np = tchild;
> + target_child.in_livetree = target->in_livetree;
> + ret = build_changeset_next_level(ovcs, &target_child, node);
> + }
> of_node_put(tchild);
>
> return ret;
> @@ -396,7 +427,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
> /**
> * build_changeset_next_level() - add level of overlay changeset
> * @ovcs: overlay changeset
> - * @target_node: where to place @overlay_node in live tree
> + * @target: where to place @overlay_node in live tree
> * @overlay_node: node from within an overlay device tree fragment
> *
> * Add the properties (if any) and nodes (if any) from @overlay_node to the
> @@ -409,27 +440,26 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
> * invalid @overlay_node.
> */
> static int build_changeset_next_level(struct overlay_changeset *ovcs,
> - struct device_node *target_node,
> - const struct device_node *overlay_node)
> + struct target *target, const struct device_node *overlay_node)
> {
> struct device_node *child;
> struct property *prop;
> int ret;
>
> for_each_property_of_node(overlay_node, prop) {
> - ret = add_changeset_property(ovcs, target_node, prop, 0);
> + ret = add_changeset_property(ovcs, target, prop, 0);
> if (ret) {
> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
> - target_node, prop->name, ret);
> + target->np, prop->name, ret);
> return ret;
> }
> }
>
> for_each_child_of_node(overlay_node, child) {
> - ret = add_changeset_node(ovcs, target_node, child);
> + ret = add_changeset_node(ovcs, target, child);
> if (ret) {
> pr_debug("Failed to apply node @%pOF/%s, err=%d\n",
> - target_node, child->name, ret);
> + target->np, child->name, ret);
> of_node_put(child);
> return ret;
> }
> @@ -442,17 +472,17 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> * Add the properties from __overlay__ node to the @ovcs->cset changeset.
> */
> static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
> - struct device_node *target_node,
> + struct target *target,
> const struct device_node *overlay_symbols_node)
> {
> struct property *prop;
> int ret;
>
> for_each_property_of_node(overlay_symbols_node, prop) {
> - ret = add_changeset_property(ovcs, target_node, prop, 1);
> + ret = add_changeset_property(ovcs, target, prop, 1);
> if (ret) {
> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
> - target_node, prop->name, ret);
> + target->np, prop->name, ret);
> return ret;
> }
> }
> @@ -475,6 +505,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
> static int build_changeset(struct overlay_changeset *ovcs)
> {
> struct fragment *fragment;
> + struct target target;
> int fragments_count, i, ret;
>
> /*
> @@ -489,7 +520,9 @@ static int build_changeset(struct overlay_changeset *ovcs)
> for (i = 0; i < fragments_count; i++) {
> fragment = &ovcs->fragments[i];
>
> - ret = build_changeset_next_level(ovcs, fragment->target,
> + target.np = fragment->target;
> + target.in_livetree = true;
> + ret = build_changeset_next_level(ovcs, &target,
> fragment->overlay);
> if (ret) {
> pr_debug("apply failed '%pOF'\n", fragment->target);
> @@ -499,7 +532,10 @@ static int build_changeset(struct overlay_changeset *ovcs)
>
> if (ovcs->symbols_fragment) {
> fragment = &ovcs->fragments[ovcs->count - 1];
> - ret = build_changeset_symbols_node(ovcs, fragment->target,
> +
> + target.np = fragment->target;
> + target.in_livetree = true;
> + ret = build_changeset_symbols_node(ovcs, &target,
> fragment->overlay);
> if (ret) {
> pr_debug("apply failed '%pOF'\n", fragment->target);
> @@ -517,7 +553,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
> * 1) "target" property containing the phandle of the target
> * 2) "target-path" property containing the path of the target
> */
> -static struct device_node *find_target_node(struct device_node *info_node)
> +static struct device_node *find_target(struct device_node *info_node)
> {
> struct device_node *node;
> const char *path;
> @@ -623,7 +659,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>
> fragment = &fragments[cnt];
> fragment->overlay = overlay_node;
> - fragment->target = find_target_node(node);
> + fragment->target = find_target(node);
> if (!fragment->target) {
> of_node_put(fragment->overlay);
> ret = -EINVAL;
> --
> Frank Rowand <[email protected]>
>
On 10/09/18 13:28, Alan Tull wrote:
> On Thu, Oct 4, 2018 at 11:14 PM <[email protected]> wrote:
>>
>> From: Frank Rowand <[email protected]>
>>
>
> Hi Frank,
>
>> The changeset entry 'update property' was used for new properties in
>> an overlay instead of 'add property'.
>>
>> The decision of whether to use 'update property' was based on whether
>> the property already exists in the subtree where the node is being
>> spliced into. At the top level of creating a changeset describing the
>> overlay, the target node is in the live devicetree, so checking whether
>> the property exists in the target node returns the correct result.
>> As soon as the changeset creation algorithm recurses into a new node,
>> the target is no longer in the live devicetree, but is instead in the
>> detached overlay tree, thus all properties are incorrectly found to
>> already exist in the target.
>
> When I applied an overlay (that added a few gpio controllers, etc),
> the node names for nodes added from the overlay end up NULL. It
I'll look at this tonight.
-Frank
> seems related to this patch and the next one. I haven't completely
> root caused this but if I back out to before this patch, the situation
> is fixed.
>
> root@arria10:~/unit_tests# ls /sys/bus/platform/drivers/altera_gpio/
> bind ff200010.<NULL> ff200020.<NULL> ff200030.<NULL>
> uevent unbind
>
> root@arria10:~/unit_tests# ls /sys/bus/platform/drivers/altera_freeze_br/
> bind ff200450.<NULL> uevent unbind
>
> Alan
>
>>
>> This fix will expose another devicetree bug that will be fixed
>> in the following patch in the series.
>>
>> When this patch is applied the errors reported by the devictree
>> unittest will change, and the unittest results will change from:
>>
>> ### dt-test ### end of unittest - 210 passed, 0 failed
>>
>> to
>>
>> ### dt-test ### end of unittest - 203 passed, 7 failed
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> drivers/of/overlay.c | 112 ++++++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 74 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 32cfee68f2e3..0b0904f44bc7 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -24,6 +24,26 @@
>> #include "of_private.h"
>>
>> /**
>> + * struct target - info about current target node as recursing through overlay
>> + * @np: node where current level of overlay will be applied
>> + * @in_livetree: @np is a node in the live devicetree
>> + *
>> + * Used in the algorithm to create the portion of a changeset that describes
>> + * an overlay fragment, which is a devicetree subtree. Initially @np is a node
>> + * in the live devicetree where the overlay subtree is targeted to be grafted
>> + * into. When recursing to the next level of the overlay subtree, the target
>> + * also recurses to the next level of the live devicetree, as long as overlay
>> + * subtree node also exists in the live devicetree. When a node in the overlay
>> + * subtree does not exist at the same level in the live devicetree, target->np
>> + * points to a newly allocated node, and all subsequent targets in the subtree
>> + * will be newly allocated nodes.
>> + */
>> +struct target {
>> + struct device_node *np;
>> + bool in_livetree;
>> +};
>> +
>> +/**
>> * struct fragment - info about fragment nodes in overlay expanded device tree
>> * @target: target of the overlay operation
>> * @overlay: pointer to the __overlay__ node
>> @@ -72,8 +92,7 @@ static int devicetree_corrupt(void)
>> }
>>
>> static int build_changeset_next_level(struct overlay_changeset *ovcs,
>> - struct device_node *target_node,
>> - const struct device_node *overlay_node);
>> + struct target *target, const struct device_node *overlay_node);
>>
>> /*
>> * of_resolve_phandles() finds the largest phandle in the live tree.
>> @@ -257,14 +276,17 @@ static struct property *dup_and_fixup_symbol_prop(
>> /**
>> * add_changeset_property() - add @overlay_prop to overlay changeset
>> * @ovcs: overlay changeset
>> - * @target_node: where to place @overlay_prop in live tree
>> + * @target: where @overlay_prop will be placed
>> * @overlay_prop: property to add or update, from overlay tree
>> * @is_symbols_prop: 1 if @overlay_prop is from node "/__symbols__"
>> *
>> - * If @overlay_prop does not already exist in @target_node, add changeset entry
>> - * to add @overlay_prop in @target_node, else add changeset entry to update
>> + * If @overlay_prop does not already exist in live devicetree, add changeset
>> + * entry to add @overlay_prop in @target, else add changeset entry to update
>> * value of @overlay_prop.
>> *
>> + * @target may be either in the live devicetree or in a new subtree that
>> + * is contained in the changeset.
>> + *
>> * Some special properties are not updated (no error returned).
>> *
>> * Update of property in symbols node is not allowed.
>> @@ -273,20 +295,22 @@ static struct property *dup_and_fixup_symbol_prop(
>> * invalid @overlay.
>> */
>> static int add_changeset_property(struct overlay_changeset *ovcs,
>> - struct device_node *target_node,
>> - struct property *overlay_prop,
>> + struct target *target, struct property *overlay_prop,
>> bool is_symbols_prop)
>> {
>> struct property *new_prop = NULL, *prop;
>> int ret = 0;
>>
>> - prop = of_find_property(target_node, overlay_prop->name, NULL);
>> -
>> if (!of_prop_cmp(overlay_prop->name, "name") ||
>> !of_prop_cmp(overlay_prop->name, "phandle") ||
>> !of_prop_cmp(overlay_prop->name, "linux,phandle"))
>> return 0;
>>
>> + if (target->in_livetree)
>> + prop = of_find_property(target->np, overlay_prop->name, NULL);
>> + else
>> + prop = NULL;
>> +
>> if (is_symbols_prop) {
>> if (prop)
>> return -EINVAL;
>> @@ -299,10 +323,10 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>> return -ENOMEM;
>>
>> if (!prop)
>> - ret = of_changeset_add_property(&ovcs->cset, target_node,
>> + ret = of_changeset_add_property(&ovcs->cset, target->np,
>> new_prop);
>> else
>> - ret = of_changeset_update_property(&ovcs->cset, target_node,
>> + ret = of_changeset_update_property(&ovcs->cset, target->np,
>> new_prop);
>>
>> if (ret) {
>> @@ -315,14 +339,14 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>
>> /**
>> * add_changeset_node() - add @node (and children) to overlay changeset
>> - * @ovcs: overlay changeset
>> - * @target_node: where to place @node in live tree
>> - * @node: node from within overlay device tree fragment
>> + * @ovcs: overlay changeset
>> + * @target: where @overlay_prop will be placed in live tree or changeset
>> + * @node: node from within overlay device tree fragment
>> *
>> - * If @node does not already exist in @target_node, add changeset entry
>> - * to add @node in @target_node.
>> + * If @node does not already exist in @target, add changeset entry
>> + * to add @node in @target.
>> *
>> - * If @node already exists in @target_node, and the existing node has
>> + * If @node already exists in @target, and the existing node has
>> * a phandle, the overlay node is not allowed to have a phandle.
>> *
>> * If @node has child nodes, add the children recursively via
>> @@ -355,15 +379,16 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>> * invalid @overlay.
>> */
>> static int add_changeset_node(struct overlay_changeset *ovcs,
>> - struct device_node *target_node, struct device_node *node)
>> + struct target *target, struct device_node *node)
>> {
>> const char *node_kbasename;
>> struct device_node *tchild;
>> + struct target target_child;
>> int ret = 0;
>>
>> node_kbasename = kbasename(node->full_name);
>>
>> - for_each_child_of_node(target_node, tchild)
>> + for_each_child_of_node(target->np, tchild)
>> if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
>> break;
>>
>> @@ -372,22 +397,28 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>> if (!tchild)
>> return -ENOMEM;
>>
>> - tchild->parent = target_node;
>> + tchild->parent = target->np;
>> of_node_set_flag(tchild, OF_OVERLAY);
>>
>> ret = of_changeset_attach_node(&ovcs->cset, tchild);
>> if (ret)
>> return ret;
>>
>> - ret = build_changeset_next_level(ovcs, tchild, node);
>> + target_child.np = tchild;
>> + target_child.in_livetree = false;
>> +
>> + ret = build_changeset_next_level(ovcs, &target_child, node);
>> of_node_put(tchild);
>> return ret;
>> }
>>
>> - if (node->phandle && tchild->phandle)
>> + if (node->phandle && tchild->phandle) {
>> ret = -EINVAL;
>> - else
>> - ret = build_changeset_next_level(ovcs, tchild, node);
>> + } else {
>> + target_child.np = tchild;
>> + target_child.in_livetree = target->in_livetree;
>> + ret = build_changeset_next_level(ovcs, &target_child, node);
>> + }
>> of_node_put(tchild);
>>
>> return ret;
>> @@ -396,7 +427,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>> /**
>> * build_changeset_next_level() - add level of overlay changeset
>> * @ovcs: overlay changeset
>> - * @target_node: where to place @overlay_node in live tree
>> + * @target: where to place @overlay_node in live tree
>> * @overlay_node: node from within an overlay device tree fragment
>> *
>> * Add the properties (if any) and nodes (if any) from @overlay_node to the
>> @@ -409,27 +440,26 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>> * invalid @overlay_node.
>> */
>> static int build_changeset_next_level(struct overlay_changeset *ovcs,
>> - struct device_node *target_node,
>> - const struct device_node *overlay_node)
>> + struct target *target, const struct device_node *overlay_node)
>> {
>> struct device_node *child;
>> struct property *prop;
>> int ret;
>>
>> for_each_property_of_node(overlay_node, prop) {
>> - ret = add_changeset_property(ovcs, target_node, prop, 0);
>> + ret = add_changeset_property(ovcs, target, prop, 0);
>> if (ret) {
>> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
>> - target_node, prop->name, ret);
>> + target->np, prop->name, ret);
>> return ret;
>> }
>> }
>>
>> for_each_child_of_node(overlay_node, child) {
>> - ret = add_changeset_node(ovcs, target_node, child);
>> + ret = add_changeset_node(ovcs, target, child);
>> if (ret) {
>> pr_debug("Failed to apply node @%pOF/%s, err=%d\n",
>> - target_node, child->name, ret);
>> + target->np, child->name, ret);
>> of_node_put(child);
>> return ret;
>> }
>> @@ -442,17 +472,17 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>> * Add the properties from __overlay__ node to the @ovcs->cset changeset.
>> */
>> static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
>> - struct device_node *target_node,
>> + struct target *target,
>> const struct device_node *overlay_symbols_node)
>> {
>> struct property *prop;
>> int ret;
>>
>> for_each_property_of_node(overlay_symbols_node, prop) {
>> - ret = add_changeset_property(ovcs, target_node, prop, 1);
>> + ret = add_changeset_property(ovcs, target, prop, 1);
>> if (ret) {
>> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
>> - target_node, prop->name, ret);
>> + target->np, prop->name, ret);
>> return ret;
>> }
>> }
>> @@ -475,6 +505,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
>> static int build_changeset(struct overlay_changeset *ovcs)
>> {
>> struct fragment *fragment;
>> + struct target target;
>> int fragments_count, i, ret;
>>
>> /*
>> @@ -489,7 +520,9 @@ static int build_changeset(struct overlay_changeset *ovcs)
>> for (i = 0; i < fragments_count; i++) {
>> fragment = &ovcs->fragments[i];
>>
>> - ret = build_changeset_next_level(ovcs, fragment->target,
>> + target.np = fragment->target;
>> + target.in_livetree = true;
>> + ret = build_changeset_next_level(ovcs, &target,
>> fragment->overlay);
>> if (ret) {
>> pr_debug("apply failed '%pOF'\n", fragment->target);
>> @@ -499,7 +532,10 @@ static int build_changeset(struct overlay_changeset *ovcs)
>>
>> if (ovcs->symbols_fragment) {
>> fragment = &ovcs->fragments[ovcs->count - 1];
>> - ret = build_changeset_symbols_node(ovcs, fragment->target,
>> +
>> + target.np = fragment->target;
>> + target.in_livetree = true;
>> + ret = build_changeset_symbols_node(ovcs, &target,
>> fragment->overlay);
>> if (ret) {
>> pr_debug("apply failed '%pOF'\n", fragment->target);
>> @@ -517,7 +553,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
>> * 1) "target" property containing the phandle of the target
>> * 2) "target-path" property containing the path of the target
>> */
>> -static struct device_node *find_target_node(struct device_node *info_node)
>> +static struct device_node *find_target(struct device_node *info_node)
>> {
>> struct device_node *node;
>> const char *path;
>> @@ -623,7 +659,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>
>> fragment = &fragments[cnt];
>> fragment->overlay = overlay_node;
>> - fragment->target = find_target_node(node);
>> + fragment->target = find_target(node);
>> if (!fragment->target) {
>> of_node_put(fragment->overlay);
>> ret = -EINVAL;
>> --
>> Frank Rowand <[email protected]>
>>
>
From: Frank Rowand <[email protected]>
"of: overlay: use prop add changeset entry for property in new nodes"
fixed a problem where an 'update property' changeset entry was
created for properties contained in nodes added by a changeset.
The fix was to use an 'add property' changeset entry.
This exposed more bugs in the apply overlay code. The properties
'name', 'phandle', and 'linux,phandle' were filtered out by
add_changeset_property() as special properties. Change the filter
to be only for existing nodes, not newly added nodes.
The second bug is that the 'name' property does not exist in the
newest FDT version, and has to be constructed from the node's
full_name. Construct an 'add property' changeset entry for
newly added nodes.
Signed-off-by: Frank Rowand <[email protected]>
---
Hi Alan,
Thanks for reporting the problem with missing node names.
I was able to replicate the problem, and have created this preliminary
version of a patch to fix the problem.
I have not extensively reviewed the patch yet, but would appreciate
if you can confirm this fixes your problem.
I created this patch as patch 17 of the series, but have also
applied it as patch 05.1, immediately after patch 05/16, and
built the kernel, booted, and verified name and phandle for
one of the nodes in a unittest overlay for both cases. So
minimal testing so far on my part.
I have not verified whether the series builds and boots after
each of patches 06..16 if this patch is applied as patch 05.1.
There is definitely more work needed for me to complete this
patch because it allocates some more memory, but does not yet
free it when the overlay is released.
-Frank
drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 67 insertions(+), 5 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 0b0904f44bc7..9746cea2aa91 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
struct property *new_prop = NULL, *prop;
int ret = 0;
- if (!of_prop_cmp(overlay_prop->name, "name") ||
- !of_prop_cmp(overlay_prop->name, "phandle") ||
- !of_prop_cmp(overlay_prop->name, "linux,phandle"))
- return 0;
+ if (target->in_livetree)
+ if (!of_prop_cmp(overlay_prop->name, "name") ||
+ !of_prop_cmp(overlay_prop->name, "phandle") ||
+ !of_prop_cmp(overlay_prop->name, "linux,phandle"))
+ return 0;
if (target->in_livetree)
prop = of_find_property(target->np, overlay_prop->name, NULL);
@@ -443,10 +444,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
struct target *target, const struct device_node *overlay_node)
{
struct device_node *child;
- struct property *prop;
+ struct property *prop, *name_prop;
+ bool has_name = false;
int ret;
for_each_property_of_node(overlay_node, prop) {
+ if (!strcmp(prop->name, "name"))
+ has_name = true;
ret = add_changeset_property(ovcs, target, prop, 0);
if (ret) {
pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
@@ -455,6 +459,57 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
}
}
+ /*
+ * With FDT version 0x10 we may not have the name property,
+ * recreate it here from the unit name if absent
+ */
+
+ if (!has_name) {
+ const char *p = target->np->full_name, *ps = p, *pa = NULL;
+ int len;
+
+ /*
+ * zzz
+ * TODO: stash name_prop on a list in ovcs, to be freed
+ * after overlay removed
+ */
+
+ while (*p) {
+ if ((*p) == '@')
+ pa = p;
+ else if ((*p) == '/')
+ ps = p + 1;
+ p++;
+ }
+
+ if (pa < ps)
+ pa = p;
+ len = (pa - ps) + 1;
+
+ name_prop = kmalloc(sizeof(*name_prop), GFP_KERNEL);
+ if (!name_prop)
+ return -ENOMEM;
+
+ name_prop->name = kstrdup("name", GFP_KERNEL);
+ name_prop->value = kmalloc(len, GFP_KERNEL);
+ if (!name_prop->name || !name_prop->value) {
+ ret = -ENOMEM;
+ goto err_free_name_prop;
+ }
+
+ memcpy(name_prop->value, ps, len - 1);
+ ((char *)name_prop->value)[len - 1] = 0;
+
+ name_prop->length = strlen(name_prop->value) + 1;
+
+ ret = add_changeset_property(ovcs, target, name_prop, 0);
+ if (ret) {
+ pr_debug("Failed to apply name_prop @%pOF/%s, err=%d\n",
+ target->np, name_prop->name, ret);
+ goto err_free_name_prop;
+ }
+ }
+
for_each_child_of_node(overlay_node, child) {
ret = add_changeset_node(ovcs, target, child);
if (ret) {
@@ -466,6 +521,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
}
return 0;
+
+err_free_name_prop:
+ kfree(name_prop->name);
+ kfree(name_prop->value);
+ kfree(name_prop);
+ return ret;
+
}
/*
--
Frank Rowand <[email protected]>
On 10/09/18 23:04, [email protected] wrote:
> From: Frank Rowand <[email protected]>
>
>
> "of: overlay: use prop add changeset entry for property in new nodes"
> fixed a problem where an 'update property' changeset entry was
> created for properties contained in nodes added by a changeset.
> The fix was to use an 'add property' changeset entry.
>
> This exposed more bugs in the apply overlay code. The properties
> 'name', 'phandle', and 'linux,phandle' were filtered out by
> add_changeset_property() as special properties. Change the filter
> to be only for existing nodes, not newly added nodes.
>
> The second bug is that the 'name' property does not exist in the
> newest FDT version, and has to be constructed from the node's
> full_name. Construct an 'add property' changeset entry for
> newly added nodes.
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
>
>
> Hi Alan,
>
> Thanks for reporting the problem with missing node names.
>
> I was able to replicate the problem, and have created this preliminary
> version of a patch to fix the problem.
>
> I have not extensively reviewed the patch yet, but would appreciate
> if you can confirm this fixes your problem.
>
> I created this patch as patch 17 of the series, but have also
> applied it as patch 05.1, immediately after patch 05/16, and
> built the kernel, booted, and verified name and phandle for
> one of the nodes in a unittest overlay for both cases. So
> minimal testing so far on my part.
>
> I have not verified whether the series builds and boots after
> each of patches 06..16 if this patch is applied as patch 05.1.
>
> There is definitely more work needed for me to complete this
> patch because it allocates some more memory, but does not yet
> free it when the overlay is released.
>
> -Frank
>
>
> drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 0b0904f44bc7..9746cea2aa91 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> struct property *new_prop = NULL, *prop;
> int ret = 0;
>
> - if (!of_prop_cmp(overlay_prop->name, "name") ||
> - !of_prop_cmp(overlay_prop->name, "phandle") ||
> - !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> - return 0;
> + if (target->in_livetree)
> + if (!of_prop_cmp(overlay_prop->name, "name") ||
> + !of_prop_cmp(overlay_prop->name, "phandle") ||
> + !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> + return 0;
This is a big hammer patch.
Nobody should waste time reviewing this patch.
The following part should not be needed (though the above section might have
to become _slightly_ more complex).
-Frank
>
> if (target->in_livetree)
> prop = of_find_property(target->np, overlay_prop->name, NULL);
> @@ -443,10 +444,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> struct target *target, const struct device_node *overlay_node)
> {
> struct device_node *child;
> - struct property *prop;
> + struct property *prop, *name_prop;
> + bool has_name = false;
> int ret;
>
> for_each_property_of_node(overlay_node, prop) {
> + if (!strcmp(prop->name, "name"))
> + has_name = true;
> ret = add_changeset_property(ovcs, target, prop, 0);
> if (ret) {
> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
> @@ -455,6 +459,57 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> }
> }
>
> + /*
> + * With FDT version 0x10 we may not have the name property,
> + * recreate it here from the unit name if absent
> + */
> +
> + if (!has_name) {
> + const char *p = target->np->full_name, *ps = p, *pa = NULL;
> + int len;
> +
> + /*
> + * zzz
> + * TODO: stash name_prop on a list in ovcs, to be freed
> + * after overlay removed
> + */
> +
> + while (*p) {
> + if ((*p) == '@')
> + pa = p;
> + else if ((*p) == '/')
> + ps = p + 1;
> + p++;
> + }
> +
> + if (pa < ps)
> + pa = p;
> + len = (pa - ps) + 1;
> +
> + name_prop = kmalloc(sizeof(*name_prop), GFP_KERNEL);
> + if (!name_prop)
> + return -ENOMEM;
> +
> + name_prop->name = kstrdup("name", GFP_KERNEL);
> + name_prop->value = kmalloc(len, GFP_KERNEL);
> + if (!name_prop->name || !name_prop->value) {
> + ret = -ENOMEM;
> + goto err_free_name_prop;
> + }
> +
> + memcpy(name_prop->value, ps, len - 1);
> + ((char *)name_prop->value)[len - 1] = 0;
> +
> + name_prop->length = strlen(name_prop->value) + 1;
> +
> + ret = add_changeset_property(ovcs, target, name_prop, 0);
> + if (ret) {
> + pr_debug("Failed to apply name_prop @%pOF/%s, err=%d\n",
> + target->np, name_prop->name, ret);
> + goto err_free_name_prop;
> + }
> + }
> +
> for_each_child_of_node(overlay_node, child) {
> ret = add_changeset_node(ovcs, target, child);
> if (ret) {
> @@ -466,6 +521,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> }
>
> return 0;
> +
> +err_free_name_prop:
> + kfree(name_prop->name);
> + kfree(name_prop->value);
> + kfree(name_prop);
> + return ret;
> +
> }
>
> /*
>
On Wed, Oct 10, 2018 at 1:49 AM Frank Rowand <[email protected]> wrote:
>
> On 10/09/18 23:04, [email protected] wrote:
> > From: Frank Rowand <[email protected]>
> >
> >
> > "of: overlay: use prop add changeset entry for property in new nodes"
> > fixed a problem where an 'update property' changeset entry was
> > created for properties contained in nodes added by a changeset.
> > The fix was to use an 'add property' changeset entry.
> >
> > This exposed more bugs in the apply overlay code. The properties
> > 'name', 'phandle', and 'linux,phandle' were filtered out by
> > add_changeset_property() as special properties. Change the filter
> > to be only for existing nodes, not newly added nodes.
> >
> > The second bug is that the 'name' property does not exist in the
> > newest FDT version, and has to be constructed from the node's
> > full_name. Construct an 'add property' changeset entry for
> > newly added nodes.
> >
> > Signed-off-by: Frank Rowand <[email protected]>
> > ---
> >
> >
> > Hi Alan,
> >
> > Thanks for reporting the problem with missing node names.
> >
> > I was able to replicate the problem, and have created this preliminary
> > version of a patch to fix the problem.
> >
> > I have not extensively reviewed the patch yet, but would appreciate
> > if you can confirm this fixes your problem.
> >
> > I created this patch as patch 17 of the series, but have also
> > applied it as patch 05.1, immediately after patch 05/16, and
> > built the kernel, booted, and verified name and phandle for
> > one of the nodes in a unittest overlay for both cases. So
> > minimal testing so far on my part.
> >
> > I have not verified whether the series builds and boots after
> > each of patches 06..16 if this patch is applied as patch 05.1.
> >
> > There is definitely more work needed for me to complete this
> > patch because it allocates some more memory, but does not yet
> > free it when the overlay is released.
> >
> > -Frank
> >
> >
> > drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 67 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 0b0904f44bc7..9746cea2aa91 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> > struct property *new_prop = NULL, *prop;
> > int ret = 0;
> >
> > - if (!of_prop_cmp(overlay_prop->name, "name") ||
> > - !of_prop_cmp(overlay_prop->name, "phandle") ||
> > - !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> > - return 0;
> > + if (target->in_livetree)
> > + if (!of_prop_cmp(overlay_prop->name, "name") ||
> > + !of_prop_cmp(overlay_prop->name, "phandle") ||
> > + !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> > + return 0;
>
> This is a big hammer patch.
>
> Nobody should waste time reviewing this patch.
I wasn't clear if you still could use the testing so I did re-run my
test. This patch adds back some of the missing properties, but the
the kobject names aren't set as dev_name() returns NULL:
* without this patch some of_node properties don't show up in sysfs:
root@arria10:~# ls
/sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
clocks compatible interrupt-parent interrupts reg
* with this patch, the of_node properties phandle and name are back:
root@arria10:~# ls
/sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
clocks compatible interrupt-parent interrupts
name phandle reg
root@arria10:~# cat
/sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node/name
freeze_controllerroot@arria10:~# ("freeze_controller" w/o the \n so
the name is correct)
* with or without the patch I see the behavior I reported yesterday,
kobj names are NULL.
root@arria10:~# ls /sys/bus/platform/drivers/altera_freeze_br/
bind ff200450.<NULL> uevent unbind
root@arria10:~# ls /sys/bus/platform/drivers/altera_gpio/
bind ff200010.<NULL> ff200020.<NULL> ff200030.<NULL>
uevent unbind
Alan
Alan
>
> The following part should not be needed (though the above section might have
> to become _slightly_ more complex).
>
> -Frank
> >
> > if (target->in_livetree)
> > prop = of_find_property(target->np, overlay_prop->name, NULL);
> > @@ -443,10 +444,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> > struct target *target, const struct device_node *overlay_node)
> > {
> > struct device_node *child;
> > - struct property *prop;
> > + struct property *prop, *name_prop;
> > + bool has_name = false;
> > int ret;
> >
> > for_each_property_of_node(overlay_node, prop) {
> > + if (!strcmp(prop->name, "name"))
> > + has_name = true;
> > ret = add_changeset_property(ovcs, target, prop, 0);
> > if (ret) {
> > pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
> > @@ -455,6 +459,57 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> > }
> > }
> >
> > + /*
> > + * With FDT version 0x10 we may not have the name property,
> > + * recreate it here from the unit name if absent
> > + */
> > +
> > + if (!has_name) {
> > + const char *p = target->np->full_name, *ps = p, *pa = NULL;
> > + int len;
> > +
> > + /*
> > + * zzz
> > + * TODO: stash name_prop on a list in ovcs, to be freed
> > + * after overlay removed
> > + */
> > +
> > + while (*p) {
> > + if ((*p) == '@')
> > + pa = p;
> > + else if ((*p) == '/')
> > + ps = p + 1;
> > + p++;
> > + }
> > +
> > + if (pa < ps)
> > + pa = p;
> > + len = (pa - ps) + 1;
> > +
> > + name_prop = kmalloc(sizeof(*name_prop), GFP_KERNEL);
> > + if (!name_prop)
> > + return -ENOMEM;
> > +
> > + name_prop->name = kstrdup("name", GFP_KERNEL);
> > + name_prop->value = kmalloc(len, GFP_KERNEL);
> > + if (!name_prop->name || !name_prop->value) {
> > + ret = -ENOMEM;
> > + goto err_free_name_prop;
> > + }
> > +
> > + memcpy(name_prop->value, ps, len - 1);
> > + ((char *)name_prop->value)[len - 1] = 0;
> > +
> > + name_prop->length = strlen(name_prop->value) + 1;
> > +
> > + ret = add_changeset_property(ovcs, target, name_prop, 0);
> > + if (ret) {
> > + pr_debug("Failed to apply name_prop @%pOF/%s, err=%d\n",
> > + target->np, name_prop->name, ret);
> > + goto err_free_name_prop;
> > + }
> > + }
> > +
> > for_each_child_of_node(overlay_node, child) {
> > ret = add_changeset_node(ovcs, target, child);
> > if (ret) {
> > @@ -466,6 +521,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> > }
> >
> > return 0;
> > +
> > +err_free_name_prop:
> > + kfree(name_prop->name);
> > + kfree(name_prop->value);
> > + kfree(name_prop);
> > + return ret;
> > +
> > }
> >
> > /*
> >
>
On 10/10/18 13:40, Alan Tull wrote:
> On Wed, Oct 10, 2018 at 1:49 AM Frank Rowand <[email protected]> wrote:
>>
>> On 10/09/18 23:04, [email protected] wrote:
>>> From: Frank Rowand <[email protected]>
>>>
>>>
>>> "of: overlay: use prop add changeset entry for property in new nodes"
>>> fixed a problem where an 'update property' changeset entry was
>>> created for properties contained in nodes added by a changeset.
>>> The fix was to use an 'add property' changeset entry.
>>>
>>> This exposed more bugs in the apply overlay code. The properties
>>> 'name', 'phandle', and 'linux,phandle' were filtered out by
>>> add_changeset_property() as special properties. Change the filter
>>> to be only for existing nodes, not newly added nodes.
>>>
>>> The second bug is that the 'name' property does not exist in the
>>> newest FDT version, and has to be constructed from the node's
>>> full_name. Construct an 'add property' changeset entry for
>>> newly added nodes.
>>>
>>> Signed-off-by: Frank Rowand <[email protected]>
>>> ---
>>>
>>>
>>> Hi Alan,
>>>
>>> Thanks for reporting the problem with missing node names.
>>>
>>> I was able to replicate the problem, and have created this preliminary
>>> version of a patch to fix the problem.
>>>
>>> I have not extensively reviewed the patch yet, but would appreciate
>>> if you can confirm this fixes your problem.
>>>
>>> I created this patch as patch 17 of the series, but have also
>>> applied it as patch 05.1, immediately after patch 05/16, and
>>> built the kernel, booted, and verified name and phandle for
>>> one of the nodes in a unittest overlay for both cases. So
>>> minimal testing so far on my part.
>>>
>>> I have not verified whether the series builds and boots after
>>> each of patches 06..16 if this patch is applied as patch 05.1.
>>>
>>> There is definitely more work needed for me to complete this
>>> patch because it allocates some more memory, but does not yet
>>> free it when the overlay is released.
>>>
>>> -Frank
>>>
>>>
>>> drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 67 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 0b0904f44bc7..9746cea2aa91 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>> struct property *new_prop = NULL, *prop;
>>> int ret = 0;
>>>
>>> - if (!of_prop_cmp(overlay_prop->name, "name") ||
>>> - !of_prop_cmp(overlay_prop->name, "phandle") ||
>>> - !of_prop_cmp(overlay_prop->name, "linux,phandle"))
>>> - return 0;
>>> + if (target->in_livetree)
>>> + if (!of_prop_cmp(overlay_prop->name, "name") ||
>>> + !of_prop_cmp(overlay_prop->name, "phandle") ||
>>> + !of_prop_cmp(overlay_prop->name, "linux,phandle"))
>>> + return 0;
>>
>> This is a big hammer patch.
>>
>> Nobody should waste time reviewing this patch.
>
> I wasn't clear if you still could use the testing so I did re-run my
> test. This patch adds back some of the missing properties, but the
> the kobject names aren't set as dev_name() returns NULL:
>
> * without this patch some of_node properties don't show up in sysfs:
> root@arria10:~# ls
> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
> clocks compatible interrupt-parent interrupts reg
>
> * with this patch, the of_node properties phandle and name are back:
> root@arria10:~# ls
> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
> clocks compatible interrupt-parent interrupts
> name phandle reg
Thanks for the testing. I'll keep chasing after this problem today.
This is useful data for me as I was not looking under the /sys/bus/...
tree that you reported, but was instead looking at /proc/device-tree/...
which showed the same type of problem since the overlay I was using
does not show up under /sys/bus/...
I'll have to create a useful overlay test case that will show up under
/sys/bus/...
In the meantime, can you send me the base FDT and the overlay FDT for
your test case?
Thanks,
Frank
>
> root@arria10:~# cat
> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node/name
> freeze_controllerroot@arria10:~# ("freeze_controller" w/o the \n so
> the name is correct)
>
> * with or without the patch I see the behavior I reported yesterday,
> kobj names are NULL.
> root@arria10:~# ls /sys/bus/platform/drivers/altera_freeze_br/
> bind ff200450.<NULL> uevent unbind
>
> root@arria10:~# ls /sys/bus/platform/drivers/altera_gpio/
> bind ff200010.<NULL> ff200020.<NULL> ff200030.<NULL>
> uevent unbind
>
> Alan
>
> Alan
>
>>
>> The following part should not be needed (though the above section might have
>> to become _slightly_ more complex).
>>
>> -Frank
>>>
>>> if (target->in_livetree)
>>> prop = of_find_property(target->np, overlay_prop->name, NULL);
>>> @@ -443,10 +444,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>> struct target *target, const struct device_node *overlay_node)
>>> {
>>> struct device_node *child;
>>> - struct property *prop;
>>> + struct property *prop, *name_prop;
>>> + bool has_name = false;
>>> int ret;
>>>
>>> for_each_property_of_node(overlay_node, prop) {
>>> + if (!strcmp(prop->name, "name"))
>>> + has_name = true;
>>> ret = add_changeset_property(ovcs, target, prop, 0);
>>> if (ret) {
>>> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
>>> @@ -455,6 +459,57 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>> }
>>> }
>>>
>>> + /*
>>> + * With FDT version 0x10 we may not have the name property,
>>> + * recreate it here from the unit name if absent
>>> + */
>>> +
>>> + if (!has_name) {
>>> + const char *p = target->np->full_name, *ps = p, *pa = NULL;
>>> + int len;
>>> +
>>> + /*
>>> + * zzz
>>> + * TODO: stash name_prop on a list in ovcs, to be freed
>>> + * after overlay removed
>>> + */
>>> +
>>> + while (*p) {
>>> + if ((*p) == '@')
>>> + pa = p;
>>> + else if ((*p) == '/')
>>> + ps = p + 1;
>>> + p++;
>>> + }
>>> +
>>> + if (pa < ps)
>>> + pa = p;
>>> + len = (pa - ps) + 1;
>>> +
>>> + name_prop = kmalloc(sizeof(*name_prop), GFP_KERNEL);
>>> + if (!name_prop)
>>> + return -ENOMEM;
>>> +
>>> + name_prop->name = kstrdup("name", GFP_KERNEL);
>>> + name_prop->value = kmalloc(len, GFP_KERNEL);
>>> + if (!name_prop->name || !name_prop->value) {
>>> + ret = -ENOMEM;
>>> + goto err_free_name_prop;
>>> + }
>>> +
>>> + memcpy(name_prop->value, ps, len - 1);
>>> + ((char *)name_prop->value)[len - 1] = 0;
>>> +
>>> + name_prop->length = strlen(name_prop->value) + 1;
>>> +
>>> + ret = add_changeset_property(ovcs, target, name_prop, 0);
>>> + if (ret) {
>>> + pr_debug("Failed to apply name_prop @%pOF/%s, err=%d\n",
>>> + target->np, name_prop->name, ret);
>>> + goto err_free_name_prop;
>>> + }
>>> + }
>>> +
>>> for_each_child_of_node(overlay_node, child) {
>>> ret = add_changeset_node(ovcs, target, child);
>>> if (ret) {
>>> @@ -466,6 +521,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>> }
>>>
>>> return 0;
>>> +
>>> +err_free_name_prop:
>>> + kfree(name_prop->name);
>>> + kfree(name_prop->value);
>>> + kfree(name_prop);
>>> + return ret;
>>> +
>>> }
>>>
>>> /*
>>>
>>
>
On 10/10/18 14:03, Frank Rowand wrote:
> On 10/10/18 13:40, Alan Tull wrote:
>> On Wed, Oct 10, 2018 at 1:49 AM Frank Rowand <[email protected]> wrote:
>>>
>>> On 10/09/18 23:04, [email protected] wrote:
>>>> From: Frank Rowand <[email protected]>
>>>>
>>>>
>>>> "of: overlay: use prop add changeset entry for property in new nodes"
>>>> fixed a problem where an 'update property' changeset entry was
>>>> created for properties contained in nodes added by a changeset.
>>>> The fix was to use an 'add property' changeset entry.
>>>>
>>>> This exposed more bugs in the apply overlay code. The properties
>>>> 'name', 'phandle', and 'linux,phandle' were filtered out by
>>>> add_changeset_property() as special properties. Change the filter
>>>> to be only for existing nodes, not newly added nodes.
>>>>
>>>> The second bug is that the 'name' property does not exist in the
>>>> newest FDT version, and has to be constructed from the node's
>>>> full_name. Construct an 'add property' changeset entry for
>>>> newly added nodes.
>>>>
>>>> Signed-off-by: Frank Rowand <[email protected]>
>>>> ---
>>>>
>>>>
>>>> Hi Alan,
>>>>
>>>> Thanks for reporting the problem with missing node names.
>>>>
>>>> I was able to replicate the problem, and have created this preliminary
>>>> version of a patch to fix the problem.
>>>>
>>>> I have not extensively reviewed the patch yet, but would appreciate
>>>> if you can confirm this fixes your problem.
>>>>
>>>> I created this patch as patch 17 of the series, but have also
>>>> applied it as patch 05.1, immediately after patch 05/16, and
>>>> built the kernel, booted, and verified name and phandle for
>>>> one of the nodes in a unittest overlay for both cases. So
>>>> minimal testing so far on my part.
>>>>
>>>> I have not verified whether the series builds and boots after
>>>> each of patches 06..16 if this patch is applied as patch 05.1.
>>>>
>>>> There is definitely more work needed for me to complete this
>>>> patch because it allocates some more memory, but does not yet
>>>> free it when the overlay is released.
>>>>
>>>> -Frank
>>>>
>>>>
>>>> drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 67 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index 0b0904f44bc7..9746cea2aa91 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>>> struct property *new_prop = NULL, *prop;
>>>> int ret = 0;
>>>>
>>>> - if (!of_prop_cmp(overlay_prop->name, "name") ||
>>>> - !of_prop_cmp(overlay_prop->name, "phandle") ||
>>>> - !of_prop_cmp(overlay_prop->name, "linux,phandle"))
>>>> - return 0;
>>>> + if (target->in_livetree)
>>>> + if (!of_prop_cmp(overlay_prop->name, "name") ||
>>>> + !of_prop_cmp(overlay_prop->name, "phandle") ||
>>>> + !of_prop_cmp(overlay_prop->name, "linux,phandle"))
>>>> + return 0;
>>>
>>> This is a big hammer patch.
>>>
>>> Nobody should waste time reviewing this patch.
>>
>> I wasn't clear if you still could use the testing so I did re-run my
>> test. This patch adds back some of the missing properties, but the
>> the kobject names aren't set as dev_name() returns NULL:
>>
>> * without this patch some of_node properties don't show up in sysfs:
>> root@arria10:~# ls
>> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
>> clocks compatible interrupt-parent interrupts reg
>>
>> * with this patch, the of_node properties phandle and name are back:
>> root@arria10:~# ls
>> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
>> clocks compatible interrupt-parent interrupts
>> name phandle reg
>
> Thanks for the testing. I'll keep chasing after this problem today.
>
> This is useful data for me as I was not looking under the /sys/bus/...
> tree that you reported, but was instead looking at /proc/device-tree/...
> which showed the same type of problem since the overlay I was using
> does not show up under /sys/bus/...
>
> I'll have to create a useful overlay test case that will show up under
> /sys/bus/...
>
> In the meantime, can you send me the base FDT and the overlay FDT for
> your test case?
I now have a test case that shows the problem under /sys/bus/... so I
no longer need the base FDT and overlay FDT for your test case.
I have determined the location that sets the name to "<NULL>" but do
not have the fix yet. Still working on that.
-Frank
>
> Thanks,
>
> Frank
>
>
>>
>> root@arria10:~# cat
>> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node/name
>> freeze_controllerroot@arria10:~# ("freeze_controller" w/o the \n so
>> the name is correct)
>>
>> * with or without the patch I see the behavior I reported yesterday,
>> kobj names are NULL.
>> root@arria10:~# ls /sys/bus/platform/drivers/altera_freeze_br/
>> bind ff200450.<NULL> uevent unbind
>>
>> root@arria10:~# ls /sys/bus/platform/drivers/altera_gpio/
>> bind ff200010.<NULL> ff200020.<NULL> ff200030.<NULL>
>> uevent unbind
>>
>> Alan
>>
>> Alan
>>
>>>
>>> The following part should not be needed (though the above section might have
>>> to become _slightly_ more complex).
>>>
>>> -Frank
>>>>
>>>> if (target->in_livetree)
>>>> prop = of_find_property(target->np, overlay_prop->name, NULL);
>>>> @@ -443,10 +444,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>>> struct target *target, const struct device_node *overlay_node)
>>>> {
>>>> struct device_node *child;
>>>> - struct property *prop;
>>>> + struct property *prop, *name_prop;
>>>> + bool has_name = false;
>>>> int ret;
>>>>
>>>> for_each_property_of_node(overlay_node, prop) {
>>>> + if (!strcmp(prop->name, "name"))
>>>> + has_name = true;
>>>> ret = add_changeset_property(ovcs, target, prop, 0);
>>>> if (ret) {
>>>> pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
>>>> @@ -455,6 +459,57 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>>> }
>>>> }
>>>>
>>>> + /*
>>>> + * With FDT version 0x10 we may not have the name property,
>>>> + * recreate it here from the unit name if absent
>>>> + */
>>>> +
>>>> + if (!has_name) {
>>>> + const char *p = target->np->full_name, *ps = p, *pa = NULL;
>>>> + int len;
>>>> +
>>>> + /*
>>>> + * zzz
>>>> + * TODO: stash name_prop on a list in ovcs, to be freed
>>>> + * after overlay removed
>>>> + */
>>>> +
>>>> + while (*p) {
>>>> + if ((*p) == '@')
>>>> + pa = p;
>>>> + else if ((*p) == '/')
>>>> + ps = p + 1;
>>>> + p++;
>>>> + }
>>>> +
>>>> + if (pa < ps)
>>>> + pa = p;
>>>> + len = (pa - ps) + 1;
>>>> +
>>>> + name_prop = kmalloc(sizeof(*name_prop), GFP_KERNEL);
>>>> + if (!name_prop)
>>>> + return -ENOMEM;
>>>> +
>>>> + name_prop->name = kstrdup("name", GFP_KERNEL);
>>>> + name_prop->value = kmalloc(len, GFP_KERNEL);
>>>> + if (!name_prop->name || !name_prop->value) {
>>>> + ret = -ENOMEM;
>>>> + goto err_free_name_prop;
>>>> + }
>>>> +
>>>> + memcpy(name_prop->value, ps, len - 1);
>>>> + ((char *)name_prop->value)[len - 1] = 0;
>>>> +
>>>> + name_prop->length = strlen(name_prop->value) + 1;
>>>> +
>>>> + ret = add_changeset_property(ovcs, target, name_prop, 0);
>>>> + if (ret) {
>>>> + pr_debug("Failed to apply name_prop @%pOF/%s, err=%d\n",
>>>> + target->np, name_prop->name, ret);
>>>> + goto err_free_name_prop;
>>>> + }
>>>> + }
>>>> +
>>>> for_each_child_of_node(overlay_node, child) {
>>>> ret = add_changeset_node(ovcs, target, child);
>>>> if (ret) {
>>>> @@ -466,6 +521,13 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>>> }
>>>>
>>>> return 0;
>>>> +
>>>> +err_free_name_prop:
>>>> + kfree(name_prop->name);
>>>> + kfree(name_prop->value);
>>>> + kfree(name_prop);
>>>> + return ret;
>>>> +
>>>> }
>>>>
>>>> /*
>>>>
>>>
>>
>
>
On Thu, Oct 11, 2018 at 12:39 AM Frank Rowand <[email protected]> wrote:
[resend of my messed up rejected email of a minute ago, sorry]
>
> On 10/10/18 14:03, Frank Rowand wrote:
> > On 10/10/18 13:40, Alan Tull wrote:
> >> On Wed, Oct 10, 2018 at 1:49 AM Frank Rowand <[email protected]> wrote:
> >>>
> >>> On 10/09/18 23:04, [email protected] wrote:
> >>>> From: Frank Rowand <[email protected]>
> >>>>
> >>>>
> >>>> "of: overlay: use prop add changeset entry for property in new nodes"
> >>>> fixed a problem where an 'update property' changeset entry was
> >>>> created for properties contained in nodes added by a changeset.
> >>>> The fix was to use an 'add property' changeset entry.
> >>>>
> >>>> This exposed more bugs in the apply overlay code. The properties
> >>>> 'name', 'phandle', and 'linux,phandle' were filtered out by
> >>>> add_changeset_property() as special properties. Change the filter
> >>>> to be only for existing nodes, not newly added nodes.
> >>>>
> >>>> The second bug is that the 'name' property does not exist in the
> >>>> newest FDT version, and has to be constructed from the node's
> >>>> full_name. Construct an 'add property' changeset entry for
> >>>> newly added nodes.
> >>>>
> >>>> Signed-off-by: Frank Rowand <[email protected]>
> >>>> ---
> >>>>
> >>>>
> >>>> Hi Alan,
> >>>>
> >>>> Thanks for reporting the problem with missing node names.
> >>>>
> >>>> I was able to replicate the problem, and have created this preliminary
> >>>> version of a patch to fix the problem.
> >>>>
> >>>> I have not extensively reviewed the patch yet, but would appreciate
> >>>> if you can confirm this fixes your problem.
> >>>>
> >>>> I created this patch as patch 17 of the series, but have also
> >>>> applied it as patch 05.1, immediately after patch 05/16, and
> >>>> built the kernel, booted, and verified name and phandle for
> >>>> one of the nodes in a unittest overlay for both cases. So
> >>>> minimal testing so far on my part.
> >>>>
> >>>> I have not verified whether the series builds and boots after
> >>>> each of patches 06..16 if this patch is applied as patch 05.1.
> >>>>
> >>>> There is definitely more work needed for me to complete this
> >>>> patch because it allocates some more memory, but does not yet
> >>>> free it when the overlay is released.
> >>>>
> >>>> -Frank
> >>>>
> >>>>
> >>>> drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >>>> 1 file changed, 67 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >>>> index 0b0904f44bc7..9746cea2aa91 100644
> >>>> --- a/drivers/of/overlay.c
> >>>> +++ b/drivers/of/overlay.c
> >>>> @@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> >>>> struct property *new_prop = NULL, *prop;
> >>>> int ret = 0;
> >>>>
> >>>> - if (!of_prop_cmp(overlay_prop->name, "name") ||
> >>>> - !of_prop_cmp(overlay_prop->name, "phandle") ||
> >>>> - !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> >>>> - return 0;
> >>>> + if (target->in_livetree)
> >>>> + if (!of_prop_cmp(overlay_prop->name, "name") ||
> >>>> + !of_prop_cmp(overlay_prop->name, "phandle") ||
> >>>> + !of_prop_cmp(overlay_prop->name, "linux,phandle"))
> >>>> + return 0;
> >>>
> >>> This is a big hammer patch.
> >>>
> >>> Nobody should waste time reviewing this patch.
> >>
> >> I wasn't clear if you still could use the testing so I did re-run my
> >> test. This patch adds back some of the missing properties, but the
> >> the kobject names aren't set as dev_name() returns NULL:
> >>
> >> * without this patch some of_node properties don't show up in sysfs:
> >> root@arria10:~# ls
> >> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
> >> clocks compatible interrupt-parent interrupts reg
> >>
> >> * with this patch, the of_node properties phandle and name are back:
> >> root@arria10:~# ls
> >> /sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
> >> clocks compatible interrupt-parent interrupts
> >> name phandle reg
> >
> > Thanks for the testing. I'll keep chasing after this problem today.
Glad to help! Thanks for all your work in this area!
> >
> > This is useful data for me as I was not looking under the /sys/bus/...
> > tree that you reported, but was instead looking at /proc/device-tree/...
> > which showed the same type of problem since the overlay I was using
> > does not show up under /sys/bus/...
> >
> > I'll have to create a useful overlay test case that will show up under
> > /sys/bus/...
> >
> > In the meantime, can you send me the base FDT and the overlay FDT for
> > your test case?
>
> I now have a test case that shows the problem under /sys/bus/... so I
> no longer need the base FDT and overlay FDT for your test case.
>
> I have determined the location that sets the name to "<NULL>" but do
> not have the fix yet. Still working on that.
I understand you're quite busy with all this, but I'm wondering
whether it might be worth it go ahead and make the properties be
kernel objects also at this point. That would be an improvement for
the case of overlay properties added to non-overlay nodes, so the
lifespan of the overlay property memory can be coupled with the
properties kobj's instead of the node kobj's.
Alan
On 10/11/18 12:33, Alan Tull wrote:
> On Thu, Oct 11, 2018 at 12:39 AM Frank Rowand <[email protected]> wrote:
>
> [resend of my messed up rejected email of a minute ago, sorry]
>
>>
>> On 10/10/18 14:03, Frank Rowand wrote:
< snip >
> I understand you're quite busy with all this, but I'm wondering
> whether it might be worth it go ahead and make the properties be
> kernel objects also at this point. That would be an improvement for
> the case of overlay properties added to non-overlay nodes, so the
> lifespan of the overlay property memory can be coupled with the
> properties kobj's instead of the node kobj's.
>
> Alan
>
That is one of the approaches that I am thinking about to handle
the potential memory leaks from those properties.
I'd like to make these changes in a step wise fashion, to let each
major change get some exposure and use before moving on to the
next step. Making properties into kernel objects would impact
a lot of code.
So not in this series. But thanks for thinking about it.
-Frank