diff mbox series

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

Message ID 20220105225013.1567871-4-lizhi.hou@xilinx.com (mailing list archive)
State New
Headers show
Series XRT Alveo driver infrastructure overview | expand

Commit Message

Lizhi Hou Jan. 5, 2022, 10:50 p.m. UTC
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@xilinx.com>
Signed-off-by: Max Zhen <max.zhen@xilinx.com>
Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
---
 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

Comments

kernel test robot Jan. 8, 2022, 3:34 a.m. UTC | #1
Hi Lizhi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linux/master linus/master v5.16-rc8 next-20220107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lizhi-Hou/XRT-Alveo-driver-infrastructure-overview/20220106-065312
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-randconfig-m021-20220105 (https://download.01.org/0day-ci/archive/20220108/202201081100.CZieN1nu-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

New smatch warnings:
drivers/of/fdt.c:541 of_fdt_root_init() warn: inconsistent indenting

Old smatch warnings:
drivers/of/fdt.c:1313 early_init_dt_add_memory_arch() warn: unsigned 'base + size' is never less than zero.
drivers/of/fdt.c:1318 early_init_dt_add_memory_arch() warn: unsigned 'base' is never less than zero.
drivers/of/fdt.c:1428 unflatten_and_copy_device_tree() warn: should '1 << (((__builtin_constant_p((((((7 * 4) + 4) + 4) + 4)) - 1)) ?((((((((7 * 4) + 4) + 4) + 4)) - 1) < 2) ?0:63 - __builtin_clzll((((((7 * 4) + 4) + 4) + 4)) - 1)):((4 <= 4)) ?__ilog2_u32((((((7 * 4) + 4) + 4) + 4)) - 1):__ilog2_u64((((((7 * 4) + 4) + 4) + 4)) - 1)) + 1)' be a 64 bit type?

vim +541 drivers/of/fdt.c

   468	
   469	static int __init of_fdt_root_init(void)
   470	{
   471		struct device_node *dt = NULL, *np;
   472		void *fdt = NULL, *fdt_aligned;
   473		struct property *prop = NULL;
   474		__be32 *val = NULL;
   475		int size, rc = 0;
   476	
   477	#if !defined(CONFIG_OF_UNITTEST)
   478		if (of_root)
   479			return 0;
   480	#endif
   481		size = __dtb_fdt_default_end - __dtb_fdt_default_begin;
   482	
   483		fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
   484		if (!fdt)
   485			return -ENOMEM;
   486	
   487		fdt_aligned = PTR_ALIGN(fdt, FDT_ALIGN_SIZE);
   488		memcpy(fdt_aligned, __dtb_fdt_default_begin, size);
   489	
   490		if (!of_fdt_unflatten_tree((const unsigned long *)fdt_aligned,
   491					   NULL, &dt)) {
   492			pr_warn("%s: unflatten default tree failed\n", __func__);
   493			kfree(fdt);
   494			return -ENODATA;
   495		}
   496		if (!dt) {
   497			pr_warn("%s: empty default tree\n", __func__);
   498			kfree(fdt);
   499			return -ENODATA;
   500		}
   501	
   502		/*
   503		 * This lock normally encloses of_resolve_phandles()
   504		 */
   505		of_overlay_mutex_lock();
   506	
   507		rc = of_resolve_phandles(dt);
   508		if (rc) {
   509			pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
   510			goto failed;
   511		}
   512	
   513		if (!of_root) {
   514			prop = kcalloc(2, sizeof(*prop), GFP_KERNEL);
   515			if (!prop) {
   516				rc = -ENOMEM;
   517				goto failed;
   518			}
   519			val = kzalloc(sizeof(*val), GFP_KERNEL);
   520			if (!val) {
   521				rc = -ENOMEM;
   522				goto failed;
   523			}
   524			*val = cpu_to_be32(sizeof(void *) / sizeof(u32));
   525	
   526			prop->name = "#address-cells";
   527			prop->value = val;
   528			prop->length = sizeof(u32);
   529			of_add_property(dt, prop);
   530			prop++;
   531			prop->name = "#size-cells";
   532			prop->value = val;
   533			prop->length = sizeof(u32);
   534			of_add_property(dt, prop);
   535			of_root = dt;
   536			for_each_of_allnodes(np)
   537				__of_attach_node_sysfs(np);
   538			of_aliases = of_find_node_by_path("/aliases");
   539			of_chosen = of_find_node_by_path("/chosen");
   540			of_overlay_mutex_unlock();
 > 541	pr_info("OF ROOT FLAG %lx\n", of_root->_flags);
   542			return 0;
   543		}
   544	
   545		unittest_data_add(dt);
   546	
   547		of_overlay_mutex_unlock();
   548	
   549		return 0;
   550	
   551	failed:
   552		of_overlay_mutex_unlock();
   553		kfree(val);
   554		kfree(prop);
   555		return rc;
   556	}
   557	pure_initcall(of_fdt_root_init);
   558	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Tom Rix Jan. 9, 2022, 6:39 p.m. UTC | #2
On 1/5/22 2:50 PM, 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.

Looks like this patch is doing two things, creating the empty node and 
refactoring the unit tests.

This patch should be split.

It is not clear why the unit test should be refactored.

Tom

>
> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>
> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
> ---
>   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
> +
extra lf, remove
>   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
> +
>   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);
> +
> +	of_overlay_mutex_unlock();
> +
> +	return 0;
> +
> +failed:
> +	of_overlay_mutex_unlock();
> +	kfree(val);
> +	kfree(prop);
> +	return rc;
> +}
> +pure_initcall(of_fdt_root_init);
> +
>   /* 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
> +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;
> +}
>   #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");
>
Lizhi Hou Jan. 11, 2022, 1:14 a.m. UTC | #3
Hi Rob,

We have modified the required device tree change according to your 
latest comments.
https://lore.kernel.org/lkml/YaWE%2F2ikgpXi2hzY@robh.at.kernel.org/
https://lore.kernel.org/lkml/YaWFksVvfQQWqKcG@robh.at.kernel.org/
Waiting for your feedback on the design and code changes.

Tom, thanks for your suggestions. Will incorporate your feedback in the 
next patch series.

Thanks,
Lizhi

On 1/9/22 10:39 AM, Tom Rix wrote:
>
> On 1/5/22 2:50 PM, 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.
>
> Looks like this patch is doing two things, creating the empty node and
> refactoring the unit tests.
>
> This patch should be split.
>
> It is not clear why the unit test should be refactored.
>
> Tom
>
>>
>> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>
>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
>> ---
>>   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
>> +
> extra lf, remove
>>   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
>> +
>>   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);
>> +
>> +     of_overlay_mutex_unlock();
>> +
>> +     return 0;
>> +
>> +failed:
>> +     of_overlay_mutex_unlock();
>> +     kfree(val);
>> +     kfree(prop);
>> +     return rc;
>> +}
>> +pure_initcall(of_fdt_root_init);
>> +
>>   /* 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
>> +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;
>> +}
>>   #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");
>>
>
Xu Yilun Jan. 11, 2022, 4:29 a.m. UTC | #4
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@xilinx.com>
> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
> ---
>  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
Lizhi Hou Jan. 19, 2022, 6:59 p.m. UTC | #5
Hi Yilun,

Thanks for your comments. Overall, we made the code change based on 
Rob's comments on previous patch set. Please see my inline comments for 
detail.

Rob, please provide your guidance here.

On 1/10/22 8:29 PM, Xu Yilun wrote:
> 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@xilinx.com>
>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
>> ---
>>   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.
Will remove.
>
>>   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?

Based on Rob's comment in 
https://lore.kernel.org/lkml/YaWFksVvfQQWqKcG@robh.at.kernel.org/, it 
needs to have a unified code to set of_root with or without 
CONFIG_OF_UNITTEST defined.  So the unified code works as this during boot

1. With CONFIG_OF_UNITEST define, of_root is set to base tree 
defined/compiled in testcases.dtb.o

2. Without CONFIG_OF_UNITEST, of_root is set to base tree 
defined/compiled in fdt_default.dtb.o

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

unittest_data_add() is not shared function. If CONFIG_OF_UNITTEST is not 
defined, this is a null function (please see of_private.h). I just 
followed the existing code style. e.g. of_property_notify() in of_private.h.

Would adding some comments to describe this be good enough?

>
>> +
>> +     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.
Sure, if needed.
>
>> +
>>   /* 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?

I am not sure I understand the point. test dt data contains a lot test 
nodes and we should not create those nodes if CONFIG_OF_UNITTEST is not 
defined.

Are you asking that we create empty of_root here and test nodes are 
created in unittest.c? I believe that was we tried to do with previous 
patch. Is this the same ask within your second comment?

We are open to change it back as previous patch does if needed.

Rob, do you have any comment here?

>
>> +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.

test nodes creation requires of_resolve_phandles() and creating empty 
of_root does not. This define is added for unifying the code.


Thanks,

Lizhi

>
> 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
Xu Yilun Jan. 21, 2022, 1:42 a.m. UTC | #6
On Wed, Jan 19, 2022 at 10:59:48AM -0800, Lizhi Hou wrote:
> Hi Yilun,
> 
> Thanks for your comments. Overall, we made the code change based on Rob's
> comments on previous patch set. Please see my inline comments for detail.
> 
> Rob, please provide your guidance here.
> 
> On 1/10/22 8:29 PM, Xu Yilun wrote:
> > 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@xilinx.com>
> > > Signed-off-by: Max Zhen <max.zhen@xilinx.com>
> > > Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
> > > ---
> > >   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.
> Will remove.
> > 
> > >   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?
> 
> Based on Rob's comment in
> https://lore.kernel.org/lkml/YaWFksVvfQQWqKcG@robh.at.kernel.org/, it needs
> to have a unified code to set of_root with or without CONFIG_OF_UNITTEST
> defined.  So the unified code works as this during boot
> 
> 1. With CONFIG_OF_UNITEST define, of_root is set to base tree
> defined/compiled in testcases.dtb.o
> 
> 2. Without CONFIG_OF_UNITEST, of_root is set to base tree defined/compiled
> in fdt_default.dtb.o
> 
> > 
> > >   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.
> 
> unittest_data_add() is not shared function. If CONFIG_OF_UNITTEST is not
> defined, this is a null function (please see of_private.h). I just followed
> the existing code style. e.g. of_property_notify() in of_private.h.
> 
> Would adding some comments to describe this be good enough?
> 
> > 
> > > +
> > > +     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.
> Sure, if needed.
> > 
> > > +
> > >   /* 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?
> 
> I am not sure I understand the point. test dt data contains a lot test nodes
> and we should not create those nodes if CONFIG_OF_UNITTEST is not defined.
> 
> Are you asking that we create empty of_root here and test nodes are created
> in unittest.c? I believe that was we tried to do with previous patch. Is
> this the same ask within your second comment?

Yes, generally this is what I mean. I think you may have some
misunderstanding for Rob's comments. My understanding is, to move the code
from unittest to the of core, refactor them and make clearly defined
functions, and let unittest call these functions.

It is generally not reasonable the core uses help functions or test data
from unittest.

> 
> We are open to change it back as previous patch does if needed.
> 
> Rob, do you have any comment here?
> 
> > 
> > > +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.
> 
> test nodes creation requires of_resolve_phandles() and creating empty
> of_root does not. This define is added for unifying the code.

If you don't need the of_resolve_phandles in core code, don't call it.

Thanks,
Yilun

> 
> 
> Thanks,
> 
> Lizhi
> 
> > 
> > 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
Lizhi Hou Jan. 21, 2022, 4:54 p.m. UTC | #7
Hi Yilun,

Thanks for your comments. To make the change simple and clear, I will 
create a patch just for creating empty of_root and submit it to device 
tree subsystem. I will CC you.

Lizhi

On 1/20/22 5:42 PM, Xu Yilun wrote:
>
> On Wed, Jan 19, 2022 at 10:59:48AM -0800, Lizhi Hou wrote:
>> Hi Yilun,
>>
>> Thanks for your comments. Overall, we made the code change based on Rob's
>> comments on previous patch set. Please see my inline comments for detail.
>>
>> Rob, please provide your guidance here.
>>
>> On 1/10/22 8:29 PM, Xu Yilun wrote:
>>> 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@xilinx.com>
>>>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
>>>> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
>>>> ---
>>>>    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.
>> Will remove.
>>>>    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?
>> Based on Rob's comment in
>> https://lore.kernel.org/lkml/YaWFksVvfQQWqKcG@robh.at.kernel.org/, it needs
>> to have a unified code to set of_root with or without CONFIG_OF_UNITTEST
>> defined.  So the unified code works as this during boot
>>
>> 1. With CONFIG_OF_UNITEST define, of_root is set to base tree
>> defined/compiled in testcases.dtb.o
>>
>> 2. Without CONFIG_OF_UNITEST, of_root is set to base tree defined/compiled
>> in fdt_default.dtb.o
>>
>>>>    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.
>> unittest_data_add() is not shared function. If CONFIG_OF_UNITTEST is not
>> defined, this is a null function (please see of_private.h). I just followed
>> the existing code style. e.g. of_property_notify() in of_private.h.
>>
>> Would adding some comments to describe this be good enough?
>>
>>>> +
>>>> +     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.
>> Sure, if needed.
>>>> +
>>>>    /* 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?
>> I am not sure I understand the point. test dt data contains a lot test nodes
>> and we should not create those nodes if CONFIG_OF_UNITTEST is not defined.
>>
>> Are you asking that we create empty of_root here and test nodes are created
>> in unittest.c? I believe that was we tried to do with previous patch. Is
>> this the same ask within your second comment?
> Yes, generally this is what I mean. I think you may have some
> misunderstanding for Rob's comments. My understanding is, to move the code
> from unittest to the of core, refactor them and make clearly defined
> functions, and let unittest call these functions.
>
> It is generally not reasonable the core uses help functions or test data
> from unittest.
>
>> We are open to change it back as previous patch does if needed.
>>
>> Rob, do you have any comment here?
>>
>>>> +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.
>> test nodes creation requires of_resolve_phandles() and creating empty
>> of_root does not. This define is added for unifying the code.
> If you don't need the of_resolve_phandles in core code, don't call it.
>
> Thanks,
> Yilun
>
>>
>> Thanks,
>>
>> Lizhi
>>
>>> 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
diff mbox series

Patch

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
+
 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
+
 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);
+
+	of_overlay_mutex_unlock();
+
+	return 0;
+
+failed:
+	of_overlay_mutex_unlock();
+	kfree(val);
+	kfree(prop);
+	return rc;
+}
+pure_initcall(of_fdt_root_init);
+
 /* 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
+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;
+}
 #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");