Dear Guennadi Liakhovetski,
In message <[EMAIL PROTECTED]> you wrote:
>
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -44,6 +44,12 @@
> #define CMD_GETENV "fw_printenv"
> #define CMD_SETENV "fw_setenv"
>
> +#define min(x, y) ({ \
> + typeof(x) _min1 = (x); \
> + typeof(y) _min2 = (y); \
> + (void) (&_min1 == &_min2); \
What does this do?
> + _min1 < _min2 ? _min1 : _min2; })
> typedef struct envdev_s {
> char devname[16]; /* Device name */
> ulong devoff; /* Device offset */
> @@ -413,179 +419,290 @@ int fw_setenv (int argc, char *argv[])
> return 0;
> }
>
> +static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
> + loff_t *blockstart, size_t blocklen)
> +{
> + if (mtdinfo->type == MTD_NANDFLASH) {
> + int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
> +
> + if (badblock < 0) {
> + perror ("Cannot read bad block mark");
It would be probably helpful to print the block address.
> + return badblock;
> + }
> +
> + if (badblock) {
> + fprintf (stderr, "Bad block at 0x%llx, "
> + "skipping\n", *blockstart);
> + *blockstart += blocklen;
> + return badblock;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * We are called with count == 0 for backing up as much data from the
> + * range as possible
> + */
Backing up?
> static int flash_read_buf (int dev, int fd, void *buf, size_t count,
> - off_t offset)
> + off_t offset, size_t range)
> {
> + struct mtd_info_user mtdinfo;
> + size_t blocklen, processed = 0;
> + size_t readlen = count ? : range;
> + off_t erase_offset, block_seek;
> + loff_t blockstart;
> int rc;
> + int backup_mode = !count;
backup_mode ?
I think there should be an explanation what exactly you are trying to
do.
> - rc = lseek (fd, offset, SEEK_SET);
> - if (rc == -1) {
> - fprintf (stderr,
> - "seek error on %s: %s\n",
> - DEVNAME (dev), strerror (errno));
> + if (!count)
> + count = range;
> +
> + rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> + if (rc < 0) {
> + perror ("Cannot get MTD information");
> return rc;
> }
Did you verify that the code still builds when MTD_OLD is set?
>
> - rc = read (fd, buf, count);
> - if (rc != count) {
> - fprintf (stderr,
> - "Read error on %s: %s\n",
> - DEVNAME (dev), strerror (errno));
> - return -1;
> + /* Erase sector size is always a power of 2 */
> + erase_offset = offset & ~(mtdinfo.erasesize - 1);
Please explain this logic.
> + blockstart = erase_offset;
> + /* Offset inside a block */
> + block_seek = offset - erase_offset;
> +
> + if (mtdinfo.type == MTD_NANDFLASH) {
> + /*
> + * NAND: calculate which blocks we are reading. We have
> + * to read one block at a time to skip bad blocks.
> + */
> + blocklen = mtdinfo.erasesize;
> + /* Limit to one block for the first read */
> + if (readlen > blocklen - block_seek)
> + readlen = blocklen - block_seek;
> + } else {
> + blocklen = 0;
> }
>
> - return rc;
> + /* This only runs once for NOR flash */
> + while (processed < count) {
> + rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);
But - NOR flash does not have bad block, so all of this is not needed
at all?
> + if (rc < 0)
> + return -1;
> + else if (blockstart + block_seek + readlen > offset + range) {
I do not understand what you are doing here. Comment?
> + /* End of range is reached */
> + if (backup_mode) {
> + return processed;
> + } else {
> + fprintf (stderr,
> + "Too few good blocks within range\n");
> + return -1;
> + }
> + } else if (rc)
> + continue;
> +
> + /*
> + * If a block is bad, we retry in the next block
> + * at the same offset - see common/env_nand.c::
> + * writeenv()
> + */
> + lseek (fd, blockstart + block_seek, SEEK_SET);
I don't see that you remember which blocks were bad. Does that mean
that you will attemopt to write the environment to known bad blocks?
Sonds not like a good idea to me.
> + rc = read (fd, buf + processed, readlen);
> + if (rc != readlen) {
> + fprintf (stderr,
> + "Read error on %s: %s\n",
> + DEVNAME (dev), strerror (errno));
> + return -1;
> + }
> + processed += readlen;
> + readlen = min(blocklen, count - processed);
> + block_seek = 0;
> + blockstart += blocklen;
> + }
> +
> + return processed;
> }
>
> -static int flash_write (void)
> +static int flash_write_buf (int dev, int fd, void *buf, size_t count,
> + off_t offset)
> {
> - int fd_current, fd_target, rc, dev_target;
> - erase_info_t erase_current = {}, erase_target;
> char *data = NULL;
> - off_t erase_offset;
> - struct mtd_info_user mtdinfo_target;
> + erase_info_t erase;
> + struct mtd_info_user mtdinfo;
> + size_t blocklen, erase_len, processed = 0;
> + size_t writelen, write_total = DEVESIZE (dev);
> + off_t erase_offset, block_seek;
> + loff_t blockstart;
> + int rc;
>
> - /* dev_current: fd_current, erase_current */
> - if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
> - fprintf (stderr,
> - "Can't open %s: %s\n",
> - DEVNAME (dev_current), strerror (errno));
> + rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> + if (rc < 0) {
> + perror ("Cannot get MTD information");
> return -1;
> }
>
> - if (HaveRedundEnv) {
> - /* switch to next partition for writing */
> - dev_target = !dev_current;
> - /* dev_target: fd_target, erase_target */
> - if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
> - fprintf (stderr,
> - "Can't open %s: %s\n",
> - DEVNAME (dev_target),
> - strerror (errno));
> - return -1;
> - }
> - } else {
> - dev_target = dev_current;
> - fd_target = fd_current;
> - }
> + /* Erase sector size is always a power of 2 */
> + erase_offset = offset & ~(mtdinfo.erasesize - 1);
> + /* Maximum area we may use */
> + erase_len = (offset - erase_offset + DEVESIZE (dev) +
> + mtdinfo.erasesize - 1) & ~(mtdinfo.erasesize - 1);
> +
> + blockstart = erase_offset;
> + /* Offset inside a block */
> + block_seek = offset - erase_offset;
>
> /*
> * Support environment anywhere within erase sectors: read out the
> * complete area to be erased, replace the environment image, write
> * the whole block back again.
> */
> - if (DEVESIZE (dev_target) > CFG_ENV_SIZE) {
> - data = malloc (DEVESIZE (dev_target));
> + if (erase_len > DEVESIZE (dev)) {
> + data = malloc (erase_len);
> if (!data) {
> fprintf (stderr,
> - "Cannot malloc %lu bytes: %s\n",
> - DEVESIZE (dev_target),
> - strerror (errno));
> + "Cannot malloc %u bytes: %s\n",
> + erase_len, strerror (errno));
> return -1;
> }
>
> - rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
> - if (rc < 0) {
> - perror ("Cannot get MTD information");
> + /*
> + * This is different from a normal read. We have to read as much
> + * as we can from a certain area, and it should be at least X
> + * bytes, instead of having to read a fixed number of bytes as
> + * usual. This also tells us how much data "fits" in the good
> + * blocks in the area.
> + */
> + write_total = flash_read_buf (dev, fd, data, 0,
> + erase_offset, erase_len);
> + if (write_total < block_seek + CFG_ENV_SIZE)
Ummm...this is flash_write_buf(), and we start reading data?
Please explain your code.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [EMAIL PROTECTED]
It would seem that evil retreats when forcibly confronted
-- Yarnek of Excalbia, "The Savage Curtain", stardate 5906.5
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
