Re: [U-Boot] Subject: [PATCH] libfdt: Add fdt functionality for more intuitive fdt handling

On Mon, May 07, 2012 at 02:56:54PM +0200, Peter Feuerer wrote:

libfdt: Add fdt functionality for more intuitive fdt handling

Hrm, "more intuitive fdt handling" is a bit of a stretch. It makes certain common operations simpler.

New functions: fdt_read - retrieve the value of a property by full path fdt_write - create or change a property with full path and create subnodes if needed

I don't love these names (too vague). fdt_{get,set}prop_global() perhaps.

fdt_create_path - create subnode path with parents

Signed-off-by: Peter Feuerer peter.feuerer@sysgo.com CC: David Gibson david@gibson.dropbear.id.au CC: Gerald Van Baren gvb@unssw.com

---

include/libfdt.h | 93 ++++++++++++++++++++++++++++++++++++++++ lib/libfdt/fdt_ro.c | 30 +++++++++++++ lib/libfdt/fdt_rw.c | 97 ++++++++++++++++++++++++++++++++++++++++++ lib/libfdt/libfdt_internal.h | 4 ++ 4 files changed, 224 insertions(+), 0 deletions(-)

You're patching the core libfdt code, so you should make a patch against upstream libfdt, rather than just the u-boot embedded copy.

diff --git a/include/libfdt.h b/include/libfdt.h index de82ed5..822ab18 100644 --- a/include/libfdt.h +++ b/include/libfdt.h @@ -548,6 +548,37 @@ static inline void *fdt_getprop_w(void *fdt, int nodeoffset, }

/**

• • fdt_read - retrieve the value of a property by full path
• • @fdt: pointer to the device tree blob
• • @path: string containing the absolute path of the property
• • @name: name of the property
• • @lenp: pointer to an integer variable (will be overwritten) or NULL
• • fdt_getprop() retrieves a pointer to the value of the property

This doesn't appear to have been updated for the new function.

• • named 'name' of the node at offset nodeoffset (this will be a
• • pointer to within the device blob itself, not a copy of the value).
• • If lenp is non-NULL, the length of the property value is also
• • returned, in the integer pointed to by lenp.
• • returns:
• • pointer to the property's value

• • if lenp is non-NULL, *lenp contains the length of the property
    

• • value (>=0)
    

• • NULL, on error

• • if lenp is non-NULL, *lenp contains an error code (<0):
    

• • -FDT_ERR_NOTFOUND, node does not have named property
    

• • -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
    

• • -FDT_ERR_BADPATH,
    

• • -FDT_ERR_BADMAGIC,
    

• • -FDT_ERR_BADVERSION,
    

• • -FDT_ERR_BADSTATE,
    

• • -FDT_ERR_BADSTRUCTURE,
    

• • -FDT_ERR_TRUNCATED, standard meanings
    

• */

+const void *fdt_read(const void *fdt, const char *path,

• 	const char *name, int *lenp);
  

+/**

• fdt_get_phandle - retrieve the phandle of a given node
• @fdt: pointer to the device tree blob
• @nodeoffset: structure block offset of the node

@@ -1066,6 +1097,38 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name); */ int fdt_setprop(void *fdt, int nodeoffset, const char *name, const void *val, int len); +/**

• • fdt_write - create or change a property with full path and create

• •         all subnodes to the property if needed
    

• • @fdt: pointer to the device tree blob
• • @path: string containing the absolute path of the property
• • @name: name of the property to change
• • @val: pointer to data to set the property value to
• • @len: length of the property value
• • fdt_write() sets the value of the named property in the given
• • node to the given value and length, creating the property if it
• • does not already exist. Additionally it creates all parent nodes if
• • not yet existing.
• • This function may insert or delete data from the blob, and will
• • therefore change the offsets of some existing nodes.
• • returns:
• • 0, on success
• • -FDT_ERR_NOSPACE, there is insufficient free space in the blob to

• • contain the new property value
    

• • -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
• • -FDT_ERR_BADLAYOUT,
• • -FDT_ERR_BADMAGIC,
• • -FDT_ERR_BADVERSION,
• • -FDT_ERR_BADSTATE,
• • -FDT_ERR_BADSTRUCTURE,
• • -FDT_ERR_BADLAYOUT,
• • -FDT_ERR_TRUNCATED, standard meanings
• */

