[U-Boot] [PATCH 2/3] libfdt: make fdt_increase_size() available to everyone

[U-Boot] [PATCH 2/3] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

The function fdt_increase_size() increases the size of the device tree by the
given amount.  This is useful for any code that wants to add a node or large
property, to avoid the possibility of running out of space.  It's much smarter
to have U-Boot increase the size of device tree when it knows it's going to
add data, instead of hoping that the DTS was compiled with the right -p value.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

 common/fdt_support.c  |   20 ++++++++++----------
 include/fdt_support.h |    1 +
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index b6f252a..01d635b 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -645,6 +645,16 @@ int fdt_resize(void *blob)
 	return actualsize;
 }
 
+int fdt_increase_size(void *fdt, int add_len)
+{
+	int newlen;
+
+	newlen = fdt_totalsize(fdt) + add_len;
+
+	/* Open in place with a new len */
+	return fdt_open_into(fdt, fdt, newlen);
+}
+
 #ifdef CONFIG_PCI
 #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
 
@@ -792,16 +802,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset)
 	return 0;
 }
 
-int fdt_increase_size(void *fdt, int add_len)
-{
-	int newlen;
-
-	newlen = fdt_totalsize(fdt) + add_len;
-
-	/* Open in place with a new len */
-	return fdt_open_into(fdt, fdt, newlen);
-}
-
 int fdt_del_partitions(void *blob, int parent_offset)
 {
 	const void *prop;
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 9a453af..d70627d 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
 
 void set_working_fdt_addr(void *addr);
 int fdt_resize(void *blob);
+int fdt_increase_size(void *fdt, int add_len);
 
 int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
 
-- 
1.6.5

[U-Boot] [PATCH 2/3] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

On Mon, May 10, 2010 at 4:26 PM, Timur Tabi <timur@freescale.com> wrote:
> The function fdt_increase_size() increases the size of the device tree by the
> given amount. ?This is useful for any code that wants to add a node or large
> property, to avoid the possibility of running out of space. ?It's much smarter
> to have U-Boot increase the size of device tree when it knows it's going to
> add data, instead of hoping that the DTS was compiled with the right -p value.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>

Please ignore the "2/3".  This patch is not part of a series.

Wolfgang, we have code (not yet ready to be published) that needs this
change.  Please apply if you don't have any objections.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/3] libfdt: make fdt_increase_size() available to everyone

[Kumar Gala]

On May 10, 2010, at 4:26 PM, Timur Tabi wrote:

> The function fdt_increase_size() increases the size of the device tree by the
> given amount.  This is useful for any code that wants to add a node or large
> property, to avoid the possibility of running out of space.  It's much smarter
> to have U-Boot increase the size of device tree when it knows it's going to
> add data, instead of hoping that the DTS was compiled with the right -p value.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---

Jerry, any comments on this?

- k

> 
> common/fdt_support.c  |   20 ++++++++++----------
> include/fdt_support.h |    1 +
> 2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index b6f252a..01d635b 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -645,6 +645,16 @@ int fdt_resize(void *blob)
> 	return actualsize;
> }
> 
> +int fdt_increase_size(void *fdt, int add_len)
> +{
> +	int newlen;
> +
> +	newlen = fdt_totalsize(fdt) + add_len;
> +
> +	/* Open in place with a new len */
> +	return fdt_open_into(fdt, fdt, newlen);
> +}
> +
> #ifdef CONFIG_PCI
> #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
> 
> @@ -792,16 +802,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset)
> 	return 0;
> }
> 
> -int fdt_increase_size(void *fdt, int add_len)
> -{
> -	int newlen;
> -
> -	newlen = fdt_totalsize(fdt) + add_len;
> -
> -	/* Open in place with a new len */
> -	return fdt_open_into(fdt, fdt, newlen);
> -}
> -
> int fdt_del_partitions(void *blob, int parent_offset)
> {
> 	const void *prop;
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 9a453af..d70627d 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
> 
> void set_working_fdt_addr(void *addr);
> int fdt_resize(void *blob);
> +int fdt_increase_size(void *fdt, int add_len);
> 
> int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
> 
> -- 
> 1.6.5
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Gerald Van Baren]

The function fdt_increase_size() increases the size of the device tree by
the given amount.  This is useful for any code that wants to add a node
or large property, to avoid the possibility of running out of space.
It's much smarter to have U-Boot increase the size of device tree when
it knows it's going to add data, instead of hoping that the DTS was
compiled with the right -p value.

Improve the size increase algorithm by malloc()ing the necessary space.
This avoids potential problems with the FDT blob overwriting things that
follow it in memory.

Signed-off-by: Timur Tabi <timur@freescale.com>
Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>
---

Hi Timur, Kumar,

This had me scratching my head briefly.  I first thought you were
creating a new function, but you just exposed the existing function in
fdt_support.h and moved it to its logical position, given its new
status.

The code has one pre-existing weakness that bothers me: if there is
something following the FDT blob, it will get overwritten by the
increased blob.  One way around this would be to malloc() a new memory
space and move and expand the blob to the new space.  The first time
this was done, the original blob should not be free()ed since it wasn't
malloc()ed, but the second and subsequent times it should be free()ed.

I've added this to your patch, but have *NOT* execution tested it.  Does
this addition (a) make sense and (b) work?

Thanks,
gvb

 common/fdt_support.c  |   35 +++++++++++++++++++++++++----------
 include/fdt_support.h |    1 +
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index b6f252a..6bd03d6 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -645,6 +645,31 @@ int fdt_resize(void *blob)
 	return actualsize;
 }
 
+/* Keep track of malloc()ed blobs for freeing */
+static void *fdt_malloc_blob = 0;
+
+int fdt_increase_size(void *fdt, int add_len)
+{
+	int newlen;
+	void *newblob;
+	int ret;
+
+	newlen = fdt_totalsize(fdt) + add_len;
+	newblob = malloc(newlen);
+
+	/* Copy the blob and expand it */
+	ret = fdt_open_into(fdt, newblob, newlen);
+	if (!ret) {
+		free(newblob);
+		return ret;
+	}
+
+	if (!fdt_malloc_blob)
+		free(fdt_malloc_blob);
+	fdt_malloc_blob = newblob;
+	return 0;
+}
+
 #ifdef CONFIG_PCI
 #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
 
@@ -792,16 +817,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset)
 	return 0;
 }
 
-int fdt_increase_size(void *fdt, int add_len)
-{
-	int newlen;
-
-	newlen = fdt_totalsize(fdt) + add_len;
-
-	/* Open in place with a new len */
-	return fdt_open_into(fdt, fdt, newlen);
-}
-
 int fdt_del_partitions(void *blob, int parent_offset)
 {
 	const void *prop;
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 9a453af..d70627d 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
 
 void set_working_fdt_addr(void *addr);
 int fdt_resize(void *blob);
+int fdt_increase_size(void *fdt, int add_len);
 
 int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
 
-- 
1.6.3.3

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

On Sat, May 15, 2010 at 9:11 PM, Gerald Van Baren <gvb.uboot@gmail.com> wrote:

> The code has one pre-existing weakness that bothers me: if there is
> something following the FDT blob, it will get overwritten by the
> increased blob. ?One way around this would be to malloc() a new memory
> space and move and expand the blob to the new space. ?The first time
> this was done, the original blob should not be free()ed since it wasn't
> malloc()ed, but the second and subsequent times it should be free()ed.

But then how does the caller know where the new blob is?  When I call
fdt_increase_size(), I pass it the address of a blob that I'm
modifying.  After the function returns, my value of 'fdt' is no longer
valid.

> I've added this to your patch, but have *NOT* execution tested it. ?Does
> this addition (a) make sense and (b) work?

I was expecting the caller of fdt_increase_size() to know that the
space after the fdt is available.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Jerry Van Baren]

Timur Tabi wrote:
> On Sat, May 15, 2010 at 9:11 PM, Gerald Van Baren <gvb.uboot@gmail.com> wrote:
> 
>> The code has one pre-existing weakness that bothers me: if there is
>> something following the FDT blob, it will get overwritten by the
>> increased blob.  One way around this would be to malloc() a new memory
>> space and move and expand the blob to the new space.  The first time
>> this was done, the original blob should not be free()ed since it wasn't
>> malloc()ed, but the second and subsequent times it should be free()ed.
> 
> But then how does the caller know where the new blob is?  When I call
> fdt_increase_size(), I pass it the address of a blob that I'm
> modifying.  After the function returns, my value of 'fdt' is no longer
> valid.

Oops, yes, that is a pretty fatal flaw.

>> I've added this to your patch, but have *NOT* execution tested it.  Does
>> this addition (a) make sense and (b) work?
> 
> I was expecting the caller of fdt_increase_size() to know that the
> space after the fdt is available.

OK.  I'll <acked-by> the original patch.  The best way to incorporate 
the patch would be for Kumar (I assume) to pick up the libfdt change 
with the Freescale change that needs it so that the changes are 
inherently coordinated.

gvb

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <AANLkTilPrgpUwL630lzSmzKt5bBXFiq6zDPIiR3pQcwa@mail.gmail.com> you wrote:
>
> > I've added this to your patch, but have *NOT* execution tested it. Does
> > this addition (a) make sense and (b) work?
> 
> I was expecting the caller of fdt_increase_size() to know that the
> space after the fdt is available.

So where is the code that makes sure that there is sufficient space
available?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
At the source of every error which is blamed on the computer you will
find at least two human errors, including the error of blaming it  on
the computer.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:
>> > I was expecting the caller of fdt_increase_size() to know that the
>> > space after the fdt is available.

> So where is the code that makes sure that there is sufficient space
> available?

Well, if I load the fdt to address c0000, and it's 10KB in size, I'm pretty
sure I can expand it to 16KB if I know that there's nothing else between
c0000 and c4000.

