VbNvStorage cleanup and comments

firmware/lib/vboot_nvstorage.c

 /* Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 /* Non-volatile storage routines.
  */
 
 #include "utility.h"
 #include "vboot_common.h"
@@ skipping 43 matching lines @@
 
   /* Nothing has changed yet. */
   context->raw_changed = 0;
   context->regenerate_crc = 0;
 
   /* Check data for consistency */
   if ((HEADER_SIGNATURE != (raw[HEADER_OFFSET] & HEADER_MASK))
       || (Crc8(raw, CRC_OFFSET) != raw[CRC_OFFSET])) {
 
     /* Data is inconsistent (bad CRC or header), so reset defaults */
-    Memset(raw, 0, NV_BLOCK_SIZE);
+    Memset(raw, 0, VBNV_BLOCK_SIZE);
     raw[HEADER_OFFSET] = (HEADER_SIGNATURE | HEADER_FIRMWARE_SETTINGS_RESET |
                           HEADER_KERNEL_SETTINGS_RESET);
 
     /* Regenerate CRC on exit */
     context->regenerate_crc = 1;
   }
 
   return 0;
 }
 
@@ skipping 37 matching lines @@
     case VBNV_LOCALIZATION_INDEX:
       *dest = raw[LOCALIZATION_OFFSET];
       return 0;
 
     case VBNV_KERNEL_FIELD:
       *dest = (raw[KERNEL_FIELD_OFFSET]
                | (raw[KERNEL_FIELD_OFFSET + 1] << 8)
                | (raw[KERNEL_FIELD_OFFSET + 2] << 16)
                | (raw[KERNEL_FIELD_OFFSET + 3] << 24));
       return 0;
 

[Luigi Semenzato, 2011/02/18 23:51:50]

Idle suggestion: since the header file mentions that it's illegal to call
get/set outside of setup/teardown, would it be worthwhile enforcing it?  It may
be too trivial---you can judge better if it would serve any purpose.

     default:
       return 1;
   }
 }
 
 
 int VbNvSet(VbNvContext* context, VbNvParam param, uint32_t value) {
   uint8_t* raw = context->raw;
   uint32_t current;
 
@@ skipping 13 matching lines @@
       if (value)
         raw[HEADER_OFFSET] |= HEADER_KERNEL_SETTINGS_RESET;
       else
         raw[HEADER_OFFSET] &= ~HEADER_KERNEL_SETTINGS_RESET;
       break;
 
     case VBNV_DEBUG_RESET_MODE:
       if (value)
         raw[BOOT_OFFSET] |= BOOT_DEBUG_RESET_MODE;
       else
         raw[BOOT_OFFSET] &= BOOT_DEBUG_RESET_MODE;

[Luigi Semenzato, 2011/02/18 23:51:50]

Missing negation?

       break;
 
     case VBNV_TRY_B_COUNT:
       raw[BOOT_OFFSET] &= ~BOOT_TRY_B_COUNT;
       raw[BOOT_OFFSET] |= (uint8_t)(value & BOOT_TRY_B_COUNT);

[Luigi Semenzato, 2011/02/18 23:51:50]

Is it worth protecting from illegal values of VALUE?
Should you apply a shift to VALUE here instead of the caller?
(If any.)

       break;
 
     case VBNV_RECOVERY_REQUEST:
       raw[RECOVERY_OFFSET] = (uint8_t)value;
       break;
 
     case VBNV_LOCALIZATION_INDEX:
       raw[LOCALIZATION_OFFSET] = (uint8_t)value;
       break;
 
     case VBNV_KERNEL_FIELD:
       raw[KERNEL_FIELD_OFFSET] = (uint8_t)(value);
       raw[KERNEL_FIELD_OFFSET + 1] = (uint8_t)(value >> 8);
       raw[KERNEL_FIELD_OFFSET + 2] = (uint8_t)(value >> 16);
       raw[KERNEL_FIELD_OFFSET + 3] = (uint8_t)(value >> 24);
       break;
 
     default:
       return 1;
   }
 
   /* Need to regenerate CRC, since the value changed. */
   context->regenerate_crc = 1;
   return 0;
 }