Re: [PATCH 1/1] libfdt: Change last_comp_version to fit spec

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

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

On Wed, Dec 02, 2020 at 05:21:47PM -0800, Justin Covell wrote:
> On Wed, Dec 02, 2020 at 08:25:04AM -0700, Rob Herring wrote:
> > On Wed, Dec 2, 2020 at 12:46 AM Justin Covell <jujugoboom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >
> > > On Mon, Nov 30, 2020 at 10:22:09AM -0700, Rob Herring wrote:
> > > > On Tue, Nov 24, 2020 at 10:50 AM Justin Covell <jujugoboom-Re5JQEeQqe8@public.gmane.orgm> wrote:
> > > > >
> > > >
> > > > Needs a commit message. For a single patch, you don't need a cover
> > > > letter. The explanation should be here.
> > > >
> > > > Besides matching the spec, what problem are you trying to solve?
> > > >
> > >
> > > Sorry about that, first time contributing. I'm trying to help with
> > > interoperability with other libraries that are made to read/write DTBs
> > > by matching the spec.
> > 
> > What library? Why does libfdt not work?
> > 
> 
> I was attempting to use a DTB library for Rust, and it will fail if the
> set last_comp_version doesn't match what is expected.

Hmm.  Although there's definitely a bug in libfdt here, depending on
exactly what "expected" means, that doesn't necessarily sound like
correct behaviour on the part of the Rust library.

> > > > > ---
> > > > >  libfdt/fdt_sw.c | 2 +-
> > > > >  libfdt/libfdt.h | 1 +
> > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> > > > > index 68b543c..4c569ee 100644
> > > > > --- a/libfdt/fdt_sw.c
> > > > > +++ b/libfdt/fdt_sw.c
> > > > > @@ -377,7 +377,7 @@ int fdt_finish(void *fdt)
> > > > >         fdt_set_totalsize(fdt, newstroffset + fdt_size_dt_strings(fdt));
> > > > >
> > > > >         /* And fix up fields that were keeping intermediate state. */
> > > > > -       fdt_set_last_comp_version(fdt, FDT_FIRST_SUPPORTED_VERSION);
> > > > > +       fdt_set_last_comp_version(fdt, FDT_LAST_COMPATIBLE_VERSION);
> > > > >         fdt_set_magic(fdt, FDT_MAGIC);
> > > > >
> > > > >         return 0;
> > > > > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > > > > index 89adee3..abad93b 100644
> > > > > --- a/libfdt/libfdt.h
> > > > > +++ b/libfdt/libfdt.h
> > > > > @@ -14,6 +14,7 @@ extern "C" {
> > > > >  #endif
> > > > >
> > > > >  #define FDT_FIRST_SUPPORTED_VERSION    0x02
> > > > > +#define FDT_LAST_COMPATIBLE_VERSION 0x10
> > > >
> > > > If the above change is correct (I'm not sure it is offhand), why not
> > > > just bump up FDT_FIRST_SUPPORTED_VERSION value?
> > > >
> > >
> > > I didn't want to bump the FDT_FIRST_SUPPORTED_VERSION to maintin
> > > backwards compatability, and assumed that libfdt actually does support
> > > working with DTBs down to version 2.
> > 
> > Looking at this more closely, I think your change is correct. It's
> > actually fixing a regression. Prior to commit f1879e1a50eb ("Add
> > limited read-only support for older (V2 and V3) device tree to
> > libfdt."), libfdt would set last_comp_version to 16. Now it sets it to
> > 2, but really 2 is what libfdt can read, not what should be in
> > last_comp_version.
> > 
> > Also, I'm not certain what happens if you tried to modify a version 2
> > dtb. It doesn't look like we do anything to fail gracefully.
> > 
> > Rob
> 
> I'm happy to test modifying a version 2 dtb and see what happens, but I
> don't have one around, and feel that might be a little out of the scope
> of this patch.

AFAICT, dtc still supports generating old dtb versions, using the -V
flag.  So you could convert a dtb into v2 with:
	dtc -I dtb -O dtb -V 2 input.dtb -o output.dtb

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