If you're asking about what code do I have that actually calls
fdt_increase_size(), that code is currently used for a new board that we're
not yet ready to publish.  I'd like my patch for fdt_increase_size()
included in the next merge window, so that when rebase our internal
repository on the next release, this patch will already be available.  It
makes our internal development easier.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BF14FBF.3040900@freescale.com> you wrote:
>
> > So where is the code that makes sure that there is sufficient space
> > available?
> 
> Well, if I load the fdt to address c0000, and it's 10KB in size, I'm pretty
> sure I can expand it to 16KB if I know that there's nothing else between
> c0000 and c4000.

Assume the case that the DTB is stored in NOR flash, and I pass the
NOR flash address to the bootm command...

I'm not sure if there is any guarantee for free room behind the DTB in
this case.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
... not meant for the weak-at-heart: /^(?=.*one)(?=.*two)/
If you can follow that, you can use it.
          - Randal L. Schwartz in <ukpw67rhl3.fsf@julie.teleport.com>

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

> Assume the case that the DTB is stored in NOR flash, and I pass the
> NOR flash address to the bootm command...
> 
> I'm not sure if there is any guarantee for free room behind the DTB in
> this case.

We can never guarantee this.  The code that calls fdt_increase_size() will
just have to ensure that there is enough room.

If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via
lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra
room is needed.

So it appear to me that there are two ways the fdt is "allocated" in memory:

1) It is copied to a location allocated via lmb_alloc_base() by
boot_relocate_fdt().

2) It is simply placed somewhere in memory (via tftp) and left there.

I don't believe we ever use malloc() to allocate the memory for the fdt, so
these two should cover 100% of all cases.

So for case #1, we can use CONFIG_SYS_FDT_PAD.  For case #2, we just have to
assume that when the user did "tftp c0000 my.dtb", that he knows what he's
doing.

I assume that fdt_increase_size() will only be used to increase the
available space by a significant amount.  One example of this (and the
reason I posted this patch in the first place), is to embed firmware inside
the device tree.  A new binding for Freescale QE firmware allows for this,
and I have code in-house which implements this.

So I say that a particular board will know whether it needs to significantly
expand the available amount of RAM beyond the default CONFIG_SYS_FDT_PAD.
Therefore, we can define a new value of CONFIG_SYS_FDT_PAD in the board
header files for those boards that need it.

I believe that this should cover everything, and that my patch is okay and
should be merged in.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BF29FE9.1070305@freescale.com> you wrote:
> Wolfgang Denk wrote:
> 
> > Assume the case that the DTB is stored in NOR flash, and I pass the
> > NOR flash address to the bootm command...
> > 
> > I'm not sure if there is any guarantee for free room behind the DTB in
> > this case.
> 
> We can never guarantee this.  The code that calls fdt_increase_size() will
> just have to ensure that there is enough room.

Such an "ensure that there is enough room" is exactly what I'm asking
for.

> If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via
> lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra
> room is needed.

Hm... it seems that not a single board uses this setting, which also
happens to be completely undocumented.

> So it appear to me that there are two ways the fdt is "allocated" in memory:
> 
> 1) It is copied to a location allocated via lmb_alloc_base() by
> boot_relocate_fdt().
> 
> 2) It is simply placed somewhere in memory (via tftp) and left there.
> 
> I don't believe we ever use malloc() to allocate the memory for the fdt, so
> these two should cover 100% of all cases.

For me lmb_alloc_base() is just another form of malloc()...

> So for case #1, we can use CONFIG_SYS_FDT_PAD.  For case #2, we just have to
> assume that when the user did "tftp c0000 my.dtb", that he knows what he's
> doing.

I'm not sure if we have the same intentions here.

I think, that for case #1 the available size is known, so we can check
if we exceed the limits. And this is what we should do then.

> I assume that fdt_increase_size() will only be used to increase the
> available space by a significant amount.  One example of this (and the
> reason I posted this patch in the first place), is to embed firmware inside
> the device tree.  A new binding for Freescale QE firmware allows for this,
> and I have code in-house which implements this.

If we are talking about such substantial increments then it is all
the more important to check for available room.

> So I say that a particular board will know whether it needs to significantly
> expand the available amount of RAM beyond the default CONFIG_SYS_FDT_PAD.
> Therefore, we can define a new value of CONFIG_SYS_FDT_PAD in the board
> header files for those boards that need it.

No. We should check if the programmed value is sufficient.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If in any problem you find yourself doing an immense amount of  work,
the answer can be obtained by simple inspection.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

>> We can never guarantee this.  The code that calls fdt_increase_size() will
>> just have to ensure that there is enough room.
> 
> Such an "ensure that there is enough room" is exactly what I'm asking
> for.

Maybe I don't understand what you're getting at.  My point is that, whenever
someone writes code that calls fdt_increase_size(), *that* person needs to
provide the assurance, not me.  Within fdt_increase_size(), there is no way
to ensure anything.  And since my patch only deals with fdt_increase_size()
itself, I don't understand what you want from *me* within the context of
*this* patch.

>> If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via
>> lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra
>> room is needed.
> 
> Hm... it seems that not a single board uses this setting, 

That's because the default has been sufficient so far.

> which also
> happens to be completely undocumented.

That's got nothing to do with me.  I didn't write the code that uses
CONFIG_SYS_FDT_PAD.

> For me lmb_alloc_base() is just another form of malloc()...

True, but it's not the same as malloc().  For example, I cannot use
realloc() to make the allocation bigger.  If all fdts were allocated via
malloc(), then I could modify fdt_increase_size() to call realloc().

>> So for case #1, we can use CONFIG_SYS_FDT_PAD.  For case #2, we just have to
>> assume that when the user did "tftp c0000 my.dtb", that he knows what he's
>> doing.
> 
> I'm not sure if we have the same intentions here.
> 
> I think, that for case #1 the available size is known, so we can check
> if we exceed the limits. And this is what we should do then.

But within fdt_increase_size(), how do I know if the memory is allocated via
lmb_alloc_base()?  The heap management data structure for an lmb is managed
external to the heap itself.

>> I assume that fdt_increase_size() will only be used to increase the
>> available space by a significant amount.  One example of this (and the
>> reason I posted this patch in the first place), is to embed firmware inside
>> the device tree.  A new binding for Freescale QE firmware allows for this,
>> and I have code in-house which implements this.
> 
> If we are talking about such substantial increments then it is all
> the more important to check for available room.

And again, the point *I* am trying to make is that it's okay to put the onus
of that check on the *caller* of fdt_increase_size(), and not on
fdt_increase_size() itself.

>> So I say that a particular board will know whether it needs to significantly
>> expand the available amount of RAM beyond the default CONFIG_SYS_FDT_PAD.
>> Therefore, we can define a new value of CONFIG_SYS_FDT_PAD in the board
>> header files for those boards that need it.
> 
> No. We should check if the programmed value is sufficient.

But that is only meaningful if the fdt is allocated via an lmb, which is not
true in case #2.  In case #2, there is no allocation of memory, so there's
no way to know within fdt_increase_size()!

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BF2B302.2030909@freescale.com> you wrote:
> 
> >> We can never guarantee this.  The code that calls fdt_increase_size() will
> >> just have to ensure that there is enough room.
> > 
> > Such an "ensure that there is enough room" is exactly what I'm asking
> > for.
> 
> Maybe I don't understand what you're getting at.  My point is that, whenever
> someone writes code that calls fdt_increase_size(), *that* person needs to
> provide the assurance, not me.  Within fdt_increase_size(), there is no way
> to ensure anything.  And since my patch only deals with fdt_increase_size()
> itself, I don't understand what you want from *me* within the context of
> *this* patch.

Your problem is that you are too much focussed on "your patch" only.
You need to keep your eyes open for the bigger whole.  Your patch has
the potential of causing nasty crashes, and I ask you to prevent this.
This may require changes outside the current scope of "your patch".

> >> If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via
> >> lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra
> >> room is needed.
> > 
> > Hm... it seems that not a single board uses this setting, 
> 
> That's because the default has been sufficient so far.

Right. But your patch is supposed to change this.

> > which also
> > happens to be completely undocumented.
> 
> That's got nothing to do with me.  I didn't write the code that uses
> CONFIG_SYS_FDT_PAD.

No. But you are trying to get code into mainline where the current
code is insufficient. So you also got to extend the code to make
yourpatch acceptable.

> > I think, that for case #1 the available size is known, so we can check
> > if we exceed the limits. And this is what we should do then.
> 
> But within fdt_increase_size(), how do I know if the memory is allocated via
> lmb_alloc_base()?  The heap management data structure for an lmb is managed
> external to the heap itself.

If you don't know this in fdt_increase_size(), then either make it
known there (for example by extending other code to pass on such
information), or add such checking to other parts of the code in the
call chain.

> > If we are talking about such substantial increments then it is all
> > the more important to check for available room.
> 
> And again, the point *I* am trying to make is that it's okay to put the onus
> of that check on the *caller* of fdt_increase_size(), and not on
> fdt_increase_size() itself.

OK. I have no problem with that. I am just missing this other part of
the required changes in your patch.

> > No. We should check if the programmed value is sufficient.
> 
> But that is only meaningful if the fdt is allocated via an lmb, which is not
> true in case #2.  In case #2, there is no allocation of memory, so there's
> no way to know within fdt_increase_size()!

Maybe. But there is still case #1, where we have a real problem, and I
will not accept your patch without seing this problem properly
addressed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Do not underestimate the value of print statements for debugging.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

On Tue, May 18, 2010 at 5:20 PM, Wolfgang Denk <wd@denx.de> wrote:

>> And again, the point *I* am trying to make is that it's okay to put the onus
>> of that check on the *caller* of fdt_increase_size(), and not on
>> fdt_increase_size() itself.
>
> OK. I have no problem with that. I am just missing this other part of
> the required changes in your patch.

I'm not ready to submit any code that calls fdt_increase_size() yet.
I'm just trying to create the infrastructure here so that I can be
sure that my in-house code is correct.  If this patch is rejected
because there's a better way to increase the size of an fdt, then I
need to know that *now*.

