[1/1] boot/uboot: add config option for uboot environment padding byte

Message ID 1513018028-18256-1-git-send-email-johannes.schmitz1@gmail.com
State Rejected
Headers show
Series
  • [1/1] boot/uboot: add config option for uboot environment padding byte
Related show

Commit Message

Johannes Schmitz Dec. 11, 2017, 6:47 p.m.
Updated the help text for the size of the uboot environment. The size of
the uboot environment, which is given in bytes, needs to be exactly
correct in order to calculate the CRC checksum as expected and
re-calculated by uboot during the target boot process. Otherwise the
environment is ignored during boot due to CRC mismatch.

Additionally, add the missing option to specify the padding byte which
is used by the mkenvimage -p parameter.

Signed-off-by: Johannes Schmitz <johannes.schmitz1@gmail.com>
---
 boot/uboot/Config.in | 13 +++++++++++--
 boot/uboot/uboot.mk  | 10 ++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

Comments

Thomas Petazzoni Dec. 12, 2017, 5:27 a.m. | #1
Hello,

On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:

>  config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
>  	string "Size of environment"
>  	help
> -	  Size of envronment, can be prefixed with 0x for hexadecimal
> -	  values.
> +	  Size of environment in bytes, can be prefixed with 0x for
> +	  hexadecimal values. Needs to match exactly for correct CRC

"Needs to match exactly" with what ?

It needs to match with the size of the environment declared in the
U-Boot configuration.

> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +	hex "Padding byte"
> +	default 0x00
> +	help
> +	  The byte used for padding at the end of the environment image.

I'm still not sure what is the point of customizing the padding byte.
Does someone has a use-case for that ? If not, I'm not sure it makes
sense to add a Buildroot option for it.

Best regards,

Thomas
Johannes Schmitz Dec. 12, 2017, 9:04 a.m. | #2
> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +     hex "Padding byte"
> +     default 0x00
> +     help
> +       The byte used for padding at the end of the environment image.

I'm still not sure what is the point of customizing the padding byte.
Does someone has a use-case for that ? If not, I'm not sure it makes
sense to add a Buildroot option for it.


I find it definitely useful for debugging. I wouldn't have been able to
isolate the issue with the size and CRC without this option because if you
want to compare with hexdump you want everything to exactly match to see
what's wrong. Without this, the next person might go down the same way like
myself. The default of uboot when saving the environment is 0x00 and the
default of mkenvimage is 0xFF. My idea is to make it the same to (0x00) to
make the developers life easier. My experience is that any of these details
takes some amount of your time to sort out so a less confusing default can
safe time for everyone. On the other hand people might want to deliberately
set it to 0xFF to exactly check the boarders of the env partition on the SD
card, especially during development of genimage.cfg. Furthermore it makes
you understand quicker what happens during env image generation. If you
hide the option from the user/platform developer again makes life harder. I
am speaking from the point of view of a buildroot newcomer. Now of course I
now what's going on but would that option have been there it would have
saved me one afternoon.

Regards
Johannes
<div dir="auto"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="quoted-text">&gt; +config BR2_TARGET_UBOOT_ENVIMAGE_<wbr>PADDING_BYTE<br>
&gt; +     hex &quot;Padding byte&quot;<br>
&gt; +     default 0x00<br>
&gt; +     help<br>
&gt; +       The byte used for padding at the end of the environment image.<br>
<br>
</div>I&#39;m still not sure what is the point of customizing the padding byte.<br>
Does someone has a use-case for that ? If not, I&#39;m not sure it makes<br>
sense to add a Buildroot option for it.</blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">I find it definitely useful for debugging. I wouldn&#39;t have been able to isolate the issue with the size and CRC without this option because if you want to compare with hexdump you want everything to exactly match to see what&#39;s wrong. Without this, the next person might go down the same way like myself. The default of uboot when saving the environment is 0x00 and the default of mkenvimage is 0xFF. My idea is to make it the same to (0x00) to make the developers life easier. My experience is that any of these details takes some amount of your time to sort out so a less confusing default can safe time for everyone. On the other hand people might want to deliberately set it to 0xFF to exactly check the boarders of the env partition on the SD card, especially during development of genimage.cfg. Furthermore it makes you understand quicker what happens during env image generation. If you hide the option from the user/platform developer again makes life harder. I am speaking from the point of view of a buildroot newcomer. Now of course I now what&#39;s going on but would that option have been there it would have saved me one afternoon. </div><div dir="auto"><br></div><div dir="auto">Regards </div><div dir="auto">Johannes </div></div>
Thomas Petazzoni Dec. 12, 2017, 10:40 a.m. | #3
Hello,

