[PATCH 0/5] MTD: bcm63xxpart: improve robustness

[PATCH 0/5] MTD: bcm63xxpart: improve robustness

[Jonas Gorski]

This patchset aims at improving the robustness of the CFE detection and
image tag validation and parsing.

The image tag parsing improvement is especially useful when booting a
ramdisk image through tftp, since there is no guarantee that the image
on the flash is valid at this point.

As usual this patchset should got through the MTD tree and is based on
the l2-mtd-2.6 git.

Jonas Gorski (5):
  MTD: bcm63xxpart: check version marker string for newer CFEs
  MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K
  MTD: bcm63xxpart: don't assume NVRAM is always the fourth partition
  MIPS: BCM63XX: bcm963xx_tag.h: make crc fields integers
  MTD: bcm63xxpart: check the image tag's crc32

 arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h |   11 +--
 drivers/mtd/bcm63xxpart.c                         |   89 ++++++++++++++-------
 2 files changed, 66 insertions(+), 34 deletions(-)

-- 
1.7.2.5

[PATCH 0/5] MTD: bcm63xxpart: improve robustness

[Jonas Gorski]

This patchset aims at improving the robustness of the CFE detection and
image tag validation and parsing.

The image tag parsing improvement is especially useful when booting a
ramdisk image through tftp, since there is no guarantee that the image
on the flash is valid at this point.

As usual this patchset should got through the MTD tree and is based on
the l2-mtd-2.6 git.

Jonas Gorski (5):
  MTD: bcm63xxpart: check version marker string for newer CFEs
  MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K
  MTD: bcm63xxpart: don't assume NVRAM is always the fourth partition
  MIPS: BCM63XX: bcm963xx_tag.h: make crc fields integers
  MTD: bcm63xxpart: check the image tag's crc32

 arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h |   11 +--
 drivers/mtd/bcm63xxpart.c                         |   89 ++++++++++++++-------
 2 files changed, 66 insertions(+), 34 deletions(-)

-- 
1.7.2.5

[PATCH 1/5] MTD: bcm63xxpart: check version marker string for newer CFEs

[Jonas Gorski]

Recent CFEs do not contain the CFE1CFE1 magic anymore, so check for the
"cfe-v" version marker string instead. As very old CFEs do not have
this string, leave the CFE1CFE1 magic as a fallback for detection.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index ac7d3c8..9933b34 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -32,22 +32,34 @@
 #include <linux/mtd/partitions.h>
 
 #include <asm/mach-bcm63xx/bcm963xx_tag.h>
+#include <asm/mach-bcm63xx/board_bcm963xx.h>
 
 #define BCM63XX_EXTENDED_SIZE	0xBFC00000	/* Extended flash address */
 
+#define BCM63XX_CFE_MAGIC_OFFSET 0x4e0
+
 static int bcm63xx_detect_cfe(struct mtd_info *master)
 {
-	int idoffset = 0x4e0;
-	static char idstring[8] = "CFE1CFE1";
 	char buf[9];
 	int ret;
 	size_t retlen;
 
-	ret = master->read(master, idoffset, 8, &retlen, (void *)buf);
+	ret = master->read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen,
+			   (void *)buf);
+	buf[retlen] = 0;
+
+	if (ret)
+		return ret;
+
+	if (strncmp("cfe-v", buf, 5) == 0)
+		return 0;
+
+	/* very old CFE's do not have the cfe-v string, so check for magic */
+	ret = master->read(master, BCM63XX_CFE_MAGIC_OFFSET, 8, &retlen,
+			   (void *)buf);
 	buf[retlen] = 0;
-	pr_info("Read Signature value of %s\n", buf);
 
-	return strncmp(idstring, buf, 8);
+	return strncmp("CFE1CFE1", buf, 8);
 }
 
 static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
-- 
1.7.2.5

[PATCH 1/5] MTD: bcm63xxpart: check version marker string for newer CFEs

[Jonas Gorski]

Recent CFEs do not contain the CFE1CFE1 magic anymore, so check for the
"cfe-v" version marker string instead. As very old CFEs do not have
this string, leave the CFE1CFE1 magic as a fallback for detection.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index ac7d3c8..9933b34 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -32,22 +32,34 @@
 #include <linux/mtd/partitions.h>
 
 #include <asm/mach-bcm63xx/bcm963xx_tag.h>
+#include <asm/mach-bcm63xx/board_bcm963xx.h>
 
 #define BCM63XX_EXTENDED_SIZE	0xBFC00000	/* Extended flash address */
 
+#define BCM63XX_CFE_MAGIC_OFFSET 0x4e0
+
 static int bcm63xx_detect_cfe(struct mtd_info *master)
 {
-	int idoffset = 0x4e0;
-	static char idstring[8] = "CFE1CFE1";
 	char buf[9];
 	int ret;
 	size_t retlen;
 
-	ret = master->read(master, idoffset, 8, &retlen, (void *)buf);
+	ret = master->read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen,
+			   (void *)buf);
+	buf[retlen] = 0;
+
+	if (ret)
+		return ret;
+
+	if (strncmp("cfe-v", buf, 5) == 0)
+		return 0;
+
+	/* very old CFE's do not have the cfe-v string, so check for magic */
+	ret = master->read(master, BCM63XX_CFE_MAGIC_OFFSET, 8, &retlen,
+			   (void *)buf);
 	buf[retlen] = 0;
