[PATCH 00/10] ARM MTD AFS v2 partition support

[PATCH 00/10] ARM MTD AFS v2 partition support

[Linus Walleij]

Years ago, the ARM reference platforms moved away from the footer
format parsed by the current AFS code in MTD. As far as I can
tell this begun with the RealView reference designs (mach-realview)
and the new format has been used in RealView, Versatile Express
and recently the Juno Development platform (64bit).

The code has not been working for newer designs for a long time.
Proper parsing of the flash image partitions is very helpful when
you want to replace firmware when running full Linux and none of
the boot monitors or boot loaders.

After this, AFS parsing will work on all ARM reference designs,
or well, all I could test. I tested the Integrator/AP, RealView
PB11MPCore and Juno Development Platform.

The first 9 patches basically refactors the current code to
make the old partition format more encapsulated, while the last
patch adds the new v2 format.

Linus Walleij (10):
  mtd: afs: rename structs and functions for v1
  mtd: enable AFS selection for ARM64
  mtd: afs: break out v1 footer magic to a define
  mtd: afs: refactor v1 partition parsing
  mtd: afs: simplify partition parsing
  mtd: afs: simplify partition detection
  mtd: factor out v1 partition parsing
  mtd: afs: factor footer parsing into the v1 part parsing
  mtd: afs: factor the IIS read into partition parser
  mtd: afs: add v2 partition parsing

 drivers/mtd/Kconfig |   2 +-
 drivers/mtd/afs.c   | 388 +++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 263 insertions(+), 127 deletions(-)

-- 
2.4.3

[PATCH 01/10] mtd: afs: rename structs and functions for v1

[Linus Walleij]

Since we're gonna add the v2 version of flash information
structure and we want to avoid confusion, rename the old
functions to *v1. Cut the word "structure" from the struct
name, it is pretty obvious that it is a struct already from
the keyword.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index 96a33e3f7b00..9af00528586f 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -34,7 +34,7 @@
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
 
