[PATCH 00/13] of: overlay/unittest: Miscellaneous fixes and improvements

[PATCH 00/13] of: overlay/unittest: Miscellaneous fixes and improvements

[Geert Uytterhoeven]

	Hi all,

This patch series contains miscellaneous fixes and improvements for
dynamic DT overlays and the related unit tests.

The first two patches are fixes for a lock-up and a crash.
The remaining patches are smaller fixes, enhancements and cleanups for
the overlay tests, including one new test.

I ran into the crash when accidentally loading the wrong overlay (using
the out-of-tree DT overlay configfs[1]), and removing it afterwards.
As this case was not yet covered by the unittests, I added a test.
I enhanced the tests to clean up partial state after a failed
overlay apply attempt, which triggered the lock-up.

Thanks for your comments!

[1] https://elinux.org/R-Car/DT-Overlays

Geert Uytterhoeven (13):
  of: dynamic: Do not use "%pOF" while holding devtree_lock
  of: overlay: Call of_changeset_init() early
  of: unittest: Fix overlay type in apply/revert check
  of: unittest: Restore indentation in overlay_bad_add_dup_prop test
  of: unittest: Improve messages and comments in apply/revert checks
  of: unittest: Merge of_unittest_apply{,_revert}_overlay_check()
  of: unittest: Cleanup partially-applied overlays
  of: unittest: Add separators to of_unittest_overlay_high_level()
  of: overlay: unittest: Add test for unresolved symbol
  of: unittest-data: Convert remaining overlay DTS files to sugar syntax
  of: unittest-data: Fix whitespace - blank lines
  of: unittest-data: Fix whitespace - indentation
  of: unittest-data: Fix whitespace - angular brackets

 drivers/of/dynamic.c                          |  12 +-
 drivers/of/overlay.c                          |   3 +-
 drivers/of/unittest-data/Makefile             |   3 +-
 drivers/of/unittest-data/overlay.dtso         |  32 ++-
 drivers/of/unittest-data/overlay_0.dtso       |  11 +-
 drivers/of/unittest-data/overlay_1.dtso       |  11 +-
 drivers/of/unittest-data/overlay_11.dtso      |   1 -
 drivers/of/unittest-data/overlay_12.dtso      |  11 +-
 drivers/of/unittest-data/overlay_13.dtso      |  11 +-
 drivers/of/unittest-data/overlay_15.dtso      |   1 +
 drivers/of/unittest-data/overlay_4.dtso       |   1 -
 .../overlay_bad_add_dup_node.dtso             |   9 +-
 .../overlay_bad_add_dup_prop.dtso             |   9 +-
 .../of/unittest-data/overlay_bad_phandle.dtso |   5 +-
 .../of/unittest-data/overlay_bad_symbol.dtso  |   5 +-
 .../unittest-data/overlay_bad_unresolved.dtso |   7 +
 drivers/of/unittest-data/overlay_common.dtsi  |  36 ++-
 drivers/of/unittest-data/overlay_gpio_01.dtso |   1 +
 .../of/unittest-data/overlay_gpio_02a.dtso    |   1 +
 .../of/unittest-data/overlay_gpio_02b.dtso    |   1 +
 drivers/of/unittest-data/overlay_gpio_03.dtso |   1 +
 .../of/unittest-data/overlay_gpio_04a.dtso    |   1 +
 .../of/unittest-data/overlay_gpio_04b.dtso    |   1 +
 .../of/unittest-data/testcases_common.dtsi    |   1 +
 .../of/unittest-data/tests-interrupts.dtsi    |   1 +
 drivers/of/unittest-data/tests-overlay.dtsi   |   1 -
 drivers/of/unittest-data/tests-phandle.dtsi   |   2 +
 drivers/of/unittest.c                         | 228 +++++++++++-------
 28 files changed, 225 insertions(+), 182 deletions(-)
 create mode 100644 drivers/of/unittest-data/overlay_bad_unresolved.dtso

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

[Geert Uytterhoeven]

Formatting strings using "%pOF" while holding devtree_lock causes a
deadlock.  Lockdep reports:

    of_get_parent from of_fwnode_get_parent+0x18/0x24
    ^^^^^^^^^^^^^
    of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
    fwnode_count_parents from fwnode_full_name_string+0x18/0xac
    fwnode_full_name_string from device_node_string+0x1a0/0x404
    device_node_string from pointer+0x3c0/0x534
    pointer from vsnprintf+0x248/0x36c
    vsnprintf from vprintk_store+0x130/0x3b4

Fix this by making the locking cover only the parts that really need it.

Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/dynamic.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b1705306..eae45a1c673ee05f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 
 	__of_changeset_entry_dump(ce);
 
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		__of_attach_node(ce->np);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		break;
 	case OF_RECONFIG_DETACH_NODE:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		__of_detach_node(ce->np);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		break;
 	case OF_RECONFIG_ADD_PROPERTY:
 		/* If the property is in deadprops then it must be removed */
@@ -619,7 +622,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 			}
 		}
 
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		ret = __of_add_property(ce->np, ce->prop);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		if (ret) {
 			pr_err("changeset: add_property failed @%pOF/%s\n",
 				ce->np,
@@ -628,7 +633,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		}
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		ret = __of_remove_property(ce->np, ce->prop);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		if (ret) {
 			pr_err("changeset: remove_property failed @%pOF/%s\n",
 				ce->np,
@@ -647,7 +654,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 			}
 		}
 
+		raw_spin_lock_irqsave(&devtree_lock, flags);
 		ret = __of_update_property(ce->np, ce->prop, &old_prop);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		if (ret) {
 			pr_err("changeset: update_property failed @%pOF/%s\n",
 				ce->np,
@@ -658,7 +667,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	default:
 		ret = -EINVAL;
 	}
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH 02/13] of: overlay: Call of_changeset_init() early

[Geert Uytterhoeven]

When of_overlay_fdt_apply() fails, the changeset may be partially
applied, and the caller is still expected to call of_overlay_remove() to
clean up this partial state.

However, of_overlay_apply() calls of_resolve_phandles() before
init_overlay_changeset().  Hence if the overlay fails to apply due to an
unresolved symbol, the overlay_changeset.cset.entries list is still
uninitialized, and cleanup will crash with a NULL-pointer dereference in
overlay_removal_is_ok().

Fix this by moving the call to of_changeset_init() from
init_overlay_changeset() to of_overlay_fdt_apply(), where all other
early initialization is done.

Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/overlay.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7feb643f13707d34..28b479afd506fdb3 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -752,8 +752,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs)
 	if (!of_node_is_root(ovcs->overlay_root))
 		pr_debug("%s() ovcs->overlay_root is not root\n", __func__);
 
-	of_changeset_init(&ovcs->cset);
-
 	cnt = 0;
 
 	/* fragment nodes */
@@ -1013,6 +1011,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 	list_add_tail(&ovcs->ovcs_list, &ovcs_list);
+	of_changeset_init(&ovcs->cset);
 
 	/*
 	 * Must create permanent copy of FDT because of_fdt_unflatten_tree()
-- 
2.34.1

[PATCH 03/13] of: unittest: Fix overlay type in apply/revert check

[Geert Uytterhoeven]

The removal check in of_unittest_apply_revert_overlay_check()
always uses the platform device overlay type, while it should use the
actual overlay type, as passed as a parameter to the function.

This has no impact on any current test, as all tests calling
of_unittest_apply_revert_overlay_check() use the platform device overlay
type.

Fixes: d5e75500ca401d31 ("of: unitest: Add I2C overlay unit tests.")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index a406a12eb20804d6..840f26477f77f622 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2180,7 +2180,7 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 	of_unittest_untrack_overlay(save_ovcs_id);
 
 	/* unittest device must be again in before state */
-	if (of_unittest_device_exists(unittest_nr, PDEV_OVERLAY) != before) {
+	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
 		unittest(0, "%s with device @\"%s\" %s\n",
 				overlay_name_from_nr(overlay_nr),
 				unittest_path(unittest_nr, ovtype),
-- 
2.34.1

[PATCH 04/13] of: unittest: Restore indentation in overlay_bad_add_dup_prop test

[Geert Uytterhoeven]

When updating the expected error messages for the
overlay_bad_add_dup_prop test, indentation of the EXPECT_END()
statements was accidentally changed.

Fixes: 29acfb65598f9167 ("of: unittest: kmemleak in duplicate property update")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 840f26477f77f622..bb7e2ec05da59070 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3687,11 +3687,11 @@ static __init void of_unittest_overlay_high_level(void)
 		 "Adding overlay 'overlay_bad_add_dup_prop' failed\n");
 
 	EXPECT_END(KERN_ERR,
-		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/name");
+		   "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/name");
 	EXPECT_END(KERN_ERR,
-		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/rpm_avail");
+		   "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/rpm_avail");
 	EXPECT_END(KERN_ERR,
-		     "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/electric");
+		   "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/electric");
 
 	unittest(overlay_data_apply("overlay_bad_phandle", NULL),
 		 "Adding overlay 'overlay_bad_phandle' failed\n");
-- 
2.34.1

[PATCH 05/13] of: unittest: Improve messages and comments in apply/revert checks

[Geert Uytterhoeven]

