[U-Boot] [RFC 0/3] Enhance env tools
There are circustances where it is desirable to run the environment tools on your build machine in order to create an environment image that can be written to the target machine at a later time.
This patch series introduces a number of enhancements to the tools that make this possible.
The first patch allows allows one to override the default location of the tool config file by setting a FW_CONFIG_FILE environment variable.
The second patch allows the environment to be written to a regular file.
The third patch increases the devname length to 4096 in order to support writing to normal files in addition to mtd devices.
Loïc Minier (3): tools/env: Default to the config specified in FW_CONFIG_FILE tools/env: Support writing to files tools/env: Bump devname length to PATH_MAX for filenames
tools/env/fw_env.c | 107 ++++++++++++++++++++++++++++++++++++---------------- 1 files changed, 74 insertions(+), 33 deletions(-)
From: Loïc Minier loic.minier@linaro.org
This patch allows one to override the default location of the config file by setting FW_CONFIG_FILE environment variable.
Signed-off-by: Loïc Minier loic.minier@linaro.org Tested-by: Steve Sakoman steve@sakoman.com --- tools/env/fw_env.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 8ff7052..75f6a98 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1224,8 +1224,15 @@ static int parse_config () struct stat st;
#if defined(CONFIG_FILE) + /* Default to the config file specified in FW_CONFIG_FILE */ + char *config_file = getenv("FW_CONFIG_FILE"); + if (!config_file || !strlen(config_file)) { + /* If unset or empty use the default config file */ + config_file = CONFIG_FILE; + } + /* Fills in DEVNAME(), ENVSIZE(), DEVESIZE(). Or don't. */ - if (get_config (CONFIG_FILE)) { + if (get_config (config_file)) { fprintf (stderr, "Cannot parse config file: %s\n", strerror (errno)); return -1;
On Friday, December 03, 2010 23:28:51 Steve Sakoman wrote:
- if (!config_file || !strlen(config_file)) {
strlen is a bad bad idea. if you only want to see if it's non-empty, check the first byte. -mike
On Sat, 2010-12-04 at 05:34 -0500, Mike Frysinger wrote:
On Friday, December 03, 2010 23:28:51 Steve Sakoman wrote:
- if (!config_file || !strlen(config_file)) {
strlen is a bad bad idea. if you only want to see if it's non-empty, check the first byte.
I'll correct this in the next revision.
Thanks for reviewing the code!
Steve
On 12/04/2010 05:28 AM, Steve Sakoman wrote:
From: Loïc Minier loic.minier@linaro.org
Hi,
#if defined(CONFIG_FILE)
- /* Default to the config file specified in FW_CONFIG_FILE */
- char *config_file = getenv("FW_CONFIG_FILE");
What about to pass the configuration file on the command line instead of an environment variable ? Something like "fw_env -c /etc/fw_config", for example ? IMHO it is easier to understand and we can add documentation in the "help" message.
Best regards, Stefano Babic
On Sat, 2010-12-04 at 11:59 +0100, Stefano Babic wrote:
On 12/04/2010 05:28 AM, Steve Sakoman wrote:
From: Loïc Minier loic.minier@linaro.org
Hi,
#if defined(CONFIG_FILE)
- /* Default to the config file specified in FW_CONFIG_FILE */
- char *config_file = getenv("FW_CONFIG_FILE");
What about to pass the configuration file on the command line instead of an environment variable ? Something like "fw_env -c /etc/fw_config", for example ? IMHO it is easier to understand and we can add documentation in the "help" message.
Good suggestion, I prefer that too.
Steve
From: Loïc Minier loic.minier@linaro.org
Track st_mode and use it to skip ioctls on file-backed fds. This allows writing to regular files transparently.
Signed-off-by: Loïc Minier loic.minier@linaro.org Tested-by: Steve Sakoman steve.sakoman@linaro.org --- tools/env/fw_env.c | 92 ++++++++++++++++++++++++++++++++++----------------- 1 files changed, 61 insertions(+), 31 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 75f6a98..d2f9748 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -60,6 +60,7 @@ struct envdev_s { ulong erase_size; /* device erase size */ ulong env_sectors; /* number of environment sectors */ uint8_t mtd_type; /* type of the MTD device */ + mode_t st_mode; /* protection / file type */ };
static struct envdev_s envdevices[2] = @@ -78,6 +79,7 @@ static int dev_current; #define DEVESIZE(i) envdevices[(i)].erase_size #define ENVSECTORS(i) envdevices[(i)].env_sectors #define DEVTYPE(i) envdevices[(i)].mtd_type +#define STMODE(i) envdevices[(i)].st_mode
#define CONFIG_ENV_SIZE ENVSIZE(dev_current)
@@ -633,8 +635,12 @@ int fw_parse_script(char *fname) * > 0 - block is bad * < 0 - failed to test */ -static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart) +static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart, + int is_mtd) { + if (!is_mtd) + return 0; + if (mtd_type == MTD_NANDFLASH) { int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
@@ -661,7 +667,7 @@ static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart) * the DEVOFFSET (dev) block. On NOR the loop is only run once. */ static int flash_read_buf (int dev, int fd, void *buf, size_t count, - off_t offset, uint8_t mtd_type) + off_t offset, uint8_t mtd_type, int is_mtd) { size_t blocklen; /* erase / write length - one block on NAND, 0 on NOR */ @@ -683,7 +689,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count, /* Offset inside a block */ block_seek = offset - blockstart;
- if (mtd_type == MTD_NANDFLASH) { + if (!is_mtd || mtd_type == MTD_NANDFLASH) { /* * NAND: calculate which blocks we are reading. We have * to read one block at a time to skip bad blocks. @@ -707,7 +713,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
/* This only runs once on NOR flash */ while (processed < count) { - rc = flash_bad_block (fd, mtd_type, &blockstart); + rc = flash_bad_block (fd, mtd_type, &blockstart, is_mtd); if (rc < 0) /* block test failed */ return -1;
@@ -754,7 +760,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count, * the whole data at once. */ static int flash_write_buf (int dev, int fd, void *buf, size_t count, - off_t offset, uint8_t mtd_type) + off_t offset, uint8_t mtd_type, int is_mtd) { void *data; struct erase_info_user erase; @@ -812,7 +818,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, }
rc = flash_read_buf (dev, fd, data, write_total, erase_offset, - mtd_type); + mtd_type, is_mtd); if (write_total != rc) return -1;
@@ -826,7 +832,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, data = buf; }
- if (mtd_type == MTD_NANDFLASH) { + if (!is_mtd || mtd_type == MTD_NANDFLASH) { /* * NAND: calculate which blocks we are writing. We have * to write one block at a time to skip bad blocks. @@ -840,7 +846,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
/* This only runs once on NOR flash */ while (processed < write_total) { - rc = flash_bad_block (fd, mtd_type, &blockstart); + rc = flash_bad_block (fd, mtd_type, &blockstart, is_mtd); if (rc < 0) /* block test failed */ return rc;
@@ -854,14 +860,16 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, continue; }
- erase.start = blockstart; - ioctl (fd, MEMUNLOCK, &erase); + if (is_mtd) { + erase.start = blockstart; + ioctl (fd, MEMUNLOCK, &erase);
- if (ioctl (fd, MEMERASE, &erase) != 0) { - fprintf (stderr, "MTD erase error on %s: %s\n", - DEVNAME (dev), - strerror (errno)); - return -1; + if (ioctl (fd, MEMERASE, &erase) != 0) { + fprintf (stderr, "MTD erase error on %s: %s\n", + DEVNAME (dev), + strerror (errno)); + return -1; + } }
if (lseek (fd, blockstart, SEEK_SET) == -1) { @@ -880,7 +888,9 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, return -1; }
- ioctl (fd, MEMLOCK, &erase); + if (is_mtd) { + ioctl (fd, MEMLOCK, &erase); + }
processed += blocklen; block_seek = 0; @@ -919,7 +929,8 @@ static int flash_flag_obsolete (int dev, int fd, off_t offset) return rc; }
-static int flash_write (int fd_current, int fd_target, int dev_target) +static int flash_write (int fd_current, int fd_target, int dev_target, + int is_mtd) { int rc;
@@ -944,7 +955,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target) #endif rc = flash_write_buf (dev_target, fd_target, environment.image, CONFIG_ENV_SIZE, DEVOFFSET (dev_target), - DEVTYPE(dev_target)); + DEVTYPE(dev_target), is_mtd); if (rc < 0) return rc;
@@ -962,26 +973,32 @@ static int flash_write (int fd_current, int fd_target, int dev_target) return 0; }
-static int flash_read (int fd) +static int flash_read (int fd, int is_mtd) { struct mtd_info_user mtdinfo; int rc;
- rc = ioctl (fd, MEMGETINFO, &mtdinfo); - if (rc < 0) { - perror ("Cannot get MTD information"); - return -1; - } + if (is_mtd) { + rc = ioctl (fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + perror ("Cannot get MTD information"); + return -1; + }
- if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH) { - fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); - return -1; + if (mtdinfo.type != MTD_NORFLASH && + mtdinfo.type != MTD_NANDFLASH) { + fprintf (stderr, + "Unsupported flash type %u\n", + mtdinfo.type); + return -1; }
- DEVTYPE(dev_current) = mtdinfo.type; + DEVTYPE(dev_current) = mtdinfo.type; + }
rc = flash_read_buf (dev_current, fd, environment.image, CONFIG_ENV_SIZE, - DEVOFFSET (dev_current), mtdinfo.type); + DEVOFFSET (dev_current), DEVTYPE(dev_current), + is_mtd);
return (rc != CONFIG_ENV_SIZE) ? -1 : 0; } @@ -989,6 +1006,7 @@ static int flash_read (int fd) static int flash_io (int mode) { int fd_current, fd_target, rc, dev_target; + int is_mtd;
/* dev_current: fd_current, erase_current */ fd_current = open (DEVNAME (dev_current), mode); @@ -999,6 +1017,16 @@ static int flash_io (int mode) return -1; }
+ /* Check whether fd is a MTD device, otherwise assume regular file */ + if (S_ISREG (STMODE (dev_current))) + is_mtd = 0; + else if (S_ISCHR (STMODE (dev_current))) + is_mtd = 1; + else + fprintf (stderr, + "%s has unknown file type: %u\n", + DEVNAME (dev_current), STMODE (dev_current)); + if (mode == O_RDWR) { if (HaveRedundEnv) { /* switch to next partition for writing */ @@ -1018,7 +1046,7 @@ static int flash_io (int mode) fd_target = fd_current; }
- rc = flash_write (fd_current, fd_target, dev_target); + rc = flash_write (fd_current, fd_target, dev_target, is_mtd);
if (HaveRedundEnv) { if (close (fd_target)) { @@ -1030,7 +1058,7 @@ static int flash_io (int mode) } } } else { - rc = flash_read (fd_current); + rc = flash_read (fd_current, is_mtd); }
exit: @@ -1258,6 +1286,7 @@ static int parse_config () DEVNAME (0), strerror (errno)); return -1; } + STMODE(0) = st.st_mode;
if (HaveRedundEnv && stat (DEVNAME (1), &st)) { fprintf (stderr, @@ -1265,6 +1294,7 @@ static int parse_config () DEVNAME (1), strerror (errno)); return -1; } + STMODE(1) = st.st_mode; return 0; }
On Sat, 2010-12-04 at 05:37 -0500, Mike Frysinger wrote:
On Friday, December 03, 2010 23:28:52 Steve Sakoman wrote:
ioctl (fd, MEMLOCK, &erase);
if (is_mtd) {ioctl (fd, MEMLOCK, &erase);}useless braces
Good catch, I will correct in the next revision.
Steve
On 12/04/2010 05:28 AM, Steve Sakoman wrote:
-static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart) +static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart,
int is_mtd)
You add an additional parameter to the flash function to check if it is a real mtd, but we have already a structure to store which type of flash we have. We could use envdevices[].mtd_type to store that we want to write into a file and not into a mtd device. There is already a MTD_ABSENT constant that we could reuse.
{
- if (!is_mtd)
return 0;
Because this function does nothing with the parameter, it should be better to check its value in the caller without calling this function.
/* This only runs once on NOR flash */ while (processed < count) {
rc = flash_bad_block (fd, mtd_type, &blockstart);
rc = flash_bad_block (fd, mtd_type, &blockstart, is_mtd);
Ditto.
rc = flash_read_buf (dev, fd, data, write_total, erase_offset,
mtd_type);
mtd_type, is_mtd);
See my first comment. mtd_type can already tell us if we want to write into a file or into a real mtd device, without adding an additional parameter.
I have an additional question: if we want to add support to prepare the environment on the host and to transfer later on the target, should we not to care about the endianess ? I think the crc is written without checking the endianess of the target. Does it work for powerpc (usually big endian) when we run on a host PC ?
Best regards, Stefano Babic
On Sat, 2010-12-04 at 12:23 +0100, Stefano Babic wrote:
On 12/04/2010 05:28 AM, Steve Sakoman wrote:
-static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart) +static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart,
int is_mtd)You add an additional parameter to the flash function to check if it is a real mtd, but we have already a structure to store which type of flash we have. We could use envdevices[].mtd_type to store that we want to write into a file and not into a mtd device. There is already a MTD_ABSENT constant that we could reuse.
{
- if (!is_mtd)
return 0;Because this function does nothing with the parameter, it should be better to check its value in the caller without calling this function.
/* This only runs once on NOR flash */ while (processed < count) {
rc = flash_bad_block (fd, mtd_type, &blockstart);
rc = flash_bad_block (fd, mtd_type, &blockstart, is_mtd);Ditto.
rc = flash_read_buf (dev, fd, data, write_total, erase_offset,
mtd_type);
mtd_type, is_mtd);See my first comment. mtd_type can already tell us if we want to write into a file or into a real mtd device, without adding an additional parameter.
I have an additional question: if we want to add support to prepare the environment on the host and to transfer later on the target, should we not to care about the endianess ? I think the crc is written without checking the endianess of the target. Does it work for powerpc (usually big endian) when we run on a host PC ?
Good points! I'll work with Loïc to revise the code and get a new version submitted.
I don't have a big endian target for testing, but perhaps Loïc has access to one. Otherwise we'll come back to the list with a request for testing help.
Steve
On Sat, Dec 04, 2010, Steve Sakoman wrote:
I don't have a big endian target for testing, but perhaps Loïc has access to one. Otherwise we'll come back to the list with a request for testing help.
(I don't have access to one either)
From: Loïc Minier loic.minier@linaro.org
This patch increases the devname length to 4096 in order to support writing to normal files in addition to mtd devices.
Signed-off-by: Loïc Minier loic.minier@linaro.org Tested-by: Steve Sakoman steve.sakoman@linaro.org --- tools/env/fw_env.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index d2f9748..a75b73b 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -53,8 +53,12 @@ (void) (&_min1 == &_min2); \ _min1 < _min2 ? _min1 : _min2; })
+#ifndef PATH_MAX +#define PATH_MAX 4096 +#endif + struct envdev_s { - char devname[16]; /* Device name */ + char devname[PATH_MAX]; /* Device name */ ulong devoff; /* Device offset */ ulong env_size; /* environment size */ ulong erase_size; /* device erase size */
Dear Steve Sakoman,
In message 1291436933-26861-1-git-send-email-steve@sakoman.com you wrote:
There are circustances where it is desirable to run the environment tools on your build machine in order to create an environment image that can be written to the target machine at a later time.
I wonder what those circumstances might be.
I mean, I know what you have in mind, and the approach may have made sense with the old environment code.
But can you please explain why any of this is needed with the new code, which can easily import and export environment settings in several formats?
Best regards,
Wolfgang Denk
participants (5)
-
Loïc Minier -
Mike Frysinger -
Stefano Babic -
Steve Sakoman -
Wolfgang Denk