-struct footer_struct {
+struct footer_v1 {
 	u32 image_info_base;	/* Address of first word of ImageFooter  */
 	u32 image_start;	/* Start of area reserved by this footer */
 	u32 signature;		/* 'Magic' number proves it's a footer   */
@@ -42,7 +42,7 @@ struct footer_struct {
 	u32 checksum;		/* Just this structure                   */
 };
 
-struct image_info_struct {
+struct image_info_v1 {
 	u32 bootFlags;		/* Boot flags, compression etc.          */
 	u32 imageNumber;	/* Unique number, selects for boot etc.  */
 	u32 loadAddress;	/* Address program should be loaded to   */
@@ -67,10 +67,10 @@ static u32 word_sum(void *words, int num)
 }
 
 static int
-afs_read_footer(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
-		u_int off, u_int mask)
+afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
+		   u_int off, u_int mask)
 {
-	struct footer_struct fs;
+	struct footer_v1 fs;
 	u_int ptr = off + mtd->erasesize - sizeof(fs);
 	size_t sz;
 	int ret;
@@ -126,7 +126,7 @@ afs_read_footer(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
 }
 
 static int
-afs_read_iis(struct mtd_info *mtd, struct image_info_struct *iis, u_int ptr)
+afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
 {
 	size_t sz;
 	int ret, i;
@@ -182,16 +182,16 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 * the strings.
 	 */
 	for (idx = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
-		struct image_info_struct iis;
+		struct image_info_v1 iis;
 		u_int iis_ptr, img_ptr;
 
-		ret = afs_read_footer(mtd, &img_ptr, &iis_ptr, off, mask);
+		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
 		if (ret < 0)
 			break;
 		if (ret == 0)
 			continue;
 
-		ret = afs_read_iis(mtd, &iis, iis_ptr);
+		ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
 		if (ret < 0)
 			break;
 		if (ret == 0)
@@ -215,18 +215,18 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 * Identify the partitions
 	 */
 	for (idx = off = 0; off < mtd->size; off += mtd->erasesize) {
-		struct image_info_struct iis;
+		struct image_info_v1 iis;
 		u_int iis_ptr, img_ptr;
 
 		/* Read the footer. */
-		ret = afs_read_footer(mtd, &img_ptr, &iis_ptr, off, mask);
+		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
 		if (ret < 0)
 			break;
 		if (ret == 0)
 			continue;
 
 		/* Read the image info block */
-		ret = afs_read_iis(mtd, &iis, iis_ptr);
+		ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
 		if (ret < 0)
 			break;
 		if (ret == 0)
-- 
2.4.3

[PATCH 02/10] mtd: enable AFS selection for ARM64

[Linus Walleij]

ARM use their special partitions also on the ARM64 architecture
reference designs, so enable this for ARM64.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index a03ad2951c7b..42cc953309f1 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -112,7 +112,7 @@ config MTD_CMDLINE_PARTS
 
 config MTD_AFS_PARTS
 	tristate "ARM Firmware Suite partition parsing"
-	depends on ARM
+	depends on (ARM || ARM64)
 	---help---
 	  The ARM Firmware Suite allows the user to divide flash devices into
 	  multiple 'images'. Each such image has a header containing its name
-- 
2.4.3

[PATCH 03/10] mtd: afs: break out v1 footer magic to a define

[Linus Walleij]

Break out the magic number to a #defined constant.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index 9af00528586f..d09280ae12bd 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -34,6 +34,8 @@
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
 
+#define AFSV1_FOOTER_MAGIC 0xA0FFFF9F
+
 struct footer_v1 {
 	u32 image_info_base;	/* Address of first word of ImageFooter  */
 	u32 image_start;	/* Start of area reserved by this footer */
@@ -90,7 +92,7 @@ afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
 	/*
 	 * Does it contain the magic number?
 	 */
-	if (fs.signature != 0xa0ffff9f)
+	if (fs.signature != AFSV1_FOOTER_MAGIC)
 		ret = 0;
 
 	/*
-- 
2.4.3

[PATCH 04/10] mtd: afs: refactor v1 partition parsing

[Linus Walleij]

Return immediately if we are not finding a valid v1 partition
in afs_read_footer_v1(), invert scanning logic so we continue
to read image information on v1 if we found a footer. This is
needed for the logic we introduce to parse v2 footers.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index d09280ae12bd..a1eea50ce180 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -87,25 +87,23 @@ afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
 		return ret;
 	}
 
-	ret = 1;
-
 	/*
 	 * Does it contain the magic number?
 	 */
 	if (fs.signature != AFSV1_FOOTER_MAGIC)
-		ret = 0;
+		return 0;
 
 	/*
 	 * Check the checksum.
 	 */
 	if (word_sum(&fs, sizeof(fs) / sizeof(u32)) != 0xffffffff)
-		ret = 0;
+		return 0;
 
 	/*
 	 * Don't touch the SIB.
 	 */
 	if (fs.type == 2)
-		ret = 0;
+		return 0;
 
 	*iis_start = fs.image_info_base & mask;
 	*img_start = fs.image_start & mask;
@@ -115,16 +113,16 @@ afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
 	 * be located after the footer structure.
 	 */
 	if (*iis_start >= ptr)
-		ret = 0;
+		return 0;
 
 	/*
 	 * Check the start of this image.  The image
 	 * data can not be located after this block.
 	 */
 	if (*img_start > off)
-		ret = 0;
+		return 0;
 
-	return ret;
+	return 1;
 }
 
 static int
@@ -190,18 +188,17 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
 		if (ret < 0)
 			break;
-		if (ret == 0)
-			continue;
-
-		ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
-		if (ret < 0)
-			break;
-		if (ret == 0)
-			continue;
-
-		sz += sizeof(struct mtd_partition);
-		sz += strlen(iis.name) + 1;
-		idx += 1;
+		if (ret) {
+			ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
+			if (ret < 0)
+				break;
+			if (ret == 0)
+				continue;
+
+			sz += sizeof(struct mtd_partition);
+			sz += strlen(iis.name) + 1;
+			idx += 1;
+		}
 	}
 
 	if (!sz)
-- 
2.4.3

[PATCH 05/10] mtd: afs: simplify partition parsing

[Linus Walleij]

This simplifies the AFS partition parsing to make the code
more straight-forward and readable.

Before this patch the code tried to calculate the memory required
to hold the partition info by adding up the sizes of the strings
of the names and adding that to a single memory allocation,
indexing the name pointers in front of the struct mtd_partition
allocations so all allocated data was in one chunk.

This is overzealous. Instead use kstrdup and bail out,
kfree():ing the memory used for MTD partitions and names alike
on the errorpath.

In the process rename the index variable from idx to i.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 67 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index a1eea50ce180..9e6089615f16 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -166,9 +166,9 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 				struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
-	u_int mask, off, idx, sz;
+	u_int mask, off, sz;
 	int ret = 0;
-	char *str;
+	int i;
 
 	/*
 	 * This is the address mask; we use this to mask off out of
@@ -181,78 +181,75 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 * partition information.  We include in this the size of
 	 * the strings.
 	 */
-	for (idx = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
-		struct image_info_v1 iis;
+	for (i = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
 		u_int iis_ptr, img_ptr;
 
 		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
 		if (ret < 0)
-			break;
+			return ret;
 		if (ret) {
-			ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
-			if (ret < 0)
-				break;
-			if (ret == 0)
-				continue;
-
 			sz += sizeof(struct mtd_partition);
-			sz += strlen(iis.name) + 1;
-			idx += 1;
+			i += 1;
 		}
 	}
 
-	if (!sz)
-		return ret;
+	if (!i)
+		return 0;
 
 	parts = kzalloc(sz, GFP_KERNEL);
 	if (!parts)
 		return -ENOMEM;
 
-	str = (char *)(parts + idx);
-
 	/*
 	 * Identify the partitions
 	 */
-	for (idx = off = 0; off < mtd->size; off += mtd->erasesize) {
+	for (i = off = 0; off < mtd->size; off += mtd->erasesize) {
 		struct image_info_v1 iis;
 		u_int iis_ptr, img_ptr;
 
 		/* Read the footer. */
 		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
 		if (ret < 0)
-			break;
+			goto out_free_parts;
 		if (ret == 0)
 			continue;
 
 		/* Read the image info block */
 		ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
 		if (ret < 0)
-			break;
+			goto out_free_parts;
 		if (ret == 0)
 			continue;
 
-		strcpy(str, iis.name);
+		parts[i].name = kstrdup(iis.name, GFP_KERNEL);
+		if (!parts[i].name) {
+			ret = -ENOMEM;
+			goto out_free_parts;
+		}
 
-		parts[idx].name		= str;
-		parts[idx].size		= (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
-		parts[idx].offset	= img_ptr;
-		parts[idx].mask_flags	= 0;
+		parts[i].size		= (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
+		parts[i].offset	= img_ptr;
+		parts[i].mask_flags	= 0;
 
 		printk("  mtd%d: at 0x%08x, %5lluKiB, %8u, %s\n",
-			idx, img_ptr, parts[idx].size / 1024,
-			iis.imageNumber, str);
-
-		idx += 1;
-		str = str + strlen(iis.name) + 1;
-	}
+			i, img_ptr, parts[i].size / 1024,
+			iis.imageNumber, parts[i].name);
 
-	if (!idx) {
-		kfree(parts);
-		parts = NULL;
+		i += 1;
 	}
 
 	*pparts = parts;
-	return idx ? idx : ret;
+	return i;
+
+out_free_parts:
+	while (i >= 0) {
+		if (parts[i].name)
+			kfree(parts[i].name);
+		i--;
+	}
+	kfree(parts);
+	*pparts = NULL;
+	return ret;
 }
 
 static struct mtd_part_parser afs_parser = {
-- 
2.4.3

[PATCH 05/10] mtd: afs: simplify partition parsing

[Brian Norris]

Hi Linus,

On Thu, Oct 15, 2015 at 03:08:48PM +0200, Linus Walleij wrote:
> This simplifies the AFS partition parsing to make the code
> more straight-forward and readable.
> 
> Before this patch the code tried to calculate the memory required
> to hold the partition info by adding up the sizes of the strings
> of the names and adding that to a single memory allocation,
> indexing the name pointers in front of the struct mtd_partition
> allocations so all allocated data was in one chunk.
> 
> This is overzealous. Instead use kstrdup and bail out,
> kfree():ing the memory used for MTD partitions and names alike
> on the errorpath.
> 
> In the process rename the index variable from idx to i.
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 67 ++++++++++++++++++++++++++-----------------------------
>  1 file changed, 32 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index a1eea50ce180..9e6089615f16 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -166,9 +166,9 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  				struct mtd_part_parser_data *data)
>  {
>  	struct mtd_partition *parts;
> -	u_int mask, off, idx, sz;
> +	u_int mask, off, sz;
>  	int ret = 0;
> -	char *str;
> +	int i;
>  
>  	/*
>  	 * This is the address mask; we use this to mask off out of
> @@ -181,78 +181,75 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  	 * partition information.  We include in this the size of
>  	 * the strings.
>  	 */

Nit: you rewrite this comment in the next patch, when it really should
be rewritten here, since this is where you change the allocation
behavior. I wouldn't worry about this, except that I have other comments
below that I think you'll need to address.

> -	for (idx = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
> -		struct image_info_v1 iis;
> +	for (i = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
>  		u_int iis_ptr, img_ptr;
>  
>  		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
>  		if (ret < 0)
> -			break;
> +			return ret;
>  		if (ret) {
> -			ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
> -			if (ret < 0)
> -				break;
> -			if (ret == 0)
> -				continue;
> -
>  			sz += sizeof(struct mtd_partition);
> -			sz += strlen(iis.name) + 1;
> -			idx += 1;
> +			i += 1;
>  		}
>  	}
>  
> -	if (!sz)
> -		return ret;
> +	if (!i)
> +		return 0;
>  
>  	parts = kzalloc(sz, GFP_KERNEL);
>  	if (!parts)
>  		return -ENOMEM;
>  
> -	str = (char *)(parts + idx);
> -
>  	/*
>  	 * Identify the partitions
>  	 */
> -	for (idx = off = 0; off < mtd->size; off += mtd->erasesize) {
> +	for (i = off = 0; off < mtd->size; off += mtd->erasesize) {
>  		struct image_info_v1 iis;
>  		u_int iis_ptr, img_ptr;
>  
>  		/* Read the footer. */
>  		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
>  		if (ret < 0)
> -			break;
> +			goto out_free_parts;
>  		if (ret == 0)
>  			continue;
>  
>  		/* Read the image info block */
>  		ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
>  		if (ret < 0)
> -			break;
> +			goto out_free_parts;
>  		if (ret == 0)
>  			continue;
>  
> -		strcpy(str, iis.name);
> +		parts[i].name = kstrdup(iis.name, GFP_KERNEL);

Unfortunately, there's a (sort of) good reason the name strings were
allocated along with the partition info all in one go; the calling code
expects to be able to kfree() just the struct to free up all the
parser's memory (see mtd_device_parse_register(), which does
kfree(real_parts)). So you're introducing N string allocations that will
never be freed in the "success" case -- i.e., a memory leak.

Now, it kinda sucks that MTD has such limited cleanup facilities for its
parsers. I believe we either have similar ugly code (or just more memory
leaks) in other parsers. It'd be nice if we added a cleanup hook to
struct mtd_part_parser, so we can allow parsers to do arbitrary
cleanups.

Brian

> +		if (!parts[i].name) {
> +			ret = -ENOMEM;
> +			goto out_free_parts;
> +		}
>  
> -		parts[idx].name		= str;
> -		parts[idx].size		= (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
> -		parts[idx].offset	= img_ptr;
> -		parts[idx].mask_flags	= 0;
> +		parts[i].size		= (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
> +		parts[i].offset	= img_ptr;
> +		parts[i].mask_flags	= 0;
>  
>  		printk("  mtd%d: at 0x%08x, %5lluKiB, %8u, %s\n",
> -			idx, img_ptr, parts[idx].size / 1024,
> -			iis.imageNumber, str);
> -
> -		idx += 1;
> -		str = str + strlen(iis.name) + 1;
> -	}
> +			i, img_ptr, parts[i].size / 1024,
> +			iis.imageNumber, parts[i].name);
>  
> -	if (!idx) {
> -		kfree(parts);
> -		parts = NULL;
> +		i += 1;
>  	}
>  
>  	*pparts = parts;
> -	return idx ? idx : ret;
> +	return i;
> +
> +out_free_parts:
> +	while (i >= 0) {
> +		if (parts[i].name)
> +			kfree(parts[i].name);
> +		i--;
> +	}
> +	kfree(parts);
> +	*pparts = NULL;
> +	return ret;
>  }
>  
>  static struct mtd_part_parser afs_parser = {
> -- 
> 2.4.3
> 

[PATCH 06/10] mtd: afs: simplify partition detection

[Linus Walleij]

Instead of reading out the AFS footers twice, create a separate
function to just check if there is a footer or not. Rids a few
local variables and prepare us to join the actual parser into
one function.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index 9e6089615f16..2307f54195f5 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -68,6 +68,26 @@ static u32 word_sum(void *words, int num)
 	return sum;
 }
 
+static bool afs_is_v1(struct mtd_info *mtd, u_int off)
+{
+	/* The magic is 12 bytes from the end of the erase block */
+	u_int ptr = off + mtd->erasesize - 12;
+	u32 magic;
+	size_t sz;
+	int ret;
+
+	ret = mtd_read(mtd, ptr, 4, &sz, (u_char *)&magic);
+	if (ret < 0) {
+		printk(KERN_ERR "AFS: mtd read failed@0x%x: %d\n",
+		       ptr, ret);
+		return false;
+	}
+	if (ret >= 0 && sz != 4)
+		return false;
+
+	return (magic == AFSV1_FOOTER_MAGIC);
+}
+
 static int
 afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
 		   u_int off, u_int mask)
@@ -176,18 +196,9 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 */
 	mask = mtd->size - 1;
 
-	/*
-	 * First, calculate the size of the array we need for the
-	 * partition information.  We include in this the size of
-	 * the strings.
-	 */
+	/* Count the partitions by looping over all erase blocks */
 	for (i = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
-		u_int iis_ptr, img_ptr;
-
-		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
-		if (ret < 0)
-			return ret;
-		if (ret) {
+		if (afs_is_v1(mtd, off)) {
 			sz += sizeof(struct mtd_partition);
 			i += 1;
 		}
-- 
2.4.3

[PATCH 06/10] mtd: afs: simplify partition detection

[Brian Norris]

Hi Linus,

On Thu, Oct 15, 2015 at 03:08:49PM +0200, Linus Walleij wrote:
> Instead of reading out the AFS footers twice, create a separate
> function to just check if there is a footer or not. Rids a few
> local variables and prepare us to join the actual parser into
> one function.
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index 9e6089615f16..2307f54195f5 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -68,6 +68,26 @@ static u32 word_sum(void *words, int num)
>  	return sum;
>  }
>  
> +static bool afs_is_v1(struct mtd_info *mtd, u_int off)
> +{
> +	/* The magic is 12 bytes from the end of the erase block */
> +	u_int ptr = off + mtd->erasesize - 12;
> +	u32 magic;
> +	size_t sz;
> +	int ret;
> +
> +	ret = mtd_read(mtd, ptr, 4, &sz, (u_char *)&magic);
> +	if (ret < 0) {
> +		printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return false;
> +	}
> +	if (ret >= 0 && sz != 4)
> +		return false;
> +
> +	return (magic == AFSV1_FOOTER_MAGIC);
> +}
> +
>  static int
>  afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
>  		   u_int off, u_int mask)
> @@ -176,18 +196,9 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  	 */
>  	mask = mtd->size - 1;
>  
> -	/*
> -	 * First, calculate the size of the array we need for the
> -	 * partition information.  We include in this the size of
> -	 * the strings.
> -	 */
> +	/* Count the partitions by looping over all erase blocks */
>  	for (i = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
> -		u_int iis_ptr, img_ptr;
> -
> -		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
> -		if (ret < 0)
> -			return ret;
> -		if (ret) {
> +		if (afs_is_v1(mtd, off)) {
>  			sz += sizeof(struct mtd_partition);
>  			i += 1;
>  		}

I think this change is more subtle than it looks at first glance. There
are a few points:

(1) This kind of flash format is naturally fairly imprecise and
unfocused; the parser is scanning every block on the device, so its
scans have to have pretty good heuristics to ensure that it doesn't
treat some other block of data as containing AFS metadata. This usually
works out OK when using magic strings and checksums, but it's still not
100% foolproof.

(2) You're dropping some of the safeguards -- particularly the checksum
-- from the first pass that calculates the number of partitions. This
seems OK for now, since you still do the same checks later, and if they
fail, you just skip the block. But it means that if the block isn't
exactly perfect (whether it's due to malice, coincidence, or
corruption), then we have a problem in the second pass through the
device.

(3) The "problem" from (2) is currently only a slightly
larger-than-necessary memory allocation -- we allocate for too many
partitions -- which is benign. But in later patches, you change the
second loop's behavior so that checksum errors become fatal. That means
that any block that happens to have the magic number in the right (or
wrong, depending on your POV) place will cause the entire parsing
process to fail. That's probably a bug, not a feature.

So, is it really worth saving mtd_read()'ing 16 bytes of flash really
worth this extra fragility? I think you should still read the whole
footer and check its checksum before declaring it a 'v1' footer. And
don't make a checksum failure completely fatal; just make it skip the
block.

Regards,
Brian

[PATCH 07/10] mtd: factor out v1 partition parsing

[Linus Walleij]

This breaks out the parsing of v1 partitions so we can later add
a v2 partition parser.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 88 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 38 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index 2307f54195f5..ace27f447abc 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -181,14 +181,18 @@ afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
 	return ret;
 }
 
-static int parse_afs_partitions(struct mtd_info *mtd,
-				struct mtd_partition **pparts,
-				struct mtd_part_parser_data *data)
+static int afs_parse_v1_partition(struct mtd_info *mtd,
+				  u_int off, struct mtd_partition *part)
 {
-	struct mtd_partition *parts;
-	u_int mask, off, sz;
-	int ret = 0;
-	int i;
+	struct image_info_v1 iis;
+	u_int mask;
+	/*
+	 * Static checks cannot see that we bail out if we have an error
+	 * reading the footer.
+	 */
+	u_int uninitialized_var(iis_ptr);
+	u_int uninitialized_var(img_ptr);
+	int ret;
 
 	/*
 	 * This is the address mask; we use this to mask off out of
@@ -196,6 +200,39 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 */
 	mask = mtd->size - 1;
 
+	ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
+	if (ret < 0)
+		return ret;
+
+	/* Read the image info block */
+	ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
+	if (ret < 0)
+		return ret;
+
+	part->name = kstrdup(iis.name, GFP_KERNEL);
+	if (!part->name)
+		return -ENOMEM;
+
+	part->size = (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
+	part->offset = img_ptr;
+	part->mask_flags = 0;
+
+	printk("  mtd: at 0x%08x, %5lluKiB, %8u, %s\n",
+	       img_ptr, part->size / 1024,
+	       iis.imageNumber, part->name);
+
+	return 0;
+}
+
+static int parse_afs_partitions(struct mtd_info *mtd,
+				struct mtd_partition **pparts,
+				struct mtd_part_parser_data *data)
+{
+	struct mtd_partition *parts;
+	u_int off, sz;
+	int ret = 0;
+	int i;
+
 	/* Count the partitions by looping over all erase blocks */
 	for (i = off = sz = 0; off < mtd->size; off += mtd->erasesize) {
 		if (afs_is_v1(mtd, off)) {
@@ -215,38 +252,13 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 * Identify the partitions
 	 */
 	for (i = off = 0; off < mtd->size; off += mtd->erasesize) {
-		struct image_info_v1 iis;
-		u_int iis_ptr, img_ptr;
-
-		/* Read the footer. */
-		ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
-		if (ret < 0)
-			goto out_free_parts;
-		if (ret == 0)
-			continue;
-
-		/* Read the image info block */
-		ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
-		if (ret < 0)
-			goto out_free_parts;
-		if (ret == 0)
-			continue;
-
-		parts[i].name = kstrdup(iis.name, GFP_KERNEL);
-		if (!parts[i].name) {
-			ret = -ENOMEM;
-			goto out_free_parts;
-		}
 
-		parts[i].size		= (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
-		parts[i].offset	= img_ptr;
-		parts[i].mask_flags	= 0;
-
-		printk("  mtd%d: at 0x%08x, %5lluKiB, %8u, %s\n",
-			i, img_ptr, parts[i].size / 1024,
-			iis.imageNumber, parts[i].name);
-
-		i += 1;
+		if (afs_is_v1(mtd, off)) {
+			ret = afs_parse_v1_partition(mtd, off, &parts[i]);
+			if (ret)
+				goto out_free_parts;
+			i++;
+		}
 	}
 
 	*pparts = parts;
-- 
2.4.3

[PATCH 07/10] mtd: factor out v1 partition parsing

[Brian Norris]

Hi,

On Thu, Oct 15, 2015 at 03:08:50PM +0200, Linus Walleij wrote:
> This breaks out the parsing of v1 partitions so we can later add
> a v2 partition parser.
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 88 +++++++++++++++++++++++++++++++------------------------
>  1 file changed, 50 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index 2307f54195f5..ace27f447abc 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -181,14 +181,18 @@ afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
>  	return ret;
>  }
>  
> -static int parse_afs_partitions(struct mtd_info *mtd,
> -				struct mtd_partition **pparts,
> -				struct mtd_part_parser_data *data)
> +static int afs_parse_v1_partition(struct mtd_info *mtd,
> +				  u_int off, struct mtd_partition *part)
>  {
> -	struct mtd_partition *parts;
> -	u_int mask, off, sz;
> -	int ret = 0;
> -	int i;
> +	struct image_info_v1 iis;
> +	u_int mask;
> +	/*
> +	 * Static checks cannot see that we bail out if we have an error
> +	 * reading the footer.
> +	 */
> +	u_int uninitialized_var(iis_ptr);
> +	u_int uninitialized_var(img_ptr);

What's this uninitialized_var() stuff about? These variables seem to
clearly be initialized before use. Did this hang around from some
preliminary code that got refactored?

> +	int ret;
>  
>  	/*
>  	 * This is the address mask; we use this to mask off out of

...

Otherwise, looks OK.

Brian

[PATCH 08/10] mtd: afs: factor footer parsing into the v1 part parsing

[Linus Walleij]

This simplifies the code by factoring in the image footer
parsing into the single function parsing the AFSv1 partitions.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 98 ++++++++++++++++++++++---------------------------------
 1 file changed, 39 insertions(+), 59 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index ace27f447abc..cdee8295d4c0 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -89,63 +89,6 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
 }
 
 static int
-afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
-		   u_int off, u_int mask)
-{
-	struct footer_v1 fs;
-	u_int ptr = off + mtd->erasesize - sizeof(fs);
-	size_t sz;
-	int ret;
-
-	ret = mtd_read(mtd, ptr, sizeof(fs), &sz, (u_char *)&fs);
-	if (ret >= 0 && sz != sizeof(fs))
-		ret = -EINVAL;
-
-	if (ret < 0) {
-		printk(KERN_ERR "AFS: mtd read failed@0x%x: %d\n",
-			ptr, ret);
-		return ret;
-	}
-
-	/*
-	 * Does it contain the magic number?
-	 */
-	if (fs.signature != AFSV1_FOOTER_MAGIC)
-		return 0;
-
-	/*
-	 * Check the checksum.
-	 */
-	if (word_sum(&fs, sizeof(fs) / sizeof(u32)) != 0xffffffff)
-		return 0;
-
-	/*
-	 * Don't touch the SIB.
-	 */
-	if (fs.type == 2)
-		return 0;
-
-	*iis_start = fs.image_info_base & mask;
-	*img_start = fs.image_start & mask;
-
-	/*
-	 * Check the image info base.  This can not
-	 * be located after the footer structure.
-	 */
-	if (*iis_start >= ptr)
-		return 0;
-
-	/*
-	 * Check the start of this image.  The image
-	 * data can not be located after this block.
-	 */
-	if (*img_start > off)
-		return 0;
-
-	return 1;
-}
-
-static int
 afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
 {
 	size_t sz;
@@ -184,6 +127,7 @@ afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
 static int afs_parse_v1_partition(struct mtd_info *mtd,
 				  u_int off, struct mtd_partition *part)
 {
+	struct footer_v1 fs;
 	struct image_info_v1 iis;
 	u_int mask;
 	/*
@@ -192,6 +136,8 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
 	 */
 	u_int uninitialized_var(iis_ptr);
 	u_int uninitialized_var(img_ptr);
+	u_int ptr;
+	size_t sz;
 	int ret;
 
 	/*
@@ -200,9 +146,43 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
 	 */
 	mask = mtd->size - 1;
 
-	ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
-	if (ret < 0)
+	ptr = off + mtd->erasesize - sizeof(fs);
+	ret = mtd_read(mtd, ptr, sizeof(fs), &sz, (u_char *)&fs);
+	if (ret >= 0 && sz != sizeof(fs))
+		ret = -EINVAL;
+	if (ret < 0) {
+		printk(KERN_ERR "AFS: mtd read failed@0x%x: %d\n",
+		       ptr, ret);
 		return ret;
+	}
+	/*
+	 * Check the checksum.
+	 */
+	if (word_sum(&fs, sizeof(fs) / sizeof(u32)) != 0xffffffff)
+		return -EINVAL;
+
+	/*
+	 * Hide the SIB (System Information Block)
+	 */
+	if (fs.type == 2)
+		return 0;
+
+	iis_ptr = fs.image_info_base & mask;
+	img_ptr = fs.image_start & mask;
+
+	/*
+	 * Check the image info base.  This can not
+	 * be located after the footer structure.
+	 */
+	if (iis_ptr >= ptr)
+		return 0;
+
+	/*
+	 * Check the start of this image.  The image
+	 * data can not be located after this block.
+	 */
+	if (img_ptr > off)
+		return 0;
 
 	/* Read the image info block */
 	ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
-- 
2.4.3

[PATCH 08/10] mtd: afs: factor footer parsing into the v1 part parsing

[Brian Norris]

Hi,

On Thu, Oct 15, 2015 at 03:08:51PM +0200, Linus Walleij wrote:
> This simplifies the code by factoring in the image footer
> parsing into the single function parsing the AFSv1 partitions.
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 98 ++++++++++++++++++++++---------------------------------
>  1 file changed, 39 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index ace27f447abc..cdee8295d4c0 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -89,63 +89,6 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
>  }
>  
>  static int
> -afs_read_footer_v1(struct mtd_info *mtd, u_int *img_start, u_int *iis_start,
> -		   u_int off, u_int mask)
> -{
> -	struct footer_v1 fs;
> -	u_int ptr = off + mtd->erasesize - sizeof(fs);
> -	size_t sz;
> -	int ret;
> -
> -	ret = mtd_read(mtd, ptr, sizeof(fs), &sz, (u_char *)&fs);
> -	if (ret >= 0 && sz != sizeof(fs))
> -		ret = -EINVAL;
> -
> -	if (ret < 0) {
> -		printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> -			ptr, ret);
> -		return ret;
> -	}
> -
> -	/*
> -	 * Does it contain the magic number?
> -	 */
> -	if (fs.signature != AFSV1_FOOTER_MAGIC)
> -		return 0;
> -
> -	/*
> -	 * Check the checksum.
> -	 */
> -	if (word_sum(&fs, sizeof(fs) / sizeof(u32)) != 0xffffffff)
> -		return 0;
> -
> -	/*
> -	 * Don't touch the SIB.
> -	 */
> -	if (fs.type == 2)
> -		return 0;
> -
> -	*iis_start = fs.image_info_base & mask;
> -	*img_start = fs.image_start & mask;
> -
> -	/*
> -	 * Check the image info base.  This can not
> -	 * be located after the footer structure.
> -	 */
> -	if (*iis_start >= ptr)
> -		return 0;
> -
> -	/*
> -	 * Check the start of this image.  The image
> -	 * data can not be located after this block.
> -	 */
> -	if (*img_start > off)
> -		return 0;
> -
> -	return 1;
> -}
> -
> -static int
>  afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
>  {
>  	size_t sz;
> @@ -184,6 +127,7 @@ afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
>  static int afs_parse_v1_partition(struct mtd_info *mtd,
>  				  u_int off, struct mtd_partition *part)
>  {
> +	struct footer_v1 fs;
>  	struct image_info_v1 iis;
>  	u_int mask;
>  	/*
> @@ -192,6 +136,8 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
>  	 */
>  	u_int uninitialized_var(iis_ptr);
>  	u_int uninitialized_var(img_ptr);
> +	u_int ptr;
> +	size_t sz;
>  	int ret;
>  
>  	/*
> @@ -200,9 +146,43 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
>  	 */
>  	mask = mtd->size - 1;
>  
> -	ret = afs_read_footer_v1(mtd, &img_ptr, &iis_ptr, off, mask);
> -	if (ret < 0)
> +	ptr = off + mtd->erasesize - sizeof(fs);
> +	ret = mtd_read(mtd, ptr, sizeof(fs), &sz, (u_char *)&fs);
> +	if (ret >= 0 && sz != sizeof(fs))
> +		ret = -EINVAL;
> +	if (ret < 0) {
> +		printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
>  		return ret;
> +	}
> +	/*
> +	 * Check the checksum.
> +	 */
> +	if (word_sum(&fs, sizeof(fs) / sizeof(u32)) != 0xffffffff)
> +		return -EINVAL;

^^ This was modified. See my other comments about it.

Brian

> +
> +	/*
> +	 * Hide the SIB (System Information Block)
> +	 */
> +	if (fs.type == 2)
> +		return 0;
> +
> +	iis_ptr = fs.image_info_base & mask;
> +	img_ptr = fs.image_start & mask;
> +
> +	/*
> +	 * Check the image info base.  This can not
> +	 * be located after the footer structure.
> +	 */
> +	if (iis_ptr >= ptr)
> +		return 0;
> +
> +	/*
> +	 * Check the start of this image.  The image
> +	 * data can not be located after this block.
> +	 */
> +	if (img_ptr > off)
> +		return 0;
>  
>  	/* Read the image info block */
>  	ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
> -- 
> 2.4.3
> 

[PATCH 09/10] mtd: afs: factor the IIS read into partition parser

[Linus Walleij]

Factor the IIS (Image Information Structure) reading into the
partition parser, giving us a single, clean partition parser
function.

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 59 +++++++++++++++++++------------------------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index cdee8295d4c0..1ce6842c917c 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -88,42 +88,6 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
 	return (magic == AFSV1_FOOTER_MAGIC);
 }
 
-static int
-afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
-{
-	size_t sz;
-	int ret, i;
-
-	memset(iis, 0, sizeof(*iis));
-	ret = mtd_read(mtd, ptr, sizeof(*iis), &sz, (u_char *)iis);
-	if (ret < 0)
-		goto failed;
-
-	if (sz != sizeof(*iis)) {
-		ret = -EINVAL;
-		goto failed;
-	}
-
-	ret = 0;
-
-	/*
-	 * Validate the name - it must be NUL terminated.
-	 */
-	for (i = 0; i < sizeof(iis->name); i++)
-		if (iis->name[i] == '\0')
-			break;
-
-	if (i < sizeof(iis->name))
-		ret = 1;
-
-	return ret;
-
- failed:
-	printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
-		ptr, ret);
-	return ret;
-}
-
 static int afs_parse_v1_partition(struct mtd_info *mtd,
 				  u_int off, struct mtd_partition *part)
 {
@@ -139,6 +103,7 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
 	u_int ptr;
 	size_t sz;
 	int ret;
+	int i;
 
 	/*
 	 * This is the address mask; we use this to mask off out of
@@ -185,9 +150,25 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
 		return 0;
 
 	/* Read the image info block */
-	ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
-	if (ret < 0)
-		return ret;
+	memset(&iis, 0, sizeof(iis));
+	ret = mtd_read(mtd, iis_ptr, sizeof(iis), &sz, (u_char *)&iis);
+	if (ret < 0) {
+		printk(KERN_ERR "AFS: mtd read failed@0x%x: %d\n",
+		       iis_ptr, ret);
+		return -EINVAL;
+	}
+
+	if (sz != sizeof(iis))
+		return -EINVAL;
+
+	/*
+	 * Validate the name - it must be NUL terminated.
+	 */
+	for (i = 0; i < sizeof(iis.name); i++)
+		if (iis.name[i] == '\0')
+			break;
+	if (i > sizeof(iis.name))
+		return -EINVAL;
 
 	part->name = kstrdup(iis.name, GFP_KERNEL);
 	if (!part->name)
-- 
2.4.3

[PATCH 09/10] mtd: afs: factor the IIS read into partition parser

[Brian Norris]

Hi Linus,

On Thu, Oct 15, 2015 at 03:08:52PM +0200, Linus Walleij wrote:
> Factor the IIS (Image Information Structure) reading into the
> partition parser, giving us a single, clean partition parser
> function.
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 59 +++++++++++++++++++------------------------------------
>  1 file changed, 20 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index cdee8295d4c0..1ce6842c917c 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -88,42 +88,6 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
>  	return (magic == AFSV1_FOOTER_MAGIC);
>  }
>  
> -static int
> -afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
> -{
> -	size_t sz;
> -	int ret, i;
> -
> -	memset(iis, 0, sizeof(*iis));
> -	ret = mtd_read(mtd, ptr, sizeof(*iis), &sz, (u_char *)iis);
> -	if (ret < 0)
> -		goto failed;
> -
> -	if (sz != sizeof(*iis)) {
> -		ret = -EINVAL;
> -		goto failed;
> -	}
> -
> -	ret = 0;
> -
> -	/*
> -	 * Validate the name - it must be NUL terminated.
> -	 */
> -	for (i = 0; i < sizeof(iis->name); i++)
> -		if (iis->name[i] == '\0')
> -			break;
> -
> -	if (i < sizeof(iis->name))
> -		ret = 1;
> -
> -	return ret;
> -
> - failed:
> -	printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> -		ptr, ret);
> -	return ret;
> -}
> -
>  static int afs_parse_v1_partition(struct mtd_info *mtd,
>  				  u_int off, struct mtd_partition *part)
>  {
> @@ -139,6 +103,7 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
>  	u_int ptr;
>  	size_t sz;
>  	int ret;
> +	int i;
>  
>  	/*
>  	 * This is the address mask; we use this to mask off out of
> @@ -185,9 +150,25 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
>  		return 0;
>  
>  	/* Read the image info block */
> -	ret = afs_read_iis_v1(mtd, &iis, iis_ptr);
> -	if (ret < 0)
> -		return ret;
> +	memset(&iis, 0, sizeof(iis));
> +	ret = mtd_read(mtd, iis_ptr, sizeof(iis), &sz, (u_char *)&iis);
> +	if (ret < 0) {
> +		printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> +		       iis_ptr, ret);
> +		return -EINVAL;
> +	}
> +
> +	if (sz != sizeof(iis))
> +		return -EINVAL;
> +
> +	/*
> +	 * Validate the name - it must be NUL terminated.
> +	 */
> +	for (i = 0; i < sizeof(iis.name); i++)
> +		if (iis.name[i] == '\0')
> +			break;
> +	if (i > sizeof(iis.name))

The above condition is not a correct refactoring. That should be an
inclusive comparison (i.e., i >= sizeof(iis.name)).

> +		return -EINVAL;
>  
>  	part->name = kstrdup(iis.name, GFP_KERNEL);
>  	if (!part->name)

Brian

[PATCH 10/10] mtd: afs: add v2 partition parsing

[Linus Walleij]

The AFS v2 partition type appear in later ARM reference designs
such as RealView, Versatile Express and the 64bit Juno Development
Platform.

The image informations is padded with a 32bit word (4 bytes) on
the 32bit platforms and a 64bit word (8 bytes) on the 64bit
platforms. The boot monitor source code gives at hand that this
is because the first entry in the struct mapped over the image
information is a "next" pointer for a linked list, filled in
by firmware after reading in the info block, and always zero
in the flash. We adjust padding by checking what padding gives
the right checksum.

This was tested on:
- Integrator/AP (v1 partitions)
- RealView PB11MPCore (v2 32bit partitions)
- Juno Development System (v2 64bit partitions)

All systems display the images in flash very nicely as separate
partitions, e.g on Juno:

4 afs partitions found on MTD device 8000000.flash
Creating 4 MTD partitions on "8000000.flash":
0x000000040000-0x0000000c0000 : "fip"
0x000000ec0000-0x0000018c0000 : "Image"
0x000000f00000-0x000000f40000 : "juno"
0x000003ec0000-0x000003f00000 : "bl1"

Cc: Ryan Harkin <ryan.harkin@linaro.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/afs.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index 1ce6842c917c..e46b035bf4d2 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -3,6 +3,7 @@
     drivers/mtd/afs.c: ARM Flash Layout/Partitioning
 
     Copyright ? 2000 ARM Limited
+    Copyright (C) 2015 Linus Walleij
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -35,6 +36,8 @@
 #include <linux/mtd/partitions.h>
 
 #define AFSV1_FOOTER_MAGIC 0xA0FFFF9F
+#define AFSV2_FOOTER_MAGIC1 0x464C5348 /* "FLSH" */
+#define AFSV2_FOOTER_MAGIC2 0x464F4F54 /* "FOOT" */
 
 struct footer_v1 {
 	u32 image_info_base;	/* Address of first word of ImageFooter  */
@@ -68,6 +71,22 @@ static u32 word_sum(void *words, int num)
 	return sum;
 }
 
+static u32 word_sum_v2(u32 *p, u32 num)
+{
+	u32 sum = 0;
+	int i;
+
+	for (i = 0; i < num; i++) {
+		u32 val;
+
+		val = p[i];
+		if (val > ~sum)
+			sum++;
+		sum += val;
+	}
+	return ~sum;
+}
+
 static bool afs_is_v1(struct mtd_info *mtd, u_int off)
 {
 	/* The magic is 12 bytes from the end of the erase block */
@@ -88,6 +107,27 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
 	return (magic == AFSV1_FOOTER_MAGIC);
 }
 
+static bool afs_is_v2(struct mtd_info *mtd, u_int off)
+{
+	/* The magic is the 8 last bytes of the erase block */
+	u_int ptr = off + mtd->erasesize - 8;
+	u32 foot[2];
+	size_t sz;
+	int ret;
+
+	ret = mtd_read(mtd, ptr, 8, &sz, (u_char *)foot);
+	if (ret < 0) {
+		printk(KERN_ERR "AFS: mtd read failed@0x%x: %d\n",
+		       ptr, ret);
+		return false;
+	}
+	if (ret >= 0 && sz != 8)
+		return false;
+
+	return (foot[0] == AFSV2_FOOTER_MAGIC1 &&
+		foot[1] == AFSV2_FOOTER_MAGIC2);
+}
+
 static int afs_parse_v1_partition(struct mtd_info *mtd,
 				  u_int off, struct mtd_partition *part)
 {
@@ -185,6 +225,113 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
 	return 0;
 }
 
+static int afs_parse_v2_partition(struct mtd_info *mtd,
+				  u_int off, struct mtd_partition *part)
+{
+	u_int ptr;
+	u32 footer[12];
+	u32 imginfo[36];
+	char *name;
+	u32 version;
+	u32 entrypoint;
+	u32 attributes;
+	u32 region_count;
+	u32 block_start;
+	u32 block_end;
+	u32 crc;
+	size_t sz;
+	int ret;
+	int i;
+	int pad = 0;
+
+	pr_debug("Parsing v2 partition @%08x-%08x\n",
+		 off, off + mtd->erasesize);
+
+	/* First read the footer */
+	ptr = off + mtd->erasesize - sizeof(footer);
+	ret = mtd_read(mtd, ptr, sizeof(footer), &sz, (u_char *)footer);
+	if ((ret < 0) || (ret >= 0 && sz != sizeof(footer))) {
+		pr_err("AFS: mtd read failed at 0x%x: %d\n",
+		       ptr, ret);
+		return -EIO;
+	}
+	name = (char *) &footer[0];
+	version = footer[9];
+	ptr = off + mtd->erasesize - sizeof(footer) - footer[8];
+
+	pr_debug("found image \"%s\", version %08x, info @%08x\n",
+		 name, version, ptr);
+
+	/* Then read the image information */
+	ret = mtd_read(mtd, ptr, sizeof(imginfo), &sz, (u_char *)imginfo);
+	if ((ret < 0) || (ret >= 0 && sz != sizeof(imginfo))) {
+		pr_err("AFS: mtd read failed at 0x%x: %d\n",
+		       ptr, ret);
+		return -EIO;
+	}
+
+	/* 32bit platforms have 4 bytes padding */
+	crc = word_sum_v2(&imginfo[1], 34);
+	if (!crc) {
+		pr_debug("Padding 1 word (4 bytes)\n");
+		pad = 1;
+	} else {
+		/* 64bit platforms have 8 bytes padding */
+		crc = word_sum_v2(&imginfo[2], 34);
+		if (!crc) {
+			pr_debug("Padding 2 words (8 bytes)\n");
+			pad = 2;
+		}
+	}
+	if (crc) {
+		pr_err("AFS: bad checksum on v2 image info: %08x\n", crc);
+		return -EINVAL;
+	}
+	entrypoint = imginfo[pad];
+	attributes = imginfo[pad+1];
+	region_count = imginfo[pad+2];
+	block_start = imginfo[20];
+	block_end = imginfo[21];
+
+	pr_debug("image entry=%08x, attr=%08x, regions=%08x, "
+		 "bs=%08x, be=%08x\n",
+		 entrypoint, attributes, region_count,
+		 block_start, block_end);
+
+	for (i = 0; i < region_count; i++) {
+		u32 region_load_addr = imginfo[pad + 3 + i*4];
+		u32 region_size = imginfo[pad + 4 + i*4];
+		u32 region_offset = imginfo[pad + 5 + i*4];
+		u32 region_start;
+		u32 region_end;
+
+		pr_debug("  region %d: address: %08x, size: %08x, "
+			 "offset: %08x\n",
+			 i,
+			 region_load_addr,
+			 region_size,
+			 region_offset);
+
+		region_start = off + region_offset;
+		region_end = region_start + region_size;
+		/* Align partition to end of erase block */
+		region_end += (mtd->erasesize - 1);
+		region_end &= ~(mtd->erasesize -1);
+		pr_debug("   partition start = %08x, partition end = %08x\n",
+			 region_start, region_end);
+
+		/* Create one partition per region */
+		part->name = kstrdup(name, GFP_KERNEL);
+		if (!part->name)
+			return -ENOMEM;
+		part->offset = region_start;
+		part->size = region_end - region_start;
+		part->mask_flags = 0;
+	}
+
+	return 0;
+}
+
 static int parse_afs_partitions(struct mtd_info *mtd,
 				struct mtd_partition **pparts,
 				struct mtd_part_parser_data *data)
@@ -200,6 +347,10 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 			sz += sizeof(struct mtd_partition);
 			i += 1;
 		}
+		if (afs_is_v2(mtd, off)) {
+			sz += sizeof(struct mtd_partition);
+			i += 1;
+		}
 	}
 
 	if (!i)
@@ -213,13 +364,18 @@ static int parse_afs_partitions(struct mtd_info *mtd,
 	 * Identify the partitions
 	 */
 	for (i = off = 0; off < mtd->size; off += mtd->erasesize) {
-
 		if (afs_is_v1(mtd, off)) {
 			ret = afs_parse_v1_partition(mtd, off, &parts[i]);
 			if (ret)
 				goto out_free_parts;
 			i++;
 		}
+		if (afs_is_v2(mtd, off)) {
+			ret = afs_parse_v2_partition(mtd, off, &parts[i]);
+			if (ret)
+				goto out_free_parts;
+			i++;
+		}
 	}
 
 	*pparts = parts;
-- 
2.4.3

[PATCH 10/10] mtd: afs: add v2 partition parsing

[Brian Norris]

Hi,

Only reviewing part of this patch, as I gotta run now.

On Thu, Oct 15, 2015 at 03:08:53PM +0200, Linus Walleij wrote:
> The AFS v2 partition type appear in later ARM reference designs
> such as RealView, Versatile Express and the 64bit Juno Development
> Platform.
> 
> The image informations is padded with a 32bit word (4 bytes) on
> the 32bit platforms and a 64bit word (8 bytes) on the 64bit
> platforms. The boot monitor source code gives at hand that this
> is because the first entry in the struct mapped over the image
> information is a "next" pointer for a linked list, filled in
> by firmware after reading in the info block, and always zero
> in the flash. We adjust padding by checking what padding gives
> the right checksum.
> 
> This was tested on:
> - Integrator/AP (v1 partitions)
> - RealView PB11MPCore (v2 32bit partitions)
> - Juno Development System (v2 64bit partitions)
> 
> All systems display the images in flash very nicely as separate
> partitions, e.g on Juno:
> 
> 4 afs partitions found on MTD device 8000000.flash
> Creating 4 MTD partitions on "8000000.flash":
> 0x000000040000-0x0000000c0000 : "fip"
> 0x000000ec0000-0x0000018c0000 : "Image"
> 0x000000f00000-0x000000f40000 : "juno"
> 0x000003ec0000-0x000003f00000 : "bl1"
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index 1ce6842c917c..e46b035bf4d2 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -3,6 +3,7 @@
>      drivers/mtd/afs.c: ARM Flash Layout/Partitioning
>  
>      Copyright ? 2000 ARM Limited
> +    Copyright (C) 2015 Linus Walleij

The two copyright symbols don't match.

>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -35,6 +36,8 @@
>  #include <linux/mtd/partitions.h>
>  
>  #define AFSV1_FOOTER_MAGIC 0xA0FFFF9F
> +#define AFSV2_FOOTER_MAGIC1 0x464C5348 /* "FLSH" */
> +#define AFSV2_FOOTER_MAGIC2 0x464F4F54 /* "FOOT" */
>  
>  struct footer_v1 {
>  	u32 image_info_base;	/* Address of first word of ImageFooter  */
> @@ -68,6 +71,22 @@ static u32 word_sum(void *words, int num)
>  	return sum;
>  }
>  
> +static u32 word_sum_v2(u32 *p, u32 num)
> +{
> +	u32 sum = 0;
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		u32 val;
> +
> +		val = p[i];
> +		if (val > ~sum)
> +			sum++;
> +		sum += val;
> +	}
> +	return ~sum;
> +}
> +
>  static bool afs_is_v1(struct mtd_info *mtd, u_int off)
>  {
>  	/* The magic is 12 bytes from the end of the erase block */
> @@ -88,6 +107,27 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
>  	return (magic == AFSV1_FOOTER_MAGIC);
>  }
>  
> +static bool afs_is_v2(struct mtd_info *mtd, u_int off)
> +{
> +	/* The magic is the 8 last bytes of the erase block */
> +	u_int ptr = off + mtd->erasesize - 8;
> +	u32 foot[2];
> +	size_t sz;
> +	int ret;
> +
> +	ret = mtd_read(mtd, ptr, 8, &sz, (u_char *)foot);
> +	if (ret < 0) {
> +		printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return false;
> +	}
> +	if (ret >= 0 && sz != 8)
> +		return false;
> +
> +	return (foot[0] == AFSV2_FOOTER_MAGIC1 &&
> +		foot[1] == AFSV2_FOOTER_MAGIC2);
> +}
> +
>  static int afs_parse_v1_partition(struct mtd_info *mtd,
>  				  u_int off, struct mtd_partition *part)
>  {
> @@ -185,6 +225,113 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
>  	return 0;
>  }
>  
> +static int afs_parse_v2_partition(struct mtd_info *mtd,
> +				  u_int off, struct mtd_partition *part)
> +{
> +	u_int ptr;
> +	u32 footer[12];
> +	u32 imginfo[36];
> +	char *name;
> +	u32 version;
> +	u32 entrypoint;
> +	u32 attributes;
> +	u32 region_count;
> +	u32 block_start;
> +	u32 block_end;
> +	u32 crc;
> +	size_t sz;
> +	int ret;
> +	int i;
> +	int pad = 0;
> +
> +	pr_debug("Parsing v2 partition @%08x-%08x\n",
> +		 off, off + mtd->erasesize);
> +
> +	/* First read the footer */
> +	ptr = off + mtd->erasesize - sizeof(footer);
> +	ret = mtd_read(mtd, ptr, sizeof(footer), &sz, (u_char *)footer);
> +	if ((ret < 0) || (ret >= 0 && sz != sizeof(footer))) {
> +		pr_err("AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return -EIO;
> +	}
> +	name = (char *) &footer[0];
> +	version = footer[9];
> +	ptr = off + mtd->erasesize - sizeof(footer) - footer[8];
> +
> +	pr_debug("found image \"%s\", version %08x, info @%08x\n",
> +		 name, version, ptr);
> +
> +	/* Then read the image information */
> +	ret = mtd_read(mtd, ptr, sizeof(imginfo), &sz, (u_char *)imginfo);
> +	if ((ret < 0) || (ret >= 0 && sz != sizeof(imginfo))) {
> +		pr_err("AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return -EIO;
> +	}
> +
> +	/* 32bit platforms have 4 bytes padding */
> +	crc = word_sum_v2(&imginfo[1], 34);
> +	if (!crc) {
> +		pr_debug("Padding 1 word (4 bytes)\n");
> +		pad = 1;
> +	} else {
> +		/* 64bit platforms have 8 bytes padding */
> +		crc = word_sum_v2(&imginfo[2], 34);
> +		if (!crc) {
> +			pr_debug("Padding 2 words (8 bytes)\n");
> +			pad = 2;
> +		}
> +	}
> +	if (crc) {
> +		pr_err("AFS: bad checksum on v2 image info: %08x\n", crc);
> +		return -EINVAL;
> +	}
> +	entrypoint = imginfo[pad];
> +	attributes = imginfo[pad+1];
> +	region_count = imginfo[pad+2];
> +	block_start = imginfo[20];
> +	block_end = imginfo[21];
> +
> +	pr_debug("image entry=%08x, attr=%08x, regions=%08x, "
> +		 "bs=%08x, be=%08x\n",
> +		 entrypoint, attributes, region_count,
> +		 block_start, block_end);
> +
> +	for (i = 0; i < region_count; i++) {
> +		u32 region_load_addr = imginfo[pad + 3 + i*4];
> +		u32 region_size = imginfo[pad + 4 + i*4];
> +		u32 region_offset = imginfo[pad + 5 + i*4];
> +		u32 region_start;
> +		u32 region_end;
> +
> +		pr_debug("  region %d: address: %08x, size: %08x, "
> +			 "offset: %08x\n",
> +			 i,
> +			 region_load_addr,
> +			 region_size,
> +			 region_offset);
> +
> +		region_start = off + region_offset;
> +		region_end = region_start + region_size;
> +		/* Align partition to end of erase block */
> +		region_end += (mtd->erasesize - 1);
> +		region_end &= ~(mtd->erasesize -1);
> +		pr_debug("   partition start = %08x, partition end = %08x\n",
> +			 region_start, region_end);
> +
> +		/* Create one partition per region */
> +		part->name = kstrdup(name, GFP_KERNEL);
> +		if (!part->name)
> +			return -ENOMEM;
> +		part->offset = region_start;
> +		part->size = region_end - region_start;
> +		part->mask_flags = 0;
> +	}
> +
> +	return 0;
> +}
> +
>  static int parse_afs_partitions(struct mtd_info *mtd,
>  				struct mtd_partition **pparts,
>  				struct mtd_part_parser_data *data)
> @@ -200,6 +347,10 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  			sz += sizeof(struct mtd_partition);
>  			i += 1;
>  		}
> +		if (afs_is_v2(mtd, off)) {
> +			sz += sizeof(struct mtd_partition);
> +			i += 1;
> +		}
>  	}
>  
>  	if (!i)
> @@ -213,13 +364,18 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  	 * Identify the partitions
>  	 */
>  	for (i = off = 0; off < mtd->size; off += mtd->erasesize) {
> -
>  		if (afs_is_v1(mtd, off)) {
>  			ret = afs_parse_v1_partition(mtd, off, &parts[i]);
>  			if (ret)
>  				goto out_free_parts;
>  			i++;
>  		}
> +		if (afs_is_v2(mtd, off)) {

Do we really want to allow a single block to be counted as both v1 and
v2? I'm not sure if that's possible given the data structure, but this
seems like it should be an 'else if', to avoid unnecessary scanning.

You also might save yourself a bit if you check for v2 first. Not a big
deal but I thought I'd throw it out there.

> +			ret = afs_parse_v2_partition(mtd, off, &parts[i]);
> +			if (ret)
> +				goto out_free_parts;
> +			i++;
> +		}
>  	}
>  
>  	*pparts = parts;

Brian

[PATCH 10/10] mtd: afs: add v2 partition parsing

[Brian Norris]

I'll finish my review now...

On Thu, Oct 15, 2015 at 03:08:53PM +0200, Linus Walleij wrote:
> The AFS v2 partition type appear in later ARM reference designs
> such as RealView, Versatile Express and the 64bit Juno Development
> Platform.
> 
> The image informations is padded with a 32bit word (4 bytes) on
> the 32bit platforms and a 64bit word (8 bytes) on the 64bit
> platforms. The boot monitor source code gives at hand that this
> is because the first entry in the struct mapped over the image
> information is a "next" pointer for a linked list, filled in
> by firmware after reading in the info block, and always zero
> in the flash. We adjust padding by checking what padding gives
> the right checksum.
> 
> This was tested on:
> - Integrator/AP (v1 partitions)
> - RealView PB11MPCore (v2 32bit partitions)
> - Juno Development System (v2 64bit partitions)
> 
> All systems display the images in flash very nicely as separate
> partitions, e.g on Juno:
> 
> 4 afs partitions found on MTD device 8000000.flash
> Creating 4 MTD partitions on "8000000.flash":
> 0x000000040000-0x0000000c0000 : "fip"
> 0x000000ec0000-0x0000018c0000 : "Image"
> 0x000000f00000-0x000000f40000 : "juno"
> 0x000003ec0000-0x000003f00000 : "bl1"
> 
> Cc: Ryan Harkin <ryan.harkin@linaro.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/afs.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index 1ce6842c917c..e46b035bf4d2 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -3,6 +3,7 @@
>      drivers/mtd/afs.c: ARM Flash Layout/Partitioning
>  
>      Copyright ? 2000 ARM Limited
> +    Copyright (C) 2015 Linus Walleij
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -35,6 +36,8 @@
>  #include <linux/mtd/partitions.h>
>  
>  #define AFSV1_FOOTER_MAGIC 0xA0FFFF9F
> +#define AFSV2_FOOTER_MAGIC1 0x464C5348 /* "FLSH" */
> +#define AFSV2_FOOTER_MAGIC2 0x464F4F54 /* "FOOT" */
>  
>  struct footer_v1 {
>  	u32 image_info_base;	/* Address of first word of ImageFooter  */
> @@ -68,6 +71,22 @@ static u32 word_sum(void *words, int num)
>  	return sum;
>  }
>  
> +static u32 word_sum_v2(u32 *p, u32 num)

Nit: this uses a different prototype than word_sum(). That seems
unnecessarily inconsistent. Why is 'num' u32, when it could easily be
'int'?

> +{
> +	u32 sum = 0;
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		u32 val;
> +
> +		val = p[i];
> +		if (val > ~sum)
> +			sum++;
> +		sum += val;
> +	}
> +	return ~sum;
> +}
> +
>  static bool afs_is_v1(struct mtd_info *mtd, u_int off)
>  {
>  	/* The magic is 12 bytes from the end of the erase block */
> @@ -88,6 +107,27 @@ static bool afs_is_v1(struct mtd_info *mtd, u_int off)
>  	return (magic == AFSV1_FOOTER_MAGIC);
>  }
>  
> +static bool afs_is_v2(struct mtd_info *mtd, u_int off)
> +{
> +	/* The magic is the 8 last bytes of the erase block */
> +	u_int ptr = off + mtd->erasesize - 8;

This comment applies to the v1 refactoring as well: by trying to save
just a few bytes of read, you introduce more magic numbers. It'd be nice
if you didn't do that; either read the whole footer struct, or define
some kind of macros for this.

> +	u32 foot[2];
> +	size_t sz;
> +	int ret;
> +
> +	ret = mtd_read(mtd, ptr, 8, &sz, (u_char *)foot);
> +	if (ret < 0) {
> +		printk(KERN_ERR "AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return false;
> +	}
> +	if (ret >= 0 && sz != 8)
> +		return false;
> +
> +	return (foot[0] == AFSV2_FOOTER_MAGIC1 &&
> +		foot[1] == AFSV2_FOOTER_MAGIC2);

I think you have the same problem as I commented on in v1; you don't
check the checksum here -- causing minor potential memory allocation
waste -- and you do check it later, but you make it fatal -- making you
susceptible to complete failures on false positives; at least now you
have 2 magic numbers to falsely detect, making this less likely, but
still...

> +}
> +
>  static int afs_parse_v1_partition(struct mtd_info *mtd,
>  				  u_int off, struct mtd_partition *part)
>  {
> @@ -185,6 +225,113 @@ static int afs_parse_v1_partition(struct mtd_info *mtd,
>  	return 0;
>  }
>  
> +static int afs_parse_v2_partition(struct mtd_info *mtd,
> +				  u_int off, struct mtd_partition *part)
> +{
> +	u_int ptr;
> +	u32 footer[12];
> +	u32 imginfo[36];

Why don't you define structs for these? This is inconsistent with the v1
code, and it also means you have a lot more magic numbers scattered
around (you could fix this with more macro definitions, but I think a
struct would look nicer). I see that there are comments about 32- vs.
64-bit structs being slightly different, but I'm pretty sure that can be
handled. See comments below.

> +	char *name;
> +	u32 version;
> +	u32 entrypoint;
> +	u32 attributes;
> +	u32 region_count;
> +	u32 block_start;
> +	u32 block_end;
> +	u32 crc;
> +	size_t sz;
> +	int ret;
> +	int i;
> +	int pad = 0;
> +
> +	pr_debug("Parsing v2 partition @%08x-%08x\n",
> +		 off, off + mtd->erasesize);
> +
> +	/* First read the footer */
> +	ptr = off + mtd->erasesize - sizeof(footer);
> +	ret = mtd_read(mtd, ptr, sizeof(footer), &sz, (u_char *)footer);
> +	if ((ret < 0) || (ret >= 0 && sz != sizeof(footer))) {
> +		pr_err("AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return -EIO;
> +	}
> +	name = (char *) &footer[0];
> +	version = footer[9];
> +	ptr = off + mtd->erasesize - sizeof(footer) - footer[8];
> +
> +	pr_debug("found image \"%s\", version %08x, info @%08x\n",
> +		 name, version, ptr);
> +
> +	/* Then read the image information */
> +	ret = mtd_read(mtd, ptr, sizeof(imginfo), &sz, (u_char *)imginfo);
> +	if ((ret < 0) || (ret >= 0 && sz != sizeof(imginfo))) {
> +		pr_err("AFS: mtd read failed at 0x%x: %d\n",
> +		       ptr, ret);
> +		return -EIO;
> +	}
> +
> +	/* 32bit platforms have 4 bytes padding */
> +	crc = word_sum_v2(&imginfo[1], 34);

Magic number 34!

> +	if (!crc) {
> +		pr_debug("Padding 1 word (4 bytes)\n");
> +		pad = 1;
> +	} else {
> +		/* 64bit platforms have 8 bytes padding */

Wait, if this is really a 64- vs. 32-bit difference... can't you just
determine the padding ahead of time? AIUI, you shouldn't have to just
guess, and use the checksum to confirm...

One easy way (I think) is if you actually make the imginfo array into a
struct, you can make the first field a pointer, and the struct will be
sized appropriately on each kernel. e.g.:

	struct image_info_v2 {
		void *link;
		u32 regular_field_1;
		u32 etc;
		...
	};

or if you want to be somewhat clever to make the non-pointer part easily
checksummable and independent, you can embed one struct in another,
like:

	struct image_info_v2 {
		u32 regular_field_1;
		u32 etc;
		...
	};

	struct linked_image_info_v2 {
		void *link;
		struct image_info_v2 iis;
	};

Then you can do things like:

	struct linked_image_info_v2 linked;
	struct image_info_v2 *iis;

	...

	ret = mtd_read(mtd, ptr, sizeof(linked), &sz, (u_char *)&linked);
	...

	iis = &linked.img;

	crc = word_sum_v2(iis, sizeof(*iis) / sizeof(u32));
	...

> +		crc = word_sum_v2(&imginfo[2], 34);

Magic number 34 again! I won't point out the rest, but there are plenty
below.

> +		if (!crc) {
> +			pr_debug("Padding 2 words (8 bytes)\n");
> +			pad = 2;
> +		}
> +	}
> +	if (crc) {
> +		pr_err("AFS: bad checksum on v2 image info: %08x\n", crc);
> +		return -EINVAL;
> +	}
> +	entrypoint = imginfo[pad];

BTW, what's the endianness situation here? Is everything always in
native endianness? Or do we need to do le32_to_cpu() conversions? (Or is
this never tested in big endian?)

> +	attributes = imginfo[pad+1];
> +	region_count = imginfo[pad+2];
> +	block_start = imginfo[20];
> +	block_end = imginfo[21];
> +
> +	pr_debug("image entry=%08x, attr=%08x, regions=%08x, "
> +		 "bs=%08x, be=%08x\n",
> +		 entrypoint, attributes, region_count,
> +		 block_start, block_end);
> +
> +	for (i = 0; i < region_count; i++) {

No error-checking on region_count? You just trust that it won't make you
read off the end of your array/struct?

If you made imginfo[] into a struct, you might have an array field like:

	struct image_info_v2 {
		...
		u32 region_count;
		...
		struct {
			u32 region_load_addr;
			u32 region_size;
			u32 region_offset;
			u32 i_guess_theres_padding_here;
		} regions[how_big_is_this?];
	};

Then you can (before you get into this loop) check for:

	if (region_count > ARRAY_SIZE(iis->regions))
		bail out;

> +		u32 region_load_addr = imginfo[pad + 3 + i*4];
> +		u32 region_size = imginfo[pad + 4 + i*4];
> +		u32 region_offset = imginfo[pad + 5 + i*4];
> +		u32 region_start;
> +		u32 region_end;
> +
> +		pr_debug("  region %d: address: %08x, size: %08x, "
> +			 "offset: %08x\n",
> +			 i,
> +			 region_load_addr,
> +			 region_size,
> +			 region_offset);
> +
> +		region_start = off + region_offset;
> +		region_end = region_start + region_size;
> +		/* Align partition to end of erase block */
> +		region_end += (mtd->erasesize - 1);
> +		region_end &= ~(mtd->erasesize -1);
> +		pr_debug("   partition start = %08x, partition end = %08x\n",
> +			 region_start, region_end);
> +
> +		/* Create one partition per region */

Huh? How can you create one partition per region in this function?
You're only passed a single mtd_partition struct to fill out, but you
have potentially many regions here...

> +		part->name = kstrdup(name, GFP_KERNEL);

...so I guess you're just leaking all but the last region name, since
you keep reassigning part->name with a new string?

Brian

> +		if (!part->name)
> +			return -ENOMEM;
> +		part->offset = region_start;
> +		part->size = region_end - region_start;
> +		part->mask_flags = 0;
> +	}
> +
> +	return 0;
> +}
> +
>  static int parse_afs_partitions(struct mtd_info *mtd,
>  				struct mtd_partition **pparts,
>  				struct mtd_part_parser_data *data)
> @@ -200,6 +347,10 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  			sz += sizeof(struct mtd_partition);
>  			i += 1;
>  		}
> +		if (afs_is_v2(mtd, off)) {
> +			sz += sizeof(struct mtd_partition);
> +			i += 1;
> +		}
>  	}
>  
>  	if (!i)
> @@ -213,13 +364,18 @@ static int parse_afs_partitions(struct mtd_info *mtd,
>  	 * Identify the partitions
>  	 */
>  	for (i = off = 0; off < mtd->size; off += mtd->erasesize) {
> -
>  		if (afs_is_v1(mtd, off)) {
>  			ret = afs_parse_v1_partition(mtd, off, &parts[i]);
>  			if (ret)
>  				goto out_free_parts;
>  			i++;
>  		}
> +		if (afs_is_v2(mtd, off)) {
> +			ret = afs_parse_v2_partition(mtd, off, &parts[i]);
> +			if (ret)
> +				goto out_free_parts;
> +			i++;
> +		}
>  	}
>  
>  	*pparts = parts;
> -- 
> 2.4.3
> 

[PATCH 00/10] ARM MTD AFS v2 partition support

[Linus Walleij]

On Thu, Oct 15, 2015 at 3:08 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> Years ago, the ARM reference platforms moved away from the footer
> format parsed by the current AFS code in MTD. As far as I can
> tell this begun with the RealView reference designs (mach-realview)
> and the new format has been used in RealView, Versatile Express
> and recently the Juno Development platform (64bit).
>
> The code has not been working for newer designs for a long time.
> Proper parsing of the flash image partitions is very helpful when
> you want to replace firmware when running full Linux and none of
> the boot monitors or boot loaders.
>
> After this, AFS parsing will work on all ARM reference designs,
> or well, all I could test. I tested the Integrator/AP, RealView
> PB11MPCore and Juno Development Platform.
>
> The first 9 patches basically refactors the current code to
> make the old partition format more encapsulated, while the last
> patch adds the new v2 format.

If there are no comments on this series, then can it be
applied for v4.4?

Yours,
Linus Walleij

[PATCH 00/10] ARM MTD AFS v2 partition support

[Brian Norris]

Hi Linus,

On Thu, Oct 15, 2015 at 03:08:43PM +0200, Linus Walleij wrote:
> Years ago, the ARM reference platforms moved away from the footer
> format parsed by the current AFS code in MTD. As far as I can
> tell this begun with the RealView reference designs (mach-realview)
> and the new format has been used in RealView, Versatile Express
> and recently the Juno Development platform (64bit).
> 
> The code has not been working for newer designs for a long time.
> Proper parsing of the flash image partitions is very helpful when
> you want to replace firmware when running full Linux and none of
> the boot monitors or boot loaders.
> 
> After this, AFS parsing will work on all ARM reference designs,
> or well, all I could test. I tested the Integrator/AP, RealView
> PB11MPCore and Juno Development Platform.
> 
> The first 9 patches basically refactors the current code to
> make the old partition format more encapsulated, while the last
> patch adds the new v2 format.

Pushed the first 4 to l2-mtd.git/next (for 4.5). I'll move them to
/master when the merge window closes.

I'm reviewing the rest, but I have some comments on patch 5.

Also, do you have a pointer that documents the v2 format? I can probably
find something myself, but I'm a bit lazy, and if you have something
easy to point me to, that'd save me some time :)

Regards,
Brian

[PATCH 00/10] ARM MTD AFS v2 partition support

[Linus Walleij]

On Wed, Nov 11, 2015 at 3:15 AM, Brian Norris
<computersforpeace@gmail.com> wrote:

> Also, do you have a pointer that documents the v2 format? I can probably
> find something myself, but I'm a bit lazy, and if you have something
> easy to point me to, that'd save me some time :)

There are no documents. The documentation is the source code for
the on-board boot monitor (first thing started by the ROM) that
comes with the board on a CD/DVD. A .h file documents the struct
that is simply written to flash as footer and image info. I don't
think I may post it, the structs in the kernel correspond to these
structs.

Yours,
Linus Walleij

[PATCH 00/10] ARM MTD AFS v2 partition support

[Brian Norris]

On Wed, Nov 11, 2015 at 04:13:19PM +0100, Linus Walleij wrote:
> On Wed, Nov 11, 2015 at 3:15 AM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> > Also, do you have a pointer that documents the v2 format? I can probably
> > find something myself, but I'm a bit lazy, and if you have something
> > easy to point me to, that'd save me some time :)
> 
> There are no documents. The documentation is the source code for
> the on-board boot monitor (first thing started by the ROM) that
> comes with the board on a CD/DVD. A .h file documents the struct
> that is simply written to flash as footer and image info. I don't
> think I may post it, the structs in the kernel correspond to these
> structs.

Well, laziness paid off then; I probably wouldn't have been very
successful if I looked myself.

Thanks,
Brian