Re: [flashrom] [PATCH] Bus Pirate buffer management revamp

[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