[PATCH] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[PATCH] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

This can be useful in particular in the kernel when booting on systems
with FDT-emitting firmware that is out of date. Releases of kexec-tools
on ppc64 prior to the end of 2014 are notable examples of such.

Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
---
 fdtget.c        |  4 +--
 libfdt/fdt.c    |  8 ++++--
 libfdt/fdt_ro.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 libfdt/libfdt.h |  5 +++-
 4 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index 6cc5242..34d8194 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
  */
 static int list_properties(const void *blob, int node)
 {
-	const struct fdt_property *data;
 	const char *name;
 	int prop;
 
@@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
 		/* Stop silently when there are no more properties */
 		if (prop < 0)
 			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
-		data = fdt_get_property_by_offset(blob, prop, NULL);
-		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
+		fdt_getprop_by_offset(blob, prop, &name, NULL);
 		if (name)
 			puts(name);
 		prop = fdt_next_property_offset(blob, prop);
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index fd13236..e783e6a 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 		return NULL;
 
 	if (fdt_version(fdt) >= 0x11)
-		if (((offset + len) < offset)
-		    || ((offset + len) > fdt_size_dt_struct(fdt)))
+		if (((offset + len) < offset) ||
+		    (fdt_version(fdt) >= 0x10 &&
+		     (offset + len) > fdt_size_dt_struct(fdt)))
 			return NULL;
 
 	return fdt_offset_ptr_(fdt, offset);
@@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		/* skip-name offset, length and value */
 		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
 			+ fdt32_to_cpu(*lenp);
+		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) > 8 &&
+		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
+			offset += 4;
 		break;
 
 	case FDT_END:
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index ce17814..025dd0f 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -233,16 +233,26 @@ int fdt_path_offset(const void *fdt, const char *path)
 const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 {
 	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
+	const char *nameptr;
 	int err;
 
 	if (((err = fdt_check_header(fdt)) != 0)
 	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
 			goto fail;
 
+	nameptr = nh->name;
+
+	if (fdt_version(fdt) < 0x10) {
+		const char *leaf;
+		leaf = strrchr(nameptr, '/');
+		if (leaf != NULL)
+			nameptr = leaf+1;
+	}
+
 	if (len)
-		*len = strlen(nh->name);
+		*len = strlen(nameptr);
 
-	return nh->name;
+	return nameptr;
 
  fail:
 	if (len)
@@ -268,9 +278,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
 	return nextprop_(fdt, offset);
 }
 
-const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
-						      int offset,
-						      int *lenp)
+static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
+						              int offset,
+						              int *lenp)
 {
 	int err;
 	const struct fdt_property *prop;
@@ -289,11 +299,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
 	return prop;
 }
 
+const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
+						      int offset,
+						      int *lenp)
+{
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
+	return _fdt_get_property_by_offset(fdt, offset, lenp);
+}
+
 const struct fdt_property *fdt_get_property_namelen(const void *fdt,
 						    int offset,
 						    const char *name,
 						    int namelen, int *lenp)
 {
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
 	for (offset = fdt_first_property_offset(fdt, offset);
 	     (offset >= 0);
 	     (offset = fdt_next_property_offset(fdt, offset))) {
@@ -324,12 +358,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 				const char *name, int namelen, int *lenp)
 {
-	const struct fdt_property *prop;
+	const struct fdt_property *prop = NULL;
+	int offset = nodeoffset;
 
-	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
-	if (!prop)
+	for (offset = fdt_first_property_offset(fdt, offset);
+	     (offset >= 0);
+	     (offset = fdt_next_property_offset(fdt, offset))) {
+		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
+			if (lenp)
+				*lenp = -FDT_ERR_INTERNAL;
+			return NULL;
+		}
+
+		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
+				   name, namelen))
+			break;
+	}
+
+	if (lenp && offset < 0)
+		*lenp = offset;
+
+	if (!prop || offset < 0)
 		return NULL;
 
+	/* Handle realignment */
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) > 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
@@ -338,11 +393,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 {
 	const struct fdt_property *prop;
 
-	prop = fdt_get_property_by_offset(fdt, offset, lenp);
+	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
 	if (!prop)
 		return NULL;
 	if (namep)
 		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) > 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index baa882c..7ac3e8a 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -54,7 +54,7 @@
 #include <libfdt_env.h>
 #include <fdt.h>
 
-#define FDT_FIRST_SUPPORTED_VERSION	0x10
+#define FDT_FIRST_SUPPORTED_VERSION	0x02
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
 /* Error codes: informative error codes */
@@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
  * fdt_property structure within the device tree blob at the given
  * offset.  If lenp is non-NULL, the length of the property value is
  * also returned, in the integer pointed to by lenp.
+ * 
+ * Note that this code only works on device tree versions >= 16. fdt_getprop()
+ * works on all versions.
  *
  * returns:
  *	pointer to the structure representing the property
-- 
2.14.1

Re: [PATCH] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

Has anyone had a chance to look at this? We need it, or something like 
it, to support some older firmwares with the FreeBSD kernel and I would 
like to make sure that our copy of libfdt does not diverge from upstream.

Thanks,
Nathan

On 12/07/17 22:31, Nathan Whitehorn wrote:
> This can be useful in particular in the kernel when booting on systems
> with FDT-emitting firmware that is out of date. Releases of kexec-tools
> on ppc64 prior to the end of 2014 are notable examples of such.
>
> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> ---
>   fdtget.c        |  4 +--
>   libfdt/fdt.c    |  8 ++++--
>   libfdt/fdt_ro.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
>   libfdt/libfdt.h |  5 +++-
>   4 files changed, 79 insertions(+), 15 deletions(-)
>
> diff --git a/fdtget.c b/fdtget.c
> index 6cc5242..34d8194 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
>    */
>   static int list_properties(const void *blob, int node)
>   {
> -	const struct fdt_property *data;
>   	const char *name;
>   	int prop;
>   
> @@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
>   		/* Stop silently when there are no more properties */
>   		if (prop < 0)
>   			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
> -		data = fdt_get_property_by_offset(blob, prop, NULL);
> -		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
> +		fdt_getprop_by_offset(blob, prop, &name, NULL);
>   		if (name)
>   			puts(name);
>   		prop = fdt_next_property_offset(blob, prop);
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index fd13236..e783e6a 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>   		return NULL;
>   
>   	if (fdt_version(fdt) >= 0x11)
> -		if (((offset + len) < offset)
> -		    || ((offset + len) > fdt_size_dt_struct(fdt)))
> +		if (((offset + len) < offset) ||
> +		    (fdt_version(fdt) >= 0x10 &&
> +		     (offset + len) > fdt_size_dt_struct(fdt)))
>   			return NULL;
>   
>   	return fdt_offset_ptr_(fdt, offset);
> @@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>   		/* skip-name offset, length and value */
>   		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>   			+ fdt32_to_cpu(*lenp);
> +		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) > 8 &&
> +		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> +			offset += 4;
>   		break;
>   
>   	case FDT_END:
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index ce17814..025dd0f 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -233,16 +233,26 @@ int fdt_path_offset(const void *fdt, const char *path)
>   const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>   {
>   	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
> +	const char *nameptr;
>   	int err;
>   
>   	if (((err = fdt_check_header(fdt)) != 0)
>   	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
>   			goto fail;
>   
> +	nameptr = nh->name;
> +
> +	if (fdt_version(fdt) < 0x10) {
> +		const char *leaf;
> +		leaf = strrchr(nameptr, '/');
> +		if (leaf != NULL)
> +			nameptr = leaf+1;
> +	}
> +
>   	if (len)
> -		*len = strlen(nh->name);
> +		*len = strlen(nameptr);
>   
> -	return nh->name;
> +	return nameptr;
>   
>    fail:
>   	if (len)
> @@ -268,9 +278,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
>   	return nextprop_(fdt, offset);
>   }
>   
> -const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> -						      int offset,
> -						      int *lenp)
> +static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
> +						              int offset,
> +						              int *lenp)
>   {
>   	int err;
>   	const struct fdt_property *prop;
> @@ -289,11 +299,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>   	return prop;
>   }
>   
> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> +						      int offset,
> +						      int *lenp)
> +{
> +	/* Prior to version 16, properties may need realignment
> +	 * and this API does not work. fdt_getprop_*() will, however. */
> +
> +	if (fdt_version(fdt) < 0x10) {
> +		if (lenp)
> +			*lenp = -FDT_ERR_BADVERSION;
> +		return NULL;
> +	}
> +
> +	return _fdt_get_property_by_offset(fdt, offset, lenp);
> +}
> +
>   const struct fdt_property *fdt_get_property_namelen(const void *fdt,
>   						    int offset,
>   						    const char *name,
>   						    int namelen, int *lenp)
>   {
> +	/* Prior to version 16, properties may need realignment
> +	 * and this API does not work. fdt_getprop_*() will, however. */
> +	if (fdt_version(fdt) < 0x10) {
> +		if (lenp)
> +			*lenp = -FDT_ERR_BADVERSION;
> +		return NULL;
> +	}
> +
>   	for (offset = fdt_first_property_offset(fdt, offset);
>   	     (offset >= 0);
>   	     (offset = fdt_next_property_offset(fdt, offset))) {
> @@ -324,12 +358,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
>   const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>   				const char *name, int namelen, int *lenp)
>   {
> -	const struct fdt_property *prop;
> +	const struct fdt_property *prop = NULL;
> +	int offset = nodeoffset;
>   
> -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
> -	if (!prop)
> +	for (offset = fdt_first_property_offset(fdt, offset);
> +	     (offset >= 0);
> +	     (offset = fdt_next_property_offset(fdt, offset))) {
> +		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
> +			if (lenp)
> +				*lenp = -FDT_ERR_INTERNAL;
> +			return NULL;
> +		}
> +
> +		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
> +				   name, namelen))
> +			break;
> +	}
> +
> +	if (lenp && offset < 0)
> +		*lenp = offset;
> +
> +	if (!prop || offset < 0)
>   		return NULL;
>   
> +	/* Handle realignment */
> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> +	    fdt32_to_cpu(prop->len) > 8)
> +		return prop->data + 4;
>   	return prop->data;
>   }
>   
> @@ -338,11 +393,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>   {
>   	const struct fdt_property *prop;
>   
> -	prop = fdt_get_property_by_offset(fdt, offset, lenp);
> +	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
>   	if (!prop)
>   		return NULL;
>   	if (namep)
>   		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> +
> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> +	    fdt32_to_cpu(prop->len) > 8)
> +		return prop->data + 4;
>   	return prop->data;
>   }
>   
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index baa882c..7ac3e8a 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -54,7 +54,7 @@
>   #include <libfdt_env.h>
>   #include <fdt.h>
>   
> -#define FDT_FIRST_SUPPORTED_VERSION	0x10
> +#define FDT_FIRST_SUPPORTED_VERSION	0x02
>   #define FDT_LAST_SUPPORTED_VERSION	0x11
>   
>   /* Error codes: informative error codes */
> @@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
>    * fdt_property structure within the device tree blob at the given
>    * offset.  If lenp is non-NULL, the length of the property value is
>    * also returned, in the integer pointed to by lenp.
> + *
> + * Note that this code only works on device tree versions >= 16. fdt_getprop()
> + * works on all versions.
>    *
>    * returns:
>    *	pointer to the structure representing the property

