dm: core: Allow copying ofnode property data when writing

At present ofnode_write_prop() is inconsistent between livetree and
flattree, in that livetree requires the caller to ensure the property
value is stable (e.g. in rodata or allocated) but flattree does not, since
it makes a copy.

This makes the API call a bit painful to use, since the caller must do
different things depending on OF_LIVE.

Add a new 'copy' argument which tells the function to make a copy if
needed. Add some tests to cover this behaviour.

Signed-off-by: Simon Glass <sjg@chromium.org>

diff --git a/doc/develop/driver-model/livetree.rst b/doc/develop/driver-model/livetree.rst
index 65b88f854eb42b243f7c1b707b3f1f6deb5a84ad..55aa3eac929bada40abd2b9b64dcb03705d1b731 100644 (file)
--- a/doc/develop/driver-model/livetree.rst
+++ b/doc/develop/driver-model/livetree.rst
@@ -325,3 +325,5 @@ Live tree support was introduced in U-Boot 2017.07. Some possible enhancements
 are:
 
 - support for livetree in SPL and before relocation (if desired)
+- freeing leaked memory caused by writing new nodes / property values to the
+  livetree (ofnode_write_prop())

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 2c4d5210095064ce2ef3bb555e18c2f51b2ea4d5..39636a5a18c92794e435d81bac67c7228b492ca4 100644 (file)
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -1388,15 +1388,27 @@ ofnode ofnode_by_prop_value(ofnode from, const char *propname,
 }
 
 int ofnode_write_prop(ofnode node, const char *propname, const void *value,
-                     int len)
+                     int len, bool copy)
 {
-       if (of_live_active())
-               return of_write_prop(ofnode_to_np(node), propname, len, value);
-       else
+       if (of_live_active()) {
+               void *newval;
+               int ret;
+
+               if (copy) {
+                       newval = malloc(len);
+                       if (!newval)
+                               return log_ret(-ENOMEM);
+                       memcpy(newval, value, len);
+                       value = newval;
+               }
+               ret = of_write_prop(ofnode_to_np(node), propname, len, value);
+               if (ret && copy)
+                       free(newval);
+               return ret;
+       } else {
                return fdt_setprop(ofnode_to_fdt(node), ofnode_to_offset(node),
                                   propname, value, len);
-
-       return 0;
+       }
 }
 
 int ofnode_write_string(ofnode node, const char *propname, const char *value)
@@ -1405,7 +1417,8 @@ int ofnode_write_string(ofnode node, const char *propname, const char *value)
 
        debug("%s: %s = %s", __func__, propname, value);
 
-       return ofnode_write_prop(node, propname, value, strlen(value) + 1);
+       return ofnode_write_prop(node, propname, value, strlen(value) + 1,
+                                false);
 }
 
 int ofnode_write_u32(ofnode node, const char *propname, u32 value)
@@ -1420,7 +1433,7 @@ int ofnode_write_u32(ofnode node, const char *propname, u32 value)
                return -ENOMEM;
        *val = cpu_to_fdt32(value);
 
-       return ofnode_write_prop(node, propname, val, sizeof(value));
+       return ofnode_write_prop(node, propname, val, sizeof(value), false);
 }
 
 int ofnode_set_enabled(ofnode node, bool value)

diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 0c5a883eafbc9ae4786060ab4f81000d0800eecc..5203045a58e7e35f7e34a75ed7544e1fe45b0811 100644 (file)
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -1350,19 +1350,23 @@ int ofnode_device_is_compatible(ofnode node, const char *compat);
 /**
  * ofnode_write_prop() - Set a property of a ofnode
  *
- * Note that the value passed to the function is *not* allocated by the
- * function itself, but must be allocated by the caller if necessary. However
- * it does allocate memory for the property struct and name.
+ * Note that if @copy is false, the value passed to the function is *not*
+ * allocated by the function itself, but must be allocated by the caller if
+ * necessary. However it does allocate memory for the property struct and name.
  *
  * @node:      The node for whose property should be set
  * @propname:  The name of the property to set
  * @value:     The new value of the property (must be valid prior to calling
  *             the function)
  * @len:       The length of the new value of the property
+ * @copy: true to allocate memory for the value. This only has any effect with
+ *     live tree, since flat tree handles this automatically. It allows a
+ *     node's value to be written to the tree, without requiring that the
+ *     caller allocate it
  * Return: 0 if successful, -ve on error
  */
 int ofnode_write_prop(ofnode node, const char *propname, const void *value,
-                     int len);
+                     int len, bool copy);
 
 /**
  * ofnode_write_string() - Set a string property of a ofnode

diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c
index 0912a53099d491043f747b5d3ac523a05c67e85f..69ffba06534bb69e1dba5cb8a1a4a6ffd4dd920c 100644 (file)
--- a/test/dm/ofnode.c
+++ b/test/dm/ofnode.c
@@ -822,7 +822,8 @@ static int dm_test_ofnode_livetree_writing(struct unit_test_state *uts)
        ut_asserteq_64(FDT_ADDR_T_NONE, dev_read_addr(dev));
        /* reg = 0x42, size = 0x100 */
        ut_assertok(ofnode_write_prop(node, "reg",
-                                     "\x00\x00\x00\x42\x00\x00\x01\x00", 8));
+                                     "\x00\x00\x00\x42\x00\x00\x01\x00", 8,
+                                     false));
        ut_asserteq(0x42, dev_read_addr(dev));
 
        /* Test disabling devices */
@@ -838,6 +839,65 @@ static int dm_test_ofnode_livetree_writing(struct unit_test_state *uts)
 DM_TEST(dm_test_ofnode_livetree_writing,
        UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
 
+static int check_write_prop(struct unit_test_state *uts, ofnode node)
+{
+       char prop[] = "middle-name";
+       char name[10];
+       int len;
+
+       strcpy(name, "cecil");
+       len = strlen(name) + 1;
+       ut_assertok(ofnode_write_prop(node, prop, name, len, false));
+       ut_asserteq_str(name, ofnode_read_string(node, prop));
+
+       /* change the underlying value, this should mess up the live tree */
+       strcpy(name, "tony");
+       if (of_live_active()) {
+               ut_asserteq_str(name, ofnode_read_string(node, prop));
+       } else {
+               ut_asserteq_str("cecil", ofnode_read_string(node, prop));
+       }
+
+       /* try again, this time copying the property */
+       strcpy(name, "mary");
+       ut_assertok(ofnode_write_prop(node, prop, name, len, true));
+       ut_asserteq_str(name, ofnode_read_string(node, prop));
+       strcpy(name, "leah");
+
+       /* both flattree and livetree behave the same */
+       ut_asserteq_str("mary", ofnode_read_string(node, prop));
+
+       return 0;
+}
+
+/* writing the tree with and without copying the property */
+static int dm_test_ofnode_write_copy(struct unit_test_state *uts)
+{
+       ofnode node;
+
+       node = ofnode_path("/a-test");
+       ut_assertok(check_write_prop(uts, node));
+
+       return 0;
+}
+DM_TEST(dm_test_ofnode_write_copy, UT_TESTF_SCAN_FDT);
+
+static int dm_test_ofnode_write_copy_ot(struct unit_test_state *uts)
+{
+       oftree otree = get_other_oftree(uts);
+       ofnode node, check_node;
+
+       node = oftree_path(otree, "/node");
+       ut_assertok(check_write_prop(uts, node));
+
+       /* make sure the control FDT is not touched */
+       check_node = ofnode_path("/node");
+       ut_assertnull(ofnode_read_string(check_node, "middle-name"));
+
+       return 0;
+}
+DM_TEST(dm_test_ofnode_write_copy_ot, UT_TESTF_OTHER_FDT);
+
 static int dm_test_ofnode_u32(struct unit_test_state *uts)
 {
        ofnode node;