[PATCH 0/3] of: overlay: unittest: fix overlay notify error, add overlay notifier tests, fix bug revealed by new unittests

[PATCH 0/3] of: overlay: unittest: fix overlay notify error, add overlay notifier tests, fix bug revealed by new unittests

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

patch 1/3: fix bug in overlay notifier handling
patch 2/3: add overlay notifier unittests, expose another bug
patch 3/3: fix bug exposed in patch 2, update unittests for fix

Frank Rowand (3):
  of: overlay: add entry to of_overlay_action_name[]
  of: overlay: unittest: add tests for overlay notifiers
  of: overlay: do not free changeset when of_overlay_apply returns error

 drivers/of/overlay.c                    |  56 ++++---
 drivers/of/unittest-data/Makefile       |  10 ++
 drivers/of/unittest-data/overlay_16.dts |  15 ++
 drivers/of/unittest-data/overlay_17.dts |  15 ++
 drivers/of/unittest-data/overlay_18.dts |  15 ++
 drivers/of/unittest-data/overlay_19.dts |  15 ++
 drivers/of/unittest-data/overlay_20.dts |  15 ++
 drivers/of/unittest.c                   | 204 ++++++++++++++++++++++++
 include/linux/of.h                      |  13 ++
 9 files changed, 333 insertions(+), 25 deletions(-)
 create mode 100644 drivers/of/unittest-data/overlay_16.dts
 create mode 100644 drivers/of/unittest-data/overlay_17.dts
 create mode 100644 drivers/of/unittest-data/overlay_18.dts
 create mode 100644 drivers/of/unittest-data/overlay_19.dts
 create mode 100644 drivers/of/unittest-data/overlay_20.dts

-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 1/3] of: overlay: add entry to of_overlay_action_name[]

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

The values of enum of_overlay_notify_action are used to index into
array of_overlay_action_name.  Add an entry to of_overlay_action_name
for the value recently added to of_overlay_notify_action.

Array of_overlay_action_name[] is moved into include/linux/of.h
adjacent to enum of_overlay_notify_action to make the connection
between the two more obvious if either is modified in the future.

The only use of of_overlay_action_name is for error reporting in
overlay_notify().  All callers of overlay_notify() report the same
error, but with fewer details.  Remove the redundant error reports
in the callers.

Fixes: 067c098766c6 ("of: overlay: rework overlay apply and remove kfree()s")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 27 +++++----------------------
 include/linux/of.h   | 13 +++++++++++++
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 64588bff99ce..48c240b06d3b 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -152,13 +152,6 @@ int of_overlay_notifier_unregister(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
 
-static char *of_overlay_action_name[] = {
-	"pre-apply",
-	"post-apply",
-	"pre-remove",
-	"post-remove",
-};
-
 static int overlay_notify(struct overlay_changeset *ovcs,
 		enum of_overlay_notify_action action)
 {
@@ -180,7 +173,7 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 		if (ret) {
 			ret = notifier_to_errno(ret);
 			pr_err("overlay changeset %s notifier error %d, target: %pOF\n",
-			       of_overlay_action_name[action], ret, nd.target);
+			       of_overlay_action_name(action), ret, nd.target);
 			return ret;
 		}
 	}
@@ -927,10 +920,8 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
 		goto out;
 
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
-	if (ret) {
-		pr_err("overlay changeset pre-apply notify error %d\n", ret);
+	if (ret)
 		goto out;
-	}
 
 	ret = build_changeset(ovcs);
 	if (ret)
@@ -953,12 +944,9 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
 	/* notify failure is not fatal, continue */
 
 	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_APPLY);
-	if (ret_tmp) {
-		pr_err("overlay changeset post-apply notify error %d\n",
-		       ret_tmp);
+	if (ret_tmp)
 		if (!ret)
 			ret = ret_tmp;
-	}
 
 out:
 	pr_debug("%s() err=%d\n", __func__, ret);
@@ -1194,10 +1182,8 @@ int of_overlay_remove(int *ovcs_id)
 	}
 
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE);
-	if (ret) {
-		pr_err("overlay changeset pre-remove notify error %d\n", ret);
+	if (ret)
 		goto err_unlock;
-	}
 
 	ret_apply = 0;
 	ret = __of_changeset_revert_entries(&ovcs->cset, &ret_apply);
