Re: [PATCH V7 1/3] of: dynamic: Add interfaces for creating device node dynamically
From: Lizhi Hou
Date: Fri Mar 24 2023 - 17:26:49 EST
On 3/24/23 07:14, Rob Herring wrote:
On Thu, Mar 23, 2023 at 9:12 PM Lizhi Hou <lizhi.hou@xxxxxxx> wrote:
On 3/23/23 15:40, Rob Herring wrote:
On Thu, Jan 19, 2023 at 9:02 PM Lizhi Hou <lizhi.hou@xxxxxxx> wrote:
of_create_node() creates device node dynamically. The parent device node
and full name are required for creating the node. It optionally creates
an OF changeset and attaches the newly created node to the changeset. The
device node pointer and the changeset pointer can be used to add
properties to the device node and apply the node to the base tree.
of_destroy_node() frees the device node created by of_create_node(). If
an OF changeset was also created for this node, it will destroy the
changeset before freeing the device node.
Expand of_changeset APIs to handle specific types of properties.
of_changeset_add_prop_string()
of_changeset_add_prop_string_array()
of_changeset_add_prop_u32_array()
Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxx>
Your Sob should be last because you sent this patch. The order of Sob
is roughly the order of possession of the patch.
Got it.
Signed-off-by: Sonal Santan <sonal.santan@xxxxxxx>
Signed-off-by: Max Zhen <max.zhen@xxxxxxx>
So Sonal and Max modified this patch?
They did not directly modify the code. And we discussed the design
together. They also reviewed the patch before I sent it out. Please let
me know if other keyword should be used in this case.
Reviewed-by or nothing. Some feel that only reviews on public lists
should get that tag and internal, private reviews don't matter.
Reviewed-by: Brian Xu <brian.xu@xxxxxxx>
Signed-off-by: Clément Léger <clement.leger@xxxxxxxxxxx>
Why does this have Clément's Sob?
I referenced Clément 's code and used one portion in my first patch
series. And I re-implemented it later to address the code review
comments/requests.
Then it goes first or you can use the 'Co-developed-by' tag.
---
drivers/of/dynamic.c | 197 +++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 24 ++++++
2 files changed, 221 insertions(+)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..4e211a1d039f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -461,6 +461,71 @@ struct device_node *__of_node_dup(const struct device_node *np,
return NULL;
}
+/**
+ * of_create_node - Dynamically create a device node
For consistency, I think this should be of_changeset_create_node().
Sure.
+ *
+ * @parent: Pointer to parent device node
+ * @full_name: Node full name
+ * @cset: Pointer to returning changeset
+ *
+ * Return: Pointer to the created device node or NULL in case of an error.
+ */
+struct device_node *of_create_node(struct device_node *parent,
+ const char *full_name,
+ struct of_changeset **cset)
+{
+ struct of_changeset *ocs;
+ struct device_node *np;
+ int ret;
+
+ np = __of_node_dup(NULL, full_name);
+ if (!np)
+ return NULL;
+ np->parent = parent;
+
+ if (!cset)
+ return np;
+
+ ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
+ if (!ocs) {
+ of_node_put(np);
+ return NULL;
+ }
+
+ of_changeset_init(ocs);
+ ret = of_changeset_attach_node(ocs, np);
+ if (ret) {
+ of_changeset_destroy(ocs);
+ of_node_put(np);
+ kfree(ocs);
+ return NULL;
+ }
+
+ np->data = ocs;
+ *cset = ocs;
+
+ return np;
+}
+EXPORT_SYMBOL(of_create_node);
+
+/**
+ * of_destroy_node - Destroy a dynamically created device node
+ *
+ * @np: Pointer to dynamically created device node
+ *
+ */
+void of_destroy_node(struct device_node *np)
+{
+ struct of_changeset *ocs;
+
+ if (np->data) {
+ ocs = (struct of_changeset *)np->data;
+ of_changeset_destroy(ocs);
+ }
+ of_node_put(np);
A sequence like this would be broken:
np = of_create_node()
of_node_get(np)
of_destroy_node(np)
The put here won't free the node because it still has a ref, but we
just freed the changeset. For this to work correctly, we would need
the release function to handle np->data instead. However, all users of
data aren't a changeset.
I'm failing to remember why we're storing the changeset in 'data', but
there doesn't seem to be a reason now so I think that can just be
dropped. Then if you want to free the node, you'd just do an
of_node_put(). (And maybe after the node is attached you do a put too,
because the attach does a get. Not completely sure.)
The question is how to save changeset and free it later. I used global
link list to track the changeset been created.
Storing the changeset in 'data' can avoid using the global link list.
To use of_node_put() to free both node and changeset, I think we can
1) add a new flag, then in of_node_release() we can know np->data is
changeset by checking the flag.
2) When creating node, allocate extra memory for changeset and set
np->data to a global function of_free_dynamic_node().
In of_node_release(), check if np->data == of_free_dynamic_node,
call of_free_dynamic_node(np).
in of_free_dynamic_node(), free changeset by
of_changeset_destroy(np+1)
Does this make sense to you? If yes, 1) or 2) sounds better?
Neither works. Changesets and nodes are not 1:1 in general though they
are in your use. So you can use the data ptr, but the caller has to
decide that, not the DT core code.
Ok. In of_pci_make_dev_node(), I can do
ocs = kmalloc(*ocs);
of_changeset_init(ocs);
np = of_changeset_create_node(ocs, name);
np->data = ocs;
Then in of_pci_remove_node(), I can do
if (!np || !of_node_check_flag(np, OF_DYNAMIC)) return;
of_changeset_destroy(np->data);
of_node_put(np);
Does this sound reasonable?
Thanks,
Lizhi
Rob