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