[L] Change in coreboot[master]: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI
Attention is currently required from: Maximilian Brune, Angel Pons, Arthur Heymans, Lean Sheng Tan.
David Milosevic has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68137 )
Change subject: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI ......................................................................
Patch Set 11:
(5 comments)
File src/mainboard/prodrive/atlas/emi.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/741de193_1ece74e3 PS11, Line 59: u8 bytes[4] = { 0 };
use u32.
Using a byte-literal here makes more sense. I need to read single bytes (in case n mod 4 != 0) and store them one by one into the user buffer. If I would use a u32 here, I would also need to shift the bytes out of it.
https://review.coreboot.org/c/coreboot/+/68137/comment/5f0caadc_eedcd833 PS11, Line 71: buff[idx++] = bytes[i];
Use memcpy
I need to store the bytes one by one and stop as soon as the desired reading length is reached, since EMI only allows 4-byte aligned read access. Using memcpy for just one byte does not make much sense.
https://review.coreboot.org/c/coreboot/+/68137/comment/ec5ff8ce_5f3a16ea PS11, Line 68: if (idx == n) : return; : : buff[idx++]
Can you put those conditions in the for loop? That makes it easier to read.
I could do that but then we would need the same branch right after the for-loop in order to determine which condition led to exiting the loop (i greater/equals to 4 or idx equals to n). This would only duplicate the checks.
Your proposal would look like this
for(u8 i = 0; i < 4 && idx != n; i++) buff[idx++] = bytes[i]; // same check if(idx == n) return;
File src/mainboard/prodrive/atlas/vpd.h:
https://review.coreboot.org/c/coreboot/+/68137/comment/ca085be6_14bde4fa PS11, Line 13: __packed
__packed is a NOOP needed here.
Yep, I can remove that.
File src/mainboard/prodrive/atlas/vpd.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/97516e15_1e1cf01b PS11, Line 36: PN_LENGTH
Do you need this in the read_region code? reading 4byte aligned data and dealing with it later is pr […]
This is not the read_region code. vpd_get() uses emi_read_region() to read PN_LENGTH bytes from the EMI region since emi_read_region() is designed to work with arbitrary lengths and not only a multiple of 4-bytes. vpd.c in general is responsible for returning you a non-null terminated byte sequence that represents your desired VPD.
-
David Milosevic (Code Review)