On Tue, 12 Dec 2017 10:04:38 +0100, Johannes Schmitz wrote:

> I find it definitely useful for debugging. I wouldn't have been able to
> isolate the issue with the size and CRC without this option because if you
> want to compare with hexdump you want everything to exactly match to see
> what's wrong. Without this, the next person might go down the same way like
> myself. The default of uboot when saving the environment is 0x00 and the
> default of mkenvimage is 0xFF. My idea is to make it the same to (0x00) to
> make the developers life easier. My experience is that any of these details
> takes some amount of your time to sort out so a less confusing default can
> safe time for everyone. On the other hand people might want to deliberately
> set it to 0xFF to exactly check the boarders of the env partition on the SD
> card, especially during development of genimage.cfg. Furthermore it makes
> you understand quicker what happens during env image generation. If you
> hide the option from the user/platform developer again makes life harder. I
> am speaking from the point of view of a buildroot newcomer. Now of course I
> now what's going on but would that option have been there it would have
> saved me one afternoon.

Hm, ok. I'm still not super convinced, but the code/complexity isn't
big enough to really argue more than that :)

Thanks!

Thomas
Yann E. MORIN Dec. 13, 2017, 6:23 p.m. | #4
Thomas, Johannes, All,

On 2017-12-12 06:27 +0100, Thomas Petazzoni spake thusly:
> On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:
[--SNIP--]
> > +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> > +	hex "Padding byte"
> > +	default 0x00
> > +	help
> > +	  The byte used for padding at the end of the environment image.
> 
> I'm still not sure what is the point of customizing the padding byte.
> Does someone has a use-case for that ? If not, I'm not sure it makes
> sense to add a Buildroot option for it.

As far as I understand, this is because the U-Boot on Johannes' board
would expect the padding byte to be 0x00 when the default for mkenvimage
is 0xFF.

I have no idea why U-Boot expects 0x00 in Johannes' case, though. The
only thing I can think of is that the 'default' byte value on flash
after a page-erase depends on the technology: for NOR, the value after
erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...

But any case, as you said in a further reply, adding that new option
does not add much complexity.

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.
Johannes Schmitz Dec. 13, 2017, 7:57 p.m. | #5
I slightly changed the help text in the last version of the patch. It seems
to actually work with other padding bytes if the size is correct. The
problem was a combination of those two things.
It's really hard to compare with hexdump if not all the bytes are exactly
identical. So setting the padding byte to the same value as uboot save
command uses helped me to finally debug and understand the problem.

There was another suggestion to slightly extend the help text regarding the
size a bit which I yet have to implement.

Regards
Johannes


Am 13.12.2017 7:23 nachm. schrieb "Yann E. MORIN" <yann.morin.1998@free.fr>:

Thomas, Johannes, All,

On 2017-12-12 06:27 +0100, Thomas Petazzoni spake thusly:
> On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:
[--SNIP--]
> > +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> > +   hex "Padding byte"
> > +   default 0x00
> > +   help
> > +     The byte used for padding at the end of the environment image.
>
> I'm still not sure what is the point of customizing the padding byte.
> Does someone has a use-case for that ? If not, I'm not sure it makes
> sense to add a Buildroot option for it.

As far as I understand, this is because the U-Boot on Johannes' board
would expect the padding byte to be 0x00 when the default for mkenvimage
is 0xFF.

I have no idea why U-Boot expects 0x00 in Johannes' case, though. The
only thing I can think of is that the 'default' byte value on flash
after a page-erase depends on the technology: for NOR, the value after
erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...

But any case, as you said in a further reply, adding that new option
does not add much complexity.

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

--
.-----------------.--------------------.------------------.-
-------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___
   |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is
no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v
 conspiracy.  |