>> But that is only meaningful if the fdt is allocated via an lmb, which is not
>> true in case #2. ?In case #2, there is no allocation of memory, so there's
>> no way to know within fdt_increase_size()!
>
> Maybe. But there is still case #1, where we have a real problem, and I
> will not accept your patch without seing this problem properly
> addressed.

And I said that we'll handle this by setting a new value of
CONFIG_SYS_FDT_PAD.  This will ensure that the initial memory
allocation is large enough.

Technically speaking, even without my patch we already have the
problem that bothers you.  When boot_relocate_fdt() allocates the LMB,
it gives it an additional buffer of 12KB.  There's nothing stopping
some code from calling fdt_setprop() and/or fdt_add_subnode()  a
thousand times, which will cause the fdt to grow outside of the
allocated area.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <AANLkTimFF-MM8jLicndervkWwRVpMQEeEuVd_RNTMxrY@mail.gmail.com> you wrote:
> 
> >> And again, the point *I* am trying to make is that it's okay to put the onus
> >> of that check on the *caller* of fdt_increase_size(), and not on
> >> fdt_increase_size() itself.
> >
> > OK. I have no problem with that. I am just missing this other part of
> > the required changes in your patch.
>
> I'm not ready to submit any code that calls fdt_increase_size() yet.
> I'm just trying to create the infrastructure here so that I can be
> sure that my in-house code is correct.  If this patch is rejected
> because there's a better way to increase the size of an fdt, then I
> need to know that *now*.

I reject your patch because it introduces the risk of crashing the
system and it appears you do not want to be bothered fixing this.

> > Maybe. But there is still case #1, where we have a real problem, and I
> > will not accept your patch without seing this problem properly
> > addressed.
>
> And I said that we'll handle this by setting a new value of
> CONFIG_SYS_FDT_PAD.  This will ensure that the initial memory
> allocation is large enough.

And I said that this is not sufficient. Static buffers have always
been a bad idea as there will always be a user who needs more space.
In this case, where we know the amount needed, we can as well arrange
for it. You may have to add some code to implemenmt this, butthen
adding this code has to be part of your patch submission.

> Technically speaking, even without my patch we already have the
> problem that bothers you.  When boot_relocate_fdt() allocates the LMB,
> it gives it an additional buffer of 12KB.  There's nothing stopping
> some code from calling fdt_setprop() and/or fdt_add_subnode()  a
> thousand times, which will cause the fdt to grow outside of the
> allocated area.

Right. We don't provide safety belts against doing stupid things. But
you mentioned your extension is needed to insert firmware blobs which
can take "significant size" - you cannot estimate in advace (at
compile time) which size the end user may need. So using a static
buffer size is a stupid thing too do as is is bound to fail rather
sooner than later.

I will not accept code that relies on such compile time assumptions
about dynamically inserted data.

[Note that it does not help that *you* *shout* *at* *me* all the time
that you need this *now*. This does not make your patch any better.]


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If ignorance is bliss, why aren't there more happy people?

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

> I reject your patch because it introduces the risk of crashing the
> system and it appears you do not want to be bothered fixing this.

But I believe I have already fixed it by stating that any users of
fdt_increase_size() must ensure that the new size fits in the allocated
area.  The same rules apply to functions like strcat(), so I don't
understand why you are so concerned.

> And I said that this is not sufficient. Static buffers have always
> been a bad idea as there will always be a user who needs more space.
> In this case, where we know the amount needed, we can as well arrange
> for it. You may have to add some code to implemenmt this, butthen
> adding this code has to be part of your patch submission.

But there is no way to ensure that every user of fdt_increase_size() will
follow those rules.

I could, for instance, add an lmb parameter to fdt_increase_size(), but this
will only apply in instances where the fdt exists inside an lmb. There is no
lmb_realloc() function, so the most I can do is return an error if the lmb
isn't big enough.

The problem is that by the time the code needs to resize the fdt, it has
long since forgotten about the lmb.  We would need to modify
ft_board_setup() to take an lmb as a parameter, but that would require
changes to almost 30 boards!

And then, that doesn't help the situation where the fdt is just dumped in
memory somewhere, like this:

tftp 0xc0000 my.dtb
bootm xxx xxx 0xc0000

In this situation, no lmb is created, so there's no way to know if there's
anything at address 0xc4000 that could be overwritten.

So the best I can do is this:

int fdt_increase_size(struct lmb *lmb, void *fdt, int add_len)
{
	int newlen;

	newlen = fdt_totalsize(fdt) + add_len;

	if (lmb) {
		int lmb_size = lmb_size(lmb);

		if (newlen > lmb_size)
			return -ENOMEM;
	}

	/* Open in place with a new len */
	return fdt_open_into(fdt, fdt, newlen);
}

So if an lmb is provided, we can check for overwrites, but we can't require
the lmb.

The only problem with the above function is that lmb_size() doesn't exist,
and I'm not sure how to implement it yet.  There doesn't appear to be any
useful support functions for the lmb code at the moment.

> Right. We don't provide safety belts against doing stupid things. But
> you mentioned your extension is needed to insert firmware blobs which
> can take "significant size" - you cannot estimate in advace (at
> compile time) which size the end user may need. 

Actually, I can.  For firmware, at least, I have macro called
CONFIG_SYS_FSL_FW_MAXSIZE that defines the largest size that firmware can
be.  This is used not only to specify the new size of the lmb (if
applicable), but to also ensure limits on the firmware itself.  This would
be a board-specific setting, since each board knows what kind of firmware it
needs.  And then I have this:

#ifndef CONFIG_SYS_FDT_PAD_DFLT
#define CONFIG_SYS_FDT_PAD_DFLT 0x3000
#endif

#ifndef CONFIG_SYS_FMAN_FW_MAXSIZE
#define CONFIG_SYS_FMAN_FW_MAXSIZE 0
#endif

#ifndef CONFIG_SYS_FDT_PAD
#define CONFIG_SYS_FDT_PAD (CONFIG_SYS_FDT_PAD_DFLT +
CONFIG_SYS_FMAN_FW_MAXSIZE)
#endif

Thus, each board can define its own settings, and boot_relocate_fdt() will
honor them.

> So using a static
> buffer size is a stupid thing too do as is is bound to fail rather
> sooner than later.

AFAIK, lmb regions cannot be resized, so once we create an lmb region for
the fdt, we're stuck with it.  That's why I've enhanced boot_relocate_fdt()
to ensure there's enough room in the lmb region to hold the firmware.  I
just haven't posted *that* code yet, since it's part of the P4080DS board
support that isn't ready yet.

> I will not accept code that relies on such compile time assumptions
> about dynamically inserted data.

I don't see any way around that.

The firmware is inserted after the lmb is created, and the code that creates
the lmb region has no knowledge of the firmware (or any other board-specific
entities that are going to be inserted into the fdt).  The best I can do is
force the firmware to be smaller than a compile-time limit, and ensure the
lmb region is expanded by that value.

> [Note that it does not help that *you* *shout* *at* *me* all the time
> that you need this *now*. This does not make your patch any better.]

Shouting is done with ALL CAPS.  Framing words with asterisks is used to
designate emphasis.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BF3FC6D.4000605@freescale.com> you wrote:
> 
> But I believe I have already fixed it by stating that any users of
> fdt_increase_size() must ensure that the new size fits in the allocated
> area.  The same rules apply to functions like strcat(), so I don't
> understand why you are so concerned.

I think the places where strcat() is used actually try and make sure
to have enough room in the target buffer; also, almost all of themn
appent static strigs with well-known sizes only.

You talked about inserting user-supplied data of unknown size.

This is a completely different thing.

> But there is no way to ensure that every user of fdt_increase_size() will
> follow those rules.

There is always a way if you really try.

> I could, for instance, add an lmb parameter to fdt_increase_size(), but this
> will only apply in instances where the fdt exists inside an lmb. There is no
> lmb_realloc() function, so the most I can do is return an error if the lmb
> isn't big enough.

You could add a lmb_realloc() function...

> The problem is that by the time the code needs to resize the fdt, it has
> long since forgotten about the lmb.  We would need to modify

You could add code so that this information gets stored in a suitable
way.

> ft_board_setup() to take an lmb as a parameter, but that would require
> changes to almost 30 boards!

There have been changes in the past that required changes to many,
many more boards.  What exactly is the problem with doing this?

> And then, that doesn't help the situation where the fdt is just dumped in
> memory somewhere, like this:

Right. In this case the user is responsible.  But that's an unrelated
use case.

> So if an lmb is provided, we can check for overwrites, but we can't require
> the lmb.

Eventually you can do something like lmb_realloc() as well.

> The only problem with the above function is that lmb_size() doesn't exist,

Then add it?

> and I'm not sure how to implement it yet.  There doesn't appear to be any
> useful support functions for the lmb code at the moment.

Probably there was no need for these yet. Now there is.

> > Right. We don't provide safety belts against doing stupid things. But
> > you mentioned your extension is needed to insert firmware blobs which
> > can take "significant size" - you cannot estimate in advace (at
> > compile time) which size the end user may need. 
> 
> Actually, I can.  For firmware, at least, I have macro called
> CONFIG_SYS_FSL_FW_MAXSIZE that defines the largest size that firmware can

Why would you add another static buffer size to excuse the limitations
of another one?

Please expect me to reject a patch that adds CONFIG_SYS_FSL_FW_MAXSIZE
with the same argumets we're discussing here.

> applicable), but to also ensure limits on the firmware itself.  This would
> be a board-specific setting, since each board knows what kind of firmware it
> needs.  And then I have this:

A "board" cannot ever know what the user will be doing.

> Thus, each board can define its own settings, and boot_relocate_fdt() will
> honor them.

This makes little or no sense to me.

When we load such firmware from some form of image, we know exactly
what size if has. And we have mechanisms in place to use dynamically
sized buffers. Use these, please.

> > I will not accept code that relies on such compile time assumptions
> > about dynamically inserted data.
> 
> I don't see any way around that.

You know my opinion.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There's only one kind of woman ..." "Or man, for  that  matter.  You
either believe in yourself or you don't."
	-- Kirk and Harry Mudd, "Mudd's Women", stardate 1330.1

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

