Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41732 )
Change subject: drivers/vpd: add VPD region VPD_RW_OR_RO ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41732/1/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41732/1/src/drivers/vpd/vpd.c@150 PS1, Line 150: struct vpd_blob blob = {0}; : : vpd_get_buffers(&blob); : if (blob.ro_size == 0 && blob.rw_size == 0) : return NULL; : : struct vpd_gets_arg arg_ro = {0}; : arg_ro.key = (const uint8_t *)key; : arg_ro.key_len = strlen(key); : uint32_t consumed = 0; : : /* Search VPD_RO for the key, if the region is not set to VPD_RW */ : if (blob.ro_size != 0 && region != VPD_RW) { : while (vpd_decode_string(blob.ro_size, blob.ro_base, : &consumed, vpd_gets_callback, &arg_ro) == VPD_DECODE_OK) { : /* Iterate until found or no more entries. */ : } : /* If the region is set to VPD_RO, the search result is returned */ : if (region == VPD_RO) { : if (!arg_ro.matched) : return NULL; : *size = arg_ro.value_len; : return arg_ro.value; : } : } : : struct vpd_gets_arg arg_rw = {0}; : arg_rw.key = (const uint8_t *)key; : arg_rw.key_len = strlen(key); : consumed = 0; : : /* Search VPD_RW for the key */ : if (blob.rw_size != 0) { : while (vpd_decode_string(blob.rw_size, blob.rw_base, : &consumed, vpd_gets_callback, &arg_rw) == VPD_DECODE_OK) { : /* Iterate until found or no more entries. */ : } : /* If the region is set to VPD_RW, the search result is returned */ : if (region == VPD_RW) { : if (!arg_rw.matched) : return NULL; : *size = arg_rw.value_len; : return arg_rw.value; : } : } : : /* : * If the region is set to VPD_RO_OR_RW, use the search result from VPD_RO : * as the preference, otherwise use the search result from VPD_RW. : */ : if (region == VPD_RO_OR_RW) { : if (arg_ro.matched) { : *size = arg_ro.value_len; : return arg_ro.value; : } else if (arg_rw.matched) { : *size = arg_rw.value_len; : return arg_rw.value; : } else { : return NULL; : } : } : : /* : * As the region is set to VPD_RW_OR_RO, use the search result from VPD_RW : * as the preference, otherwise use the search result from VPD_RO. : */ : assert(region == VPD_RW_OR_RO); : if (arg_rw.matched) { : *size = arg_rw.value_len; : return arg_rw.value; : } else if (arg_ro.matched) { : *size = arg_ro.value_len; : return arg_ro.value; : } else { : return NULL; : } This works, but it'll also waste time searching RO if there's already data in RW, and duplicated the logic.
I think we can have a more straightforward implementation:
static const void *vpd_find_in_buffer(const char *key, int *size, void *blob, size_t blob_size) { uint_32 consumed = 0; struct vpd_gets_arg arg = {0};
if (!blob_size) return NULL;
arg.key = (const uint8_t *)key; arg.key_len = strlen(key); while (vpd_decode_string(blob_size, blob, &consumed, vpd_gets_callback, &arg) == VPD_DECODE_OK) { /* Iterate until found or no more entries. */ }
if (!arg.matched) return NULL; *size = arg.value_len; return arg.value; }
void *vpd_find(const char *key, int *size, enum vpd_region region) { struct vpd_blob blob = {0}; void *ret = NULL;
vpd_get_buffers(&blob);
if (region == VPD_RO || region == VPD_RO_OR_RW) ret = vpd_find_in_buffer(key, size, &blob.ro_base, blob.ro_size); if (!ret && region != VPD_RO) ret = vpd_find_in_buffer(key, size, &blob.rw_base, blob.rw_size); if (!ret && region == VPD_RW_RO) ret = vpd_find_in_buffer(key, size, &blob.ro_base, blob.ro_size);
return ret; }