@@ -1220,12 +1206,9 @@ int of_overlay_remove(int *ovcs_id)
 	 * OF_OVERLAY_POST_REMOVE returns an error.
 	 */
 	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
-	if (ret_tmp) {
-		pr_err("overlay changeset post-remove notify error %d\n",
-		       ret_tmp);
+	if (ret_tmp)
 		if (!ret)
 			ret = ret_tmp;
-	}
 
 	free_overlay_changeset(ovcs);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 17741eee0ca4..f0a5d6b10c5a 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1550,6 +1550,19 @@ enum of_overlay_notify_action {
 	OF_OVERLAY_POST_REMOVE,
 };
 
+static inline char *of_overlay_action_name(enum of_overlay_notify_action action)
+{
+	static char *of_overlay_action_name[] = {
+		"init",
+		"pre-apply",
+		"post-apply",
+		"pre-remove",
+		"post-remove",
+	};
+
+	return of_overlay_action_name[action];
+}
+
 struct of_overlay_notify_data {
 	struct device_node *overlay;
 	struct device_node *target;
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 2/3] of: overlay: unittest: add tests for overlay notifiers

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

Add tests for overlay apply and remove notifiers.  Trigger errors
for each of the notifier actions.

These tests will reveal a memory leak problem when a notifier returns
an error for action OF_OVERLAY_POST_APPLY.  The pr_err() message is:

   OF: ERROR: memory leak, expected refcount 1 instead of 3,
   of_node_get()/of_node_put() unbalanced - destroy cset entry: attach
   overlay node /testcase-data/overlay-node/test-bus/test-unittest17

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

===

Output of the new overlay notifier unittests, as filtered by
scripts/dtc/of_unittest_expect:

   ### dt-test ### pass of_unittest_overlay_notify():2825
ok OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2846
   ### dt-test ### pass of_unittest_overlay_notify():2851
ok OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus
   OF: ERROR: memory leak, expected refcount 1 instead of 3, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /testcase-data/overlay-node/test-bus/test-unittest17
   ### dt-test ### pass of_unittest_overlay_notify():2857
   ### dt-test ### pass of_unittest_overlay_notify():2862
   ### dt-test ### pass of_unittest_overlay_notify():2866
   ### dt-test ### pass of_unittest_overlay_notify():2869
ok OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2880
   ### dt-test ### pass of_unittest_overlay_notify():2888
   ### dt-test ### pass of_unittest_overlay_notify():2892
   ### dt-test ### pass of_unittest_overlay_notify():2895
ok OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2902
   ### dt-test ### pass of_unittest_overlay_notify():2909
   ### dt-test ### pass of_unittest_overlay_notify():2914
   ### dt-test ### pass of_unittest_overlay_notify():2926

Note that four new kernel error messages are triggered by the new
tests, and of_unittest_expect labels them as expected errors with
the string 'ok ' in the first three columns.

A fifth new kernel error message is a real bug, which will be fixed
in patch 3/3:

   OF: ERROR: memory leak, expected refcount 1 instead of 3, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /testcase-data/overlay-node/test-bus/test-unittest17


 drivers/of/unittest-data/Makefile       |  10 ++
 drivers/of/unittest-data/overlay_16.dts |  15 ++
 drivers/of/unittest-data/overlay_17.dts |  15 ++
 drivers/of/unittest-data/overlay_18.dts |  15 ++
 drivers/of/unittest-data/overlay_19.dts |  15 ++
 drivers/of/unittest-data/overlay_20.dts |  15 ++
 drivers/of/unittest.c                   | 198 ++++++++++++++++++++++++
 7 files changed, 283 insertions(+)
 create mode 100644 drivers/of/unittest-data/overlay_16.dts
 create mode 100644 drivers/of/unittest-data/overlay_17.dts
 create mode 100644 drivers/of/unittest-data/overlay_18.dts
 create mode 100644 drivers/of/unittest-data/overlay_19.dts
 create mode 100644 drivers/of/unittest-data/overlay_20.dts

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index fbded24c608c..d072f3ba3971 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -17,6 +17,11 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \
 			    overlay_12.dtb.o \
 			    overlay_13.dtb.o \
 			    overlay_15.dtb.o \
