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

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

[Justin Covell]

This adds a new FDT_LAST_COMPATIBLE_VERSION constant that should keep up to date to match devicetree spec. Since
the latest format spec specifies that version 17 is compatible with version 16, but not earlier versions, this
constant has been set to indicate that newly generated devicetrees are compatible with version 16. 

Signed-off-by: Justin Covell <jujugoboom-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Justin Covell (1):
  libfdt: Change last_comp_version to fit spec

 libfdt/fdt_sw.c | 2 +-
 libfdt/libfdt.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.25.1

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

[Justin Covell]

---
 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
 #define FDT_LAST_SUPPORTED_VERSION	0x11
 
 /* Error codes: informative error codes */
-- 
2.25.1

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

[Rob Herring]

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.

Besides matching the spec, what problem are you trying to solve?

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

>  #define FDT_LAST_SUPPORTED_VERSION     0x11
>
>  /* Error codes: informative error codes */
> --
> 2.25.1
>

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

[Justin Covell]

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

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

> >  #define FDT_LAST_SUPPORTED_VERSION     0x11
> >
> >  /* Error codes: informative error codes */
> > --
> > 2.25.1
> >

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

[Rob Herring]

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

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

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

[Justin Covell]

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

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

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

[David Gibson]

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

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

[David Gibson]

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