Miscellaneous improvements for the apply and apply/revert checks,
making them more similar:
  - Fix inverted comment for before state check,
  - Add more comments to improve symmetry,
  - Fix grammar s/must be to set to/must be in/,
  - Avoid saying "create" in messages, as the actual operation depends
    on the value of the before/after parameters.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index bb7e2ec05da59070..42abfcd0cdffa4af 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2108,7 +2108,7 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
 {
 	int ret, ovcs_id;
 
-	/* unittest device must not be in before state */
+	/* unittest device must be in before state */
 	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
 		unittest(0, "%s with device @\"%s\" %s\n",
 				overlay_name_from_nr(overlay_nr),
@@ -2117,6 +2117,7 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
 		return -EINVAL;
 	}
 
+	/* apply the overlay */
 	ovcs_id = 0;
 	ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
 	if (ret != 0) {
@@ -2124,9 +2125,9 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
 		return ret;
 	}
 
-	/* unittest device must be to set to after state */
+	/* unittest device must be in after state */
 	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
-		unittest(0, "%s failed to create @\"%s\" %s\n",
+		unittest(0, "%s with device @\"%s\" %s\n",
 				overlay_name_from_nr(overlay_nr),
 				unittest_path(unittest_nr, ovtype),
 				!after ? "enabled" : "disabled");
@@ -2162,13 +2163,14 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 
 	/* unittest device must be in after state */
 	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
-		unittest(0, "%s failed to create @\"%s\" %s\n",
+		unittest(0, "%s with device @\"%s\" %s\n",
 				overlay_name_from_nr(overlay_nr),
 				unittest_path(unittest_nr, ovtype),
 				!after ? "enabled" : "disabled");
 		return -EINVAL;
 	}
 
+	/* remove the overlay */
 	save_ovcs_id = ovcs_id;
 	ret = of_overlay_remove(&ovcs_id);
 	if (ret != 0) {
-- 
2.34.1

[PATCH 06/13] of: unittest: Merge of_unittest_apply{,_revert}_overlay_check()

[Geert Uytterhoeven]

of_unittest_apply_overlay_check() and the first part of
of_unittest_apply_revert_overlay_check() are identical.
Reduce code duplication by replacing them by two wrappers around a
common helper.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest.c | 61 ++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 42abfcd0cdffa4af..4c21c8ce99b9d8ff 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2101,46 +2101,10 @@ static int __init of_unittest_apply_overlay(int overlay_nr, int *ovcs_id)
 	return 0;
 }
 
-/* apply an overlay while checking before and after states */
-static int __init of_unittest_apply_overlay_check(int overlay_nr,
-		int unittest_nr, int before, int after,
-		enum overlay_type ovtype)
-{
-	int ret, ovcs_id;
-
-	/* unittest device must be in before state */
-	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
-		unittest(0, "%s with device @\"%s\" %s\n",
-				overlay_name_from_nr(overlay_nr),
-				unittest_path(unittest_nr, ovtype),
-				!before ? "enabled" : "disabled");
-		return -EINVAL;
-	}
-
-	/* apply the overlay */
-	ovcs_id = 0;
-	ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
-	if (ret != 0) {
-		/* of_unittest_apply_overlay already called unittest() */
-		return ret;
-	}
-
-	/* unittest device must be in after state */
-	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
-		unittest(0, "%s with device @\"%s\" %s\n",
-				overlay_name_from_nr(overlay_nr),
-				unittest_path(unittest_nr, ovtype),
-				!after ? "enabled" : "disabled");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-/* apply an overlay and then revert it while checking before, after states */
-static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
+/* apply an overlay and optionally revert it while checking states */
+static int __init __of_unittest_apply_revert_overlay_check(int overlay_nr,
 		int unittest_nr, int before, int after,
-		enum overlay_type ovtype)
+		enum overlay_type ovtype, bool revert)
 {
 	int ret, ovcs_id, save_ovcs_id;
 
@@ -2170,6 +2134,9 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 		return -EINVAL;
 	}
 
+	if (!revert)
+		return 0;
+
 	/* remove the overlay */
 	save_ovcs_id = ovcs_id;
 	ret = of_overlay_remove(&ovcs_id);
@@ -2193,6 +2160,22 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 	return 0;
 }
 
