[PATCH] Bus Pirate buffer management revamp

[Carl-Daniel Hailfinger]

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

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

[Stefan Tauner]

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

if at least the memleak gets a FIXME comment, this is
Acked-by: Stefan Tauner

[Carl-Daniel Hailfinger]

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

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

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

[Stefan Tauner]

On Tue, 15 Nov 2011 08:54:23 +0100
Carl-Daniel Hailfinger  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 :)

[Carl-Daniel Hailfinger]

Am 15.11.2011 09:38 schrieb Stefan Tauner:
> On Tue, 15 Nov 2011 08:54:23 +0100
> Carl-Daniel Hailfinger  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

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

[Carl-Daniel Hailfinger]

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

I got an ack on IRC in #coreboot :
Acked-by: Patrick Georgi

Thanks for the review.
Committed in r1541.

Regards,
Carl-Daniel