-	pr_info("Read Signature value of %s\n", buf);
 
-	return strncmp(idstring, buf, 8);
+	return strncmp("CFE1CFE1", buf, 8);
 }
 
 static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
-- 
1.7.2.5

[PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

The CFE and NVRAM are always at least 64K big, so make sure their
partitions are never smaller than this in case the flash has smaller
erase sectors.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 9933b34..23f6201 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -36,6 +36,9 @@
 
 #define BCM63XX_EXTENDED_SIZE	0xBFC00000	/* Extended flash address */
 
+#define BCM63XX_MIN_CFE_SIZE	0x10000		/* always at least 64K */
+#define BCM63XX_MIN_NVRAM_SIZE	0x10000		/* always at least 64K */
+
 #define BCM63XX_CFE_MAGIC_OFFSET 0x4e0
 
 static int bcm63xx_detect_cfe(struct mtd_info *master)
@@ -74,6 +77,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	size_t retlen;
 	unsigned int rootfsaddr, kerneladdr, spareaddr;
 	unsigned int rootfslen, kernellen, sparelen, totallen;
+	unsigned int cfelen, nvramlen;
 	int namelen = 0;
 	int i;
 	char *boardid;
@@ -82,14 +86,18 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	if (bcm63xx_detect_cfe(master))
 		return -EINVAL;
 
+	cfelen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_CFE_SIZE);
+	nvramlen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_NVRAM_SIZE);
+
 	/* Allocate memory for buffer */
 	buf = vmalloc(sizeof(struct bcm_tag));
 	if (!buf)
 		return -ENOMEM;
 
 	/* Get the tag */
-	ret = master->read(master, master->erasesize, sizeof(struct bcm_tag),
-							&retlen, (void *)buf);
+	ret = master->read(master, cfelen, sizeof(struct bcm_tag), &retlen,
+			   (void *)buf);
+
 	if (retlen != sizeof(struct bcm_tag)) {
 		vfree(buf);
 		return -EIO;
@@ -106,8 +114,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 
 	kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
 	rootfsaddr = kerneladdr + kernellen;
-	spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
-	sparelen = master->size - spareaddr - master->erasesize;
+	spareaddr = roundup(totallen, master->erasesize) + cfelen;
+	sparelen = master->size - spareaddr - nvramlen;
 	rootfslen = spareaddr - rootfsaddr;
 
 	/* Determine number of partitions */
@@ -131,7 +139,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	/* Start building partition list */
 	parts[curpart].name = "CFE";
 	parts[curpart].offset = 0;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].size = cfelen;
 	curpart++;
 
 	if (kernellen > 0) {
@@ -151,8 +159,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	}
 
 	parts[curpart].name = "nvram";
-	parts[curpart].offset = master->size - master->erasesize;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].offset = master->size - nvramlen;
+	parts[curpart].size = nvramlen;
 
 	/* Global partition "linux" to make easy firmware upgrade */
 	curpart++;
-- 
1.7.2.5

[PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

The CFE and NVRAM are always at least 64K big, so make sure their
partitions are never smaller than this in case the flash has smaller
erase sectors.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 9933b34..23f6201 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -36,6 +36,9 @@
 
 #define BCM63XX_EXTENDED_SIZE	0xBFC00000	/* Extended flash address */
 
+#define BCM63XX_MIN_CFE_SIZE	0x10000		/* always at least 64K */
+#define BCM63XX_MIN_NVRAM_SIZE	0x10000		/* always at least 64K */
+
 #define BCM63XX_CFE_MAGIC_OFFSET 0x4e0
 
 static int bcm63xx_detect_cfe(struct mtd_info *master)
@@ -74,6 +77,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	size_t retlen;
 	unsigned int rootfsaddr, kerneladdr, spareaddr;
 	unsigned int rootfslen, kernellen, sparelen, totallen;
+	unsigned int cfelen, nvramlen;
 	int namelen = 0;
 	int i;
 	char *boardid;
@@ -82,14 +86,18 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	if (bcm63xx_detect_cfe(master))
 		return -EINVAL;
 
+	cfelen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_CFE_SIZE);
+	nvramlen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_NVRAM_SIZE);
+
 	/* Allocate memory for buffer */
 	buf = vmalloc(sizeof(struct bcm_tag));
 	if (!buf)
 		return -ENOMEM;
 
 	/* Get the tag */