Re: [PATCH] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8477 bytes --]

On Fri, Dec 29, 2017 at 10:12:04AM -0800, Nathan Whitehorn wrote:
> Has anyone had a chance to look at this? We need it, or something like it,
> to support some older firmwares with the FreeBSD kernel and I would like to
> make sure that our copy of libfdt does not diverge from upstream.

Sorry, I've been busy and haven't had a chance to look at it.  I
haven't forgotten..

> 
> Thanks,
> Nathan
> 
> On 12/07/17 22:31, Nathan Whitehorn wrote:
> > This can be useful in particular in the kernel when booting on systems
> > with FDT-emitting firmware that is out of date. Releases of kexec-tools
> > on ppc64 prior to the end of 2014 are notable examples of such.
> > 
> > Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> > ---
> >   fdtget.c        |  4 +--
> >   libfdt/fdt.c    |  8 ++++--
> >   libfdt/fdt_ro.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >   libfdt/libfdt.h |  5 +++-
> >   4 files changed, 79 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fdtget.c b/fdtget.c
> > index 6cc5242..34d8194 100644
> > --- a/fdtget.c
> > +++ b/fdtget.c
> > @@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
> >    */
> >   static int list_properties(const void *blob, int node)
> >   {
> > -	const struct fdt_property *data;
> >   	const char *name;
> >   	int prop;
> > @@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
> >   		/* Stop silently when there are no more properties */
> >   		if (prop < 0)
> >   			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
> > -		data = fdt_get_property_by_offset(blob, prop, NULL);
> > -		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
> > +		fdt_getprop_by_offset(blob, prop, &name, NULL);
> >   		if (name)
> >   			puts(name);
> >   		prop = fdt_next_property_offset(blob, prop);
> > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > index fd13236..e783e6a 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> >   		return NULL;
> >   	if (fdt_version(fdt) >= 0x11)
> > -		if (((offset + len) < offset)
> > -		    || ((offset + len) > fdt_size_dt_struct(fdt)))
> > +		if (((offset + len) < offset) ||
> > +		    (fdt_version(fdt) >= 0x10 &&
> > +		     (offset + len) > fdt_size_dt_struct(fdt)))
> >   			return NULL;
> >   	return fdt_offset_ptr_(fdt, offset);
> > @@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> >   		/* skip-name offset, length and value */
> >   		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> >   			+ fdt32_to_cpu(*lenp);
> > +		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) > 8 &&
> > +		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> > +			offset += 4;
> >   		break;
> >   	case FDT_END:
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index ce17814..025dd0f 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -233,16 +233,26 @@ int fdt_path_offset(const void *fdt, const char *path)
> >   const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> >   {
> >   	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
> > +	const char *nameptr;
> >   	int err;
> >   	if (((err = fdt_check_header(fdt)) != 0)
> >   	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> >   			goto fail;
> > +	nameptr = nh->name;
> > +
> > +	if (fdt_version(fdt) < 0x10) {
> > +		const char *leaf;
> > +		leaf = strrchr(nameptr, '/');
> > +		if (leaf != NULL)
> > +			nameptr = leaf+1;
> > +	}
> > +
> >   	if (len)
> > -		*len = strlen(nh->name);
> > +		*len = strlen(nameptr);
> > -	return nh->name;
> > +	return nameptr;
> >    fail:
> >   	if (len)
> > @@ -268,9 +278,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
> >   	return nextprop_(fdt, offset);
> >   }
> > -const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> > -						      int offset,
> > -						      int *lenp)
> > +static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
> > +						              int offset,
> > +						              int *lenp)
> >   {
> >   	int err;
> >   	const struct fdt_property *prop;
> > @@ -289,11 +299,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> >   	return prop;
> >   }
> > +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> > +						      int offset,
> > +						      int *lenp)
> > +{
> > +	/* Prior to version 16, properties may need realignment
> > +	 * and this API does not work. fdt_getprop_*() will, however. */
> > +
> > +	if (fdt_version(fdt) < 0x10) {
> > +		if (lenp)
> > +			*lenp = -FDT_ERR_BADVERSION;
> > +		return NULL;
> > +	}
> > +
> > +	return _fdt_get_property_by_offset(fdt, offset, lenp);
> > +}
> > +
> >   const struct fdt_property *fdt_get_property_namelen(const void *fdt,
> >   						    int offset,
> >   						    const char *name,
> >   						    int namelen, int *lenp)
> >   {
> > +	/* Prior to version 16, properties may need realignment
> > +	 * and this API does not work. fdt_getprop_*() will, however. */
> > +	if (fdt_version(fdt) < 0x10) {
> > +		if (lenp)
> > +			*lenp = -FDT_ERR_BADVERSION;
> > +		return NULL;
> > +	}
> > +
> >   	for (offset = fdt_first_property_offset(fdt, offset);
> >   	     (offset >= 0);
> >   	     (offset = fdt_next_property_offset(fdt, offset))) {
> > @@ -324,12 +358,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
> >   const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> >   				const char *name, int namelen, int *lenp)
> >   {
> > -	const struct fdt_property *prop;
> > +	const struct fdt_property *prop = NULL;
> > +	int offset = nodeoffset;
> > -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
> > -	if (!prop)
> > +	for (offset = fdt_first_property_offset(fdt, offset);
> > +	     (offset >= 0);
> > +	     (offset = fdt_next_property_offset(fdt, offset))) {
> > +		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
> > +			if (lenp)
> > +				*lenp = -FDT_ERR_INTERNAL;
> > +			return NULL;
> > +		}
> > +
> > +		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
> > +				   name, namelen))
> > +			break;
> > +	}
> > +
> > +	if (lenp && offset < 0)
> > +		*lenp = offset;
> > +
> > +	if (!prop || offset < 0)
> >   		return NULL;
> > +	/* Handle realignment */
> > +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> > +	    fdt32_to_cpu(prop->len) > 8)
> > +		return prop->data + 4;
> >   	return prop->data;
> >   }
> > @@ -338,11 +393,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
> >   {
> >   	const struct fdt_property *prop;
> > -	prop = fdt_get_property_by_offset(fdt, offset, lenp);
> > +	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
> >   	if (!prop)
> >   		return NULL;
> >   	if (namep)
> >   		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> > +
> > +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> > +	    fdt32_to_cpu(prop->len) > 8)
> > +		return prop->data + 4;
> >   	return prop->data;
> >   }
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index baa882c..7ac3e8a 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -54,7 +54,7 @@
> >   #include <libfdt_env.h>
> >   #include <fdt.h>
> > -#define FDT_FIRST_SUPPORTED_VERSION	0x10
> > +#define FDT_FIRST_SUPPORTED_VERSION	0x02
> >   #define FDT_LAST_SUPPORTED_VERSION	0x11
> >   /* Error codes: informative error codes */
> > @@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
> >    * fdt_property structure within the device tree blob at the given
> >    * offset.  If lenp is non-NULL, the length of the property value is
> >    * also returned, in the integer pointed to by lenp.
> > + *
> > + * Note that this code only works on device tree versions >= 16. fdt_getprop()
> > + * works on all versions.
> >    *
> >    * returns:
> >    *	pointer to the structure representing the property
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

