i have rebased the patch (hopefully correctly, please re-check before commit). review below.
> From 06e81f785a7bfa31a5b42c2c61a78a95ec6468b0 Mon Sep 17 00:00:00 2001 > From: Stefan Tauner <[email protected]> > Date: Thu, 11 Aug 2011 15:40:33 +0200 > Subject: [PATCH] Bus Pirate buffer management revamp > > Use separate functions for managing the Bus Pirate command/data buffer. > Open-coding the buffer management was the first step, now the goal is to > make it readable. > This is the buffer management part of > "Re: [flashrom] [PATCH] Make Bus Pirate init more robust, speed up flashing" > > The buffer management of the Bus Pirate driver has been revamped > to use grow-only buffers with a reasonable initial default size > so realloc() will not have to be called in normal operation. > A side effect is the ability to switch to a static buffer without > major hassle. > Handle OOM gracefully. > > Signed-off-by: Carl-Daniel Hailfinger <[email protected]> > --- > buspirate_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++---------- > flash.h | 1 + > 2 files changed, 69 insertions(+), 16 deletions(-) > > diff --git a/buspirate_spi.c b/buspirate_spi.c > index 467d271..0e1e5cc 100644 > --- a/buspirate_spi.c > +++ b/buspirate_spi.c > @@ -50,6 +50,55 @@ static int buspirate_serialport_setup(char *dev) > #define sp_flush_incoming(...) 0 > #endif > > +static unsigned char *bp_commbuf = NULL; > +static int bp_commbufsize = 0; > + > +static int buspirate_commbuf_resize(int bufsize) this does not "resize" but grow the buffer. why not name it accordingly? > +{ > + unsigned char *tmpbuf; > + > + /* Never shrink. realloc() calls are expensive. */ > + if (bufsize <= bp_commbufsize) > + return 0; > + > + tmpbuf = realloc(bp_commbuf, bufsize); > + if (!tmpbuf) { > + /* This is debatable. Do we really want to free the existing > + * buffer or do we want to keep it around, especially if memory > + * is already tight? > + */ > + free(bp_commbuf); > + bp_commbuf = NULL; > + bp_commbufsize = 0; > + msg_perr("Out of memory!\n"); > + return OOM_ERROR; > + } > + > + bp_commbuf = tmpbuf; > + bp_commbufsize = bufsize; > + return 0; > +} > + > +static int buspirate_commbuf_init(int bufsize) > +{ > + bp_commbuf = malloc(bufsize); > + if (!bp_commbuf) { > + bp_commbufsize = 0; > + msg_perr("Out of memory!\n"); > + return OOM_ERROR; > + } > + > + bp_commbufsize = bufsize; > + return 0; > +} > + > +static void buspirate_commbuf_shutdown(void) > +{ > + free(bp_commbuf); > + bp_commbuf = NULL; > + bp_commbufsize = 0; > +} why does the buffer need its own (quite trivial) init and shutdown functions instead being embedded in the programmer's init + shutdown functions? > static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, > unsigned int readcnt) > { > @@ -142,6 +191,9 @@ static int buspirate_spi_shutdown(void *data) should probably use the goto free_resources; scheme. else this would create a memleak... > ret = serialport_shutdown(NULL); > if (ret) > return ret; > + > + buspirate_commbuf_shutdown(); > + > msg_pdbg("Bus Pirate shutdown completed.\n"); > > return 0; > @@ -285,6 +337,10 @@ int buspirate_spi_init(void) hm... the init method still uses a constant sized stack buffer. why not init the commbuffer early and use that instead? would need a cleanup in error cases similar to the hunk above though. > return 1; > } > > + /* Sensible default buffer size. */ i would expect that one has chosen a sensible default, hence the comment does not provide any new information. ;) i am more interested into the reason why 16 + 3 is a good default. > + if (buspirate_commbuf_init(16 + 3)) > + return OOM_ERROR; > + > register_spi_programmer(&spi_programmer_buspirate); > > return 0; > @@ -293,55 +349,51 @@ int buspirate_spi_init(void) > static int buspirate_spi_send_command(unsigned int writecnt, unsigned int > readcnt, > const unsigned char *writearr, unsigned char *readarr) > { > - static unsigned char *buf = NULL; > int i = 0, ret = 0; > > if (writecnt > 16 || readcnt > 16 || (readcnt + writecnt) > 16) > return SPI_INVALID_LENGTH; > > /* 3 bytes extra for CS#, len, CS#. */ > - buf = realloc(buf, writecnt + readcnt + 3); > - if (!buf) { > - msg_perr("Out of memory!\n"); > - exit(1); // -1 > - } > + if (buspirate_commbuf_resize(writecnt + readcnt + 3)) > + return OOM_ERROR; > > /* Assert CS# */ > - buf[i++] = 0x02; > + bp_commbuf[i++] = 0x02; > > - buf[i++] = 0x10 | (writecnt + readcnt - 1); > - memcpy(buf + i, writearr, writecnt); > + bp_commbuf[i++] = 0x10 | (writecnt + readcnt - 1); > + memcpy(bp_commbuf + i, writearr, writecnt); > i += writecnt; > - memset(buf + i, 0, readcnt); > + memset(bp_commbuf + i, 0, readcnt); > > i += readcnt; > /* De-assert CS# */ > - buf[i++] = 0x03; > + bp_commbuf[i++] = 0x03; > > - ret = buspirate_sendrecv(buf, i, i); > + ret = buspirate_sendrecv(bp_commbuf, i, i); > > if (ret) { > msg_perr("Bus Pirate communication error!\n"); > return SPI_GENERIC_ERROR; > } > > - if (buf[0] != 0x01) { > + if (bp_commbuf[0] != 0x01) { > msg_perr("Protocol error while lowering CS#!\n"); > return SPI_GENERIC_ERROR; > } > > - if (buf[1] != 0x01) { > + if (bp_commbuf[1] != 0x01) { > msg_perr("Protocol error while reading/writing SPI!\n"); > return SPI_GENERIC_ERROR; > } > > - if (buf[i - 1] != 0x01) { > + if (bp_commbuf[i - 1] != 0x01) { > msg_perr("Protocol error while raising CS#!\n"); > return SPI_GENERIC_ERROR; > } > > /* Skip CS#, length, writearr. */ > - memcpy(readarr, buf + 2 + writecnt, readcnt); > + memcpy(readarr, bp_commbuf + 2 + writecnt, readcnt); > > return ret; > } > diff --git a/flash.h b/flash.h > index 4b1cca2..61180e3 100644 > --- a/flash.h > +++ b/flash.h > @@ -37,6 +37,7 @@ > > /* Error codes */ > #define TIMEOUT_ERROR -101 > +#define OOM_ERROR -100 those should be ERR_TIMEOUT and ERR_OOM imho. but this can be discussed and changed later. > > typedef unsigned long chipaddr; > > -- > 1.7.1 > if at least the memleak gets a FIXME comment, this is Acked-by: Stefan Tauner <[email protected]> -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From 06e81f785a7bfa31a5b42c2c61a78a95ec6468b0 Mon Sep 17 00:00:00 2001 From: Stefan Tauner <[email protected]> Date: Thu, 11 Aug 2011 15:40:33 +0200 Subject: [PATCH] Bus Pirate buffer management revamp Use separate functions for managing the Bus Pirate command/data buffer. Open-coding the buffer management was the first step, now the goal is to make it readable. This is the buffer management part of "Re: [flashrom] [PATCH] Make Bus Pirate init more robust, speed up flashing" The buffer management of the Bus Pirate driver has been revamped to use grow-only buffers with a reasonable initial default size so realloc() will not have to be called in normal operation. A side effect is the ability to switch to a static buffer without major hassle. Handle OOM gracefully. Signed-off-by: Carl-Daniel Hailfinger <[email protected]> --- buspirate_spi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++---------- flash.h | 1 + 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/buspirate_spi.c b/buspirate_spi.c index 467d271..0e1e5cc 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -50,6 +50,55 @@ static int buspirate_serialport_setup(char *dev) #define sp_flush_incoming(...) 0 #endif +static unsigned char *bp_commbuf = NULL; +static int bp_commbufsize = 0; + +static int buspirate_commbuf_resize(int bufsize) +{ + unsigned char *tmpbuf; + + /* Never shrink. realloc() calls are expensive. */ + if (bufsize <= bp_commbufsize) + return 0; + + tmpbuf = realloc(bp_commbuf, bufsize); + if (!tmpbuf) { + /* This is debatable. Do we really want to free the existing + * buffer or do we want to keep it around, especially if memory + * is already tight? + */ + free(bp_commbuf); + bp_commbuf = NULL; + bp_commbufsize = 0; + msg_perr("Out of memory!\n"); + return OOM_ERROR; + } + + bp_commbuf = tmpbuf; + bp_commbufsize = bufsize; + return 0; +} + +static int buspirate_commbuf_init(int bufsize) +{ + bp_commbuf = malloc(bufsize); + if (!bp_commbuf) { + bp_commbufsize = 0; + msg_perr("Out of memory!\n"); + return OOM_ERROR; + } + + bp_commbufsize = bufsize; + return 0; +} + +static void buspirate_commbuf_shutdown(void) +{ + free(bp_commbuf); + bp_commbuf = NULL; + bp_commbufsize = 0; +} + static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, unsigned int readcnt) { @@ -142,6 +191,9 @@ static int buspirate_spi_shutdown(void *data) ret = serialport_shutdown(NULL); if (ret) return ret; + + buspirate_commbuf_shutdown(); + msg_pdbg("Bus Pirate shutdown completed.\n"); return 0; @@ -285,6 +337,10 @@ int buspirate_spi_init(void) return 1; } + /* Sensible default buffer size. */ + if (buspirate_commbuf_init(16 + 3)) + return OOM_ERROR; + register_spi_programmer(&spi_programmer_buspirate); return 0; @@ -293,55 +349,51 @@ int buspirate_spi_init(void) static int buspirate_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { - static unsigned char *buf = NULL; int i = 0, ret = 0; if (writecnt > 16 || readcnt > 16 || (readcnt + writecnt) > 16) return SPI_INVALID_LENGTH; /* 3 bytes extra for CS#, len, CS#. */ - buf = realloc(buf, writecnt + readcnt + 3); - if (!buf) { - msg_perr("Out of memory!\n"); - exit(1); // -1 - } + if (buspirate_commbuf_resize(writecnt + readcnt + 3)) + return OOM_ERROR; /* Assert CS# */ - buf[i++] = 0x02; + bp_commbuf[i++] = 0x02; - buf[i++] = 0x10 | (writecnt + readcnt - 1); - memcpy(buf + i, writearr, writecnt); + bp_commbuf[i++] = 0x10 | (writecnt + readcnt - 1); + memcpy(bp_commbuf + i, writearr, writecnt); i += writecnt; - memset(buf + i, 0, readcnt); + memset(bp_commbuf + i, 0, readcnt); i += readcnt; /* De-assert CS# */ - buf[i++] = 0x03; + bp_commbuf[i++] = 0x03; - ret = buspirate_sendrecv(buf, i, i); + ret = buspirate_sendrecv(bp_commbuf, i, i); if (ret) { msg_perr("Bus Pirate communication error!\n"); return SPI_GENERIC_ERROR; } - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while lowering CS#!\n"); return SPI_GENERIC_ERROR; } - if (buf[1] != 0x01) { + if (bp_commbuf[1] != 0x01) { msg_perr("Protocol error while reading/writing SPI!\n"); return SPI_GENERIC_ERROR; } - if (buf[i - 1] != 0x01) { + if (bp_commbuf[i - 1] != 0x01) { msg_perr("Protocol error while raising CS#!\n"); return SPI_GENERIC_ERROR; } /* Skip CS#, length, writearr. */ - memcpy(readarr, buf + 2 + writecnt, readcnt); + memcpy(readarr, bp_commbuf + 2 + writecnt, readcnt); return ret; } diff --git a/flash.h b/flash.h index 4b1cca2..61180e3 100644 --- a/flash.h +++ b/flash.h @@ -37,6 +37,7 @@ /* Error codes */ #define TIMEOUT_ERROR -101 +#define OOM_ERROR -100 typedef unsigned long chipaddr; -- 1.7.1
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