+			    overlay_16.dtb.o \
+			    overlay_17.dtb.o \
+			    overlay_18.dtb.o \
+			    overlay_19.dtb.o \
+			    overlay_20.dtb.o \
 			    overlay_bad_add_dup_node.dtb.o \
 			    overlay_bad_add_dup_prop.dtb.o \
 			    overlay_bad_phandle.dtb.o \
@@ -75,6 +80,11 @@ apply_static_overlay_1 := overlay_0.dtbo \
 			  overlay_12.dtbo \
 			  overlay_13.dtbo \
 			  overlay_15.dtbo \
+			  overlay_16.dtbo \
+			  overlay_17.dtbo \
+			  overlay_18.dtbo \
+			  overlay_19.dtbo \
+			  overlay_20.dtbo \
 			  overlay_gpio_01.dtbo \
 			  overlay_gpio_02a.dtbo \
 			  overlay_gpio_02b.dtbo \
diff --git a/drivers/of/unittest-data/overlay_16.dts b/drivers/of/unittest-data/overlay_16.dts
new file mode 100644
index 000000000000..80a46dc02581
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_16.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/* overlay_16 - notify test */
+
+&unittest_test_bus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	test-unittest16 {
+		compatible = "unittest";
+		reg = <16>;
+	};
+};
diff --git a/drivers/of/unittest-data/overlay_17.dts b/drivers/of/unittest-data/overlay_17.dts
new file mode 100644
index 000000000000..5b8a2209177f
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_17.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/* overlay_17 - notify test */
+
+&unittest_test_bus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	test-unittest17 {
+		compatible = "unittest";
+		reg = <17>;
+	};
+};
diff --git a/drivers/of/unittest-data/overlay_18.dts b/drivers/of/unittest-data/overlay_18.dts
new file mode 100644
index 000000000000..dcddd5f6d301
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_18.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/* overlay_18 - notify test */
+
+&unittest_test_bus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	test-unittest18 {
+		compatible = "unittest";
+		reg = <18>;
+	};
+};
diff --git a/drivers/of/unittest-data/overlay_19.dts b/drivers/of/unittest-data/overlay_19.dts
new file mode 100644
index 000000000000..32b3ba0b66a3
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_19.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/* overlay_19 - notify test */
+
+&unittest_test_bus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	test-unittest19 {
+		compatible = "unittest";
+		reg = <19>;
+	};
+};
diff --git a/drivers/of/unittest-data/overlay_20.dts b/drivers/of/unittest-data/overlay_20.dts
new file mode 100644
index 000000000000..d4a4f2f32ec7
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_20.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/* overlay_20 - notify test */
+
+&unittest_test_bus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	test-unittest20 {
+		compatible = "unittest";
+		reg = <20>;
+	};
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index c4106de9f137..e28c3df2c4c2 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2741,6 +2741,192 @@ static inline void of_unittest_overlay_i2c_15(void) { }
 
 #endif
 
