Attention is currently required from: Lean Sheng Tan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73937 )
Change subject: mb/prodrive/atlas: Implement initial VPD support ......................................................................
Patch Set 4:
(7 comments)
Patchset:
PS4:
Sry for late review, but your patch was merged so quickly that I didn't see it.
Sheng did wait 24 hours as per Gerrit guidelines, but yes this sometimes happens.
File src/mainboard/prodrive/atlas/mainboard.c:
https://review.coreboot.org/c/coreboot/+/73937/comment/9f35e9ee_895cec2b PS4, Line 58: 32
why 32?
This is the space for the prefix. The value comes from Hermes code, where the prefix is passed as a function parameter. It doesn't matter if the buffer is larger than needed, it just needs to be large enough.
File src/mainboard/prodrive/atlas/vpd.h:
https://review.coreboot.org/c/coreboot/+/73937/comment/47e6a312_038df368 PS4, Line 26: uint8_t _rfu[15];
Is there a reason for the number 15? […]
The reason to have reserved bytes is to allow adding new fields to the header without having to move the "body" fields (serial number, part number, profile). This space could be used to store a checksum, for instance.
The reason to have 15 reserved bytes is rather arbitrary, actually. In a nutshell, it's a value that ensures that the header length is a multiple of 4. These RFU bytes have nothing to do with the size of the entire struct.
Also, note that the EC firmware currently reads and writes the EEPROM byte by byte. It might not be the most performant approach, but it is certainly the most straightforward one (no need to worry about endianness issues).
https://review.coreboot.org/c/coreboot/+/73937/comment/ad7c4b09_b6098780 PS4, Line 53: uint8_t raw[EMI_EEPROM_LAYOUT_LENGTH];
Do we need this? can't we just cast the emi_eeprom_vpd to a uint8_t pointer if we need to memcpy stu […]
Actually, this is discussed in the very Wikipedia page we looked at together yesterday: https://en.wikipedia.org/wiki/Fast_inverse_square_root#Avoiding_undefined_be...
The problem with casts is that they tell the compiler "I know what I'm doing", so it's possible to make a huge mess. It's possible to avoid having to manually compute the struct length:
``` struct __packed emi_eeprom_vpd_layout { struct emi_eeprom_vpd_header header; char serial_number[ATLAS_SN_PN_LENGTH]; /* xx-xx-xxx-xxx */ char part_number[ATLAS_SN_PN_LENGTH]; /* xxx-xxxx-xxxx.Rxx */ uint16_t profile; };
union emi_eeprom_vpd { struct emi_eeprom_vpd_layout layout; uint8_t raw[sizeof(struct emi_eeprom_vpd_layout)]; }; ```
The biggest disadvantage of this approach is that accesses to the fields requires more indirection:
``` get_emi_eeprom_vpd()->layout.serial_number ```
Hmmm, assuming that the callers of `get_emi_eeprom_vpd()` won't need access to the raw bytes, maybe something can be done.
File src/mainboard/prodrive/atlas/vpd.c:
https://review.coreboot.org/c/coreboot/+/73937/comment/976233a4_d3e7f807 PS4, Line 12: : static union emi_eeprom_vpd vpd = {0};
Putting the variable in this scope implies that the outside of this function has no access to it. […]
The reason why this isn't global is that the function only returns read-only pointers to it, and only after it has been initialised. Also, note that this pattern already exists in the tree.
https://review.coreboot.org/c/coreboot/+/73937/comment/3ccff000_db18b856 PS4, Line 29: /* : * For backwards compatibility, if the VPD from the EC is an older : * version, uprev it to the latest version coreboot knows about by : * filling in the remaining fields with default values. Should the : * EC provide a newer VPD revision, coreboot would downgrade it to : * the latest version it knows about as the VPD layout is designed : * to be backwards compatible. : * : * Whenever the value of `VPD_LATEST_REVISION` is incremented, add : * a new `case` label just before the `default` label that matches : * the second latest revision to initialise the newly-added fields : * of the VPD structure with a reasonable fallback value. Note the : * intentional falling through. : */
For me it makes more sense to just return the VPD struct if deemed valid and otherwise return NULL. […]
The problem with returning NULL is that code still needs to deal with having no data from the EEPROM, and the fallback values end up scattered across multiple files. Hermes code used to do this, and it was a nightmare to deal with.
Not sure how returning NULL pointers makes things simpler and smaller. It moves the complexity to the caller side, which currently looks like this:
``` get_emi_eeprom_vpd()->serial_number ```
No checks are needed on the caller side because `get_emi_eeprom_vpd()` always returns a valid pointer.
Also, note that this implementation is designed to handle the case where new EEPROM fields get added (and `VPD_LATEST_REVISION` gets incremented), but the actual EEPROM data hasn't been updated (e.g. already-deployed boards). This code would still use the available values in the EEPROM and only use fallback values for the newer fields not in the EEPROM.
https://review.coreboot.org/c/coreboot/+/73937/comment/c9dbb9ab_03df194e PS4, Line 47: vpd.serial_number[0] = '\0'; : vpd.part_number[0] = '\0'; : vpd.profile = ATLAS_PROF_UNPROGRAMMED;
isn't this reduntant because of the memset above?
It is redundant, yes, but this explicitly shows what the fallback values for these fields are. The idea is that all the fallback values appear in this switch, rather than having implicit zeroes.
Then again, this is only executed if the EEPROM contents are invalid. Micro-optimising this for performance doesn't make sense.
-
Angel Pons (Code Review)