[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path

From: wangyanan (Y)
Subject: Re: [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path
Date: Mon, 19 Apr 2021 15:02:46 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0

Hi David,

On 2021/4/19 9:13, David Gibson wrote:
On Sat, Apr 17, 2021 at 10:36:20AM +0800, wangyanan (Y) wrote:
Hi David,

On 2021/4/16 12:52, David Gibson wrote:
On Tue, Apr 13, 2021 at 04:07:40PM +0800, Yanan Wang wrote:
From: Andrew Jones <drjones@redhat.com>

qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except
it also adds any missing subnodes in the path. We also tweak
an error message of qemu_fdt_add_subnode().

We'll make use of this new function in a coming patch.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
   include/sysemu/device_tree.h |  1 +
   softmmu/device_tree.c        | 45 ++++++++++++++++++++++++++++++++++--
   2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 8a2fe55622..ef060a9759 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -121,6 +121,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
   uint32_t qemu_fdt_alloc_phandle(void *fdt);
   int qemu_fdt_nop_node(void *fdt, const char *node_path);
   int qemu_fdt_add_subnode(void *fdt, const char *name);
+int qemu_fdt_add_path(void *fdt, const char *path);
   #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                
       do {                                                                     
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 2691c58cf6..8592c7aa1b 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -541,8 +541,8 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
       retval = fdt_add_subnode(fdt, parent, basename);
       if (retval < 0) {
-        error_report("FDT: Failed to create subnode %s: %s", name,
-                     fdt_strerror(retval));
+        error_report("%s: Failed to create subnode %s: %s",
+                     __func__, name, fdt_strerror(retval));
@@ -550,6 +550,47 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
       return retval;
+ * Like qemu_fdt_add_subnode(), but will add all missing
+ * subnodes in the path.
+ */
+int qemu_fdt_add_path(void *fdt, const char *path)
+    char *dupname, *basename, *p;
+    int parent, retval = -1;
+    if (path[0] != '/') {
+        return retval;
+    }
+    parent = fdt_path_offset(fdt, "/");
Getting the offset for "/" is never needed - it's always 0.
Thanks, will fix it.
+    p = dupname = g_strdup(path);
You shouldn't need the strdup(), see below.

+    while (p) {
+        *p = '/';
+        basename = p + 1;
+        p = strchr(p + 1, '/');
+        if (p) {
+            *p = '\0';
+        }
+        retval = fdt_path_offset(fdt, dupname);
The fdt_path_offset_namelen() function exists *exactly* so that you
can look up partial parths without having to mangle your input
string.  Just set the namelen right, and it will ignore anything to
the right of that.
Function fdt_path_offset_namelen() seems more reasonable.

After we call qemu_fdt_add_path() to add "/cpus/cpu-map/socket0/core0"
if we want to add another path like "/cpus/cpu-map/socket0/core1" we will
get the error
-FDT_ERR_NOTFOUND for each partial path. But actually
already exists, so by using fdt_path_offset_namelen() with right namelen we
can avoid
the error retval for this part.
I don't quite follow what you're saying here.  AFAICT your logic was
correct - it just involved a lot of mangling the given path (adding
and removing \0s) which becomes unnecessary with
+        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
+            error_report("%s: Invalid path %s: %s",
+                         __func__, path, fdt_strerror(retval));
If you're getting an error other than FDT_ERR_NOTFOUND here, chances
are it's not an invalid path, but a corrupted fdt blob or something
Right, there can be variable reasons for the fail in addition to the invalid

+            exit(1);
+        } else if (retval == -FDT_ERR_NOTFOUND) {
+            retval = fdt_add_subnode(fdt, parent, basename);
+            if (retval < 0) {
+                break;
+            }
I found another question here. If path "/cpus/cpu-map/socket0/core0" has
been added, when we want to add another path "/cpus/cpu-map/socket0/core1"
and go here with retval = fdt_add_subnode(fdt, parent, "cpus"), then retval
be -FDT_ERR_EXISTS, but we can't just break the loop in this case.

Am I right of the explanation ?
Not exactly.  If you called that fdt_add_subnode() in that case you
would get FDT_ERR_EXISTS.  However, because the previous
fdt_path_offset() has returned -FDT_ERR_NOTFOUND, you've already
established that the partial path doesn't exist, so if you got an
FDT_ERR_EXISTS here something has gone very strangely wrong (such as
concurrent access to the flat tree, which would very likely corrupt
Thanks for the analysis, I was wrong...
Note that the repeated use of fdt_path_offset() in this function will
repeatedly rescan the whole fdt from the beginning.  If you're
concerned about performance (which you might well not be), then a more
efficient approach would be to take your input path component by
component and use fdt_subnode_offset() directly.
I will try to address your comments and have some rework for this patch in next version.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]