Re: [PATCH v3 10/12] libfdt: Add overlay application function

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

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

On Mon, Jun 27, 2016 at 01:40:00PM +0200, Maxime Ripard wrote:
> Hi David,
> 
> On Mon, Jun 27, 2016 at 03:26:07PM +1000, David Gibson wrote:
> > > +static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
> > > +{
> > > +	const uint32_t *val;
> > > +	int len;
> > > +
> > > +	val = fdt_getprop(fdto, fragment, "target", &len);
> > > +	if (!val || (*val == 0xffffffff) || (len != sizeof(*val)))
> > > +		return 0;
> > 
> > This doesn't distinguish between a missing property (which may
> > indicate a valid overlay using a target-path or some other method)
> > and a badly formatted 'target' property, which is definitely an error
> > in the overlay.
> > 
> > I think those should be treated differently.
> 
> AFAIK, phandles can have any 32 bits values but 0xffffffff. In order
> to cover the two cases, we would need to have some error code, but
> that doesn't really work with returning a uint32_t.

Actually phandles can have any value except 0xffffffff *or* 0.  So you
can use 0 for "couldn't find" and -1 for "badly formatted".

> Or maybe we can simply remove all the checks but the missing property,
> and let fdt_node_offset_by_phandle deal with the improper values?
> 
> > 
> > > +	return fdt32_to_cpu(*val);
> > > +}
> > > +
> > > +static int overlay_get_target(const void *fdt, const void *fdto,
> > > +			      int fragment)
> > > +{
> > > +	uint32_t phandle;
> > > +	const char *path;
> > > +
> > > +	/* Try first to do a phandle based lookup */
> > > +	phandle = overlay_get_target_phandle(fdto, fragment);
> > > +	if (phandle)
> > > +		return fdt_node_offset_by_phandle(fdt, phandle);
> > > +
> > > +	/* And then a path based lookup */
> > > +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > > +	if (!path)
> > > +		return -FDT_ERR_NOTFOUND;
> > > +
> > > +	return fdt_path_offset(fdt, path);
> > > +}
> > > +
> > > +static int overlay_phandle_add_offset(void *fdt, int node,
> > > +				      const char *name, uint32_t delta)
> > > +{
> > > +	const uint32_t *val;
> > > +	uint32_t adj_val;
> > > +	int len;
> > > +
> > > +	val = fdt_getprop(fdt, node, name, &len);
> > > +	if (!val)
> > > +		return len;
> > > +
> > > +	if (len != sizeof(*val))
> > > +		return -FDT_ERR_BADSTRUCTURE;
> > > +
> > > +	adj_val = fdt32_to_cpu(*val);
> > > +	adj_val += delta;
> > 
> > You should probably check for overflow here.
> > 
> > > +
> > > +	return fdt_setprop_inplace_u32(fdt, node, name, adj_val);
> > > +}
> > > +
> > > +static int overlay_adjust_node_phandles(void *fdto, int node,
> > > +					uint32_t delta)
> > > +{
> > > +	bool found = false;
> > > +	int child;
> > > +	int ret;
> > > +
> > > +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> > > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > > +		return ret;
> > > +
> > > +	if (!ret)
> > > +		found = true;
> > > +
> > > +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> > > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > > +		return ret;
> > 
> > I think the check for phandle vs. linux,phandle should be folded into
> > overlay_phandle_add_offset().
> 
> I created overlay_phandle_add_offset to avoid duplicating the getprop,
> offset, setprop, pattern which I don't think is a good idea.
> 
> And we'll have to have that kind of errors construct anyway to know if
> we modified any of the two, which is a success, or none, which is a
> failure.

Hm.. ok, you convinced me.

> > > +	/*
> > > +	 * If neither phandle nor linux,phandle have been found return
> > > +	 * an error.
> > > +	 */
> > > +	if (!found && !ret)
> > > +		return ret;
> > > +
> > > +	fdt_for_each_subnode(fdto, child, node)
> > > +		overlay_adjust_node_phandles(fdto, child, delta);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> > > +{
> > > +	/*
> > > +	 * Start adjusting the phandles from the overlay root
> > > +	 */
> > > +	return overlay_adjust_node_phandles(fdto, 0, delta);
> > > +}
> > > +
> > > +static int overlay_update_local_node_references(void *fdto,
> > > +						int tree_node,
> > > +						int fixup_node,
> > > +						uint32_t delta)
> > > +{
> > > +	int fixup_prop;
> > > +	int fixup_child;
> > > +	int ret;
> > > +
> > > +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> > > +		const uint32_t *val = NULL;
> > > +		uint32_t adj_val, index;
> > > +		const char *name;
> > > +		int fixup_len;
> > > +		int tree_len;
> > > +
> > > +		val = fdt_getprop_by_offset(fdto, fixup_prop,
> > > +					    &name, &fixup_len);
> > > +		if (!val)
> > > +			return fixup_len;
> > > +		index = fdt32_to_cpu(*val);
> > 
> > This still doesn't handle the case of multiple fixups within the same
> > property.  That would be entirely plausible for something like an
> > interrupt-map property.
> 
> Good point, I'll fix that and add a test case for it.
> 
> > > +		val = fdt_getprop(fdto, tree_node, name, &tree_len);
> > > +		if (!val)
> > > +			return tree_len;
> > > +
> > > +		adj_val = fdt32_to_cpu(*(val + (index / sizeof(uint32_t))));
> > 
> > phandle values need not be aligned within the property containint them
> > (e.g. properties which mix strings and integer values).  So you're
> > going to need to work directly with a byte offset here.
> 
> I didn't even know it was something that was valid.

I'm afraid so.  In general, property values are treated as packed
bytestrings and never have internal alignment constraints.

> It would probably require a temporary variable and a memcpy though to
> deal with architectures that cannot do unaligned accesses.

Alas, yes.  Make sure you put a comment there to explain this, because
it will appear unnecessary to people only familiar with archs that
allow unaligned access (which includes both x86 and Power).

> 
> > 
> > > +		adj_val += delta;
> > > +		adj_val = cpu_to_fdt32(adj_val);
> > > +
> > > +		ret = fdt_setprop_inplace_namelen_by_index(fdto, tree_node,
> > > +							   name, strlen(name),
> > > +							   index, &adj_val,
> > > +							   sizeof(adj_val));
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> > > +		const char *fixup_child_name = fdt_get_name(fdto, fixup_child,
> > > +							    NULL);
> > > +		int tree_child;
> > > +
> > > +		tree_child = fdt_subnode_offset(fdto, tree_node,
> > > +						fixup_child_name);
> > > +		if (tree_child < 0)
> > > +			return tree_child;
> > > +
> > > +		ret = overlay_update_local_node_references(fdto,
> > > +							   tree_child,
> > > +							   fixup_child,
> > > +							   delta);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int overlay_update_local_references(void *dto, uint32_t delta)
> > > +{
> > > +	int fixups;
> > > +
> > > +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> > > +	if (fixups < 0) {
> > > +		/*
> > > +		 * There's no local phandles to adjust, bail out
> > > +		 */
> > > +		if (fixups == -FDT_ERR_NOTFOUND)
> > > +			return 0;
> > > +
> > > +		return fixups;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Update our local references from the root of the tree
> > > +	 */
> > > +	return overlay_update_local_node_references(dto, 0, fixups,
> > > +						    delta);
> > > +}
> > > +
> > > +static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> > > +				     int symbols_off,
> > > +				     const char *path, uint32_t path_len,
> > > +				     const char *name, uint32_t name_len,
> > > +				     int index, const char *label)
> > > +{
> > > +	const char *symbol_path;
> > > +	uint32_t phandle;
> > > +	int symbol_off, fixup_off;
> > > +	int prop_len;
> > > +
> > > +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> > > +				  &prop_len);
> > > +	if (!symbol_path)
> > > +		return -FDT_ERR_NOTFOUND;
> > > +
> > > +	symbol_off = fdt_path_offset(fdt, symbol_path);
> > > +	if (symbol_off < 0)
> > > +		return symbol_off;
> > > +
> > > +	phandle = fdt_get_phandle(fdt, symbol_off);
> > > +	if (!phandle)
> > > +		return -FDT_ERR_NOTFOUND;
> > > +
> > > +	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
> > > +	if (fixup_off < 0)
> > > +		return fixup_off;
> > > +
> > > +	phandle = cpu_to_fdt32(phandle);
> > > +	return fdt_setprop_inplace_namelen_by_index(fdto, fixup_off,
> > > +						    name, name_len, index,
> > > +						    &phandle, sizeof(phandle));
> > 
> > This will be broken on BE systems because fdt_get_phandle() byteswaps
> > to native order, but setprop_inplace doesn't byteswap back (because
> > it's input is a byte array, not a u32).
> 
> Hmmmm, so calling cpu_to_fdt32 is not enough? I'm not sure what the
> proper fix for that would be.

Oh, sorry, I just missed the cpu_to_fdt32().

> > > +};
> > > +
> > > +static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> > > +				 int property)
> > > +{
> > > +	const char *value;
> > > +	const char *label;
> > > +	int len;
> > > +
> > > +	value = fdt_getprop_by_offset(fdto, property,
> > > +				      &label, &len);
> > > +	if (!value)
> > > +		return len;
> > > +
> > > +	do {
> > > +		const char *prop_string = value;
> > > +		const char *path, *name;
> > > +		uint32_t path_len, name_len;
> > > +		char *sep, *endptr;
> > > +		int index;
> > > +		int ret;
> > > +
> > > +		path = prop_string;
> > > +		sep = strchr(prop_string, ':');
> > > +		path_len = sep - path;
> > 
> > You need to check for strchr() returning NULL (indicating a badly
> > formatted fixup missing a separator).
> > 
> > Also... strchr() is not safe.  Again, if the fixup string is badly
> > formatted and doesn't have a terminating \0, this could overrun, so
> > you want memchr() instead.
> 
> Ok
> 
> > > +		name = sep + 1;
> > > +		sep = strchr(name, ':');
> > > +		name_len = sep - name;
> > 
> > And again.
> > 
> > > +		index = strtoul(sep + 1, &endptr, 10);
> > > +		if (endptr <= (sep + 1))
> > > +			return -FDT_ERR_BADSTRUCTURE;
> > 
> > IIRC the index value is supposed to go to the end of that portion of
> > the fixup, so you should be able to check for *endptr == '\0'.
> > 
> > Except.. strtoul() could also overrun if the fixup property doesn't
> > contain a \0 like it should.
> > 
> > So I think you'd be better off using a memchr() (or strnlen()) to
> > search for the \0 before you attempt parsing the innards of a single
> > fixup string.
> 
> Ok
> 
> > Ugh.  I hate that we're parsing text numbers in decimal here, but
> > we're pretty much stuck with that format here.  Doubly so that it's a
> > gratuitously different way of encoding the (node, property, offset)
> > tuple from local_fixups.
> 
> I'm not sure to get what you're saying here. Is that a general remark
> on the format used by the overlays, or an hint that my code should be
> reworked to deal with this differently?

Just a general remark on the format.

> 
> > 
> > > +		index = strtoul(sep + 1, &endptr, 10);
> > > +		if (endptr <= (sep + 1))
> > > +			return -FDT_ERR_BADSTRUCTURE;
> > > +
> > > +		len -= strlen(prop_string) + 1;
> > > +		value += strlen(prop_string) + 1;
> > 
> > Please don't double call strlen() on the same string - remember it's
> > O(n) in C.
> 
> Ok
> 
> > > +
> > > +		ret = overlay_fixup_one_phandle(fdt, fdto, symbols_off,
> > > +						path, path_len, name, name_len,
> > > +						index, label);
> > > +		if (ret)
> > > +			return ret;
> > > +	} while (len > 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int overlay_fixup_phandles(void *dt, void *dto)
> > > +{
> > > +	int fixups_off, symbols_off;
> > > +	int property;
> > > +
> > > +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> > > +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> > > +
> > > +	fdt_for_each_property_offset(property, dto, fixups_off)
> > > +		overlay_fixup_phandle(dt, dto, symbols_off, property);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_overlay_node(void *dt, int target,
> > > +			      void *dto, int overlay)
> > 
> > I think 'overlay' should be 'fragment' here for consistency?
> 
> Ok.
> 
> > > +{
> > > +	int property;
> > > +	int node;
> > > +
> > > +	fdt_for_each_property_offset(property, dto, overlay) {
> > > +		const char *name;
> > > +		const void *prop;
> > > +		int prop_len;
> > > +		int ret;
> > > +
> > > +		prop = fdt_getprop_by_offset(dto, property, &name,
> > > +					     &prop_len);
> > > +		if (!prop)
> > > +			return -FDT_ERR_INTERNAL;
> > 
> > Actually, you probably should check the error code returned in
> > prop_len.  Getting a NOTFOUND would indicate an internal error, but
> > you could also get BADSTRUCTURE or similar errors which would indicate
> > an error in input.
> 
> Ok.
> 
> > > +int fdt_overlay_apply(void *fdt, void *fdto)
> > > +{
> > > +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> > > +	int ret;
> > > +
> > > +	FDT_CHECK_HEADER(fdt);
> > > +	FDT_CHECK_HEADER(fdto);
> > > +
> > > +	ret = overlay_adjust_local_phandles(fdto, delta);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > > +	ret = overlay_update_local_references(fdto, delta);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > > +	ret = overlay_fixup_phandles(fdt, fdto);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > > +	ret = overlay_merge(fdt, fdto);
> > > +	if (!ret)
> > > +		goto out;
> > > +
> > > +err:
> > 
> > This is a confusing use of gotos - this looks like it is in the exit
> > path for both success and failure cases, but it's not due to the
> > easy-to-miss goto out above.
> 
> I know, but I couldn't really come up with something better to avoid
> code duplication.
> 
> Or maybe it's too small to care?

I think a little duplicated code is the lesser evil here.

-- 
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: 819 bytes --]