[U-Boot] [PATCH 3/5] fdt: Allow stacked overlays phandle references

[Pantelis Antoniou]

Hi Marek,

On Sat, 2017-07-01 at 16:07 +0200, Marek Vasut wrote:
> On 06/30/2017 06:23 PM, Pantelis Antoniou wrote:
> 
> [...]
> 
> > +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 = NULL;
> > +
> > +	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;
> 
> If you have symbol table in either the DTO or the base DT, but not in
> the other one, wouldn't it make sense to use that one symbol table
> instead of bailing out ?
> 

It's bailing out without an error.

The logic is simple; if there's no symbol table in the base tree, that
means that there's no symbols in the base device tree, i.e. not compiled
with the option to generate it. No point in inserting overlay symbols.

If there's no symbol table in the overlay that perfectly fine, no symbol
work then.

> > +	buf = malloc(FDT_PATH_MAX);
> > +	if (!buf)
> > +		return -FDT_ERR_NOSPACE;
> 
> Would it make sense to allocate this on stack ?
> 

FDT_PATH_MAX = 4K, dubious if the u-boot stack will have enough space.
Especially in SPL cases. 

> > +	/* 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) {
> > +			ret = path_len;
> > +			goto out;
> > +		}
> > +
> > +		/* skip autogenerated properties */
> > +		if (!strcmp(name, "name"))
> > +			continue;
> > +
> > +		/* format: /<fragment-name>/__overlay__/<relative-subnode-path> */
> > +
> > +		if (*path != '/') {
> > +			ret = -FDT_ERR_BADVALUE;
> > +			goto out;
> > +		}
> > +
> > +		/* get frag name first */
> 
> "frag name" ? What is this now, Unreal Tournament ? :)
> 

Fragment. There were computers before first person shooters.

> > +		s = strchr(path + 1, '/');
> > +		if (!s) {
> > +			ret = -FDT_ERR_BADVALUE;
> > +			goto out;
> > +		}
> > +		frag_name = path + 1;
> > +		frag_name_len = s - path - 1;
> > +
> > +		/* verify format */
> > +		len = strlen("/__overlay__/");
> > +		if (strncmp(s, "/__overlay__/", len)) {
> > +			ret = -FDT_ERR_NOTFOUND;
> > +			goto out;
> > +		}
> > +
> > +		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) {
> > +			ret = -FDT_ERR_NOTFOUND;
> > +			goto out;
> > +		}
> > +
> > +		/* an __overlay__ subnode must exist */
> > +		ret = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		/* get the target of the fragment */
> > +		ret = overlay_get_target(fdt, fdto, fragment);
> > +		if (ret < 0)
> > +			goto out;
> > +		target = ret;
> > +
> > +		/* get the path of the target */
> > +		ret = fdt_get_path(fdt, target, buf, FDT_PATH_MAX);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		len = strlen(buf);
> > +
> > +		/* if the target is root strip leading / */
> > +		if (len == 1 && buf[0] == '/')
> > +			len = 0;
> > +
> > +		/* make sure we have enough space */
> > +		if (len + 1 + rel_path_len + 1 > FDT_PATH_MAX) {
> > +			ret = -FDT_ERR_NOSPACE;
> > +			goto out;
> > +		}
> > +
> > +		/* create new symbol path in place */
> > +		buf[len] = '/';
> > +		memcpy(buf + len + 1, rel_path, rel_path_len);
> > +		buf[len + 1 + rel_path_len] = '\0';
> 
> Can snprintf() help somewhere here instead of the memcpy() ?
> 

It is going to be considerably slower (although that doesn't matter
much). We still need to do the space check before so it's not going to
be worth the change IMO.
 
> > +		ret = fdt_setprop_string(fdt, root_sym, name, buf);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> > +	ret = 0;
> > +
> > +out:
> > +	if (buf)
> > +		free(buf);
> > +
> > +	return ret;
> > +}
> > +
> >  int fdt_overlay_apply(void *fdt, void *fdto)
> >  {
> >  	uint32_t delta = fdt_get_max_phandle(fdt);
> > @@ -654,6 +796,10 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> >  	if (ret)
> >  		goto err;
> >  
> > +	ret = overlay_symbol_update(fdt, fdto);
> > +	if (ret)
> > +		goto err;
> > +
> >  	/*
> >  	 * The overlay has been damaged, erase its magic.
> >  	 */
> > 
> 
> 

Regards

-- Pantelis