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

vpd.c

Index: vpd.c
diff --git a/vpd.c b/vpd.c
index 45fbc1d66d319feb170e26c6ff25190a28961725..5e843d75abb37d6b4f21499c34a35c906e1a9bb6 100644
--- a/vpd.c
+++ b/vpd.c
@@ -34,7 +34,12 @@
 /* The buffer length. Right now the VPD partition size on flash is 128KB. */
 #define BUF_LEN (128 * 1024)
 
-static uint8_t flashrom_tmp_file[] = "/tmp/vpd.flashrom.XXXXXX";
+/* temp filename for partial read area. */
+static uint8_t tmp_part_file[] = "/tmp/vpd.flashrom.XXXXXX";
+/* temp filename for the full flash content (actually only partial content
+ * is available). But for flashrom -w to match flash size, we need to keep this
+ * file from flashromRead() to writeFlashrom(). */
+static uint8_t tmp_full_file[] = "/tmp/vpd.flashrom.XXXXXX";

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

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.

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

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:

 
 /* 2 containers:
  *   file:      stores decoded pairs from file.
@@ -61,9 +66,11 @@ int max_buf_len = sizeof(buf);
  * User can overwrite this by -E argument.
  */
 uint32_t eps_base = GOOGLE_EPS_BASE;
+int eps_base_force_specified = 0;  /* a bool value to indicate if -E argument
+                                    * is given. */
 
 /* the fmap name of VPD. */
-uint8_t fmap_vpd_area_name[FMAP_STRLEN] = "RO VPD";
+uint8_t fmap_vpd_area_name[FMAP_STRLEN] = "RO_VPD";
 
 /* If found_vpd, replace the VPD partition when saveFile().
  * If not found, always create new file when saveFlie(). */
@@ -257,7 +264,17 @@ int loadFile(const char *filename, struct PairContainer *container,
       retval = 0;
       goto teardown;
     }
+
+    if (!eps_base_force_specified) {
+      /* Since we lost the fmap info in this mode, we try to guess the
+       * eps_base from EPS field. */
+      if (eps->table_address > file_size) {
+        eps_base = eps->table_address - sizeof(*eps);
+        fprintf(stderr, "[INFO] we guess eps_base is 0x%x.\n", eps_base);
+      }
+    }
   } else {
+    /* FMAP signature is found, try to search the partition name in table. */
     int i;
 
     fmap = (struct fmap *)&read_buf[sig_offset];
@@ -292,7 +309,11 @@ int loadFile(const char *filename, struct PairContainer *container,
   /* jump if the VPD partition is not recognized. */
   if (memcmp(VPD_ENTRY_MAGIC, eps, sizeof(VPD_ENTRY_MAGIC) - 1)) {
     /* But OKAY if the VPD partition is all-FF, which is un-used. */
-    if (memcmp("\xff\xff\xff\xff", eps, sizeof(VPD_ENTRY_MAGIC) - 1)) {
+    if (!memcmp("\xff\xff\xff\xff", eps, sizeof(VPD_ENTRY_MAGIC) - 1)) {
+      fprintf(stderr, "[WARN] VPD partition not formatted. Ignored.\n");
+      retval = 0;
+      goto teardown;
+    } else {

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

nice!

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

Done.

       fprintf(stderr, "SMBIOS signature is not matched.\n");
       fprintf(stderr, "You may use -O to overwrite the data.\n");
       retval = 1;
@@ -315,7 +336,8 @@ int loadFile(const char *filename, struct PairContainer *container,
   /* Now header points to the first SMBIOS entry, and data points to the
    * first BBP entry. The first entry could be SPD data. We don't care. */
   if (header->handle != expected_handle) {
-    fprintf(stderr, "The first handle value must be 0, but is %d.\n",
+    fprintf(stderr, "[ERROR] The first handle value must be 0, but is %d.\n"
+                    "        Use -O option to re-format.\n",
                     header->handle);
     retval = 1;
     goto teardown;
@@ -362,7 +384,8 @@ int loadFile(const char *filename, struct PairContainer *container,
 
   /* The 2nd could be VPD 2.0 data or End Of Table. */
   if (header->handle != expected_handle) {
-    fprintf(stderr, "The second handle value must be 1, but is %d.\n",
+    fprintf(stderr, "[ERROR] The second handle value must be 1, but is %d.\n"
+                    "        Use -O option to re-format.\n",
                     header->handle);
     retval = 1;
     goto teardown;
@@ -490,6 +513,20 @@ teardown:
   return retval;
 }
 
+int myMkTemp(uint8_t *tmp_file) {
+  int fd;
+  fd = mkstemp(tmp_file);
+  if (fd < 0) {
+    fprintf(stderr, "mkstemp(%s) failed\n", tmp_file);
+  }
+  return fd;
+}
+
+int generateTempFilenames(void) {
+  if (myMkTemp(tmp_part_file) < 0) return -1;
+  if (myMkTemp(tmp_full_file) < 0) return -1;
+  return 0;
+}
 
 static void usage(const char *progname) {
   printf("Chrome OS VPD 2.0 utility --\n");
@@ -551,6 +588,7 @@ int main(int argc, char *argv[]) {
       case 'E':
         errno = 0;
         eps_base = strtoul(optarg, (char **) NULL, 0);
+        eps_base_force_specified = 1;
 
         /* FIXME: this is not a stable way to detect error because
          *        implementation may (or may not) assign errno. */
@@ -605,9 +643,14 @@ int main(int argc, char *argv[]) {
     }
   }
 
-  fd = mkstemp(flashrom_tmp_file);
-  if (fd < 0) {
-    fprintf(stderr, "mkstemp(%s) failed\n", flashrom_tmp_file);
+  if (optind < argc) {
+    fprintf(stderr, "[ERROR] unexpected argument: %s\n\n", argv[optind]);
+    usage(argv[0]);
+    retval = 1;
+    goto teardown;
+  }
+
+  if (generateTempFilenames() < 0) {
     retval = 1;
     goto teardown;
   }
@@ -617,13 +660,14 @@ int main(int argc, char *argv[]) {
 
   /* if no filename is specified, call flashrom to read from flash. */
   if (!filename) {
-    if (FLASHROM_OK != flashromRead(flashrom_tmp_file)) {
+    if (FLASHROM_OK != flashromRead(tmp_part_file, tmp_full_file,
+                                    fmap_vpd_area_name)) {
       fprintf(stderr, "flashromRead() error!\n");
       retval = 1;
       goto teardown;
     }
     write_back_to_flash = 1;
-    filename = strdup(flashrom_tmp_file);
+    filename = strdup(tmp_part_file);
   }
 
   if (retval = loadFile(filename, &file, overwrite_it)) {
@@ -655,7 +699,8 @@ int main(int argc, char *argv[]) {
     }
 
     if (write_back_to_flash) {
-      if (FLASHROM_OK != flashromPartialWrite(filename, vpd_offset, vpd_size)) {
+      if (FLASHROM_OK != flashromPartialWrite(filename, tmp_full_file,
+                                              fmap_vpd_area_name)) {
         fprintf(stderr, "flashromPartialWrite() error.\n");
         retval = 1;
         goto teardown;