diff mbox series

[RFC,1/1] of: dynamic: allow freeing of_nodes after the overlay changeset

Message ID 20230511151047.1779841-2-nuno.sa@analog.com
State New
Headers show
Series of: dynamic: allow freeing of_nodes after the overlay changeset | expand

Commit Message

Nuno Sa May 11, 2023, 3:10 p.m. UTC
There are valid cases where we might get in here with an unexpected
refcount. One example are devlinks between suppliers <-> consumers.
When a link is created, it will grab a reference to both the supplier and
the consumer devices (which can, potentially, hold a reference to it's
of_node) and this reference is not synchronously dropped when unbinding the
device from the driver. Instead, a work item is queued (see
devlink_dev_release()). This async nature will make that
__of_changeset_entry_destroy() is reached with a refcount > 1. But,
eventually, all the refcounts are released and of_node_release() is
reached.

So, the best we can do is to just warn for a possible leak and "inform" the
node that the changeset is already gone so, if we do reach
of_node_release(), we can free the node instead assuming something bad
has happened.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/of/dynamic.c | 31 +++++++++++++++++++++++++++----
 include/linux/of.h   |  1 +
 2 files changed, 28 insertions(+), 4 deletions(-)
diff mbox series

Patch

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b170..354e641f63df 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -360,8 +360,10 @@  void of_node_release(struct kobject *kobj)
 		return;
 
 	if (of_node_check_flag(node, OF_OVERLAY)) {
-
-		if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
+		if (of_node_check_flag(node, OF_OVERLAY_CSET_GONE)) {
+			pr_info("INFO: node %s is now being freed after overlay changeset. All should be fine now...\n",
+				node->full_name);
+		} else if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
 			/* premature refcount of zero, do not free memory */
 			pr_err("ERROR: memory leak before free overlay changeset,  %pOF\n",
 			       node);
@@ -492,8 +494,29 @@  static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 	if (ce->action == OF_RECONFIG_ATTACH_NODE &&
 	    of_node_check_flag(ce->np, OF_OVERLAY)) {
 		if (kref_read(&ce->np->kobj.kref) > 1) {
-			pr_err("ERROR: memory leak, expected refcount 1 instead of %d, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node %pOF\n",
-			       kref_read(&ce->np->kobj.kref), ce->np);
+			pr_warn("WARNING: Possible memory leak, expected refcount 1 instead of %d, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node %pOF\n",
+				kref_read(&ce->np->kobj.kref), ce->np);
+			/*
+			 * There are valid cases where we might get in here with
+			 * an unexpected refcount. One example are devlinks
+			 * between suppliers <-> consumers. When a link is
+			 * created, it will grab a reference to both the
+			 * supplier and the consumer devices (which can,
+			 * potentially, hold a reference to it's of_node) and
+			 * this reference is not synchronously dropped when
+			 * unbinding the device from the driver. Instead, a work
+			 * item is queued (see devlink_dev_release()). This
+			 * async nature will make that
+			 * __of_changeset_entry_destroy() is reached with a
+			 * refcount > 1. But, eventually, all the refcounts are
+			 * released and of_node_release() is reached.
+			 *
+			 * So, the best we can do is to just warn for a possible
+			 * leak and "inform" the node that the changeset is already
+			 * gone so, if we do reach of_node_release(), we can free
+			 * the node.
+			 */
+			of_node_set_flag(ce->np, OF_OVERLAY_CSET_GONE);
 		} else {
 			of_node_set_flag(ce->np, OF_OVERLAY_FREE_CSET);
 		}
diff --git a/include/linux/of.h b/include/linux/of.h
index 6ecde0515677..4f562b3f601d 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -152,6 +152,7 @@  extern struct device_node *of_stdout;
 #define OF_POPULATED_BUS	4 /* platform bus created for children */
 #define OF_OVERLAY		5 /* allocated for an overlay */
 #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
+#define OF_OVERLAY_CSET_GONE	7 /* overlay cset already freed */
 
 #define OF_BAD_ADDR	((u64)-1)