> I think the places where strcat() is used actually try and make sure
> to have enough room in the target buffer; also, almost all of themn
> appent static strigs with well-known sizes only.
> 
> You talked about inserting user-supplied data of unknown size.

Yes and no.  The data is supplied by the user, but it will also be limited
in size by a compile-time constant.  In this case, the firmware should come
only from Freescale, and the limit is double the size of current firmware,
so that should be enough to handle all situations, now and in the future.

>> I could, for instance, add an lmb parameter to fdt_increase_size(), but this
>> will only apply in instances where the fdt exists inside an lmb. There is no
>> lmb_realloc() function, so the most I can do is return an error if the lmb
>> isn't big enough.
> 
> You could add a lmb_realloc() function...

I'm not sure that would work in the context of the way lmb regions are
currently being used.  reallocating an lmb will probably require moving it
as well, and the caller of lmb_realloc() might not be able to handle updated
pointers.

>> The problem is that by the time the code needs to resize the fdt, it has
>> long since forgotten about the lmb.  We would need to modify
> 
> You could add code so that this information gets stored in a suitable
> way.

You make a lot of statements saying that I can do this and I can do that,
but you provide no examples.  Have you even looked at the code to see what
such a change would require?

I believe that modifying the parameters for fdt_board_setup() is too
intrusive of a change.  So the only available spot where we could put the
lmb pointers is in the bd_t structure.  And my understanding is that this
structure is already too fragile and should not be modified.

>> ft_board_setup() to take an lmb as a parameter, but that would require
>> changes to almost 30 boards!
> 
> There have been changes in the past that required changes to many,
> many more boards.  What exactly is the problem with doing this?

It's not worth it.  It would be easier for me to take the code of
fdt_increase_size() and just copy it into boot_relocate_fdt().

In fact, I think that's what I will do.  I thought I was being polite by
simply re-using an existing function, but I'd rather just copy the code into
boot_relocate_fdt() if it means I don't have to argue with you about it.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BF4051A.1090202@freescale.com> you wrote:
>
> 
> You make a lot of statements saying that I can do this and I can do
> that, but you provide no examples. Have you even looked at the code
> to see what such a change would require?

No, I haven't. I don't have to look at that code to see that what you
are trying to do is wrong. It's not my task to provide a solution to
ypur problem; all I want to make sure is we do not add code that is
known to have problems.

> In fact, I think that's what I will do.  I thought I was being polite by
> simply re-using an existing function, but I'd rather just copy the code into
> boot_relocate_fdt() if it means I don't have to argue with you about it.

What makes you think such a patch would be accepted?

Don't try to wiggle out of the symptoms. Please solve the problem at
the root.


I think he have sufficiently discussed this topic now?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Nobody trips over mountains. It is the small pebble that  causes  you
to  stumble.  Pass all the pebbles in your path and you will find you
have crossed the mountain.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Kumar Gala]

So I tried to read this whole thread and got lost in the discussion.  I'm wondering of something along the following lines would address your concerns:

#define CONFIG_SYS_FDT_PAD 0x3000

/* Allow for arch specific config before we boot */
int __fdt_board_size(void)
{
	return CONFIG_SYS_FDT_PAD;
}
int fdt_board_size(void) __attribute__((weak, alias("__ fdt_board_size")));


int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base,
                char **of_flat_tree, ulong *of_size)
{
...

        if ((fdt_blob + *of_size + fdt_board_size()) >=
                        ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base))
                relocate = 1;

...
}

Than board specific code knows what fix ups might happen and can implement dynamic behavior and do something like:


int fdt_board_size(void)
{
	determine_my_firmsize() + __fdt_board_size()
}

- k

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Kumar Gala,

In message <52E9D06A-E721-4907-9024-11BDC8D006E0@kernel.crashing.org> you wrote:
> So I tried to read this whole thread and got lost in the discussion.
> I'm wondering of something along the following lines would address your
> concerns:

My concerns are simple: if we need to increase the size of the FDT
because we pull in some random amountof data, we should make sure to
have enough room for this, or fail with a clear error message. 

And we should not try to use fixed sized buffers, but instead adapt
to the actual amount required by the end user.

Timur assumes that all such code and it's sizess will be known in
advance, and I disagree - other users will have other ideas what they
can do with this, and his assumptions will break.

> #define CONFIG_SYS_FDT_PAD 0x3000

Where exactly is this 0x3000 coming from?

> /* Allow for arch specific config before we boot */
> int __fdt_board_size(void)
> {
> 	return CONFIG_SYS_FDT_PAD;
> }
> int fdt_board_size(void) __attribute__((weak, alias("__
> fdt_board_size")));
> 
> 
> int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base,
>                 char **of_flat_tree, ulong *of_size)
> {
> ...
> 
>         if ((fdt_blob + *of_size + fdt_board_size()) >=
>                         ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base))
>                 relocate = 1;
> 
> ...
> }
> 
> Than board specific code knows what fix ups might happen and can
> implement dynamic behavior and do something like:

If any board specific code can determine the needed sizes, then how
would such code differ from one board to another?

Why would this in any way be a board specific implementation? This
makes no sense to me. The feature to include some binary data into the
DTB is IMO in no way dependent on or specific to a certain board.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To program is to be.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:
> Why would this in any way be a board specific implementation? This
> makes no sense to me. The feature to include some binary data into the
> DTB is IMO in no way dependent on or specific to a certain board.

The data I'm trying to embed is firmware for various devices on some of our
SOCs, such as the QE on the MPC8360.  Only boards with SOCs that have these
devices come with firmware, and not all of them require the firmware to be
passed to Linux.

Please note that fdt_increase_size() is just a front-end to fdt_open_into(),
so technically I don't need to fdt_increase_size().  However, you said you
would reject any patch that uses fdt_open_into() in this manner, so we're
back to square one.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BF4623B.1080109@freescale.com> you wrote:
>
> > Why would this in any way be a board specific implementation? This
> > makes no sense to me. The feature to include some binary data into the
> > DTB is IMO in no way dependent on or specific to a certain board.
> 
> The data I'm trying to embed is firmware for various devices on some of our
> SOCs, such as the QE on the MPC8360.  Only boards with SOCs that have these
> devices come with firmware, and not all of them require the firmware to be
> passed to Linux.

Yes, I know all of this. This is your specific use case. But maybe you
can take the blinkers off for a moment, and face up to other potential
use cases as well?

User A might want to ambed a FPGA bit stream, user B something we
don't even dream of yet.

Instead of implementing this feature in a way that makes it restricted
to your current use case only we can as well make it generic enough so
others can use it as well.

> Please note that fdt_increase_size() is just a front-end to fdt_open_into(),
> so technically I don't need to fdt_increase_size().  However, you said you
> would reject any patch that uses fdt_open_into() in this manner, so we're
> back to square one.

Back to square one? I did not realize you ever left that position ;-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Do not simplify the design of a program if a way can be found to make
it complex and wonderful.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

On Thu, May 20, 2010 at 3:28 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Timur Tabi,
>
> In message <4BF4623B.1080109@freescale.com> you wrote:
>>
>> > Why would this in any way be a board specific implementation? This
>> > makes no sense to me. The feature to include some binary data into the
>> > DTB is IMO in no way dependent on or specific to a certain board.
>>
>> The data I'm trying to embed is firmware for various devices on some of our
>> SOCs, such as the QE on the MPC8360. ?Only boards with SOCs that have these
>> devices come with firmware, and not all of them require the firmware to be
>> passed to Linux.
>
> Yes, I know all of this. This is your specific use case. But maybe you
> can take the blinkers off for a moment, and face up to other potential
> use cases as well?

Come on, Wolfgang.  Why do you think I posted this patch in the first
place?  I even said so in the title of patch: "make
fdt_increase_size() available to everyone".

You asked why the information would be board-specific, and I answered
that question.

Now I believe you're intentionally trying to be difficult.

> User A might want to ambed a FPGA bit stream, user B something we
> don't even dream of yet.

Which is exactly why I want fdt_increase_size() to be available to everyone.

> Instead of implementing this feature in a way that makes it restricted
> to your current use case only we can as well make it generic enough so
> others can use it as well.

Exactly!  And the best way to do that is to make the function that
adds space to the fdt available to everyone.

>> Please note that fdt_increase_size() is just a front-end to fdt_open_into(),
>> so technically I don't need to fdt_increase_size(). ?However, you said you
>> would reject any patch that uses fdt_open_into() in this manner, so we're
>> back to square one.
>
> Back to square one? I did not realize you ever left that position ;-)

How silly of me.  For a moment, I forgot that I was trying to improve U-Boot.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

On Thu, May 20, 2010 at 3:28 AM, Wolfgang Denk <wd@denx.de> wrote:
> Instead of implementing this feature in a way that makes it restricted
> to your current use case only we can as well make it generic enough so
> others can use it as well.

Wolfgang, can you tell me what method you would like me to use to pass
the address and/or size of the firmware FIT image to
boot_relocate_fdt()?  Obviously, you don't like CONFIG_SYS_FDT_PAD,
but I want to know what direction to go in before I start coding.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <AANLkTikGuHbXqZYwCup04C2SCuB6Uof75zdoH-boZzUX@mail.gmail.com> you wrote:
> On Thu, May 20, 2010 at 3:28 AM, Wolfgang Denk <wd@denx.de> wrote:
> > Instead of implementing this feature in a way that makes it restricted
> > to your current use case only we can as well make it generic enough so
> > others can use it as well.
> 
> Wolfgang, can you tell me what method you would like me to use to pass
> the address and/or size of the firmware FIT image to
> boot_relocate_fdt()?  Obviously, you don't like CONFIG_SYS_FDT_PAD,
> but I want to know what direction to go in before I start coding.

I'm not sure if I understand what you are trying to ask.

If you have an address and a size and want to pass these to a
function, you can of course use two arguments to that function, say
"phys_addr_t addr, size_t size".

