Attention is currently required from: Cliff Huang, Furquan Shaikh, Martin Roth, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55503 )
Change subject: util/cse_serger: Add a new tool for stitching CSE components ......................................................................
Patch Set 22:
(11 comments)
File util/cbfstool/bpdt_formats/bpdt_1_7.c:
https://review.coreboot.org/c/coreboot/+/55503/comment/c5432680_5147c759 PS22, Line 210: l->checksum = calculate_layout_checksum(l); nit: both this line and `calculate_layout_checksum` set l->checksum
https://review.coreboot.org/c/coreboot/+/55503/comment/c9680cf4_ec43d801 PS22, Line 235: printf("BP4 size: 0x%x\n", l->bp4_size); remove this line (extra copy of line 237)
https://review.coreboot.org/c/coreboot/+/55503/comment/b346f316_a111f22b PS22, Line 317: crc32(0xffff so the layout checksum is inverted but the header checksum is not?
File util/cbfstool/bpdt_formats/subpart_entry_1.c:
https://review.coreboot.org/c/coreboot/+/55503/comment/5e50eb17_b23c127f PS22, Line 49: printf("%-25s%-25s%-25s%-25s%-25s%-25s\n", "Entry #", "Name", "Offset", "Huffman Compressed?", : "Length", "Rsvd"); nit: the line is already over 96, either make it 1 line or break before 96
File util/cbfstool/cse_serger.h:
https://review.coreboot.org/c/coreboot/+/55503/comment/a109d88e_829b1975 PS21, Line 15: nit: extra blank line
https://review.coreboot.org/c/coreboot/+/55503/comment/8bddb2b3_0b15bff9 PS21, Line 17: write_member(_buff, &(_x), sizeof(_x)) nit: align with read_member above
File util/cbfstool/cse_serger.c:
https://review.coreboot.org/c/coreboot/+/55503/comment/b6b1d570_45907a57 PS22, Line 79: ifwi nit: I think the struct name is superfluous
https://review.coreboot.org/c/coreboot/+/55503/comment/cc7bce3f_e2241114 PS22, Line 99: subpart_info also superfluous?
https://review.coreboot.org/c/coreboot/+/55503/comment/8f4dffce_4c889865 PS22, Line 167: : void write_member(struct buffer *buff, void *src, size_t size) : { : void *dst = buffer_get(buff); : : switch (size) { : case 1: : write_le8(dst, *(uint8_t *)src); : break; : case 2: : write_le16(dst, *(uint16_t *)src); : break; : case 4: : write_le32(dst, *(uint32_t *)src); : break; : case 8: : write_le64(dst, *(uint64_t *)src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } : : void read_member(struct buffer *buff, void *dst, size_t size) : { : const void *src = buffer_get(buff); : : switch (size) { : case 1: : *(uint8_t *)dst = read_le8(src); : break; : case 2: : *(uint16_t *)dst = read_le16(src); : break; : case 4: : *(uint32_t *)dst = read_le32(src); : break; : case 8: : *(uint64_t *)dst = read_le64(src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } is it worth it to break these out into a separate file for cse_fpt and cse_serger to share? it's not very much code, so maybe not.
https://review.coreboot.org/c/coreboot/+/55503/comment/f254926a_18242439 PS22, Line 304: int size_t
https://review.coreboot.org/c/coreboot/+/55503/comment/74531de7_510798fc PS22, Line 857: if (c >= LONGOPT_START && c < LONGOPT_END) : return true; : : return false; nit: `return c >= LONGOPT_START && c < LONGOPT_END;`