On 01/02/18 16:40, David Gibson wrote:
> On Fri, Dec 29, 2017 at 10:12:04AM -0800, Nathan Whitehorn wrote:
>> Has anyone had a chance to look at this? We need it, or something like it,
>> to support some older firmwares with the FreeBSD kernel and I would like to
>> make sure that our copy of libfdt does not diverge from upstream.
> Sorry, I've been busy and haven't had a chance to look at it.  I
> haven't forgotten..

Great! Just wanted to make sure it didn't get lost in the noise. Please 
let me know if there is anything I can do to help.
-Nathan

>> Thanks,
>> Nathan
>>
>> On 12/07/17 22:31, Nathan Whitehorn wrote:
>>> This can be useful in particular in the kernel when booting on systems
>>> with FDT-emitting firmware that is out of date. Releases of kexec-tools
>>> on ppc64 prior to the end of 2014 are notable examples of such.
>>>
>>> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
>>> ---
>>>    fdtget.c        |  4 +--
>>>    libfdt/fdt.c    |  8 ++++--
>>>    libfdt/fdt_ro.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>>    libfdt/libfdt.h |  5 +++-
>>>    4 files changed, 79 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/fdtget.c b/fdtget.c
>>> index 6cc5242..34d8194 100644
>>> --- a/fdtget.c
>>> +++ b/fdtget.c
>>> @@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
>>>     */
>>>    static int list_properties(const void *blob, int node)
>>>    {
>>> -	const struct fdt_property *data;
>>>    	const char *name;
>>>    	int prop;
>>> @@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
>>>    		/* Stop silently when there are no more properties */
>>>    		if (prop < 0)
>>>    			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
>>> -		data = fdt_get_property_by_offset(blob, prop, NULL);
>>> -		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
>>> +		fdt_getprop_by_offset(blob, prop, &name, NULL);
>>>    		if (name)
>>>    			puts(name);
>>>    		prop = fdt_next_property_offset(blob, prop);
>>> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
>>> index fd13236..e783e6a 100644
>>> --- a/libfdt/fdt.c
>>> +++ b/libfdt/fdt.c
>>> @@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>>>    		return NULL;
>>>    	if (fdt_version(fdt) >= 0x11)
>>> -		if (((offset + len) < offset)
>>> -		    || ((offset + len) > fdt_size_dt_struct(fdt)))
>>> +		if (((offset + len) < offset) ||
>>> +		    (fdt_version(fdt) >= 0x10 &&
>>> +		     (offset + len) > fdt_size_dt_struct(fdt)))
>>>    			return NULL;
>>>    	return fdt_offset_ptr_(fdt, offset);
>>> @@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>>>    		/* skip-name offset, length and value */
>>>    		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>>>    			+ fdt32_to_cpu(*lenp);
>>> +		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) > 8 &&
>>> +		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
>>> +			offset += 4;
>>>    		break;
>>>    	case FDT_END:
>>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>>> index ce17814..025dd0f 100644
>>> --- a/libfdt/fdt_ro.c
>>> +++ b/libfdt/fdt_ro.c
>>> @@ -233,16 +233,26 @@ int fdt_path_offset(const void *fdt, const char *path)
>>>    const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>>>    {
>>>    	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
>>> +	const char *nameptr;
>>>    	int err;
>>>    	if (((err = fdt_check_header(fdt)) != 0)
>>>    	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
>>>    			goto fail;
>>> +	nameptr = nh->name;
>>> +
>>> +	if (fdt_version(fdt) < 0x10) {
>>> +		const char *leaf;
>>> +		leaf = strrchr(nameptr, '/');
>>> +		if (leaf != NULL)
>>> +			nameptr = leaf+1;
>>> +	}
>>> +
>>>    	if (len)
>>> -		*len = strlen(nh->name);
>>> +		*len = strlen(nameptr);
>>> -	return nh->name;
>>> +	return nameptr;
>>>     fail:
>>>    	if (len)
>>> @@ -268,9 +278,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
>>>    	return nextprop_(fdt, offset);
>>>    }
>>> -const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>>> -						      int offset,
>>> -						      int *lenp)
>>> +static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
>>> +						              int offset,
>>> +						              int *lenp)
>>>    {
>>>    	int err;
>>>    	const struct fdt_property *prop;
>>> @@ -289,11 +299,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>>>    	return prop;
>>>    }
>>> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>>> +						      int offset,
>>> +						      int *lenp)
>>> +{
>>> +	/* Prior to version 16, properties may need realignment
>>> +	 * and this API does not work. fdt_getprop_*() will, however. */
>>> +
>>> +	if (fdt_version(fdt) < 0x10) {
>>> +		if (lenp)
>>> +			*lenp = -FDT_ERR_BADVERSION;
>>> +		return NULL;
>>> +	}
>>> +
>>> +	return _fdt_get_property_by_offset(fdt, offset, lenp);
>>> +}
>>> +
>>>    const struct fdt_property *fdt_get_property_namelen(const void *fdt,
>>>    						    int offset,
>>>    						    const char *name,
>>>    						    int namelen, int *lenp)
>>>    {
>>> +	/* Prior to version 16, properties may need realignment
>>> +	 * and this API does not work. fdt_getprop_*() will, however. */
>>> +	if (fdt_version(fdt) < 0x10) {
>>> +		if (lenp)
>>> +			*lenp = -FDT_ERR_BADVERSION;
>>> +		return NULL;
>>> +	}
>>> +
>>>    	for (offset = fdt_first_property_offset(fdt, offset);
>>>    	     (offset >= 0);
>>>    	     (offset = fdt_next_property_offset(fdt, offset))) {
>>> @@ -324,12 +358,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
>>>    const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>>>    				const char *name, int namelen, int *lenp)
>>>    {
>>> -	const struct fdt_property *prop;
>>> +	const struct fdt_property *prop = NULL;
>>> +	int offset = nodeoffset;
>>> -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
>>> -	if (!prop)
>>> +	for (offset = fdt_first_property_offset(fdt, offset);
>>> +	     (offset >= 0);
>>> +	     (offset = fdt_next_property_offset(fdt, offset))) {
>>> +		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
>>> +			if (lenp)
>>> +				*lenp = -FDT_ERR_INTERNAL;
>>> +			return NULL;
>>> +		}
>>> +
>>> +		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
>>> +				   name, namelen))
>>> +			break;
>>> +	}
>>> +
>>> +	if (lenp && offset < 0)
>>> +		*lenp = offset;
>>> +
>>> +	if (!prop || offset < 0)
>>>    		return NULL;
>>> +	/* Handle realignment */
>>> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
>>> +	    fdt32_to_cpu(prop->len) > 8)
>>> +		return prop->data + 4;
>>>    	return prop->data;
>>>    }
>>> @@ -338,11 +393,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>>>    {
>>>    	const struct fdt_property *prop;
>>> -	prop = fdt_get_property_by_offset(fdt, offset, lenp);
>>> +	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
>>>    	if (!prop)
>>>    		return NULL;
>>>    	if (namep)
>>>    		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>>> +
>>> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
>>> +	    fdt32_to_cpu(prop->len) > 8)
>>> +		return prop->data + 4;
>>>    	return prop->data;
>>>    }
>>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>>> index baa882c..7ac3e8a 100644
>>> --- a/libfdt/libfdt.h
>>> +++ b/libfdt/libfdt.h
>>> @@ -54,7 +54,7 @@
>>>    #include <libfdt_env.h>
>>>    #include <fdt.h>
>>> -#define FDT_FIRST_SUPPORTED_VERSION	0x10
>>> +#define FDT_FIRST_SUPPORTED_VERSION	0x02
>>>    #define FDT_LAST_SUPPORTED_VERSION	0x11
>>>    /* Error codes: informative error codes */
>>> @@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
>>>     * fdt_property structure within the device tree blob at the given
>>>     * offset.  If lenp is non-NULL, the length of the property value is
>>>     * also returned, in the integer pointed to by lenp.
>>> + *
>>> + * Note that this code only works on device tree versions >= 16. fdt_getprop()
>>> + * works on all versions.
>>>     *
>>>     * returns:
>>>     *	pointer to the structure representing the property

[PATCH v2] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[nwhitehorn-h+KGxgPPiopAfugRpC6u6w]

From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>

This can be useful in particular in the kernel when booting on systems
with FDT-emitting firmware that is out of date. Releases of kexec-tools
on ppc64 prior to the end of 2014 are notable examples of such.

Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
---

