[PATCH 0/2] mtd: ubiformat: Add support for image confirmation

[PATCH 0/2] mtd: ubiformat: Add support for image confirmation

[Joe Balough]

Working with ubi images being flashed to NAND using ubiformat, I
found it desirable to ensure that the data that has been flashed is 
properly written to the device using a readback mechanism.

These patches add a -c, --confirm argument to ubiformat that will
cause the program to read back the eraseblock written in the
flash_image function, then use a memcmp to verify it was written
correctly. 

This is my first patch and I am happy to make any requested changes.

Joe Balough (2):
  mtd: ubiformat: Add --confirm argument
  mtd: ubiformat: Add confirmation fail exit

 ubi-utils/ubiformat.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

-- 
2.1.3

[PATCH 1/2] mtd: ubiformat: Add --confirm argument

[Joe Balough]

Add support for an argument to ubiformat that will read back every
written eraseblock and verify the read data matches the written data.

Signed-off-by: Joe Balough <jbb5044@gmail.com>
---
 ubi-utils/ubiformat.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
index 21409ca..4129404 100644
--- a/ubi-utils/ubiformat.c
+++ b/ubi-utils/ubiformat.c
@@ -49,6 +49,7 @@
 
 /* The variables below are set by command line arguments */
 struct args {
+	unsigned int confirm:1;
 	unsigned int yes:1;
 	unsigned int quiet:1;
 	unsigned int verbose:1;
@@ -93,6 +94,7 @@ static const char optionsstr[] =
 "                             (default is 1)\n"
 "-Q, --image-seq=<num>        32-bit UBI image sequence number to use\n"
 "                             (by default a random number is picked)\n"
+"-c, --confirm                Read back written value and verify it matches\n"
 "-y, --yes                    assume the answer is \"yes\" for all question\n"
 "                             this program would otherwise ask\n"
 "-q, --quiet                  suppress progress percentage information\n"
@@ -105,8 +107,8 @@ static const char usage[] =
 "\t\t\t[-Q <num>] [-f <file>] [-S <bytes>] [-e <value>] [-x <num>] [-y] [-q] [-v] [-h]\n"
 "\t\t\t[--sub-page-size=<bytes>] [--vid-hdr-offset=<offs>] [--no-volume-table]\n"
 "\t\t\t[--flash-image=<file>] [--image-size=<bytes>] [--erase-counter=<value>]\n"
-"\t\t\t[--image-seq=<num>] [--ubi-ver=<num>] [--yes] [--quiet] [--verbose]\n"
-"\t\t\t[--help] [--version]\n\n"
+"\t\t\t[--image-seq=<num>] [--ubi-ver=<num>] [--confirm] [--yes] [--quiet]\n"
+"\t\t\t[--verbose] [--help] [--version]\n\n"
 "Example 1: " PROGRAM_NAME " /dev/mtd0 -y - format MTD device number 0 and do\n"
 "           not ask questions.\n"
 "Example 2: " PROGRAM_NAME " /dev/mtd0 -q -e 0 - format MTD device number 0,\n"
@@ -118,6 +120,7 @@ static const struct option long_options[] = {
 	{ .name = "no-volume-table", .has_arg = 0, .flag = NULL, .val = 'n' },
 	{ .name = "flash-image",     .has_arg = 1, .flag = NULL, .val = 'f' },
 	{ .name = "image-size",      .has_arg = 1, .flag = NULL, .val = 'S' },
+	{ .name = "confirm",         .has_arg = 0, .flag = NULL, .val = 'c' },
 	{ .name = "yes",             .has_arg = 0, .flag = NULL, .val = 'y' },
 	{ .name = "erase-counter",   .has_arg = 1, .flag = NULL, .val = 'e' },
 	{ .name = "quiet",           .has_arg = 0, .flag = NULL, .val = 'q' },
@@ -137,7 +140,7 @@ static int parse_opt(int argc, char * const argv[])
 		int key, error = 0;
 		unsigned long int image_seq;
 
-		key = getopt_long(argc, argv, "nh?Vyqve:x:s:O:f:S:", long_options, NULL);
+		key = getopt_long(argc, argv, "nh?Vycqve:x:s:O:f:S:", long_options, NULL);
 		if (key == -1)
 			break;
 
@@ -178,6 +181,10 @@ static int parse_opt(int argc, char * const argv[])
 		case 'n':
 			args.novtbl = 1;
 			break;
+			
+		case 'c':
+			args.confirm = 1;
+			break;
 
 		case 'y':
 			args.yes = 1;
@@ -535,6 +542,20 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 			skip_data_read = 1;
 			continue;
 		}
+		
+		if (args.confirm) {
+			char read_buf[mtd->eb_size];
+			err = mtd_read(mtd, args.node_fd, eb, 0, read_buf, new_len);
+			if (err) {
+				sys_errmsg("cannot readback eraseblock %d for confirmation", eb);
+				goto out_close;
+			}
+			
+			if (memcmp(buf, read_buf, new_len) != 0) {
+				sys_errmsg("readback failed on eraseblock %d", eb);
+			}
+		}
+		
 		if (++written_ebs >= img_ebs)
 			break;
 	}
