Re: [PATCH v2 1/2] fdt: Allow stacked overlays phandle references

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

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

On Thu, Jul 13, 2017 at 12:24:18AM +0300, Pantelis Antoniou wrote:
> This patch enables an overlay to refer to a previous overlay's
> labels by performing a merge of symbol information at application
> time.
> 
> In a nutshell it allows an overlay to refer to a symbol that a previous
> overlay has defined. It requires both the base and all the overlays
> to be compiled with the -@ command line switch so that symbol
> information is included.

[snip]
> +/**
> + * overlay_symbol_update - Update the symbols of base tree after a merge
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_symbol_update() updates the symbols of the base tree with the
> + * symbols of the applied overlay
> + *
> + * This is the last step in the device tree overlay application
> + * process, allowing the reference of overlay symbols by subsequent
> + * overlay operations.
> + *
> + * returns:
> + *      0 on success
> + *      Negative error code on failure
> + */
> +static int overlay_symbol_update(void *fdt, void *fdto)
> +{
> +	int root_sym, ov_sym, prop, path_len, fragment, target;
> +	int len, frag_name_len, ret, rel_path_len;
> +	const char *s;
> +	const char *path;
> +	const char *name;
> +	const char *frag_name;
> +	const char *rel_path;
> +	char *buf;
> +	void *p;
> +
> +	root_sym = fdt_subnode_offset(fdt, 0, "__symbols__");
> +	ov_sym = fdt_subnode_offset(fdto, 0, "__symbols__");
> +
> +	/* if neither exist we can't update symbols, but that's OK */
> +	if (root_sym < 0 || ov_sym < 0)
> +		return 0;
> +
> +
> +	/* iterate over each overlay symbol */
> +	fdt_for_each_property_offset(prop, fdto, ov_sym) {
> +
> +		path = fdt_getprop_by_offset(fdto, prop, &name, &path_len);
> +		if (!path)
> +			return path_len;
> +
> +		/* skip autogenerated properties */
> +		if (!strcmp(name, "name"))
> +			continue;
> +
> +		/* format: /<fragment-name>/__overlay__/<relative-subnode-path> */
> +
> +		if (*path != '/')
> +			return -FDT_ERR_BADVALUE;
> +
> +		/* get fragment name first */
> +		s = strchr(path + 1, '/');
> +		if (!s)
> +			return -FDT_ERR_BADVALUE;
> +
> +		frag_name = path + 1;
> +		frag_name_len = s - path - 1;
> +
> +		/* verify format */
> +		len = strlen("/__overlay__/");
> +		if (strncmp(s, "/__overlay__/", len))
> +			return -FDT_ERR_NOTFOUND;
> +
> +		rel_path = s + len;
> +		rel_path_len = strlen(rel_path);
> +
> +		/* find the fragment index in which the symbol lies */
> +		fdt_for_each_subnode(fragment, fdto, 0) {
> +
> +			s = fdt_get_name(fdto, fragment, &len);
> +			if (!s)
> +				continue;
> +
> +			/* name must match */
> +			if (len == frag_name_len && !memcmp(s, frag_name, len))
> +				break;
> +		}
> +
> +		/* not found? */
> +		if (fragment < 0)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		/* an __overlay__ subnode must exist */
> +		ret = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +		if (ret < 0)
> +			return ret;
> +
> +		/* get the target of the fragment */
> +		ret = overlay_get_target(fdt, fdto, fragment);
> +		if (ret < 0)
> +			return ret;
> +		target = ret;
> +
> +		len = fdt_get_path_len(fdt, target);
> +
> +		ret = fdt_setprop_alloc(fdt, root_sym, name,
> +				len + (len > 1) + rel_path_len + 1, &p);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* again in case setprop_alloc changed it */
> +		ret = overlay_get_target(fdt, fdto, fragment);
> +		if (ret < 0)
> +			return ret;
> +		target = ret;
> +
> +		buf = p;
> +		if (len > 1) { /* target is not root */
> +			ret = fdt_get_path(fdt, target, buf, len + 1);
> +			if (ret < 0)
> +				return ret;
> +
> +		} else
> +			len--;
> +
> +		buf[len] = '/';
> +		memcpy(buf + len + 1, rel_path, rel_path_len);
> +		buf[len + 1 + rel_path_len] = '\0';
> +	}
> +
> +	return 0;
> +}

So, as noted in the other thread, even by the standards of the slow
functions in libfdt, fdt_get_path_len() is really slow as proposed.
So the question is can we avoid using fdt_get_path() here.

When the target is designated with target-path, then yes we can:
rather than going via the actual target node, we can just take the
path directly from the property.

For targets designated by phandle it's.. fiddly.  AFAICT the path will
already be found in the existing blobs: specifically it will be in the
__symbols__ of the base tree for the label which the target is being
applied to.  Getting there is kind of awkward though: we'd have to
search __fixups__ in the overlay for the entry applying to the
relevant target property, then follow that to find the right
__symbols__ entry.

I'm not sure at this point whether that's preferable to the slow
fdt_get_path_len().

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