Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson <david@gibson.dropbear.id.au>]

On Sat, Mar 17, 2007 at 07:22:27PM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> >        while ((addr = va_arg(ap, void *))) {
> >                devp = find_node_by_prop_value(NULL,
> > linux,network-index",
> >                                               &index, sizeof(index));
> > 
> >                if (devp)
> >                       setprop(devp, "local-mac-address", addr, 6);
> 
> The problem with this version is that it only updates local-mac-address, 
> and only if it already exists.  

Uh.. no.  Both my version and Scott's use setprop.  I think at present
that won't add the property if it doesn't exist, but that's a bug
which one of Scott's other patches fixes.  In any case, they have the
same behaviour in terms of whether the property needs to exist first.

> Scott's version also updates 
> mac-address.  

Well, first, details shmetails.  It's the interface and approach I
thin is better.  Second, I don't think the zImage should be setting
mac-address anyway.  In OF that property is based on what the
interface has been used for during booting, which the zImage doesn't
know.  The kernel doesn't need mac-address if local-mac-address is
present, so we should just leave it out.

> Also, in the future, we hope to eliminate *all* MAC 
> address entries from the DTS files, and the boot loader and/or 
> bootwrapper will add one.

I don't think that's really a good idea.  The bootloader certainly
should be able to add the property if it's not there, but it seems
silly to make the bootloader do memmove()s to insert a new property
when it's basically just as easy to set up the device tree to allow an
in-place edit.

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