leverage flashrom fast partial read function to speed up VPD read.

leverage flashrom fast partial read function to speed up VPD read.

Dmitry, this is FYR. After this is committed, the VPD read can be much faster for OOBE to read default locale setting. Hi, David, please kindly review my code. Thanks.

This patch basically re-wrote the flashromRead() and  flashromWrite().
In the old code, to handle image on BIOS, we always read the whole flash
chip content and then parse fmap. Since David's CL
(http://codereview.chromium.org/6025013/) makes flashrom to parse fmap,
we can just call flashrom partial read to get VPD partition.
This reduce the VPD read from 5 secs to less than 1 sec.

fmapNormalizeAreaName() is also refined because we no longer generate
layout file for flashrom. Hence, user must specify the exact partition
name on BIOS fmap, and unfortunately Mario BIOS' partition name has space.

Some comments are also refined.

Another minor improvement is that VPD utility now guesses the EPS base address
by looking at eps->table_address. This is more user-friendly when handling the
small VPD partition slice file: no longer specify -E argument. But still can
use -E argument to overwrite it (with -O option).

Change-Id: I7caa215ff775da78b7844586d45e5e1ceae3c7a9

BUG=chromium-os:12355
TEST=Tested on Z600 and Mario machine.
On Z600, I manually run the below commands:
  1. a standalone VPD.bin file
  2. Mario BIOS with VPD 1.0 partition
  3. Mario BIOS with blank VPD partition
  4. old version of VPD 2.0 partition (located at 0x100)
  5. new BIOS with SPD data present (at 0x400) and VPD 2.0 (at 0x600)
On Mario machine,
  % vpd -i "RO VPD" -O
  % vpd -i "RO VPD" -s A=B

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=1de3ef2

[dhendrix, 2011-03-10 01:33:51]

LGTM, with minor comments.

http://codereview.chromium.org/6646016/diff/1/lib/fmap.c
File lib/fmap.c (right):

http://codereview.chromium.org/6646016/diff/1/lib/fmap.c#newcode55
lib/fmap.c:55: if (!(isascii(*name) && (isalnum(*name) || *name == ' '))) {
Is isascii() required if isalnum() used?

http://codereview.chromium.org/6646016/diff/1/vpd.c
File vpd.c (right):

http://codereview.chromium.org/6646016/diff/1/vpd.c#newcode42
vpd.c:42: static uint8_t tmp_full_file[] = "/tmp/vpd.flashrom.XXXXXX";
Just a thought (not worth the effort for this CL):

As i looked over this file, it seemed there is a lot being done with these
temporary filenames. One example is where the temporary file is made, then the
filename is strdup'd and later passed into another function.

. I am curious if it would simplify the code if users such as loadFile(),
flashromRead(), and flashromPartialWrite() were modified to use file descriptors
instead of file names. That way you could replace these uint8_t arrays with:
#define template    "/tmp/vpd.flashrom.XXXXXX"

then call mkstemp() directly (without myMkTemp).

Anyway, just a thought. The current code should work fine.

http://codereview.chromium.org/6646016/diff/1/vpd.c#newcode316
vpd.c:316: } else {
nice!

[Louis, 2011-03-10 08:22:51]

David, thanks for reviews. I am going to submit after testing.

http://codereview.chromium.org/6646016/diff/1/lib/fmap.c
File lib/fmap.c (right):

http://codereview.chromium.org/6646016/diff/1/lib/fmap.c#newcode55
lib/fmap.c:55: if (!(isascii(*name) && (isalnum(*name) || *name == ' '))) {
I think isalnum() is locale-dependent implementation. According to the manpage,
there may be additional characters in some locales.


       isalnum()
              checks for an alphanumeric character; it is equivalent to (isal‐
              pha(c) || isdigit(c)).

       isalpha()
              checks for an alphabetic character; in the standard "C"  locale,
              it  is  equivalent  to  (isupper(c)  ||  islower(c)).   In  some
              locales, there may be additional characters for which  isalpha()
              is true—letters which are neither upper case nor lower case.




On 2011/03/10 01:33:51, dhendrix wrote:
> Is isascii() required if isalnum() used?

http://codereview.chromium.org/6646016/diff/1/vpd.c
File vpd.c (right):

http://codereview.chromium.org/6646016/diff/1/vpd.c#newcode42
vpd.c:42: static uint8_t tmp_full_file[] = "/tmp/vpd.flashrom.XXXXXX";
mkstemp() is a good idea. I will modify that next time I get a chance.

For file descriptor, I am not sure good or bad. Will think more later.

On 2011/03/10 01:33:51, dhendrix wrote:
> Just a thought (not worth the effort for this CL):
> 
> As i looked over this file, it seemed there is a lot being done with these
> temporary filenames. One example is where the temporary file is made, then the
> filename is strdup'd and later passed into another function.
> 
> . I am curious if it would simplify the code if users such as loadFile(),
> flashromRead(), and flashromPartialWrite() were modified to use file
descriptors
> instead of file names. That way you could replace these uint8_t arrays with:
> #define template    "/tmp/vpd.flashrom.XXXXXX"
> 
> then call mkstemp() directly (without myMkTemp).
> 
> Anyway, just a thought. The current code should work fine.

http://codereview.chromium.org/6646016/diff/1/vpd.c#newcode316
vpd.c:316: } else {
On 2011/03/10 01:33:51, dhendrix wrote:
> nice!

Done.