Attention is currently required from: Iru Cai. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41393 )
Change subject: util: add a tool to dump and insert MEC1322 firmware ......................................................................
Patch Set 6:
(22 comments)
File util/mec1322/mec1322.c:
https://review.coreboot.org/c/coreboot/+/41393/comment/61f1b6b3_d578e5fd PS6, Line 16: void usage(void) All functions except `main()` should be declared as `static`.
https://review.coreboot.org/c/coreboot/+/41393/comment/243349b8_f4c0d8e0 PS6, Line 26: SPI_CLOCK_LIST we only use uppercase names for macros
https://review.coreboot.org/c/coreboot/+/41393/comment/8e56d8b4_3d8e4d2a PS6, Line 26: int `unsigned int` ?
https://review.coreboot.org/c/coreboot/+/41393/comment/7c2536fc_45251e69 PS6, Line 26: const int SPI_CLOCK_LIST[] = {48, 24, 12, 8}; : const uint8_t SPI_READ_CMD_LIST[] = {0x3, 0xb, 0x3b}; These two arrays are only used in one function. Why not declare them as local variables?
https://review.coreboot.org/c/coreboot/+/41393/comment/4716fa7b_f0b3059d PS6, Line 29: // FIXME: uint32_t cannot be used directly on big endian systems See CB:55039 for a possible solution. Also, same problem applies to `uint16_t`
https://review.coreboot.org/c/coreboot/+/41393/comment/54a2b306_9c729b16 PS6, Line 30: struct mec1322_header { : uint8_t sig[6]; // "CSMS\0\0" : uint8_t spi_clk; : uint8_t spi_read_cmd; : uint32_t load_addr; : uint32_t entry_point; : uint16_t payload_blks; // payload_len >> 6 : uint8_t padding0[2]; : uint32_t payload_offset; : uint8_t padding1[8]; : uint8_t payload_key_exp[16]; // @0x20 : uint8_t payload_key_modulus[SIGNATURE_LEN]; // @0x30 : }; Shouldn't this struct be __packed?
https://review.coreboot.org/c/coreboot/+/41393/comment/4c612f2f_e5dc02a4 PS6, Line 64: memcmp Why not use `strncmp()` instead?
https://review.coreboot.org/c/coreboot/+/41393/comment/6bf9b9da_5541f575 PS6, Line 73: (size_t) This cast shouldn't be necessary
https://review.coreboot.org/c/coreboot/+/41393/comment/341dbc20_3cc98b2a PS6, Line 74: %lx Please use `%zx` to print `size_t` values in hex for portability reasons.
https://review.coreboot.org/c/coreboot/+/41393/comment/d1c63248_26e619db PS6, Line 69: printf("SPI clock = %d MHz\n", SPI_CLOCK_LIST[hdr->spi_clk]); : printf("SPI read cmd = 0x%hhx\n", SPI_READ_CMD_LIST[hdr->spi_read_cmd]); : printf("load address = 0x%x\n", hdr->load_addr); : printf("entry point = 0x%x\n", hdr->entry_point); : size_t payload_len = (size_t)hdr->payload_blks * 64; : printf("payload length = 0x%lx\n", payload_len); : printf("payload offset = 0x%x\n", hdr->payload_offset); nit: align equals signs on printed messages for readability? I'd also move the `payload_len` assignment at the top:
size_t payload_len = (size_t)hdr->payload_blks * 64; printf("SPI clock = %d MHz\n", SPI_CLOCK_LIST[hdr->spi_clk]); printf("SPI read cmd = 0x%hhx\n", SPI_READ_CMD_LIST[hdr->spi_read_cmd]); printf("load address = 0x%x\n", hdr->load_addr); printf("entry point = 0x%x\n", hdr->entry_point); printf("payload length = 0x%zx\n", payload_len); printf("payload offset = 0x%x\n", hdr->payload_offset);
https://review.coreboot.org/c/coreboot/+/41393/comment/f2587301_fee2216c PS6, Line 94: (size_t)(hdr->payload_blks) * 64 Cast shouldn't be needed here.
https://review.coreboot.org/c/coreboot/+/41393/comment/0637cd93_7cd3feb6 PS6, Line 118: spisize = strtol(argv[1], NULL, 0); What if `spisize` is negative?
https://review.coreboot.org/c/coreboot/+/41393/comment/02689ead_3966f3ed PS6, Line 126: fprintf(stderr, "Fail to open file %s!\n", argv[1]); Does the errno get printed? If it doesn't, I'd print it using `strerror()` or similar.
https://review.coreboot.org/c/coreboot/+/41393/comment/e8b37a20_f465d869 PS6, Line 126: Fail Fail*ed*
https://review.coreboot.org/c/coreboot/+/41393/comment/2ba53f08_c0774df2 PS6, Line 124: outfile = fopen(argv[1], "w"); : if (outfile == NULL) { : fprintf(stderr, "Fail to open file %s!\n", argv[1]); : exit(1); : } This pattern appears at least three more times. Why not put it into a helper function?
static FILE *open_file(const char *filename, const char *mode) { FILE *file = fopen(filename, mode); if (file == NULL) { fprintf(stderr, "Could not open file %s: %s\n", filename, strerror(errno)); exit(1); } return file; }
https://review.coreboot.org/c/coreboot/+/41393/comment/6baaaa47_9a6a1d49 PS6, Line 155: fseek(infile, -256, SEEK_END); No error checking?
https://review.coreboot.org/c/coreboot/+/41393/comment/d95b7db6_3b15b7e8 PS6, Line 158: fread(fw_tag, 1, 4, infile); No error checking?
https://review.coreboot.org/c/coreboot/+/41393/comment/2e39990f_346ed94b PS6, Line 165: return 1; Shouldn't file descriptors be closed?
https://review.coreboot.org/c/coreboot/+/41393/comment/8b82b1fa_f9e9e292 PS6, Line 180: void *image = malloc(fwlen); No error checking?
https://review.coreboot.org/c/coreboot/+/41393/comment/1c300eef_67d83df9 PS6, Line 212: spisize = strtol(argv[1], NULL, 0); What if `spisize` is negative?
https://review.coreboot.org/c/coreboot/+/41393/comment/f7ccd561_70e3d993 PS6, Line 234: header_loc = strtol(argv[1], NULL, 0); What if `header_loc` is negative?
https://review.coreboot.org/c/coreboot/+/41393/comment/54ac1156_c795e546 PS6, Line 259: image = malloc(totallen); No error checking?