Re: [PATCH v2] Add limited read-only support for older (V2 and V3) device tree to libfdt.

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

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

On Thu, Jan 18, 2018 at 12:41:28PM -0800, Nathan Whitehorn wrote:
> 
> 
> On 01/17/18 20:59, David Gibson wrote:
> > On Sat, Dec 30, 2017 at 06:28:58PM -0800, nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org wrote:
> > > From: Nathan Whitehorn <nwhitehorn-h+KGxgPPiopAfugRpC6u6w@public.gmane.org>
> > > 
> > > This can be useful in particular in the kernel when booting on systems
> > > with FDT-emitting firmware that is out of date.
> > Hrm, really, *really* out of date.  V2 and V3 are positively ancient.
> 
> Yes, I was *really* disappointed to run into these things in the wild.
> 
> > 
> > > Releases of kexec-tools
> > > on ppc64 prior to the end of 2014 are notable examples of such.
> > Good grief, that's ridiculous.  They were also using dts-v0 years and
> > years after it should have been gotten rid of.
> 
> Yes. And Red Hat at least is still shipping such versions of kexec-tools
> today...
> 
> From a fully up-to-date supported RHEL6 system:
> nwhitehorn@gordita:~$ kexec -v
> kexec-tools 2.0.0 released 19th July 2008

*sigh*

> > Anyway that said, the changes below don't look too bad.  There's a few
> > nits, but in principle I'd be ok to apply
> 
> OK, great. I'll plan to submit a revised diff later today or
> tomorrow.

Ok, thanks.

[snip]
> > > +	if (fdt_version(fdt) < 0x10 && nodeoffset != 0) {
> > > +		/*
> > > +		 * For old FDT versions, match the naming conventions of V16:
> > > +		 * give only the leaf name (after all /) except for the
> > > +		 * root node, where we should still return / rather than ""
> > What's the rationale for returning "/" rather than "" on the root
> > node?  a V16 file will return "", typically.
> 
> Are you sure?

Pretty sure, yeah..

> The immediate motivation was that parts of the FreeBSD kernel
> were breaking if it was "". Since those parts work fine with V16 trees, I
> did this. I will double-check what the exact problem is -- it might be on
> our side somehow.

[snip]
> > > @@ -324,12 +364,33 @@ const struct fdt_property *fdt_get_property(const void *fdt,
> > >   const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> > >   				const char *name, int namelen, int *lenp)
> > >   {
> > > -	const struct fdt_property *prop;
> > > +	const struct fdt_property *prop = NULL;
> > > +	int offset = nodeoffset;
> > > -	prop = fdt_get_property_namelen(fdt, nodeoffset, name, namelen, lenp);
> > Couldn't you use an fdt_get_property_namelen_() here instead of having
> > to duplicate most of its logic.
> 
> You could. Since it is pretty short, adding another function did not seem to
> be much shorter. Happy to do this if you prefer.

It's not so much the physical shortness, but the fact that getting the
property scan logic exactly right does have some edge cases you have
to be careful of, so I'd rather only have to do it in one place.

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