But as this seems obvious I feel you might have something else in
mind, yet I cannot figure out what it might be.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There's no honorable way to kill, no gentle way to destroy.  There is
nothing good in war.  Except its ending.
	-- Abraham Lincoln, "The Savage Curtain", stardate 5906.5

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

> But as this seems obvious I feel you might have something else in
> mind, yet I cannot figure out what it might be.

How would you prefer the user to be able to specify the address of the
firmware FIT image, when he wants to boot Linux?  I think I remember Kumar
saying something about an environment variable, but I can't find that email
any more.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BFC1075.5010508@freescale.com> you wrote:
> 
> > But as this seems obvious I feel you might have something else in
> > mind, yet I cannot figure out what it might be.
> 
> How would you prefer the user to be able to specify the address of the
> firmware FIT image, when he wants to boot Linux?  I think I remember Kumar
> saying something about an environment variable, but I can't find that email
> any more.

That was on IRC; here the relevant snippet:

(17:09:35) wdenk_: If the feature is enabled for a board, and if
        there is an entry "insert blob type xxx here", then common
        code should do that.
(17:10:10) wdenk_: in plain old style it could be a 4th argument, or
	with FIT images it could be part of the image description.
(17:11:46) wdenk_: or it could be an env variable, or ...
(17:12:05) wdenk_: this is a design question then, and probably easy
	to solve.
(17:12:45) galak: are we ok w/the use of an env variable to point to
	the blob?
(17:13:16) galak: manly I'm trying to avoid moving to FIT images as
	that's a big churn on the user community
(17:13:24) galak: s/manly/mainly/
(17:14:09) TimurTabi: it would be nice if we didn't have to wrap
        everything in a FIT image in order for U-boot to be able to
        use it.
(17:14:52) TimurTabi: the QE firmware, for instance, already has a
	documented header that includes its size
(17:15:06) galak: I'd say there is a point at which FIT images make
	sense
(17:15:22) TimurTabi: as a hard requirement for this feature?
(17:15:39) galak: not for this feature, but in general
(17:16:08) galak: we need to be doing some tooling to make FIT usage
	as seamless as "old style/mkimage" is today
(17:16:43) galak: I'd like to try and separate the two issues, and
        thus asking if an env var pointing to the address of the qe
        firmware in this case would be acceptable to wdenk1
(17:16:52) TimurTabi: how do we make the "add a blob" feature generic
	without using FIT images?
(17:18:26) wdenk_: galak: please omit the "qe" part :-) It would be
        the address of a IH_TYPE_FIRMWARE image then, right?
(17:20:11) galak: wdenk1: I'd say the address can point to that, but
        I think its still a getenv ("qe_fw_addr")
(17:20:49) wdenk_: NAK. I want a _generic_ implementation, and "qe"
	is highly special.
(17:21:32) wdenk_: I don't want to use "qe_something" for an Altera
	FPGA bitstream.
(17:21:38) galak: wdenk1: agreed
(17:22:02) wdenk_: fdt_fw_addr or so might be better.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(17:22:21) galak: I'd be ok with that
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(17:22:23) TimurTabi: wdenk1: so you want all such firmware to be
        wrapped in an IH_TYPE_FIRMWARE image so that we can know
        generically how large that image is, even thought we need
        board-specific code to embed that image into the fdt?
(17:22:52) wdenk_: TimurTabi: we do not need any board-specific code
	for that.
(17:23:12) TimurTabi: wdenk1: yes we do. There is a specific device
        tree binding for embedding QE firmware.
(17:23:22) galak: its not board specific
(17:23:35) wdenk_: if a board enables the feature in it's
        configuration, and if the variable is found in the env, then
        generic code will do what needs to be done.
(17:26:03) galak: its firmware type specific, do we have anyway in
        the image hdr to determine QE firmware from Altera FPGA
        firmware?
(17:26:56) wdenk_: TimurTabi: That was a silly thing to add to the DT
        spec. Now nobody else can use it. If I have a MPC823E and I
        need to load it with CPM microcode I cannot use this, because
        CPM microcode is not QEFW. If I have a FPGA bitstream I
        cannot use it because it is not QEFW. If I have a firmware
        blob for FOO I cannot use it because it's not QEFW.
(17:28:00) TimurTabi: wdenk1: I don't think it's silly at all. The
        compatible property tells the linux driver whether the
        embedded firmware is for it or for something else
(17:28:07) galak: there needs to be some bit of information to convey
        the intended purpose of the FW
(17:28:28) wdenk_: galak: guess we don't have yet, but adding one
        specific type of firmware image and making it so special that
        nobody else who needs the same feature is just stupid.
(17:29:15) TimurTabi: wdenk1: generic firmware really doesn't make
	much sense, to me.
(17:29:58) galak: I agree, however there are few items that are
        specific. I think the vast majority of the code can be used
        by any firmware but we do need something to convey the
        intended user of the firmware (FPGA, QE, usb dev, etc.)
(17:35:32) TimurTabi: wdenk1: so qe firmware needs to be embedded in
	the device tree differently than other firmware
(17:37:05) wdenk_:  add a type identifier, then
(17:37:27) TimurTabi: wdenk1: I have. It's called the "compatible"
	property
(17:38:25) TimurTabi: U-boot also needs to locate all the QE nodes in
        the device tree, and put pointers from those nodes to the
        firmware node
(17:38:35) TimurTabi: so this can't be done generically
(17:38:58) galak: thus my question of type or use of name in the env
	var to imply type
(17:39:06) TimurTabi: this is the reason I posted the patch "libfdt:
        introduce function fdt_get_max_phandle"
(17:39:32) wdenk_: you and your "can't be done".
(17:39:43) TimurTabi: ok, "can't be done in a reasonable manner"
(17:40:15) wdenk_: use the name field in the image, for example
(17:40:45) TimurTabi: wdenk1: how do I know which nodes in the device
        tree should be updated?
(17:40:56) TimurTabi: also, the binding says that the qe firmware
        node should be located inside the first qe node
(17:41:17) TimurTabi: and that the other qe nodes should have
        phandles pointing to the firmware node in the first qe node
(17:41:53) wdenk_: TimurTabi: I do not care about QE...
(17:42:22) TimurTabi: wdenk1: I'm giving you an example of why we
	can't treat embedded firmware blobs in the device tree in a
	completely generic manner
(17:42:49) TimurTabi: the process of putting the firmware in the
	device tree is specific to the type of firmware itself
(17:43:20) wdenk_: sorry, gotta run now
(17:43:30) TimurTabi: ok

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"No one talks peace unless he's ready to back it up with war."
"He talks of peace if it is the only way to live."
	-- Colonel Green and Surak of Vulcan, "The Savage Curtain",
	   stardate 5906.5.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

> That was on IRC; here the relevant snippet:

Thanks.  Just to be clear, do you expect fdt_fw_addr always to point to a
FIT-wrapped firmware binary?

> (17:40:56) TimurTabi: also, the binding says that the qe firmware
>         node should be located inside the first qe node
> (17:41:17) TimurTabi: and that the other qe nodes should have
>         phandles pointing to the firmware node in the first qe node
> (17:41:53) wdenk_: TimurTabi: I do not care about QE...
> (17:42:22) TimurTabi: wdenk1: I'm giving you an example of why we
> 	can't treat embedded firmware blobs in the device tree in a
> 	completely generic manner
> (17:42:49) TimurTabi: the process of putting the firmware in the
> 	device tree is specific to the type of firmware itself
> (17:43:20) wdenk_: sorry, gotta run now
> (17:43:30) TimurTabi: ok

We never finished this discussion.  My point was that even if the firmware
is wrapped in a FIT image, the process by which the firmware is actually
inserted into the device tree is specific to the actual firmware.  You could
even say it's board-specific.

In contrast, you want the fdt relocation code to be able to increase the
size of the fdt without knowing any details about the firmware itself.
Therefore, there will be two pieces of code that references fdt_fw_addr.
The first is in boot_relocate_fdt(), which will extract the size information
from the FIT image that fdt_fw_addr points to.  The second is the QE code
which extracts the firmware from the FIT image and embeds it into the device
tree, in a QE-specific way.

I just want to make sure that we're on the same page.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BFC24DB.8070805@freescale.com> you wrote:
> Wolfgang Denk wrote:
> 
> > That was on IRC; here the relevant snippet:
> 
> Thanks.  Just to be clear, do you expect fdt_fw_addr always to point to a
> FIT-wrapped firmware binary?

Please re-read the IRC log. Kumar explicitly stated he was trying to
avoid making FIT images mandatory, at least for now. And I explicitly
wrote that it should be "the address of a IH_TYPE_FIRMWARE image
then".

> We never finished this discussion.  My point was that even if the firmware
> is wrapped in a FIT image, the process by which the firmware is actually
> inserted into the device tree is specific to the actual firmware.  You could
> even say it's board-specific.

You could say that. You could also say that 2+2=5.

I will argue in both cases.

> In contrast, you want the fdt relocation code to be able to increase the
> size of the fdt without knowing any details about the firmware itself.

That's not correct. At least we know the address and the size. And if
you follow my recommendation of using the name entry for type
information (or come up with a better proposal) we also know the
type.

> Therefore, there will be two pieces of code that references fdt_fw_addr.
> The first is in boot_relocate_fdt(), which will extract the size information
> from the FIT image that fdt_fw_addr points to.  The second is the QE code
> which extracts the firmware from the FIT image and embeds it into the device
> tree, in a QE-specific way.

I see no inherent problems with having a generic, common part for all
systems enabling this feature, plus eventually hooks for (additional)
customized processing of certain firmware image types.

Of course one can argue that making the decision on the type based on
the name entry is a stupid thing, and come up for example with
additional IH_TYPE entries; or even try to define subtypes. I think
I'll leave this as an exercise to you :-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The reasonable man adapts himself to the world; the unreasonable  one
persists  in  trying  to  adapt  the  world to himself. Therefore all
progress depends on the unreasonable man."      - George Bernard Shaw

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

