[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree
Implement a set of functions to manipulate properties in a live device tree:
* ofnode_set_property() to set generic properties of a node * ofnode_write_string() to set string properties of a node * ofnode_set_enabled() to either enable or disable a node
Signed-off-by: Mario Six mario.six@gdsys.cc
---
v1 -> v2: * Fix potential NULL pointer dereference in ofnode_write_property
--- drivers/core/ofnode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/ofnode.h | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 5909a25f85..a55aa75e5b 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr) else return fdt_translate_address(gd->fdt_blob, ofnode_to_offset(node), in_addr); } + +#ifdef CONFIG_OF_LIVE +int ofnode_write_property(ofnode node, const char *propname, int len, + const void *value) +{ + const struct device_node *np = ofnode_to_np(node); + struct property *pp; + struct property *pp_last = NULL; + struct property *new; + + if (!np) + return -EINVAL; + + for (pp = np->properties; pp; pp = pp->next) { + if (strcmp(pp->name, propname) == 0) { + /* Property exists -> change value */ + pp->value = (void *)value; + pp->length = len; + return 0; + } + pp_last = pp; + } + + if (!pp_last) + return -ENOENT; + + /* Property does not exist -> append new property */ + new = malloc(sizeof(struct property)); + + new->name = strdup(propname); + new->value = malloc(len); + memcpy(new->value, value, len); + new->length = len; + new->next = NULL; + + pp_last->next = new; + + return 0; +} + +int ofnode_write_string(ofnode node, const char *propname, const char *value) +{ + assert(ofnode_valid(node)); + debug("%s: %s = %s", __func__, propname, value); + + return ofnode_write_property(node, propname, strlen(value) + 1, value); +} + +int ofnode_set_enabled(ofnode node, bool value) +{ + assert(ofnode_valid(node)); + + if (value) + return ofnode_write_string(node, "status", "okay"); + else + return ofnode_write_string(node, "status", "disable"); +} +#endif diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 0d008404f9..9224d583fc 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -681,4 +681,41 @@ int ofnode_read_resource_byname(ofnode node, const char *name, * @return the translated address; OF_BAD_ADDR on error */ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr); + +/** + * ofnode_write_property() - Set a property of a ofnode + * + * @node: The node for whose property should be set + * @propname: The name of the property to set + * @len: The length of the new value of the property + * @value: The new value of the property + * @return 0 if successful, -ve on error + */ +int ofnode_write_property(ofnode node, const char *propname, int len, + const void *value); + +/** + * ofnode_write_string() - Set a string property of a ofnode + * + * @node: The node for whose string property should be set + * @propname: The name of the string property to set + * @value: The new value of the string property + * @return 0 if successful, -ve on error + */ +int ofnode_write_string(ofnode node, const char *propname, const char *value); + +/** + * ofnode_set_enabled() - Enable or disable a device tree node given by its ofnode + * + * This function effectively sets the node's "status" property to either "okay" + * or "disable", hence making it available for driver model initialization or + * not. + * + * @node: The node to enable + * @value: Flag that tells the function to either disable or enable the + * node + * @return 0 if successful, -ve on error + */ +int ofnode_set_enabled(ofnode node, bool value); + #endif -- 2.16.1
Add tests for the ofnode_set_enabled, ofnode_write_string, and ofnode_write_property functions.
Signed-off-by: Mario Six mario.six@gdsys.cc
---
v1 -> v2: New in v2
--- test/dm/test-fdt.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 0d11bfdb2f..3067510433 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -15,6 +15,8 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <dm/util.h> +#include <dm/lists.h> +#include <dm/of_access.h> #include <test/ut.h>
DECLARE_GLOBAL_DATA_PTR; @@ -462,3 +464,54 @@ static int dm_test_fdt_translation(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_fdt_translation, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +static int dm_test_fdt_livetree_writing(struct unit_test_state *uts) +{ + struct udevice *dev; + ofnode node; + + if (!of_live_active()) { + printf("Live tree not active; ignore test\n"); + return 0; + } + + /* Test enabling devices */ + + node = ofnode_path("/usb@2"); + + ut_assert(!of_device_is_available(ofnode_to_np(node))); + ofnode_set_enabled(node, true); + ut_assert(of_device_is_available(ofnode_to_np(node))); + + device_bind_driver_to_node(dm_root(), "usb_sandbox", "usb@2", node, &dev); + ut_assertok(uclass_find_device_by_seq(UCLASS_USB, 2, true, &dev)); + + /* Test string property setting */ + + ut_assert(device_is_compatible(dev, "sandbox,usb")); + ofnode_write_string(node, "compatible", "gdsys,super-usb"); + ut_assert(device_is_compatible(dev, "gdsys,super-usb")); + ofnode_write_string(node, "compatible", "sandbox,usb"); + ut_assert(device_is_compatible(dev, "sandbox,usb")); + + /* Test setting generic properties */ + + /* Non-existent in DTB */ + ut_asserteq(FDT_ADDR_T_NONE, dev_read_addr(dev)); + /* reg = 0x42, size = 0x100 */ + ut_assertok(ofnode_write_property(node, "reg", 8, + "\x00\x00\x00\x42\x00\x00\x01\x00")); + ut_asserteq(0x42, dev_read_addr(dev)); + + /* Test disabling devices */ + + device_remove(dev, DM_REMOVE_NORMAL); + device_unbind(dev); + + ut_assert(of_device_is_available(ofnode_to_np(node))); + ofnode_set_enabled(node, false); + ut_assert(!of_device_is_available(ofnode_to_np(node))); + + return 0; +} +DM_TEST(dm_test_fdt_livetree_writing, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); -- 2.16.1
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
Add tests for the ofnode_set_enabled, ofnode_write_string, and ofnode_write_property functions.
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2: New in v2
test/dm/test-fdt.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
We cannot use device structures to disable devices, since getting them with the API functions would bind and activate the device, which would fail if the underlying device does not exist.
Hence, add a function to disable devices by path in a live device tree.
Signed-off-by: Mario Six mario.six@gdsys.cc
---
v1 -> v2: * Simplified np_to_ofnode(of_find_node_by_path(path)) to ofnode_path(path) * Switched to returning -ENOSYS if livetree is not enabled * Removed device_find_by_ofnode usage
--- drivers/core/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/device.h | 20 ++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 940a153c58..3bb2fa02af 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -724,3 +724,61 @@ bool of_machine_is_compatible(const char *compat)
return !fdt_node_check_compatible(fdt, 0, compat); } + +int dev_disable_by_path(const char *path) +{ + struct uclass *uc; + ofnode node = ofnode_path(path); + struct udevice *dev; + int res = 1; + + if (!of_live_active()) + return -ENOSYS; + + list_for_each_entry(uc, &gd->uclass_root, sibling_node) { + res = uclass_find_device_by_ofnode(uc->uc_drv->id, node, &dev); + if (!res || dev) + break; + } + + if (res || !dev) + return res; + + res = device_remove(dev, DM_REMOVE_NORMAL); + if (res) + return res; + + res = device_unbind(dev); + if (res) + return res; + + return ofnode_set_enabled(node, false); +} + +int dev_enable_by_path(const char *path) +{ + struct uclass *uc; + ofnode node = ofnode_path(path); + ofnode pnode = ofnode_get_parent(node); + struct udevice *parent; + int res = 1; + + if (!of_live_active()) + return -ENOSYS; + + list_for_each_entry(uc, &gd->uclass_root, sibling_node) { + res = uclass_find_device_by_ofnode(uc->uc_drv->id, pnode, + &parent); + if (!res || parent) + break; + } + + if (res || !parent) + return res; + + res = ofnode_set_enabled(node, true); + if (res) + return res; + + return lists_bind_fdt(parent, node, NULL); +} diff --git a/include/dm/device.h b/include/dm/device.h index 7786b1cf4e..f55907966a 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -586,6 +586,26 @@ bool device_is_compatible(struct udevice *dev, const char *compat); */ bool of_machine_is_compatible(const char *compat);
+#ifdef CONFIG_OF_LIVE + +/** + * dev_disable_by_path() - Disable a device given its device tree path + * + * @path: The device tree path identifying the device to be disabled + * @return 0 on success, -ve on error + */ +int dev_disable_by_path(const char *path); + +/** + * dev_enable_by_path() - Enable a device given its device tree path + * + * @path: The device tree path identifying the device to be enabled + * @return 0 on success, -ve on error + */ +int dev_enable_by_path(const char *path); + +#endif /* CONFIG_OF_LIVE */ + /** * device_is_on_pci_bus - Test if a device is on a PCI bus * -- 2.16.1
Hi Mario,
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
We cannot use device structures to disable devices, since getting them with the API functions would bind and activate the device, which would fail if the underlying device does not exist.
Hence, add a function to disable devices by path in a live device tree.
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2:
- Simplified np_to_ofnode(of_find_node_by_path(path)) to ofnode_path(path)
- Switched to returning -ENOSYS if livetree is not enabled
- Removed device_find_by_ofnode usage
drivers/core/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/device.h | 20 ++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 940a153c58..3bb2fa02af 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -724,3 +724,61 @@ bool of_machine_is_compatible(const char *compat)
return !fdt_node_check_compatible(fdt, 0, compat);}
+int dev_disable_by_path(const char *path) +{
struct uclass *uc;ofnode node = ofnode_path(path);struct udevice *dev;int res = 1;
For consistency with current code can you please use ret instead of res?
if (!of_live_active())return -ENOSYS;list_for_each_entry(uc, &gd->uclass_root, sibling_node) {res = uclass_find_device_by_ofnode(uc->uc_drv->id, node, &dev);
How about putting this in global device_find_by_ofnode() function, which doesn't work on a per-uclass basis?
if (!res || dev)break;
Just !res
}if (res || !dev)return res;
Just
if (res)
res = device_remove(dev, DM_REMOVE_NORMAL);if (res)return res;res = device_unbind(dev);if (res)return res;return ofnode_set_enabled(node, false);+}
+int dev_enable_by_path(const char *path) +{
struct uclass *uc;ofnode node = ofnode_path(path);ofnode pnode = ofnode_get_parent(node);struct udevice *parent;int res = 1;if (!of_live_active())return -ENOSYS;list_for_each_entry(uc, &gd->uclass_root, sibling_node) {res = uclass_find_device_by_ofnode(uc->uc_drv->id, pnode,&parent);if (!res || parent)
if (!res)
break;}if (res || !parent)
if (res)
return res;res = ofnode_set_enabled(node, true);if (res)return res;return lists_bind_fdt(parent, node, NULL);+} diff --git a/include/dm/device.h b/include/dm/device.h index 7786b1cf4e..f55907966a 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -586,6 +586,26 @@ bool device_is_compatible(struct udevice *dev, const char *compat); */ bool of_machine_is_compatible(const char *compat);
+#ifdef CONFIG_OF_LIVE
+/**
- dev_disable_by_path() - Disable a device given its device tree path
- @path: The device tree path identifying the device to be disabled
- @return 0 on success, -ve on error
- */
+int dev_disable_by_path(const char *path);
+/**
- dev_enable_by_path() - Enable a device given its device tree path
- @path: The device tree path identifying the device to be enabled
- @return 0 on success, -ve on error
- */
+int dev_enable_by_path(const char *path);
+#endif /* CONFIG_OF_LIVE */
/**
- device_is_on_pci_bus - Test if a device is on a PCI bus
-- 2.16.1
Regards, Simon
Hi Simon,
On Thu, May 3, 2018 at 4:33 AM, Simon Glass sjg@chromium.org wrote:
Hi Mario,
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
We cannot use device structures to disable devices, since getting them with the API functions would bind and activate the device, which would fail if the underlying device does not exist.
Hence, add a function to disable devices by path in a live device tree.
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2:
- Simplified np_to_ofnode(of_find_node_by_path(path)) to ofnode_path(path)
- Switched to returning -ENOSYS if livetree is not enabled
- Removed device_find_by_ofnode usage
drivers/core/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/device.h | 20 ++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 940a153c58..3bb2fa02af 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -724,3 +724,61 @@ bool of_machine_is_compatible(const char *compat)
return !fdt_node_check_compatible(fdt, 0, compat);}
+int dev_disable_by_path(const char *path) +{
struct uclass *uc;ofnode node = ofnode_path(path);struct udevice *dev;int res = 1;For consistency with current code can you please use ret instead of res?
Sure, will be fixed in v3.
if (!of_live_active())return -ENOSYS;list_for_each_entry(uc, &gd->uclass_root, sibling_node) {res = uclass_find_device_by_ofnode(uc->uc_drv->id, node, &dev);How about putting this in global device_find_by_ofnode() function, which doesn't work on a per-uclass basis?
if (!res || dev)break;Just !res
Will be fixed in v3.
}if (res || !dev)return res;Just
if (res)
Will be fixed in v3.
res = device_remove(dev, DM_REMOVE_NORMAL);if (res)return res;res = device_unbind(dev);if (res)return res;return ofnode_set_enabled(node, false);+}
+int dev_enable_by_path(const char *path) +{
struct uclass *uc;ofnode node = ofnode_path(path);ofnode pnode = ofnode_get_parent(node);struct udevice *parent;int res = 1;if (!of_live_active())return -ENOSYS;list_for_each_entry(uc, &gd->uclass_root, sibling_node) {res = uclass_find_device_by_ofnode(uc->uc_drv->id, pnode,&parent);if (!res || parent)if (!res)
Will be fixed in v3.
break;}if (res || !parent)if (res)
Will be fixed in v3.
return res;res = ofnode_set_enabled(node, true);if (res)return res;return lists_bind_fdt(parent, node, NULL);+} diff --git a/include/dm/device.h b/include/dm/device.h index 7786b1cf4e..f55907966a 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -586,6 +586,26 @@ bool device_is_compatible(struct udevice *dev, const char *compat); */ bool of_machine_is_compatible(const char *compat);
+#ifdef CONFIG_OF_LIVE
+/**
- dev_disable_by_path() - Disable a device given its device tree path
- @path: The device tree path identifying the device to be disabled
- @return 0 on success, -ve on error
- */
+int dev_disable_by_path(const char *path);
+/**
- dev_enable_by_path() - Enable a device given its device tree path
- @path: The device tree path identifying the device to be enabled
- @return 0 on success, -ve on error
- */
+int dev_enable_by_path(const char *path);
+#endif /* CONFIG_OF_LIVE */
/**
- device_is_on_pci_bus - Test if a device is on a PCI bus
-- 2.16.1
Regards, Simon
Best regards, Mario
Add tests for the dev_{enable,disable}_by_path functions.
Signed-off-by: Mario Six mario.six@gdsys.cc
---
v1 -> v2: New in v2
--- test/dm/test-fdt.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 3067510433..903b8cc9dc 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -515,3 +515,30 @@ static int dm_test_fdt_livetree_writing(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_fdt_livetree_writing, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +static int dm_test_fdt_disable_enable_by_path(struct unit_test_state *uts) +{ + ofnode node; + + if (!of_live_active()) { + printf("Live tree not active; ignore test\n"); + return 0; + } + + node = ofnode_path("/usb@2"); + + /* Test enabling devices */ + + ut_assert(!of_device_is_available(ofnode_to_np(node))); + dev_enable_by_path("/usb@2"); + ut_assert(of_device_is_available(ofnode_to_np(node))); + + /* Test disabling devices */ + + ut_assert(of_device_is_available(ofnode_to_np(node))); + dev_disable_by_path("/usb@2"); + ut_assert(!of_device_is_available(ofnode_to_np(node))); + + return 0; +} +DM_TEST(dm_test_fdt_disable_enable_by_path, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); -- 2.16.1
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
Add tests for the dev_{enable,disable}_by_path functions.
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2: New in v2
test/dm/test-fdt.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Mario,
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
Implement a set of functions to manipulate properties in a live device tree:
- ofnode_set_property() to set generic properties of a node
- ofnode_write_string() to set string properties of a node
- ofnode_set_enabled() to either enable or disable a node
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2:
- Fix potential NULL pointer dereference in ofnode_write_property
drivers/core/ofnode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/ofnode.h | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
But please see below.
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 5909a25f85..a55aa75e5b 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr) else return fdt_translate_address(gd->fdt_blob, ofnode_to_offset(node), in_addr); }
+#ifdef CONFIG_OF_LIVE +int ofnode_write_property(ofnode node, const char *propname, int len,
const void *value)+{
const struct device_node *np = ofnode_to_np(node);struct property *pp;struct property *pp_last = NULL;struct property *new;if (!np)return -EINVAL;for (pp = np->properties; pp; pp = pp->next) {if (strcmp(pp->name, propname) == 0) {/* Property exists -> change value */pp->value = (void *)value;pp->length = len;return 0;}pp_last = pp;}if (!pp_last)return -ENOENT;/* Property does not exist -> append new property */new = malloc(sizeof(struct property));new->name = strdup(propname);new->value = malloc(len);memcpy(new->value, value, len);
To me it seems odd that you allocate space for the value here, but above (in the loop) you just assign it.
new->length = len;new->next = NULL;pp_last->next = new;return 0;+}
+int ofnode_write_string(ofnode node, const char *propname, const char *value) +{
assert(ofnode_valid(node));debug("%s: %s = %s", __func__, propname, value);return ofnode_write_property(node, propname, strlen(value) + 1, value);+}
+int ofnode_set_enabled(ofnode node, bool value) +{
assert(ofnode_valid(node));if (value)return ofnode_write_string(node, "status", "okay");elsereturn ofnode_write_string(node, "status", "disable");+} +#endif diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 0d008404f9..9224d583fc 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -681,4 +681,41 @@ int ofnode_read_resource_byname(ofnode node, const char *name,
- @return the translated address; OF_BAD_ADDR on error
*/ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr);
+/**
- ofnode_write_property() - Set a property of a ofnode
- @node: The node for whose property should be set
- @propname: The name of the property to set
- @len: The length of the new value of the property
- @value: The new value of the property
Need to mention if this is copied or if the pointer needs to remain valid forever.
- @return 0 if successful, -ve on error
- */
+int ofnode_write_property(ofnode node, const char *propname, int len,
const void *value);+/**
- ofnode_write_string() - Set a string property of a ofnode
- @node: The node for whose string property should be set
- @propname: The name of the string property to set
- @value: The new value of the string property
Same here
- @return 0 if successful, -ve on error
- */
+int ofnode_write_string(ofnode node, const char *propname, const char *value);
+/**
- ofnode_set_enabled() - Enable or disable a device tree node given by its ofnode
- This function effectively sets the node's "status" property to either "okay"
- or "disable", hence making it available for driver model initialization or
- not.
- @node: The node to enable
- @value: Flag that tells the function to either disable or enable the
node
- @return 0 if successful, -ve on error
- */
+int ofnode_set_enabled(ofnode node, bool value);
#endif
2.16.1
Regards, Simon
Hi Simon,
On Tue, May 1, 2018 at 1:13 AM, Simon Glass sjg@chromium.org wrote:
Hi Mario,
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
Implement a set of functions to manipulate properties in a live device tree:
- ofnode_set_property() to set generic properties of a node
- ofnode_write_string() to set string properties of a node
- ofnode_set_enabled() to either enable or disable a node
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2:
- Fix potential NULL pointer dereference in ofnode_write_property
drivers/core/ofnode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/ofnode.h | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
But please see below.
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 5909a25f85..a55aa75e5b 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr) else return fdt_translate_address(gd->fdt_blob, ofnode_to_offset(node), in_addr); }
+#ifdef CONFIG_OF_LIVE +int ofnode_write_property(ofnode node, const char *propname, int len,
const void *value)+{
const struct device_node *np = ofnode_to_np(node);struct property *pp;struct property *pp_last = NULL;struct property *new;if (!np)return -EINVAL;for (pp = np->properties; pp; pp = pp->next) {if (strcmp(pp->name, propname) == 0) {/* Property exists -> change value */pp->value = (void *)value;pp->length = len;return 0;}pp_last = pp;}if (!pp_last)return -ENOENT;/* Property does not exist -> append new property */new = malloc(sizeof(struct property));new->name = strdup(propname);new->value = malloc(len);memcpy(new->value, value, len);To me it seems odd that you allocate space for the value here, but above (in the loop) you just assign it.
Right, just the pointer in the property itself is allocated; just assigning the pointer to the property value with the one passed in might lead to it being deallocated.
Unfortunately a "free and malloc" or "realloc" won't work, since the unflatten procedure in lib/of_live.c allocates the memory for the whole device tree as one chunk, which cannot be partially freed. So the only choice would either be only a "malloc" without prior freeing (which would leak memory if the property is set multiple times), or require that the property value passed in is valid forever (i.e. the caller has to malloc it himself), which would make the interface more complicated to use, and also pushes the responsibility of freeing the value onto the caller, who probably cannot safely decide when to free it anyway.
Another idea would be to find out the size of the unflattened device tree; that way one could decide whether the property value pointer points into the allocated memory for the tree (in that case, just allocate a new property), or if it points into a different memory region, which would indicate that the property was changed before already (in that case, free and allocate new property or reallocate). I don't know how complicated that would be, though. I'll investigate.
new->length = len;new->next = NULL;pp_last->next = new;return 0;+}
+int ofnode_write_string(ofnode node, const char *propname, const char *value) +{
assert(ofnode_valid(node));debug("%s: %s = %s", __func__, propname, value);return ofnode_write_property(node, propname, strlen(value) + 1, value);+}
+int ofnode_set_enabled(ofnode node, bool value) +{
assert(ofnode_valid(node));if (value)return ofnode_write_string(node, "status", "okay");elsereturn ofnode_write_string(node, "status", "disable");+} +#endif diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 0d008404f9..9224d583fc 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -681,4 +681,41 @@ int ofnode_read_resource_byname(ofnode node, const char *name,
- @return the translated address; OF_BAD_ADDR on error
*/ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr);
+/**
- ofnode_write_property() - Set a property of a ofnode
- @node: The node for whose property should be set
- @propname: The name of the property to set
- @len: The length of the new value of the property
- @value: The new value of the property
Need to mention if this is copied or if the pointer needs to remain valid forever.
OK, will be documented in v3.
- @return 0 if successful, -ve on error
- */
+int ofnode_write_property(ofnode node, const char *propname, int len,
const void *value);+/**
- ofnode_write_string() - Set a string property of a ofnode
- @node: The node for whose string property should be set
- @propname: The name of the string property to set
- @value: The new value of the string property
Same here
OK, will be documented in v3.
- @return 0 if successful, -ve on error
- */
+int ofnode_write_string(ofnode node, const char *propname, const char *value);
+/**
- ofnode_set_enabled() - Enable or disable a device tree node given by its ofnode
- This function effectively sets the node's "status" property to either "okay"
- or "disable", hence making it available for driver model initialization or
- not.
- @node: The node to enable
- @value: Flag that tells the function to either disable or enable the
node
- @return 0 if successful, -ve on error
- */
+int ofnode_set_enabled(ofnode node, bool value);
#endif
2.16.1
Regards, Simon
Best regards, Mario
Hi Mario,
On 4 May 2018 at 01:14, Mario Six mario.six@gdsys.cc wrote:
Hi Simon,
On Tue, May 1, 2018 at 1:13 AM, Simon Glass sjg@chromium.org wrote:
Hi Mario,
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
Implement a set of functions to manipulate properties in a live device tree:
- ofnode_set_property() to set generic properties of a node
- ofnode_write_string() to set string properties of a node
- ofnode_set_enabled() to either enable or disable a node
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2:
- Fix potential NULL pointer dereference in ofnode_write_property
drivers/core/ofnode.c | 58
+++++++++++++++++++++++++++++++++++++++++++++++++++
include/dm/ofnode.h | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
But please see below.
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 5909a25f85..a55aa75e5b 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const
fdt32_t *in_addr)
else return fdt_translate_address(gd->fdt_blob,
ofnode_to_offset(node), in_addr);
}
+#ifdef CONFIG_OF_LIVE +int ofnode_write_property(ofnode node, const char *propname, int len,
const void *value)+{
const struct device_node *np = ofnode_to_np(node);struct property *pp;struct property *pp_last = NULL;struct property *new;if (!np)return -EINVAL;for (pp = np->properties; pp; pp = pp->next) {if (strcmp(pp->name, propname) == 0) {/* Property exists -> change value */pp->value = (void *)value;pp->length = len;return 0;}pp_last = pp;}if (!pp_last)return -ENOENT;/* Property does not exist -> append new property */new = malloc(sizeof(struct property));new->name = strdup(propname);new->value = malloc(len);memcpy(new->value, value, len);To me it seems odd that you allocate space for the value here, but above (in the loop) you just assign it.
Right, just the pointer in the property itself is allocated; just
assigning the
pointer to the property value with the one passed in might lead to it
being
deallocated.
Who will ever deallocate it? To me these cases are the same and I can't see why an existing property should be simply assigned, but a new property must be allocated. At the very lease that seems like a confusing thing to explain in the function comment you're going to add :-)
Unfortunately a "free and malloc" or "realloc" won't work, since the
unflatten
procedure in lib/of_live.c allocates the memory for the whole device tree
as
one chunk, which cannot be partially freed. So the only choice would
either be
only a "malloc" without prior freeing (which would leak memory if the
property
is set multiple times), or require that the property value passed in is
valid
forever (i.e. the caller has to malloc it himself), which would make the interface more complicated to use, and also pushes the responsibility of freeing the value onto the caller, who probably cannot safely decide when
to
free it anyway.
Another idea would be to find out the size of the unflattened device
tree; that
way one could decide whether the property value pointer points into the allocated memory for the tree (in that case, just allocate a new
property), or
if it points into a different memory region, which would indicate that the property was changed before already (in that case, free and allocate new property or reallocate). I don't know how complicated that would be,
though.
I'll investigate.
Hmm, I think just documenting behaviour clearly is good enough. How about we don't allocate the memory here. The caller can do it if necessary.
Regards, Simon
Hi Simon,
On Fri, May 4, 2018 at 11:37 PM, Simon Glass sjg@chromium.org wrote:
Hi Mario,
On 4 May 2018 at 01:14, Mario Six mario.six@gdsys.cc wrote:
Hi Simon,
On Tue, May 1, 2018 at 1:13 AM, Simon Glass sjg@chromium.org wrote:
Hi Mario,
On 27 April 2018 at 06:51, Mario Six mario.six@gdsys.cc wrote:
Implement a set of functions to manipulate properties in a live device tree:
- ofnode_set_property() to set generic properties of a node
- ofnode_write_string() to set string properties of a node
- ofnode_set_enabled() to either enable or disable a node
Signed-off-by: Mario Six mario.six@gdsys.cc
v1 -> v2:
- Fix potential NULL pointer dereference in ofnode_write_property
drivers/core/ofnode.c | 58
+++++++++++++++++++++++++++++++++++++++++++++++++++
include/dm/ofnode.h | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
But please see below.
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 5909a25f85..a55aa75e5b 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const
fdt32_t *in_addr)
else return fdt_translate_address(gd->fdt_blob,ofnode_to_offset(node), in_addr);
}
+#ifdef CONFIG_OF_LIVE +int ofnode_write_property(ofnode node, const char *propname, int len,
const void *value)+{
const struct device_node *np = ofnode_to_np(node);struct property *pp;struct property *pp_last = NULL;struct property *new;if (!np)return -EINVAL;for (pp = np->properties; pp; pp = pp->next) {if (strcmp(pp->name, propname) == 0) {/* Property exists -> change value */pp->value = (void *)value;pp->length = len;return 0;}pp_last = pp;}if (!pp_last)return -ENOENT;/* Property does not exist -> append new property */new = malloc(sizeof(struct property));new->name = strdup(propname);new->value = malloc(len);memcpy(new->value, value, len);To me it seems odd that you allocate space for the value here, but above (in the loop) you just assign it.
Right, just the pointer in the property itself is allocated; just
assigning the
pointer to the property value with the one passed in might lead to it
being
deallocated.
Who will ever deallocate it? To me these cases are the same and I can't see why an existing property should be simply assigned, but a new property must be allocated. At the very lease that seems like a confusing thing to explain in the function comment you're going to add :-)
Unfortunately a "free and malloc" or "realloc" won't work, since the
unflatten
procedure in lib/of_live.c allocates the memory for the whole device tree
as
one chunk, which cannot be partially freed. So the only choice would
either be
only a "malloc" without prior freeing (which would leak memory if the
property
is set multiple times), or require that the property value passed in is
valid
forever (i.e. the caller has to malloc it himself), which would make the interface more complicated to use, and also pushes the responsibility of freeing the value onto the caller, who probably cannot safely decide when
to
free it anyway.
Another idea would be to find out the size of the unflattened device
tree; that
way one could decide whether the property value pointer points into the allocated memory for the tree (in that case, just allocate a new
property), or
if it points into a different memory region, which would indicate that the property was changed before already (in that case, free and allocate new property or reallocate). I don't know how complicated that would be,
though.
I'll investigate.
Hmm, I think just documenting behaviour clearly is good enough. How about we don't allocate the memory here. The caller can do it if necessary.
OK, should not be too difficult. I'll use that approach for v3.
Regards, Simon
Best regards, Mario
participants (2)
-
Mario Six -
Simon Glass