Patchwork [U-Boot,04/12] tools/env: Reduce the impact on real-time processes

login
register
mail settings
Submitter Joe Hershberger
Date Aug. 17, 2012, 8:49 p.m.
Message ID <1345236586-19076-5-git-send-email-joe.hershberger@ni.com>
Download mbox | patch
Permalink /patch/178381/
State Rejected
Delegated to: Tom Rini
Headers show

Comments

Joe Hershberger - Aug. 17, 2012, 8:49 p.m.
Modify fw_printenv to read in chunks of 0x20 at a time.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---
 tools/env/fw_env.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)
Mike Frysinger - Aug. 23, 2012, 3:30 a.m.
On Friday 17 August 2012 16:49:38 Joe Hershberger wrote:
> +		* Break reads up into very small chunks so fw_printenv doesn't
> +		* block the kernel long enough to starve other kernel tasks.

err, this sounds like a bug in your kernel driver.  why should userspace be 
working around buggy drivers ?
-mike
Joe Hershberger - Aug. 23, 2012, 4:26 p.m.
Hi Mike,

On Wed, Aug 22, 2012 at 10:30 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Friday 17 August 2012 16:49:38 Joe Hershberger wrote:
>> +             * Break reads up into very small chunks so fw_printenv doesn't
>> +             * block the kernel long enough to starve other kernel tasks.
>
> err, this sounds like a bug in your kernel driver.  why should userspace be
> working around buggy drivers ?

The problem is something to do with the mtd driver blocking some
resource on large reads that starves USB transfers.  We never fully
investigated it.  It's true that this is a work-around.  Is that so
bad?  Maybe so.

-Joe
Mike Frysinger - Aug. 23, 2012, 8:31 p.m.
On Thursday 23 August 2012 12:26:08 Joe Hershberger wrote:
> On Wed, Aug 22, 2012 at 10:30 PM, Mike Frysinger wrote:
> > On Friday 17 August 2012 16:49:38 Joe Hershberger wrote:
> >> +	* Break reads up into very small chunks so fw_printenv doesn't
> >> +	* block the kernel long enough to starve other kernel tasks.
> > 
> > err, this sounds like a bug in your kernel driver.  why should userspace
> > be working around buggy drivers ?
> 
> The problem is something to do with the mtd driver blocking some
> resource on large reads that starves USB transfers.  We never fully
> investigated it.  It's true that this is a work-around.  Is that so
> bad?  Maybe so.

i think it's a bad precedent that we shouldn't encourage.  you add your wonky 
hardware workaround, and then the next guy will want to add his hack for their 
broken driver, and we're left with an ugly unmaintainable pile.
-mike

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 8bb7f9a..9ecc22a 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -45,6 +45,8 @@ 
 
 #include "fw_env.h"
 
+#define MAX_BYTES_PER_READ 0x20
+
 #define WHITESPACE(c) ((c == '\t') || (c == ' '))
 
 #define min(x, y) ({				\
@@ -671,6 +673,7 @@  static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 	size_t blocklen;	/* erase / write length - one block on NAND,
 				   0 on NOR */
 	size_t processed = 0;	/* progress counter */
+	size_t bytesread = 0;	/* bytes read so far */
 	size_t readlen = count;	/* current read length */
 	off_t top_of_range;	/* end of the last block we may use */
 	off_t block_seek;	/* offset inside the current block to the start
@@ -730,11 +733,23 @@  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 != readlen) {
-			fprintf (stderr, "Read error on %s: %s\n",
-				 DEVNAME (dev), strerror (errno));
-			return -1;
+		/*
+		* Break reads up into very small chunks so fw_printenv doesn't
+		* block the kernel long enough to starve other kernel tasks.
+		*/
+		bytesread = 0;
+		while (bytesread < readlen) {
+			size_t bytestoread = readlen - bytesread;
+
+			if (bytestoread > MAX_BYTES_PER_READ)
+				bytestoread = MAX_BYTES_PER_READ;
+			rc = read(fd, buf + processed + bytesread, bytestoread);
+			if (rc != bytestoread) {
+				fprintf(stderr, "Read error on %s: %s\n",
+					 DEVNAME(dev), strerror(errno));
+				return -1;
+			}
+			bytesread += bytestoread;
 		}
 #ifdef DEBUG
 		fprintf (stderr, "Read 0x%x bytes at 0x%llx\n",