Re: [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path
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
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
Signed-off-by: Yanan Wang
---
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));
exit(1);
}
@@ -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"
successfully,
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
"/cpus/cpu-map/socket0"
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
fdt_path_offset_namelen().
Right.
+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
else.
Right, there can be variable reasons for the fail in addition to the invalid
path.
+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
already
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
will
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
it).
Thanks for the analysis, I was wrong...
Note that the repeated use of
Re: [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path
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
> > >
> > > 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
> > > Signed-off-by: Yanan Wang
> > > ---
> > > 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));
> > > exit(1);
> > > }
> > > @@ -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"
> successfully,
> 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
> "/cpus/cpu-map/socket0"
> 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
fdt_path_offset_namelen().
> > > +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
> > else.
>
> Right, there can be variable reasons for the fail in addition to the invalid
> path.
>
> > > +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
> already
> 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
> will
> be -FDT_ERR_EXISTS, but we can't just break the loop in this case.
>
> Am I right of the explanation
Re: [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path
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
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
Signed-off-by: Yanan Wang
---
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));
exit(1);
}
@@ -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"
successfully,
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
"/cpus/cpu-map/socket0"
already exists, so by using fdt_path_offset_namelen() with right namelen
we can avoid
the error retval for this part.
+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
else.
Right, there can be variable reasons for the fail in addition to the
invalid path.
+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
already
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 will
be -FDT_ERR_EXISTS, but we can't just break the loop in this case.
Am I right of the explanation ?
Thanks,
Yanan
+}
+parent = retval;
+}
+
+g_free(dupname);
+return retval;
+}
+
void qemu_fdt_dumpdtb(void *fdt, int size)
{
const char *dumpdtb = current_machine->dumpdtb;
Re: [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path
On Tue, Apr 13, 2021 at 04:07:40PM +0800, Yanan Wang wrote:
> From: Andrew Jones
>
> 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
> Signed-off-by: Yanan Wang
> ---
> 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));
> exit(1);
> }
>
> @@ -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.
> +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.
> +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
else.
> +exit(1);
> +} else if (retval == -FDT_ERR_NOTFOUND) {
> +retval = fdt_add_subnode(fdt, parent, basename);
> +if (retval < 0) {
> +break;
> +}
> +}
> +parent = retval;
> +}
> +
> +g_free(dupname);
> +return retval;
> +}
> +
> void qemu_fdt_dumpdtb(void *fdt, int size)
> {
> const char *dumpdtb = current_machine->dumpdtb;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path
From: Andrew Jones
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
Signed-off-by: Yanan Wang
---
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));
exit(1);
}
@@ -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, "/");
+p = dupname = g_strdup(path);
+
+while (p) {
+*p = '/';
+basename = p + 1;
+p = strchr(p + 1, '/');
+if (p) {
+*p = '\0';
+}
+retval = fdt_path_offset(fdt, dupname);
+if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
+error_report("%s: Invalid path %s: %s",
+ __func__, path, fdt_strerror(retval));
+exit(1);
+} else if (retval == -FDT_ERR_NOTFOUND) {
+retval = fdt_add_subnode(fdt, parent, basename);
+if (retval < 0) {
+break;
+}
+}
+parent = retval;
+}
+
+g_free(dupname);
+return retval;
+}
+
void qemu_fdt_dumpdtb(void *fdt, int size)
{
const char *dumpdtb = current_machine->dumpdtb;
--
2.19.1
