Re: [RFC PATCH 06/10] MIPS: Octeon: Initialize and fixup device tree.

[Grant Likely <grant.likely@secretlab.ca>]

On Wed, Feb 23, 2011 at 10:40:32AM -0800, David Daney wrote:
> On 02/23/2011 09:41 AM, Grant Likely wrote:
> >On Tue, Feb 22, 2011 at 12:57:50PM -0800, David Daney wrote:
> >>Signed-off-by: David Daney<ddaney@caviumnetworks.com>
> >>---
> >>  arch/mips/Kconfig                         |    2 +
> >>  arch/mips/cavium-octeon/octeon-platform.c |  280 +++++++++++++++++++++++++++++
> >>  arch/mips/cavium-octeon/setup.c           |   17 ++
> >>  3 files changed, 299 insertions(+), 0 deletions(-)
> >
> >I've got an odd feeling of foreboding about this patch.  It makes me
> >nervous, but I can't articulate why yet.  Gut-wise I'd rather see the
> >device tree pruned/fixed up before it gets unflattened,
> 
> I chose to work on the unflattened form because there were already
> functions to do it.  I didn't see anything that would make
> manipulating the flattened form easy.
> 
> I agree that working on the unflattened form would be best.  At a
> minium the /proc/device-tree structure would better reflect reality.
> 
> What do you think about adding some helper functions to
> drivers/of/fdt.c for the manipulation of the flattened form?

It would probably be easier/safer to link libfdt into the kernel
proper.  It's already used in the powerpc bootwrapper, and there has
been talk about replacing some of fdt.c with libfdt.  See
scripts/dtc/libfdt

> 
> >or for the
> >kernel to have a separate .dtb linked in for each legacy platform.
> 
> I think there are too many variants to make this viable.

Out of curiosity, how many variants?

btw, did you know about the dtc '/include/' functionality?  It is
possible to set up .dts include files that represent a SoC and can be
modified by the .dts files that include them.  See
arch/powerpc/boot/dts/*5200*.dts

> 
> > I
> >need to think about this some more....
> >
> >I've made some comments below anyway.
> 
> And I will respond.  Although if I end up modifying the flattened
> form, it will all change.
> 
> >
> [...]
> >>+
> >>+static int __init set_phy_addr_prop(struct device_node *n, int phy)
> >>+{
> >>+	u32 *vp;
> >>+	struct property *old_p;
> >>+	struct property *p = kzalloc(sizeof(struct device_node) + sizeof(u32), GFP_KERNEL);
> >>+	if (!p)
> >>+		return -ENOMEM;
> >>+	/* The value will immediatly follow the node in memory. */
> >>+	vp = (u32 *)(&p[1]);
> >
> >This is unsafe (I was on the losing end of an argument when I tried to
> >do exactly the same thing).  If you want to allocate 2 things with one
> >appended to the other, then you need to define a structure
> >with the two element in it and allocate the size of that structure.
> 
> Weird.  alloc_netdev() does this, so it is not unheard of.

Not unheard of, but still bad practise.

> >>+	old_p = of_find_property(n, "reg", NULL);
> >>+	if (old_p)
> >>+		prom_remove_property(n, old_p);
> >>+	return prom_add_property(n, p);
> >
> >Would it not be more efficient to change the value in the existing reg
> >property instead of doing this allocation song-and-dance?
> >
> 
> I think I did it this way to try to get /proc/device-tree to reflect
> the new value.

Sounds like a bug in /proc/device-tree.  :-)  /proc/device-tree should
be pointing directly at the device tree property itself.  I'd be
surprised if modifying the data of 'reg' didn't show up there.

> >>+}
> >>+arch_initcall(octeon_fix_device_tree);
> >
> >Calling this from an initcall really makes me nervous.  I'm worried
> >about ordering issues.  Why can this code not be part of the prune
> >routine above?
> >
> 
> Again, done to try to make /proc/device-tree reflect reality.

yeah, /proc/device-tree should not be driving design decisions.  Let's
try to fix it instead.

g.