[U-Boot-Users] [PATCH] Support dynamic/patched NAND ENV offset

[Scott Wood]

On Wed, Jul 09, 2008 at 04:11:29PM +0800, Harald Welte wrote:
> +#if defined(CFG_ENV_OFFSET_OOB)

Can you push this conditional into the Makefile?

> +int do_dynenv(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	struct mtd_info *mtd = &nand_info[0];
> +	int ret, size = 8;
> +	struct mtd_oob_ops ops;
> +	uint8_t *buf;
> +
> +	char *cmd = argv[1];
> +
> +	buf = malloc(mtd->oobsize);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ops.mode = MTD_OOB_RAW;

Use MTD_OOB_AUTO, or else you'll conflict with bad block markers, ECC,
etc.

> +	ops.ooboffs = 0;
> +	ops.ooblen = mtd->oobsize;
> +	ops.oobbuf = buf;
> +	ops.datbuf = buf;

You're passing the same buffer for data and oob?  And the buffer is only
oob-sized?

> +	ops.len = size;
> +
> +	ret = mtd->read_oob(mtd, 8, &ops);
> +	if (!strcmp(cmd, "get")) {
> +
> +		if (buf[0] == 'E' && buf[1] == 'N' &&
> +		    buf[2] == 'V' && buf[3] == '0')

Use strcmp().

> +			printf("0x%08x\n", *((u_int32_t *) &buf[4]));

This violates C99 aliasing rules, and could cause unaligned accesses
(likewise elsewhere).

Use a union, or just declare it as uint32_t or u32 (not u_int32_t) and
cast to char for strcmp() or define the magic value as an integer rather
than a string.

The last option is probably the best, in terms of keeping code simple and
small.

> +		else
> +			printf("No dynamic environment marker in OOB block 0\n");
> +
> +	} else if (!strcmp(cmd, "set")) {

No blank lines at beginning/end of blocks.

> +		unsigned long addr, dummy;
> +
> +		if (argc < 3)
> +			goto usage;
> +
> +		buf[0] = 'E';
> +		buf[1] = 'N';
> +		buf[2] = 'V';
> +		buf[3] = '0';
> +
> +		if (arg_off_size(argc-2, argv+2, mtd, &addr, &dummy, 1) < 0) {

Spaces around operators (here and elsewhere).

> +			printf("Offset or partition name expected\n");
> +			goto fail;
> +		}
> +		if (!ret) {

Why are you checking the success of read_oob here, and not for "get"?

> +			uint8_t tmp[4];
> +			int i;
> +
> +			memcpy(&tmp, &addr, 4);
> +			for (i = 0; i != 4; i++)
> +				if (tmp[i] & ~buf[i+4]) {
> +					printf("ERROR: erase OOB block to "
> +					  "write this value\n");
> +					goto fail;
> +				}
> +		}

This would be much simpler if you used integer accesses on a u32 array.

What does "erase OOB block" mean?  Don't you mean "erase block zero"?

Are you really intending to support changing the location multiple times
per erase, as long as bits are only cleared?  Sure, it's technically
possible, but still...

> +		ret = mtd->write_oob(mtd, 8, &ops);
> +		if (!ret)
> +			CFG_ENV_OFFSET = addr;

And silently fail if write_oob fails?

> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index 7c26ceb..a72e553 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -101,7 +101,7 @@ static inline int str2long(char *p, ulong *num)
>  	return (*p != '\0' && *endptr == '\0') ? 1 : 0;
>  }
>  
> -static int
> +int
>  arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size)

This is too generic a name for a global function that is NAND-specific.

-Scott