[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Mario Six]

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

[U-Boot] [PATCH v2 2/4] test: Add tests for DT-manipulation functions

[Mario Six]

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 at 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 at 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

[U-Boot] [PATCH v2 3/4] core: Add dev_{disable,enable}_by_path

[Mario Six]

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

[U-Boot] [PATCH v2 4/4] test: Add tests for dev_{enable, disable}_by_path

[Mario Six]

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 at 2");
+
+	/* Test enabling devices */
+
+	ut_assert(!of_device_is_available(ofnode_to_np(node)));
+	dev_enable_by_path("/usb at 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 at 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

[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Simon Glass]

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");
> +       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

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

[U-Boot] [PATCH v2 2/4] test: Add tests for DT-manipulation functions

[Simon Glass]

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>

[U-Boot] [PATCH v2 3/4] core: Add dev_{disable,enable}_by_path

[Simon Glass]

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

[U-Boot] [PATCH v2 4/4] test: Add tests for dev_{enable, disable}_by_path

[Simon Glass]

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>

[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Mario Six]

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");
>> +       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
>
> 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

[U-Boot] [PATCH v2 3/4] core: Add dev_{disable,enable}_by_path

[Mario Six]

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

[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Simon Glass]

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

[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Mario Six]

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