-	ret = master->read(master, master->erasesize, sizeof(struct bcm_tag),
-							&retlen, (void *)buf);
+	ret = master->read(master, cfelen, sizeof(struct bcm_tag), &retlen,
+			   (void *)buf);
+
 	if (retlen != sizeof(struct bcm_tag)) {
 		vfree(buf);
 		return -EIO;
@@ -106,8 +114,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 
 	kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
 	rootfsaddr = kerneladdr + kernellen;
-	spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
-	sparelen = master->size - spareaddr - master->erasesize;
+	spareaddr = roundup(totallen, master->erasesize) + cfelen;
+	sparelen = master->size - spareaddr - nvramlen;
 	rootfslen = spareaddr - rootfsaddr;
 
 	/* Determine number of partitions */
@@ -131,7 +139,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	/* Start building partition list */
 	parts[curpart].name = "CFE";
 	parts[curpart].offset = 0;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].size = cfelen;
 	curpart++;
 
 	if (kernellen > 0) {
@@ -151,8 +159,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	}
 
 	parts[curpart].name = "nvram";
-	parts[curpart].offset = master->size - master->erasesize;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].offset = master->size - nvramlen;
+	parts[curpart].size = nvramlen;
 
 	/* Global partition "linux" to make easy firmware upgrade */
 	curpart++;
-- 
1.7.2.5

[PATCH 3/5] MTD: bcm63xxpart: don't assume NVRAM is always the fourth partition

[Jonas Gorski]

Instead of referencing the sizes of fixed partitions, use the
precomputed CFE/NVRAM lengths.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 23f6201..3becb4d 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -165,8 +165,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	/* Global partition "linux" to make easy firmware upgrade */
 	curpart++;
 	parts[curpart].name = "linux";