>> Thanks.  Just to be clear, do you expect fdt_fw_addr always to point to a
>> FIT-wrapped firmware binary?
> 
> Please re-read the IRC log. Kumar explicitly stated he was trying to
> avoid making FIT images mandatory, at least for now.

And he proposed a board-specific function that would allow this to work, but
you rejected it.  So I don't still know how to implement what you want.

> And I explicitly
> wrote that it should be "the address of a IH_TYPE_FIRMWARE image
> then".

So you're saying fdt_fw_addr should pointer to either a FIT image or the
older image type?  What's the point in supporting the older type?  Isn't it
deprecated?

But either way, the firmware needs to be wrapped inside an image object.  I
think Kumar was implying that he didn't want to make *any* image type (FIT
or legacy) mandatory.

>> We never finished this discussion.  My point was that even if the firmware
>> is wrapped in a FIT image, the process by which the firmware is actually
>> inserted into the device tree is specific to the actual firmware.  You could
>> even say it's board-specific.
> 
> You could say that. You could also say that 2+2=5.
> 
> I will argue in both cases.

I don't understand your position.  The method by which firmware is to be
embedded in the device tree *is* specific to the kind of firmware in
question, and therefore requires knowledge of the kind of firmware.  A QE
firmware is not embedded in the device tree the same way an FPGA firmware
is.  This is just a fact.

You keep telling me that there's a counter argument to this statement, but I
don't know what it is.  You just tell me you disagree.  In effect, you are
the one saying that 2+2=5.

>> In contrast, you want the fdt relocation code to be able to increase the
>> size of the fdt without knowing any details about the firmware itself.
> 
> That's not correct. At least we know the address and the size.

Address and size is *not* details about the firmware itself.  When I say
"details about the firmware itself", I mean stuff like what kind of firmware
it is, what chip it's for, what it's supposed to do, etc.

> And if
> you follow my recommendation of using the name entry for type
> information (or come up with a better proposal) we also know the
> type.

Are you talking about the ih_name field in the image_header_t structure?  So
for instance, if the ih_name field says "QE Firmware", we can assume assume
that it's a QE firmware, and the generic code should have something like
this in it:

#include "qe.h"

if (strcmp(image->ih_name, "QE Firmware") == 0) {
	/*
	 * Embed the firmware in the device tree using the binding
	 * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe
	 * /qe.txt
         */
}

Doesn't that seem really clunky to you?  That requires the generic code to
have knowledge of every type of firmware.  Wouldn't it be simpler if we just
followed Kumar's recommendation to have a board-specific __weak__ function
that handled this code?

>> Therefore, there will be two pieces of code that references fdt_fw_addr.
>> The first is in boot_relocate_fdt(), which will extract the size information
>> from the FIT image that fdt_fw_addr points to.  The second is the QE code
>> which extracts the firmware from the FIT image and embeds it into the device
>> tree, in a QE-specific way.
> 
> I see no inherent problems with having a generic, common part for all
> systems enabling this feature, plus eventually hooks for (additional)
> customized processing of certain firmware image types.

So you agree with Kumar's idea of having a weak function that embeds the
firmware into the device tree, but the firmware must always be wrapped in an
image format?

> Of course one can argue that making the decision on the type based on
> the name entry is a stupid thing, and come up for example with
> additional IH_TYPE entries; or even try to define subtypes. I think
> I'll leave this as an exercise to you :-)

I'd rather not use subtypes, because I don't think we want anything like this:

	if (is_qe_firmware()) {
		/* embed QE firmware */
	} else if (is_amd_fpga_firmware()) {
		/* embed AMD fpga firmware */
	} ...

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BFD3A39.4090209@freescale.com> you wrote:
> 
> > Please re-read the IRC log. Kumar explicitly stated he was trying to
> > avoid making FIT images mandatory, at least for now.
> 
> And he proposed a board-specific function that would allow this to work, but
> you rejected it.  So I don't still know how to implement what you want.

Well, in a way that may be image-type dependent, but that is not
board-specific.

> > And I explicitly
> > wrote that it should be "the address of a IH_TYPE_FIRMWARE image
> > then".
> 
> So you're saying fdt_fw_addr should pointer to either a FIT image or the
> older image type?  What's the point in supporting the older type?  Isn't it
> deprecated?

No, not really. It works fine for the intended purpose. Actually I
still prefer it in a lot of cases because we have checksum protection
of the header information, while you can have a totally corrupted
DTB without really being able to detect it.

> But either way, the firmware needs to be wrapped inside an image object.  I
> think Kumar was implying that he didn't want to make *any* image type (FIT
> or legacy) mandatory.

And where would you then get type and size information from?

> I don't understand your position.  The method by which firmware is to be
> embedded in the device tree *is* specific to the kind of firmware in
> question, and therefore requires knowledge of the kind of firmware.  A QE
> firmware is not embedded in the device tree the same way an FPGA firmware
> is.  This is just a fact.

I also said that I see no problems with ading type specific hooks.

> You keep telling me that there's a counter argument to this statement, but I
> don't know what it is.  You just tell me you disagree.  In effect, you are
> the one saying that 2+2=5.

Really?

> >> In contrast, you want the fdt relocation code to be able to increase the
> >> size of the fdt without knowing any details about the firmware itself.
> > 
> > That's not correct. At least we know the address and the size.
> 
> Address and size is *not* details about the firmware itself.  When I say

No, but they are important properties, for example when it comes to
find out by how much the DT needs to be grown.

> "details about the firmware itself", I mean stuff like what kind of firmware
> it is, what chip it's for, what it's supposed to do, etc.

Maybe we can abstrct off most of this, and/or leave it to image type
specific handlers?

> Are you talking about the ih_name field in the image_header_t structure?  So
> for instance, if the ih_name field says "QE Firmware", we can assume assume
> that it's a QE firmware, and the generic code should have something like
> this in it:
> 
> #include "qe.h"

No. There would be no "qe.h" needed in that generic code.

> if (strcmp(image->ih_name, "QE Firmware") == 0) {
> 	/*
> 	 * Embed the firmware in the device tree using the binding
> 	 * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe
> 	 * /qe.txt
>          */
> }

No. More like

	if (strcmp(image->ih_name, "QE Firmware") == 0)
		handle_fdt_fw_qe(image);

or similar. Probably #ifdef'ed for machines that have enabled QE
support in their board config, or with a weak handle_fdt_fw_qe() that
gets filled in for QE aware boards only so the compiler optimizes away
that call everywhere else.

> Doesn't that seem really clunky to you?  That requires the generic code to
> have knowledge of every type of firmware.  Wouldn't it be simpler if we just
> followed Kumar's recommendation to have a board-specific __weak__ function
> that handled this code?

D*mn. Don't you get it. There is NOT BOARD-SPECIFIC CODE anywhere.

It is feature specific. Either you enable QE support or not, but then
the same code will be used for all boards enabling this feature.

> > I see no inherent problems with having a generic, common part for all
> > systems enabling this feature, plus eventually hooks for (additional)
> > customized processing of certain firmware image types.
> 
> So you agree with Kumar's idea of having a weak function that embeds the
> firmware into the device tree, but the firmware must always be wrapped in an
> image format?

Yes. Note that there is NOT any board-specific code.

> > Of course one can argue that making the decision on the type based on
> > the name entry is a stupid thing, and come up for example with
> > additional IH_TYPE entries; or even try to define subtypes. I think
> > I'll leave this as an exercise to you :-)
> 
> I'd rather not use subtypes, because I don't think we want anything like this:
> 
> 	if (is_qe_firmware()) {
> 		/* embed QE firmware */
> 	} else if (is_amd_fpga_firmware()) {
> 		/* embed AMD fpga firmware */
> 	} ...

In which way would that be worse compared to

	if (strcmp(image->ih_name, "QE Firmware") == 0)
		handle_fdt_fw_qe(image);
	else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0)
		handle_fdt_fw_amd(image);
	...
?

Actually it would be easier to read...


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The game of life is a game of boomerangs.  Our  thoughts,  deeds  and
words return to us sooner or later with astounding accuracy.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

>> And he proposed a board-specific function that would allow this to work, but
>> you rejected it.  So I don't still know how to implement what you want.
> 
> Well, in a way that may be image-type dependent, but that is not
> board-specific.

Technically, that's true, but in most cases the function that returns the
address/size of the firmware would exist in board code.

For example, the MPC8323 has a bug in its QE UART support, so if you want to
use the QE as a UART, you need to download a special QE firmware to
work-around the bug.  So even though all MPC8323 boards have a QE that can
run in UART mode, only those boards that have a QE wired to an RS232 port
need to enable the UART work-around, so only those boards would embed the
firmware in the device tree.

>> So you're saying fdt_fw_addr should pointer to either a FIT image or the
>> older image type?  What's the point in supporting the older type?  Isn't it
>> deprecated?
> 
> No, not really. It works fine for the intended purpose. Actually I
> still prefer it in a lot of cases because we have checksum protection
> of the header information, while you can have a totally corrupted
> DTB without really being able to detect it.

Ok.

> 
>> But either way, the firmware needs to be wrapped inside an image object.  I
>> think Kumar was implying that he didn't want to make *any* image type (FIT
>> or legacy) mandatory.
> 
> And where would you then get type and size information from?

For example, QE firmware comes with its own header that includes size
information, as well as a magic number and a CRC.  See struct qe_firmware in
qe.h.  Since we distribute the QE firmware on our boards as-is, it would be
nice if we didn't need to wrap those binaries inside a legacy/FIT image.  So
on an MPC8323E-MDS board, for example, the board-specific implementation of
fdt_board_size() could support having fdt_fw_addr points directly to a QE
firmware binary.  We already have code that validates a QE firmware (see
function qe_upload_firmware()), so this is safe.

For those firmware types that don't have such a header built-in, they would
need to be wrapped in a legacy/FIT image.

