mtd: ubiformat: Add support for image confirmation

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 <[email protected]>

---
 ubi-utils/ubiformat.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

ubi-utils/ubiformat.c +6 -3

index 21409ca..4129404 100644
@@ -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;
 

ubi-utils/ubiformat.c +7 -0

@@ -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;

On Sat, Jan 3, 2015 at 9:31 PM, hujianyang <[email protected] <mailto:[email protected]>> 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.

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.

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 <[email protected]>

---
 ubi-utils/ubiformat.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

ubi-utils/ubiformat.c +8 -2

index 4129404..3b8a22e 100644
@@ -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;
 

On Sat, Jan 3, 2015 at 9:38 PM, hujianyang <[email protected] <mailto:[email protected]>> 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.