On Fri, Jun 12, 2009 at 17:01, Carl-Daniel Hailfinger<[email protected]> wrote: > flashrom only checks for very few chips if the erase worked. > And even when it checks if the erase worked, the result of that check is > often ignored. > > Fix the majority of these problems. More to come, but I'd like to get > this reviewed first. > > Signed-off-by: Carl-Daniel Hailfinger <[email protected]>
I'll do first a review-like listing of my changes and then inline my new patch (and attach too). Attached patch: Signed-off-by: Urja Rannikko <[email protected]> Also carl-daniel's patch with my additions is Acked-by me, but... > +/* start is an offset to the base address of the flash chip */ > +int check_erased_range(struct flashchip *flash, int start, int len) > +{ > + int page_size = flash->page_size; > + uint8_t *cmpbuf = malloc(page_size); malloc (len), not page_size > + if (!cmpbuf) { > + fprintf(stderr, "Could not allocate memory!\n"); > + exit(1); > + } > + memset(cmpbuf, 0xff, len); > + return verify_range(flash, cmpbuf, start, len, "ERASE"); add variable to hold return value, free cmpbuf after calling verify_range > +} > + > +/* start is an offset to the base address of the flash chip */ > +int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int > len, char *message) > +{ > + int i, j, starthere, lenhere; > + chipaddr bios = flash->virtual_memory; > + int page_size = flash->page_size; > + uint8_t *readbuf = malloc(page_size); > + > + if (!readbuf) { > + fprintf(stderr, "Could not allocate memory!\n"); > + exit(1); > + } > + if (start + len > flash->total_size * 1024) { > + fprintf(stderr, "Error: %s called with start 0x%x + len 0x%x > >" > + " total_size 0x%x\n", __func__, start, len, > + flash->total_size * 1024); free readbuf > + return -1; > + } > + if (!len) > + return 0; free readbuf > + if (!message) > + message = "VERIFY"; > + > + /* Warning: This loop has a very unusual condition and body. > + * The loop needs to go through each page with at least one affected > + * byte. The lowest page number is (start / page_size) since that > + * division rounds down. The highest page number we want is the page > + * where the last byte of the range lives. That last byte has the > + * address (start + len - 1), thus the highest page number is > + * (start + len - 1) / page_size. Since we want to include that last > + * page as well, the loop condition uses <=. > + */ > + for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { > + /* Byte position of the first byte in the range in this page. > */ > + starthere = max(start, i * page_size); > + /* Length of bytes in the range in this page. */ > + lenhere = min(start + len, (i + 1) * page_size) - starthere; > + /* starthere is an offset to the base address of the chip. */ > + chip_readn(readbuf, bios + starthere, lenhere); > + for (j = 0; j < lenhere; j++) { > + if (cmpbuf[starthere + j] != readbuf[j]) { > + fprintf(stderr, "%s FAILED at 0x%08x! " > + "Expected=0x%02x, Read=0x%02x\n", > + message, starthere + j, > + cmpbuf[starthere + j], readbuf[j]); as above > + return -1; > + } > + } > + } as above > + return 0; > +} > + Also, changed the erase check in jedec.c to use check_erased_range. My new patch inlined: Index: flash.h =================================================================== --- flash.h (revision 589) +++ flash.h (working copy) @@ -722,6 +722,9 @@ void map_flash_registers(struct flashchip *flash); int read_memmapped(struct flashchip *flash, uint8_t *buf); int min(int a, int b); +int max(int a, int b); +int check_erased_range(struct flashchip *flash, int start, int len); +int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message); extern char *pcidev_bdf; /* layout.c */ Index: en29f002a.c =================================================================== --- en29f002a.c (revision 589) +++ en29f002a.c (working copy) @@ -98,7 +98,10 @@ //chip_writeb(0xF0, bios); programmer_delay(10); - erase_chip_jedec(flash); + if (erase_chip_jedec(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } printf("Programming page: "); for (i = 0; i < total_size; i++) { Index: jedec.c =================================================================== --- jedec.c (revision 589) +++ jedec.c (working copy) @@ -344,13 +344,11 @@ erase_chip_jedec(flash); // dumb check if erase was successful. - for (i = 0; i < total_size; i++) { - if (chip_readb(bios + i) != 0xff) { - printf("ERASE FAILED @%d, val %02x!\n", i, chip_readb(bios + i)); - return -1; - } + if (check_erased_range(flash, 0, total_size)) { + fprintf(stderr,"ERASE FAILED!\n"); + return -1; } - + printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size); Index: sharplhf00l04.c =================================================================== --- sharplhf00l04.c (revision 589) +++ sharplhf00l04.c (working copy) @@ -124,6 +124,10 @@ print_lhf00l04_status(status); printf("DONE BLOCK 0x%x\n", offset); + if (check_erased_range(flash, offset, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; } @@ -135,7 +139,10 @@ printf("total_size is %d; flash->page_size is %d\n", total_size, flash->page_size); for (i = 0; i < total_size; i += flash->page_size) - erase_lhf00l04_block(flash, i); + if (erase_lhf00l04_block(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } printf("DONE ERASE\n"); return 0; @@ -161,9 +168,8 @@ int page_size = flash->page_size; chipaddr bios = flash->virtual_memory; - erase_lhf00l04(flash); - if (chip_readb(bios) != 0xff) { - printf("ERASE FAILED!\n"); + if (erase_lhf00l04(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } printf("Programming page: "); Index: w39v040c.c =================================================================== --- w39v040c.c (revision 589) +++ w39v040c.c (working copy) @@ -60,16 +60,13 @@ { int i; unsigned int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory; - for (i = 0; i < total_size; i += flash->page_size) - erase_sector_jedec(flash->virtual_memory, i); - - for (i = 0; i < total_size; i++) - if (0xff != chip_readb(bios + i)) { - printf("ERASE FAILED at 0x%08x! Expected=0xff, Read=0x%02x\n", i, chip_readb(bios + i)); + for (i = 0; i < total_size; i += flash->page_size) { + if (erase_sector_jedec(flash->virtual_memory, i)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } + } return 0; } @@ -81,8 +78,10 @@ int page_size = flash->page_size; chipaddr bios = flash->virtual_memory; - if (flash->erase(flash)) + if (flash->erase(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; + } printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { Index: stm50flw0x0x.c =================================================================== --- stm50flw0x0x.c (revision 589) +++ stm50flw0x0x.c (working copy) @@ -163,7 +163,6 @@ int erase_block_stm50flw0x0x(struct flashchip *flash, int offset) { chipaddr bios = flash->virtual_memory + offset; - int j; // clear status register chip_writeb(0x50, bios); @@ -175,13 +174,10 @@ wait_stm50flw0x0x(flash->virtual_memory); - for (j = 0; j < flash->page_size; j++) { - if (chip_readb(bios + j) != 0xFF) { - printf("Erase failed at 0x%x\n", offset + j); - return -1; - } + if (check_erased_range(flash, offset, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; } - printf("DONE BLOCK 0x%x\n", offset); return 0; @@ -230,24 +226,29 @@ */ int erase_stm50flw0x0x(struct flashchip *flash) { - int i, rc = 0; + int i; int total_size = flash->total_size * 1024; int page_size = flash->page_size; chipaddr bios = flash->virtual_memory; printf("Erasing page:\n"); - for (i = 0; (i < total_size / page_size) && (rc == 0); i++) { + for (i = 0; i < total_size / page_size; i++) { printf ("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); printf("%04d at address: 0x%08x ", i, i * page_size); - rc = unlock_block_stm50flw0x0x(flash, i * page_size); - if (!rc) - rc = erase_block_stm50flw0x0x(flash, i * page_size); + if (unlock_block_stm50flw0x0x(flash, i * page_size)) { + fprintf(stderr, "UNLOCK FAILED!\n"); + return -1; + } + if (erase_block_stm50flw0x0x(flash, i * page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } } printf("\n"); protect_stm50flw0x0x(bios); - return rc; + return 0; } int write_stm50flw0x0x(struct flashchip *flash, uint8_t * buf) Index: sst_fwhub.c =================================================================== --- sst_fwhub.c (revision 589) +++ sst_fwhub.c (working copy) @@ -104,7 +104,10 @@ return 1; } - erase_block_jedec(flash->virtual_memory, offset); + if (erase_block_jedec(flash->virtual_memory, offset)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } toggle_ready_jedec(flash->virtual_memory); return 0; @@ -114,15 +117,10 @@ { int i; unsigned int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory; - for (i = 0; i < total_size; i += flash->page_size) - erase_sst_fwhub_block(flash, i); - - // dumb check if erase was successful. - for (i = 0; i < total_size; i++) { - if (chip_readb(bios + i) != 0xff) { - printf("ERASE FAILED!\n"); + for (i = 0; i < total_size; i += flash->page_size) { + if (erase_sst_fwhub_block(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } } Index: am29f040b.c =================================================================== --- am29f040b.c (revision 589) +++ am29f040b.c (working copy) @@ -20,8 +20,11 @@ #include "flash.h" -static int erase_sector_29f040b(chipaddr bios, unsigned long address) +static int erase_sector_29f040b(struct flashchip *flash, unsigned long address) { + int page_size = flash->page_size; + chipaddr bios = flash->virtual_memory; + chip_writeb(0xAA, bios + 0x555); chip_writeb(0x55, bios + 0x2AA); chip_writeb(0x80, bios + 0x555); @@ -34,6 +37,10 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(bios + address); + if (check_erased_range(flash, address, page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; } @@ -86,6 +93,7 @@ int erase_29f040b(struct flashchip *flash) { + int total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory; chip_writeb(0xAA, bios + 0x555); @@ -98,6 +106,10 @@ programmer_delay(10); toggle_ready_jedec(bios); + if (check_erased_range(flash, 0, total_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; } @@ -111,7 +123,10 @@ printf("Programming page "); for (i = 0; i < total_size / page_size; i++) { /* erase the page before programming */ - erase_sector_29f040b(bios, i * page_size); + if (erase_sector_29f040b(flash, i * page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } /* write to the sector */ printf("%04d at address: ", i); Index: w39v080fa.c =================================================================== --- w39v080fa.c (revision 589) +++ w39v080fa.c (working copy) @@ -142,9 +142,10 @@ return 0; } -static int erase_sector_winbond_fwhub(chipaddr bios, +static int erase_sector_winbond_fwhub(struct flashchip *flash, unsigned int sector) { + chipaddr bios = flash->virtual_memory; /* Remember: too much sleep can waste your day. */ printf("0x%08x\b\b\b\b\b\b\b\b\b\b", sector); @@ -161,30 +162,30 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(bios); + if (check_erased_range(flash, sector, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; } int erase_winbond_fwhub(struct flashchip *flash) { int i, total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory; unlock_winbond_fwhub(flash); printf("Erasing: "); - for (i = 0; i < total_size; i += flash->page_size) - erase_sector_winbond_fwhub(bios, i); - - printf("\n"); - - for (i = 0; i < total_size; i++) { - if (chip_readb(bios + i) != 0xff) { - fprintf(stderr, "Error: Flash chip erase failed at 0x%08x(0x%02x)\n", i, chip_readb(bios + i)); + for (i = 0; i < total_size; i += flash->page_size) { + if (erase_sector_winbond_fwhub(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } } + printf("\n"); + return 0; } Index: flashrom.c =================================================================== --- flashrom.c (revision 589) +++ flashrom.c (working copy) @@ -205,6 +205,11 @@ return (a < b) ? a : b; } +int max(int a, int b) +{ + return (a > b) ? a : b; +} + char *strcat_realloc(char *dest, const char *src) { dest = realloc(dest, strlen(dest) + strlen(src) + 1); @@ -245,6 +250,79 @@ return ret; } +/* start is an offset to the base address of the flash chip */ +int check_erased_range(struct flashchip *flash, int start, int len) +{ + int rv; + uint8_t *cmpbuf = malloc(len); + + if (!cmpbuf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + } + memset(cmpbuf, 0xff, len); + rv = verify_range(flash, cmpbuf, start, len, "ERASE"); + free(cmpbuf); + return rv; +} + +/* start is an offset to the base address of the flash chip */ +int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message) +{ + int i, j, starthere, lenhere; + chipaddr bios = flash->virtual_memory; + int page_size = flash->page_size; + uint8_t *readbuf = malloc(page_size); + + if (!readbuf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + } + if (start + len > flash->total_size * 1024) { + fprintf(stderr, "Error: %s called with start 0x%x + len 0x%x >" + " total_size 0x%x\n", __func__, start, len, + flash->total_size * 1024); + free(readbuf); + return -1; + } + if (!len) { + free(readbuf); + return 0; + } + if (!message) + message = "VERIFY"; + + /* Warning: This loop has a very unusual condition and body. + * The loop needs to go through each page with at least one affected + * byte. The lowest page number is (start / page_size) since that + * division rounds down. The highest page number we want is the page + * where the last byte of the range lives. That last byte has the + * address (start + len - 1), thus the highest page number is + * (start + len - 1) / page_size. Since we want to include that last + * page as well, the loop condition uses <=. + */ + for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { + /* Byte position of the first byte in the range in this page. */ + starthere = max(start, i * page_size); + /* Length of bytes in the range in this page. */ + lenhere = min(start + len, (i + 1) * page_size) - starthere; + /* starthere is an offset to the base address of the chip. */ + chip_readn(readbuf, bios + starthere, lenhere); + for (j = 0; j < lenhere; j++) { + if (cmpbuf[starthere + j] != readbuf[j]) { + fprintf(stderr, "%s FAILED at 0x%08x! " + "Expected=0x%02x, Read=0x%02x\n", + message, starthere + j, + cmpbuf[starthere + j], readbuf[j]); + free(readbuf); + return -1; + } + } + } + free(readbuf); + return 0; +} + struct flashchip *probe_flash(struct flashchip *first_flash, int force) { struct flashchip *flash; Index: 82802ab.c =================================================================== --- 82802ab.c (revision 589) +++ 82802ab.c (working copy) @@ -110,7 +110,6 @@ { chipaddr bios = flash->virtual_memory + offset; chipaddr wrprotect = flash->virtual_registers + offset + 2; - int j; uint8_t status; // clear status register @@ -129,11 +128,9 @@ // now let's see what the register is status = wait_82802ab(flash->virtual_memory); //print_82802ab_status(status); - for (j = 0; j < flash->page_size; j++) { - if (chip_readb(bios + j) != 0xFF) { - printf("BLOCK ERASE failed at 0x%x\n", offset); - return -1; - } + if (check_erased_range(flash, offset, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; } printf("DONE BLOCK 0x%x\n", offset); @@ -148,7 +145,10 @@ printf("total_size is %d; flash->page_size is %d\n", total_size, flash->page_size); for (i = 0; i < total_size; i += flash->page_size) - erase_82802ab_block(flash, i); + if (erase_82802ab_block(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } printf("DONE ERASE\n"); return 0; @@ -199,7 +199,10 @@ } /* erase block by block and write block by block; this is the most secure way */ - erase_82802ab_block(flash, i * page_size); + if (erase_82802ab_block(flash, i * page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } write_page_82802ab(bios, buf + i * page_size, bios + i * page_size, page_size); } -- urjaman
flashrom_check_erase_range03.diff
Description: Binary data
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
