Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root

From: Xu Yilun
Date: Mon Jan 10 2022 - 23:37:24 EST

Next message: Nicholas Piggin: "Re: [PATCH v2 1/2] powerpc: Fix virt_addr_valid() check"
Previous message: Reiji Watanabe: "Re: [RFC PATCH v3 03/11] KVM: Introduce KVM_CAP_ARM_HVC_FW_REG_BMAP"
In reply to: Lizhi Hou: "Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jan 05, 2022 at 02:50:11PM -0800, Lizhi Hou wrote:
> When OF_FLATTREE is selected and there is not a device tree, create an
> empty device tree root node. of/unittest.c code is referenced.
>
> Signed-off-by: Sonal Santan <sonal.santan@xxxxxxxxxx>
> Signed-off-by: Max Zhen <max.zhen@xxxxxxxxxx>
> Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxxxxx>
> ---
> drivers/of/Makefile | 5 +++
> drivers/of/fdt.c | 90 ++++++++++++++++++++++++++++++++++++++
> drivers/of/fdt_default.dts | 5 +++
> drivers/of/of_private.h | 17 +++++++
> drivers/of/unittest.c | 72 ++----------------------------
> 5 files changed, 120 insertions(+), 69 deletions(-)
> create mode 100644 drivers/of/fdt_default.dts
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c13b982084a3..a2989055c578 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-y = base.o device.o platform.o property.o
> +

remove the blank line.

> obj-$(CONFIG_OF_KOBJ) += kobj.o
> obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> obj-$(CONFIG_OF_FLATTREE) += fdt.o
> @@ -20,4 +21,8 @@ obj-y += kexec.o
> endif
> endif
>
> +ifndef CONFIG_OF_UNITTEST
> +obj-$(CONFIG_OF_FLATTREE) += fdt_default.dtb.o
> +endif
> +

Same question as Tom, the unittest should work well with or without
of_root, is it? So creating an empty root will not affect unittest, so
why so many ifdefs for CONFIG_OF_UNITTEST?

> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4546572af24b..66ef9ac97829 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -466,6 +466,96 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
> }
> EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>
> +static int __init of_fdt_root_init(void)
> +{
> + struct device_node *dt = NULL, *np;
> + void *fdt = NULL, *fdt_aligned;
> + struct property *prop = NULL;
> + __be32 *val = NULL;
> + int size, rc = 0;
> +
> +#if !defined(CONFIG_OF_UNITTEST)
> + if (of_root)
> + return 0;
> +#endif
> + size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
> +
> + fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> + if (!fdt)
> + return -ENOMEM;
> +
> + fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
> + memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
> +
> + if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
> + NULL, &dt)) {
> + pr_warn("%s: unflatten default tree failed\n", __func__);
> + kfree(fdt);
> + return -ENODATA;
> + }
> + if (!dt) {
> + pr_warn("%s: empty default tree\n", __func__);
> + kfree(fdt);
> + return -ENODATA;
> + }
> +
> + /*
> + * This lock normally encloses of_resolve_phandles()
> + */
> + of_overlay_mutex_lock();
> +
> + rc = of_resolve_phandles(dt);
> + if (rc) {
> + pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> + goto failed;
> + }
> +
> + if (!of_root) {
> + prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
> + if (!prop) {
> + rc = -ENOMEM;
> + goto failed;
> + }
> + val = kzalloc(sizeof(*val), GFP_KERNEL);
> + if (!val) {
> + rc = -ENOMEM;
> + goto failed;
> + }
> + *val = cpu_to_be32(sizeof(void *) / sizeof(u32));
> +
> + prop->name = "#address-cells";
> + prop->value = val;
> + prop->length = sizeof(u32);
> + of_add_property(dt, prop);
> + prop++;
> + prop->name = "#size-cells";
> + prop->value = val;
> + prop->length = sizeof(u32);
> + of_add_property(dt, prop);
> + of_root = dt;
> + for_each_of_allnodes(np)
> + __of_attach_node_sysfs(np);
> + of_aliases = of_find_node_by_path("/aliases");
> + of_chosen = of_find_node_by_path("/chosen");
> + of_overlay_mutex_unlock();
> +pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
> + return 0;
> + }
> +
> + unittest_data_add(dt);

It's confusing to me. If we need to share some functions with unittest,
make a new clearly defined (and named) function.

> +
> + of_overlay_mutex_unlock();
> +
> + return 0;
> +
> +failed:
> + of_overlay_mutex_unlock();
> + kfree(val);
> + kfree(prop);
> + return rc;
> +}
> +pure_initcall(of_fdt_root_init);

Is it better we have a new Kconfig option for the empty tree creation.

> +
> /* Everything below here references initial_boot_params directly. */
> int __initdata dt_root_addr_cells;
> int __initdata dt_root_size_cells;
> diff --git a/drivers/of/fdt_default.dts b/drivers/of/fdt_default.dts
> new file mode 100644
> index 000000000000..d1f12a76dfc6
> --- /dev/null
> +++ b/drivers/of/fdt_default.dts
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +/ {
> +};
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 631489f7f8c0..1ef93bccfdba 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -41,6 +41,18 @@ extern struct mutex of_mutex;
> extern struct list_head aliases_lookup;
> extern struct kset *of_kset;
>
> +#if defined(CONFIG_OF_UNITTEST)
> +extern u8 __dtb_testcases_begin[];
> +extern u8 __dtb_testcases_end[];
> +#define __dtb_fdt_default_begin __dtb_testcases_begin
> +#define __dtb_fdt_default_end __dtb_testcases_end

