Second try for this patch, after some review/comments on #coreboot channel. - renamed generic_read() to read_flash() - renamed options chunk_start/size to offset/length - fixed some indentation spaces - fixed some error messages - replaces chk_start/size by offset/length - added a description in the usage()
The goal for this patch is to allow flashrom to read some part of the flash, described by a pair offset/length. Added flashrom arguments --offset (-S) and --length (-N). The result is stored in a <length>-size file.
The design intends to provide offset and length arguments to the xxx_read functions, so that each can read only <length> bytes from <offset>. Actually, for the READ part, there are still 2 or 3 functions to modify. If this design is agreed, I'll modify the others too.
The same kind of design could be used for the VERIFY (easy), ERASE and WRITE (more difficult) parts too.
Also in attachment for GMail users.
Any comment is welcome.
Stephan.
Signed-off-by: Stephan Guilloux mailto:stephan.guilloux@free.fr
Index: flashrom/flash.h =================================================================== --- flashrom/flash.h (révision 3789) +++ flashrom/flash.h (copie de travail) @@ -74,7 +74,7 @@ int (*probe) (struct flashchip *flash); int (*erase) (struct flashchip *flash); int (*write) (struct flashchip *flash, uint8_t *buf); - int (*read) (struct flashchip *flash, uint8_t *buf); + int (*read) (struct flashchip *flash, uint8_t *buf, uint32_t start, uint32_t size);
/* Some flash devices have an additional register space. */ volatile uint8_t *virtual_memory; @@ -464,7 +464,7 @@ int spi_block_erase_52(const struct flashchip *flash, unsigned long addr); int spi_block_erase_d8(const struct flashchip *flash, unsigned long addr); int spi_chip_write(struct flashchip *flash, uint8_t *buf); -int spi_chip_read(struct flashchip *flash, uint8_t *buf); +int spi_chip_read(struct flashchip *flash, uint8_t *buf, uint32_t start, uint32_t size); uint8_t spi_read_status_register(); int spi_disable_blockprotect(void); void spi_byte_program(int address, uint8_t byte); @@ -488,7 +488,7 @@ /* ichspi.c */ int ich_spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); -int ich_spi_read(struct flashchip *flash, uint8_t * buf); +int ich_spi_read(struct flashchip *flash, uint8_t * buf, uint32_t start, uint32_t size); int ich_spi_write(struct flashchip *flash, uint8_t * buf);
/* it87spi.c */ Index: flashrom/spi.c =================================================================== --- flashrom/spi.c (révision 3789) +++ flashrom/spi.c (copie de travail) @@ -544,21 +544,29 @@ return spi_command(sizeof(cmd), len, cmd, bytes); }
-int spi_chip_read(struct flashchip *flash, uint8_t *buf) +int spi_chip_read(struct flashchip *flash, uint8_t *buf, uint32_t offset, uint32_t length) { + unsigned int total_size = flash->total_size * 1024; + switch (flashbus) { case BUS_TYPE_IT87XX_SPI: + if ((offset != 0) || (length != total_size)) { + printf ("Error: offset/lenght options not supported on ITE 87xx SPI bus masters.\n"); + return (1); + } return it8716f_spi_chip_read(flash, buf); case BUS_TYPE_SB600_SPI: + if ((offset != 0) || (length != total_size)) { + printf ("Error: offset/lenght options not supported on SB 600 SPI bus masters.\n"); + return (1); + } return sb600_spi_read(flash, buf); case BUS_TYPE_ICH7_SPI: case BUS_TYPE_ICH9_SPI: case BUS_TYPE_VIA_SPI: - return ich_spi_read(flash, buf); + return ich_spi_read(flash, buf, offset, length); default: - printf_debug - ("%s called, but no SPI chipset/strapping detected\n", - __FUNCTION__); + printf_debug ("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__); }
return 1; Index: flashrom/flashrom.c =================================================================== --- flashrom/flashrom.c (révision 3789) +++ flashrom/flashrom.c (copie de travail) @@ -46,6 +46,8 @@ int verbose = 0; int fd_mem;
+#define UNKNOWN_LENGTH 0xffffffff + struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) { struct pci_dev *temp; @@ -171,15 +173,41 @@ return flash; }
+static int read_flash(struct flashchip *flash, uint8_t *buf, uint32_t offset, uint32_t length) +{ + int total_size = flash->total_size * 1024; + + if (offset >= total_size) { + printf ("Error: Invalid start address.\n"); + return (1); + } + + if ((length == 0) || (offset + length > total_size)) { + printf ("Error: Invalid size.\n"); + return (1); + } + + if (flash->read == NULL) { + memcpy(buf, (const char *)flash->virtual_memory + offset, length); + } else { + if (flash->read(flash, buf, offset, length)) { + printf ("ERROR : flash read failed\n"); + return (1); + } + } + return (0); +} + int verify_flash(struct flashchip *flash, uint8_t *buf) { int idx; int total_size = flash->total_size * 1024; uint8_t *buf2 = (uint8_t *) calloc(total_size, sizeof(char)); - if (flash->read == NULL) - memcpy(buf2, (const char *)flash->virtual_memory, total_size); - else - flash->read(flash, buf2); + + if (read_flash(flash, buf2, 0, total_size)) { + printf ("ERROR : verification aborted.\n"); + return (1); + }
printf("Verifying flash... ");
@@ -224,7 +252,8 @@ { printf("usage: %s [-rwvEVfLhR] [-c chipname] [-s exclude_start]\n", name); - printf(" [-e exclude_end] [-m [vendor:]part] [-l file.layout] [-i imagename] [file]\n"); + printf(" [-e exclude_end] [-m [vendor:]part] [-l file.layout] [-i imagename]\n"); + printf(" [-S offs] [-N length] [file]\n"); printf (" -r | --read: read flash and save into file\n" " -w | --write: write file into flash\n" @@ -241,6 +270,8 @@ " -L | --list-supported: print supported devices\n" " -h | --help: print this help text\n" " -R | --version: print the version (release)\n" + " -S | --offset <offs>: start address\n" + " -N | --length <length>: number of bytes to process (READ)\n" "\n" " If no file is specified, then all that happens" " is that flash info is dumped.\n\n"); exit(1); @@ -280,6 +311,8 @@ {"force", 0, 0, 'f'}, {"layout", 1, 0, 'l'}, {"image", 1, 0, 'i'}, + {"offset", 1, 0, 'S'}, + {"length", 1, 0, 'N'}, {"list-supported", 0, 0, 'L'}, {"help", 0, 0, 'h'}, {"version", 0, 0, 'R'}, @@ -288,6 +321,8 @@
char *filename = NULL;
+ unsigned int offset = 0; + unsigned int length = UNKNOWN_LENGTH; unsigned int exclude_start_position = 0, exclude_end_position = 0; // [x,y) char *tempstr = NULL, *tempstr2 = NULL;
@@ -329,6 +364,12 @@ tempstr = strdup(optarg); sscanf(tempstr, "%x", &exclude_end_position); break; + case 'S': + sscanf(optarg, "%x", &offset); + break; + case 'N': + sscanf(optarg, "%x", &length); + break; case 'm': tempstr = strdup(optarg); strtok(tempstr, ":"); @@ -463,18 +504,22 @@ perror(filename); exit(1); } + + if (length == UNKNOWN_LENGTH) + length = size - offset; + printf("Force reading flash... "); - if (!flashes[0]->read) - memcpy(buf, (const char *)flashes[0]->virtual_memory, size); - else - flashes[0]->read(flashes[0], buf); + if (read_flash(flashes[0], buf + offset, offset, length)) { + fclose(image); + exit (1); + }
if (exclude_end_position - exclude_start_position > 0) memset(buf + exclude_start_position, 0, exclude_end_position - exclude_start_position);
- fwrite(buf, sizeof(char), size, image); + fwrite(buf + offset, sizeof(char), length, image); fclose(image); printf("done.\n"); free(buf); @@ -546,21 +591,25 @@ printf("done.\n"); exit(0); } else if (read_it) { + printf("Reading flash... "); if ((image = fopen(filename, "w")) == NULL) { perror(filename); exit(1); } - printf("Reading flash... "); - if (flash->read == NULL) - memcpy(buf, (const char *)flash->virtual_memory, size); - else - flash->read(flash, buf);
+ if (length == UNKNOWN_LENGTH) + length = size - offset; + + if (read_flash(flash, buf + offset, offset, length)) { + fclose(image); + exit(1); + } + if (exclude_end_position - exclude_start_position > 0) memset(buf + exclude_start_position, 0, exclude_end_position - exclude_start_position);
- fwrite(buf, sizeof(char), size, image); + fwrite(buf + offset, sizeof(char), length, image); fclose(image); printf("done.\n"); } else { Index: flashrom/ichspi.c =================================================================== --- flashrom/ichspi.c (révision 3789) +++ flashrom/ichspi.c (copie de travail) @@ -545,18 +545,26 @@ return 0; }
-int ich_spi_read(struct flashchip *flash, uint8_t * buf) +int ich_spi_read(struct flashchip *flash, uint8_t * buf, uint32_t offset, uint32_t length) { int i, rc = 0; - int total_size = flash->total_size * 1024; int page_size = flash->page_size; int maxdata = 64; + int page_start; + int page_nbr;
+ if ((offset % page_size) || (length % page_size)) { + printf ("Error: offset/length must be page aligned (page=%d bytes).\n", page_size); + return (1); + } + page_start = offset / page_size; + page_nbr = length / page_size; + if (flashbus == BUS_TYPE_VIA_SPI) { maxdata = 16; }
- for (i = 0; (i < total_size / page_size) && (rc == 0); i++) { + for (i = page_start; (i < page_start + page_nbr) && (rc == 0); i++) { rc = ich_spi_read_page(flash, (void *)(buf + i * page_size), i * page_size, maxdata); }
On 03.12.2008 02:04, Stephan GUILLOUX wrote:
Second try for this patch, after some review/comments on #coreboot channel.
- renamed generic_read() to read_flash()
- renamed options chunk_start/size to offset/length
- fixed some indentation spaces
- fixed some error messages
- replaces chk_start/size by offset/length
- added a description in the usage()
The goal for this patch is to allow flashrom to read some part of the flash, described by a pair offset/length. Added flashrom arguments --offset (-S) and --length (-N). The result is stored in a <length>-size file.
The design intends to provide offset and length arguments to the xxx_read functions, so that each can read only <length> bytes from <offset>. Actually, for the READ part, there are still 2 or 3 functions to modify. If this design is agreed, I'll modify the others too.
The same kind of design could be used for the VERIFY (easy), ERASE and WRITE (more difficult) parts too.
Also in attachment for GMail users.
Any comment is welcome.
Stephan.
Signed-off-by: Stephan Guilloux mailto:stephan.guilloux@free.fr
Partial review follows.
+static int read_flash(struct flashchip *flash, uint8_t *buf, uint32_t offset, uint32_t length) +{
- int total_size = flash->total_size * 1024;
total_size should be unsigned.
- if (offset >= total_size) {
printf ("Error: Invalid start address.\n");return (1);- }
You have to check for length>= total_size as well.
- if ((length == 0) || (offset + length > total_size)) {
(offset + length > total_size) can overflow. This is not a problem as long as flash chips are smaller than 2 GB and the checks mentioned above are in place.
printf ("Error: Invalid size.\n");return (1);- }
- if (flash->read == NULL) {
memcpy(buf, (const char *)flash->virtual_memory + offset, length);- } else {
if (flash->read(flash, buf, offset, length)) {printf ("ERROR : flash read failed\n");return (1);}- }
- return (0);
+}
int verify_flash(struct flashchip *flash, uint8_t *buf) { int idx; int total_size = flash->total_size * 1024; uint8_t *buf2 = (uint8_t *) calloc(total_size, sizeof(char));
- if (flash->read == NULL)
memcpy(buf2, (const char *)flash->virtual_memory, total_size);- else
flash->read(flash, buf2);
- if (read_flash(flash, buf2, 0, total_size)) {
printf ("ERROR : verification aborted.\n");
"ERROR : verification aborted because flash could not be read.\n"
return (1);}
printf("Verifying flash... ");
I noticed you are using parentheses around return values. That's rather uncommon in flashrom. Please avoid them if possible.
Regards, Carl-Daniel
After partial review, the following fixes are included : - return used without parentheses. I prefer parentheses, as it allows macros around. For instance, I oftenly use macros like FUNCTION_RETURN(xx) do dump the return code at debug time. Not the case here, so, () removed. - unsigned int total_size. errr. yes ;-) - verification error message changed to Error: verification aborted because of flash read failure. - length > total_size length == total_size is allowed !.
Anything else ?
Stephan.
+static int read_flash(struct flashchip *flash, uint8_t *buf, uint32_t offset, uint32_t length) +{
- int total_size = flash->total_size * 1024;
total_size should be unsigned.
Agreed, but found some inconsistencies for total_size : - "size_t" is used, like in map_flash_registers() - "int" is used, like in verify_flash() Anyway, the best would be to use "size_t", not "unsigned int" or "unsigned long".
Stephan.
Stephan GUILLOUX wrote:
+static int read_flash(struct flashchip *flash, uint8_t *buf, uint32_t offset, uint32_t length) +{
- int total_size = flash->total_size * 1024;
total_size should be unsigned.
Agreed, but found some inconsistencies for total_size :
I disagree, this does not matter at all right now, and will not for some time to come. Worry about it later.
//Peter
-
Carl-Daniel Hailfinger -
Peter Stuge -
Stephan GUILLOUX