This fixes some minor bugs in the original version of the patch with name
matching and properties that were exactly 8 bytes long. Tested and running
(though uncommitted for now, to prevent divergence from upstream) in the
FreeBSD kernel.

 fdtget.c        |  4 +--
 libfdt/fdt.c    |  8 ++++--
 libfdt/fdt_ro.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 libfdt/libfdt.h |  5 +++-
 4 files changed, 87 insertions(+), 17 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index 6cc5242..34d8194 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
  */
 static int list_properties(const void *blob, int node)
 {
-	const struct fdt_property *data;
 	const char *name;
 	int prop;
 
@@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
 		/* Stop silently when there are no more properties */
 		if (prop < 0)
 			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
-		data = fdt_get_property_by_offset(blob, prop, NULL);
-		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
+		fdt_getprop_by_offset(blob, prop, &name, NULL);
 		if (name)
 			puts(name);
 		prop = fdt_next_property_offset(blob, prop);
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index fd13236..b1cc253 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 		return NULL;
 
 	if (fdt_version(fdt) >= 0x11)
-		if (((offset + len) < offset)
-		    || ((offset + len) > fdt_size_dt_struct(fdt)))
+		if (((offset + len) < offset) ||
+		    (fdt_version(fdt) >= 0x10 &&
+		     (offset + len) > fdt_size_dt_struct(fdt)))
 			return NULL;
 
 	return fdt_offset_ptr_(fdt, offset);
@@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		/* skip-name offset, length and value */
 		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
 			+ fdt32_to_cpu(*lenp);
+		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
+		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
+			offset += 4;
 		break;
 
 	case FDT_END:
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index ce17814..d011bd3 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -58,9 +58,10 @@
 static int fdt_nodename_eq_(const void *fdt, int offset,
 			    const char *s, int len)
 {
-	const char *p = fdt_offset_ptr(fdt, offset + FDT_TAGSIZE, len+1);
+	int olen;
+	const char *p = fdt_get_name(fdt, offset, &olen);
 
-	if (!p)
+	if (!p || olen < len)
 		/* short match */
 		return 0;
 
@@ -233,16 +234,31 @@ int fdt_path_offset(const void *fdt, const char *path)
 const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 {
 	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
+	const char *nameptr;
 	int err;
 
 	if (((err = fdt_check_header(fdt)) != 0)
 	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
 			goto fail;
 
+	nameptr = nh->name;
+
+	if (fdt_version(fdt) < 0x10 && nodeoffset != 0) {
+		/*
+		 * For old FDT versions, match the naming conventions of V16:
+		 * give only the leaf name (after all /) except for the 
+		 * root node, where we should still return / rather than ""
+		 */
+		const char *leaf;
+		leaf = strrchr(nameptr, '/');
+		if (leaf != NULL)
+			nameptr = leaf+1;
+	}
+
 	if (len)
-		*len = strlen(nh->name);
+		*len = strlen(nameptr);
 
-	return nh->name;
+	return nameptr;
 
  fail:
 	if (len)
@@ -268,9 +284,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
 	return nextprop_(fdt, offset);
 }
 
-const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
-						      int offset,
-						      int *lenp)
+static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
+						              int offset,
+						              int *lenp)
 {
 	int err;
 	const struct fdt_property *prop;
@@ -289,11 +305,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
 	return prop;
 }
 
+const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
+						      int offset,
+						      int *lenp)
+{
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
+	return _fdt_get_property_by_offset(fdt, offset, lenp);
+}
+
 const struct fdt_property *fdt_get_property_namelen(const void *fdt,
 						    int offset,
 						    const char *name,
 						    int namelen, int *lenp)
 {
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
 	for (offset = fdt_first_property_offset(fdt, offset);
 	     (offset >= 0);
 	     (offset = fdt_next_property_offset(fdt, offset))) {
@@ -324,12 +364,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 				const char *name, int namelen, int *lenp)
 {
-	const struct fdt_property *prop;
+	const struct fdt_property *prop = NULL;
+	int offset = nodeoffset;
 
-	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
-	if (!prop)
+	for (offset = fdt_first_property_offset(fdt, offset);
+	     (offset >= 0);
+	     (offset = fdt_next_property_offset(fdt, offset))) {
+		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
+			if (lenp)
+				*lenp = -FDT_ERR_INTERNAL;
+			return NULL;
+		}
+
+		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
+				   name, namelen))
+			break;
+	}
+
+	if (lenp && offset < 0)
+		*lenp = offset;
+
+	if (!prop || offset < 0)
 		return NULL;
 
+	/* Handle realignment */
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) >= 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
@@ -338,11 +399,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 {
 	const struct fdt_property *prop;
 
-	prop = fdt_get_property_by_offset(fdt, offset, lenp);
+	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
 	if (!prop)
 		return NULL;
 	if (namep)
 		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) >= 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index baa882c..7ac3e8a 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -54,7 +54,7 @@
 #include <libfdt_env.h>
 #include <fdt.h>
 
-#define FDT_FIRST_SUPPORTED_VERSION	0x10
+#define FDT_FIRST_SUPPORTED_VERSION	0x02
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
 /* Error codes: informative error codes */
@@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
  * fdt_property structure within the device tree blob at the given
  * offset.  If lenp is non-NULL, the length of the property value is
  * also returned, in the integer pointed to by lenp.
+ * 
+ * Note that this code only works on device tree versions >= 16. fdt_getprop()
+ * works on all versions.
  *
  * returns:
  *	pointer to the structure representing the property
-- 
2.14.1

Re: [PATCH v2] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9648 bytes --]

On Sat, Dec 30, 2017 at 06:28:58PM -0800, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
> From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> 
> This can be useful in particular in the kernel when booting on systems
> with FDT-emitting firmware that is out of date.

Hrm, really, *really* out of date.  V2 and V3 are positively ancient.

> Releases of kexec-tools
> on ppc64 prior to the end of 2014 are notable examples of such.

Good grief, that's ridiculous.  They were also using dts-v0 years and
years after it should have been gotten rid of.


Anyway that said, the changes below don't look too bad.  There's a few
nits, but in principle I'd be ok to apply

