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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Dec 3 02:52:18 CET 2008


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 at 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

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list