> 
>> I don't understand your position.  The method by which firmware is to be
>> embedded in the device tree *is* specific to the kind of firmware in
>> question, and therefore requires knowledge of the kind of firmware.  A QE
>> firmware is not embedded in the device tree the same way an FPGA firmware
>> is.  This is just a fact.
> 
> I also said that I see no problems with ading type specific hooks.

Ok.

>>> That's not correct. At least we know the address and the size.
>>
>> Address and size is *not* details about the firmware itself.  When I say
> 
> No, but they are important properties, for example when it comes to
> find out by how much the DT needs to be grown.

I agree, although I would say that only "size" is necessary.

> 
>> "details about the firmware itself", I mean stuff like what kind of firmware
>> it is, what chip it's for, what it's supposed to do, etc.
> 
> Maybe we can abstrct off most of this, and/or leave it to image type
> specific handlers?

I'm not sure that's a good idea.  We can't predict what kind of firmware
we'll need to support in the future, and a board-specific fdt_board_size()
function is, in my opinion, a more elegant way to solve the problem.

>> #include "qe.h"
> 
> No. There would be no "qe.h" needed in that generic code.
> 
>> if (strcmp(image->ih_name, "QE Firmware") == 0) {
>> 	/*
>> 	 * Embed the firmware in the device tree using the binding
>> 	 * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe
>> 	 * /qe.txt
>>          */
>> }
> 
> No. More like
> 
> 	if (strcmp(image->ih_name, "QE Firmware") == 0)
> 		handle_fdt_fw_qe(image);

Where is the function prototype for handle_fdt_fw_qe()?  Wouldn't it be in qe.h?

Also, I really think the strcmp is a bad idea, because then we have to put a
specific string in the ih_name field, not something unique to the actual
firmware in the image.

> or similar. Probably #ifdef'ed for machines that have enabled QE
> support in their board config, or with a weak handle_fdt_fw_qe() that
> gets filled in for QE aware boards only so the compiler optimizes away
> that call everywhere else.

So it would look like board_init_r()?

>> Doesn't that seem really clunky to you?  That requires the generic code to
>> have knowledge of every type of firmware.  Wouldn't it be simpler if we just
>> followed Kumar's recommendation to have a board-specific __weak__ function
>> that handled this code?
> 
> D*mn. Don't you get it. There is NOT BOARD-SPECIFIC CODE anywhere.
>
> It is feature specific. Either you enable QE support or not, but then
> the same code will be used for all boards enabling this feature.

Ok.

> 
>>> I see no inherent problems with having a generic, common part for all
>>> systems enabling this feature, plus eventually hooks for (additional)
>>> customized processing of certain firmware image types.
>>
>> So you agree with Kumar's idea of having a weak function that embeds the
>> firmware into the device tree, but the firmware must always be wrapped in an
>> image format?
> 
> Yes. Note that there is NOT any board-specific code.

Kumar's function would be defined in a board-specific source file.  Using
the QE firmware example for the 8323 above, I would add this code to
board/freescale/mpc832xemds/mpc832xemds.c:

#include "qe.h"

int fdt_board_size(void)
{
	struct qe_firmware *qefw = getenv("fdt_fw_addr");

	if (!qefw)
		return 0;

	if (!is_valid_qe_firmware(qefw))
		return 0;

	return qefw->header.length;
}

>> I'd rather not use subtypes, because I don't think we want anything like this:
>>
>> 	if (is_qe_firmware()) {
>> 		/* embed QE firmware */
>> 	} else if (is_amd_fpga_firmware()) {
>> 		/* embed AMD fpga firmware */
>> 	} ...
> 
> In which way would that be worse compared to
> 
> 	if (strcmp(image->ih_name, "QE Firmware") == 0)
> 		handle_fdt_fw_qe(image);
> 	else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0)
> 		handle_fdt_fw_amd(image);
> 	...
> ?

It wouldn't be.  I don't like either method.

I believe we should have a board-specific function that figures out how much
extra space is needed, and just returns a single integer that the
boot_relocate_fdt() uses to pad the FDT when it relocates it.

That board-specific function can do whatever it needs to do to figure out
which firmware binaries need to be embedded, where they are, how big they
are, and more importantly, what format they're in.

Later on, function ft_board_setup() will be the function that actually
embeds the firmware into the device tree.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Scott Wood]

On Wed, May 26, 2010 at 11:38:27AM -0500, Timur Tabi wrote:
> I believe we should have a board-specific function that figures out how much
> extra space is needed, and just returns a single integer that the
> boot_relocate_fdt() uses to pad the FDT when it relocates it.

Why don't we just grow the FDT on demand, after we make sure that it always  
lives someplace that is safely growable?

Or if we absolutely must resize it all at once, have a variable that
contains the size required, which gets increased by whatever init code
determines a reason for it (whether it be QE firmware in this environment
variable, some other firmware in that environment variable, just a bunch of
nodes that u-boot creates on this platform, etc).

-Scott

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Scott Wood wrote:
> On Wed, May 26, 2010 at 11:38:27AM -0500, Timur Tabi wrote:
>> I believe we should have a board-specific function that figures out how much
>> extra space is needed, and just returns a single integer that the
>> boot_relocate_fdt() uses to pad the FDT when it relocates it.
> 
> Why don't we just grow the FDT on demand, after we make sure that it always  
> lives someplace that is safely growable?

We use lmb regions to allocate space for the fdt in the bootmap, so we need
to know how big the lmb region should be before we allocate it.  Resizing an
lmb region will probably require moving it, and that might confuse the
upper-level functions that expect the fdt pointer to remain constant.

> Or if we absolutely must resize it all at once, have a variable that
> contains the size required, which gets increased by whatever init code
> determines a reason for it (whether it be QE firmware in this environment
> variable, some other firmware in that environment variable, just a bunch of
> nodes that u-boot creates on this platform, etc).

The issue is that how those functions will look.  Kumar and I are advocating
a board-specific weak function that figures it all out at once and returns a
single number.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Scott Wood]

On 05/26/2010 12:56 PM, Timur Tabi wrote:
> Scott Wood wrote:
>> On Wed, May 26, 2010 at 11:38:27AM -0500, Timur Tabi wrote:
>>> I believe we should have a board-specific function that figures out how much
>>> extra space is needed, and just returns a single integer that the
>>> boot_relocate_fdt() uses to pad the FDT when it relocates it.
>>
>> Why don't we just grow the FDT on demand, after we make sure that it always
>> lives someplace that is safely growable?
>
> We use lmb regions to allocate space for the fdt in the bootmap, so we need
> to know how big the lmb region should be before we allocate it.

But you can reasonably allocate significantly more than you'll need 
without actually causing the fdt to get that big.  The actual cap could 
be a board specific magic number (like CONFIG_SYS_MALLOC_LEN), or we 
could cap it at something based on the amount of RAM.

> Resizing an
> lmb region will probably require moving it, and that might confuse the
> upper-level functions that expect the fdt pointer to remain constant.
>
>> Or if we absolutely must resize it all at once, have a variable that
>> contains the size required, which gets increased by whatever init code
>> determines a reason for it (whether it be QE firmware in this environment
>> variable, some other firmware in that environment variable, just a bunch of
>> nodes that u-boot creates on this platform, etc).
>
> The issue is that how those functions will look.

What functions?  I'm just talking about arbitrary code that runs before 
the fdt is resized, and after relocation (or even before if we make it a 
gd_t variable), and adds a certain amount to a variable.

> Kumar and I are advocating
> a board-specific weak function that figures it all out at once and returns a
> single number.

That seems an awkward combination of centralization (you need to combine 
every fdt-expanding feature found on a board into the board file) and 
decentralization (if such a feature is added or changed you may need to 
update a bunch of board files).

-Scott

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Scott Wood wrote:

> But you can reasonably allocate significantly more than you'll need 
> without actually causing the fdt to get that big.  The actual cap could 
> be a board specific magic number (like CONFIG_SYS_MALLOC_LEN), or we 
> could cap it at something based on the amount of RAM.

We have something like that already:

#ifndef CONFIG_SYS_FDT_PAD
#define CONFIG_SYS_FDT_PAD 0x3000
#endif

And Wolfgang doesn't like it.

I was going to do this in my board header file:

#define CONFIG_SYS_MAX_QE_FW_SIZE	0x4000

#define CONFIG_SYS_FDT_PAD (0x3000 + CONFIG_SYS_MAX_QE_FW_SIZE)

And I was even going to add code to the QE firmware functions to verify that
the firmware is not larger than CONFIG_SYS_MAX_QE_FW_SIZE.

>> The issue is that how those functions will look.
> 
> What functions?  I'm just talking about arbitrary code that runs before 
> the fdt is resized, and after relocation (or even before if we make it a 
> gd_t variable), and adds a certain amount to a variable.

Actually, that would be *during* relocation because we relocate and resize
the fdt in the same function: fdt_open_into().

Depending on the nature of the firmware, we may not know anything about the
firmware until we try to boot Linux, so we need to decide where this
arbitrary code is supposed to exist.

>> Kumar and I are advocating
>> a board-specific weak function that figures it all out at once and returns a
>> single number.
> 
> That seems an awkward combination of centralization (you need to combine 
> every fdt-expanding feature found on a board into the board file) and 
> decentralization (if such a feature is added or changed you may need to 
> update a bunch of board files).

We could have feature-specific functions that do the work of handling the
fdt size for a specific feature.  So qe_firmware_size(void *) would tell you
the size of the QE firmware, and your board's fdt_board_size() would just be
a front-end to qe_firmware_size().

Are you suggesting that we implement some user interface where the user, on
the command line, can specify any number and type of firmware binaries to
get embedded in the device tree?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BFD6704.2040800@freescale.com> you wrote:
> 
> We have something like that already:
> 
> #ifndef CONFIG_SYS_FDT_PAD
> #define CONFIG_SYS_FDT_PAD 0x3000
> #endif
> 
> And Wolfgang doesn't like it.

Because nobody can explain where this magic number 0x3000 is coming
from or why it is supposed to be sufficient.

> I was going to do this in my board header file:
> 
> #define CONFIG_SYS_MAX_QE_FW_SIZE	0x4000

