[U-Boot] [RFC 0/3] Enhance env tools

[U-Boot] [RFC 0/3] Enhance env tools

[Steve Sakoman]

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(-)

[U-Boot] [RFC 1/3] tools/env: Default to the config specified in FW_CONFIG_FILE

[Steve Sakoman]

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;
-- 
1.7.0.4

[U-Boot] [RFC 1/3] tools/env: Default to the config specified in FW_CONFIG_FILE

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101204/a2a72569/attachment.pgp 

[U-Boot] [RFC 1/3] tools/env: Default to the config specified in FW_CONFIG_FILE

[Steve Sakoman]

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

[U-Boot] [RFC 1/3] tools/env: Default to the config specified in FW_CONFIG_FILE

[Stefano Babic]

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

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [RFC 1/3] tools/env: Default to the config specified in FW_CONFIG_FILE

[Steve Sakoman]

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

[U-Boot] [RFC 2/3] tools/env: Support writing to files

[Steve Sakoman]

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@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;
 }
 
-- 
1.7.0.4

[U-Boot] [RFC 2/3] tools/env: Support writing to files

[Mike Frysinger]

On Friday, December 03, 2010 23:28:52 Steve Sakoman wrote:
> -		ioctl (fd, MEMLOCK, &erase);
> +		if (is_mtd) {
> +			ioctl (fd, MEMLOCK, &erase);
> +		}

useless braces
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101204/83025433/attachment.pgp 

[U-Boot] [RFC 2/3] tools/env: Support writing to files

[Steve Sakoman]

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

[U-Boot] [RFC 2/3] tools/env: Support writing to files

[Stefano Babic]

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

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [RFC 2/3] tools/env: Support writing to files

[Steve Sakoman]

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

[U-Boot] [RFC 2/3] tools/env: Support writing to files

[Loïc Minier]

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)

-- 
Lo?c Minier

[U-Boot] [RFC 3/3] tools/env: Bump devname length to PATH_MAX for filenames

[Steve Sakoman]

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 */
-- 
1.7.0.4

[U-Boot] [RFC 0/3] Enhance env tools

[Wolfgang Denk]

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

-- 
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: wd at denx.de
In the realm of scientific observation, luck is granted only to those
who are prepared.                                     - Louis Pasteur