On Fri, Jan 31, 2020 at 11:14:27AM -0700, Simon Glass wrote:
> Hi David,
>
> On Tue, 28 Jan 2020 at 01:37, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Nov 12, 2019 at 06:54:21PM -0700, Simon Glass wrote:
> > > Allow enabling FDT_ASSUME_LATEST to disable version checks.
> > >
> > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> >
> > LGTM, one minor comment below.
> >
> > > ---
> > >
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > > libfdt/fdt.c | 34 +++++++++++++++++++++-------------
> > > libfdt/fdt_ro.c | 16 ++++++++--------
> > > libfdt/fdt_rw.c | 6 +++---
> > > 3 files changed, 32 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > index d89c0e0..78dbe58 100644
> > > --- a/libfdt/fdt.c
> > > +++ b/libfdt/fdt.c
> > > @@ -21,10 +21,13 @@ int32_t fdt_ro_probe_(const void *fdt)
> > >
> > > if (fdt_magic(fdt) == FDT_MAGIC) {
> > > /* Complete tree */
> > > - if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > - return -FDT_ERR_BADVERSION;
> > > - if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
> > > - return -FDT_ERR_BADVERSION;
> > > + if (!can_assume(LATEST)) {
> > > + if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > + return -FDT_ERR_BADVERSION;
> > > + if (fdt_last_comp_version(fdt) >
> > > + FDT_LAST_SUPPORTED_VERSION)
> > > + return -FDT_ERR_BADVERSION;
> > > + }
> > > } else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
> > > /* Unfinished sequential-write blob */
> > > if (fdt_size_dt_struct(fdt) == 0)
> > > @@ -72,7 +75,8 @@ size_t fdt_header_size_(uint32_t version)
> > >
> > > size_t fdt_header_size(const void *fdt)
> > > {
> > > - return fdt_header_size_(fdt_version(fdt));
> > > + return can_assume(LATEST) ? FDT_V17_SIZE :
> > > + fdt_header_size_(fdt_version(fdt));
> > > }
> > >
> > > int fdt_check_header(const void *fdt)
> > > @@ -81,11 +85,14 @@ int fdt_check_header(const void *fdt)
> > >
> > > if (fdt_magic(fdt) != FDT_MAGIC)
> > > return -FDT_ERR_BADMAGIC;
> > > - if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > - || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > > - return -FDT_ERR_BADVERSION;
> > > - if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > - return -FDT_ERR_BADVERSION;
> > > + if (!can_assume(LATEST)) {
> > > + if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > + || (fdt_last_comp_version(fdt) >
> > > + FDT_LAST_SUPPORTED_VERSION))
> > > + return -FDT_ERR_BADVERSION;
> > > + if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > + return -FDT_ERR_BADVERSION;
> > > + }
> > > hdrsize = fdt_header_size(fdt);
> > > if (!can_assume(SANE)) {
> > >
> > > @@ -101,7 +108,7 @@ int fdt_check_header(const void *fdt)
> > >
> > > if (!can_assume(SANE)) {
> > > /* Bounds check structure block */
> > > - if (fdt_version(fdt) < 17) {
> > > + if (!can_assume(LATEST) && fdt_version(fdt) < 17) {
> >
> > I wonder if we should make fdt_version() return 17 unconditionally in
> > the can_assume() case, which might make some of these tests redundant.
>
> I think that would require fdt_version() to change from being a macro,
> since we cannot check assumptions in libfdt.h without including
> libfdt_internal.h in libfdt.h
Ah.. good point.
> Is that acceptable?
Making it not a macro is fine per se, but for this you'd also need to
make it out-of-line, rather than inline in libfdt.h, and I strongly
suspect that won't be worth the cost. So, no.
--
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:
signature.asc
Description: PGP signature