-- 
2.1.3

Re: [PATCH 1/2] mtd: ubiformat: Add --confirm argument

[hujianyang]

Hi Joe,

On 2014/12/31 22:58, Joe Balough wrote:
> Add support for an argument to ubiformat that will read back every
> written eraseblock and verify the read data matches the written data.
> 
> Signed-off-by: Joe Balough <jbb5044@gmail.com>
> ---
>  ubi-utils/ubiformat.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
> index 21409ca..4129404 100644
> --- a/ubi-utils/ubiformat.c
> +++ b/ubi-utils/ubiformat.c
> @@ -49,6 +49,7 @@
>  
>  /* The variables below are set by command line arguments */
>  struct args {
> +	unsigned int confirm:1;
>  	unsigned int yes:1;
>  	unsigned int quiet:1;
>  	unsigned int verbose:1;
> @@ -93,6 +94,7 @@ static const char optionsstr[] =
>  "                             (default is 1)\n"
>  "-Q, --image-seq=<num>        32-bit UBI image sequence number to use\n"
>  "                             (by default a random number is picked)\n"
> +"-c, --confirm                Read back written value and verify it matches\n"
>  "-y, --yes                    assume the answer is \"yes\" for all question\n"
>  "                             this program would otherwise ask\n"
>  "-q, --quiet                  suppress progress percentage information\n"
> @@ -105,8 +107,8 @@ static const char usage[] =
>  "\t\t\t[-Q <num>] [-f <file>] [-S <bytes>] [-e <value>] [-x <num>] [-y] [-q] [-v] [-h]\n"
>  "\t\t\t[--sub-page-size=<bytes>] [--vid-hdr-offset=<offs>] [--no-volume-table]\n"
>  "\t\t\t[--flash-image=<file>] [--image-size=<bytes>] [--erase-counter=<value>]\n"
> -"\t\t\t[--image-seq=<num>] [--ubi-ver=<num>] [--yes] [--quiet] [--verbose]\n"
> -"\t\t\t[--help] [--version]\n\n"
> +"\t\t\t[--image-seq=<num>] [--ubi-ver=<num>] [--confirm] [--yes] [--quiet]\n"
> +"\t\t\t[--verbose] [--help] [--version]\n\n"
>  "Example 1: " PROGRAM_NAME " /dev/mtd0 -y - format MTD device number 0 and do\n"
>  "           not ask questions.\n"
>  "Example 2: " PROGRAM_NAME " /dev/mtd0 -q -e 0 - format MTD device number 0,\n"
> @@ -118,6 +120,7 @@ static const struct option long_options[] = {
>  	{ .name = "no-volume-table", .has_arg = 0, .flag = NULL, .val = 'n' },
>  	{ .name = "flash-image",     .has_arg = 1, .flag = NULL, .val = 'f' },
>  	{ .name = "image-size",      .has_arg = 1, .flag = NULL, .val = 'S' },
> +	{ .name = "confirm",         .has_arg = 0, .flag = NULL, .val = 'c' },
>  	{ .name = "yes",             .has_arg = 0, .flag = NULL, .val = 'y' },
>  	{ .name = "erase-counter",   .has_arg = 1, .flag = NULL, .val = 'e' },
>  	{ .name = "quiet",           .has_arg = 0, .flag = NULL, .val = 'q' },
> @@ -137,7 +140,7 @@ static int parse_opt(int argc, char * const argv[])
>  		int key, error = 0;
>  		unsigned long int image_seq;
>  
> -		key = getopt_long(argc, argv, "nh?Vyqve:x:s:O:f:S:", long_options, NULL);
> +		key = getopt_long(argc, argv, "nh?Vycqve:x:s:O:f:S:", long_options, NULL);
>  		if (key == -1)
>  			break;
>  
> @@ -178,6 +181,10 @@ static int parse_opt(int argc, char * const argv[])
>  		case 'n':
>  			args.novtbl = 1;
>  			break;
> +			
> +		case 'c':
> +			args.confirm = 1;
> +			break;
>  
>  		case 'y':
>  			args.yes = 1;

This option 'c' seems only use when *flash-image* is specified. So maybe you
can check the validity of this option depending on *args.image* in parse_opt().

> @@ -535,6 +542,20 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
>  			skip_data_read = 1;
>  			continue;
>  		}
> +		
> +		if (args.confirm) {
> +			char read_buf[mtd->eb_size];
> +			err = mtd_read(mtd, args.node_fd, eb, 0, read_buf, new_len);
> +			if (err) {
> +				sys_errmsg("cannot readback eraseblock %d for confirmation", eb);
> +				goto out_close;
> +			}
> +			
> +			if (memcmp(buf, read_buf, new_len) != 0) {
> +				sys_errmsg("readback failed on eraseblock %d", eb);
> +			}
> +		}
> +		
>  		if (++written_ebs >= img_ebs)
>  			break;
>  	}
> 

