From f29c5ca33df4c77b9af2cbfb7ed90bf336613522 Mon Sep 17 00:00:00 2001 From: Rafał Miłecki Date: Tue, 12 Dec 2023 18:33:27 +0100 Subject: fw_env: fix reading NVMEM device's "compatible" value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call to fread() was changed to check for return value. The problem is it can't be checked for returning 1 (as it is) to determine success. We call fread() with buffer size as "size" argument. Reading any "compatible" value shorter than buffer size will result in returning 0 even on success. Modify code to use fstat() to determine expected read length. This fixes regression that broke using fw_env with NVMEM devices. Fixes: c059a22b7776 ("tools: env: fw_env: Fix unused-result warning") Cc: Jaehoon Chung Signed-off-by: Rafał Miłecki --- tools/env/fw_env.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index c9a8774acef..bf8b7ba3042 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1732,6 +1732,7 @@ static int find_nvmem_device(void) } while (!nvmem && (dent = readdir(dir))) { + struct stat s; FILE *fp; size_t size; @@ -1749,14 +1750,22 @@ static int find_nvmem_device(void) continue; } - size = fread(buf, sizeof(buf), 1, fp); + if (fstat(fileno(fp), &s)) { + fprintf(stderr, "Failed to fstat %s\n", comp); + goto next; + } + + if (s.st_size >= sizeof(buf)) { + goto next; + } + + size = fread(buf, s.st_size, 1, fp); if (size != 1) { fprintf(stderr, "read failed about %s\n", comp); - fclose(fp); - return -EIO; + goto next; } - + buf[s.st_size] = '\0'; if (!strcmp(buf, "u-boot,env")) { bytes = asprintf(&nvmem, "%s/%s/nvmem", path, dent->d_name); @@ -1765,6 +1774,7 @@ static int find_nvmem_device(void) } } +next: fclose(fp); } -- cgit v1.3.1 From 73b30800f56883a26a24f87acb30d21cd4dd25d4 Mon Sep 17 00:00:00 2001 From: Rafał Miłecki Date: Wed, 13 Dec 2023 10:23:44 +0100 Subject: fw_env: rename variables holding env data content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename "addr0" and "addr1" to "buf0" and "buf1" accordingly. Name "addr" suggests that variable contains a numeric value being some kind of address. Name "buf" is de facto a standard name for pointer to allocated memory for reading data to. While at it drop redundant checks for NULL before calling free(). Signed-off-by: Rafał Miłecki --- tools/env/fw_env.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index bf8b7ba3042..0347e9d15a5 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1416,11 +1416,11 @@ int fw_env_open(struct env_opts *opts) { int crc0, crc0_ok; unsigned char flag0; - void *addr0 = NULL; + void *buf0 = NULL; int crc1, crc1_ok; unsigned char flag1; - void *addr1 = NULL; + void *buf1 = NULL; int ret; @@ -1430,8 +1430,8 @@ int fw_env_open(struct env_opts *opts) if (parse_config(opts)) /* should fill envdevices */ return -EINVAL; - addr0 = calloc(1, CUR_ENVSIZE); - if (addr0 == NULL) { + buf0 = calloc(1, CUR_ENVSIZE); + if (buf0 == NULL) { fprintf(stderr, "Not enough memory for environment (%ld bytes)\n", CUR_ENVSIZE); @@ -1440,13 +1440,13 @@ int fw_env_open(struct env_opts *opts) } dev_current = 0; - if (flash_io(O_RDONLY, addr0, CUR_ENVSIZE)) { + if (flash_io(O_RDONLY, buf0, CUR_ENVSIZE)) { ret = -EIO; goto open_cleanup; } if (!have_redund_env) { - struct env_image_single *single = addr0; + struct env_image_single *single = buf0; crc0 = crc32(0, (uint8_t *)single->data, ENV_SIZE); crc0_ok = (crc0 == single->crc); @@ -1458,12 +1458,12 @@ int fw_env_open(struct env_opts *opts) environment.dirty = 1; } - environment.image = addr0; + environment.image = buf0; environment.crc = &single->crc; environment.flags = NULL; environment.data = single->data; } else { - struct env_image_redundant *redundant0 = addr0; + struct env_image_redundant *redundant0 = buf0; struct env_image_redundant *redundant1; crc0 = crc32(0, (uint8_t *)redundant0->data, ENV_SIZE); @@ -1472,17 +1472,17 @@ int fw_env_open(struct env_opts *opts) flag0 = redundant0->flags; dev_current = 1; - addr1 = calloc(1, CUR_ENVSIZE); - if (addr1 == NULL) { + buf1 = calloc(1, CUR_ENVSIZE); + if (buf1 == NULL) { fprintf(stderr, "Not enough memory for environment (%ld bytes)\n", CUR_ENVSIZE); ret = -ENOMEM; goto open_cleanup; } - redundant1 = addr1; + redundant1 = buf1; - if (flash_io(O_RDONLY, addr1, CUR_ENVSIZE)) { + if (flash_io(O_RDONLY, buf1, CUR_ENVSIZE)) { ret = -EIO; goto open_cleanup; } @@ -1571,17 +1571,17 @@ int fw_env_open(struct env_opts *opts) * flags before writing out */ if (dev_current) { - environment.image = addr1; + environment.image = buf1; environment.crc = &redundant1->crc; environment.flags = &redundant1->flags; environment.data = redundant1->data; - free(addr0); + free(buf0); } else { - environment.image = addr0; + environment.image = buf0; environment.crc = &redundant0->crc; environment.flags = &redundant0->flags; environment.data = redundant0->data; - free(addr1); + free(buf1); } #ifdef DEBUG fprintf(stderr, "Selected env in %s\n", DEVNAME(dev_current)); @@ -1590,11 +1590,8 @@ int fw_env_open(struct env_opts *opts) return 0; open_cleanup: - if (addr0) - free(addr0); - - if (addr1) - free(addr1); + free(buf0); + free(buf1); return ret; } -- cgit v1.3.1 From 9e3003f79d168eac7ee65cd457e3904e2fb4eea8 Mon Sep 17 00:00:00 2001 From: Rafał Miłecki Date: Wed, 13 Dec 2023 13:13:54 +0100 Subject: fw_env: keep calling read() until whole flash block is read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's totally valid for read() to provide less bytes than requested maximum. It may happen if there is no more data available yet or source pushes data in small chunks. This actually happens when trying to read env data from NVMEM device. Kernel may provide NVMEM content in page size parts (like 4096 B). This fixes warnings like: Warning on /sys/bus/nvmem/devices/u-boot-env0/nvmem: Attempted to read 16384 bytes but got 4096 Warning on /sys/bus/nvmem/devices/u-boot-env0/nvmem: Attempted to read 12288 bytes but got 4096 Warning on /sys/bus/nvmem/devices/u-boot-env0/nvmem: Attempted to read 8192 bytes but got 4096 Since the main loop in flash_read_buf() is used to read blocks this patch adds a new nested one. Signed-off-by: Rafał Miłecki --- tools/env/fw_env.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 0347e9d15a5..90010dabd82 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -948,29 +948,25 @@ static int flash_read_buf(int dev, int fd, void *buf, size_t count, */ lseek(fd, blockstart + block_seek, SEEK_SET); - rc = read(fd, buf + processed, readlen); - if (rc == -1) { - fprintf(stderr, "Read error on %s: %s\n", - DEVNAME(dev), strerror(errno)); - return -1; - } + while (readlen) { + rc = read(fd, buf + processed, readlen); + if (rc == -1) { + fprintf(stderr, "Read error on %s: %s\n", + DEVNAME(dev), strerror(errno)); + return -1; + } #ifdef DEBUG - fprintf(stderr, "Read 0x%x bytes at 0x%llx on %s\n", - rc, (unsigned long long)blockstart + block_seek, - DEVNAME(dev)); + fprintf(stderr, "Read 0x%x bytes at 0x%llx on %s\n", + rc, (unsigned long long)blockstart + block_seek, + DEVNAME(dev)); #endif - processed += rc; - if (rc != readlen) { - fprintf(stderr, - "Warning on %s: Attempted to read %zd bytes but got %d\n", - DEVNAME(dev), readlen, rc); + processed += rc; readlen -= rc; - block_seek += rc; - } else { - blockstart += blocklen; - readlen = min(blocklen, count - processed); - block_seek = 0; } + + blockstart += blocklen; + readlen = min(blocklen, count - processed); + block_seek = 0; } return processed; -- cgit v1.3.1 From d73a6641868029b5cae53ed00c5766921c9d8b1f Mon Sep 17 00:00:00 2001 From: Anthony Loiseau Date: Thu, 21 Dec 2023 23:44:38 +0100 Subject: fw_env: autodetect NAND erase size and env sectors As already done for NOR chips, if device ESIZE and ENVSECTORS static configurations are both zero, then autodetect them at runtime. Cc: Joe Hershberger cc: Stefan Agner cc: Rasmus Villemoes Signed-off-by: Anthony Loiseau --- tools/env/README | 3 +++ tools/env/fw_env.c | 11 +++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/tools/env/README b/tools/env/README index 480a893202f..b8c6a7e1974 100644 --- a/tools/env/README +++ b/tools/env/README @@ -58,6 +58,9 @@ DEVICEx_ENVSECTORS defines the number of sectors that may be used for this environment instance. On NAND this is used to limit the range within which bad blocks are skipped, on NOR it is not used. +If DEVICEx_ESIZE and DEVICEx_ENVSECTORS are both zero, then a runtime +detection is attempted for NOR and NAND mtd types. + To prevent losing changes to the environment and to prevent confusing the MTD drivers, a lock file at /run/fw_printenv.lock is used to serialize access to the environment. diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 90010dabd82..74451ecb945 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1652,8 +1652,15 @@ static int check_device_config(int dev) } DEVTYPE(dev) = mtdinfo.type; if (DEVESIZE(dev) == 0 && ENVSECTORS(dev) == 0 && - mtdinfo.type == MTD_NORFLASH) - DEVESIZE(dev) = mtdinfo.erasesize; + mtdinfo.erasesize > 0) { + if (mtdinfo.type == MTD_NORFLASH) + DEVESIZE(dev) = mtdinfo.erasesize; + else if (mtdinfo.type == MTD_NANDFLASH) { + DEVESIZE(dev) = mtdinfo.erasesize; + ENVSECTORS(dev) = + mtdinfo.size / mtdinfo.erasesize; + } + } if (DEVESIZE(dev) == 0) /* Assume the erase size is the same as the env-size */ DEVESIZE(dev) = ENVSIZE(dev); -- cgit v1.3.1