-	parts[curpart].offset = parts[0].size;
-	parts[curpart].size = master->size - parts[0].size - parts[3].size;
+	parts[curpart].offset = cfelen;
+	parts[curpart].size = master->size - cfelen - nvramlen;
 
 	for (i = 0; i < nrparts; i++)
 		pr_info("Partition %d is %s offset %lx and length %lx\n", i,
-- 
1.7.2.5

[PATCH 3/5] MTD: bcm63xxpart: don't assume NVRAM is always the fourth partition

[Jonas Gorski]

Instead of referencing the sizes of fixed partitions, use the
precomputed CFE/NVRAM lengths.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 23f6201..3becb4d 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -165,8 +165,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	/* Global partition "linux" to make easy firmware upgrade */
 	curpart++;
 	parts[curpart].name = "linux";
-	parts[curpart].offset = parts[0].size;
-	parts[curpart].size = master->size - parts[0].size - parts[3].size;
+	parts[curpart].offset = cfelen;
+	parts[curpart].size = master->size - cfelen - nvramlen;
 
 	for (i = 0; i < nrparts; i++)
 		pr_info("Partition %d is %s offset %lx and length %lx\n", i,
-- 
1.7.2.5

[PATCH 4/5] MIPS: BCM63XX: bcm963xx_tag.h: make crc fields integers

[Jonas Gorski]

All CRC32 fields are 32 bit integers, so define them as such to prevent
unnecessary casts if we want to use them.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---

I decided for __u32 against u32 so that user space applications can still
use the header (like image creation utilities).

 arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h b/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h
index ed72e6a..1e6b587 100644
--- a/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h
+++ b/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h
@@ -16,7 +16,6 @@
 #define TAGINFO1_LEN		30	/* Length of vendor information field1 in tag */
 #define FLASHLAYOUTVER_LEN	4	/* Length of Flash Layout Version String tag */
 #define TAGINFO2_LEN		16	/* Length of vendor information field2 in tag */
-#define CRC_LEN			4	/* Length of CRC in bytes */
 #define ALTTAGINFO_LEN		54	/* Alternate length for vendor information; Pirelli */
 
 #define NUM_PIRELLI		2
@@ -77,19 +76,19 @@ struct bcm_tag {
 	/* 192-195: Version flash layout */
 	char flash_layout_ver[FLASHLAYOUTVER_LEN];
 	/* 196-199: kernel+rootfs CRC32 */
-	char fskernel_crc[CRC_LEN];
+	__u32 fskernel_crc;
 	/* 200-215: Unused except on Alice Gate where is is information */
 	char information2[TAGINFO2_LEN];
 	/* 216-219: CRC32 of image less imagetag (kernel for Alice Gate) */
-	char image_crc[CRC_LEN];
+	__u32 image_crc;
 	/* 220-223: CRC32 of rootfs partition */
-	char rootfs_crc[CRC_LEN];
+	__u32 rootfs_crc;
 	/* 224-227: CRC32 of kernel partition */
-	char kernel_crc[CRC_LEN];
+	__u32 kernel_crc;
 	/* 228-235: Unused at present */
 	char reserved1[8];
 	/* 236-239: CRC32 of header excluding last 20 bytes */
-	char header_crc[CRC_LEN];
+	__u32 header_crc;
 	/* 240-255: Unused at present */
 	char reserved2[16];
 };
-- 
1.7.2.5

[PATCH 4/5] MIPS: BCM63XX: bcm963xx_tag.h: make crc fields integers

[Jonas Gorski]

All CRC32 fields are 32 bit integers, so define them as such to prevent
unnecessary casts if we want to use them.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---

I decided for __u32 against u32 so that user space applications can still
use the header (like image creation utilities).

 arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h b/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h
index ed72e6a..1e6b587 100644
--- a/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h
+++ b/arch/mips/include/asm/mach-bcm63xx/bcm963xx_tag.h
@@ -16,7 +16,6 @@
 #define TAGINFO1_LEN		30	/* Length of vendor information field1 in tag */
 #define FLASHLAYOUTVER_LEN	4	/* Length of Flash Layout Version String tag */
 #define TAGINFO2_LEN		16	/* Length of vendor information field2 in tag */
-#define CRC_LEN			4	/* Length of CRC in bytes */
 #define ALTTAGINFO_LEN		54	/* Alternate length for vendor information; Pirelli */
 
 #define NUM_PIRELLI		2
@@ -77,19 +76,19 @@ struct bcm_tag {
 	/* 192-195: Version flash layout */
 	char flash_layout_ver[FLASHLAYOUTVER_LEN];
 	/* 196-199: kernel+rootfs CRC32 */
-	char fskernel_crc[CRC_LEN];
+	__u32 fskernel_crc;
 	/* 200-215: Unused except on Alice Gate where is is information */
 	char information2[TAGINFO2_LEN];
 	/* 216-219: CRC32 of image less imagetag (kernel for Alice Gate) */
-	char image_crc[CRC_LEN];
+	__u32 image_crc;
 	/* 220-223: CRC32 of rootfs partition */
-	char rootfs_crc[CRC_LEN];
+	__u32 rootfs_crc;
 	/* 224-227: CRC32 of kernel partition */
-	char kernel_crc[CRC_LEN];
+	__u32 kernel_crc;
 	/* 228-235: Unused at present */
 	char reserved1[8];
 	/* 236-239: CRC32 of header excluding last 20 bytes */
-	char header_crc[CRC_LEN];
+	__u32 header_crc;
 	/* 240-255: Unused at present */
 	char reserved2[16];
 };
-- 
1.7.2.5

[PATCH 5/5] MTD: bcm63xxpart: check the image tag's crc32

[Jonas Gorski]

Only use the values from the image tag if it is valid. Always create
the CFE, NVRAM and linux partitions, to allow flashing a new image even
if the old is invalid without overwriting CFE or NVRAM.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |   45 +++++++++++++++++++++++++++++----------------
 1 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 3becb4d..0d5fecf 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -24,6 +24,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/crc32.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -80,8 +81,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	unsigned int cfelen, nvramlen;
 	int namelen = 0;
 	int i;
-	char *boardid;
-	char *tagversion;
+	u32 computed_crc;
 
 	if (bcm63xx_detect_cfe(master))
 		return -EINVAL;
@@ -103,20 +103,33 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 		return -EIO;
 	}
 
-	sscanf(buf->kernel_address, "%u", &kerneladdr);
-	sscanf(buf->kernel_length, "%u", &kernellen);
-	sscanf(buf->total_length, "%u", &totallen);
-	tagversion = &(buf->tag_version[0]);
-	boardid = &(buf->board_id[0]);
-
-	pr_info("CFE boot tag found with version %s and board type %s\n",
-		tagversion, boardid);
-
-	kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
-	rootfsaddr = kerneladdr + kernellen;
-	spareaddr = roundup(totallen, master->erasesize) + cfelen;
-	sparelen = master->size - spareaddr - nvramlen;
-	rootfslen = spareaddr - rootfsaddr;
+	computed_crc = crc32_le(IMAGETAG_CRC_START, (u8 *)buf,
+				offsetof(struct bcm_tag, header_crc));
+	if (computed_crc == buf->header_crc) {
+		char *boardid = &(buf->board_id[0]);
+		char *tagversion = &(buf->tag_version[0]);
+
+		sscanf(buf->kernel_address, "%u", &kerneladdr);
+		sscanf(buf->kernel_length, "%u", &kernellen);
+		sscanf(buf->total_length, "%u", &totallen);
+
+		pr_info("CFE boot tag found with version %s and board type %s\n",
+			tagversion, boardid);
+
+		kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
+		rootfsaddr = kerneladdr + kernellen;
+		spareaddr = roundup(totallen, master->erasesize) + cfelen;
+		sparelen = master->size - spareaddr - nvramlen;
+		rootfslen = spareaddr - rootfsaddr;
+	} else {
+		pr_warn("CFE boot tag CRC invalid (expected %08x, actual %08x)\n",
+			buf->header_crc, computed_crc);
+		kernellen = 0;
+		rootfslen = 0;
+		rootfsaddr = 0;
+		spareaddr = cfelen;
+		sparelen = master->size - cfelen - nvramlen;
+	}
 
 	/* Determine number of partitions */
 	namelen = 8;
-- 
1.7.2.5

[PATCH 5/5] MTD: bcm63xxpart: check the image tag's crc32

[Jonas Gorski]

Only use the values from the image tag if it is valid. Always create
the CFE, NVRAM and linux partitions, to allow flashing a new image even
if the old is invalid without overwriting CFE or NVRAM.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/mtd/bcm63xxpart.c |   45 +++++++++++++++++++++++++++++----------------
 1 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 3becb4d..0d5fecf 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -24,6 +24,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/crc32.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -80,8 +81,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	unsigned int cfelen, nvramlen;
 	int namelen = 0;
 	int i;
-	char *boardid;
-	char *tagversion;
+	u32 computed_crc;
 
 	if (bcm63xx_detect_cfe(master))
 		return -EINVAL;
@@ -103,20 +103,33 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 		return -EIO;
 	}
 
