[LIBFDT] Fixup byteswapping code

[LIBFDT] Fixup byteswapping code

[Grant Likely]

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 libfdt.h |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/libfdt.h b/libfdt.h
index 3872eea..8216655 100644
--- a/libfdt.h
+++ b/libfdt.h
@@ -45,16 +45,16 @@ #define FDT_ERR_EXISTS		15
 
 #define FDT_ERR_MAX		14
 
-#define fdt_magic(fdt)			(fdt32_to_cpu(fdt)->magic)
-#define fdt_totalsize(fdt)		(fdt32_to_cpu(fdt)->totalsize)
-#define fdt_off_dt_struct(fdt)		(fdt32_to_cpu(fdt)->off_dt_struct)
-#define fdt_off_dt_strings(fdt)		(fdt32_to_cpu(fdt)->off_dt_strings)
-#define fdt_off_mem_rsvmap(fdt)		(fdt32_to_cpu(fdt)->off_mem_rsvmap)
-#define fdt_version(fdt)		(fdt32_to_cpu(fdt)->version)
-#define fdt_last_comp_version(fdt)	(fdt32_to_cpu(fdt)->last_comp_version)
-#define fdt_boot_cpuid_phys(fdt)	(fdt32_to_cpu(fdt)->boot_cpuid_phys)
-#define fdt_size_dt_strings(fdt)	(fdt32_to_cpu(fdt)->size_dt_strings)
-#define fdt_size_dt_struct(fdt)		(fdt32_to_cpu(fdt)->size_dt_struct)
+#define fdt_magic(fdt)			(fdt32_to_cpu(fdt->magic))
+#define fdt_totalsize(fdt)		(fdt32_to_cpu(fdt->totalsize))
+#define fdt_off_dt_struct(fdt)		(fdt32_to_cpu(fdt->off_dt_struct))
+#define fdt_off_dt_strings(fdt)		(fdt32_to_cpu(fdt->off_dt_strings))
+#define fdt_off_mem_rsvmap(fdt)		(fdt32_to_cpu(fdt->off_mem_rsvmap))
+#define fdt_version(fdt)		(fdt32_to_cpu(fdt->version))
+#define fdt_last_comp_version(fdt)	(fdt32_to_cpu(fdt->last_comp_version))
+#define fdt_boot_cpuid_phys(fdt)	(fdt32_to_cpu(fdt->boot_cpuid_phys))
+#define fdt_size_dt_strings(fdt)	(fdt32_to_cpu(fdt->size_dt_strings))
+#define fdt_size_dt_struct(fdt)		(fdt32_to_cpu(fdt->size_dt_struct))
 
 void *fdt_offset_ptr(const struct fdt_header *fdt, int offset, int checklen);
 
-- 
1.4.3.rc2.g0503

[LIBFDT] Add .dtb files to .gitignore

[Grant Likely]

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 .gitignore |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 4b147f1..42b3e35 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,3 +3,4 @@
 *.a
 *.so
 *~
