Am 15.11.2011 09:38 schrieb Stefan Tauner: > On Tue, 15 Nov 2011 08:54:23 +0100 > Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> 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; } -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