-	sscanf(buf->kernel_address, "%u", &kerneladdr);
-	sscanf(buf->kernel_length, "%u", &kernellen);
-	sscanf(buf->total_length, "%u", &totallen);
-	tagversion = &(buf->tag_version[0]);
-	boardid = &(buf->board_id[0]);
-
-	pr_info("CFE boot tag found with version %s and board type %s\n",
-		tagversion, boardid);
-
-	kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
-	rootfsaddr = kerneladdr + kernellen;
-	spareaddr = roundup(totallen, master->erasesize) + cfelen;
-	sparelen = master->size - spareaddr - nvramlen;
-	rootfslen = spareaddr - rootfsaddr;
+	computed_crc = crc32_le(IMAGETAG_CRC_START, (u8 *)buf,
+				offsetof(struct bcm_tag, header_crc));
+	if (computed_crc == buf->header_crc) {
+		char *boardid = &(buf->board_id[0]);
+		char *tagversion = &(buf->tag_version[0]);
+
+		sscanf(buf->kernel_address, "%u", &kerneladdr);
+		sscanf(buf->kernel_length, "%u", &kernellen);
+		sscanf(buf->total_length, "%u", &totallen);
+
+		pr_info("CFE boot tag found with version %s and board type %s\n",
+			tagversion, boardid);
+
+		kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
+		rootfsaddr = kerneladdr + kernellen;
+		spareaddr = roundup(totallen, master->erasesize) + cfelen;
+		sparelen = master->size - spareaddr - nvramlen;
+		rootfslen = spareaddr - rootfsaddr;
+	} else {
+		pr_warn("CFE boot tag CRC invalid (expected %08x, actual %08x)\n",
+			buf->header_crc, computed_crc);
+		kernellen = 0;
+		rootfslen = 0;
+		rootfsaddr = 0;
+		spareaddr = cfelen;
+		sparelen = master->size - cfelen - nvramlen;
+	}
 
 	/* Determine number of partitions */
 	namelen = 8;
-- 
1.7.2.5

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Guillaume LECERF]

Hi Jonas,

2011/12/17 Jonas Gorski <jonas.gorski@gmail.com>:
> -       spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
> -       sparelen = master->size - spareaddr - master->erasesize;
> +       spareaddr = roundup(totallen, master->erasesize) + cfelen;

spareaddr = roundup(totallen, cfelen) + cfelen ?

-- 
Guillaume LECERF
OpenBricks developer - www.openbricks.org

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Guillaume LECERF]

Hi Jonas,

2011/12/17 Jonas Gorski <jonas.gorski@gmail.com>:
> -       spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
> -       sparelen = master->size - spareaddr - master->erasesize;
> +       spareaddr = roundup(totallen, master->erasesize) + cfelen;

spareaddr = roundup(totallen, cfelen) + cfelen ?

-- 
Guillaume LECERF
OpenBricks developer - www.openbricks.org

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

Hi Guillaume,

On 17 December 2011 14:13, Guillaume LECERF <glecerf@gmail.com> wrote:
> Hi Jonas,
>
> 2011/12/17 Jonas Gorski <jonas.gorski@gmail.com>:
>> -       spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
>> -       sparelen = master->size - spareaddr - master->erasesize;
>> +       spareaddr = roundup(totallen, master->erasesize) + cfelen;
>
> spareaddr = roundup(totallen, cfelen) + cfelen ?

The intention is to align the spareaddr to the next eraseblock (cfelen
can be bigger than the erasesize). Maybe writing it as

       spareaddr = roundup(cfelen + totallen, master->erasesize);

would be cleaner.


Jonas

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

Hi Guillaume,

On 17 December 2011 14:13, Guillaume LECERF <glecerf@gmail.com> wrote:
> Hi Jonas,
>
> 2011/12/17 Jonas Gorski <jonas.gorski@gmail.com>:
>> -       spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
>> -       sparelen = master->size - spareaddr - master->erasesize;
>> +       spareaddr = roundup(totallen, master->erasesize) + cfelen;
>
> spareaddr = roundup(totallen, cfelen) + cfelen ?

The intention is to align the spareaddr to the next eraseblock (cfelen
can be bigger than the erasesize). Maybe writing it as

       spareaddr = roundup(cfelen + totallen, master->erasesize);

would be cleaner.


Jonas

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Guillaume LECERF]

2011/12/17 Jonas Gorski <jonas.gorski@gmail.com>:
> The intention is to align the spareaddr to the next eraseblock (cfelen
> can be bigger than the erasesize). Maybe writing it as
>
>        spareaddr = roundup(cfelen + totallen, master->erasesize);
>
> would be cleaner.


Humm, you're right, forget my comment.
BTW I don't mind what syntax you chose.

Good job ;)


-- 
Guillaume LECERF
OpenBricks developer - www.openbricks.org

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Guillaume LECERF]