+int fdt_write(void *fdt, const char *path, const char *name,

• const void *val, int len);
  
  

/**

• fdt_setprop_cell - set a property to a single cell value

@@ -1204,6 +1267,36 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset, int fdt_add_subnode(void *fdt, int parentoffset, const char *name);

/**

• • fdt_create_path - create subnode path with parents
• • @fdt: pointer to the device tree blob
• • @path: absolute path of subnodes to create
• • fdt_create_path() creates absolute subnode path with all needed
• • parents, if they don't exist.
• • This function will insert data into the blob, and will therefore
• • change the offsets of some existing nodes.
• • returns:
• • structure block offset of the created nodeequested subnode (>=0), on success
• • -FDT_ERR_NOTFOUND, if the requested subnode does not exist
• • -FDT_ERR_BADOFFSET, if parentoffset did not point to an FDT_BEGIN_NODE tag
• • -FDT_ERR_EXISTS, if the node at parentoffset already has a subnode of

• • the given name
    

• • -FDT_ERR_NOSPACE, if there is insufficient free space in the

• • blob to contain the new node
    

• • -FDT_ERR_NOSPACE,
• • -FDT_ERR_BADPATH,
• • -FDT_ERR_BADLAYOUT,
• • -FDT_ERR_BADMAGIC,
• • -FDT_ERR_BADVERSION,
• • -FDT_ERR_BADSTATE,
• • -FDT_ERR_BADSTRUCTURE,
• • -FDT_ERR_TRUNCATED, standard meanings.
• */

+int fdt_create_path(struct fdt_header *fdt, const char *path);

This should take a void *fdt like everything else, not a struct fdt_header *.

+/**

• fdt_del_node - delete a node (subtree)
• @fdt: pointer to the device tree blob
• @nodeoffset: offset of the node to nop

diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c index 1933010..9a08200 100644 --- a/lib/libfdt/fdt_ro.c +++ b/lib/libfdt/fdt_ro.c @@ -324,6 +324,36 @@ const void *fdt_getprop(const void *fdt, int nodeoffset, return fdt_getprop_namelen(fdt, nodeoffset, name, strlen(name), lenp); }

+const void *fdt_read(const void *fdt, const char *path,

• 	const char *name, int *lenp)
  

+{

• int nodeoffset;
• int len;
• const void *nodep ;
• if (fdt_check_header(fdt) != 0) {

• if (lenp)
  

• 	*lenp = -FDT_ERR_BADSTRUCTURE;
  

Uh.. you should set *lenp to the error returned by fdt_check_header(), not just assume FDT_ERR_BADSTRUCTURE.

• return NULL;
  

• }
• nodeoffset = fdt_path_offset(fdt, path);
• if (nodeoffset < 0) {

• if (lenp)
  

• 	*lenp = -FDT_ERR_BADPATH;
  

Likewise here you should actually use the error returned by fdt_path_offset().

• return NULL;
  

• } else {

• nodep = fdt_getprop(fdt, nodeoffset, name, &len);
  

• if (len <= 0)
  

• 	return NULL;
  

• if (lenp)
  

• 	*lenp = len;
  

• }
• return nodep;

+}

uint32_t fdt_get_phandle(const void *fdt, int nodeoffset) { const uint32_t *php; diff --git a/lib/libfdt/fdt_rw.c b/lib/libfdt/fdt_rw.c index 5c27a67..ccebafb 100644 --- a/lib/libfdt/fdt_rw.c +++ b/lib/libfdt/fdt_rw.c @@ -293,6 +293,21 @@ int fdt_setprop(void *fdt, int nodeoffset, const char *name, return 0; }

+int fdt_write(void *fdt, const char *path, const char *name,

• const void *val, int len)
  

+{

• int nodeoffset;
• FDT_CHECK_HEADER(fdt);
• nodeoffset = fdt_create_path(fdt, path);
• if (nodeoffset < 0)

• return nodeoffset;
  

• return fdt_setprop(fdt, nodeoffset, name, val, len);

+}

int fdt_delprop(void *fdt, int nodeoffset, const char *name) { struct fdt_property *prop; @@ -354,6 +369,88 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name) return fdt_add_subnode_namelen(fdt, parentoffset, name, strlen(name)); }

+int fdt_create_path_internal(struct fdt_header *fdt, char *path) +{

• int offset = 0;
• char *p = NULL;
• char *last_slash = NULL;
• char *path2 = path;
• int ret = 0;
• /* search for last slash */
• for (p = path; *p != 0; ++p)

