Re: [PATCH v2 6/6] libfdt: Add overlay application function

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

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

On Wed, Jul 13, 2016 at 10:12:16PM +0200, Maxime Ripard wrote:
> Hi David,
> 
> I'll fix your comments, but I have a bunch of questions, see below.
> 
> On Wed, Jul 13, 2016 at 12:31:20AM +1000, David Gibson wrote:
> > > +/**
> > > + * overlay_adjust_node_phandles - Offsets the phandles of a node
> > > + * @fdto: Device tree overlay blob
> > > + * @node: Offset of the node we want to adjust
> > > + * @delta: Offset to shift the phandles of
> > > + *
> > > + * overlay_adjust_node_phandles() adds a constant to all the phandles
> > > + * of a given node. This is mainly use as part of the overlay
> > > + * application process, when we want to update all the overlay
> > > + * phandles to not conflict with the overlays of the base device tree.
> > > + *
> > > + * returns:
> > > + *      0 on success
> > > + *      Negative error code on failure
> > > + */
> > > +static int overlay_adjust_node_phandles(void *fdto, int node,
> > > +					uint32_t delta)
> > > +{
> > > +	unsigned char found = 0;
> > 
> > Just use an int.  I should probably think about introducing bool to libfdt.
> > 
> > > +	int child;
> > > +	int ret;
> > > +
> > > +	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> > > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > > +		return ret;
> > > +
> > > +	if (!ret)
> > > +		found = 1;
> > > +
> > > +	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> > > +	if (ret && ret != -FDT_ERR_NOTFOUND)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * If neither phandle nor linux,phandle have been found return
> > > +	 * an error.
> > > +	 */
> > > +	if (!found && !ret)
> > > +		return ret;
> > 
> > Clearer to return 0 here, since ret must equal 0.
> > 
> > Except.. it's not mandatory for a node to have a phandle, so this just
> > looks wrong, anyway.
> 
> Hmmm, that's true. I have no idea how that can possibly work
> though. It should break at the very first node, since the root node
> will not have one...
> 
> I'll look into this.

Ok.

> > > +
> > > +	fdt_for_each_subnode(child, fdto, node)
> > > +		overlay_adjust_node_phandles(fdto, child, delta);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * overlay_adjust_local_phandles - Adjust the phandles of a whole overlay
> > > + * @fdto: Device tree overlay blob
> > > + * @delta: Offset to shift the phandles of
> > > + *
> > > + * overlay_adjust_local_phandles() adds a constant to all the
> > > + * phandles of an overlay. This is mainly use as part of the overlay
> > > + * application process, when we want to update all the overlay
> > > + * phandles to not conflict with the overlays of the base device tree.
> > > + *
> > > + * returns:
> > > + *      0 on success
> > > + *      Negative error code on failure
> > > + */
> > > +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);
> > > +}
> > > +
> > > +/**
> > > + * overlay_update_local_node_references - Adjust the overlay references
> > > + * @fdto: Device tree overlay blob
> > > + * @tree_node: Node offset of the node to operate on
> > > + * @fixup_node: Node offset of the matching local fixups node
> > > + * @delta: Offset to shift the phandles of
> > > + *
> > > + * overlay_update_local_nodes_references() update the phandles
> > > + * pointing to a node within the device tree overlay by adding a
> > > + * constant delta.
> > > + *
> > > + * This is mainly used as part of a device tree application process,
> > > + * where you want the device tree overlays phandles to not conflict
> > > + * with the ones from the base device tree before merging them.
> > > + *
> > > + * returns:
> > > + *      0 on success
> > > + *      Negative error code on failure
> > > + */
> > > +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 unsigned char *fixup_val, *tree_val;
> > 
> > You might as well make fixup_val a u32 *, it should remove some casts
> > below.
> 
> Yes, but then, if it's poorly formatted, we might get unaligned
> accesses too..

No.  The flattening format guarantees that property values are 32-bit
aligned, so since the property consists of nothing but u32s they'll
all remain aligned.

The difference with applying the fixups is that the properties could
have formats that are a mixture of u32s and strings or other unaligned
data, so the locations to be fixed may not be aligned.

> And we can't really use memcpy here, since we don't
> know the size of the property at compile time.

Um.. what?

> 
> > > +		const char *name;
> > > +		int fixup_len;
> > > +		int tree_len;
> > > +		int i;
> > > +
> > > +		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> > > +						  &name, &fixup_len);
> > > +		if (!fixup_val)
> > > +			return fixup_len;
> > > +
> > > +		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> > > +		if (!tree_val)
> > > +			return tree_len;
> > > +		for (i = 0; i < fixup_len; i += sizeof(uint32_t)) {
> > 
> > This could lead to an unsafe dereference if fixup_len is not a
> > multiple of 4 (i.e. badly formatted fixup properyt).  Another reason
> > to use a u32 *.
> 
> Then maybe the safest would be to simply check that the length is a
> multiple of 4, and return an error if it's not?

Yes, I think so.

> > > diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> > > index 6a2662b2abaf..054758298de9 100755
> > > --- a/tests/run_tests.sh
> > > +++ b/tests/run_tests.sh
> > > @@ -280,6 +280,14 @@ libfdt_tests () {
> > >      run_test get_alias aliases.dtb
> > >      run_test path_offset_aliases aliases.dtb
> > >  
> > > +    # Overlay tests
> > > +    echo "/dts-v1/; / {};" | $DTC -@ > /dev/null 2>&1
> > 
> >        ^^^^^
> > 
> > What's this about?
> 
> In current master tree, calling dtc with -@ will fail, since the
> support to compile the overlays is not upstream yet (afaik).
> 
> This is just to make sur that the dtc binary is actually able to
> compile something we can run our test on. Otherwise we would have
> failed tests, while the code might or might not work, which seems bad.

Ah, yes.  I was mixing up the dtc patches with the libfdt patches.

Hrm.. it would be nice if the libfdt testcases, or at least some of
the, didn't require the patched dtc.  It might be worth making
examples with manually constructed __symbols__ and __fixups__ for this
purpose.

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