+static int of_notify(struct notifier_block *nb, unsigned long action,
+		     void *arg)
+{
+	struct of_overlay_notify_data *nd = arg;
+	struct device_node *found;
+	int ret;
+
+	/*
+	 * For overlay_16 .. overlay_19, check that returning an error
+	 * works for each of the actions by setting an arbitrary return
+	 * error number that matches the test number.  e.g. for unittest16,
+	 * ret = -EBUSY which is -16.
+	 *
+	 * OVERLAY_INFO() for the overlays is declared to expect the same
+	 * error number, so overlay_data_apply() will return no error.
+	 *
+	 * overlay_20 will return NOTIFY_DONE
+	 */
+
+	ret = 0;
+	of_node_get(nd->overlay);
+
+	switch (action) {
+
+	case OF_OVERLAY_PRE_APPLY:
+		found = of_find_node_by_name(nd->overlay, "test-unittest16");
+		if (found) {
+			of_node_put(found);
+			ret = -EBUSY;
+		}
+		break;
+
+	case OF_OVERLAY_POST_APPLY:
+		found = of_find_node_by_name(nd->overlay, "test-unittest17");
+		if (found) {
+			of_node_put(found);
+			ret = -EEXIST;
+		}
+		break;
+
+	case OF_OVERLAY_PRE_REMOVE:
+		found = of_find_node_by_name(nd->overlay, "test-unittest18");
+		if (found) {
+			of_node_put(found);
+			ret = -EXDEV;
+		}
+		break;
+
+	case OF_OVERLAY_POST_REMOVE:
+		found = of_find_node_by_name(nd->overlay, "test-unittest19");
+		if (found) {
+			of_node_put(found);
+			ret = -ENODEV;
+		}
+		break;
+
+	default:			/* should not happen */
+		of_node_put(nd->overlay);
+		ret = -EINVAL;
+		break;
+	}
+
+	if (ret)
+		return notifier_from_errno(ret);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block of_nb = {
+	.notifier_call = of_notify,
+};
+
+
+
+
+static void __init of_unittest_overlay_notify(void)
+{
+	int ovcs_id;
+	int ret;
+
+	ret = of_overlay_notifier_register(&of_nb);
+	unittest(!ret,
+		 "of_overlay_notifier_register() failed, ret = %d\n", ret);
+	if (ret)
+		return;
+
+	/*
+	 * The overlays are applied by overlay_data_apply()
+	 * instead of of_unittest_apply_overlay() so that they
+	 * will not be tracked.  Thus they will not be removed
+	 * by of_unittest_remove_tracked_overlays().
+	 *
+	 * Applying overlays 16 - 19 will each trigger an error for a
+	 * different action in of_notify().
+	 *
+	 * Applying overlay 20 will not trigger any error in of_notify().
+	 */
+
+	/* ---  overlay 16  --- */
+
+	EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus");
+
+	unittest(overlay_data_apply("overlay_16", &ovcs_id),
+		 "test OF_OVERLAY_PRE_APPLY notify injected error\n");
+
+	EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus");
+
+	unittest(!ovcs_id, "ovcs_id created for overlay_16\n");
+
+	/* ---  overlay 17  --- */
+
+	EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus");
+
+	unittest(overlay_data_apply("overlay_17", &ovcs_id),
+		 "test OF_OVERLAY_POST_APPLY notify injected error\n");
+
+	EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus");
+
+	unittest(!ovcs_id, "ovcs_id created for overlay_17\n");
+
+	/* ---  overlay 18  --- */
+
+	unittest(overlay_data_apply("overlay_18", &ovcs_id),
+		 "OF_OVERLAY_PRE_REMOVE notify injected error\n");
+
+	unittest(ovcs_id, "ovcs_id not created for overlay_18\n");
+
+	if (ovcs_id) {
+		EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus");
+
+		ret = of_overlay_remove(&ovcs_id);
+		EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus");
+		if (ret == -EXDEV) {
+			/*
+			 * change set ovcs_id should still exist
+			 */
+			unittest(1, "overlay_18 of_overlay_remove() injected error for OF_OVERLAY_PRE_REMOVE\n");
+		} else {
+			unittest(0, "overlay_18 of_overlay_remove() injected error for OF_OVERLAY_PRE_REMOVE not returned\n");
+		}
+	} else {
+		unittest(1, "ovcs_id not created for overlay_18\n");
+	}
+
+	unittest(ovcs_id, "ovcs_id removed for overlay_18\n");
+
+	/* ---  overlay 19  --- */
+
+	unittest(overlay_data_apply("overlay_19", &ovcs_id),
+		 "OF_OVERLAY_POST_REMOVE notify injected error\n");
+
+	unittest(ovcs_id, "ovcs_id not created for overlay_19\n");
+
+	if (ovcs_id) {
+		EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus");
+		ret = of_overlay_remove(&ovcs_id);
+		EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus");
+		if (ret == -ENODEV)
+			unittest(1, "overlay_19 of_overlay_remove() injected error for OF_OVERLAY_POST_REMOVE\n");
+		else
+			unittest(0, "overlay_19 of_overlay_remove() injected error for OF_OVERLAY_POST_REMOVE not returned\n");
+	} else {
+		unittest(1, "ovcs_id removed for overlay_19\n");
+	}
+
+	unittest(!ovcs_id, "changeset ovcs_id = %d not removed for overlay_19\n",
+		 ovcs_id);
+
+	/* ---  overlay 20  --- */
+
+	unittest(overlay_data_apply("overlay_20", &ovcs_id),
+		 "overlay notify no injected error\n");
+
+	if (ovcs_id) {
+		ret = of_overlay_remove(&ovcs_id);
+		if (ret)
+			unittest(1, "overlay_20 failed to be destroyed, ret = %d\n",
+				 ret);
+	} else {
+		unittest(1, "ovcs_id not created for overlay_20\n");
+	}
+
+	unittest(!of_overlay_notifier_unregister(&of_nb),
+		 "of_overlay_notifier_unregister() failed, ret = %d\n", ret);
+}
+
 static void __init of_unittest_overlay(void)
 {
 	struct device_node *bus_np = NULL;
@@ -2804,6 +2990,8 @@ static void __init of_unittest_overlay(void)
 
 	of_unittest_remove_tracked_overlays();
 
+	of_unittest_overlay_notify();
+
 out:
 	of_node_put(bus_np);
 }
@@ -2855,6 +3043,11 @@ OVERLAY_INFO_EXTERN(overlay_11);
 OVERLAY_INFO_EXTERN(overlay_12);
 OVERLAY_INFO_EXTERN(overlay_13);
 OVERLAY_INFO_EXTERN(overlay_15);
+OVERLAY_INFO_EXTERN(overlay_16);
+OVERLAY_INFO_EXTERN(overlay_17);
+OVERLAY_INFO_EXTERN(overlay_18);
+OVERLAY_INFO_EXTERN(overlay_19);
+OVERLAY_INFO_EXTERN(overlay_20);
 OVERLAY_INFO_EXTERN(overlay_gpio_01);
 OVERLAY_INFO_EXTERN(overlay_gpio_02a);
 OVERLAY_INFO_EXTERN(overlay_gpio_02b);
@@ -2885,6 +3078,11 @@ static struct overlay_info overlays[] = {
 	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),
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 3/3] of: overlay: do not free changeset when of_overlay_apply returns error

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