And I rejected this because it is pecific to just one use case (QE
support) while I want a generic solution. Also, compile time size
limits are always a bad thing if we can have run-time computed limits
instead.

> Are you suggesting that we implement some user interface where the user, on
> the command line, can specify any number and type of firmware binaries to
> get embedded in the device tree?

No. Such information should be part of the firmware image itself.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Documentation is the castor oil of programming.
Managers know it must be good because the programmers hate it so much.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Wolfgang Denk]

Dear Timur Tabi,

In message <4BFD4E83.2080202@freescale.com> you wrote:
> 
> > Well, in a way that may be image-type dependent, but that is not
> > board-specific.
> 
> Technically, that's true, but in most cases the function that returns the
> address/size of the firmware would exist in board code.
> 
> For example, the MPC8323 has a bug in its QE UART support, so if you want to
> use the QE as a UART, you need to download a special QE firmware to
> work-around the bug.  So even though all MPC8323 boards have a QE that can
> run in UART mode, only those boards that have a QE wired to an RS232 port
> need to enable the UART work-around, so only those boards would embed the
> firmware in the device tree.

Right. Then these boards can enable the feature in their board config
files, and get what they want. But it's still generic code, and not
spoecific to any special board.

> >> But either way, the firmware needs to be wrapped inside an image object.  I
> >> think Kumar was implying that he didn't want to make *any* image type (FIT
> >> or legacy) mandatory.
> > 
> > And where would you then get type and size information from?
> 
> For example, QE firmware comes with its own header that includes size
> information, as well as a magic number and a CRC.  See struct qe_firmware in

But we want a generic solution, and this includes firmware blobs that
may not contain such information. Ergo we use what we use elsewhere in
U-Boot for the very purpose: U-Boot image files.

> For those firmware types that don't have such a header built-in, they would
> need to be wrapped in a legacy/FIT image.

And for the sake of using common code we will do this always.

> > Maybe we can abstrct off most of this, and/or leave it to image type
> > specific handlers?
> 
> I'm not sure that's a good idea.  We can't predict what kind of firmware
> we'll need to support in the future, and a board-specific fdt_board_size()
> function is, in my opinion, a more elegant way to solve the problem.

You completely fail to understand what "board-specific" means. After
so many rounds of discussion and so many attempts to explain this, you
still don't get it. I'm on the verge of giving up.

This code is in no way board-specific, so board-specific functions
make no sense.

Just swallow and accept this if you cannot understand it.


> >> #include "qe.h"
> > 
> > No. There would be no "qe.h" needed in that generic code.
> > 
> >> if (strcmp(image->ih_name, "QE Firmware") == 0) {
> >> 	/*
> >> 	 * Embed the firmware in the device tree using the binding
> >> 	 * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe
> >> 	 * /qe.txt
> >>          */
> >> }
> > 
> > No. More like
> > 
> > 	if (strcmp(image->ih_name, "QE Firmware") == 0)
> > 		handle_fdt_fw_qe(image);
> 
> Where is the function prototype for handle_fdt_fw_qe()?  Wouldn't it be in qe.h?

Grrrrrghhhh!!!!!

Please open your eyes and start reading!!!

Just 15 lines above I wrote: "There would be no "qe.h" needed in that
generic code." Then what good couldit do if we add a prototype to that
header file that we need in common code? Nothing!

Obviously we need that prototype in a header file included by that
common code, and this has NOTHING to do with any *qe*.h files.

> Also, I really think the strcmp is a bad idea, because then we have to put a
> specific string in the ih_name field, not something unique to the actual
> firmware in the image.

Well, I said that this might need discussion - I offered image
subtypes as an alternative, maybe there are other/better ideas. Input
welcome.

> > or similar. Probably #ifdef'ed for machines that have enabled QE
> > support in their board config, or with a weak handle_fdt_fw_qe() that
> > gets filled in for QE aware boards only so the compiler optimizes away
> > that call everywhere else.
> 
> So it would look like board_init_r()?

Not really. board_init_r() is still architecture specific - ARM and
PowerPC and MIPS and ... use different implementations.

Here this is feature dependent, not architecture or CPU or board
dependent code.

> >> So you agree with Kumar's idea of having a weak function that embeds the
> >> firmware into the device tree, but the firmware must always be wrapped in an
> >> image format?
> > 
> > Yes. Note that there is NOT any board-specific code.
> 
> Kumar's function would be defined in a board-specific source file.  Using

I don;t think so. It would not be accepted for mainline then.

> the QE firmware example for the 8323 above, I would add this code to
> board/freescale/mpc832xemds/mpc832xemds.c:

Do you see my NAK?  NAK!!!

In which way is this specific to the mpc832xemds board?

Would my custom 8323 board not need the same thing? Do you expect us
to duplicate this code to all boards which need this QE feature?


> > In which way would that be worse compared to
> > 
> > 	if (strcmp(image->ih_name, "QE Firmware") == 0)
> > 		handle_fdt_fw_qe(image);
> > 	else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0)
> > 		handle_fdt_fw_amd(image);
> > 	...
> > ?
> 
> It wouldn't be.  I don't like either method.

I'll happily accept more elegant / powerful approaches.

> I believe we should have a board-specific function that figures out how much
> extra space is needed, and just returns a single integer that the
> boot_relocate_fdt() uses to pad the FDT when it relocates it.
> 
> That board-specific function can do whatever it needs to do to figure out
> which firmware binaries need to be embedded, where they are, how big they
> are, and more importantly, what format they're in.
> 
> Later on, function ft_board_setup() will be the function that actually
> embeds the firmware into the device tree.

Seems you really cannot look beyond your own nose, it seems. 

Can't you ask somebody else to implement this for you?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The philosophy exam was a piece of cake  -  which  was  a  bit  of  a
surprise, actually, because I was expecting some questions on a sheet
of paper.

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Kumar Gala]

On May 19, 2010, at 5:06 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <52E9D06A-E721-4907-9024-11BDC8D006E0@kernel.crashing.org> you wrote:
>> So I tried to read this whole thread and got lost in the discussion.
>> I'm wondering of something along the following lines would address your
>> concerns:
> 
> My concerns are simple: if we need to increase the size of the FDT
> because we pull in some random amountof data, we should make sure to
> have enough room for this, or fail with a clear error message. 
> 
> And we should not try to use fixed sized buffers, but instead adapt
> to the actual amount required by the end user.
> 
> Timur assumes that all such code and it's sizess will be known in
> advance, and I disagree - other users will have other ideas what they
> can do with this, and his assumptions will break.
> 
>> #define CONFIG_SYS_FDT_PAD 0x3000
> 
> Where exactly is this 0x3000 coming from?

I think this was done to manage some growth of the dtb based on board fix ups.  However, not sure where the number came from.


>> /* Allow for arch specific config before we boot */
>> int __fdt_board_size(void)
>> {
>> 	return CONFIG_SYS_FDT_PAD;
>> }
>> int fdt_board_size(void) __attribute__((weak, alias("__
>> fdt_board_size")));
>> 
>> 
>> int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base,
>>                char **of_flat_tree, ulong *of_size)
>> {
>> ...
>> 
>>        if ((fdt_blob + *of_size + fdt_board_size()) >=
>>                        ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base))
>>                relocate = 1;
>> 
>> ...
>> }
>> 
>> Than board specific code knows what fix ups might happen and can
>> implement dynamic behavior and do something like:
> 
> If any board specific code can determine the needed sizes, then how
> would such code differ from one board to another?
> 
> Why would this in any way be a board specific implementation? This
> makes no sense to me. The feature to include some binary data into the
> DTB is IMO in no way dependent on or specific to a certain board.

I agree this isn't 100% board specific, but my intent was to leave it under the board control.  One assumes the board knows if it needs firmware blob A in dtb or not.

It follows the same logic as why we have ft_board_setup.

- k

[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Kumar Gala wrote:

> int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base,
>                 char **of_flat_tree, ulong *of_size)
> {
> ...
> 
>         if ((fdt_blob + *of_size + fdt_board_size()) >=
>                         ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base))
>                 relocate = 1;

Kumar, what do you think about having boot_relocate_fdt() always relocate
the fdt, even if it is already in the bootmap?  That would ensure that it
gets resized and put into an lmb region.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/3] libfdt: make fdt_increase_size() available to everyone

[Jerry Van Baren]

Timur Tabi wrote:
> The function fdt_increase_size() increases the size of the device tree by the
> given amount.  This is useful for any code that wants to add a node or large
> property, to avoid the possibility of running out of space.  It's much smarter
> to have U-Boot increase the size of device tree when it knows it's going to
> add data, instead of hoping that the DTS was compiled with the right -p value.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Acked-by: Gerald Van Baren <vanbaren@cideas.com>

> ---
> 
>  common/fdt_support.c  |   20 ++++++++++----------
>  include/fdt_support.h |    1 +
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index b6f252a..01d635b 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -645,6 +645,16 @@ int fdt_resize(void *blob)
>  	return actualsize;
>  }
>  
> +int fdt_increase_size(void *fdt, int add_len)
> +{
> +	int newlen;
> +
> +	newlen = fdt_totalsize(fdt) + add_len;
> +
> +	/* Open in place with a new len */
> +	return fdt_open_into(fdt, fdt, newlen);
> +}
> +
>  #ifdef CONFIG_PCI
>  #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
>  
> @@ -792,16 +802,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset)
>  	return 0;
>  }
>  
> -int fdt_increase_size(void *fdt, int add_len)
> -{
> -	int newlen;
> -
> -	newlen = fdt_totalsize(fdt) + add_len;
> -
> -	/* Open in place with a new len */
> -	return fdt_open_into(fdt, fdt, newlen);
> -}
> -
>  int fdt_del_partitions(void *blob, int parent_offset)
>  {
>  	const void *prop;
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 9a453af..d70627d 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
>  
>  void set_working_fdt_addr(void *addr);
>  int fdt_resize(void *blob);
> +int fdt_increase_size(void *fdt, int add_len);
>  
>  int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
>