2011/12/17 Jonas Gorski <jonas.gorski@gmail.com>:
> The intention is to align the spareaddr to the next eraseblock (cfelen
> can be bigger than the erasesize). Maybe writing it as
>
>        spareaddr = roundup(cfelen + totallen, master->erasesize);
>
> would be cleaner.


Humm, you're right, forget my comment.
BTW I don't mind what syntax you chose.

Good job ;)


-- 
Guillaume LECERF
OpenBricks developer - www.openbricks.org

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

On Sat, 2011-12-17 at 13:58 +0100, Jonas Gorski wrote:
> The CFE and NVRAM are always at least 64K big, so make sure their
> partitions are never smaller than this in case the flash has smaller
> erase sectors.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

Not that I have any knowledge about BCM platform, but still, I think
it is good idea to explain why these partitions have to be at least
64KiB. Could you please do this, just for the sake of having good
commit messages?

Artem.

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

On Sat, 2011-12-17 at 13:58 +0100, Jonas Gorski wrote:
> The CFE and NVRAM are always at least 64K big, so make sure their
> partitions are never smaller than this in case the flash has smaller
> erase sectors.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

Not that I have any knowledge about BCM platform, but still, I think
it is good idea to explain why these partitions have to be at least
64KiB. Could you please do this, just for the sake of having good
commit messages?

Artem.

Re: [PATCH 0/5] MTD: bcm63xxpart: improve robustness

[Artem Bityutskiy]

On Sat, 2011-12-17 at 13:58 +0100, Jonas Gorski wrote:
> This patchset aims at improving the robustness of the CFE detection and
> image tag validation and parsing.
> 
> The image tag parsing improvement is especially useful when booting a
> ramdisk image through tftp, since there is no guarantee that the image
> on the flash is valid at this point.
> 
> As usual this patchset should got through the MTD tree and is based on
> the l2-mtd-2.6 git.

Looks good except of the "explain 64KiB" nit-pick. Pushing to
l2-mtd-2.6.git, but I'll take the improved version of patch 2.

Artem.

Re: [PATCH 0/5] MTD: bcm63xxpart: improve robustness

[Artem Bityutskiy]

On Sat, 2011-12-17 at 13:58 +0100, Jonas Gorski wrote:
> This patchset aims at improving the robustness of the CFE detection and
> image tag validation and parsing.
> 
> The image tag parsing improvement is especially useful when booting a
> ramdisk image through tftp, since there is no guarantee that the image
> on the flash is valid at this point.
> 
> As usual this patchset should got through the MTD tree and is based on
> the l2-mtd-2.6 git.

Looks good except of the "explain 64KiB" nit-pick. Pushing to
l2-mtd-2.6.git, but I'll take the improved version of patch 2.

Artem.

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

On 17 December 2011 22:33, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Not that I have any knowledge about BCM platform, but still, I think
> it is good idea to explain why these partitions have to be at least
> 64KiB. Could you please do this, just for the sake of having good
> commit messages?

Sure, no problem. Should I sent the whole series again or is a V2 of
this one enough?


Jonas

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

On 17 December 2011 22:33, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Not that I have any knowledge about BCM platform, but still, I think
> it is good idea to explain why these partitions have to be at least
> 64KiB. Could you please do this, just for the sake of having good
> commit messages?

Sure, no problem. Should I sent the whole series again or is a V2 of
this one enough?


Jonas

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Sat, 2011-12-17 at 22:57 +0100, Jonas Gorski wrote:
> On 17 December 2011 22:33, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Not that I have any knowledge about BCM platform, but still, I think
> > it is good idea to explain why these partitions have to be at least
> > 64KiB. Could you please do this, just for the sake of having good
> > commit messages?
> 
> Sure, no problem. Should I sent the whole series again or is a V2 of
> this one enough?

Just one patch is better - less traffic.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Sat, 2011-12-17 at 22:57 +0100, Jonas Gorski wrote:
> On 17 December 2011 22:33, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Not that I have any knowledge about BCM platform, but still, I think
> > it is good idea to explain why these partitions have to be at least
> > 64KiB. Could you please do this, just for the sake of having good
> > commit messages?
> 
> Sure, no problem. Should I sent the whole series again or is a V2 of
> this one enough?

Just one patch is better - less traffic.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
partition to be 64 KiB (or erase block sized, if larger).
Ensure this assumption is also met when creating the partitions to
prevent accidential erasure of CFE or NVRAM.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---

Changes V1 -> V2:
  Clarified the need for the minimum size.


Hope this one's better :)

 drivers/mtd/bcm63xxpart.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 9933b34..23f6201 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -36,6 +36,9 @@
 
 #define BCM63XX_EXTENDED_SIZE	0xBFC00000	/* Extended flash address */
 
+#define BCM63XX_MIN_CFE_SIZE	0x10000		/* always at least 64K */
+#define BCM63XX_MIN_NVRAM_SIZE	0x10000		/* always at least 64K */
+
 #define BCM63XX_CFE_MAGIC_OFFSET 0x4e0
 
 static int bcm63xx_detect_cfe(struct mtd_info *master)