New unittests for overlay notifiers reveal a memory leak in
of_overlay_apply() when a notifier returns an error for action
OF_OVERLAY_POST_APPLY.  The pr_err() message is:

   OF: ERROR: memory leak, expected refcount 1 instead of 3,
   of_node_get()/of_node_put() unbalanced - destroy cset entry: attach
   overlay node /testcase-data/overlay-node/test-bus/test-unittest17

Change the error path to no longer call free_overlay_changeset(),
and document that the caller of of_overlay_fdt_apply() may choose
to remove the overlay.

Update the unittest that triggered the error to expect the changed
return values and to call of_overlay_remove().

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

===

Output of the new overlay notifier unittests, as filtered by
scripts/dtc/of_unittest_expect:

   ### dt-test ### pass of_unittest_overlay_notify():2825
ok OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2846
   ### dt-test ### pass of_unittest_overlay_notify():2851
ok OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2857
   ### dt-test ### pass of_unittest_overlay_notify():2862
   ### dt-test ### pass of_unittest_overlay_notify():2866
   ### dt-test ### pass of_unittest_overlay_notify():2872
   ### dt-test ### pass of_unittest_overlay_notify():2875
ok OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2886
   ### dt-test ### pass of_unittest_overlay_notify():2894
   ### dt-test ### pass of_unittest_overlay_notify():2898
   ### dt-test ### pass of_unittest_overlay_notify():2901
ok OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus
   ### dt-test ### pass of_unittest_overlay_notify():2908
   ### dt-test ### pass of_unittest_overlay_notify():2915
   ### dt-test ### pass of_unittest_overlay_notify():2920
   ### dt-test ### pass of_unittest_overlay_notify():2932


 drivers/of/overlay.c  | 29 ++++++++++++++++++++++++++---
 drivers/of/unittest.c | 10 ++++++++--
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 48c240b06d3b..4c1ac36216b8 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -954,6 +954,25 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
 	return ret;
 }
 