Do we have a function like *mtd_write_and_check()*? I think the operation
first write and then check it by reading may be used in many cases.

It's worthy to check the writing data of the *flash-image* without any
additional options in my considering.

Thanks,
Hu

Re: [PATCH 1/2] mtd: ubiformat: Add --confirm argument

[hujianyang]

On 2015/1/7 4:27, Joseph Balough wrote:
> 
> 
> On Sat, Jan 3, 2015 at 9:31 PM, hujianyang <hujianyang@huawei.com <mailto:hujianyang@huawei.com>> wrote:
> 
>     This option 'c' seems only use when *flash-image* is specified. So maybe you
>     can check the validity of this option depending on *args.image* in parse_opt().
> 
> 
> That's a good idea. I'll do that and send an updated patch.
> 
>     Do we have a function like *mtd_write_and_check()*? I think the operation
>     first write and then check it by reading may be used in many cases.
> 
> 
> I looked through the libmtd source and didn't see an already existing mtd_write_and_check function (or anything like it). And in the whole mtd-utils repository, only ubiformat and nandwrite actually use the mtd_write function itself.
> 

Er, I'm not sure if it's related to CONFIG_MTD_NAND_VERIFY_WRITE or
something else.

Actually, I'm writing a script to check nand state. This script will
first write some bytes to nand and then read back to check. After
repeating several times, production team will ensure whether the nand
in their product is in right state and sell them out.

That's why I need a function like *mtd_write_and_check*. I don't think
it's necessarily for your need.


> 
>     It's worthy to check the writing data of the *flash-image* without any
>     additional options in my considering.
> 
> 
> I feel like most programs like this require an additional argument to check the written data which is why I made it a separate argument. I can change it to be default behavior if that is desired.
> 

I'm not the maintainer of the utility, you can resend a new version and
wait for Artem's reply.

Thanks,
Hu

[PATCH 2/2] mtd: ubiformat: Add confirmation fail exit

[Joe Balough]

If flashing fails, ubiformat will exit with the value of 1.
If confirmation fails, ubiformat will exit with the value of 2.

Signed-off-by: Joe Balough <jbb5044@gmail.com>
---
 ubi-utils/ubiformat.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
index 4129404..3b8a22e 100644
--- a/ubi-utils/ubiformat.c
+++ b/ubi-utils/ubiformat.c
@@ -548,7 +548,7 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 			err = mtd_read(mtd, args.node_fd, eb, 0, read_buf, new_len);
 			if (err) {
 				sys_errmsg("cannot readback eraseblock %d for confirmation", eb);
-				goto out_close;
+				goto out_confirm_fail;
 			}
 			
 			if (memcmp(buf, read_buf, new_len) != 0) {
@@ -568,6 +568,10 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 out_close:
 	close(fd);
 	return -1;
+	
+out_confirm_fail:
+	close(fd);
+	return -2;
 }
 
 static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