@@ -74,6 +77,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	size_t retlen;
 	unsigned int rootfsaddr, kerneladdr, spareaddr;
 	unsigned int rootfslen, kernellen, sparelen, totallen;
+	unsigned int cfelen, nvramlen;
 	int namelen = 0;
 	int i;
 	char *boardid;
@@ -82,14 +86,18 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	if (bcm63xx_detect_cfe(master))
 		return -EINVAL;
 
+	cfelen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_CFE_SIZE);
+	nvramlen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_NVRAM_SIZE);
+
 	/* Allocate memory for buffer */
 	buf = vmalloc(sizeof(struct bcm_tag));
 	if (!buf)
 		return -ENOMEM;
 
 	/* Get the tag */
-	ret = master->read(master, master->erasesize, sizeof(struct bcm_tag),
-							&retlen, (void *)buf);
+	ret = master->read(master, cfelen, sizeof(struct bcm_tag), &retlen,
+			   (void *)buf);
+
 	if (retlen != sizeof(struct bcm_tag)) {
 		vfree(buf);
 		return -EIO;
@@ -106,8 +114,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 
 	kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
 	rootfsaddr = kerneladdr + kernellen;
-	spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
-	sparelen = master->size - spareaddr - master->erasesize;
+	spareaddr = roundup(totallen, master->erasesize) + cfelen;
+	sparelen = master->size - spareaddr - nvramlen;
 	rootfslen = spareaddr - rootfsaddr;
 
 	/* Determine number of partitions */
@@ -131,7 +139,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	/* Start building partition list */
 	parts[curpart].name = "CFE";
 	parts[curpart].offset = 0;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].size = cfelen;
 	curpart++;
 
 	if (kernellen > 0) {
@@ -151,8 +159,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	}
 
 	parts[curpart].name = "nvram";
-	parts[curpart].offset = master->size - master->erasesize;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].offset = master->size - nvramlen;
+	parts[curpart].size = nvramlen;
 
 	/* Global partition "linux" to make easy firmware upgrade */
 	curpart++;
-- 
1.7.2.5

[PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
partition to be 64 KiB (or erase block sized, if larger).
Ensure this assumption is also met when creating the partitions to
prevent accidential erasure of CFE or NVRAM.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---

Changes V1 -> V2:
  Clarified the need for the minimum size.


Hope this one's better :)

 drivers/mtd/bcm63xxpart.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index 9933b34..23f6201 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -36,6 +36,9 @@
 
 #define BCM63XX_EXTENDED_SIZE	0xBFC00000	/* Extended flash address */
 
+#define BCM63XX_MIN_CFE_SIZE	0x10000		/* always at least 64K */
+#define BCM63XX_MIN_NVRAM_SIZE	0x10000		/* always at least 64K */
+
 #define BCM63XX_CFE_MAGIC_OFFSET 0x4e0
 
 static int bcm63xx_detect_cfe(struct mtd_info *master)
@@ -74,6 +77,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	size_t retlen;
 	unsigned int rootfsaddr, kerneladdr, spareaddr;
 	unsigned int rootfslen, kernellen, sparelen, totallen;
+	unsigned int cfelen, nvramlen;
 	int namelen = 0;
 	int i;
 	char *boardid;
@@ -82,14 +86,18 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	if (bcm63xx_detect_cfe(master))
 		return -EINVAL;
 
+	cfelen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_CFE_SIZE);
+	nvramlen = max_t(uint32_t, master->erasesize, BCM63XX_MIN_NVRAM_SIZE);
+
 	/* Allocate memory for buffer */
 	buf = vmalloc(sizeof(struct bcm_tag));
 	if (!buf)
 		return -ENOMEM;
 
 	/* Get the tag */
-	ret = master->read(master, master->erasesize, sizeof(struct bcm_tag),
-							&retlen, (void *)buf);
+	ret = master->read(master, cfelen, sizeof(struct bcm_tag), &retlen,
+			   (void *)buf);
+
 	if (retlen != sizeof(struct bcm_tag)) {
 		vfree(buf);
 		return -EIO;
@@ -106,8 +114,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 
 	kerneladdr = kerneladdr - BCM63XX_EXTENDED_SIZE;
 	rootfsaddr = kerneladdr + kernellen;
-	spareaddr = roundup(totallen, master->erasesize) + master->erasesize;
-	sparelen = master->size - spareaddr - master->erasesize;
+	spareaddr = roundup(totallen, master->erasesize) + cfelen;
+	sparelen = master->size - spareaddr - nvramlen;
 	rootfslen = spareaddr - rootfsaddr;
 
 	/* Determine number of partitions */
@@ -131,7 +139,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	/* Start building partition list */
 	parts[curpart].name = "CFE";
 	parts[curpart].offset = 0;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].size = cfelen;
 	curpart++;
 
 	if (kernellen > 0) {
@@ -151,8 +159,8 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
 	}
 
 	parts[curpart].name = "nvram";
-	parts[curpart].offset = master->size - master->erasesize;
-	parts[curpart].size = master->erasesize;
+	parts[curpart].offset = master->size - nvramlen;
+	parts[curpart].size = nvramlen;
 
 	/* Global partition "linux" to make easy firmware upgrade */
 	curpart++;
