Re: [coreboot] [PATCH] flashrom patch for partial flash read #2

[Carl-Daniel Hailfinger]

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

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