Maybe we don't have to use the test dt data, stick to the default empty
fdt is fine?

> +void __init unittest_data_add(struct device_node *dt);
> +#else
> +extern u8 __dtb_fdt_default_begin[];
> +extern u8 __dtb_fdt_default_end[];
> +static inline void unittest_data_add(struct device_node *dt) {}
> +#endif
> +
> #if defined(CONFIG_OF_DYNAMIC)
> extern int of_property_notify(int action, struct device_node *np,
> struct property *prop, struct property *old_prop);
> @@ -84,6 +96,11 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}
>
> #if defined(CONFIG_OF_RESOLVE)
> int of_resolve_phandles(struct device_node *tree);
> +#else
> +static inline int of_resolve_phandles(struct device_node *tree)
> +{
> + return 0;
> +}

If we have an empty of_resolve_phandles, does the empty tree creation
still works? Or if we don't need this func, just delete in the code.

Thanks,
Yilun

> #endif
>
> void __of_phandle_cache_inv_entry(phandle handle);
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 8c056972a6dd..745f455235cc 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -1402,73 +1402,15 @@ static void attach_node_and_children(struct device_node *np)
> * unittest_data_add - Reads, copies data from
> * linked tree and attaches it to the live tree
> */
> -static int __init unittest_data_add(void)
> +void __init unittest_data_add(struct device_node *dt)
> {
> - void *unittest_data;
> - void *unittest_data_align;
> - struct device_node *unittest_data_node = NULL, *np;
> - /*
> - * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
> - * created by cmd_dt_S_dtb in scripts/Makefile.lib
> - */
> - extern uint8_t __dtb_testcases_begin[];
> - extern uint8_t __dtb_testcases_end[];
> - const int size = __dtb_testcases_end - __dtb_testcases_begin;
> - int rc;
> - void *ret;
> -
> - if (!size) {
> - pr_warn("%s: testcases is empty\n", __func__);
> - return -ENODATA;
> - }
> -
> - /* creating copy */
> - unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> - if (!unittest_data)
> - return -ENOMEM;
> -
> - unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> - memcpy(unittest_data_align, __dtb_testcases_begin, size);
> -
> - ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
> - if (!ret) {
> - pr_warn("%s: unflatten testcases tree failed\n", __func__);
> - kfree(unittest_data);
> - return -ENODATA;
> - }
> - if (!unittest_data_node) {
> - pr_warn("%s: testcases tree is empty\n", __func__);
> - kfree(unittest_data);
> - return -ENODATA;
> - }
> -
> - /*
> - * This lock normally encloses of_resolve_phandles()
> - */
> - of_overlay_mutex_lock();
> -
> - rc = of_resolve_phandles(unittest_data_node);
> - if (rc) {
> - pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> - of_overlay_mutex_unlock();
> - return -EINVAL;
> - }
> -
> - if (!of_root) {
> - of_root = unittest_data_node;
> - for_each_of_allnodes(np)
> - __of_attach_node_sysfs(np);
> - of_aliases = of_find_node_by_path("/aliases");
> - of_chosen = of_find_node_by_path("/chosen");
> - of_overlay_mutex_unlock();
> - return 0;
> - }
> + struct device_node *np;
>
> EXPECT_BEGIN(KERN_INFO,
> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
>
> /* attach the sub-tree to live tree */
> - np = unittest_data_node->child;
> + np = dt->child;
> while (np) {
> struct device_node *next = np->sibling;
>
> @@ -1479,10 +1421,6 @@ static int __init unittest_data_add(void)
>
> EXPECT_END(KERN_INFO,
> "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
> -
> - of_overlay_mutex_unlock();
> -
> - return 0;
> }
>
> #ifdef CONFIG_OF_OVERLAY
> @@ -3258,7 +3196,6 @@ static inline __init void of_unittest_overlay_high_level(void) {}
> static int __init of_unittest(void)
> {
> struct device_node *np;
> - int res;
>
> pr_info("start of unittest - you will see error messages\n");
>
> @@ -3267,9 +3204,6 @@ static int __init of_unittest(void)
> if (IS_ENABLED(CONFIG_UML))
> unittest_unflatten_overlay_base();
>
> - res = unittest_data_add();
> - if (res)
> - return res;
> if (!of_aliases)
> of_aliases = of_find_node_by_path("/aliases");
>
> --
> 2.27.0

Next message: Nicholas Piggin: "Re: [PATCH v2 1/2] powerpc: Fix virt_addr_valid() check"
Previous message: Reiji Watanabe: "Re: [RFC PATCH v3 03/11] KVM: Introduce KVM_CAP_ARM_HVC_FW_REG_BMAP"
In reply to: Lizhi Hou: "Re: [PATCH V4 XRT Alveo Infrastructure 3/5] of: create empty of root"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]