@@ -702,7 +706,7 @@ out_free:
 
 int main(int argc, char * const argv[])
 {
-	int err, verbose;
+	int err, verbose, confirm_fail = 0;
 	libmtd_t libmtd;
 	struct mtd_info mtd_info;
 	struct mtd_dev_info mtd;
@@ -932,6 +936,8 @@ int main(int argc, char * const argv[])
 
 	if (args.image) {
 		err = flash_image(libmtd, &mtd, &ui, si);
+		if (err == -2)
+			confirm_fail = 1;
 		if (err < 0)
 			goto out_free;
 
@@ -955,5 +961,8 @@ out_close:
 	close(args.node_fd);
 out_close_mtd:
 	libmtd_close(libmtd);
-	return -1;
+	if (confirm_fail)
+		return 2;
+	else
+		return 1;
 }
-- 
2.1.3

Re: [PATCH 2/2] mtd: ubiformat: Add confirmation fail exit

[hujianyang]

On 2014/12/31 22:58, Joe Balough wrote:
> If flashing fails, ubiformat will exit with the value of 1.
> If confirmation fails, ubiformat will exit with the value of 2.
> 
> Signed-off-by: Joe Balough <jbb5044@gmail.com>
> ---
>  ubi-utils/ubiformat.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
> index 4129404..3b8a22e 100644
> --- a/ubi-utils/ubiformat.c
> +++ b/ubi-utils/ubiformat.c
> @@ -548,7 +548,7 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
>  			err = mtd_read(mtd, args.node_fd, eb, 0, read_buf, new_len);
>  			if (err) {
>  				sys_errmsg("cannot readback eraseblock %d for confirmation", eb);
> -				goto out_close;
> +				goto out_confirm_fail;
>  			}
>  			
>  			if (memcmp(buf, read_buf, new_len) != 0) {
> @@ -568,6 +568,10 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
>  out_close:
>  	close(fd);
>  	return -1;
> +	
> +out_confirm_fail:
> +	close(fd);
> +	return -2;
>  }
>  
>  static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
> @@ -702,7 +706,7 @@ out_free:
>  
>  int main(int argc, char * const argv[])
>  {
> -	int err, verbose;
> +	int err, verbose, confirm_fail = 0;
>  	libmtd_t libmtd;
>  	struct mtd_info mtd_info;
>  	struct mtd_dev_info mtd;
> @@ -932,6 +936,8 @@ int main(int argc, char * const argv[])
>  
>  	if (args.image) {
>  		err = flash_image(libmtd, &mtd, &ui, si);
> +		if (err == -2)
> +			confirm_fail = 1;
>  		if (err < 0)
>  			goto out_free;
>  
> @@ -955,5 +961,8 @@ out_close:
>  	close(args.node_fd);
>  out_close_mtd:
>  	libmtd_close(libmtd);
> -	return -1;
> +	if (confirm_fail)
> +		return 2;
> +	else
> +		return 1;
>  }
> 

There is not need to return different values for any error cases, and a
positive return value is not proper for a aborted case.

Just print a error message at failure happening is enough, I think.

Re: [PATCH 2/2] mtd: ubiformat: Add confirmation fail exit

[hujianyang]

On 2015/1/5 23:07, Joseph Balough wrote:
> 
> On Sat, Jan 3, 2015 at 9:38 PM, hujianyang <hujianyang@huawei.com <mailto:hujianyang@huawei.com>> wrote:
> 
> 
>     There is not need to return different values for any error cases, and a
>     positive return value is not proper for a aborted case.
> 
>     Just print a error message at failure happening is enough, I think.
> 
> 
> It was useful for me to know what the specific error was -- A messed up ubi file could indicate a bad download while a confirm error could indicate bad flash. It's no problem for me, I can just leave my patch as something I apply locally. I do wonder about the current exit value on error. ubiformat currently exits -1 when there is an error. The exit status for programs in UNIX are 8-bit unsigned integers, so the program actually exits 255. Perhaps this should at least return 1 instead of -1.

The exit status for programs in UNIX are 8-bit unsigned integers, so the program actually exits 255. Perhaps this should at least return 1 instead of -1.


Yes, you are right~!

Sorry for it.