+tests/*.dtb
-- 
1.4.3.rc2.g0503

Re: [LIBFDT] Add .dtb files to .gitignore

[Segher Boessenkool]

> diff --git a/.gitignore b/.gitignore
> index 4b147f1..42b3e35 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -3,3 +3,4 @@
>  *.a
>  *.so
>  *~
> +tests/*.dtb

You should put this in tests/.gitignore, instead.


Segher

Re: [LIBFDT] Add .dtb files to .gitignore

[David Gibson]

On Sat, Dec 02, 2006 at 10:30:59AM +0100, Segher Boessenkool wrote:
> > diff --git a/.gitignore b/.gitignore
> > index 4b147f1..42b3e35 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -3,3 +3,4 @@
> >  *.a
> >  *.so
> >  *~
> > +tests/*.dtb
> 
> You should put this in tests/.gitignore, instead.

Applied as suggested.

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

Re: [LIBFDT] Fixup byteswapping code

[David Gibson]

I just applied a patch with a whole batch of endian fixes, obsoleting
this patch of yours.

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

Re: [LIBFDT] Fixup byteswapping code

[Grant Likely]

On 12/3/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> I just applied a patch with a whole batch of endian fixes, obsoleting
> this patch of yours.

Cool, looks good and the tests pass on my machine now.

Now; here's an AMD64 related bug; consider line #44 in libfdt_internal.h:

#define PTR_ERROR(code)         (void *)(-(code))

Followed by line #67 in libfdt.h:

#define fdt_ptr_error(ptr) \
	( (((long)(ptr) < 0) && ((long)(ptr) >= -FDT_ERR_MAX)) ? -(long)(ptr) : 0 )


'code' being an integer.

This requires a hacky cast from int to pointer and back again, and it
makes the assumption that int and void* are the same size.  However,
on a x86_64 system (not sure about power64), int is 32 bits and void*
is 64.  Therefore; a return of (void*)-1 results in address
0xffffffff; not 0xffffffffffffffff.  0xffffffff is right in the middle
of valid address ranges.

This, of course, produces the warning: "cast to pointer from integer
of different size"

I believe the accepted convention (at least in the kernel) is to
return NULL on error, and libfdt should probably follow that
convention.  If you must return the actual error code; maybe rework
the API to accept a void** argument to be used to pass back the
pointer and use 'return code', or include an 'int*' argument for
passing back the err code.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [LIBFDT] Fixup byteswapping code

[David Gibson]

On Sun, Dec 03, 2006 at 10:18:25PM -0700, Grant Likely wrote:
> On 12/3/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> > I just applied a patch with a whole batch of endian fixes, obsoleting
> > this patch of yours.
> 
> Cool, looks good and the tests pass on my machine now.
> 
> Now; here's an AMD64 related bug; consider line #44 in libfdt_internal.h:
> 
> #define PTR_ERROR(code)         (void *)(-(code))
> 
> Followed by line #67 in libfdt.h:
> 
> #define fdt_ptr_error(ptr) \
> 	( (((long)(ptr) < 0) && ((long)(ptr) >= -FDT_ERR_MAX)) ? -(long)(ptr) : 0 )
> 
> 
> 'code' being an integer.
> 
> This requires a hacky cast from int to pointer and back again, and it
> makes the assumption that int and void* are the same size.  However,
> on a x86_64 system (not sure about power64), int is 32 bits and void*
> is 64.  Therefore; a return of (void*)-1 results in address
> 0xffffffff; not 0xffffffffffffffff.  0xffffffff is right in the middle
> of valid address ranges.

Urg.  Yes indeed.  The same is true on powerpc64, but userspace is
usually 32-bits on powerpc64 machines; I hadn't tested compiling
64-bit.  An extra (long) in PTR_ERROR() appears to be enough to fix
it.

> This, of course, produces the warning: "cast to pointer from integer
> of different size"
> 
> I believe the accepted convention (at least in the kernel) is to
> return NULL on error, and libfdt should probably follow that
> convention.  

Actually, there are two conventions in the kernel.  Just returning
NULL on error is the most common.  But there's a significant number of
functions that encode an errno into the returned pointer in just this
way - see the IS_ERR(), PTR_ERR() and ERR_PTR() macros.

> If you must return the actual error code; maybe rework
> the API to accept a void** argument to be used to pass back the
> pointer and use 'return code', or include an 'int*' argument for
> passing back the err code.

I thought quite a bit about how to pass back errors here.  I'm not
very fond of the current method of encoding the error codes into the
return values - it's more magic than I'm comfortable with (especially
given the multiple encoding methods for different return types) - but
I haven't thought of a better way.

Having to use a void ** for the real return value is ugly - and means
you can't stack library calls together (result of one call as
parameter to another call).  Using int * for the error code is also
ugly and all too much encouragement for people not too check it.

What I'd really prefer is an errno-style interface, where a separate
variable or call must be consulted to find the details of the error.
But, I can't see any way to do that safely in code that's supposed to
be easy to import into a wide variety of execution environments
(kernel, zImage, bootloader, userspace) - some of which could be
multithreaded.

Oh incidentally, this isn't true at present, but I'd like to make it
true:  all the library functions should check their parameters so if
they're *given* one of the encoded error values, they'll just
propagate the error out again and won't do anything nasty.  That will
make it safe to nest library calls, even if the inner ones could fail.

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

Re: [LIBFDT] Fixup byteswapping code

[Grant Likely]

On 12/3/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sun, Dec 03, 2006 at 10:18:25PM -0700, Grant Likely wrote:
> > This requires a hacky cast from int to pointer and back again, and it
> > makes the assumption that int and void* are the same size.  However,
> > on a x86_64 system (not sure about power64), int is 32 bits and void*
> > is 64.  Therefore; a return of (void*)-1 results in address
> > 0xffffffff; not 0xffffffffffffffff.  0xffffffff is right in the middle
> > of valid address ranges.
>
> Urg.  Yes indeed.  The same is true on powerpc64, but userspace is
> usually 32-bits on powerpc64 machines; I hadn't tested compiling
> 64-bit.  An extra (long) in PTR_ERROR() appears to be enough to fix
> it.
>
> > This, of course, produces the warning: "cast to pointer from integer
> > of different size"
> >
> > I believe the accepted convention (at least in the kernel) is to
> > return NULL on error, and libfdt should probably follow that
> > convention.
>
> Actually, there are two conventions in the kernel.  Just returning
> NULL on error is the most common.  But there's a significant number of
> functions that encode an errno into the returned pointer in just this
> way - see the IS_ERR(), PTR_ERR() and ERR_PTR() macros.

Ah, okay, I see that code now.

Hmmm....

Gah, this still makes me really nervous.  The hard part will be how to
make it *blatently* obvious to users that checking for NULL is *not*
sufficient for error checking when using these functions.  Otherwise a
very common code defect will be code using just a NULL check before
dereferencing the returned pointer.

>
> > If you must return the actual error code; maybe rework
> > the API to accept a void** argument to be used to pass back the
> > pointer and use 'return code', or include an 'int*' argument for
> > passing back the err code.
>
> I thought quite a bit about how to pass back errors here.  I'm not
> very fond of the current method of encoding the error codes into the
> return values - it's more magic than I'm comfortable with (especially
> given the multiple encoding methods for different return types) - but
> I haven't thought of a better way.

heh; Isn't beating your head against the limits of a language fun?  I
wish I had a better suggestion myself, but I understand your point and
the difficulty involved.

>
> Having to use a void ** for the real return value is ugly - and means
> you can't stack library calls together (result of one call as
> parameter to another call).  Using int * for the error code is also
> ugly and all too much encouragement for people not too check it.

Yeah, I agree 100%.

>
> What I'd really prefer is an errno-style interface, where a separate
> variable or call must be consulted to find the details of the error.
> But, I can't see any way to do that safely in code that's supposed to
> be easy to import into a wide variety of execution environments
> (kernel, zImage, bootloader, userspace) - some of which could be
> multithreaded.

Unfortunately, there is no simple way to keep around execution
context.  newlib uses a scheme where the exec environment has to
specifically tell it which thread context to use, but it's a bit ugly
in that hooks must be added to the thread scheduler to change the
context pointer on every schedule.

> Oh incidentally, this isn't true at present, but I'd like to make it
> true:  all the library functions should check their parameters so if
> they're *given* one of the encoded error values, they'll just
> propagate the error out again and won't do anything nasty.  That will
> make it safe to nest library calls, even if the inner ones could fail.

good idea

Cheers,
g.
-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [LIBFDT] Fixup byteswapping code

[David Gibson]

On Sun, Dec 03, 2006 at 11:33:42PM -0700, Grant Likely wrote:
> On 12/3/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Sun, Dec 03, 2006 at 10:18:25PM -0700, Grant Likely wrote:
> > > This requires a hacky cast from int to pointer and back again, and it
> > > makes the assumption that int and void* are the same size.  However,
> > > on a x86_64 system (not sure about power64), int is 32 bits and void*
> > > is 64.  Therefore; a return of (void*)-1 results in address
> > > 0xffffffff; not 0xffffffffffffffff.  0xffffffff is right in the middle
> > > of valid address ranges.
> >
> > Urg.  Yes indeed.  The same is true on powerpc64, but userspace is
> > usually 32-bits on powerpc64 machines; I hadn't tested compiling
> > 64-bit.  An extra (long) in PTR_ERROR() appears to be enough to fix
> > it.
> >
> > > This, of course, produces the warning: "cast to pointer from integer
> > > of different size"
> > >
> > > I believe the accepted convention (at least in the kernel) is to
> > > return NULL on error, and libfdt should probably follow that
> > > convention.
> >
> > Actually, there are two conventions in the kernel.  Just returning
> > NULL on error is the most common.  But there's a significant number of
> > functions that encode an errno into the returned pointer in just this
> > way - see the IS_ERR(), PTR_ERR() and ERR_PTR() macros.
> 
> Ah, okay, I see that code now.
> 
> Hmmm....
> 
> Gah, this still makes me really nervous.  The hard part will be how to
> make it *blatently* obvious to users that checking for NULL is *not*
> sufficient for error checking when using these functions.  Otherwise a
> very common code defect will be code using just a NULL check before
> dereferencing the returned pointer.

Exactly.  It makes me very nervous, too.

> > > If you must return the actual error code; maybe rework
> > > the API to accept a void** argument to be used to pass back the
> > > pointer and use 'return code', or include an 'int*' argument for
> > > passing back the err code.
> >
> > I thought quite a bit about how to pass back errors here.  I'm not
> > very fond of the current method of encoding the error codes into the
> > return values - it's more magic than I'm comfortable with (especially
> > given the multiple encoding methods for different return types) - but
> > I haven't thought of a better way.
> 
> heh; Isn't beating your head against the limits of a language fun?  I
> wish I had a better suggestion myself, but I understand your point and
> the difficulty involved.
> 
> > Having to use a void ** for the real return value is ugly - and means
> > you can't stack library calls together (result of one call as
> > parameter to another call).  Using int * for the error code is also
> > ugly and all too much encouragement for people not too check it.
> 
> Yeah, I agree 100%.
> 
> > What I'd really prefer is an errno-style interface, where a separate
> > variable or call must be consulted to find the details of the error.
> > But, I can't see any way to do that safely in code that's supposed to
> > be easy to import into a wide variety of execution environments
> > (kernel, zImage, bootloader, userspace) - some of which could be
> > multithreaded.
> 
> Unfortunately, there is no simple way to keep around execution
> context.  newlib uses a scheme where the exec environment has to
> specifically tell it which thread context to use, but it's a bit ugly
> in that hooks must be added to the thread scheduler to change the
> context pointer on every schedule.

Sure, and if I set up something to use the newlib scheme, it would
break in things using a different approach.

> > Oh incidentally, this isn't true at present, but I'd like to make it
> > true:  all the library functions should check their parameters so if
> > they're *given* one of the encoded error values, they'll just
> > propagate the error out again and won't do anything nasty.  That will
> > make it safe to nest library calls, even if the inner ones could fail.
> 
> good idea

I do have one idea for better error handling: I'd like to put in
something defined in the the libfdt_env.h (which is designed to be
replaced when libfdt is compiled in different environments) to report
an error.  That would be invoked every time a libfdt function exits
with an error - I would envisage it calling through to a user supplied
function pointer.

In many practical situations, I think most libfdt errors would be the
result of something unrecoverable problem (such as a corrupted device
tree blob), the callback could just print an error (by whatever means
is appropritate for the environment) and bail out, avoiding the need
for checking return values.  At least for the "serious" errors
(BADMAGIC, BADSTRUCTURE and so forth).  NOTFOUND (and maybe TRUNCATED)
would need to be passed back as a return value still, since that can
certainly occur in a legitimate test.

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

Re: [LIBFDT] Fixup byteswapping code

[Grant Likely]

On 12/4/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> I do have one idea for better error handling: I'd like to put in
> something defined in the the libfdt_env.h (which is designed to be
> replaced when libfdt is compiled in different environments) to report
> an error.  That would be invoked every time a libfdt function exits
> with an error - I would envisage it calling through to a user supplied
> function pointer.
>
> In many practical situations, I think most libfdt errors would be the
> result of something unrecoverable problem (such as a corrupted device
> tree blob), the callback could just print an error (by whatever means
> is appropritate for the environment) and bail out, avoiding the need
> for checking return values.  At least for the "serious" errors
> (BADMAGIC, BADSTRUCTURE and so forth).  NOTFOUND (and maybe TRUNCATED)
> would need to be passed back as a return value still, since that can
> certainly occur in a legitimate test.

Yeah, that's not a bad idea.  That way the complexity of making it
thread safe is left with the execution environment, not with libfdt,
and it still leaves the possibility of application code retrieving the
error code if it is interested....  Still not ideal, but of the
options presented, I like this one best.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [LIBFDT] Fixup byteswapping code

[David Gibson]

On Mon, Dec 04, 2006 at 07:55:59AM -0700, Grant Likely wrote:
> On 12/4/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> > I do have one idea for better error handling: I'd like to put in
> > something defined in the the libfdt_env.h (which is designed to be
> > replaced when libfdt is compiled in different environments) to report
> > an error.  That would be invoked every time a libfdt function exits
> > with an error - I would envisage it calling through to a user supplied
> > function pointer.
> >
> > In many practical situations, I think most libfdt errors would be the
> > result of something unrecoverable problem (such as a corrupted device
> > tree blob), the callback could just print an error (by whatever means
> > is appropritate for the environment) and bail out, avoiding the need
> > for checking return values.  At least for the "serious" errors
> > (BADMAGIC, BADSTRUCTURE and so forth).  NOTFOUND (and maybe TRUNCATED)
> > would need to be passed back as a return value still, since that can
> > certainly occur in a legitimate test.
> 
> Yeah, that's not a bad idea.  That way the complexity of making it
> thread safe is left with the execution environment, not with libfdt,
> and it still leaves the possibility of application code retrieving the
> error code if it is interested....  Still not ideal, but of the
> options presented, I like this one best.

Actually, I had some more thought about this.  There really aren't
that many functions returning pointers there's:
	_fdt_getprop()	(which I'm thinking about renaming and making 
			 externally visible)
	fdt_getprop()
	fdt_create()
	fdt_open_into()
	fdt_pack()
	fdt_move()

fdt_pack() doesn't actually move anything, so it could just return an
error code instead.

fdt_open_into(), fdt_create() and fdt_move() all return a device tree
at the beginning of the given buffer.  So, they too could return just
an error code instead.  That's a little ugly from the user's POV
because they'd have to manually cast the (void *) buffer pointer into
(struct fdt_header *).  However, I was thinking in any case about
changing all the fdt parameters to just be void * and doing the casts
inside the library, which removes that problem.  In addition it
re-emphasises the idea of the blob as a blob, whose internal structure
the caller need not understand.

That leaves _fdt_getprop() and fdt_getprop().  Those both take an int
pointer to return the length.  So, I was thinking they could instead
return NULL for errors, and store the detailed error code in the
length parameter:  both structure offsets and lengths have the nice
property that negative values are "naturally" out-of-bounds.

What do you think?

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

Re: [LIBFDT] Fixup byteswapping code

[David Gibson]

On Fri, Dec 08, 2006 at 05:09:37PM +1100, David Gibson wrote:
> On Mon, Dec 04, 2006 at 07:55:59AM -0700, Grant Likely wrote:
> > On 12/4/06, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > I do have one idea for better error handling: I'd like to put in
> > > something defined in the the libfdt_env.h (which is designed to be
> > > replaced when libfdt is compiled in different environments) to report
> > > an error.  That would be invoked every time a libfdt function exits
> > > with an error - I would envisage it calling through to a user supplied
> > > function pointer.
> > >
> > > In many practical situations, I think most libfdt errors would be the
> > > result of something unrecoverable problem (such as a corrupted device
> > > tree blob), the callback could just print an error (by whatever means
> > > is appropritate for the environment) and bail out, avoiding the need
> > > for checking return values.  At least for the "serious" errors
> > > (BADMAGIC, BADSTRUCTURE and so forth).  NOTFOUND (and maybe TRUNCATED)
> > > would need to be passed back as a return value still, since that can
> > > certainly occur in a legitimate test.
> > 
> > Yeah, that's not a bad idea.  That way the complexity of making it
> > thread safe is left with the execution environment, not with libfdt,
> > and it still leaves the possibility of application code retrieving the
> > error code if it is interested....  Still not ideal, but of the
> > options presented, I like this one best.
> 
> Actually, I had some more thought about this.  There really aren't
> that many functions returning pointers there's:

And I just pushed up a batch of patches which cleans up the errorr
handling as suggested there.

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