* [U-Boot] [PATCH 1/2] tools/env: complete environment device config early
@ 2016-07-14 0:14 Stefan Agner
2016-07-14 0:14 ` [U-Boot] [PATCH 2/2] tools/env: allow negative offsets Stefan Agner
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Stefan Agner @ 2016-07-14 0:14 UTC (permalink / raw)
To: u-boot
From: Stefan Agner <stefan.agner@toradex.com>
Currently flash_read completes a crucial part of the environment
device configuration, the device type (mtd_type). This is rather
confusing as flash_io calls flash_read conditionally, and one might
think flash_write, which also makes use of mtd_type, gets called
before flash_read. But since flash_io is always called with O_RDONLY
first, this is not actually the case in reality.
However, it is much cleaner to complete and verify the config early
in parse_config. This also prepares the code for further extension.
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---
tools/env/fw_env.c | 110 +++++++++++++++++++++++++++++------------------------
1 file changed, 60 insertions(+), 50 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 692abda..06d6865 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1021,41 +1021,10 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
static int flash_read (int fd)
{
- struct mtd_info_user mtdinfo;
- struct stat st;
int rc;
- rc = fstat(fd, &st);
- if (rc < 0) {
- fprintf(stderr, "Cannot stat the file %s\n",
- DEVNAME(dev_current));
- return -1;
- }
-
- if (S_ISCHR(st.st_mode)) {
- rc = ioctl(fd, MEMGETINFO, &mtdinfo);
- if (rc < 0) {
- fprintf(stderr, "Cannot get MTD information for %s\n",
- DEVNAME(dev_current));
- return -1;
- }
- if (mtdinfo.type != MTD_NORFLASH &&
- mtdinfo.type != MTD_NANDFLASH &&
- mtdinfo.type != MTD_DATAFLASH &&
- mtdinfo.type != MTD_UBIVOLUME) {
- fprintf (stderr, "Unsupported flash type %u on %s\n",
- mtdinfo.type, DEVNAME(dev_current));
- return -1;
- }
- } else {
- memset(&mtdinfo, 0, sizeof(mtdinfo));
- mtdinfo.type = MTD_ABSENT;
- }
-
- DEVTYPE(dev_current) = mtdinfo.type;
-
rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE,
- DEVOFFSET (dev_current), mtdinfo.type);
+ DEVOFFSET(dev_current), DEVTYPE(dev_current));
if (rc != CUR_ENVSIZE)
return -1;
@@ -1328,10 +1297,55 @@ int fw_env_open(struct env_opts *opts)
return 0;
}
+static int check_device_config(int dev)
+{
+ struct stat st;
+ int fd, rc = 0;
+
+ fd = open(DEVNAME(dev), O_RDONLY);
+ if (fd < 0) {
+ fprintf(stderr,
+ "Cannot open %s: %s\n",
+ DEVNAME(dev), strerror(errno));
+ return -1;
+ }
+
+ rc = fstat(fd, &st);
+ if (rc < 0) {
+ fprintf(stderr, "Cannot stat the file %s\n",
+ DEVNAME(dev));
+ goto err;
+ }
+
+ if (S_ISCHR(st.st_mode)) {
+ struct mtd_info_user mtdinfo;
+ rc = ioctl(fd, MEMGETINFO, &mtdinfo);
+ if (rc < 0) {
+ fprintf(stderr, "Cannot get MTD information for %s\n",
+ DEVNAME(dev));
+ goto err;
+ }
+ if (mtdinfo.type != MTD_NORFLASH &&
+ mtdinfo.type != MTD_NANDFLASH &&
+ mtdinfo.type != MTD_DATAFLASH &&
+ mtdinfo.type != MTD_UBIVOLUME) {
+ fprintf(stderr, "Unsupported flash type %u on %s\n",
+ mtdinfo.type, DEVNAME(dev));
+ goto err;
+ }
+ DEVTYPE(dev) = mtdinfo.type;
+ } else {
+ DEVTYPE(dev) = MTD_ABSENT;
+ }
+
+err:
+ close(fd);
+ return rc;
+}
static int parse_config(struct env_opts *opts)
{
- struct stat st;
+ int rc;
if (!opts)
opts = &default_opts;
@@ -1375,25 +1389,21 @@ static int parse_config(struct env_opts *opts)
HaveRedundEnv = 1;
#endif
#endif
- if (stat (DEVNAME (0), &st)) {
- fprintf (stderr,
- "Cannot access MTD device %s: %s\n",
- DEVNAME (0), strerror (errno));
- return -1;
- }
+ rc = check_device_config(0);
+ if (rc < 0)
+ return rc;
- if (HaveRedundEnv && stat (DEVNAME (1), &st)) {
- fprintf (stderr,
- "Cannot access MTD device %s: %s\n",
- DEVNAME (1), strerror (errno));
- return -1;
- }
+ if (HaveRedundEnv) {
+ rc = check_device_config(1);
+ if (rc < 0)
+ return rc;
- if (HaveRedundEnv && ENVSIZE(0) != ENVSIZE(1)) {
- ENVSIZE(0) = ENVSIZE(1) = min(ENVSIZE(0), ENVSIZE(1));
- fprintf(stderr,
- "Redundant environments have inequal size, set to 0x%08lx\n",
- ENVSIZE(1));
+ if (ENVSIZE(0) != ENVSIZE(1)) {
+ ENVSIZE(0) = ENVSIZE(1) = min(ENVSIZE(0), ENVSIZE(1));
+ fprintf(stderr,
+ "Redundant environments have inequal size, set to 0x%08lx\n",
+ ENVSIZE(1));
+ }
}
usable_envsize = CUR_ENVSIZE - sizeof(uint32_t);
--
2.9.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] tools/env: allow negative offsets
2016-07-14 0:14 [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Stefan Agner
@ 2016-07-14 0:14 ` Stefan Agner
2016-07-17 13:37 ` Andreas Fenkart
2016-07-23 0:12 ` [U-Boot] [U-Boot,2/2] " Tom Rini
2016-07-17 13:18 ` [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Andreas Fenkart
2016-07-23 0:12 ` [U-Boot] [U-Boot, " Tom Rini
2 siblings, 2 replies; 7+ messages in thread
From: Stefan Agner @ 2016-07-14 0:14 UTC (permalink / raw)
To: u-boot
From: Stefan Agner <stefan.agner@toradex.com>
A negative value for the offset is treated as a backwards offset for
from the end of the device/partition for block devices. This aligns
the behavior of the config file with the syntax of CONFIG_ENV_OFFSET
where the functionality has been introduced with
commit 5c088ee841f9 ("env_mmc: allow negative CONFIG_ENV_OFFSET").
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---
tools/env/fw_env.c | 39 ++++++++++++++++++++++++++++++---------
tools/env/fw_env.config | 5 +++++
2 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 06d6865..be338a5b 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -14,6 +14,7 @@
#include <errno.h>
#include <env_flags.h>
#include <fcntl.h>
+#include <linux/fs.h>
#include <linux/stringify.h>
#include <ctype.h>
#include <stdio.h>
@@ -51,7 +52,7 @@ struct env_opts default_opts = {
struct envdev_s {
const char *devname; /* Device name */
- ulong devoff; /* Device offset */
+ long long devoff; /* Device offset */
ulong env_size; /* environment size */
ulong erase_size; /* device erase size */
ulong env_sectors; /* number of environment sectors */
@@ -994,7 +995,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
}
#ifdef DEBUG
- fprintf(stderr, "Writing new environment at 0x%lx on %s\n",
+ fprintf(stderr, "Writing new environment at 0x%llx on %s\n",
DEVOFFSET (dev_target), DEVNAME (dev_target));
#endif
@@ -1010,7 +1011,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
offsetof (struct env_image_redundant, flags);
#ifdef DEBUG
fprintf(stderr,
- "Setting obsolete flag in environment at 0x%lx on %s\n",
+ "Setting obsolete flag in environment at 0x%llx on %s\n",
DEVOFFSET (dev_current), DEVNAME (dev_current));
#endif
flash_flag_obsolete (dev_current, fd_current, offset);
@@ -1335,7 +1336,27 @@ static int check_device_config(int dev)
}
DEVTYPE(dev) = mtdinfo.type;
} else {
+ uint64_t size;
DEVTYPE(dev) = MTD_ABSENT;
+
+ /*
+ * Check for negative offsets, treat it as backwards offset
+ * from the end of the block device
+ */
+ if (DEVOFFSET(dev) < 0) {
+ rc = ioctl(fd, BLKGETSIZE64, &size);
+ if (rc < 0) {
+ fprintf(stderr, "Could not get block device size on %s\n",
+ DEVNAME(dev));
+ goto err;
+ }
+
+ DEVOFFSET(dev) = DEVOFFSET(dev) + size;
+#ifdef DEBUG
+ fprintf(stderr, "Calculated device offset 0x%llx on %s\n",
+ DEVOFFSET(dev), DEVNAME(dev));
+#endif
+ }
}
err:
@@ -1434,12 +1455,12 @@ static int get_config (char *fname)
if (dump[0] == '#')
continue;
- rc = sscanf (dump, "%ms %lx %lx %lx %lx",
- &devname,
- &DEVOFFSET (i),
- &ENVSIZE (i),
- &DEVESIZE (i),
- &ENVSECTORS (i));
+ rc = sscanf(dump, "%ms %lli %lx %lx %lx",
+ &devname,
+ &DEVOFFSET(i),
+ &ENVSIZE(i),
+ &DEVESIZE(i),
+ &ENVSECTORS(i));
if (rc < 3)
continue;
diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
index 6f216f9..f0f66aa 100644
--- a/tools/env/fw_env.config
+++ b/tools/env/fw_env.config
@@ -4,6 +4,7 @@
# Notice, that the "Number of sectors" is not required on NOR and SPI-dataflash.
# Futhermore, if the Flash sector size is ommitted, this value is assumed to
# be the same as the Environment size, which is valid for NOR and SPI-dataflash
+# Device offset must be prefixed with 0x to be parsed as a hexadecimal value.
# NOR example
# MTD device name Device offset Env. size Flash sector size Number of sectors
@@ -18,8 +19,12 @@
# NAND example
#/dev/mtd0 0x4000 0x4000 0x20000 2
+# On a block device a negative offset is treated as a backwards offset from the
+# end of the device/partition, rather than a forwards offset from the start.
+
# Block device example
#/dev/mmcblk0 0xc0000 0x20000
+#/dev/mmcblk0 -0x20000 0x20000
# VFAT example
#/boot/uboot.env 0x0000 0x4000
--
2.9.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] tools/env: complete environment device config early
2016-07-14 0:14 [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Stefan Agner
2016-07-14 0:14 ` [U-Boot] [PATCH 2/2] tools/env: allow negative offsets Stefan Agner
@ 2016-07-17 13:18 ` Andreas Fenkart
2016-07-23 0:12 ` [U-Boot] [U-Boot, " Tom Rini
2 siblings, 0 replies; 7+ messages in thread
From: Andreas Fenkart @ 2016-07-17 13:18 UTC (permalink / raw)
To: u-boot
2016-07-14 2:14 GMT+02:00 Stefan Agner <stefan@agner.ch>:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Currently flash_read completes a crucial part of the environment
> device configuration, the device type (mtd_type). This is rather
> confusing as flash_io calls flash_read conditionally, and one might
> think flash_write, which also makes use of mtd_type, gets called
> before flash_read. But since flash_io is always called with O_RDONLY
> first, this is not actually the case in reality.
>
> However, it is much cleaner to complete and verify the config early
> in parse_config. This also prepares the code for further extension.
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Reviewed-by: Andreas Fenkart
> ---
>
> tools/env/fw_env.c | 110 +++++++++++++++++++++++++++++------------------------
> 1 file changed, 60 insertions(+), 50 deletions(-)
>
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 692abda..06d6865 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -1021,41 +1021,10 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
>
> static int flash_read (int fd)
> {
> - struct mtd_info_user mtdinfo;
> - struct stat st;
> int rc;
>
> - rc = fstat(fd, &st);
> - if (rc < 0) {
> - fprintf(stderr, "Cannot stat the file %s\n",
> - DEVNAME(dev_current));
> - return -1;
> - }
> -
> - if (S_ISCHR(st.st_mode)) {
> - rc = ioctl(fd, MEMGETINFO, &mtdinfo);
> - if (rc < 0) {
> - fprintf(stderr, "Cannot get MTD information for %s\n",
> - DEVNAME(dev_current));
> - return -1;
> - }
> - if (mtdinfo.type != MTD_NORFLASH &&
> - mtdinfo.type != MTD_NANDFLASH &&
> - mtdinfo.type != MTD_DATAFLASH &&
> - mtdinfo.type != MTD_UBIVOLUME) {
> - fprintf (stderr, "Unsupported flash type %u on %s\n",
> - mtdinfo.type, DEVNAME(dev_current));
> - return -1;
> - }
> - } else {
> - memset(&mtdinfo, 0, sizeof(mtdinfo));
> - mtdinfo.type = MTD_ABSENT;
> - }
> -
> - DEVTYPE(dev_current) = mtdinfo.type;
> -
> rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE,
> - DEVOFFSET (dev_current), mtdinfo.type);
> + DEVOFFSET(dev_current), DEVTYPE(dev_current));
> if (rc != CUR_ENVSIZE)
> return -1;
>
> @@ -1328,10 +1297,55 @@ int fw_env_open(struct env_opts *opts)
> return 0;
> }
>
> +static int check_device_config(int dev)
> +{
> + struct stat st;
> + int fd, rc = 0;
> +
> + fd = open(DEVNAME(dev), O_RDONLY);
> + if (fd < 0) {
> + fprintf(stderr,
> + "Cannot open %s: %s\n",
> + DEVNAME(dev), strerror(errno));
> + return -1;
> + }
> +
> + rc = fstat(fd, &st);
> + if (rc < 0) {
> + fprintf(stderr, "Cannot stat the file %s\n",
> + DEVNAME(dev));
> + goto err;
> + }
> +
> + if (S_ISCHR(st.st_mode)) {
> + struct mtd_info_user mtdinfo;
> + rc = ioctl(fd, MEMGETINFO, &mtdinfo);
> + if (rc < 0) {
> + fprintf(stderr, "Cannot get MTD information for %s\n",
> + DEVNAME(dev));
> + goto err;
> + }
> + if (mtdinfo.type != MTD_NORFLASH &&
> + mtdinfo.type != MTD_NANDFLASH &&
> + mtdinfo.type != MTD_DATAFLASH &&
> + mtdinfo.type != MTD_UBIVOLUME) {
> + fprintf(stderr, "Unsupported flash type %u on %s\n",
> + mtdinfo.type, DEVNAME(dev));
> + goto err;
> + }
> + DEVTYPE(dev) = mtdinfo.type;
> + } else {
> + DEVTYPE(dev) = MTD_ABSENT;
> + }
> +
> +err:
> + close(fd);
> + return rc;
> +}
>
> static int parse_config(struct env_opts *opts)
> {
> - struct stat st;
> + int rc;
>
> if (!opts)
> opts = &default_opts;
> @@ -1375,25 +1389,21 @@ static int parse_config(struct env_opts *opts)
> HaveRedundEnv = 1;
> #endif
> #endif
> - if (stat (DEVNAME (0), &st)) {
> - fprintf (stderr,
> - "Cannot access MTD device %s: %s\n",
> - DEVNAME (0), strerror (errno));
> - return -1;
> - }
> + rc = check_device_config(0);
> + if (rc < 0)
> + return rc;
>
> - if (HaveRedundEnv && stat (DEVNAME (1), &st)) {
> - fprintf (stderr,
> - "Cannot access MTD device %s: %s\n",
> - DEVNAME (1), strerror (errno));
> - return -1;
> - }
> + if (HaveRedundEnv) {
> + rc = check_device_config(1);
> + if (rc < 0)
> + return rc;
>
> - if (HaveRedundEnv && ENVSIZE(0) != ENVSIZE(1)) {
> - ENVSIZE(0) = ENVSIZE(1) = min(ENVSIZE(0), ENVSIZE(1));
> - fprintf(stderr,
> - "Redundant environments have inequal size, set to 0x%08lx\n",
> - ENVSIZE(1));
> + if (ENVSIZE(0) != ENVSIZE(1)) {
> + ENVSIZE(0) = ENVSIZE(1) = min(ENVSIZE(0), ENVSIZE(1));
> + fprintf(stderr,
> + "Redundant environments have inequal size, set to 0x%08lx\n",
> + ENVSIZE(1));
> + }
> }
>
> usable_envsize = CUR_ENVSIZE - sizeof(uint32_t);
> --
> 2.9.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] tools/env: allow negative offsets
2016-07-14 0:14 ` [U-Boot] [PATCH 2/2] tools/env: allow negative offsets Stefan Agner
@ 2016-07-17 13:37 ` Andreas Fenkart
2016-07-17 17:12 ` Stefan Agner
2016-07-23 0:12 ` [U-Boot] [U-Boot,2/2] " Tom Rini
1 sibling, 1 reply; 7+ messages in thread
From: Andreas Fenkart @ 2016-07-17 13:37 UTC (permalink / raw)
To: u-boot
Hi,
2016-07-14 2:14 GMT+02:00 Stefan Agner <stefan@agner.ch>:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> A negative value for the offset is treated as a backwards offset for
> from the end of the device/partition for block devices. This aligns
> the behavior of the config file with the syntax of CONFIG_ENV_OFFSET
> where the functionality has been introduced with
> commit 5c088ee841f9 ("env_mmc: allow negative CONFIG_ENV_OFFSET").
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
lgtm
> ---
>
> tools/env/fw_env.c | 39 ++++++++++++++++++++++++++++++---------
> tools/env/fw_env.config | 5 +++++
> 2 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 06d6865..be338a5b 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -14,6 +14,7 @@
> #include <errno.h>
> #include <env_flags.h>
> #include <fcntl.h>
> +#include <linux/fs.h>
> #include <linux/stringify.h>
> #include <ctype.h>
> #include <stdio.h>
> @@ -51,7 +52,7 @@ struct env_opts default_opts = {
>
> struct envdev_s {
> const char *devname; /* Device name */
> - ulong devoff; /* Device offset */
> + long long devoff; /* Device offset */
> ulong env_size; /* environment size */
> ulong erase_size; /* device erase size */
> ulong env_sectors; /* number of environment sectors */
> @@ -994,7 +995,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
> }
>
> #ifdef DEBUG
> - fprintf(stderr, "Writing new environment at 0x%lx on %s\n",
> + fprintf(stderr, "Writing new environment at 0x%llx on %s\n",
> DEVOFFSET (dev_target), DEVNAME (dev_target));
> #endif
>
> @@ -1010,7 +1011,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
> offsetof (struct env_image_redundant, flags);
> #ifdef DEBUG
> fprintf(stderr,
> - "Setting obsolete flag in environment at 0x%lx on %s\n",
> + "Setting obsolete flag in environment at 0x%llx on %s\n",
> DEVOFFSET (dev_current), DEVNAME (dev_current));
> #endif
> flash_flag_obsolete (dev_current, fd_current, offset);
> @@ -1335,7 +1336,27 @@ static int check_device_config(int dev)
> }
> DEVTYPE(dev) = mtdinfo.type;
> } else {
> + uint64_t size;
> DEVTYPE(dev) = MTD_ABSENT;
> +
> + /*
> + * Check for negative offsets, treat it as backwards offset
> + * from the end of the block device
> + */
> + if (DEVOFFSET(dev) < 0) {
> + rc = ioctl(fd, BLKGETSIZE64, &size);
> + if (rc < 0) {
> + fprintf(stderr, "Could not get block device size on %s\n",
> + DEVNAME(dev));
> + goto err;
> + }
> +
> + DEVOFFSET(dev) = DEVOFFSET(dev) + size;
I wonder if we need a check, that the redundant areas don't overlap.
But we can add that later
> +#ifdef DEBUG
> + fprintf(stderr, "Calculated device offset 0x%llx on %s\n",
> + DEVOFFSET(dev), DEVNAME(dev));
> +#endif
> + }
> }
>
> err:
> @@ -1434,12 +1455,12 @@ static int get_config (char *fname)
> if (dump[0] == '#')
> continue;
>
> - rc = sscanf (dump, "%ms %lx %lx %lx %lx",
> - &devname,
> - &DEVOFFSET (i),
> - &ENVSIZE (i),
> - &DEVESIZE (i),
> - &ENVSECTORS (i));
> + rc = sscanf(dump, "%ms %lli %lx %lx %lx",
> + &devname,
> + &DEVOFFSET(i),
> + &ENVSIZE(i),
> + &DEVESIZE(i),
> + &ENVSECTORS(i));
>
> if (rc < 3)
> continue;
> diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
> index 6f216f9..f0f66aa 100644
> --- a/tools/env/fw_env.config
> +++ b/tools/env/fw_env.config
> @@ -4,6 +4,7 @@
> # Notice, that the "Number of sectors" is not required on NOR and SPI-dataflash.
> # Futhermore, if the Flash sector size is ommitted, this value is assumed to
> # be the same as the Environment size, which is valid for NOR and SPI-dataflash
> +# Device offset must be prefixed with 0x to be parsed as a hexadecimal value.
>
> # NOR example
> # MTD device name Device offset Env. size Flash sector size Number of sectors
> @@ -18,8 +19,12 @@
> # NAND example
> #/dev/mtd0 0x4000 0x4000 0x20000 2
>
> +# On a block device a negative offset is treated as a backwards offset from the
> +# end of the device/partition, rather than a forwards offset from the start.
> +
> # Block device example
> #/dev/mmcblk0 0xc0000 0x20000
> +#/dev/mmcblk0 -0x20000 0x20000
>
> # VFAT example
> #/boot/uboot.env 0x0000 0x4000
> --
> 2.9.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] tools/env: allow negative offsets
2016-07-17 13:37 ` Andreas Fenkart
@ 2016-07-17 17:12 ` Stefan Agner
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Agner @ 2016-07-17 17:12 UTC (permalink / raw)
To: u-boot
On 2016-07-17 06:37, Andreas Fenkart wrote:
> Hi,
>
> 2016-07-14 2:14 GMT+02:00 Stefan Agner <stefan@agner.ch>:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> A negative value for the offset is treated as a backwards offset for
>> from the end of the device/partition for block devices. This aligns
>> the behavior of the config file with the syntax of CONFIG_ENV_OFFSET
>> where the functionality has been introduced with
>> commit 5c088ee841f9 ("env_mmc: allow negative CONFIG_ENV_OFFSET").
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> lgtm
>
>> ---
>>
>> tools/env/fw_env.c | 39 ++++++++++++++++++++++++++++++---------
>> tools/env/fw_env.config | 5 +++++
>> 2 files changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>> index 06d6865..be338a5b 100644
>> --- a/tools/env/fw_env.c
>> +++ b/tools/env/fw_env.c
>> @@ -14,6 +14,7 @@
>> #include <errno.h>
>> #include <env_flags.h>
>> #include <fcntl.h>
>> +#include <linux/fs.h>
>> #include <linux/stringify.h>
>> #include <ctype.h>
>> #include <stdio.h>
>> @@ -51,7 +52,7 @@ struct env_opts default_opts = {
>>
>> struct envdev_s {
>> const char *devname; /* Device name */
>> - ulong devoff; /* Device offset */
>> + long long devoff; /* Device offset */
>> ulong env_size; /* environment size */
>> ulong erase_size; /* device erase size */
>> ulong env_sectors; /* number of environment sectors */
>> @@ -994,7 +995,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
>> }
>>
>> #ifdef DEBUG
>> - fprintf(stderr, "Writing new environment at 0x%lx on %s\n",
>> + fprintf(stderr, "Writing new environment at 0x%llx on %s\n",
>> DEVOFFSET (dev_target), DEVNAME (dev_target));
>> #endif
>>
>> @@ -1010,7 +1011,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
>> offsetof (struct env_image_redundant, flags);
>> #ifdef DEBUG
>> fprintf(stderr,
>> - "Setting obsolete flag in environment at 0x%lx on %s\n",
>> + "Setting obsolete flag in environment at 0x%llx on %s\n",
>> DEVOFFSET (dev_current), DEVNAME (dev_current));
>> #endif
>> flash_flag_obsolete (dev_current, fd_current, offset);
>> @@ -1335,7 +1336,27 @@ static int check_device_config(int dev)
>> }
>> DEVTYPE(dev) = mtdinfo.type;
>> } else {
>> + uint64_t size;
>> DEVTYPE(dev) = MTD_ABSENT;
>> +
>> + /*
>> + * Check for negative offsets, treat it as backwards offset
>> + * from the end of the block device
>> + */
>> + if (DEVOFFSET(dev) < 0) {
>> + rc = ioctl(fd, BLKGETSIZE64, &size);
>> + if (rc < 0) {
>> + fprintf(stderr, "Could not get block device size on %s\n",
>> + DEVNAME(dev));
>> + goto err;
>> + }
>> +
>> + DEVOFFSET(dev) = DEVOFFSET(dev) + size;
>
> I wonder if we need a check, that the redundant areas don't overlap.
> But we can add that later
We haven't had it before, but yeah I agree it would be a sensible thing
to do. And we have now a good place to add such checks...
--
Stefan
>
>
>> +#ifdef DEBUG
>> + fprintf(stderr, "Calculated device offset 0x%llx on %s\n",
>> + DEVOFFSET(dev), DEVNAME(dev));
>> +#endif
>> + }
>> }
>>
>> err:
>> @@ -1434,12 +1455,12 @@ static int get_config (char *fname)
>> if (dump[0] == '#')
>> continue;
>>
>> - rc = sscanf (dump, "%ms %lx %lx %lx %lx",
>> - &devname,
>> - &DEVOFFSET (i),
>> - &ENVSIZE (i),
>> - &DEVESIZE (i),
>> - &ENVSECTORS (i));
>> + rc = sscanf(dump, "%ms %lli %lx %lx %lx",
>> + &devname,
>> + &DEVOFFSET(i),
>> + &ENVSIZE(i),
>> + &DEVESIZE(i),
>> + &ENVSECTORS(i));
>>
>> if (rc < 3)
>> continue;
>> diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
>> index 6f216f9..f0f66aa 100644
>> --- a/tools/env/fw_env.config
>> +++ b/tools/env/fw_env.config
>> @@ -4,6 +4,7 @@
>> # Notice, that the "Number of sectors" is not required on NOR and SPI-dataflash.
>> # Futhermore, if the Flash sector size is ommitted, this value is assumed to
>> # be the same as the Environment size, which is valid for NOR and SPI-dataflash
>> +# Device offset must be prefixed with 0x to be parsed as a hexadecimal value.
>>
>> # NOR example
>> # MTD device name Device offset Env. size Flash sector size Number of sectors
>> @@ -18,8 +19,12 @@
>> # NAND example
>> #/dev/mtd0 0x4000 0x4000 0x20000 2
>>
>> +# On a block device a negative offset is treated as a backwards offset from the
>> +# end of the device/partition, rather than a forwards offset from the start.
>> +
>> # Block device example
>> #/dev/mmcblk0 0xc0000 0x20000
>> +#/dev/mmcblk0 -0x20000 0x20000
>>
>> # VFAT example
>> #/boot/uboot.env 0x0000 0x4000
>> --
>> 2.9.0
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [U-Boot, 1/2] tools/env: complete environment device config early
2016-07-14 0:14 [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Stefan Agner
2016-07-14 0:14 ` [U-Boot] [PATCH 2/2] tools/env: allow negative offsets Stefan Agner
2016-07-17 13:18 ` [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Andreas Fenkart
@ 2016-07-23 0:12 ` Tom Rini
2 siblings, 0 replies; 7+ messages in thread
From: Tom Rini @ 2016-07-23 0:12 UTC (permalink / raw)
To: u-boot
On Wed, Jul 13, 2016 at 05:14:37PM -0700, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Currently flash_read completes a crucial part of the environment
> device configuration, the device type (mtd_type). This is rather
> confusing as flash_io calls flash_read conditionally, and one might
> think flash_write, which also makes use of mtd_type, gets called
> before flash_read. But since flash_io is always called with O_RDONLY
> first, this is not actually the case in reality.
>
> However, it is much cleaner to complete and verify the config early
> in parse_config. This also prepares the code for further extension.
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> Reviewed-by: Andreas Fenkart
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/8f0abc0b/attachment.sig>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [U-Boot,2/2] tools/env: allow negative offsets
2016-07-14 0:14 ` [U-Boot] [PATCH 2/2] tools/env: allow negative offsets Stefan Agner
2016-07-17 13:37 ` Andreas Fenkart
@ 2016-07-23 0:12 ` Tom Rini
1 sibling, 0 replies; 7+ messages in thread
From: Tom Rini @ 2016-07-23 0:12 UTC (permalink / raw)
To: u-boot
On Wed, Jul 13, 2016 at 05:14:38PM -0700, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> A negative value for the offset is treated as a backwards offset for
> from the end of the device/partition for block devices. This aligns
> the behavior of the config file with the syntax of CONFIG_ENV_OFFSET
> where the functionality has been introduced with
> commit 5c088ee841f9 ("env_mmc: allow negative CONFIG_ENV_OFFSET").
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/fd1361fe/attachment.sig>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-23 0:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-14 0:14 [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Stefan Agner
2016-07-14 0:14 ` [U-Boot] [PATCH 2/2] tools/env: allow negative offsets Stefan Agner
2016-07-17 13:37 ` Andreas Fenkart
2016-07-17 17:12 ` Stefan Agner
2016-07-23 0:12 ` [U-Boot] [U-Boot,2/2] " Tom Rini
2016-07-17 13:18 ` [U-Boot] [PATCH 1/2] tools/env: complete environment device config early Andreas Fenkart
2016-07-23 0:12 ` [U-Boot] [U-Boot, " Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.