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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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