-- 
1.7.2.5

Re: [PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Mon, 2011-12-19 at 11:36 +0100, Jonas Gorski wrote:
> The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
> partition to be 64 KiB (or erase block sized, if larger).
> Ensure this assumption is also met when creating the partitions to
> prevent accidential erasure of CFE or NVRAM.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

If someone creates a partition smaller than 64 KiB, then why it is
better to silently make it 64 KiB (and thus doing not what the user
asked to do and possibly confusing him), rather than returning an error
or just printing a warning?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Mon, 2011-12-19 at 11:36 +0100, Jonas Gorski wrote:
> The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
> partition to be 64 KiB (or erase block sized, if larger).
> Ensure this assumption is also met when creating the partitions to
> prevent accidential erasure of CFE or NVRAM.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

If someone creates a partition smaller than 64 KiB, then why it is
better to silently make it 64 KiB (and thus doing not what the user
asked to do and possibly confusing him), rather than returning an error
or just printing a warning?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

On 19 December 2011 11:56, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2011-12-19 at 11:36 +0100, Jonas Gorski wrote:
>> The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
>> partition to be 64 KiB (or erase block sized, if larger).
>> Ensure this assumption is also met when creating the partitions to
>> prevent accidential erasure of CFE or NVRAM.
>>
>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>
> If someone creates a partition smaller than 64 KiB, then why it is
> better to silently make it 64 KiB (and thus doing not what the user
> asked to do and possibly confusing him), rather than returning an error
> or just printing a warning?

This adjustment to 64 KiB is only done for the CFE and NVRAM
partitions, not for the rootfs or kernel partitions. The CFE and NVRAM
lengths/offsets are defined in the CFE boot loader at build time and
fixed, so to change them you would need to build and flash your own
CFE (and the sources are not public, so you also need to be a Broadcom
customer). I have yet to see a device where this was done, so this is
currently just a theoretical possibility.

Also you can't create/modify partitions arbitrarily as there is no
partition table, just a fixed image header format (which the CFE
parses on boot). So the only two partitions changeable from the
outside are the kernel and rootfs partitions, which are defined in the
image tag, which always resides at the beginning of the first erase
block after the CFE. Changing the CFE length would result in a changed
offset of the image tag, leading to a "wrong" image tag being read (or
in case after patch 5, a warning that the image tag is likely corrupt,
and no rootfs/kernel partitions created).

Of course everything is done under the assumption the boot loader is
CFE (there are a few devices with RedBoot out there), but the parser
already bails out if no CFE is detected.

Hope that clears things up.

Jonas

Re: [PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Jonas Gorski]

On 19 December 2011 11:56, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2011-12-19 at 11:36 +0100, Jonas Gorski wrote:
>> The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
>> partition to be 64 KiB (or erase block sized, if larger).
>> Ensure this assumption is also met when creating the partitions to
>> prevent accidential erasure of CFE or NVRAM.
>>
>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>
> If someone creates a partition smaller than 64 KiB, then why it is
> better to silently make it 64 KiB (and thus doing not what the user
> asked to do and possibly confusing him), rather than returning an error
> or just printing a warning?

This adjustment to 64 KiB is only done for the CFE and NVRAM
partitions, not for the rootfs or kernel partitions. The CFE and NVRAM
lengths/offsets are defined in the CFE boot loader at build time and
fixed, so to change them you would need to build and flash your own
CFE (and the sources are not public, so you also need to be a Broadcom
customer). I have yet to see a device where this was done, so this is
currently just a theoretical possibility.

Also you can't create/modify partitions arbitrarily as there is no
partition table, just a fixed image header format (which the CFE
parses on boot). So the only two partitions changeable from the
outside are the kernel and rootfs partitions, which are defined in the
image tag, which always resides at the beginning of the first erase
block after the CFE. Changing the CFE length would result in a changed
offset of the image tag, leading to a "wrong" image tag being read (or
in case after patch 5, a warning that the image tag is likely corrupt,
and no rootfs/kernel partitions created).

Of course everything is done under the assumption the boot loader is
CFE (there are a few devices with RedBoot out there), but the parser
already bails out if no CFE is detected.

Hope that clears things up.

Jonas

Re: [PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

On Mon, 2011-12-19 at 11:36 +0100, Jonas Gorski wrote:
> The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
> partition to be 64 KiB (or erase block sized, if larger).
> Ensure this assumption is also met when creating the partitions to
> prevent accidential erasure of CFE or NVRAM.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

Updated, thanks!

Re: [PATCH V2 2/5] MTD: bcm63xxpart: make sure CFE and NVRAM partitions are at least 64K

[Artem Bityutskiy]

On Mon, 2011-12-19 at 11:36 +0100, Jonas Gorski wrote:
> The CFE boot loader on BCM63XX platforms assumes itself and the NVRAM
> partition to be 64 KiB (or erase block sized, if larger).
> Ensure this assumption is also met when creating the partitions to
> prevent accidential erasure of CFE or NVRAM.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

Updated, thanks!