+/* apply an overlay while checking before and after states */
+static inline int __init of_unittest_apply_overlay_check(int overlay_nr,
+	int unittest_nr, int before, int after, enum overlay_type ovtype)
+{
+	return __of_unittest_apply_revert_overlay_check(overlay_nr,
+				unittest_nr, before, after, ovtype, false);
+}
+
+/* apply an overlay and then revert it while checking before, after states */
+static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
+	int unittest_nr, int before, int after, enum overlay_type ovtype)
+{
+	return __of_unittest_apply_revert_overlay_check(overlay_nr,
+				unittest_nr, before, after, ovtype, true);
+}
+
 /* test activation of device */
 static void __init of_unittest_overlay_0(void)
 {
-- 
2.34.1

[PATCH 07/13] of: unittest: Cleanup partially-applied overlays

[Geert Uytterhoeven]

When of_overlay_fdt_apply() fails, the changeset may be partially
applied, and the caller is still expected to call of_overlay_remove() to
clean up this partial state.  However, overlay_17 is the only test that
takes care of cleaning up after an (expected) failure.

Instead of adding cleanup code to each individual test, extend
overlay_info with the optional expected return value of
of_overlay_remove(), and handle cleanup in the overlay_data_apply()
helper.  While at it, simplify the end marker in the overlay_info table.

Update the expected error output for errors during the newly cleanup.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest.c | 130 +++++++++++++++++++++++++++---------------
 1 file changed, 83 insertions(+), 47 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 4c21c8ce99b9d8ff..86e109cc5a5385ba 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2956,12 +2956,6 @@ static void __init of_unittest_overlay_notify(void)
 
 	unittest(ovcs_id, "ovcs_id not created for overlay_17\n");
 
-	if (ovcs_id) {
-		ret = of_overlay_remove(&ovcs_id);
-		unittest(!ret,
-			"overlay_17 of_overlay_remove(), ret = %d\n", ret);
-	}
-
 	/* ---  overlay 18  --- */
 
 	unittest(overlay_data_apply("overlay_18", &ovcs_id),
@@ -3249,17 +3243,19 @@ static void __init of_unittest_lifecycle(void)
 	extern uint8_t __dtbo_##overlay_name##_begin[]; \
 	extern uint8_t __dtbo_##overlay_name##_end[]
 
-#define OVERLAY_INFO(overlay_name, expected) \
-{	.dtbo_begin       = __dtbo_##overlay_name##_begin, \
-	.dtbo_end         = __dtbo_##overlay_name##_end, \
-	.expected_result = expected, \
-	.name            = #overlay_name, \
+#define OVERLAY_INFO(overlay_name, expected, expected_remove) \
+{	.dtbo_begin		= __dtbo_##overlay_name##_begin, \
+	.dtbo_end		= __dtbo_##overlay_name##_end, \
+	.expected_result	= expected, \
+	.expected_result_remove	= expected_remove, \
+	.name			= #overlay_name, \
 }
 
 struct overlay_info {
 	uint8_t		*dtbo_begin;
 	uint8_t		*dtbo_end;
 	int		expected_result;
+	int		expected_result_remove;	/* if apply failed */
 	int		ovcs_id;
 	char		*name;
 };
@@ -3299,40 +3295,40 @@ OVERLAY_INFO_EXTERN(overlay_bad_symbol);
 
 /* entries found by name */
 static struct overlay_info overlays[] = {
-	OVERLAY_INFO(overlay_base, -9999),
-	OVERLAY_INFO(overlay, 0),
-	OVERLAY_INFO(overlay_0, 0),
-	OVERLAY_INFO(overlay_1, 0),
-	OVERLAY_INFO(overlay_2, 0),
-	OVERLAY_INFO(overlay_3, 0),
-	OVERLAY_INFO(overlay_4, 0),
-	OVERLAY_INFO(overlay_5, 0),
-	OVERLAY_INFO(overlay_6, 0),
-	OVERLAY_INFO(overlay_7, 0),
-	OVERLAY_INFO(overlay_8, 0),
-	OVERLAY_INFO(overlay_9, 0),
-	OVERLAY_INFO(overlay_10, 0),
-	OVERLAY_INFO(overlay_11, 0),
-	OVERLAY_INFO(overlay_12, 0),
-	OVERLAY_INFO(overlay_13, 0),
-	OVERLAY_INFO(overlay_15, 0),
-	OVERLAY_INFO(overlay_16, -EBUSY),
-	OVERLAY_INFO(overlay_17, -EEXIST),
-	OVERLAY_INFO(overlay_18, 0),
-	OVERLAY_INFO(overlay_19, 0),
-	OVERLAY_INFO(overlay_20, 0),
-	OVERLAY_INFO(overlay_gpio_01, 0),
-	OVERLAY_INFO(overlay_gpio_02a, 0),
-	OVERLAY_INFO(overlay_gpio_02b, 0),
-	OVERLAY_INFO(overlay_gpio_03, 0),
-	OVERLAY_INFO(overlay_gpio_04a, 0),
-	OVERLAY_INFO(overlay_gpio_04b, 0),
-	OVERLAY_INFO(overlay_bad_add_dup_node, -EINVAL),
-	OVERLAY_INFO(overlay_bad_add_dup_prop, -EINVAL),
-	OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
-	OVERLAY_INFO(overlay_bad_symbol, -EINVAL),
+	OVERLAY_INFO(overlay_base, -9999, 0),
+	OVERLAY_INFO(overlay, 0, 0),
+	OVERLAY_INFO(overlay_0, 0, 0),
+	OVERLAY_INFO(overlay_1, 0, 0),
+	OVERLAY_INFO(overlay_2, 0, 0),
+	OVERLAY_INFO(overlay_3, 0, 0),
+	OVERLAY_INFO(overlay_4, 0, 0),
+	OVERLAY_INFO(overlay_5, 0, 0),
+	OVERLAY_INFO(overlay_6, 0, 0),
+	OVERLAY_INFO(overlay_7, 0, 0),
+	OVERLAY_INFO(overlay_8, 0, 0),
+	OVERLAY_INFO(overlay_9, 0, 0),
+	OVERLAY_INFO(overlay_10, 0, 0),
+	OVERLAY_INFO(overlay_11, 0, 0),
+	OVERLAY_INFO(overlay_12, 0, 0),
+	OVERLAY_INFO(overlay_13, 0, 0),
+	OVERLAY_INFO(overlay_15, 0, 0),
+	OVERLAY_INFO(overlay_16, -EBUSY, 0),
+	OVERLAY_INFO(overlay_17, -EEXIST, 0),
+	OVERLAY_INFO(overlay_18, 0, 0),
+	OVERLAY_INFO(overlay_19, 0, 0),
+	OVERLAY_INFO(overlay_20, 0, 0),
+	OVERLAY_INFO(overlay_gpio_01, 0, 0),
+	OVERLAY_INFO(overlay_gpio_02a, 0, 0),
+	OVERLAY_INFO(overlay_gpio_02b, 0, 0),
+	OVERLAY_INFO(overlay_gpio_03, 0, 0),
+	OVERLAY_INFO(overlay_gpio_04a, 0, 0),
+	OVERLAY_INFO(overlay_gpio_04b, 0, 0),
+	OVERLAY_INFO(overlay_bad_add_dup_node, -EINVAL, -ENODEV),
+	OVERLAY_INFO(overlay_bad_add_dup_prop, -EINVAL, -ENODEV),
+	OVERLAY_INFO(overlay_bad_phandle, -EINVAL, 0),
+	OVERLAY_INFO(overlay_bad_symbol, -EINVAL, -ENODEV),
 	/* end marker */
-	{.dtbo_begin = NULL, .dtbo_end = NULL, .expected_result = 0, .name = NULL}
+	{ }
 };
 
 static struct device_node *overlay_base_root;
@@ -3427,8 +3423,9 @@ void __init unittest_unflatten_overlay_base(void)
 static int __init overlay_data_apply(const char *overlay_name, int *ovcs_id)
 {
 	struct overlay_info *info;
+	int passed = 1;
 	int found = 0;
-	int ret;
+	int ret, ret2;
 	u32 size;
 
 	for (info = overlays; info && info->name; info++) {
@@ -3455,11 +3452,24 @@ static int __init overlay_data_apply(const char *overlay_name, int *ovcs_id)
 	pr_debug("%s applied\n", overlay_name);
 
 out:
-	if (ret != info->expected_result)
+	if (ret != info->expected_result) {
 		pr_err("of_overlay_fdt_apply() expected %d, ret=%d, %s\n",
 		       info->expected_result, ret, overlay_name);
+		passed = 0;
+	}
+
+	if (ret < 0) {
+		/* changeset may be partially applied */
+		ret2 = of_overlay_remove(&info->ovcs_id);
+		if (ret2 != info->expected_result_remove) {
+			pr_err("of_overlay_remove() expected %d, ret=%d, %s\n",
+			       info->expected_result_remove, ret2,
+			       overlay_name);
+			passed = 0;
+		}
+	}
 
-	return (ret == info->expected_result);
+	return passed;
 }
 
 /*
@@ -3652,10 +3662,18 @@ static __init void of_unittest_overlay_high_level(void)
 		     "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller");
 	EXPECT_BEGIN(KERN_ERR,
 		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/controller/name");
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: changeset: remove_property failed @/testcase-data-2/substation@100/motor-1/controller/name");
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: Error reverting changeset (-19)");
 
 	unittest(overlay_data_apply("overlay_bad_add_dup_node", NULL),
 		 "Adding overlay 'overlay_bad_add_dup_node' failed\n");
 
+	EXPECT_END(KERN_ERR,
+		   "OF: Error reverting changeset (-19)");
+	EXPECT_END(KERN_ERR,
+		   "OF: changeset: remove_property failed @/testcase-data-2/substation@100/motor-1/controller/name");
 	EXPECT_END(KERN_ERR,
 		   "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/controller/name");
 	EXPECT_END(KERN_ERR,
@@ -3667,10 +3685,18 @@ static __init void of_unittest_overlay_high_level(void)
 		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/rpm_avail");
 	EXPECT_BEGIN(KERN_ERR,
 		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/name");
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: changeset: remove_property failed @/testcase-data-2/substation@100/motor-1/electric/name");
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: Error reverting changeset (-19)");
 
 	unittest(overlay_data_apply("overlay_bad_add_dup_prop", NULL),
 		 "Adding overlay 'overlay_bad_add_dup_prop' failed\n");
 
+	EXPECT_END(KERN_ERR,
+		   "OF: Error reverting changeset (-19)");
+	EXPECT_END(KERN_ERR,
+		   "OF: changeset: remove_property failed @/testcase-data-2/substation@100/motor-1/electric/name");
 	EXPECT_END(KERN_ERR,
 		   "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/name");
 	EXPECT_END(KERN_ERR,
@@ -3681,9 +3707,19 @@ static __init void of_unittest_overlay_high_level(void)
 	unittest(overlay_data_apply("overlay_bad_phandle", NULL),
 		 "Adding overlay 'overlay_bad_phandle' failed\n");
 
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: changeset: remove_property failed @/testcase-data-2/substation@100/hvac-medium-2/name");
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: resolver: overlay phandle fixup failed: -22");
+
 	unittest(overlay_data_apply("overlay_bad_symbol", NULL),
 		 "Adding overlay 'overlay_bad_symbol' failed\n");
 
+	EXPECT_END(KERN_ERR,
+		   "OF: resolver: overlay phandle fixup failed: -22");
+	EXPECT_END(KERN_ERR,
+		   "OF: changeset: remove_property failed @/testcase-data-2/substation@100/hvac-medium-2/name");
+
 	return;
 
 err_unlock:
-- 
2.34.1

[PATCH 08/13] of: unittest: Add separators to of_unittest_overlay_high_level()

[Geert Uytterhoeven]

It is hard to see the start and end of each individual test in
of_unittest_overlay_high_level().  Add visual cues in the form of
separator comments, like was done in of_unittest_overlay_notify().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 86e109cc5a5385ba..4e3d8f72f979918f 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3608,6 +3608,8 @@ static __init void of_unittest_overlay_high_level(void)
 
 	/* now do the normal overlay usage test */
 
+	/* ---  overlay  --- */
+
 	EXPECT_BEGIN(KERN_ERR,
 		     "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/status");
 	EXPECT_BEGIN(KERN_ERR,
@@ -3658,6 +3660,8 @@ static __init void of_unittest_overlay_high_level(void)
 
 	unittest(ret, "Adding overlay 'overlay' failed\n");
 
+	/* ---  overlay_bad_add_dup_node  --- */
+
 	EXPECT_BEGIN(KERN_ERR,
 		     "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller");
 	EXPECT_BEGIN(KERN_ERR,
@@ -3679,6 +3683,8 @@ static __init void of_unittest_overlay_high_level(void)
 	EXPECT_END(KERN_ERR,
 		   "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller");
 
+	/* ---  overlay_bad_add_dup_prop  --- */
+
 	EXPECT_BEGIN(KERN_ERR,
 		     "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/electric");
 	EXPECT_BEGIN(KERN_ERR,
@@ -3704,9 +3710,13 @@ static __init void of_unittest_overlay_high_level(void)
 	EXPECT_END(KERN_ERR,
 		   "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/electric");
 
+	/* ---  overlay_bad_phandle  --- */
+
 	unittest(overlay_data_apply("overlay_bad_phandle", NULL),
 		 "Adding overlay 'overlay_bad_phandle' failed\n");
 
+	/* ---  overlay_bad_symbol  --- */
+
 	EXPECT_BEGIN(KERN_ERR,
 		     "OF: changeset: remove_property failed @/testcase-data-2/substation@100/hvac-medium-2/name");
 	EXPECT_BEGIN(KERN_ERR,
-- 
2.34.1

[PATCH 09/13] of: overlay: unittest: Add test for unresolved symbol

[Geert Uytterhoeven]

Add a test to exercise the error paths when trying to apply an overlay
with an unresolved symbol and cleaning up the resulting partial state.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest-data/Makefile               |  3 ++-
 .../unittest-data/overlay_bad_unresolved.dtso   |  7 +++++++
 drivers/of/unittest.c                           | 17 +++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 drivers/of/unittest-data/overlay_bad_unresolved.dtso

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index ea5f4da68e23acd0..f79daa1d45713958 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -32,7 +32,8 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtbo.o \
 			    overlay_gpio_02b.dtbo.o \
 			    overlay_gpio_03.dtbo.o \
 			    overlay_gpio_04a.dtbo.o \
-			    overlay_gpio_04b.dtbo.o
+			    overlay_gpio_04b.dtbo.o \
+			    overlay_bad_unresolved.dtbo.o
 
 # enable creation of __symbols__ node
 DTC_FLAGS_overlay += -@
diff --git a/drivers/of/unittest-data/overlay_bad_unresolved.dtso b/drivers/of/unittest-data/overlay_bad_unresolved.dtso
new file mode 100644
index 0000000000000000..3b75a53ae8a492bd
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_bad_unresolved.dtso
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+&this_label_does_not_exist {
+	status = "ok";
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 4e3d8f72f979918f..8b6f746abfec8985 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3292,6 +3292,7 @@ OVERLAY_INFO_EXTERN(overlay_bad_add_dup_node);
 OVERLAY_INFO_EXTERN(overlay_bad_add_dup_prop);
 OVERLAY_INFO_EXTERN(overlay_bad_phandle);
 OVERLAY_INFO_EXTERN(overlay_bad_symbol);
+OVERLAY_INFO_EXTERN(overlay_bad_unresolved);
 
 /* entries found by name */
 static struct overlay_info overlays[] = {
@@ -3327,6 +3328,7 @@ static struct overlay_info overlays[] = {
 	OVERLAY_INFO(overlay_bad_add_dup_prop, -EINVAL, -ENODEV),
 	OVERLAY_INFO(overlay_bad_phandle, -EINVAL, 0),
 	OVERLAY_INFO(overlay_bad_symbol, -EINVAL, -ENODEV),
+	OVERLAY_INFO(overlay_bad_unresolved, -EINVAL, 0),
 	/* end marker */
 	{ }
 };
@@ -3730,6 +3732,21 @@ static __init void of_unittest_overlay_high_level(void)
 	EXPECT_END(KERN_ERR,
 		   "OF: changeset: remove_property failed @/testcase-data-2/substation@100/hvac-medium-2/name");
 
+	/* ---  overlay_bad_unresolved  --- */
+
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: resolver: node label 'this_label_does_not_exist' not found in live devicetree symbols table");
+	EXPECT_BEGIN(KERN_ERR,
+		     "OF: resolver: overlay phandle fixup failed: -22");
+
+	unittest(overlay_data_apply("overlay_bad_unresolved", NULL),
+		 "Adding overlay 'overlay_bad_unresolved' failed\n");
+
+	EXPECT_END(KERN_ERR,
+		   "OF: resolver: overlay phandle fixup failed: -22");
+	EXPECT_END(KERN_ERR,
+		   "OF: resolver: node label 'this_label_does_not_exist' not found in live devicetree symbols table");
+
 	return;
 
 err_unlock:
-- 
2.34.1

[PATCH 10/13] of: unittest-data: Convert remaining overlay DTS files to sugar syntax

[Geert Uytterhoeven]

Overlay syntactic sugar for generating target-path fragments is
supported by the version of dtc supplied with the kernel since commit
50aafd60898a8b3e ("scripts/dtc: Update to upstream version
v1.4.6-21-g84e414b0b5bc").  Hence convert the remaining unittest overlay
devicetree source files to sugar syntax, improving readability.

This completes the work started in commit db2f3762d609318e ("of: convert
unittest overlay devicetree source to sugar syntax").

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest-data/overlay_0.dtso  | 11 +++--------
 drivers/of/unittest-data/overlay_1.dtso  | 11 +++--------
 drivers/of/unittest-data/overlay_12.dtso | 11 +++--------
 drivers/of/unittest-data/overlay_13.dtso | 11 +++--------
 4 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/drivers/of/unittest-data/overlay_0.dtso b/drivers/of/unittest-data/overlay_0.dtso
index ac0f9e0fe65f80f3..bb46582e0485318f 100644
--- a/drivers/of/unittest-data/overlay_0.dtso
+++ b/drivers/of/unittest-data/overlay_0.dtso
@@ -2,13 +2,8 @@
 /dts-v1/;
 /plugin/;
 
-/ {
-	/* overlay_0 - enable using absolute target path */
+/* overlay_0 - enable using absolute target path */
 
-	fragment@0 {
-		target-path = "/testcase-data/overlay-node/test-bus/test-unittest0";
-		__overlay__ {
-			status = "okay";
-		};
-	};
+&{/testcase-data/overlay-node/test-bus/test-unittest0} {
+	status = "okay";
 };
diff --git a/drivers/of/unittest-data/overlay_1.dtso b/drivers/of/unittest-data/overlay_1.dtso
index e92a626e29483a32..9c0fc8ffa4a1d3d8 100644
--- a/drivers/of/unittest-data/overlay_1.dtso
+++ b/drivers/of/unittest-data/overlay_1.dtso
@@ -2,13 +2,8 @@
 /dts-v1/;
 /plugin/;
 
-/ {
-	/* overlay_1 - disable using absolute target path */
+/* overlay_1 - disable using absolute target path */
 
-	fragment@0 {
-		target-path = "/testcase-data/overlay-node/test-bus/test-unittest1";
-		__overlay__ {
-			status = "disabled";
-		};
-	};
+&{/testcase-data/overlay-node/test-bus/test-unittest1} {
+	status = "disabled";
 };
diff --git a/drivers/of/unittest-data/overlay_12.dtso b/drivers/of/unittest-data/overlay_12.dtso
index ca3441e2cbec94ce..8d5087793eb42688 100644
--- a/drivers/of/unittest-data/overlay_12.dtso
+++ b/drivers/of/unittest-data/overlay_12.dtso
@@ -2,13 +2,8 @@
 /dts-v1/;
 /plugin/;
 
-/ {
-	/* overlay_12 - enable using absolute target path (i2c) */
+/* overlay_12 - enable using absolute target path (i2c) */
 
-	fragment@0 {
-		target-path = "/testcase-data/overlay-node/test-bus/i2c-test-bus/test-unittest12";
-		__overlay__ {
-			status = "okay";
-		};
-	};
+&{/testcase-data/overlay-node/test-bus/i2c-test-bus/test-unittest12} {
+	status = "okay";
 };
diff --git a/drivers/of/unittest-data/overlay_13.dtso b/drivers/of/unittest-data/overlay_13.dtso
index 3c30dec6389436df..da200ae94f459ade 100644
--- a/drivers/of/unittest-data/overlay_13.dtso
+++ b/drivers/of/unittest-data/overlay_13.dtso
@@ -2,13 +2,8 @@
 /dts-v1/;
 /plugin/;
 
-/ {
-	/* overlay_13 - disable using absolute target path (i2c) */
+/* overlay_13 - disable using absolute target path (i2c) */
 
-	fragment@0 {
-		target-path = "/testcase-data/overlay-node/test-bus/i2c-test-bus/test-unittest13";
-		__overlay__ {
-			status = "disabled";
-		};
-	};
+&{/testcase-data/overlay-node/test-bus/i2c-test-bus/test-unittest13} {
+	status = "disabled";
 };
-- 
2.34.1

[PATCH 11/13] of: unittest-data: Fix whitespace - blank lines

[Geert Uytterhoeven]

Blank line between properties and subnodes.
Blank line between subsequent subnodes.
No blank line after subnode opening curly brace.
No blank line after subnode closing curly brace.
No blank line at end of file.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest-data/overlay.dtso                  | 2 --
 drivers/of/unittest-data/overlay_11.dtso               | 1 -
 drivers/of/unittest-data/overlay_15.dtso               | 1 +
 drivers/of/unittest-data/overlay_4.dtso                | 1 -
 drivers/of/unittest-data/overlay_bad_add_dup_node.dtso | 1 -
 drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso | 1 -
 drivers/of/unittest-data/overlay_bad_phandle.dtso      | 1 -
 drivers/of/unittest-data/overlay_bad_symbol.dtso       | 1 -
 drivers/of/unittest-data/overlay_common.dtsi           | 2 --
 drivers/of/unittest-data/overlay_gpio_01.dtso          | 1 +
 drivers/of/unittest-data/overlay_gpio_02a.dtso         | 1 +
 drivers/of/unittest-data/overlay_gpio_02b.dtso         | 1 +
 drivers/of/unittest-data/overlay_gpio_03.dtso          | 1 +
 drivers/of/unittest-data/overlay_gpio_04a.dtso         | 1 +
 drivers/of/unittest-data/overlay_gpio_04b.dtso         | 1 +
 drivers/of/unittest-data/testcases_common.dtsi         | 1 +
 drivers/of/unittest-data/tests-interrupts.dtsi         | 1 +
 drivers/of/unittest-data/tests-overlay.dtsi            | 1 -
 drivers/of/unittest-data/tests-phandle.dtsi            | 2 ++
 19 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/of/unittest-data/overlay.dtso b/drivers/of/unittest-data/overlay.dtso
index 3bbc59e922fe71c3..0c078b08ef083561 100644
--- a/drivers/of/unittest-data/overlay.dtso
+++ b/drivers/of/unittest-data/overlay.dtso
@@ -3,7 +3,6 @@
 /plugin/;
 
 &electric_1 {
-
 	status = "okay";
 
 	hvac_2: hvac-large-1 {
@@ -57,7 +56,6 @@ ride_200_right: track@20 {
 };
 
 &lights_2 {
-
 	status = "okay";
 	color = "purple", "white", "red", "green";
 	rate = < 3 256 >;
diff --git a/drivers/of/unittest-data/overlay_11.dtso b/drivers/of/unittest-data/overlay_11.dtso
index 9a79b253a8098833..7d04ff503a18ffba 100644
--- a/drivers/of/unittest-data/overlay_11.dtso
+++ b/drivers/of/unittest-data/overlay_11.dtso
@@ -23,6 +23,5 @@ test-unittest111 {
 			status = "okay";
 			reg = <1>;
 		};
-
 	};
 };
diff --git a/drivers/of/unittest-data/overlay_15.dtso b/drivers/of/unittest-data/overlay_15.dtso
index 5728490474f6bd2d..ba02ae1fed38b64a 100644
--- a/drivers/of/unittest-data/overlay_15.dtso
+++ b/drivers/of/unittest-data/overlay_15.dtso
@@ -7,6 +7,7 @@
 &unittest_i2c_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	test-unittest15 {
 		reg = <11>;
 		compatible = "unittest-i2c-mux";
diff --git a/drivers/of/unittest-data/overlay_4.dtso b/drivers/of/unittest-data/overlay_4.dtso
index a8a77ddf9abe829c..9b9eadddb4a09af3 100644
--- a/drivers/of/unittest-data/overlay_4.dtso
+++ b/drivers/of/unittest-data/overlay_4.dtso
@@ -5,7 +5,6 @@
 /* overlay_4 - test insertion of a full node */
 
 &unittest_test_bus {
-
 	/* suppress DTC warning */
 	#address-cells = <1>;
 	#size-cells = <0>;
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso b/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
index 145dfc3b1024191a..a8d8534f725c10ea 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
@@ -13,7 +13,6 @@
  */
 
 &electric_1 {
-
 	motor-1 {
 		controller {
 			power_bus = < 0x1 0x2 >;
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
index 6327d1ffb9636589..3f0ee9cbefb815be 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
@@ -24,7 +24,6 @@
  */
 
 &electric_1 {
-
 	motor-1 {
 		electric {
 			rpm_avail = < 100 >;
diff --git a/drivers/of/unittest-data/overlay_bad_phandle.dtso b/drivers/of/unittest-data/overlay_bad_phandle.dtso
index 83b79736031878fc..ab2c7df1019674f1 100644
--- a/drivers/of/unittest-data/overlay_bad_phandle.dtso
+++ b/drivers/of/unittest-data/overlay_bad_phandle.dtso
@@ -3,7 +3,6 @@
 /plugin/;
 
 &electric_1 {
-
 	// This label should cause an error when the overlay
 	// is applied.  There is already a phandle value
 	// in the base tree for motor-1.
diff --git a/drivers/of/unittest-data/overlay_bad_symbol.dtso b/drivers/of/unittest-data/overlay_bad_symbol.dtso
index 98c6d1de144a21ab..5d4e34baf1d8e3ed 100644
--- a/drivers/of/unittest-data/overlay_bad_symbol.dtso
+++ b/drivers/of/unittest-data/overlay_bad_symbol.dtso
@@ -3,7 +3,6 @@
 /plugin/;
 
 &electric_1 {
-
 	// This label should cause an error when the overlay
 	// is applied.  There is already a symbol hvac_1
 	// in the base tree
diff --git a/drivers/of/unittest-data/overlay_common.dtsi b/drivers/of/unittest-data/overlay_common.dtsi
index 08874a72556e050f..491b89c43cf38321 100644
--- a/drivers/of/unittest-data/overlay_common.dtsi
+++ b/drivers/of/unittest-data/overlay_common.dtsi
@@ -85,7 +85,5 @@ retail_1: vending@50000 {
 			compatible = "ot,tickets";
 			status = "disabled";
 		};
-
 	};
 };
-
diff --git a/drivers/of/unittest-data/overlay_gpio_01.dtso b/drivers/of/unittest-data/overlay_gpio_01.dtso
index 699ff104ae1062f9..bb3a31a2137aea17 100644
--- a/drivers/of/unittest-data/overlay_gpio_01.dtso
+++ b/drivers/of/unittest-data/overlay_gpio_01.dtso
@@ -5,6 +5,7 @@
 &unittest_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	gpio@0 {
 		compatible = "unittest-gpio";
 		reg = <0>;
diff --git a/drivers/of/unittest-data/overlay_gpio_02a.dtso b/drivers/of/unittest-data/overlay_gpio_02a.dtso
index ec59aff6ed47ed50..da955537df74be90 100644
--- a/drivers/of/unittest-data/overlay_gpio_02a.dtso
+++ b/drivers/of/unittest-data/overlay_gpio_02a.dtso
@@ -5,6 +5,7 @@
 &unittest_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	gpio@2 {
 		compatible = "unittest-gpio";
 		reg = <2>;
diff --git a/drivers/of/unittest-data/overlay_gpio_02b.dtso b/drivers/of/unittest-data/overlay_gpio_02b.dtso
index 43ce111d41ceb8d5..79503965d3d7d090 100644
--- a/drivers/of/unittest-data/overlay_gpio_02b.dtso
+++ b/drivers/of/unittest-data/overlay_gpio_02b.dtso
@@ -5,6 +5,7 @@
 &unittest_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	gpio@2 {
 		line-a {
 			gpio-hog;
diff --git a/drivers/of/unittest-data/overlay_gpio_03.dtso b/drivers/of/unittest-data/overlay_gpio_03.dtso
index 6e0312340a1ba758..d8c709616029b8ba 100644
--- a/drivers/of/unittest-data/overlay_gpio_03.dtso
+++ b/drivers/of/unittest-data/overlay_gpio_03.dtso
@@ -5,6 +5,7 @@
 &unittest_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	gpio@3 {
 		compatible = "unittest-gpio";
 		reg = <3>;
diff --git a/drivers/of/unittest-data/overlay_gpio_04a.dtso b/drivers/of/unittest-data/overlay_gpio_04a.dtso
index 7b1e04ebfa7a0410..de86511972c202a0 100644
--- a/drivers/of/unittest-data/overlay_gpio_04a.dtso
+++ b/drivers/of/unittest-data/overlay_gpio_04a.dtso
@@ -5,6 +5,7 @@
 &unittest_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	gpio@4 {
 		compatible = "unittest-gpio";
 		reg = <4>;
diff --git a/drivers/of/unittest-data/overlay_gpio_04b.dtso b/drivers/of/unittest-data/overlay_gpio_04b.dtso
index a14e95c6699aeb06..dc6eff22f927218b 100644
--- a/drivers/of/unittest-data/overlay_gpio_04b.dtso
+++ b/drivers/of/unittest-data/overlay_gpio_04b.dtso
@@ -5,6 +5,7 @@
 &unittest_test_bus {
 	#address-cells = <1>;
 	#size-cells = <0>;
+
 	gpio@4 {
 		line-c {
 			gpio-hog;
diff --git a/drivers/of/unittest-data/testcases_common.dtsi b/drivers/of/unittest-data/testcases_common.dtsi
index e7887f2301c102e6..1c2cdf353ae36b50 100644
--- a/drivers/of/unittest-data/testcases_common.dtsi
+++ b/drivers/of/unittest-data/testcases_common.dtsi
@@ -5,6 +5,7 @@ testcase-data {
 		changeset {
 			prop-update = "hello";
 			prop-remove = "world";
+
 			node-remove {
 			};
 		};
diff --git a/drivers/of/unittest-data/tests-interrupts.dtsi b/drivers/of/unittest-data/tests-interrupts.dtsi
index ecc74dbcc373f655..7c9f31cc131bae79 100644
--- a/drivers/of/unittest-data/tests-interrupts.dtsi
+++ b/drivers/of/unittest-data/tests-interrupts.dtsi
@@ -5,6 +5,7 @@ testcase-data {
 		interrupts {
 			#address-cells = <1>;
 			#size-cells = <1>;
+
 			test_intc0: intc0 {
 				interrupt-controller;
 				#interrupt-cells = <1>;
diff --git a/drivers/of/unittest-data/tests-overlay.dtsi b/drivers/of/unittest-data/tests-overlay.dtsi
index 4ea024d908ee22d6..eb35e8aa5d5ace95 100644
--- a/drivers/of/unittest-data/tests-overlay.dtsi
+++ b/drivers/of/unittest-data/tests-overlay.dtsi
@@ -3,7 +3,6 @@
 / {
 	testcase-data {
 		overlay-node {
-
 			/* test bus */
 			unittest_test_bus: test-bus {
 				compatible = "simple-bus";
diff --git a/drivers/of/unittest-data/tests-phandle.dtsi b/drivers/of/unittest-data/tests-phandle.dtsi
index 6b33be4c4416ce7b..d01f92f0f0db7f57 100644
--- a/drivers/of/unittest-data/tests-phandle.dtsi
+++ b/drivers/of/unittest-data/tests-phandle.dtsi
@@ -8,7 +8,9 @@ aliases {
 	testcase: testcase-data {
 		security-password = "password";
 		duplicate-name = "duplicate";
+
 		duplicate-name { };
+
 		phandle-tests {
 			provider0: provider0 {
 				#phandle-cells = <0>;
-- 
2.34.1

[PATCH 12/13] of: unittest-data: Fix whitespace - indentation

[Geert Uytterhoeven]

Single space for each indentation level.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest-data/overlay_bad_add_dup_node.dtso | 6 +++---
 drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso b/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
index a8d8534f725c10ea..11aa1401244d9685 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
@@ -21,7 +21,7 @@ controller {
 };
 
 &spin_ctrl_1 {
-		controller {
-			power_bus_emergency = < 0x101 0x102 >;
-		};
+	controller {
+		power_bus_emergency = < 0x101 0x102 >;
+	};
 };
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
index 3f0ee9cbefb815be..5af099cc2174e273 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
@@ -32,7 +32,7 @@ electric {
 };
 
 &spin_ctrl_1 {
-		electric {
-			rpm_avail = < 100 200 >;
-		};
+	electric {
+		rpm_avail = < 100 200 >;
+	};
 };
-- 
2.34.1

[PATCH 13/13] of: unittest-data: Fix whitespace - angular brackets

[Geert Uytterhoeven]

No space after opening or before closing angular bracket.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/unittest-data/overlay.dtso         | 30 ++++++++--------
 .../overlay_bad_add_dup_node.dtso             |  4 +--
 .../overlay_bad_add_dup_prop.dtso             |  4 +--
 .../of/unittest-data/overlay_bad_phandle.dtso |  4 +--
 .../of/unittest-data/overlay_bad_symbol.dtso  |  4 +--
 drivers/of/unittest-data/overlay_common.dtsi  | 34 +++++++++----------
 6 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/of/unittest-data/overlay.dtso b/drivers/of/unittest-data/overlay.dtso
index 0c078b08ef083561..b3e807b99852363e 100644
--- a/drivers/of/unittest-data/overlay.dtso
+++ b/drivers/of/unittest-data/overlay.dtso
@@ -7,8 +7,8 @@ &electric_1 {
 
 	hvac_2: hvac-large-1 {
 		compatible = "ot,hvac-large";
-		heat-range = < 40 75 >;
-		cool-range = < 65 80 >;
+		heat-range = <40 75>;
+		cool-range = <65 80>;
 	};
 };
 
@@ -23,11 +23,11 @@ ride@100 {
 		#size-cells = <1>;
 
 		track@30 {
-			incline-up = < 48 32 16 >;
+			incline-up = <48 32 16>;
 		};
 
 		track@40 {
-			incline-up = < 47 31 15 >;
+			incline-up = <47 31 15>;
 		};
 	};
 
@@ -35,22 +35,22 @@ ride_200: ride@200 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "ot,ferris-wheel";
-		reg = < 0x00000200 0x100 >;
-		hvac-provider = < &hvac_2 >;
-		hvac-thermostat = < 27 32 > ;
-		hvac-zones = < 12 5 >;
+		reg = <0x00000200 0x100>;
+		hvac-provider = <&hvac_2>;
+		hvac-thermostat = <27 32> ;
+		hvac-zones = <12 5>;
 		hvac-zone-names = "operator", "snack-bar";
-		spin-controller = < &spin_ctrl_1 3 >;
-		spin-rph = < 30 >;
-		gondolas = < 16 >;
-		gondola-capacity = < 6 >;
+		spin-controller = <&spin_ctrl_1 3>;
+		spin-rph = <30>;
+		gondolas = <16>;
+		gondola-capacity = <6>;
 
 		ride_200_left: track@10 {
-			reg = < 0x00000010 0x10 >;
+			reg = <0x00000010 0x10>;
 		};
 
 		ride_200_right: track@20 {
-			reg = < 0x00000020 0x10 >;
+			reg = <0x00000020 0x10>;
 		};
 	};
 };
@@ -58,5 +58,5 @@ ride_200_right: track@20 {
 &lights_2 {
 	status = "okay";
 	color = "purple", "white", "red", "green";
-	rate = < 3 256 >;
+	rate = <3 256>;
 };
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso b/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
index 11aa1401244d9685..9b53412b20796e29 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_node.dtso
@@ -15,13 +15,13 @@
 &electric_1 {
 	motor-1 {
 		controller {
-			power_bus = < 0x1 0x2 >;
+			power_bus = <0x1 0x2>;
 		};
 	};
 };
 
 &spin_ctrl_1 {
 	controller {
-		power_bus_emergency = < 0x101 0x102 >;
+		power_bus_emergency = <0x101 0x102>;
 	};
 };
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
index 5af099cc2174e273..e03f791655b07b3f 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dtso
@@ -26,13 +26,13 @@
 &electric_1 {
 	motor-1 {
 		electric {
-			rpm_avail = < 100 >;
+			rpm_avail = <100>;
 		};
 	};
 };
 
 &spin_ctrl_1 {
 	electric {
-		rpm_avail = < 100 200 >;
+		rpm_avail = <100 200>;
 	};
 };
diff --git a/drivers/of/unittest-data/overlay_bad_phandle.dtso b/drivers/of/unittest-data/overlay_bad_phandle.dtso
index ab2c7df1019674f1..a61ffc0738e3bf01 100644
--- a/drivers/of/unittest-data/overlay_bad_phandle.dtso
+++ b/drivers/of/unittest-data/overlay_bad_phandle.dtso
@@ -7,7 +7,7 @@ &electric_1 {
 	// is applied.  There is already a phandle value
 	// in the base tree for motor-1.
 	spin_ctrl_1_conflict: motor-1 {
-		accelerate = < 3 >;
-		decelerate = < 5 >;
+		accelerate = <3>;
+		decelerate = <5>;
 	};
 };
diff --git a/drivers/of/unittest-data/overlay_bad_symbol.dtso b/drivers/of/unittest-data/overlay_bad_symbol.dtso
index 5d4e34baf1d8e3ed..07f730384cdde1f8 100644
--- a/drivers/of/unittest-data/overlay_bad_symbol.dtso
+++ b/drivers/of/unittest-data/overlay_bad_symbol.dtso
@@ -8,8 +8,8 @@ &electric_1 {
 	// in the base tree
 	hvac_1: hvac-medium-2 {
 		compatible = "ot,hvac-medium";
-		heat-range = < 50 75 >;
-		cool-range = < 60 80 >;
+		heat-range = <50 75>;
+		cool-range = <60 80>;
 	};
 
 };
diff --git a/drivers/of/unittest-data/overlay_common.dtsi b/drivers/of/unittest-data/overlay_common.dtsi
index 491b89c43cf38321..a9d7cdbd5ddc470c 100644
--- a/drivers/of/unittest-data/overlay_common.dtsi
+++ b/drivers/of/unittest-data/overlay_common.dtsi
@@ -16,19 +16,19 @@ testcase-data-2 {
 
 		electric_1: substation@100 {
 			compatible = "ot,big-volts-control";
-			reg = < 0x00000100 0x100 >;
+			reg = <0x00000100 0x100>;
 			status = "disabled";
 
 			hvac_1: hvac-medium-1 {
 				compatible = "ot,hvac-medium";
-				heat-range = < 50 75 >;
-				cool-range = < 60 80 >;
+				heat-range = <50 75>;
+				cool-range = <60 80>;
 			};
 
 			spin_ctrl_1: motor-1 {
 				compatible = "ot,ferris-wheel-motor";
 				spin = "clockwise";
-				rpm_avail = < 50 >;
+				rpm_avail = <50>;
 			};
 
 			spin_ctrl_2: motor-8 {
@@ -41,27 +41,27 @@ rides_1: fairway-1 {
 			#size-cells = <1>;
 			compatible = "ot,rides";
 			status = "disabled";
-			orientation = < 127 >;
+			orientation = <127>;
 
 			ride@100 {
 				#address-cells = <1>;
 				#size-cells = <1>;
 				compatible = "ot,roller-coaster";
-				reg = < 0x00000100 0x100 >;
-				hvac-provider = < &hvac_1 >;
-				hvac-thermostat = < 29 > ;
-				hvac-zones = < 14 >;
+				reg = <0x00000100 0x100>;
+				hvac-provider = <&hvac_1>;
+				hvac-thermostat = <29> ;
+				hvac-zones = <14>;
 				hvac-zone-names = "operator";
-				spin-controller = < &spin_ctrl_2 5 &spin_ctrl_2 7 >;
+				spin-controller = <&spin_ctrl_2 5 &spin_ctrl_2 7>;
 				spin-controller-names = "track_1", "track_2";
-				queues = < 2 >;
+				queues = <2>;
 
 				track@30 {
-					reg = < 0x00000030 0x10 >;
+					reg = <0x00000030 0x10>;
 				};
 
 				track@40 {
-					reg = < 0x00000040 0x10 >;
+					reg = <0x00000040 0x10>;
 				};
 
 			};
@@ -69,19 +69,19 @@ track@40 {
 
 		lights_1: lights@30000 {
 			compatible = "ot,work-lights";
-			reg = < 0x00030000 0x1000 >;
+			reg = <0x00030000 0x1000>;
 			status = "disabled";
 		};
 
 		lights_2: lights@40000 {
 			compatible = "ot,show-lights";
-			reg = < 0x00040000 0x1000 >;
+			reg = <0x00040000 0x1000>;
 			status = "disabled";
-			rate = < 13 138 >;
+			rate = <13 138>;
 		};
 
 		retail_1: vending@50000 {
-			reg = < 0x00050000 0x1000 >;
+			reg = <0x00050000 0x1000>;
 			compatible = "ot,tickets";
 			status = "disabled";
 		};
-- 
2.34.1

Re: [PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

[Rob Herring]

On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> Formatting strings using "%pOF" while holding devtree_lock causes a
> deadlock.  Lockdep reports:
> 
>     of_get_parent from of_fwnode_get_parent+0x18/0x24
>     ^^^^^^^^^^^^^

I'm wondering if we really need the lock in there. We never unset or 
change the parent. It gets detached, but we're not checking for that. 
The node could get freed, but the lock is not for that, refcounts are.

>     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28

count parents? Huh? Isn't it always 1? 

>     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
>     fwnode_full_name_string from device_node_string+0x1a0/0x404
>     device_node_string from pointer+0x3c0/0x534
>     pointer from vsnprintf+0x248/0x36c
>     vsnprintf from vprintk_store+0x130/0x3b4
> 
> Fix this by making the locking cover only the parts that really need it.
> 
> Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/of/dynamic.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index e311d406b1705306..eae45a1c673ee05f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  
>  	__of_changeset_entry_dump(ce);
>  
> -	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	switch (ce->action) {
>  	case OF_RECONFIG_ATTACH_NODE:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		__of_attach_node(ce->np);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);

I think you could just move the spinlock into __of_attach_node(). The 
only other caller looks like this.

>  		break;
>  	case OF_RECONFIG_DETACH_NODE:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		__of_detach_node(ce->np);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		break;
>  	case OF_RECONFIG_ADD_PROPERTY:
>  		/* If the property is in deadprops then it must be removed */
> @@ -619,7 +622,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  			}
>  		}
>  
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_add_property(ce->np, ce->prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: add_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -628,7 +633,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  		}
>  		break;
>  	case OF_RECONFIG_REMOVE_PROPERTY:
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_remove_property(ce->np, ce->prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: remove_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -647,7 +654,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  			}
>  		}
>  
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>  		ret = __of_update_property(ce->np, ce->prop, &old_prop);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  		if (ret) {
>  			pr_err("changeset: update_property failed @%pOF/%s\n",
>  				ce->np,
> @@ -658,7 +667,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  	default:
>  		ret = -EINVAL;
>  	}
> -	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	if (ret)
>  		return ret;
> -- 
> 2.34.1
> 

Re: [PATCH 06/13] of: unittest: Merge of_unittest_apply{,_revert}_overlay_check()

[Rob Herring]

On Wed, Jul 19, 2023 at 05:00:06PM +0200, Geert Uytterhoeven wrote:
> of_unittest_apply_overlay_check() and the first part of
> of_unittest_apply_revert_overlay_check() are identical.
> Reduce code duplication by replacing them by two wrappers around a
> common helper.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/of/unittest.c | 61 ++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 39 deletions(-)

I would do something like this instead:

8<-------------------------------------------------------------------
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index a406a12eb208..a9635935aa26 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2102,7 +2102,7 @@ static int __init of_unittest_apply_overlay(int overlay_nr, int *ovcs_id)
 }
 
 /* apply an overlay while checking before and after states */
-static int __init of_unittest_apply_overlay_check(int overlay_nr,
+static int __init _of_unittest_apply_overlay_check(int overlay_nr,
 		int unittest_nr, int before, int after,
 		enum overlay_type ovtype)
 {
@@ -2133,6 +2133,16 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
 		return -EINVAL;
 	}
 
+	return ovcs_id;
+}
+
+static int __init of_unittest_apply_overlay_check(int overlay_nr,
+		int unittest_nr, int before, int after,
+		enum overlay_type ovtype)
+{
+	int ovcs_id = _of_unittest_apply_overlay_check(overlay_nr, unittest_nr, before, after, ovtype);
+	if (ovcs_id < 0)
+		return ovcs_id;
 	return 0;
 }
 
@@ -2143,31 +2153,9 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 {
 	int ret, ovcs_id, save_ovcs_id;
 
-	/* unittest device must be in before state */
-	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
-		unittest(0, "%s with device @\"%s\" %s\n",
-				overlay_name_from_nr(overlay_nr),
-				unittest_path(unittest_nr, ovtype),
-				!before ? "enabled" : "disabled");
-		return -EINVAL;
-	}
-
-	/* apply the overlay */
-	ovcs_id = 0;
-	ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
-	if (ret != 0) {
-		/* of_unittest_apply_overlay already called unittest() */
-		return ret;
-	}
-
-	/* unittest device must be in after state */
-	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
-		unittest(0, "%s failed to create @\"%s\" %s\n",
-				overlay_name_from_nr(overlay_nr),
-				unittest_path(unittest_nr, ovtype),
-				!after ? "enabled" : "disabled");
-		return -EINVAL;
-	}
+	ovcs_id = _of_unittest_apply_overlay_check(overlay_nr, unittest_nr, before, after, ovtype);
+	if (ovcs_id < 0)
+		return ovcs_id;
 
 	save_ovcs_id = ovcs_id;
 	ret = of_overlay_remove(&ovcs_id);

Re: [PATCH 00/13] of: overlay/unittest: Miscellaneous fixes and improvements

[Rob Herring]

On Wed, Jul 19, 2023 at 05:00:00PM +0200, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> This patch series contains miscellaneous fixes and improvements for
> dynamic DT overlays and the related unit tests.
> 
> The first two patches are fixes for a lock-up and a crash.
> The remaining patches are smaller fixes, enhancements and cleanups for
> the overlay tests, including one new test.
> 
> I ran into the crash when accidentally loading the wrong overlay (using
> the out-of-tree DT overlay configfs[1]), and removing it afterwards.
> As this case was not yet covered by the unittests, I added a test.
> I enhanced the tests to clean up partial state after a failed
> overlay apply attempt, which triggered the lock-up.
> 
> Thanks for your comments!

Other than my comments all looks good to me. Of course, I treat code 
deletions, documentation additions, and test additions the same: 
automatic acceptance. ;)

I'll give it a bit more time in case Frank has comments.

Rob

Re: [PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

[Rob Herring]

+Sakari

On Wed, Jul 19, 2023 at 05:02:56PM -0600, Rob Herring wrote:
> On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> > Formatting strings using "%pOF" while holding devtree_lock causes a
> > deadlock.  Lockdep reports:
> > 
> >     of_get_parent from of_fwnode_get_parent+0x18/0x24
> >     ^^^^^^^^^^^^^
> 
> I'm wondering if we really need the lock in there. We never unset or 
> change the parent. It gets detached, but we're not checking for that. 
> The node could get freed, but the lock is not for that, refcounts are.

The lock existed since 2.6.12 for powerpc. It's not clear to me whether 
it was really ever needed. There's lots of places we just access 
'parent' without a lock. Not to say that's right.

The lock doesn't even help in this case because we release the lock on 
each count and between counting and getting the names. If the tree 
changes, the lock isn't going to help.

> >     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> 
> count parents? Huh? Isn't it always 1? 
> 
> >     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> >     fwnode_full_name_string from device_node_string+0x1a0/0x404
> >     device_node_string from pointer+0x3c0/0x534
> >     pointer from vsnprintf+0x248/0x36c
> >     vsnprintf from vprintk_store+0x130/0x3b4
> > 
> > Fix this by making the locking cover only the parts that really need it.
> > 
> > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")

That's the wrong commit. My implementation in vsprintf.c worked with 
this. It's commit a92eb7621b9f ("lib/vsprintf: Make use of fwnode API to 
obtain node names and separators") which broke it. It came 2 years 
later.

The fwnode based implementation looks like the wrong level of 
abstraction to me. Why not just push 'give me the full name' down to the 
fwnode backends? The functions defined are *only* used by vsprintf.c.

I don't really understand the "let's change everything to use fwnode" 
even for things which will never be anything but DT. %pOF is DT 
only. </rant>

Rob

Re: [PATCH 01/13] of: dynamic: Do not use "%pOF" while holding devtree_lock

[Geert Uytterhoeven]

Hi Rob,

On Thu, Jul 20, 2023 at 1:03 AM Rob Herring <robh@kernel.org> wrote:
> On Wed, Jul 19, 2023 at 05:00:01PM +0200, Geert Uytterhoeven wrote:
> > Formatting strings using "%pOF" while holding devtree_lock causes a
> > deadlock.  Lockdep reports:
> >
> >     of_get_parent from of_fwnode_get_parent+0x18/0x24
> >     ^^^^^^^^^^^^^
> >     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> >     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> >     fwnode_full_name_string from device_node_string+0x1a0/0x404
> >     device_node_string from pointer+0x3c0/0x534
> >     pointer from vsnprintf+0x248/0x36c
> >     vsnprintf from vprintk_store+0x130/0x3b4
> >
> > Fix this by making the locking cover only the parts that really need it.
> >
> > Fixes: 0d638a07d3a1e98a ("of: Convert to using %pOF instead of full_name")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/of/dynamic.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index e311d406b1705306..eae45a1c673ee05f 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -601,13 +601,16 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> >
> >       __of_changeset_entry_dump(ce);
> >
> > -     raw_spin_lock_irqsave(&devtree_lock, flags);
> >       switch (ce->action) {
> >       case OF_RECONFIG_ATTACH_NODE:
> > +             raw_spin_lock_irqsave(&devtree_lock, flags);
> >               __of_attach_node(ce->np);
> > +             raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> I think you could just move the spinlock into __of_attach_node(). The
> only other caller looks like this.

I'd rather not do that, as the double underscore is typically used to
indicate that this function does not take the lock.
Cfr. of_find_property() vs. __of_find_property().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 06/13] of: unittest: Merge of_unittest_apply{,_revert}_overlay_check()

[Geert Uytterhoeven]

Hi Rob,

On Thu, Jul 20, 2023 at 8:31 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Jul 19, 2023 at 05:00:06PM +0200, Geert Uytterhoeven wrote:
> > of_unittest_apply_overlay_check() and the first part of
> > of_unittest_apply_revert_overlay_check() are identical.
> > Reduce code duplication by replacing them by two wrappers around a
> > common helper.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/of/unittest.c | 61 ++++++++++++++++---------------------------
> >  1 file changed, 22 insertions(+), 39 deletions(-)
>
> I would do something like this instead:
>
> 8<-------------------------------------------------------------------
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index a406a12eb208..a9635935aa26 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -2102,7 +2102,7 @@ static int __init of_unittest_apply_overlay(int overlay_nr, int *ovcs_id)
>  }
>
>  /* apply an overlay while checking before and after states */
> -static int __init of_unittest_apply_overlay_check(int overlay_nr,
> +static int __init _of_unittest_apply_overlay_check(int overlay_nr,
>                 int unittest_nr, int before, int after,
>                 enum overlay_type ovtype)
>  {
> @@ -2133,6 +2133,16 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
>                 return -EINVAL;
>         }
>
> +       return ovcs_id;
> +}
> +
> +static int __init of_unittest_apply_overlay_check(int overlay_nr,
> +               int unittest_nr, int before, int after,
> +               enum overlay_type ovtype)
> +{
> +       int ovcs_id = _of_unittest_apply_overlay_check(overlay_nr, unittest_nr, before, after, ovtype);
> +       if (ovcs_id < 0)
> +               return ovcs_id;
>         return 0;
>  }
>
> @@ -2143,31 +2153,9 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
>  {
>         int ret, ovcs_id, save_ovcs_id;
>
> -       /* unittest device must be in before state */
> -       if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
> -               unittest(0, "%s with device @\"%s\" %s\n",
> -                               overlay_name_from_nr(overlay_nr),
> -                               unittest_path(unittest_nr, ovtype),
> -                               !before ? "enabled" : "disabled");
> -               return -EINVAL;
> -       }
> -
> -       /* apply the overlay */
> -       ovcs_id = 0;
> -       ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
> -       if (ret != 0) {
> -               /* of_unittest_apply_overlay already called unittest() */
> -               return ret;
> -       }
> -
> -       /* unittest device must be in after state */
> -       if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
> -               unittest(0, "%s failed to create @\"%s\" %s\n",
> -                               overlay_name_from_nr(overlay_nr),
> -                               unittest_path(unittest_nr, ovtype),
> -                               !after ? "enabled" : "disabled");
> -               return -EINVAL;
> -       }
> +       ovcs_id = _of_unittest_apply_overlay_check(overlay_nr, unittest_nr, before, after, ovtype);
> +       if (ovcs_id < 0)
> +               return ovcs_id;
>
>         save_ovcs_id = ovcs_id;
>         ret = of_overlay_remove(&ovcs_id);

That's what I had done first, before I realized I could reduce it by
five more lines of code ;-)

mine:  1 file changed, 22 insertions(+), 39 deletions(-)
yours: 1 file changed, 14 insertions(+), 26 deletions(-)

Anyway, you're the maintainer, so I can update my patch if you insist...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 06/13] of: unittest: Merge of_unittest_apply{,_revert}_overlay_check()

[Rob Herring]

On Thu, Jul 27, 2023 at 8:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Thu, Jul 20, 2023 at 8:31 PM Rob Herring <robh@kernel.org> wrote:
> > On Wed, Jul 19, 2023 at 05:00:06PM +0200, Geert Uytterhoeven wrote:
> > > of_unittest_apply_overlay_check() and the first part of
> > > of_unittest_apply_revert_overlay_check() are identical.
> > > Reduce code duplication by replacing them by two wrappers around a
> > > common helper.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  drivers/of/unittest.c | 61 ++++++++++++++++---------------------------
> > >  1 file changed, 22 insertions(+), 39 deletions(-)
> >
> > I would do something like this instead:
> >
> > 8<-------------------------------------------------------------------
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index a406a12eb208..a9635935aa26 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -2102,7 +2102,7 @@ static int __init of_unittest_apply_overlay(int overlay_nr, int *ovcs_id)
> >  }
> >
> >  /* apply an overlay while checking before and after states */
> > -static int __init of_unittest_apply_overlay_check(int overlay_nr,
> > +static int __init _of_unittest_apply_overlay_check(int overlay_nr,
> >                 int unittest_nr, int before, int after,
> >                 enum overlay_type ovtype)
> >  {
> > @@ -2133,6 +2133,16 @@ static int __init of_unittest_apply_overlay_check(int overlay_nr,
> >                 return -EINVAL;
> >         }
> >
> > +       return ovcs_id;
> > +}
> > +
> > +static int __init of_unittest_apply_overlay_check(int overlay_nr,
> > +               int unittest_nr, int before, int after,
> > +               enum overlay_type ovtype)
> > +{
> > +       int ovcs_id = _of_unittest_apply_overlay_check(overlay_nr, unittest_nr, before, after, ovtype);
> > +       if (ovcs_id < 0)
> > +               return ovcs_id;
> >         return 0;
> >  }
> >
> > @@ -2143,31 +2153,9 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
> >  {
> >         int ret, ovcs_id, save_ovcs_id;
> >
> > -       /* unittest device must be in before state */
> > -       if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
> > -               unittest(0, "%s with device @\"%s\" %s\n",
> > -                               overlay_name_from_nr(overlay_nr),
> > -                               unittest_path(unittest_nr, ovtype),
> > -                               !before ? "enabled" : "disabled");
> > -               return -EINVAL;
> > -       }
> > -
> > -       /* apply the overlay */
> > -       ovcs_id = 0;
> > -       ret = of_unittest_apply_overlay(overlay_nr, &ovcs_id);
> > -       if (ret != 0) {
> > -               /* of_unittest_apply_overlay already called unittest() */
> > -               return ret;
> > -       }
> > -
> > -       /* unittest device must be in after state */
> > -       if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
> > -               unittest(0, "%s failed to create @\"%s\" %s\n",
> > -                               overlay_name_from_nr(overlay_nr),
> > -                               unittest_path(unittest_nr, ovtype),
> > -                               !after ? "enabled" : "disabled");
> > -               return -EINVAL;
> > -       }
> > +       ovcs_id = _of_unittest_apply_overlay_check(overlay_nr, unittest_nr, before, after, ovtype);
> > +       if (ovcs_id < 0)
> > +               return ovcs_id;
> >
> >         save_ovcs_id = ovcs_id;
> >         ret = of_overlay_remove(&ovcs_id);
>
> That's what I had done first, before I realized I could reduce it by
> five more lines of code ;-)
>
> mine:  1 file changed, 22 insertions(+), 39 deletions(-)
> yours: 1 file changed, 14 insertions(+), 26 deletions(-)

Less change to review is also worthwhile.

> Anyway, you're the maintainer, so I can update my patch if you insist...

The other thing about this that I noticed is I recall gregkh not
liking the pattern where function parameters change what the function
does (e.g. do_x_or_y(bool do_y)).

So yes, I prefer mine.

Rob

Re: [PATCH 00/13] of: overlay/unittest: Miscellaneous fixes and improvements

[Frank Rowand]

On 7/20/23 13:37, Rob Herring wrote:
> On Wed, Jul 19, 2023 at 05:00:00PM +0200, Geert Uytterhoeven wrote:
>> 	Hi all,
>>
>> This patch series contains miscellaneous fixes and improvements for
>> dynamic DT overlays and the related unit tests.
>>
>> The first two patches are fixes for a lock-up and a crash.
>> The remaining patches are smaller fixes, enhancements and cleanups for
>> the overlay tests, including one new test.
>>
>> I ran into the crash when accidentally loading the wrong overlay (using
>> the out-of-tree DT overlay configfs[1]), and removing it afterwards.
>> As this case was not yet covered by the unittests, I added a test.
>> I enhanced the tests to clean up partial state after a failed
>> overlay apply attempt, which triggered the lock-up.
>>
>> Thanks for your comments!
> 
> Other than my comments all looks good to me. Of course, I treat code 
> deletions, documentation additions, and test additions the same: 
> automatic acceptance. ;)
> 
> I'll give it a bit more time in case Frank has comments.

Thanks Rob, but no need to wait for me as I'm easing out of devicetree.

-Frank

> 
> Rob