> 
> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> ---
> 
> This fixes some minor bugs in the original version of the patch with name
> matching and properties that were exactly 8 bytes long. Tested and running
> (though uncommitted for now, to prevent divergence from upstream) in the
> FreeBSD kernel.
> 
>  fdtget.c        |  4 +--
>  libfdt/fdt.c    |  8 ++++--
>  libfdt/fdt_ro.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  libfdt/libfdt.h |  5 +++-
>  4 files changed, 87 insertions(+), 17 deletions(-)
> 
> diff --git a/fdtget.c b/fdtget.c
> index 6cc5242..34d8194 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
>   */
>  static int list_properties(const void *blob, int node)
>  {
> -	const struct fdt_property *data;
>  	const char *name;
>  	int prop;
>  
> @@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
>  		/* Stop silently when there are no more properties */
>  		if (prop < 0)
>  			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
> -		data = fdt_get_property_by_offset(blob, prop, NULL);
> -		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
> +		fdt_getprop_by_offset(blob, prop, &name, NULL);
>  		if (name)
>  			puts(name);
>  		prop = fdt_next_property_offset(blob, prop);
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index fd13236..b1cc253 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  		return NULL;
>  
>  	if (fdt_version(fdt) >= 0x11)
> -		if (((offset + len) < offset)
> -		    || ((offset + len) > fdt_size_dt_struct(fdt)))
> +		if (((offset + len) < offset) ||
> +		    (fdt_version(fdt) >= 0x10 &&

This doesn't make sense - you're already guarded by an fdt_version > 0x11
above.

> +		     (offset + len) > fdt_size_dt_struct(fdt)))
>  			return NULL;
>  
>  	return fdt_offset_ptr_(fdt, offset);
> @@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		/* skip-name offset, length and value */
>  		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>  			+ fdt32_to_cpu(*lenp);
> +		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> +		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> +			offset += 4;
>  		break;
>  
>  	case FDT_END:
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index ce17814..d011bd3 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -58,9 +58,10 @@
>  static int fdt_nodename_eq_(const void *fdt, int offset,
>  			    const char *s, int len)
>  {
> -	const char *p = fdt_offset_ptr(fdt, offset + FDT_TAGSIZE, len+1);
> +	int olen;
> +	const char *p = fdt_get_name(fdt, offset, &olen);
>  
> -	if (!p)
> +	if (!p || olen < len)
>  		/* short match */
>  		return 0;
>  
> @@ -233,16 +234,31 @@ int fdt_path_offset(const void *fdt, const char *path)
>  const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  {
>  	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
> +	const char *nameptr;
>  	int err;
>  
>  	if (((err = fdt_check_header(fdt)) != 0)
>  	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
>  			goto fail;
>  
> +	nameptr = nh->name;
> +
> +	if (fdt_version(fdt) < 0x10 && nodeoffset != 0) {
> +		/*
> +		 * For old FDT versions, match the naming conventions of V16:
> +		 * give only the leaf name (after all /) except for the 
> +		 * root node, where we should still return / rather than ""

What's the rationale for returning "/" rather than "" on the root
node?  a V16 file will return "", typically.

> +		 */
> +		const char *leaf;
> +		leaf = strrchr(nameptr, '/');
> +		if (leaf != NULL)
> +			nameptr = leaf+1;

If leaf is NULL (no '/') I think that indicates a badly formed V3 or
less tree, the full path should already have at least one /.

> +	}
> +
>  	if (len)
> -		*len = strlen(nh->name);
> +		*len = strlen(nameptr);
>  
> -	return nh->name;
> +	return nameptr;
>  
>   fail:
>  	if (len)
> @@ -268,9 +284,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
>  	return nextprop_(fdt, offset);
>  }
>  
> -const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> -						      int offset,
> -						      int *lenp)
> +static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
> +						              int offset,
> +						              int *lenp)

I've been trying to get rid of symbols starting with _ since they're
technically reserved for the system and can cause problems in some
compilation environments.  Go to alternative is ending with a _ instead.

>  {
>  	int err;
>  	const struct fdt_property *prop;
> @@ -289,11 +305,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>  	return prop;
>  }
>  
> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> +						      int offset,
> +						      int *lenp)
> +{
> +	/* Prior to version 16, properties may need realignment
> +	 * and this API does not work. fdt_getprop_*() will, however. */
> +
> +	if (fdt_version(fdt) < 0x10) {
> +		if (lenp)
> +			*lenp = -FDT_ERR_BADVERSION;
> +		return NULL;
> +	}
> +
> +	return _fdt_get_property_by_offset(fdt, offset, lenp);
> +}
> +
>  const struct fdt_property *fdt_get_property_namelen(const void *fdt,
>  						    int offset,
>  						    const char *name,
>  						    int namelen, int *lenp)
>  {
> +	/* Prior to version 16, properties may need realignment
> +	 * and this API does not work. fdt_getprop_*() will, however. */
> +	if (fdt_version(fdt) < 0x10) {
> +		if (lenp)
> +			*lenp = -FDT_ERR_BADVERSION;
> +		return NULL;
> +	}
> +
>  	for (offset = fdt_first_property_offset(fdt, offset);
>  	     (offset >= 0);
>  	     (offset = fdt_next_property_offset(fdt, offset))) {
> @@ -324,12 +364,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
>  const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>  				const char *name, int namelen, int *lenp)
>  {
> -	const struct fdt_property *prop;
> +	const struct fdt_property *prop = NULL;
> +	int offset = nodeoffset;
>  
> -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);

Couldn't you use an fdt_get_property_namelen_() here instead of having
to duplicate most of its logic.

> -	if (!prop)
> +	for (offset = fdt_first_property_offset(fdt, offset);
> +	     (offset >= 0);
> +	     (offset = fdt_next_property_offset(fdt, offset))) {



> +		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
> +			if (lenp)
> +				*lenp = -FDT_ERR_INTERNAL;
> +			return NULL;
> +		}
> +
> +		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
> +				   name, namelen))
> +			break;
> +	}
> +
> +	if (lenp && offset < 0)
> +		*lenp = offset;
> +
> +	if (!prop || offset < 0)
>  		return NULL;
>  
> +	/* Handle realignment */
> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> +	    fdt32_to_cpu(prop->len) >= 8)
> +		return prop->data + 4;
>  	return prop->data;
>  }
>  
> @@ -338,11 +399,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  {
>  	const struct fdt_property *prop;
>  
> -	prop = fdt_get_property_by_offset(fdt, offset, lenp);
> +	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
>  	if (!prop)
>  		return NULL;
>  	if (namep)
>  		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> +
> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> +	    fdt32_to_cpu(prop->len) >= 8)
> +		return prop->data + 4;
>  	return prop->data;
>  }
>  
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index baa882c..7ac3e8a 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -54,7 +54,7 @@
>  #include <libfdt_env.h>
>  #include <fdt.h>
>  
> -#define FDT_FIRST_SUPPORTED_VERSION	0x10
> +#define FDT_FIRST_SUPPORTED_VERSION	0x02
>  #define FDT_LAST_SUPPORTED_VERSION	0x11
>  
>  /* Error codes: informative error codes */
> @@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
>   * fdt_property structure within the device tree blob at the given
>   * offset.  If lenp is non-NULL, the length of the property value is
>   * also returned, in the integer pointed to by lenp.
> + * 
> + * Note that this code only works on device tree versions >= 16. fdt_getprop()
> + * works on all versions.
>   *
>   * returns:
>   *	pointer to the structure representing the property

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

On 01/17/18 20:59, David Gibson wrote:
> On Sat, Dec 30, 2017 at 06:28:58PM -0800, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
>> From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
>>
>> This can be useful in particular in the kernel when booting on systems
>> with FDT-emitting firmware that is out of date.
> Hrm, really, *really* out of date.  V2 and V3 are positively ancient.

Yes, I was *really* disappointed to run into these things in the wild.

>
>> Releases of kexec-tools
>> on ppc64 prior to the end of 2014 are notable examples of such.
> Good grief, that's ridiculous.  They were also using dts-v0 years and
> years after it should have been gotten rid of.

Yes. And Red Hat at least is still shipping such versions of kexec-tools 
today...

 From a fully up-to-date supported RHEL6 system:
nwhitehorn@gordita:~$ kexec -v
kexec-tools 2.0.0 released 19th July 2008
>
> Anyway that said, the changes below don't look too bad.  There's a few
> nits, but in principle I'd be ok to apply

OK, great. I'll plan to submit a revised diff later today or tomorrow.

>> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
>> ---
>>
>> This fixes some minor bugs in the original version of the patch with name
>> matching and properties that were exactly 8 bytes long. Tested and running
>> (though uncommitted for now, to prevent divergence from upstream) in the
>> FreeBSD kernel.
>>
>>   fdtget.c        |  4 +--
>>   libfdt/fdt.c    |  8 ++++--
>>   libfdt/fdt_ro.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   libfdt/libfdt.h |  5 +++-
>>   4 files changed, 87 insertions(+), 17 deletions(-)
>>
>> diff --git a/fdtget.c b/fdtget.c
>> index 6cc5242..34d8194 100644
>> --- a/fdtget.c
>> +++ b/fdtget.c
>> @@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
>>    */
>>   static int list_properties(const void *blob, int node)
>>   {
>> -	const struct fdt_property *data;
>>   	const char *name;
>>   	int prop;
>>   
>> @@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
>>   		/* Stop silently when there are no more properties */
>>   		if (prop < 0)
>>   			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
>> -		data = fdt_get_property_by_offset(blob, prop, NULL);
>> -		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
>> +		fdt_getprop_by_offset(blob, prop, &name, NULL);
>>   		if (name)
>>   			puts(name);
>>   		prop = fdt_next_property_offset(blob, prop);
>> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
>> index fd13236..b1cc253 100644
>> --- a/libfdt/fdt.c
>> +++ b/libfdt/fdt.c
>> @@ -84,8 +84,9 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>>   		return NULL;
>>   
>>   	if (fdt_version(fdt) >= 0x11)
>> -		if (((offset + len) < offset)
>> -		    || ((offset + len) > fdt_size_dt_struct(fdt)))
>> +		if (((offset + len) < offset) ||
>> +		    (fdt_version(fdt) >= 0x10 &&
> This doesn't make sense - you're already guarded by an fdt_version > 0x11
> above.

That is true. I have no idea why I wrote that and will remove it.

>
>> +		     (offset + len) > fdt_size_dt_struct(fdt)))
>>   			return NULL;
>>   
>>   	return fdt_offset_ptr_(fdt, offset);
>> @@ -123,6 +124,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>>   		/* skip-name offset, length and value */
>>   		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>>   			+ fdt32_to_cpu(*lenp);
>> +		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
>> +		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
>> +			offset += 4;
>>   		break;
>>   
>>   	case FDT_END:
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index ce17814..d011bd3 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -58,9 +58,10 @@
>>   static int fdt_nodename_eq_(const void *fdt, int offset,
>>   			    const char *s, int len)
>>   {
>> -	const char *p = fdt_offset_ptr(fdt, offset + FDT_TAGSIZE, len+1);
>> +	int olen;
>> +	const char *p = fdt_get_name(fdt, offset, &olen);
>>   
>> -	if (!p)
>> +	if (!p || olen < len)
>>   		/* short match */
>>   		return 0;
>>   
>> @@ -233,16 +234,31 @@ int fdt_path_offset(const void *fdt, const char *path)
>>   const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>>   {
>>   	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
>> +	const char *nameptr;
>>   	int err;
>>   
>>   	if (((err = fdt_check_header(fdt)) != 0)
>>   	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
>>   			goto fail;
>>   
>> +	nameptr = nh->name;
>> +
>> +	if (fdt_version(fdt) < 0x10 && nodeoffset != 0) {
>> +		/*
>> +		 * For old FDT versions, match the naming conventions of V16:
>> +		 * give only the leaf name (after all /) except for the
>> +		 * root node, where we should still return / rather than ""
> What's the rationale for returning "/" rather than "" on the root
> node?  a V16 file will return "", typically.

Are you sure? The immediate motivation was that parts of the FreeBSD 
kernel were breaking if it was "". Since those parts work fine with V16 
trees, I did this. I will double-check what the exact problem is -- it 
might be on our side somehow.

>> +		 */
>> +		const char *leaf;
>> +		leaf = strrchr(nameptr, '/');
>> +		if (leaf != NULL)
>> +			nameptr = leaf+1;
> If leaf is NULL (no '/') I think that indicates a badly formed V3 or
> less tree, the full path should already have at least one /.

Good point. I will make this an error condition.

>
>> +	}
>> +
>>   	if (len)
>> -		*len = strlen(nh->name);
>> +		*len = strlen(nameptr);
>>   
>> -	return nh->name;
>> +	return nameptr;
>>   
>>    fail:
>>   	if (len)
>> @@ -268,9 +284,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
>>   	return nextprop_(fdt, offset);
>>   }
>>   
>> -const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>> -						      int offset,
>> -						      int *lenp)
>> +static const struct fdt_property *_fdt_get_property_by_offset(const void *fdt,
>> +						              int offset,
>> +						              int *lenp)
> I've been trying to get rid of symbols starting with _ since they're
> technically reserved for the system and can cause problems in some
> compilation environments.  Go to alternative is ending with a _ instead.

This patch was originally written against an older libfdt with preceding 
_ -- I seem to have forgotten to move this. Will fix.

>>   {
>>   	int err;
>>   	const struct fdt_property *prop;
>> @@ -289,11 +305,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>>   	return prop;
>>   }
>>   
>> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>> +						      int offset,
>> +						      int *lenp)
>> +{
>> +	/* Prior to version 16, properties may need realignment
>> +	 * and this API does not work. fdt_getprop_*() will, however. */
>> +
>> +	if (fdt_version(fdt) < 0x10) {
>> +		if (lenp)
>> +			*lenp = -FDT_ERR_BADVERSION;
>> +		return NULL;
>> +	}
>> +
>> +	return _fdt_get_property_by_offset(fdt, offset, lenp);
>> +}
>> +
>>   const struct fdt_property *fdt_get_property_namelen(const void *fdt,
>>   						    int offset,
>>   						    const char *name,
>>   						    int namelen, int *lenp)
>>   {
>> +	/* Prior to version 16, properties may need realignment
>> +	 * and this API does not work. fdt_getprop_*() will, however. */
>> +	if (fdt_version(fdt) < 0x10) {
>> +		if (lenp)
>> +			*lenp = -FDT_ERR_BADVERSION;
>> +		return NULL;
>> +	}
>> +
>>   	for (offset = fdt_first_property_offset(fdt, offset);
>>   	     (offset >= 0);
>>   	     (offset = fdt_next_property_offset(fdt, offset))) {
>> @@ -324,12 +364,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
>>   const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>>   				const char *name, int namelen, int *lenp)
>>   {
>> -	const struct fdt_property *prop;
>> +	const struct fdt_property *prop = NULL;
>> +	int offset = nodeoffset;
>>   
>> -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
> Couldn't you use an fdt_get_property_namelen_() here instead of having
> to duplicate most of its logic.

You could. Since it is pretty short, adding another function did not 
seem to be much shorter. Happy to do this if you prefer.
-Nathan

>> -	if (!prop)
>> +	for (offset = fdt_first_property_offset(fdt, offset);
>> +	     (offset >= 0);
>> +	     (offset = fdt_next_property_offset(fdt, offset))) {
>
>
>> +		if (!(prop = _fdt_get_property_by_offset(fdt, offset, lenp))) {
>> +			if (lenp)
>> +				*lenp = -FDT_ERR_INTERNAL;
>> +			return NULL;
>> +		}
>> +
>> +		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
>> +				   name, namelen))
>> +			break;
>> +	}
>> +
>> +	if (lenp && offset < 0)
>> +		*lenp = offset;
>> +
>> +	if (!prop || offset < 0)
>>   		return NULL;
>>   
>> +	/* Handle realignment */
>> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
>> +	    fdt32_to_cpu(prop->len) >= 8)
>> +		return prop->data + 4;
>>   	return prop->data;
>>   }
>>   
>> @@ -338,11 +399,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>>   {
>>   	const struct fdt_property *prop;
>>   
>> -	prop = fdt_get_property_by_offset(fdt, offset, lenp);
>> +	prop = _fdt_get_property_by_offset(fdt, offset, lenp);
>>   	if (!prop)
>>   		return NULL;
>>   	if (namep)
>>   		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>> +
>> +	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
>> +	    fdt32_to_cpu(prop->len) >= 8)
>> +		return prop->data + 4;
>>   	return prop->data;
>>   }
>>   
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index baa882c..7ac3e8a 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -54,7 +54,7 @@
>>   #include <libfdt_env.h>
>>   #include <fdt.h>
>>   
>> -#define FDT_FIRST_SUPPORTED_VERSION	0x10
>> +#define FDT_FIRST_SUPPORTED_VERSION	0x02
>>   #define FDT_LAST_SUPPORTED_VERSION	0x11
>>   
>>   /* Error codes: informative error codes */
>> @@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
>>    * fdt_property structure within the device tree blob at the given
>>    * offset.  If lenp is non-NULL, the length of the property value is
>>    * also returned, in the integer pointed to by lenp.
>> + *
>> + * Note that this code only works on device tree versions >= 16. fdt_getprop()
>> + * works on all versions.
>>    *
>>    * returns:
>>    *	pointer to the structure representing the property

Re: [PATCH v2] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]

On Thu, Jan 18, 2018 at 12:41:28PM -0800, Nathan Whitehorn wrote:
> 
> 
> On 01/17/18 20:59, David Gibson wrote:
> > On Sat, Dec 30, 2017 at 06:28:58PM -0800, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
> > > From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> > > 
> > > This can be useful in particular in the kernel when booting on systems
> > > with FDT-emitting firmware that is out of date.
> > Hrm, really, *really* out of date.  V2 and V3 are positively ancient.
> 
> Yes, I was *really* disappointed to run into these things in the wild.
> 
> > 
> > > Releases of kexec-tools
> > > on ppc64 prior to the end of 2014 are notable examples of such.
> > Good grief, that's ridiculous.  They were also using dts-v0 years and
> > years after it should have been gotten rid of.
> 
> Yes. And Red Hat at least is still shipping such versions of kexec-tools
> today...
> 
> From a fully up-to-date supported RHEL6 system:
> nwhitehorn@gordita:~$ kexec -v
> kexec-tools 2.0.0 released 19th July 2008

*sigh*

> > Anyway that said, the changes below don't look too bad.  There's a few
> > nits, but in principle I'd be ok to apply
> 
> OK, great. I'll plan to submit a revised diff later today or
> tomorrow.

Ok, thanks.