+/*
+ * of_overlay_fdt_apply() - Create and apply an overlay changeset
+ * @overlay_fdt:	pointer to overlay FDT
+ * @overlay_fdt_size:	number of bytes in @overlay_fdt
+ * @ret_ovcs_id:	pointer for returning created changeset id
+ *
+ * Creates and applies an overlay changeset.
+ *
+ * See of_overlay_apply() for important behavior information.
+ *
+ * Return: 0 on success, or a negative error number.  *@ret_ovcs_id is set to
+ * the value of overlay changeset id, which can be passed to of_overlay_remove()
+ * to remove the overlay.
+ *
+ * On error return, the changeset may be partially applied.  This is especially
+ * likely if an OF_OVERLAY_POST_APPLY notifier returns an error.  In this case
+ * the caller should call of_overlay_remove() with the value in *@ret_ovcs_id.
+ */
+
 int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 			 int *ret_ovcs_id)
 {
@@ -1021,15 +1040,19 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 	ovcs->overlay_mem = overlay_mem;
 
 	ret = of_overlay_apply(ovcs);
-	if (ret < 0)
-		goto err_free_ovcs;
+	/*
+	 * If of_overlay_apply() error, calling free_overlay_changeset() may
+	 * result in a memory leak if the apply partly succeeded, so do NOT
+	 * goto err_free_ovcs.  Instead, the caller of of_overlay_fdt_apply()
+	 * can call of_overlay_remove();
+	 */
 
 	mutex_unlock(&of_mutex);
 	of_overlay_mutex_unlock();
 
 	*ret_ovcs_id = ovcs->id;
 
-	return 0;
+	return ret;
 
 err_free_ovcs:
 	free_overlay_changeset(ovcs);
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e28c3df2c4c2..dff55ae09d97 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2848,7 +2848,7 @@ static void __init of_unittest_overlay_notify(void)
 
 	EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus");
 
-	unittest(!ovcs_id, "ovcs_id created for overlay_16\n");
+	unittest(ovcs_id, "ovcs_id not created for overlay_16\n");
 
 	/* ---  overlay 17  --- */
 
@@ -2859,7 +2859,13 @@ static void __init of_unittest_overlay_notify(void)
 
 	EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus");
 
-	unittest(!ovcs_id, "ovcs_id created for overlay_17\n");
+	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  --- */
 
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH 1/3] of: overlay: add entry to of_overlay_action_name[]

[Rob Herring]

On Sat, Apr 30, 2022 at 07:05:41PM -0500, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> The values of enum of_overlay_notify_action are used to index into
> array of_overlay_action_name.  Add an entry to of_overlay_action_name
> for the value recently added to of_overlay_notify_action.
> 
> Array of_overlay_action_name[] is moved into include/linux/of.h
> adjacent to enum of_overlay_notify_action to make the connection
> between the two more obvious if either is modified in the future.
> 
> The only use of of_overlay_action_name is for error reporting in
> overlay_notify().  All callers of overlay_notify() report the same
> error, but with fewer details.  Remove the redundant error reports
> in the callers.
> 
> Fixes: 067c098766c6 ("of: overlay: rework overlay apply and remove kfree()s")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  drivers/of/overlay.c | 27 +++++----------------------
>  include/linux/of.h   | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 22 deletions(-)

This isn't applying for me.

Rob

Re: [PATCH 1/3] of: overlay: add entry to of_overlay_action_name[]

[Frank Rowand]

On 5/2/22 12:00, Rob Herring wrote:
> On Sat, Apr 30, 2022 at 07:05:41PM -0500, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> The values of enum of_overlay_notify_action are used to index into
>> array of_overlay_action_name.  Add an entry to of_overlay_action_name
>> for the value recently added to of_overlay_notify_action.
>>
>> Array of_overlay_action_name[] is moved into include/linux/of.h
>> adjacent to enum of_overlay_notify_action to make the connection
>> between the two more obvious if either is modified in the future.
>>
>> The only use of of_overlay_action_name is for error reporting in
>> overlay_notify().  All callers of overlay_notify() report the same
>> error, but with fewer details.  Remove the redundant error reports
>> in the callers.
>>
>> Fixes: 067c098766c6 ("of: overlay: rework overlay apply and remove kfree()s")
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>  drivers/of/overlay.c | 27 +++++----------------------
>>  include/linux/of.h   | 13 +++++++++++++
>>  2 files changed, 18 insertions(+), 22 deletions(-)
> 
> This isn't applying for me.

