Hi, this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way). Temporary URL of the findings: http://coresystems.de/~stepan/flashrom-scanbuild/ Stefan This patch cleans up flashrom so that it passes LLVM/clang's scan-build without warnings. It also drops a completely unnecessary goto in ft2232_spi.c Signed-off-by: Stefan Reinauer <stepan@coresystems.de> Index: serprog.c =================================================================== --- serprog.c (revision 721) +++ serprog.c (working copy) @@ -95,7 +95,7 @@ whether the command is supported before doing it */ static int sp_check_avail_automatic = 0; -static void sp_die(char *msg) +static void __attribute__((noreturn)) sp_die(char *msg) { perror(msg); exit(1); Index: sharplhf00l04.c =================================================================== --- sharplhf00l04.c (revision 721) +++ sharplhf00l04.c (working copy) @@ -73,7 +73,6 @@ uint8_t wait_lhf00l04(chipaddr bios) { uint8_t status; - uint8_t id1, id2; chip_writeb(0x70, bios); if ((chip_readb(bios) & 0x80) == 0) { // it's busy @@ -82,13 +81,13 @@ status = chip_readb(bios); - // put another command to get out of status register mode + // put another command to get out of status register mode. chip_writeb(0x90, bios); programmer_delay(10); - id1 = chip_readb(bios); - id2 = chip_readb(bios + 0x01); + chip_readb(bios); // vendor ID + chip_readb(bios + 0x01); // device ID // this is needed to jam it out of "read id" mode chip_writeb(0xAA, bios + 0x5555); Index: ft2232_spi.c =================================================================== --- ft2232_spi.c (revision 721) +++ ft2232_spi.c (working copy) @@ -70,7 +70,6 @@ int f; struct ftdi_context *ftdic = &ftdic_context; unsigned char buf[512]; - unsigned char port_val = 0; char *portpos = NULL; int ft2232_type = FTDI_FT4232H; enum ftdi_interface ft2232_interface = INTERFACE_B; @@ -175,9 +174,8 @@ * dir: 0x0b CS=output, DI=input, DO=output, SK=output */ #define CS_BIT 0x08 - buf[0] = SET_BITS_LOW; - buf[1] = (port_val = CS_BIT); + buf[1] = CS_BIT; buf[2] = 0x0b; if (send_buf(ftdic, buf, 3)) return -1; @@ -195,7 +193,6 @@ { struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; - unsigned char port_val = 0; int i, ret = 0; if (writecnt > 65536 || readcnt > 65536) @@ -213,10 +210,11 @@ * as possible together. if we're not expecting to * read, we can assert CS, write, and deassert CS all * in one shot. if reading, we do three separate - * operations. */ + * operations. + */ printf_debug("Assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = (port_val &= ~CS_BIT); + buf[i++] = 0 & ~CS_BIT; /* assertive */ buf[i++] = 0x0b; if (writecnt) { @@ -236,20 +234,19 @@ buf[i++] = ((readcnt - 1) >> 8) & 0xff; ret = send_buf(ftdic, buf, i); i = 0; - if (ret) goto deassert_cs; + if (ret == 0) { + /* FIXME: This is unreliable. There's no guarantee that we read + * the response directly after sending the read command. + * We may be scheduled out etc. + */ + ret = get_buf(ftdic, readarr, readcnt); + } - /* FIXME: This is unreliable. There's no guarantee that we read - * the response directly after sending the read command. - * We may be scheduled out etc. - */ - ret = get_buf(ftdic, readarr, readcnt); - } -deassert_cs: printf_debug("De-assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = (port_val |= CS_BIT); + buf[i++] = CS_BIT; buf[i++] = 0x0b; if (send_buf(ftdic, buf, i)) return -1; Index: sst28sf040.c =================================================================== --- sst28sf040.c (revision 721) +++ sst28sf040.c (working copy) @@ -30,28 +30,24 @@ static void protect_28sf040(chipaddr bios) { - uint8_t tmp; - - tmp = chip_readb(bios + 0x1823); - tmp = chip_readb(bios + 0x1820); - tmp = chip_readb(bios + 0x1822); - tmp = chip_readb(bios + 0x0418); - tmp = chip_readb(bios + 0x041B); - tmp = chip_readb(bios + 0x0419); - tmp = chip_readb(bios + 0x040A); + chip_readb(bios + 0x1823); + chip_readb(bios + 0x1820); + chip_readb(bios + 0x1822); + chip_readb(bios + 0x0418); + chip_readb(bios + 0x041B); + chip_readb(bios + 0x0419); + chip_readb(bios + 0x040A); } static void unprotect_28sf040(chipaddr bios) { - uint8_t tmp; - - tmp = chip_readb(bios + 0x1823); - tmp = chip_readb(bios + 0x1820); - tmp = chip_readb(bios + 0x1822); - tmp = chip_readb(bios + 0x0418); - tmp = chip_readb(bios + 0x041B); - tmp = chip_readb(bios + 0x0419); - tmp = chip_readb(bios + 0x041A); + chip_readb(bios + 0x1823); + chip_readb(bios + 0x1820); + chip_readb(bios + 0x1822); + chip_readb(bios + 0x0418); + chip_readb(bios + 0x041B); + chip_readb(bios + 0x0419); + chip_readb(bios + 0x041A); } static int erase_sector_28sf040(struct flashchip *flash, unsigned long address, int sector_size) Index: stm50flw0x0x.c =================================================================== --- stm50flw0x0x.c (revision 721) +++ stm50flw0x0x.c (working copy) @@ -54,19 +54,17 @@ static void wait_stm50flw0x0x(chipaddr bios) { - uint8_t id1; - // id2; - chip_writeb(0x70, bios); if ((chip_readb(bios) & 0x80) == 0) { // it's busy while ((chip_readb(bios) & 0x80) == 0) ; } + // put another command to get out of status register mode chip_writeb(0x90, bios); programmer_delay(10); - id1 = chip_readb(bios); + chip_readb(bios); // Read device ID (to make sure?) // this is needed to jam it out of "read id" mode chip_writeb(0xAA, bios + 0x5555); Index: sb600spi.c =================================================================== --- sb600spi.c (revision 721) +++ sb600spi.c (working copy) @@ -64,6 +64,11 @@ printf("Programming flash"); for (i = 0; i < total_size; i++, buf++) { result = spi_byte_program(i, *buf); + if (result) { + // spi_byte_program reported the error for us already + printf_debug("... continuing anyway.\n"); + } + /* wait program complete. */ if (i % 0x8000 == 0) printf("."); Index: 82802ab.c =================================================================== --- 82802ab.c (revision 721) +++ 82802ab.c (working copy) @@ -30,16 +30,15 @@ #include <stdlib.h> #include "flash.h" -// I need that Berkeley bit-map printer void print_82802ab_status(uint8_t status) { - printf("%s", status & 0x80 ? "Ready:" : "Busy:"); - printf("%s", status & 0x40 ? "BE SUSPEND:" : "BE RUN/FINISH:"); - printf("%s", status & 0x20 ? "BE ERROR:" : "BE OK:"); - printf("%s", status & 0x10 ? "PROG ERR:" : "PROG OK:"); - printf("%s", status & 0x8 ? "VP ERR:" : "VPP OK:"); - printf("%s", status & 0x4 ? "PROG SUSPEND:" : "PROG RUN/FINISH:"); - printf("%s", status & 0x2 ? "WP|TBL#|WP#,ABORT:" : "UNLOCK:"); + printf_debug("%s", status & 0x80 ? "Ready:" : "Busy:"); + printf_debug("%s", status & 0x40 ? "BE SUSPEND:" : "BE RUN/FINISH:"); + printf_debug("%s", status & 0x20 ? "BE ERROR:" : "BE OK:"); + printf_debug("%s", status & 0x10 ? "PROG ERR:" : "PROG OK:"); + printf_debug("%s", status & 0x8 ? "VP ERR:" : "VPP OK:"); + printf_debug("%s", status & 0x4 ? "PROG SUSPEND:" : "PROG RUN/FINISH:"); + printf_debug("%s", status & 0x2 ? "WP|TBL#|WP#,ABORT:" : "UNLOCK:"); } int probe_82802ab(struct flashchip *flash) @@ -98,20 +97,19 @@ // clear status register chip_writeb(0x50, bios); - //printf("Erase at %p\n", bios); + // clear write protect - //printf("write protect is at %p\n", (wrprotect)); - //printf("write protect is 0x%x\n", *(wrprotect)); chip_writeb(0, wrprotect); - //printf("write protect is 0x%x\n", *(wrprotect)); // now start it chip_writeb(0x20, bios); chip_writeb(0xd0, bios); programmer_delay(10); + // now let's see what the register is status = wait_82802ab(flash->virtual_memory); - //print_82802ab_status(status); + print_82802ab_status(status); + if (check_erased_range(flash, offset, flash->page_size)) { fprintf(stderr, "ERASE FAILED!\n"); return -1;
Acked-by: Ronald G. Minnich <rminnich@gmail.com> Though I will miss my comment about the berkely bit-map printer. ron
ron minnich wrote:
Acked-by: Ronald G. Minnich <rminnich@gmail.com>
Though I will miss my comment about the berkely bit-map printer.
Ah, what is that about? I googled for it and couldn't find any hits,... Maybe we should leave that then :-) Stefan
Hi, On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
It does fix the warnings, but it papers over some of the bugs found by scan-build. Given that the real bugfixes would have to revert parts of the patch, please hold off committing. I'll post a detailed review later. Regards, Carl-Daniel -- http://www.hailfinger.org/
Carl-Daniel Hailfinger wrote:
Hi,
On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
It does fix the warnings, but it papers over some of the bugs found by scan-build. Given that the real bugfixes would have to revert parts of the patch, please hold off committing. I'll post a detailed review later.
Looking forward to that. I didn't intend to paper over anything. I surely didn't intend a redesign of that code either though ;-) Stefan
[Paul, I included you in CC because you wrote the ft2232 code.] Hi, On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
The following parts of the patch are Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> sst28sf040.c (definitely) sharplhf00l04.c (we shouldn't enter ID mode at all) stm50flw0x0x.c (we shouldn't enter ID mode at all) serprog.c (serprog_die should be killed, but for now...) I can do the ID mode removal in a followup patch. The serprog.c change is correct, but we need a design review of serprog anyway and I hope to kill serprog_die() completely in that review. ft2232_spi.c is a bit special because I don't have any idea what port_val is used for. It looks like a dead variable, but maybe it was intended to track the status of other lines besides CS# and in that case we should keep the variable and fill it with the correct value. In 82802ab.c I miss some of the commented out commands. Some of them should be fixed and reenabled (especially the chip protection status). Regards, Carl-Daniel -- http://www.hailfinger.org/
carl-daniel wrote:
[Paul, I included you in CC because you wrote the ft2232 code.]
thanks carl.
Hi,
On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
The following parts of the patch are Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
sst28sf040.c (definitely) sharplhf00l04.c (we shouldn't enter ID mode at all) stm50flw0x0x.c (we shouldn't enter ID mode at all) serprog.c (serprog_die should be killed, but for now...)
I can do the ID mode removal in a followup patch.
The serprog.c change is correct, but we need a design review of serprog anyway and I hope to kill serprog_die() completely in that review. ft2232_spi.c is a bit special because I don't have any idea what port_val is used for. It looks like a dead variable, but maybe it was intended to track the status of other lines besides CS# and in that case we should keep the variable and fill it with the correct value.
it looks dead to me. i tracked it back as coming from the bitbang.c sample code that came (i think) with libftdi, and in my copy of that code, at least, it's already a useless variable. get rid of it. paul
In 82802ab.c I miss some of the commented out commands. Some of them should be fixed and reenabled (especially the chip protection status).
Regards, Carl-Daniel
=--------------------- paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 58.8 degrees)
On 16.09.2009 06:25, Paul Fox wrote:
carl-daniel wrote:
[Paul, I included you in CC because you wrote the ft2232 code.]
thanks carl.
Thanks for commenting. By the way, are you using plain flashrom from svn at OLPC or do you have any local patches applied?
On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
The following parts of the patch are Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
sst28sf040.c (definitely) sharplhf00l04.c (we shouldn't enter ID mode at all) stm50flw0x0x.c (we shouldn't enter ID mode at all) serprog.c (serprog_die should be killed, but for now...)
ft2232_spi.c is a bit special because I don't have any idea what port_val is used for. It looks like a dead variable, but maybe it was intended to track the status of other lines besides CS# and in that case we should keep the variable and fill it with the correct value.
it looks dead to me. i tracked it back as coming from the bitbang.c sample code that came (i think) with libftdi, and in my copy of that code, at least, it's already a useless variable. get rid of it.
Stefan, with that information, the changes in ft2232_spi.c are Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Regards, Carl-Daniel -- http://www.hailfinger.org/
Carl-Daniel Hailfinger wrote:
[Paul, I included you in CC because you wrote the ft2232 code.]
Hi,
On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
The following parts of the patch are Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
sst28sf040.c (definitely) sharplhf00l04.c (we shouldn't enter ID mode at all) stm50flw0x0x.c (we shouldn't enter ID mode at all) serprog.c (serprog_die should be killed, but for now...)
Ok, r722
I can do the ID mode removal in a followup patch.
The serprog.c change is correct, but we need a design review of serprog anyway and I hope to kill serprog_die() completely in that review.
Don't let my fixes keep you from doing further work on the code.
ft2232_spi.c is a bit special because I don't have any idea what port_val is used for. It looks like a dead variable, but maybe it was intended to track the status of other lines besides CS# and in that case we should keep the variable and fill it with the correct value.
It is a dead variable. At least in this instance of code. If this is not the way it's supposed to be, we still want to get rid of it so it is easier to spot that part in the future.
In 82802ab.c I miss some of the commented out commands. Some of them should be fixed and reenabled (especially the chip protection status).
I didn't comment out or remove anny commands, just debug output. Stefan
On 16.09.2009 10:25, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 15.09.2009 19:49, Stefan Reinauer wrote:
this patch fixes all 27 flashrom source code issues reported by LLVM/clang's scan-build (r79326, new build on the way).
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
The following parts of the patch are Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
sst28sf040.c (definitely) sharplhf00l04.c (we shouldn't enter ID mode at all) stm50flw0x0x.c (we shouldn't enter ID mode at all) serprog.c (serprog_die should be killed, but for now...)
Ok, r722
Thanks!
I can do the ID mode removal in a followup patch.
The serprog.c change is correct, but we need a design review of serprog anyway and I hope to kill serprog_die() completely in that review.
Don't let my fixes keep you from doing further work on the code.
I was thinking about postponing the immediate fix for a longterm cleanup, but I think that will need a bit more time and the postponement wouldn't have any purpose except turning up on scan-build and reminding us of the larger cleanup... so in the end I'm happy you applied this.
ft2232_spi.c is a bit special because I don't have any idea what port_val is used for. It looks like a dead variable, but maybe it was intended to track the status of other lines besides CS# and in that case we should keep the variable and fill it with the correct value.
It is a dead variable. At least in this instance of code. If this is not the way it's supposed to be, we still want to get rid of it so it is easier to spot that part in the future.
True, see my reaction to Paul's mail. No objection anymore.
In 82802ab.c I miss some of the commented out commands. Some of them should be fixed and reenabled (especially the chip protection status).
I didn't comment out or remove anny commands, just debug output.
My apologies. The wording was a bit unclear. You removed some commands (write protect status printing) which were already commented out and thus inactive. I believe they shouldn't have been commented out in the first place. I'll send a patch to restore them into full effect on top of r723. Regards, Carl-Daniel -- http://www.hailfinger.org/
participants (4)
-
Carl-Daniel Hailfinger -
Paul Fox -
ron minnich -
Stefan Reinauer