[snip]
> > > +	if (fdt_version(fdt) < 0x10 && nodeoffset != 0) {
> > > +		/*
> > > +		 * For old FDT versions, match the naming conventions of V16:
> > > +		 * give only the leaf name (after all /) except for the
> > > +		 * root node, where we should still return / rather than ""
> > What's the rationale for returning "/" rather than "" on the root
> > node?  a V16 file will return "", typically.
> 
> Are you sure?

Pretty sure, yeah..

> The immediate motivation was that parts of the FreeBSD kernel
> were breaking if it was "". Since those parts work fine with V16 trees, I
> did this. I will double-check what the exact problem is -- it might be on
> our side somehow.

[snip]
> > > @@ -324,12 +364,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
> > >   const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> > >   				const char *name, int namelen, int *lenp)
> > >   {
> > > -	const struct fdt_property *prop;
> > > +	const struct fdt_property *prop = NULL;
> > > +	int offset = nodeoffset;
> > > -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
> > Couldn't you use an fdt_get_property_namelen_() here instead of having
> > to duplicate most of its logic.
> 
> You could. Since it is pretty short, adding another function did not seem to
> be much shorter. Happy to do this if you prefer.

It's not so much the physical shortness, but the fact that getting the
property scan logic exactly right does have some edge cases you have
to be careful of, so I'd rather only have to do it in one place.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

On 01/18/18 15:25, David Gibson wrote:
> On Thu, Jan 18, 2018 at 12:41:28PM -0800, Nathan Whitehorn wrote:
[snip]
>>> Anyway that said, the changes below don't look too bad.  There's a few
>>> nits, but in principle I'd be ok to apply
>> OK, great. I'll plan to submit a revised diff later today or
>> tomorrow.
> Ok, thanks.

Sent separately. I've implemented all your comments besides the last one 
related to fdt_get_property_namelen_(), in large part because I did not 
see this email until after sending the patch. Happy to retool that if 
you like.

>
> [snip]
>>>> +	if (fdt_version(fdt) < 0x10 && nodeoffset != 0) {
>>>> +		/*
>>>> +		 * For old FDT versions, match the naming conventions of V16:
>>>> +		 * give only the leaf name (after all /) except for the
>>>> +		 * root node, where we should still return / rather than ""
>>> What's the rationale for returning "/" rather than "" on the root
>>> node?  a V16 file will return "", typically.
>> Are you sure?
> Pretty sure, yeah..

I was apparently misremembering or misdiagnosed something else. The new 
patch deletes this code (and makes path names without a / an error).
-Nathan

>
>> The immediate motivation was that parts of the FreeBSD kernel
>> were breaking if it was "". Since those parts work fine with V16 trees, I
>> did this. I will double-check what the exact problem is -- it might be on
>> our side somehow.
> [snip]
>>>> @@ -324,12 +364,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
>>>>    const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>>>>    				const char *name, int namelen, int *lenp)
>>>>    {
>>>> -	const struct fdt_property *prop;
>>>> +	const struct fdt_property *prop = NULL;
>>>> +	int offset = nodeoffset;
>>>> -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
>>> Couldn't you use an fdt_get_property_namelen_() here instead of having
>>> to duplicate most of its logic.
>> You could. Since it is pretty short, adding another function did not seem to
>> be much shorter. Happy to do this if you prefer.
> It's not so much the physical shortness, but the fact that getting the
> property scan logic exactly right does have some edge cases you have
> to be careful of, so I'd rather only have to do it in one place.
>

[PATCH v3] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[nwhitehorn-h+KGxgPPiopAfugRpC6u6w]

From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>

This can be useful in particular in the kernel when booting on systems
with FDT-emitting firmware that is out of date. Releases of kexec-tools
on ppc64 prior to the end of 2014 are notable examples of such.

Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
---

This version of the patch fixes a number of style issues and removes some
unnecessary redundant code noted in review.

 fdtget.c        |  4 +--
 libfdt/fdt.c    |  3 ++
 libfdt/fdt_ro.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 libfdt/libfdt.h |  5 +++-
 4 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index 6cc5242..34d8194 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
  */
 static int list_properties(const void *blob, int node)
 {
-	const struct fdt_property *data;
 	const char *name;
 	int prop;
 
@@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
 		/* Stop silently when there are no more properties */
 		if (prop < 0)
 			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
-		data = fdt_get_property_by_offset(blob, prop, NULL);
-		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
+		fdt_getprop_by_offset(blob, prop, &name, NULL);
 		if (name)
 			puts(name);
 		prop = fdt_next_property_offset(blob, prop);
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index fd13236..7855a17 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -123,6 +123,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		/* skip-name offset, length and value */
 		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
 			+ fdt32_to_cpu(*lenp);
+		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
+		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
+			offset += 4;
 		break;
 
 	case FDT_END:
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index ce17814..c31275f 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -58,9 +58,10 @@
 static int fdt_nodename_eq_(const void *fdt, int offset,
 			    const char *s, int len)
 {
-	const char *p = fdt_offset_ptr(fdt, offset + FDT_TAGSIZE, len+1);
+	int olen;
+	const char *p = fdt_get_name(fdt, offset, &olen);
 
-	if (!p)
+	if (!p || olen < len)
 		/* short match */
 		return 0;
 
@@ -233,16 +234,34 @@ int fdt_path_offset(const void *fdt, const char *path)
 const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 {
 	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
+	const char *nameptr;
 	int err;
 
 	if (((err = fdt_check_header(fdt)) != 0)
 	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
 			goto fail;
 
+	nameptr = nh->name;
+
+	if (fdt_version(fdt) < 0x10) {
+		/*
+		 * For old FDT versions, match the naming conventions of V16:
+		 * give only the leaf name (after all /). The actual tree
+		 * contents are loosely checked.
+		 */
+		const char *leaf;
+		leaf = strrchr(nameptr, '/');
+		if (leaf == NULL) {
+			err = FDT_ERR_BADVALUE;
+			goto fail;
+		}
+		nameptr = leaf+1;
+	}
+
 	if (len)
-		*len = strlen(nh->name);
+		*len = strlen(nameptr);
 
-	return nh->name;
+	return nameptr;
 
  fail:
 	if (len)
@@ -268,9 +287,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
 	return nextprop_(fdt, offset);
 }
 
-const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
-						      int offset,
-						      int *lenp)
+static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
+						              int offset,
+						              int *lenp)
 {
 	int err;
 	const struct fdt_property *prop;
@@ -289,11 +308,35 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
 	return prop;
 }
 
+const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
+						      int offset,
+						      int *lenp)
+{
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
+	return fdt_get_property_by_offset_(fdt, offset, lenp);
+}
+
 const struct fdt_property *fdt_get_property_namelen(const void *fdt,
 						    int offset,
 						    const char *name,
 						    int namelen, int *lenp)
 {
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
 	for (offset = fdt_first_property_offset(fdt, offset);
 	     (offset >= 0);
 	     (offset = fdt_next_property_offset(fdt, offset))) {
@@ -324,12 +367,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 				const char *name, int namelen, int *lenp)
 {
-	const struct fdt_property *prop;
+	const struct fdt_property *prop = NULL;
+	int offset = nodeoffset;
 
-	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
-	if (!prop)
+	for (offset = fdt_first_property_offset(fdt, offset);
+	     (offset >= 0);
+	     (offset = fdt_next_property_offset(fdt, offset))) {
+		if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
+			if (lenp)
+				*lenp = -FDT_ERR_INTERNAL;
+			return NULL;
+		}
+
+		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
+				   name, namelen))
+			break;
+	}
+
+	if (lenp && offset < 0)
+		*lenp = offset;
+
+	if (!prop || offset < 0)
 		return NULL;
 
+	/* Handle realignment */
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) >= 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
@@ -338,11 +402,15 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 {
 	const struct fdt_property *prop;
 
-	prop = fdt_get_property_by_offset(fdt, offset, lenp);
+	prop = fdt_get_property_by_offset_(fdt, offset, lenp);
 	if (!prop)
 		return NULL;
 	if (namep)
 		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) >= 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index baa882c..7ac3e8a 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -54,7 +54,7 @@
 #include <libfdt_env.h>
 #include <fdt.h>
 
-#define FDT_FIRST_SUPPORTED_VERSION	0x10
+#define FDT_FIRST_SUPPORTED_VERSION	0x02
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
 /* Error codes: informative error codes */
@@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
  * fdt_property structure within the device tree blob at the given
  * offset.  If lenp is non-NULL, the length of the property value is
  * also returned, in the integer pointed to by lenp.
+ * 
+ * Note that this code only works on device tree versions >= 16. fdt_getprop()
+ * works on all versions.
  *
  * returns:
  *	pointer to the structure representing the property
-- 
2.15.1

Re: [PATCH v3] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On Fri, Jan 19, 2018 at 09:48:03PM +0000, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
> From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> 
> This can be useful in particular in the kernel when booting on systems
> with FDT-emitting firmware that is out of date. Releases of kexec-tools
> on ppc64 prior to the end of 2014 are notable examples of such.
> 
> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> ---
> 
> This version of the patch fixes a number of style issues and removes some
> unnecessary redundant code noted in review.

As suggested, please implement the change to use
fdt_get_property_namelen_() in fdt_getprop_namelen().  Plus I spotted
one other bug..