Weird, patch can apply it, but 'git am' does not work.  I see that when
I try that on your dt/next branch.

The problem seems to be that I did not create the series on top of
dt/next: 5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}.

I have rebased the series on top of that patch and am sending v2.

-Frank


> 
> Rob

Re: [PATCH 1/3] of: overlay: add entry to of_overlay_action_name[]

[Rob Herring]

On Mon, May 2, 2022 at 1:17 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 5/2/22 12:00, Rob Herring wrote:
> > On Sat, Apr 30, 2022 at 07:05:41PM -0500, frowand.list@gmail.com wrote:
> >> From: Frank Rowand <frank.rowand@sony.com>
> >>
> >> The values of enum of_overlay_notify_action are used to index into
> >> array of_overlay_action_name.  Add an entry to of_overlay_action_name
> >> for the value recently added to of_overlay_notify_action.
> >>
> >> Array of_overlay_action_name[] is moved into include/linux/of.h
> >> adjacent to enum of_overlay_notify_action to make the connection
> >> between the two more obvious if either is modified in the future.
> >>
> >> The only use of of_overlay_action_name is for error reporting in
> >> overlay_notify().  All callers of overlay_notify() report the same
> >> error, but with fewer details.  Remove the redundant error reports
> >> in the callers.
> >>
> >> Fixes: 067c098766c6 ("of: overlay: rework overlay apply and remove kfree()s")
> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> >> ---
> >>  drivers/of/overlay.c | 27 +++++----------------------
> >>  include/linux/of.h   | 13 +++++++++++++
> >>  2 files changed, 18 insertions(+), 22 deletions(-)
> >
> > This isn't applying for me.
>
> Weird, patch can apply it, but 'git am' does not work.  I see that when
> I try that on your dt/next branch.
>
> The problem seems to be that I did not create the series on top of
> dt/next: 5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}.

It should be more than just that. It was also not any base I have in
my tree, so 'git am' wouldn't try a 3-way merge either.

> I have rebased the series on top of that patch and am sending v2.

Thanks.

Rob

Re: [PATCH 1/3] of: overlay: add entry to of_overlay_action_name[]

[Frank Rowand]

On 5/2/22 13:29, Rob Herring wrote:
> On Mon, May 2, 2022 at 1:17 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 5/2/22 12:00, Rob Herring wrote:
>>> On Sat, Apr 30, 2022 at 07:05:41PM -0500, frowand.list@gmail.com wrote:
>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>
>>>> The values of enum of_overlay_notify_action are used to index into
>>>> array of_overlay_action_name.  Add an entry to of_overlay_action_name
>>>> for the value recently added to of_overlay_notify_action.
>>>>
>>>> Array of_overlay_action_name[] is moved into include/linux/of.h
>>>> adjacent to enum of_overlay_notify_action to make the connection
>>>> between the two more obvious if either is modified in the future.
>>>>
>>>> The only use of of_overlay_action_name is for error reporting in
>>>> overlay_notify().  All callers of overlay_notify() report the same
>>>> error, but with fewer details.  Remove the redundant error reports
>>>> in the callers.
>>>>
>>>> Fixes: 067c098766c6 ("of: overlay: rework overlay apply and remove kfree()s")
>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>>> ---
>>>>  drivers/of/overlay.c | 27 +++++----------------------
>>>>  include/linux/of.h   | 13 +++++++++++++
>>>>  2 files changed, 18 insertions(+), 22 deletions(-)
>>>
>>> This isn't applying for me.
>>
>> Weird, patch can apply it, but 'git am' does not work.  I see that when
>> I try that on your dt/next branch.
>>
>> The problem seems to be that I did not create the series on top of
>> dt/next: 5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}.
> 
> It should be more than just that. It was also not any base I have in
> my tree, so 'git am' wouldn't try a 3-way merge either.

That was the only thing patch mentioned when it applied successfully,
a little bit of fuzz around the few lines in that patch.  I thought
it a little weird that 'git am' didn't handle that, but it was easy
enough to rebase instead of debugging git.

> 
>> I have rebased the series on top of that patch and am sending v2.
> 
> Thanks.

I just now tried the v2 series emails on top of your dt/next and 'git am'
is happy with it.

> 
> Rob