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

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

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

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.

> 
> > +
> > +	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.. And we can't really use memcpy here, since we don't
know the size of the property at compile time.

> > +		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?

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

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]