[snip]
> +	if (fdt_version(fdt) < 0x10) {
> +		/*
> +		 * For old FDT versions, match the naming conventions of V16:
> +		 * give only the leaf name (after all /). The actual tree
> +		 * contents are loosely checked.
> +		 */
> +		const char *leaf;
> +		leaf = strrchr(nameptr, '/');
> +		if (leaf == NULL) {
> +			err = FDT_ERR_BADVALUE;

The error codes are defined positive, but used negative, so this needs
to be err = -FDT_ERR_BADVAULE,  Except, actually, I think BADSTRUCTURE
would be a better option.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

On 01/21/18 01:40, David Gibson wrote:
> On Fri, Jan 19, 2018 at 09:48:03PM +0000, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
>> From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
>>
>> This can be useful in particular in the kernel when booting on systems
>> with FDT-emitting firmware that is out of date. Releases of kexec-tools
>> on ppc64 prior to the end of 2014 are notable examples of such.
>>
>> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
>> ---
>>
>> This version of the patch fixes a number of style issues and removes some
>> unnecessary redundant code noted in review.
> As suggested, please implement the change to use
> fdt_get_property_namelen_() in fdt_getprop_namelen().  Plus I spotted
> one other bug..

The reason for the duplication was that fdt_getprop_namelen() needs to 
know the offset of the property as well as its contents to decide 
whether it needs to realign the property data to 8-byte boundaries. 
That's pretty easily fixed by adding a final argument to 
fdt_get_property_namelen_() to return the offset. Is there a better way 
to get the offset from an fdt_property than that?

> [snip]
>> +	if (fdt_version(fdt) < 0x10) {
>> +		/*
>> +		 * For old FDT versions, match the naming conventions of V16:
>> +		 * give only the leaf name (after all /). The actual tree
>> +		 * contents are loosely checked.
>> +		 */
>> +		const char *leaf;
>> +		leaf = strrchr(nameptr, '/');
>> +		if (leaf == NULL) {
>> +			err = FDT_ERR_BADVALUE;
> The error codes are defined positive, but used negative, so this needs
> to be err = -FDT_ERR_BADVAULE,  Except, actually, I think BADSTRUCTURE
> would be a better option.
>
Whoops! Thanks for catching. I have a revised patch that I will send 
tonight after testing.
-Nathan

[PATCH v4] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[nwhitehorn-h+KGxgPPiopAfugRpC6u6w]

From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>

This can be useful in particular in the kernel when booting on systems
with FDT-emitting firmware that is out of date. Releases of kexec-tools
on ppc64 prior to the end of 2014 are notable examples of such.

Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
---
 fdtget.c        |   4 +--
 libfdt/fdt.c    |   3 ++
 libfdt/fdt_ro.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++---------
 libfdt/libfdt.h |   5 ++-
 4 files changed, 93 insertions(+), 19 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index 6cc5242..34d8194 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -140,7 +140,6 @@ static int show_data(struct display_info *disp, const char *data, int len)
  */
 static int list_properties(const void *blob, int node)
 {
-	const struct fdt_property *data;
 	const char *name;
 	int prop;
 
@@ -149,8 +148,7 @@ static int list_properties(const void *blob, int node)
 		/* Stop silently when there are no more properties */
 		if (prop < 0)
 			return prop == -FDT_ERR_NOTFOUND ? 0 : prop;
-		data = fdt_get_property_by_offset(blob, prop, NULL);
-		name = fdt_string(blob, fdt32_to_cpu(data->nameoff));
+		fdt_getprop_by_offset(blob, prop, &name, NULL);
 		if (name)
 			puts(name);
 		prop = fdt_next_property_offset(blob, prop);
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index fd13236..7855a17 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -123,6 +123,9 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		/* skip-name offset, length and value */
 		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
 			+ fdt32_to_cpu(*lenp);
+		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
+		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
+			offset += 4;
 		break;
 
 	case FDT_END:
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index ce17814..a79b8ca 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -58,9 +58,10 @@
 static int fdt_nodename_eq_(const void *fdt, int offset,
 			    const char *s, int len)
 {
-	const char *p = fdt_offset_ptr(fdt, offset + FDT_TAGSIZE, len+1);
+	int olen;
+	const char *p = fdt_get_name(fdt, offset, &olen);
 
-	if (!p)
+	if (!p || olen < len)
 		/* short match */
 		return 0;
 
@@ -233,16 +234,34 @@ int fdt_path_offset(const void *fdt, const char *path)
 const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 {
 	const struct fdt_node_header *nh = fdt_offset_ptr_(fdt, nodeoffset);
+	const char *nameptr;
 	int err;
 
 	if (((err = fdt_check_header(fdt)) != 0)
 	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
 			goto fail;
 
+	nameptr = nh->name;
+
+	if (fdt_version(fdt) < 0x10) {
+		/*
+		 * For old FDT versions, match the naming conventions of V16:
+		 * give only the leaf name (after all /). The actual tree
+		 * contents are loosely checked.
+		 */
+		const char *leaf;
+		leaf = strrchr(nameptr, '/');
+		if (leaf == NULL) {
+			err = -FDT_ERR_BADSTRUCTURE;
+			goto fail;
+		}
+		nameptr = leaf+1;
+	}
+
 	if (len)
-		*len = strlen(nh->name);
+		*len = strlen(nameptr);
 
-	return nh->name;
+	return nameptr;
 
  fail:
 	if (len)
@@ -268,9 +287,9 @@ int fdt_next_property_offset(const void *fdt, int offset)
 	return nextprop_(fdt, offset);
 }
 
-const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
-						      int offset,
-						      int *lenp)
+static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
+						              int offset,
+						              int *lenp)
 {
 	int err;
 	const struct fdt_property *prop;
@@ -289,23 +308,44 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
 	return prop;
 }
 
-const struct fdt_property *fdt_get_property_namelen(const void *fdt,
-						    int offset,
-						    const char *name,
-						    int namelen, int *lenp)
+const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
+						      int offset,
+						      int *lenp)
+{
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
+	return fdt_get_property_by_offset_(fdt, offset, lenp);
+}
+
+static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
+						            int offset,
+						            const char *name,
+						            int namelen,
+							    int *lenp,
+							    int *poffset)
 {
 	for (offset = fdt_first_property_offset(fdt, offset);
 	     (offset >= 0);
 	     (offset = fdt_next_property_offset(fdt, offset))) {
 		const struct fdt_property *prop;
 
-		if (!(prop = fdt_get_property_by_offset(fdt, offset, lenp))) {
+		if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
 			offset = -FDT_ERR_INTERNAL;
 			break;
 		}
 		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
-				   name, namelen))
+				   name, namelen)) {
+			if (poffset)
+				*poffset = offset;
 			return prop;
+		}
 	}
 
 	if (lenp)
@@ -313,6 +353,25 @@ const struct fdt_property *fdt_get_property_namelen(const void *fdt,
 	return NULL;
 }
 
+
+const struct fdt_property *fdt_get_property_namelen(const void *fdt,
+						    int offset,
+						    const char *name,
+						    int namelen, int *lenp)
+{
+	/* Prior to version 16, properties may need realignment
+	 * and this API does not work. fdt_getprop_*() will, however. */
+	if (fdt_version(fdt) < 0x10) {
+		if (lenp)
+			*lenp = -FDT_ERR_BADVERSION;
+		return NULL;
+	}
+
+	return fdt_get_property_namelen_(fdt, offset, name, namelen, lenp,
+	    				 NULL);
+}
+
+
 const struct fdt_property *fdt_get_property(const void *fdt,
 					    int nodeoffset,
 					    const char *name, int *lenp)
@@ -324,12 +383,18 @@ const struct fdt_property *fdt_get_property(const void *fdt,
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 				const char *name, int namelen, int *lenp)
 {
+	int poffset;
 	const struct fdt_property *prop;
 
-	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
+	prop = fdt_get_property_namelen_(fdt, nodeoffset, name, namelen, lenp,
+					 &poffset);
 	if (!prop)
 		return NULL;
 
+	/* Handle realignment */
+	if (fdt_version(fdt) < 0x10 && (poffset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) >= 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
@@ -338,11 +403,16 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 {
 	const struct fdt_property *prop;
 
-	prop = fdt_get_property_by_offset(fdt, offset, lenp);
+	prop = fdt_get_property_by_offset_(fdt, offset, lenp);
 	if (!prop)
 		return NULL;
 	if (namep)
 		*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+
+	/* Handle realignment */
+	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
+	    fdt32_to_cpu(prop->len) >= 8)
+		return prop->data + 4;
 	return prop->data;
 }
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index baa882c..7ac3e8a 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -54,7 +54,7 @@
 #include <libfdt_env.h>
 #include <fdt.h>
 
-#define FDT_FIRST_SUPPORTED_VERSION	0x10
+#define FDT_FIRST_SUPPORTED_VERSION	0x02
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
 /* Error codes: informative error codes */
@@ -526,6 +526,9 @@ int fdt_next_property_offset(const void *fdt, int offset);
  * fdt_property structure within the device tree blob at the given
  * offset.  If lenp is non-NULL, the length of the property value is
  * also returned, in the integer pointed to by lenp.
+ * 
+ * Note that this code only works on device tree versions >= 16. fdt_getprop()
+ * works on all versions.
  *
  * returns:
  *	pointer to the structure representing the property
-- 
2.15.1

Re: [PATCH v4] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Thu, Jan 25, 2018 at 05:13:40AM +0000, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
> From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> 
> This can be useful in particular in the kernel when booting on systems
> with FDT-emitting firmware that is out of date. Releases of kexec-tools
> on ppc64 prior to the end of 2014 are notable examples of such.
> 
> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>

Applied, thanks.

I have fixed up the whitespace in a couple of places that git am
pointed out to me.

Testcases for reading the old format trees would be a nice follow up.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] Add limited read-only support for older (V2 and V3) device tree to libfdt.

[Nathan Whitehorn]

On 01/26/18 23:46, David Gibson wrote:
> On Thu, Jan 25, 2018 at 05:13:40AM +0000, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
>> From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
>>
>> This can be useful in particular in the kernel when booting on systems
>> with FDT-emitting firmware that is out of date. Releases of kexec-tools
>> on ppc64 prior to the end of 2014 are notable examples of such.
>>
>> Signed-off-by: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> Applied, thanks.
>
> I have fixed up the whitespace in a couple of places that git am
> pointed out to me.
>
> Testcases for reading the old format trees would be a nice follow up.
>

Thanks much, and apologies for the long back-and-forth. I'll try to gin 
up some test cases now that the test framework works on FreeBSD.
-Nathan