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 <[email protected]> --- (no changes since v1) doc/develop/driver-model/livetree.rst | 2 + drivers/core/ofnode.c | 29 +++++++++---- include/dm/ofnode.h | 12 ++++-- test/dm/ofnode.c | 62 ++++++++++++++++++++++++++- 4 files changed, 92 insertions(+), 13 deletions(-) diff --git a/doc/develop/driver-model/livetree.rst b/doc/develop/driver-model/livetree.rst index 65b88f854eb..55aa3eac929 100644 --- 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 1f477322d5d..51a16537b0d 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -1322,15 +1322,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) @@ -1339,7 +1351,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) @@ -1354,7 +1367,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 113dd15afe3..c7d61e94121 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -1310,19 +1310,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 0912a53099d..69ffba06534 100644 --- 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; -- 2.37.2.789.g6183377224-goog
