[flashrom] [PATCH] Bus Pirate buffer management revamp
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Thu Aug 11 17:00:47 CEST 2011
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 at 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 at 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 at student.tuwien.ac.at>
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Bus-Pirate-buffer-management-revamp.patch
Type: text/x-patch
Size: 4879 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110811/67e4728a/attachment.patch>
More information about the flashrom
mailing list