Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41732 )
Change subject: drivers/vpd: add VPD region VPD_RW_THEN_RO ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41732/2/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/41732/2/src/drivers/vpd/vpd.h@11 PS2, Line 11: VPD_RO = 0,
Generally, enums should only use an explict = 0, = 1, etc. […]
Done
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 […]
Done
https://review.coreboot.org/c/coreboot/+/41732/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41732/3/src/drivers/vpd/vpd.c@215 PS3, Line 215: region != VPD_RW
changed my mind - give we have so many cases now, let's do an explicit list here so people will be e […]
Done