[PATCH] nandwrite: new mtd-utils applet

Baruch Siach baruch at tkos.co.il
Mon Aug 23 04:54:05 UTC 2010


Hi Denys,

Thanks for your review. See my comments below.

On Sun, Aug 22, 2010 at 07:24:19PM +0200, Denys Vlasenko wrote:
> On Wednesday 18 August 2010 14:32, Baruch Siach wrote:
> > Signed-off-by: Baruch Siach <baruch at tkos.co.il>
> > ---
> >  miscutils/nandwrite.c |  130 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 130 insertions(+), 0 deletions(-)
> >  create mode 100644 miscutils/nandwrite.c
> > 
> > diff --git a/miscutils/nandwrite.c b/miscutils/nandwrite.c
> > new file mode 100644
> > index 0000000..f5ee0be
> > --- /dev/null
> > +++ b/miscutils/nandwrite.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * nandwrite.c - ported to busybox from mtd-utils
> > + *
> > + * Author: Baruch Siach <baruch at tkos.co.il>, Orex Computed Radiography
> > + *
> > + * Licensed under GPLv2, see file LICENSE in this source tree.
> > + *
> > + * TODO: add support for large (>4GB) MTD devices
> > + */
> > +
> > +//applet:IF_NANDWRITE(APPLET(nandwrite, _BB_DIR_USR_SBIN, _BB_SUID_DROP))
> > +
> > +//kbuild:lib-$(CONFIG_NANDWRITE) += nandwrite.o
> > +
> > +//config:config NANDWRITE
> > +//config:	bool "nandwrite"
> > +//config:	default n
> > +//config:	depends on PLATFORM_LINUX
> > +//config:	help
> > +//config:	  Write to the specified MTD device, with bad blocks awareness
> > +
> > +#include "libbb.h"
> > +#include <mtd/mtd-user.h>
> > +
> > +#define OPTION_P	(1 << 0)
> > +#define OPTION_S	(1 << 1)
> > +
> > +//usage:#define nandwrite_trivial_usage
> > +//usage:	"MTD_DEVICE [-p] [-s ADDR] [INPUTFILE | -]"
> > +//usage:#define nandwrite_full_usage "\n\n"
> > +//usage:	"Write to the specified MTD device\n"
> > +//usage:	"\nOptions:"
> > +//usage:	"\n	-p	Pad to page size"
> > +//usage:	"\n	-s addr	Set start address (default is 0)"
> > +
> > +static unsigned next_good_eraseblock(int fd, struct mtd_info_user *meminfo, 
> > +		unsigned block_offset)
> > +{
> > +	loff_t offs = block_offset;
> > +
> > +	while (1) {
> > +		if (block_offset >= meminfo->size)
> > +			bb_error_msg_and_die("not enough space in MTD device");
> > +		if (xioctl(fd, MEMGETBADBLOCK, &offs)) {
> 
> xioctl does not return on error. Maybe you need to use plain
> ioctl?

This ioctl returns 1 on bad block, and 0 otherwise. Other errors should 
terminate the process with the appropriate errno message. So, xioctl is the 
right choice here.

> > +			offs += meminfo->erasesize;
> > +			printf("Skipping bad block at 0x%08llx\n", offs);
> > +		} else
> > +			return offs;
> > +	}
> > +
> > +}
> > +
> > +int nandwrite_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> > +int nandwrite_main(int argc, char **argv)
> > +{
> > +	unsigned opts = 0;
> > +	int opt;
> > +	char *mtd_device, *img = NULL;
> > +	unsigned mtdoffset = 0, blockstart;
> > +	int fd, ifd = STDIN_FILENO;
> > +	struct mtd_info_user meminfo;
> > +	unsigned char *filebuf;
> > +	ssize_t cnt = -1;
> > +
> > +	/* TODO: switch to getopt32 once it supports strtou */
> > +	while ((opt = getopt(argc, argv, "ps:")) != -1) {
> 
> Do use getopt32.

And how do I do the strtou conversion with getopt32?

> and btw, why mtdoffset is not off_t? MTDs can be larger than 4G, right?

This is how upstream nandwrite is implemented. Upstream also does not support 
MTDs larger than 4GB (yet?), since it uses the 32bit limited MEMERASE ioctl.  
I'm reluctant to extend the Busybox version of nandwrite beyond the 
capabilities of upstream, because upstream might implement this in an 
incompatible way in the future.  IMO we should wait for this support to appear 
upstream first, and then catch up. What do you think?

> > +	argc -= optind;
> > +	if (argc < 1 || argc > 2)
> > +		bb_show_usage();
> > +	mtd_device = argv[optind];
> > +	if (argc == 2)
> > +		img = argv[optind+1];
> 
> I am sure using only argv is smaller code. Something like:

OK.

> 	argv += optind;
> 	mtd_device = argv[0];
> 	if (!argv[0] || (argv[1] && argv[2]))
> 		bb_show_usage();
> 	if (argv[1])
> 		img = argv[1];
> 
> > +	if (img && strcmp(img, "-") != 0)
> > +		ifd = xopen(img, O_RDONLY);
> 
> Use xopen_stdin() - you can drop check for "-" then.

Thanks for the tip. Will do.

> > +	filebuf = xmalloc(meminfo.writesize);
> > +
> > +	while (mtdoffset < meminfo.size) {
> > +		blockstart = mtdoffset & (~meminfo.erasesize + 1);
> > +		if (blockstart == mtdoffset) {
> > +			/* starting a new eraseblock */
> > +			mtdoffset = next_good_eraseblock(fd, &meminfo,
> > +					blockstart);
> > +			printf("Writing at 0x%08x\n", mtdoffset);
> > +		}
> > +
> > +		/* get some more data from input */
> > +		cnt = full_read(ifd, filebuf, meminfo.writesize);
> > +		if (cnt == 0)
> > +			break; /* we are done */
> > +		if (cnt < meminfo.writesize && !(opts & OPTION_P))
> > +			bb_error_msg_and_die("Unexpected EOF, "
> > +					"use padding");
> > +
> > +		xlseek(fd, mtdoffset, SEEK_SET);
> > +		xwrite(fd, filebuf, meminfo.writesize);
> 
> Where's the zero-padding for cnt < meminfo.writesize and -p case?

Will fix.

> > +		mtdoffset += meminfo.writesize;
> > +	}

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -


More information about the busybox mailing list