| Message ID | 1345236586-19076-5-git-send-email-joe.hershberger@ni.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
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
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
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
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",
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(-)