* [PATCH v2 1/3] of: overlay: add entry to of_overlay_action_name[]
2022-05-02 18:17 [PATCH v2 0/3] of: overlay: unittest: fix overlay notify error, add overlay notifier tests, fix bug revealed by new unittests frowand.list
@ 2022-05-02 18:17 ` frowand.list
2022-05-03 0:56 ` Rob Herring
2022-05-02 18:17 ` [PATCH v2 2/3] of: overlay: unittest: add tests for overlay notifiers frowand.list
2022-05-02 18:17 ` [PATCH v2 3/3] of: overlay: do not free changeset when of_overlay_apply returns error frowand.list
2 siblings, 1 reply; 7+ messages in thread
From: frowand.list @ 2022-05-02 18:17 UTC (permalink / raw)
To: Rob Herring, pantelis.antoniou, Slawomir Stepien, Dan Carpenter
Cc: devicetree, linux-kernel, Slawomir Stepien, Jan Kiszka,
Geert Uytterhoeven
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>
---
Changes since version 1:
- patch 1/1 v1 did not apply on Rob's dt/next branch, rebase on top of
5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}
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 bd522da7f76b..ae5ea5b1079b 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)
{
@@ -178,7 +171,7 @@ static int overlay_notify(struct overlay_changeset *ovcs,
if (notifier_to_errno(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;
}
}
@@ -925,10 +918,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)
@@ -951,12 +942,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);
@@ -1192,10 +1180,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);
@@ -1218,12 +1204,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>
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/3] of: overlay: unittest: add tests for overlay notifiers
2022-05-02 18:17 [PATCH v2 0/3] of: overlay: unittest: fix overlay notify error, add overlay notifier tests, fix bug revealed by new unittests frowand.list
2022-05-02 18:17 ` [PATCH v2 1/3] of: overlay: add entry to of_overlay_action_name[] frowand.list
@ 2022-05-02 18:17 ` frowand.list
2022-05-03 0:58 ` Rob Herring
2022-05-02 18:17 ` [PATCH v2 3/3] of: overlay: do not free changeset when of_overlay_apply returns error frowand.list
2 siblings, 1 reply; 7+ messages in thread
From: frowand.list @ 2022-05-02 18:17 UTC (permalink / raw)
To: Rob Herring, pantelis.antoniou, Slawomir Stepien, Dan Carpenter
Cc: devicetree, linux-kernel, Slawomir Stepien, Jan Kiszka,
Geert Uytterhoeven
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>
---
Changes since version 1:
- patch 1/1 v1 did not apply on Rob's dt/next branch, rebase on top of
5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}
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>
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 3/3] of: overlay: do not free changeset when of_overlay_apply returns error
2022-05-02 18:17 [PATCH v2 0/3] of: overlay: unittest: fix overlay notify error, add overlay notifier tests, fix bug revealed by new unittests frowand.list
2022-05-02 18:17 ` [PATCH v2 1/3] of: overlay: add entry to of_overlay_action_name[] frowand.list
2022-05-02 18:17 ` [PATCH v2 2/3] of: overlay: unittest: add tests for overlay notifiers frowand.list
@ 2022-05-02 18:17 ` frowand.list
2022-05-03 0:59 ` Rob Herring
2 siblings, 1 reply; 7+ messages in thread
From: frowand.list @ 2022-05-02 18:17 UTC (permalink / raw)
To: Rob Herring, pantelis.antoniou, Slawomir Stepien, Dan Carpenter
Cc: devicetree, linux-kernel, Slawomir Stepien, Jan Kiszka,
Geert Uytterhoeven
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>
---
Changes since version 1:
- patch 1/1 v1 did not apply on Rob's dt/next branch, rebase on top of
5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}
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 ae5ea5b1079b..4044ddcb02c6 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -952,6 +952,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)
{
@@ -1019,15 +1038,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>
^ permalink raw reply related [flat|nested] 7+ messages in thread