• if (*p == '/')
  

• 	last_slash = p;
  

Hrm. It might be worth adding strrchr() to libfdt's external dependencies rather than open coding it here (we already use strchr(), so it's not a big imposition).

• /* if path ends with "/" then, error */
• if (*(last_slash + 1) == 0)

• return -FDT_ERR_BADPATH;
  

• /*

• * if last_slash is root, use "/" instead of string
  

• * otherwise terminate string at slash
  

• */
  

• if (last_slash == path)

• path2 = "/";
  

• else

• *last_slash = 0;
  

In place changing the arguments is rather nasty.

• /*

• * test if the parent path is valid, if not valid,
  

• * recursively add paths
  

• */
  

• offset = fdt_path_offset(fdt, path2);
• if (offset == -FDT_ERR_NOTFOUND)

• offset = fdt_create_path_internal(fdt, path2);
  

Hrm. Use of recursion might limit libfdt's usability in stack-constrained environments - I'd prefer an iterative solution.

• if (offset < 0)

• return offset;
  

• /* path valid, create subnode */
• ret = fdt_add_subnode(fdt, offset, last_slash + 1);
• /* undo termination */
• if (last_slash && last_slash != path)

• *last_slash = '/';
  

• return ret;

+}

+int fdt_create_path(struct fdt_header *fdt, const char *path) +{

• int offset = 0;
• int ret = 0;
• int size = 0;

+#ifndef FDT_USE_MALLOC

Eugh. This horrible ifdef must die.

• char path_mem[FDT_MAX_PATHLEN] = {0};
• size = FDT_MAX_PATHLEN;

+#else

• char *path_mem;
• size = strlen(path);
• path_mem = calloc(size, 1);
• if (!path_mem)

• return -FDT_ERR_NOSPACE;
  

+#endif

• /* if full path already exists, return the offset */
• offset = fdt_path_offset(fdt, path);
• if (offset >= 0) {

• ret = offset;
  

• goto end;
  

• }
• /* copy path to local path variable, so that we can modify it */
• strncpy(path_mem, path, size - 1);

So.. this adds strncpy() as an external dependency of libfdt, which should be noted in the patch comment. But, having to copy the path to a temporary is nasty anyway. Avoiding this sort of thing is why we have the various _namelen() versions of things internally. I'd prefer to add an fdt_path_offset_namelen() and use it here than have the copy.

But then, it might be better not to use fdt_path_offset() inside this function anyway. You have to step through the path components anyway, so it would really be no more complex to test for the existence of each path component one level at a time using fdt_subnode_offset_namelen().

That implementation would also lead, naturally, to a version of this function which takes a nodeoffset and a relative path.

• ret = fdt_create_path_internal(fdt, path_mem);

+end: +#ifdef FDT_USE_MALLOC

• free(path_mem);

+#endif

• return ret;

+}

int fdt_del_node(void *fdt, int nodeoffset) { int endoffset; diff --git a/lib/libfdt/libfdt_internal.h b/lib/libfdt/libfdt_internal.h index 381133b..786a052 100644 --- a/lib/libfdt/libfdt_internal.h +++ b/lib/libfdt/libfdt_internal.h @@ -55,6 +55,10 @@ #define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1)) #define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))

+#ifndef FDT_USE_MALLOC +#define FDT_MAX_PATHLEN 2048 +#endif

#define FDT_CHECK_HEADER(fdt) \ { \ int err; \