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

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(-)

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

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