'------------------------------^-------^------------------^-
-------------------'
<div dir="auto"><div>I slightly changed the help text in the last version of the patch. It seems to actually work with other padding bytes if the size is correct. The problem was a combination of those two things. <div dir="auto">It&#39;s really hard to compare with hexdump if not all the bytes are exactly identical. So setting the padding byte to the same value as uboot save command uses helped me to finally debug and understand the problem. </div><div dir="auto"><br></div><div dir="auto">There was another suggestion to slightly extend the help text regarding the size a bit which I yet have to implement. </div><div dir="auto"><br></div><div dir="auto">Regards </div><div dir="auto">Johannes </div><br><div class="gmail_extra"><br><div class="gmail_quote">Am 13.12.2017 7:23 nachm. schrieb &quot;Yann E. MORIN&quot; &lt;<a href="mailto:yann.morin.1998@free.fr">yann.morin.1998@free.fr</a>&gt;:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thomas, Johannes, All,<br>
<br>
On 2017-12-12 06:27 +0100, Thomas Petazzoni spake thusly:<br>
<div class="quoted-text">&gt; On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:<br>
</div>[--SNIP--]<br>
<div class="quoted-text">&gt; &gt; +config BR2_TARGET_UBOOT_ENVIMAGE_<wbr>PADDING_BYTE<br>
&gt; &gt; +   hex &quot;Padding byte&quot;<br>
&gt; &gt; +   default 0x00<br>
&gt; &gt; +   help<br>
&gt; &gt; +     The byte used for padding at the end of the environment image.<br>
&gt;<br>
&gt; I&#39;m still not sure what is the point of customizing the padding byte.<br>
&gt; Does someone has a use-case for that ? If not, I&#39;m not sure it makes<br>
&gt; sense to add a Buildroot option for it.<br>
<br>
</div>As far as I understand, this is because the U-Boot on Johannes&#39; board<br>
would expect the padding byte to be 0x00 when the default for mkenvimage<br>
is 0xFF.<br>
<br>
I have no idea why U-Boot expects 0x00 in Johannes&#39; case, though. The<br>
only thing I can think of is that the &#39;default&#39; byte value on flash<br>
after a page-erase depends on the technology: for NOR, the value after<br>
erase ix 0x00, while for NAND, it is 0xFF. So maybe that&#39;s the reason...<br>
<br>
But any case, as you said in a further reply, adding that new option<br>
does not add much complexity.<br>
<br>
Reviewed-by: &quot;Yann E. MORIN&quot; &lt;<a href="mailto:yann.morin.1998@free.fr">yann.morin.1998@free.fr</a>&gt;<br>
<br>
Regards,<br>
Yann E. MORIN.<br>
<font color="#888888"><br>
--<br>
.-----------------.-----------<wbr>---------.------------------.-<wbr>-------------------.<br>
|  Yann E. MORIN  | Real-Time Embedded | /&quot;\ ASCII RIBBON | Erics&#39; conspiracy: |<br>
| <a href="tel:%2B33%20662%20376%20056" value="+33662376056">+33 662 376 056</a> | Software  Designer | \ / CAMPAIGN     |  ___               |<br>
| <a href="tel:%2B33%20223%20225%20172" value="+33223225172">+33 223 225 172</a> `------------.-------:  X  AGAINST      |  \e/  There is no  |<br>
| <a href="http://ymorin.is-a-geek.org/" rel="noreferrer" target="_blank">http://ymorin.is-a-geek.org/</a> | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |<br>
&#39;-----------------------------<wbr>-^-------^------------------^-<wbr>-------------------&#39;<br>
</font></blockquote></div><br></div></div></div>
Thomas Petazzoni Dec. 14, 2017, 6:13 a.m. | #6
Hello,

On Wed, 13 Dec 2017 19:23:32 +0100, Yann E. MORIN wrote:

> > I'm still not sure what is the point of customizing the padding byte.
> > Does someone has a use-case for that ? If not, I'm not sure it makes
> > sense to add a Buildroot option for it.  
> 
> As far as I understand, this is because the U-Boot on Johannes' board
> would expect the padding byte to be 0x00 when the default for mkenvimage
> is 0xFF.

No, U-Boot does not care at all what the padding byte is. Johannes
board works just fine with the default padding byte of 0xff. All what
matters for U-Boot is that the CRC is correct.

Obviously if your environment image has a size of X bytes, its CRC is
calculated over X bytes. Then if the U-Boot configuration says the
environment has Y bytes, it calculates the CRC on Y bytes. So if X !=
Y, you're screwed, as the CRCs won't match.

Johannes problem was *not* a padding byte problem, but *only* a wrong
environment size.

> I have no idea why U-Boot expects 0x00 in Johannes' case, though. 

It does not. Johannes' U-Boot works fine with 0xff as the padding byte.

> The only thing I can think of is that the 'default' byte value on flash
> after a page-erase depends on the technology: for NOR, the value after
> erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...
> 
> But any case, as you said in a further reply, adding that new option
> does not add much complexity.

But it serves no purpose whatsoever :-)

Thomas
Johannes Schmitz Dec. 14, 2017, 7:06 a.m. | #7
Am 14.12.2017 7:13 vorm. schrieb "Thomas Petazzoni" <
thomas.petazzoni@free-electrons.com>:

Hello,

On Wed, 13 Dec 2017 19:23:32 +0100, Yann E. MORIN wrote:

>
> But any case, as you said in a further reply, adding that new option
> does not add much complexity.

But it serves no purpose whatsoever :-)


I disagree with this but maybe it's my electrical engineering background, I
always want full control down to the physical layer.
And mostly there is a use case even if I don't know all of them myself.

I have a question though, is 32k the default uboot environment size? Then I
could also set this as a default. Otherwise again it is a good argument for
keeping the padding option in for debugging  purposes.

Regards
Johannes
<div dir="auto"><div><br><div class="gmail_extra"><br><div class="gmail_quote">Am 14.12.2017 7:13 vorm. schrieb &quot;Thomas Petazzoni&quot; &lt;<a href="mailto:thomas.petazzoni@free-electrons.com">thomas.petazzoni@free-electrons.com</a>&gt;:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello,<br>
<div class="quoted-text"><br>
On Wed, 13 Dec 2017 19:23:32 +0100, Yann E. MORIN wrote:<br>
<br></div><div class="quoted-text">
&gt;<br>
&gt; But any case, as you said in a further reply, adding that new option<br>
&gt; does not add much complexity.<br>
<br>
</div>But it serves no purpose whatsoever :-)</blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">I disagree with this but maybe it&#39;s my electrical engineering background, I always want full control down to the physical layer. </div><div dir="auto">And mostly there is a use case even if I don&#39;t know all of them myself. </div><div dir="auto"><br></div><div dir="auto">I have a question though, is 32k the default uboot environment size? Then I could also set this as a default. Otherwise again it is a good argument for keeping the padding option in for debugging  purposes. </div><div dir="auto"><br></div><div dir="auto">Regards </div><div dir="auto">Johannes </div><div dir="auto"></div></div>
Thomas Petazzoni Dec. 14, 2017, 7:21 a.m. | #8
Hello,

On Thu, 14 Dec 2017 08:06:13 +0100, Johannes Schmitz wrote:

> I disagree with this but maybe it's my electrical engineering background, I
> always want full control down to the physical layer.
> And mostly there is a use case even if I don't know all of them myself.

If Buildroot starts to support use cases that we don't even understand
when they are needed, then we are going on a very, very slippery road.

> I have a question though, is 32k the default uboot environment size? Then I
> could also set this as a default. Otherwise again it is a good argument for
> keeping the padding option in for debugging  purposes.

No, there is nothing like a default environment size. Each board has
its own value in its U-boot configuration file. 

Thomas
Peter Korsgaard Feb. 3, 2018, 9:01 p.m. | #9
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Thu, 14 Dec 2017 08:06:13 +0100, Johannes Schmitz wrote:

 >> I disagree with this but maybe it's my electrical engineering background, I
 >> always want full control down to the physical layer.
 >> And mostly there is a use case even if I don't know all of them myself.

 > If Buildroot starts to support use cases that we don't even understand
 > when they are needed, then we are going on a very, very slippery road.

I agree. I was marked this patch as rejected in patchwork.

Patch

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 2829d2c..32e283a 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -413,8 +413,17 @@  config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
 config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
 	string "Size of environment"
 	help
-	  Size of envronment, can be prefixed with 0x for hexadecimal
-	  values.
+	  Size of environment in bytes, can be prefixed with 0x for
+	  hexadecimal values. Needs to match exactly for correct CRC
+	  checksum calculation as re-calculated by uboot during each
+	  target boot. If a CRC mismatch occurs during boot the
+	  environment will be ignored.
+
+config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
+	hex "Padding byte"
+	default 0x00
+	help
+	  The byte used for padding at the end of the environment image.
 
 config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT
 	bool "Environment has two copies"
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 3917599..43fdde4 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -256,10 +256,12 @@  define UBOOT_INSTALL_IMAGES_CMDS
 	)
 	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
 		cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
-			$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
-			$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
-			$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
-			-o $(BINARIES_DIR)/uboot-env.bin -)
+			$(HOST_DIR)/bin/mkenvimage \
+				$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
+				$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
+				-p $(BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE) \
+				-s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
+				-o $(BINARIES_DIR)/uboot-env.bin -)
 	$(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
 		$(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
 			-d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \