Skip to contentSkip to conversation

JFFS2 images: bugfixes and enhancements

PATCH In review Andrea Adami 2 participants 12 messages Oct 20, 2013 – Oct 30, 2013 1 message not in the archive

lore
Share & tools
lore
#0 lore raw

cover JFFS2 images: bugfixes and enhancements

The first patch is a bugfix allowing the creation of images with cleanmarkers for devices with NOR flash.
The second and the third ones are fixes for the options passed to sumtool.

After those first patches it is possible to customize again i.e.

EXTRA_IMAGECMD_JFFS2 = "--pad=14680064 -l --erase-block=${JFFS2_ERASEBLOCK}"

In the normal case (NAND flash) only JFFS2_ERASEBLOCK is necessary and having to redeclare the full EXTRA_IMAGECMD_jffs2 seems counter-intuitive and has revealed to be error-prone.

So, the fourth and fifth patches are proposed enhancements to further parametrize for both jffs2 and sum.jffs2 images by using two new optional vars like done for JFFS2_ERASEBLOCK:

JFFS2_ERASEBLOCK = "0x20000"
JFFS2_NOCLEANMARKERS = ""
JFFS2_PADDING = "--pad=14680064"

Andrea Adami (5):
image_types.bbclass: do not force --no-cleanmarkers for jffs2 image_types.bbclass: fix endiannes for sumtool (jffs2 summary) image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2 image_types.bbclass: add JFFS2_NOCLEANMARKERS variable image_types.bbclass: add JFFS2_PADDING variable

 meta/classes/image_types.bbclass | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
Signature · 1 line
1.8.1.5
#1 lore raw

PATCH 1/5 image_types.bbclass: do not force --no-cleanmarkers for jffs2

---
 meta/classes/image_types.bbclass | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +2 -2
index 9ead059..8693a8f 100644
@@ -140,9 +140,9 @@ XZ_COMPRESSION_LEVEL ?= "-e -6"
 XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
-IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -n ${EXTRA_IMAGECMD}"
+IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
 IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-	-o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 -n ${EXTRA_IMAGECMD}"
+	-o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
Signature · 1 line
1.8.1.5
#2 lore raw

PATCH 2/5 image_types.bbclass: fix endiannes for sumtool (jffs2 summary)

In reply to Andrea Adami

For mkfs.jffs2 endianness can be be expressed in the long or short form with the optional size argument:

--little-endian
-l

Strangely the sumtool has a different syntax and does accept the forms:

--littleendian
-l

Prefer the short form valid for both tools.

---
 meta/classes/image_types.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
meta/classes/image_types.bbclass +1 -1
index 8693a8f..b8779e0 100644
@@ -209,7 +209,7 @@ IMAGE_CMD_ubifs = "mkfs.ubifs -r ${IMAGE_ROOTFS} -o ${DEPLOY_DIR_IMAGE}/${IMAGE_
 EXTRA_IMAGECMD = ""
 
 inherit siteinfo
-JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '--little-endian', '--big-endian', d)}"
+JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
Signature · 1 line
1.8.1.5
#3 lore raw

PATCH 3/5 image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

In reply to Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -2
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-	-o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+	&& sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"
Signature · 1 line
1.8.1.5
#4 lore raw

Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

BUMP

I've seen the first two bugfixes have been applied and now it is possible to override EXTRA_IMAGECMD_jffs2.

This patch fixes the case of sum.jffs2 when a padding value is specified because sumtool won't swallow it.

Thx

Andrea

Quoted 38 lines · Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -3
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-       -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+       && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"
--

1.8.1.5

#5 lore raw

Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

In reply to Andrea Adami
Quoted 28 lines · Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +2 -2
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-	-o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+	&& sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 

meta/classes/image_types.bbclass inherit siteinfo · line 218

Quoted 7 lines
meta/classes/image_types.bbclass +1 -0
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"

This patch is very confused. You say sumtool doesn't take a --pad option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with sumtool does have the option.

We need to make this clearer and I don't think this patch does that, I'm not even sure it works...

Cheers,

Richard

#6 lore raw

Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

Quoted 34 lines · Richard Purdie
Quoted 33 lines · Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -2
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-     -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+     && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
Quoted 7 lines

meta/classes/image_types.bbclass inherit siteinfo · line 218

Quoted 2 lines
meta/classes/image_types.bbclass +0 -0
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"

This patch is very confused. You say sumtool doesn't take a --pad option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with sumtool does have the option.

After the commits you've done the creation of sum.jffs2 images is broken on collie.
The error message is described clearly in the patch:

sumtool: option '--pad' doesn't allow an argument

Why that? Because collie needs to customize the padding. EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}"

This --pad=14680064 is then passed to IMAGE_CMD_sum.jffs2 and we get build error.

That's why we need to redefine both IMAGE_CMD_sum.jffs2 and EXTRA_IMAGECMD_sum.jffs2 so to pass --pad=XY only to mkfs.jffs2 and not to the sumtool part of IMAGE_CMD_sum.jffs2.

---
By the way this sum.jffs2 imagetype is used only in meta-handheld afaik. It would be also ok if you'd remove IMAGE_CMD_sum.jffs2 so we can append the whole stuff in a customized EXTRA_IMAGECMD_jffs2.

Interestingly the sum.jffs2 ican not be padded to a desired size and that could cause some issues flashing over older jffs partitions (happened on collie flashing on top of previous jffs format).

In the case of modern devices with NAND there is a clear benefit.

Regards

Andrea

Quoted 7 lines

We need to make this clearer and I don't think this patch does that, I'm not even sure it works...

Cheers,

Richard

#7 lore raw

Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

Quoted 56 lines · Andrea Adami
Quoted 34 lines · Richard Purdie
Quoted 33 lines · Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -2
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-     -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+     && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
Quoted 7 lines

meta/classes/image_types.bbclass inherit siteinfo · line 218

Quoted 2 lines
meta/classes/image_types.bbclass +0 -0
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"

This patch is very confused. You say sumtool doesn't take a --pad option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with sumtool does have the option.

After the commits you've done the creation of sum.jffs2 images is broken on collie.
The error message is described clearly in the patch:

sumtool: option '--pad' doesn't allow an argument

Why that? Because collie needs to customize the padding. EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}"

This --pad=14680064 is then passed to IMAGE_CMD_sum.jffs2 and we get build error.

Quoted 8 lines

That's why we need to redefine both IMAGE_CMD_sum.jffs2 and EXTRA_IMAGECMD_sum.jffs2 so to pass --pad=XY only to mkfs.jffs2 and not to the sumtool part of IMAGE_CMD_sum.jffs2.

---
By the way this sum.jffs2 imagetype is used only in meta-handheld afaik. It would be also ok if you'd remove IMAGE_CMD_sum.jffs2 so we can append the whole stuff in a customized EXTRA_IMAGECMD_jffs2.

I understand your problem however:

a) Copy and pasting the entire IMAGE_CMD_jffs2 command into the second one is rather ugly.
b) You still end up passing "--pad ${JFFS2_ENDIANNESS} --eraseblock= ${JFFS2_ERASEBLOCK} --no-cleanmarkers" as parameters to sumtool, including a --pad option with no argument. Why/how?

You call:

sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}

and EXTRA_IMAGECMD is set to EXTRA_IMAGECMD_sum.jffs2 due to the use of overrides in the image generation code.

So whilst collie is broken, I do not think this patch makes readable code and I think we need to find something better.

The thing I'm confused on is whether there should be any --pad option to sumtool or not.

Cheers,

Richard

A message that is not in the archive (sent off-list or not yet received): CAAQYJAs91R1qH8nvTGG_rSUg_JYcZq-sZgX23tHteyqWfBF4iQ@mail.gmail.com

#9 lore raw

Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

Quoted 99 lines · Andrea Adami
Quoted 66 lines · Richard Purdie
Quoted 56 lines · Andrea Adami
Quoted 34 lines · Richard Purdie
Quoted 33 lines · Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -2
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-     -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+     && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
Quoted 7 lines

meta/classes/image_types.bbclass inherit siteinfo · line 218

Quoted 2 lines
meta/classes/image_types.bbclass +0 -0
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"

This patch is very confused. You say sumtool doesn't take a --pad option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with sumtool does have the option.

After the commits you've done the creation of sum.jffs2 images is broken on collie.
The error message is described clearly in the patch:

sumtool: option '--pad' doesn't allow an argument

Why that? Because collie needs to customize the padding. EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}"

This --pad=14680064 is then passed to IMAGE_CMD_sum.jffs2 and we get build error.

Quoted 8 lines

That's why we need to redefine both IMAGE_CMD_sum.jffs2 and EXTRA_IMAGECMD_sum.jffs2 so to pass --pad=XY only to mkfs.jffs2 and not to the sumtool part of IMAGE_CMD_sum.jffs2.

---
By the way this sum.jffs2 imagetype is used only in meta-handheld afaik. It would be also ok if you'd remove IMAGE_CMD_sum.jffs2 so we can append the whole stuff in a customized EXTRA_IMAGECMD_jffs2.

Quoted 4 lines

I understand your problem however:

a) Copy and pasting the entire IMAGE_CMD_jffs2 command into the second one is rather ugly.

Yes, but using the IMAGE_CMD_jffs2 we incorporate the EXTRA_IMAGECMD_jffs2 = "--pad=xyz.."

Quoted 3 lines

b) You still end up passing "--pad ${JFFS2_ENDIANNESS} --eraseblock= ${JFFS2_ERASEBLOCK} --no-cleanmarkers" as parameters to sumtool, including a --pad option with no argument. Why/how?

Because this padding is limited to the last eraseblock.
" -p, --pad Pad the OUTPUT with 0xFF to the end of the
final eraseblock"

Quoted 7 lines

You call:

sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}

and EXTRA_IMAGECMD is set to EXTRA_IMAGECMD_sum.jffs2 due to the use of overrides in the image generation code.

We cannot reuse IMAGE_CMD_jffs2 because in that case EXTRA_IMAGECMD_jffs2 is already evaluated and this contains the infamous --pad==xy.

Quoted 4 lines

So whilst collie is broken, I do not think this patch makes readable code and I think we need to find something better.

Quoted 7 lines
Quoted 3 lines

The thing I'm confused on is whether there should be any --pad option to sumtool or not.

Yes, but without <size> argument

http://git.infradead.org/mtd-utils.git/blob/HEAD:/sumtool.c

Ok, we need to make this clearer in the commit message. Looking and thinking about this further, how about we add some dependency ordering into this code, something like the patch below. This moves the jffs2 call into its own true namespace. You should then just be able to explicitly set EXTRA_IMGAGECMD_sum.jffs2 to the value you need (yet still have EXTRA_IMGAGECMD_jffs2 able to take a different value).

See if you can build something on top of the patch below...

Cheers,

Richard

From 4ff883824221ce5f1b993239249b1ce1e3ab9b32 Mon Sep 17 00:00:00 2001
From: Richard Purdie <[email protected]>
Date: Tue, 29 Oct 2013 15:26:54 +0000
Subject: image_types: Improve dependency handling between types (and use for sum.jffs2)

We're seeing a pattern of one image type needing to depend on another type. A good example is jffs2 and sum.jffs2. This patch makes sum.jffs2 depend on jffs2 which will then allow a EXTRA_IMGAGECMD to be set for sum.jffs2 individually without changing the jffs2 command. This allows the -pad option to be configured differently.

---

meta/classes/image_types.bbclass +12 -7
index b8779e0..5ce1ddb 100644
@@ -7,14 +7,20 @@ def get_imagecmds(d):
     ctypes = d.getVar('COMPRESSIONTYPES', True).split()
     cimages = {}
 
+    # Image type b depends on a having been generated first
+    def addtypedepends(a, b):
+        if a in alltypes:
+            alltypes.remove(a):
+            if b not in alltypes:
+                alltypes.append(b)
+            alltypes.append(a)
+
     # The elf image depends on the cpio.gz image already having
     # been created, so we add that explicit ordering here.
+    addtypedepends("elf", "cpio.gz")
 
-    if "elf" in alltypes:
-        alltypes.remove("elf")
-        if "cpio.gz" not in alltypes:
-                alltypes.append("cpio.gz")
-        alltypes.append("elf")
+    # jffs2 sumtool'd images need jffs2
+    addtypedepends("sum.jffs2", "jffs2")
 
     # Filter out all the compressed images from alltypes
     for type in alltypes:
@@ -141,8 +147,7 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-	-o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
author Acked-by
#10 lore raw

Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

Quoted 171 lines · Richard Purdie
Quoted 99 lines · Andrea Adami
Quoted 66 lines · Richard Purdie
Quoted 56 lines · Andrea Adami
Quoted 34 lines · Richard Purdie
Quoted 33 lines · Andrea Adami

When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..." we are passing a malformed option to sumtool:

sumtool: option '--pad' doesn't allow an argument

Fix this by declaring a separate variable for the purpose.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -2
index b8779e0..21391e8 100644
@@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-     -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
+     && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 
@@ -212,6 +212,7 @@ inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
 EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
 
Quoted 7 lines

meta/classes/image_types.bbclass inherit siteinfo · line 218

Quoted 2 lines
meta/classes/image_types.bbclass +0 -0
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"

This patch is very confused. You say sumtool doesn't take a --pad option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with sumtool does have the option.

After the commits you've done the creation of sum.jffs2 images is broken on collie.
The error message is described clearly in the patch:

sumtool: option '--pad' doesn't allow an argument

Why that? Because collie needs to customize the padding. EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}"

This --pad=14680064 is then passed to IMAGE_CMD_sum.jffs2 and we get build error.

Quoted 8 lines

That's why we need to redefine both IMAGE_CMD_sum.jffs2 and EXTRA_IMAGECMD_sum.jffs2 so to pass --pad=XY only to mkfs.jffs2 and not to the sumtool part of IMAGE_CMD_sum.jffs2.

---
By the way this sum.jffs2 imagetype is used only in meta-handheld afaik. It would be also ok if you'd remove IMAGE_CMD_sum.jffs2 so we can append the whole stuff in a customized EXTRA_IMAGECMD_jffs2.

Quoted 4 lines

I understand your problem however:

a) Copy and pasting the entire IMAGE_CMD_jffs2 command into the second one is rather ugly.

Yes, but using the IMAGE_CMD_jffs2 we incorporate the EXTRA_IMAGECMD_jffs2 = "--pad=xyz.."

Quoted 3 lines

b) You still end up passing "--pad ${JFFS2_ENDIANNESS} --eraseblock= ${JFFS2_ERASEBLOCK} --no-cleanmarkers" as parameters to sumtool, including a --pad option with no argument. Why/how?

Because this padding is limited to the last eraseblock.
" -p, --pad Pad the OUTPUT with 0xFF to the end of the
final eraseblock"

Quoted 7 lines

You call:

sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}

and EXTRA_IMAGECMD is set to EXTRA_IMAGECMD_sum.jffs2 due to the use of overrides in the image generation code.

We cannot reuse IMAGE_CMD_jffs2 because in that case EXTRA_IMAGECMD_jffs2 is already evaluated and this contains the infamous --pad==xy.

Quoted 4 lines

So whilst collie is broken, I do not think this patch makes readable code and I think we need to find something better.

Quoted 7 lines
Quoted 3 lines

The thing I'm confused on is whether there should be any --pad option to sumtool or not.

Yes, but without <size> argument

http://git.infradead.org/mtd-utils.git/blob/HEAD:/sumtool.c

Ok, we need to make this clearer in the commit message. Looking and thinking about this further, how about we add some dependency ordering into this code, something like the patch below. This moves the jffs2 call into its own true namespace. You should then just be able to explicitly set EXTRA_IMGAGECMD_sum.jffs2 to the value you need (yet still have EXTRA_IMGAGECMD_jffs2 able to take a different value).

See if you can build something on top of the patch below...

Cheers,

Richard

From 4ff883824221ce5f1b993239249b1ce1e3ab9b32 Mon Sep 17 00:00:00 2001
From: Richard Purdie <[email protected]>
Date: Tue, 29 Oct 2013 15:26:54 +0000
Subject: image_types: Improve dependency handling between types (and use for sum.jffs2)

We're seeing a pattern of one image type needing to depend on another type. A good example is jffs2 and sum.jffs2. This patch makes sum.jffs2 depend on jffs2 which will then allow a EXTRA_IMGAGECMD to be set for sum.jffs2 individually without changing the jffs2 command. This allows the -pad option to be configured differently.

---

meta/classes/image_types.bbclass +11 -5
index b8779e0..5ce1ddb 100644
@@ -7,14 +7,20 @@ def get_imagecmds(d):
     ctypes = d.getVar('COMPRESSIONTYPES', True).split()
     cimages = {}
 
+    # Image type b depends on a having been generated first
+    def addtypedepends(a, b):
+        if a in alltypes:
+            alltypes.remove(a):
+            if b not in alltypes:
+                alltypes.append(b)
+            alltypes.append(a)
+
     # The elf image depends on the cpio.gz image already having
     # been created, so we add that explicit ordering here.
+    addtypedepends("elf", "cpio.gz")
 
-    if "elf" in alltypes:
-        alltypes.remove("elf")
-        if "cpio.gz" not in alltypes:
-                alltypes.append("cpio.gz")
-        alltypes.append("elf")
+    # jffs2 sumtool'd images need jffs2
+    addtypedepends("sum.jffs2", "jffs2")
 
     # Filter out all the compressed images from alltypes
     for type in alltypes:

meta/classes/image_types.bbclass XZ_INTEGRITY_CHECK ?= "crc32" · line 153

Quoted 9 lines
meta/classes/image_types.bbclass +1 -2
@@ -141,8 +147,7 @@ XZ_INTEGRITY_CHECK ?= "crc32"
 XZ_THREADS ?= "-T 0"
 
 IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
-IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
-       -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
+IMAGE_CMD_sum.jffs2 = "sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
 
 IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
 

Richard,

your proposed solution does work as expected and I'm able to set in the specific case (collie.conf):

EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}" EXTRA_IMAGECMD_sum.jffs2 = "-p -l -e ${JFFS2_ERASEBLOCK}"

The run.do_rootfs shows the correct commands are used:

mkfs.jffs2 --root=/oe/oe-core/build/tmp-eglibc/work/collie-oe-linux-gnueabi/core-image-base/1.0-r0/rootfs --faketime --output=/oe/oe-core/build/tmp-eglibc/deploy/images/collie/core-image-base-collie-20131030212928.rootfs.jffs2 --pad=14680064 -l -e 0x20000

sumtool -i /oe/oe-core/build/tmp-eglibc/deploy/images/collie/core-image-base-collie-20131030212928.rootfs.jffs2 -o /oe/oe-core/build/tmp-eglibc/deploy/images/collie/core-image-base-collie-20131030212928.rootfs.sum.jffs2 -p -l -e 0x20000

Thanks

Andrea

#11 lore raw

PATCH 4/5 image_types.bbclass: add JFFS2_NOCLEANMARKERS variable

In reply to Andrea Adami

The '--no-clenmarkers' value is the actual default and is valid for devices with NAND flash.

Machine configuration files can redefine it instead of rewriting EXTRA_IMAGECMD_JFFS2.

---
 meta/classes/image_types.bbclass | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
meta/classes/image_types.bbclass +3 -2
index 21391e8..372420d 100644
@@ -211,8 +211,9 @@ EXTRA_IMAGECMD = ""
 inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
 JFFS2_ERASEBLOCK ?= "0x40000"
-EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
-EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
+JFFS2_NOCLEANMARKERS ?= "--no-cleanmarkers"
+EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} ${JFFS2_NOCLEANMARKERS}"
+EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} ${JFFS2_NOCLEANMARKERS}"
 
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
 EXTRA_IMAGECMD_ext2 ?= "-i 8192"
Signature · 1 line
1.8.1.5
#12 lore raw

PATCH 5/5 image_types.bbclass: add JFFS2_PADDING variable

In reply to Andrea Adami

Defaulting to '--pad', meaning to pad until the end of the erasesector.

Allow machine.conf to override the value without redeclaring EXTRA_IMAGECMD_jffs2.

---
 meta/classes/image_types.bbclass | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
meta/classes/image_types.bbclass +2 -1
index 372420d..c5bacd8 100644
@@ -210,9 +210,10 @@ EXTRA_IMAGECMD = ""
 
 inherit siteinfo
 JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
+JFFS2_PADDING ?= "--pad"
 JFFS2_ERASEBLOCK ?= "0x40000"
 JFFS2_NOCLEANMARKERS ?= "--no-cleanmarkers"
-EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} ${JFFS2_NOCLEANMARKERS}"
+EXTRA_IMAGECMD_jffs2 ?= "${JFFS2_PADDING} ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} ${JFFS2_NOCLEANMARKERS}"
 EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} ${JFFS2_NOCLEANMARKERS}"
 
 # Change these if you want default mkfs behavior (i.e. create minimal inode number)
Signature · 1 line
1.8.1.5