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

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > >
> > >
> > > Needs a commit message. For a single patch, you don't need a cover
> > > letter. The explanation should be here.

Right.  This will need to be resent with a real commit message and a
Signed-off-by line.

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

Right, we can read version 2, but we can't write it, so yes that looks
like a regression (which the commit message should explain).  It
certainly looks like a correct fix - a version 0x11 tree can't
possibly be compatible with a version 0x2 tree.

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

Ah.. good point.  If you tried to use an rw function immediately, it
would fail correctly with BADVERSION in fdt_rw_probe_().  However
fdt_open_into() will incorrectly think it can handle it (when in
reality it can only handle 0x10 and later) then mark it as version
0x11 so now the rest of the functions will think they can handle it
and fail horribly.

Justin, if you can fix up some of those additional problems that would
be great.  Otherwise I'll try to look at it, but no promises when.

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