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 c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-buspirate_buffermanagement/flash.h =================================================================== --- flashrom-buspirate_buffermanagement/flash.h (Revision 1184) +++ flashrom-buspirate_buffermanagement/flash.h (Arbeitskopie) @@ -35,6 +35,9 @@
#define ERROR_PTR ((void*)-1)
+/* Error codes */ +#define OOM_ERROR -100 + typedef unsigned long chipaddr;
int register_shutdown(void (*function) (void *data), void *data); Index: flashrom-buspirate_buffermanagement/buspirate_spi.c =================================================================== --- flashrom-buspirate_buffermanagement/buspirate_spi.c (Revision 1184) +++ flashrom-buspirate_buffermanagement/buspirate_spi.c (Arbeitskopie) @@ -46,6 +46,55 @@ #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) { int i, ret = 0; @@ -231,6 +280,10 @@ return 1; }
+ /* Sensible default buffer size. */ + if (buspirate_commbuf_init(16 + 3)) + return OOM_ERROR; + buses_supported = CHIP_BUSTYPE_SPI; spi_controller = SPI_CONTROLLER_BUSPIRATE;
@@ -267,6 +320,9 @@ ret = serialport_shutdown(); if (ret) return ret; + + buspirate_commbuf_shutdown(); + msg_pdbg("Bus Pirate shutdown completed.\n");
return 0; @@ -275,55 +331,51 @@ 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; }
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 stefan.tauner@student.tuwien.ac.at 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 c-d.hailfinger.devel.2006@gmx.net
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 stefan.tauner@student.tuwien.ac.at
Am 11.08.2011 17:00 schrieb Stefan Tauner:
Use a separate function 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.
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 c-d.hailfinger.devel.2006@gmx.net
New version, significant changes to fix all memleaks and to address all review comments. Stefan, I know you already acked the earlier version subject to a few changes, but this version probably needs a new review. Sorry about that. If you want a diff between the old and new version, tell me and I'll send it. The shutdown function still has its own static buffer to allow shutdown even if memory is tight, but AFAICS that should not be needed because the buffer can never be too small for shutdown once init is complete. If you agree with that assessment, I could change the shutdown function to use the common buffer as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-buspirate_buffermanagement/flash.h =================================================================== --- flashrom-buspirate_buffermanagement/flash.h (Revision 1464) +++ flashrom-buspirate_buffermanagement/flash.h (Arbeitskopie) @@ -36,6 +36,7 @@ #define ERROR_PTR ((void*)-1)
/* Error codes */ +#define ERROR_OOM -100 #define TIMEOUT_ERROR -101
typedef unsigned long chipaddr; Index: flashrom-buspirate_buffermanagement/buspirate_spi.c =================================================================== --- flashrom-buspirate_buffermanagement/buspirate_spi.c (Revision 1464) +++ flashrom-buspirate_buffermanagement/buspirate_spi.c (Arbeitskopie) @@ -39,6 +39,7 @@ { /* 115200bps, 8 databits, no parity, 1 stopbit */ sp_fd = sp_openserport(dev, 115200); + /* FIXME: Error checking */ return 0; } #else @@ -49,6 +50,29 @@ #define sp_flush_incoming(...) 0 #endif
+static unsigned char *bp_commbuf = NULL; +static int bp_commbufsize = 0; + +static int buspirate_commbuf_grow(int bufsize) +{ + unsigned char *tmpbuf; + + /* Never shrink. realloc() calls are expensive. */ + if (bufsize <= bp_commbufsize) + return 0; + + tmpbuf = realloc(bp_commbuf, bufsize); + if (!tmpbuf) { + /* Keep the existing buffer because memory is already tight. */ + msg_perr("Out of memory!\n"); + return ERROR_OOM; + } + + bp_commbuf = tmpbuf; + bp_commbufsize = bufsize; + return 0; +} + static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, unsigned int readcnt) { @@ -114,41 +138,48 @@ static int buspirate_spi_shutdown(void *data) { unsigned char buf[5]; - int ret = 0; + int ret = 0, ret2 = 0;
/* Exit raw SPI mode (enter raw bitbang mode) */ buf[0] = 0x00; ret = buspirate_sendrecv(buf, 1, 5); if (ret) - return ret; + goto out_shutdown; if (memcmp(buf, "BBIO", 4)) { msg_perr("Entering raw bitbang mode failed!\n"); - return 1; + ret = 1; + goto out_shutdown; } msg_pdbg("Raw bitbang mode version %c\n", buf[4]); if (buf[4] != '1') { msg_perr("Can't handle raw bitbang mode version %c!\n", buf[4]); - return 1; + ret = 1; + goto out_shutdown; } /* Reset Bus Pirate (return to user terminal) */ buf[0] = 0x0f; ret = buspirate_sendrecv(buf, 1, 0); - if (ret) - return ret;
+out_shutdown: /* Shut down serial port communication */ - ret = serialport_shutdown(NULL); + ret2 = serialport_shutdown(NULL); + /* Keep the oldest error, it is probably the best indicator. */ + if (ret2 && !ret) + ret = ret2; + bp_commbufsize = 0; + free(bp_commbuf); + bp_commbuf = NULL; if (ret) - return ret; - msg_pdbg("Bus Pirate shutdown completed.\n"); + msg_pdbg("Bus Pirate shutdown failed.\n"); + else + msg_pdbg("Bus Pirate shutdown completed.\n");
- return 0; + return ret; }
int buspirate_spi_init(void) { - unsigned char buf[512]; char *dev = NULL; char *speed = NULL; int spispeed = 0x7; @@ -178,10 +209,24 @@ /* This works because speeds numbering starts at 0 and is contiguous. */ msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
+ /* Default buffer size is 19: 16 bytes data, 3 bytes control. */ +#define DEFAULT_BUFSIZE (16 + 3) + bp_commbuf = malloc(DEFAULT_BUFSIZE); + if (!bp_commbuf) { + bp_commbufsize = 0; + msg_perr("Out of memory!\n"); + return ERROR_OOM; + } + bp_commbufsize = DEFAULT_BUFSIZE; + ret = buspirate_serialport_setup(dev); - if (ret) + free(dev); + if (ret) { + bp_commbufsize = 0; + free(bp_commbuf); + bp_commbuf = NULL; return ret; - free(dev); + }
if (register_shutdown(buspirate_spi_shutdown, NULL)) return 1; @@ -189,9 +234,9 @@ /* This is the brute force version, but it should work. */ for (i = 0; i < 19; i++) { /* Enter raw bitbang mode */ - buf[0] = 0x00; + bp_commbuf[0] = 0x00; /* Send the command, don't read the response. */ - ret = buspirate_sendrecv(buf, 1, 0); + ret = buspirate_sendrecv(bp_commbuf, 1, 0); if (ret) return ret; /* Read any response and discard it. */ @@ -212,76 +257,78 @@ sp_flush_incoming(); } /* Enter raw bitbang mode */ - buf[0] = 0x00; - ret = buspirate_sendrecv(buf, 1, 5); + bp_commbuf[0] = 0x00; + ret = buspirate_sendrecv(bp_commbuf, 1, 5); if (ret) return ret; - if (memcmp(buf, "BBIO", 4)) { + if (memcmp(bp_commbuf, "BBIO", 4)) { msg_perr("Entering raw bitbang mode failed!\n"); msg_pdbg("Got %02x%02x%02x%02x%02x\n", - buf[0], buf[1], buf[2], buf[3], buf[4]); + bp_commbuf[0], bp_commbuf[1], bp_commbuf[2], + bp_commbuf[3], bp_commbuf[4]); return 1; } - msg_pdbg("Raw bitbang mode version %c\n", buf[4]); - if (buf[4] != '1') { + msg_pdbg("Raw bitbang mode version %c\n", bp_commbuf[4]); + if (bp_commbuf[4] != '1') { msg_perr("Can't handle raw bitbang mode version %c!\n", - buf[4]); + bp_commbuf[4]); return 1; } /* Enter raw SPI mode */ - buf[0] = 0x01; - ret = buspirate_sendrecv(buf, 1, 4); + bp_commbuf[0] = 0x01; + ret = buspirate_sendrecv(bp_commbuf, 1, 4); if (ret) return ret; - if (memcmp(buf, "SPI", 3)) { + if (memcmp(bp_commbuf, "SPI", 3)) { msg_perr("Entering raw SPI mode failed!\n"); msg_pdbg("Got %02x%02x%02x%02x\n", - buf[0], buf[1], buf[2], buf[3]); + bp_commbuf[0], bp_commbuf[1], bp_commbuf[2], + bp_commbuf[3]); return 1; } - msg_pdbg("Raw SPI mode version %c\n", buf[3]); - if (buf[3] != '1') { + msg_pdbg("Raw SPI mode version %c\n", bp_commbuf[3]); + if (bp_commbuf[3] != '1') { msg_perr("Can't handle raw SPI mode version %c!\n", - buf[3]); + bp_commbuf[3]); return 1; }
/* Initial setup (SPI peripherals config): Enable power, CS high, AUX */ - buf[0] = 0x40 | 0xb; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x40 | 0xb; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting power/CS/AUX!\n"); return 1; }
/* Set SPI speed */ - buf[0] = 0x60 | spispeed; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x60 | spispeed; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI speed!\n"); return 1; } /* Set SPI config: output type, idle, clock edge, sample */ - buf[0] = 0x80 | 0xa; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x80 | 0xa; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI config!\n"); return 1; }
/* De-assert CS# */ - buf[0] = 0x03; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x03; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while raising CS#!\n"); return 1; } @@ -294,55 +341,51 @@ 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_grow(writecnt + readcnt + 3)) + return ERROR_OOM;
/* 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; }
On Tue, 15 Nov 2011 08:54:23 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The shutdown function still has its own static buffer to allow shutdown even if memory is tight, but AFAICS that should not be needed because the buffer can never be too small for shutdown once init is complete. If you agree with that assessment, I could change the shutdown function to use the common buffer as well.
from a glimpse that's true afaics. the shutdown function is registered after the buffer is set to DEFAULT_BUFSIZE. one would want to assert that this is big enough for the shutdown function to work in general... the grow function does what it says. i have not investigated if the buffer makes sense at all, or if it should by generalized... nevertheless i think it would make sense to implement your proposal before a final review... wont get much messier than it is now i guess :)
Am 15.11.2011 09:38 schrieb Stefan Tauner:
On Tue, 15 Nov 2011 08:54:23 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
The shutdown function still has its own static buffer to allow shutdown even if memory is tight, but AFAICS that should not be needed because the buffer can never be too small for shutdown once init is complete. If you agree with that assessment, I could change the shutdown function to use the common buffer as well.
from a glimpse that's true afaics. the shutdown function is registered after the buffer is set to DEFAULT_BUFSIZE. one would want to assert that this is big enough for the shutdown function to work in general... the grow function does what it says. i have not investigated if the buffer makes sense at all, or if it should by generalized... nevertheless i think it would make sense to implement your proposal before a final review... wont get much messier than it is now i guess :)
New version, all review comments addressed. Tested, works.
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 c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-buspirate_buffermanagement/flash.h =================================================================== --- flashrom-buspirate_buffermanagement/flash.h (Revision 1540) +++ flashrom-buspirate_buffermanagement/flash.h (Arbeitskopie) @@ -36,6 +36,7 @@ #define ERROR_PTR ((void*)-1)
/* Error codes */ +#define ERROR_OOM -100 #define TIMEOUT_ERROR -101
typedef unsigned long chipaddr; Index: flashrom-buspirate_buffermanagement/buspirate_spi.c =================================================================== --- flashrom-buspirate_buffermanagement/buspirate_spi.c (Revision 1540) +++ flashrom-buspirate_buffermanagement/buspirate_spi.c (Arbeitskopie) @@ -39,6 +39,7 @@ { /* 115200bps, 8 databits, no parity, 1 stopbit */ sp_fd = sp_openserport(dev, 115200); + /* FIXME: Error checking */ return 0; } #else @@ -49,6 +50,29 @@ #define sp_flush_incoming(...) 0 #endif
+static unsigned char *bp_commbuf = NULL; +static int bp_commbufsize = 0; + +static int buspirate_commbuf_grow(int bufsize) +{ + unsigned char *tmpbuf; + + /* Never shrink. realloc() calls are expensive. */ + if (bufsize <= bp_commbufsize) + return 0; + + tmpbuf = realloc(bp_commbuf, bufsize); + if (!tmpbuf) { + /* Keep the existing buffer because memory is already tight. */ + msg_perr("Out of memory!\n"); + return ERROR_OOM; + } + + bp_commbuf = tmpbuf; + bp_commbufsize = bufsize; + return 0; +} + static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, unsigned int readcnt) { @@ -116,42 +140,48 @@
static int buspirate_spi_shutdown(void *data) { - unsigned char buf[5]; - int ret = 0; + int ret = 0, ret2 = 0; + /* No need to allocate a buffer here, we know that bp_commbuf is at least DEFAULT_BUFSIZE big. */
/* Exit raw SPI mode (enter raw bitbang mode) */ - buf[0] = 0x00; - ret = buspirate_sendrecv(buf, 1, 5); + bp_commbuf[0] = 0x00; + ret = buspirate_sendrecv(bp_commbuf, 1, 5); if (ret) - return ret; - if (memcmp(buf, "BBIO", 4)) { + goto out_shutdown; + if (memcmp(bp_commbuf, "BBIO", 4)) { msg_perr("Entering raw bitbang mode failed!\n"); - return 1; + ret = 1; + goto out_shutdown; } - msg_pdbg("Raw bitbang mode version %c\n", buf[4]); - if (buf[4] != '1') { - msg_perr("Can't handle raw bitbang mode version %c!\n", - buf[4]); - return 1; + msg_pdbg("Raw bitbang mode version %c\n", bp_commbuf[4]); + if (bp_commbuf[4] != '1') { + msg_perr("Can't handle raw bitbang mode version %c!\n", bp_commbuf[4]); + ret = 1; + goto out_shutdown; } /* Reset Bus Pirate (return to user terminal) */ - buf[0] = 0x0f; - ret = buspirate_sendrecv(buf, 1, 0); - if (ret) - return ret; + bp_commbuf[0] = 0x0f; + ret = buspirate_sendrecv(bp_commbuf, 1, 0);
+out_shutdown: /* Shut down serial port communication */ - ret = serialport_shutdown(NULL); + ret2 = serialport_shutdown(NULL); + /* Keep the oldest error, it is probably the best indicator. */ + if (ret2 && !ret) + ret = ret2; + bp_commbufsize = 0; + free(bp_commbuf); + bp_commbuf = NULL; if (ret) - return ret; - msg_pdbg("Bus Pirate shutdown completed.\n"); + msg_pdbg("Bus Pirate shutdown failed.\n"); + else + msg_pdbg("Bus Pirate shutdown completed.\n");
- return 0; + return ret; }
int buspirate_spi_init(void) { - unsigned char buf[512]; char *dev = NULL; char *speed = NULL; int spispeed = 0x7; @@ -181,10 +211,24 @@ /* This works because speeds numbering starts at 0 and is contiguous. */ msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
+ /* Default buffer size is 19: 16 bytes data, 3 bytes control. */ +#define DEFAULT_BUFSIZE (16 + 3) + bp_commbuf = malloc(DEFAULT_BUFSIZE); + if (!bp_commbuf) { + bp_commbufsize = 0; + msg_perr("Out of memory!\n"); + return ERROR_OOM; + } + bp_commbufsize = DEFAULT_BUFSIZE; + ret = buspirate_serialport_setup(dev); - if (ret) + free(dev); + if (ret) { + bp_commbufsize = 0; + free(bp_commbuf); + bp_commbuf = NULL; return ret; - free(dev); + }
if (register_shutdown(buspirate_spi_shutdown, NULL)) return 1; @@ -192,9 +236,9 @@ /* This is the brute force version, but it should work. */ for (i = 0; i < 19; i++) { /* Enter raw bitbang mode */ - buf[0] = 0x00; + bp_commbuf[0] = 0x00; /* Send the command, don't read the response. */ - ret = buspirate_sendrecv(buf, 1, 0); + ret = buspirate_sendrecv(bp_commbuf, 1, 0); if (ret) return ret; /* Read any response and discard it. */ @@ -215,76 +259,78 @@ sp_flush_incoming(); } /* Enter raw bitbang mode */ - buf[0] = 0x00; - ret = buspirate_sendrecv(buf, 1, 5); + bp_commbuf[0] = 0x00; + ret = buspirate_sendrecv(bp_commbuf, 1, 5); if (ret) return ret; - if (memcmp(buf, "BBIO", 4)) { + if (memcmp(bp_commbuf, "BBIO", 4)) { msg_perr("Entering raw bitbang mode failed!\n"); msg_pdbg("Got %02x%02x%02x%02x%02x\n", - buf[0], buf[1], buf[2], buf[3], buf[4]); + bp_commbuf[0], bp_commbuf[1], bp_commbuf[2], + bp_commbuf[3], bp_commbuf[4]); return 1; } - msg_pdbg("Raw bitbang mode version %c\n", buf[4]); - if (buf[4] != '1') { + msg_pdbg("Raw bitbang mode version %c\n", bp_commbuf[4]); + if (bp_commbuf[4] != '1') { msg_perr("Can't handle raw bitbang mode version %c!\n", - buf[4]); + bp_commbuf[4]); return 1; } /* Enter raw SPI mode */ - buf[0] = 0x01; - ret = buspirate_sendrecv(buf, 1, 4); + bp_commbuf[0] = 0x01; + ret = buspirate_sendrecv(bp_commbuf, 1, 4); if (ret) return ret; - if (memcmp(buf, "SPI", 3)) { + if (memcmp(bp_commbuf, "SPI", 3)) { msg_perr("Entering raw SPI mode failed!\n"); msg_pdbg("Got %02x%02x%02x%02x\n", - buf[0], buf[1], buf[2], buf[3]); + bp_commbuf[0], bp_commbuf[1], bp_commbuf[2], + bp_commbuf[3]); return 1; } - msg_pdbg("Raw SPI mode version %c\n", buf[3]); - if (buf[3] != '1') { + msg_pdbg("Raw SPI mode version %c\n", bp_commbuf[3]); + if (bp_commbuf[3] != '1') { msg_perr("Can't handle raw SPI mode version %c!\n", - buf[3]); + bp_commbuf[3]); return 1; }
/* Initial setup (SPI peripherals config): Enable power, CS high, AUX */ - buf[0] = 0x40 | 0xb; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x40 | 0xb; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting power/CS/AUX!\n"); return 1; }
/* Set SPI speed */ - buf[0] = 0x60 | spispeed; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x60 | spispeed; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI speed!\n"); return 1; } /* Set SPI config: output type, idle, clock edge, sample */ - buf[0] = 0x80 | 0xa; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x80 | 0xa; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI config!\n"); return 1; }
/* De-assert CS# */ - buf[0] = 0x03; - ret = buspirate_sendrecv(buf, 1, 1); + bp_commbuf[0] = 0x03; + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; - if (buf[0] != 0x01) { + if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while raising CS#!\n"); return 1; } @@ -300,7 +346,6 @@ const unsigned char *writearr, unsigned char *readarr) { - static unsigned char *buf = NULL; unsigned int i = 0; int ret = 0;
@@ -308,48 +353,45 @@ 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_grow(writecnt + readcnt + 3)) + return ERROR_OOM;
/* 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; }
Am 06.06.2012 12:06 schrieb Carl-Daniel Hailfinger:
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 c-d.hailfinger.devel.2006@gmx.net
I got an ack on IRC in #coreboot : Acked-by: Patrick Georgi patrick@georgi-clan.de
Thanks for the review. Committed in r1541.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Tauner