Attention is currently required from: Ricardo Quesada, Paul Menzel. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56406 )
Change subject: util/elogtool: add tool to print elog events ......................................................................
Patch Set 4: Code-Review+1
(17 comments)
Patchset:
PS4: Great work! Really happy to see this 😊
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56406/comment/b7517dcb_aaade66e PS4, Line 32: fprintf(stderr, "elogtool: list elog events\n"); : fprintf(stderr, "\nUSAGE:\n"); : fprintf(stderr, "\t%s COMMAND [-f <filename>]\n\n", invoked_as); : fprintf(stderr, "where, COMMAND is:\n"); : fprintf(stderr, " list = lists all the event logs in human readable format\n"); : fprintf(stderr, "\nARGS\n"); : fprintf(stderr, "-f, --file <filename> Input file that holds event log partition.\n"); : fprintf(stderr, : " If empty it will try to read from the RW_ELOG ROM region\n"); : fprintf(stderr, "-h, --help Print this help\n"); nit: could be one fprintf() call, e.g.:
``` fprinf(stderr, "elogtool: list elog events\n\n" "USAGE:\n" "\t%s COMMAND [-f..." ```
https://review.coreboot.org/c/coreboot/+/56406/comment/f952cdef_b7b104f4 PS4, Line 54: from
https://review.coreboot.org/c/coreboot/+/56406/comment/9686eed7_7af13d64 PS4, Line 64: return 0; nit: blank line after `}`
https://review.coreboot.org/c/coreboot/+/56406/comment/3f3d3495_f4f539eb PS4, Line 72: int nit: `unsigned int`
https://review.coreboot.org/c/coreboot/+/56406/comment/39172353_ebafcd11 PS4, Line 141: input_file = optarg; nit: blank line after `}`
File util/cbfstool/eventlog.c:
https://review.coreboot.org/c/coreboot/+/56406/comment/5699e313_32d1fab7 PS4, Line 43: time_t time; : char tm_string[40]; : struct tm tm; : const char *tm_format = "%y-%m-%d%t%H:%M:%S"; suggestion: if you have no better order for declarations, reverse xmas tree is nice: ``` const char *tm_format = "%y-%m-%d%t%H:%M:%S"; char tm_string[40]; struct tm tm; time_t time; ```
https://review.coreboot.org/c/coreboot/+/56406/comment/55350ea3_9afd5f34 PS4, Line 48: /* make certain _all_ members of tm get initialized */ is this comment required?
https://review.coreboot.org/c/coreboot/+/56406/comment/3cd858b7_59809f2d PS4, Line 190: uint8_t `const uint8_t`
https://review.coreboot.org/c/coreboot/+/56406/comment/239fb5c3_3780f4db PS4, Line 203: (extra >> 8) & 0xff, (extra >> 3) & 0x1f, : (extra & 0x3)); suggestion: break this out into some #defines, e.g.:
``` #define PATH_PCI_BUS_SHIFT 8 #define PATH_PCI_BUS_MASK 0xff #define PATH_PCI_DEV_SHIFT 3 #define PATH_PCI_DEV_MASK 0x1f #define PATH_PCI_FN_SHIFT 0 #define PATH_PCI_FN_MASK 0x03
... eventlog_printf("%02x:%02x.%1x", (extra >> PATH_PCI_BUS_SHIFT) & PATH_PCI_BUS_MASK, (extra >> PATH_PCI_DEV_SHIFT) & PATH_PCI_DEV_MASK, (extra >> PATH_PCI_FN_SHIFT) & PATH_PCI_FN_MASK);
```
https://review.coreboot.org/c/coreboot/+/56406/comment/be5fc703_ec6a44ea PS4, Line 208: 8 PATH_I2C_MODE10BIT_SHIFT
https://review.coreboot.org/c/coreboot/+/56406/comment/be0260eb_51b729df PS4, Line 208: 0xff PATH_I2C_MODE10BIT_MASK
https://review.coreboot.org/c/coreboot/+/56406/comment/c7f4a968_c52382c0 PS4, Line 208: 0xff PATH_I2C_ADDRESS_MASK
https://review.coreboot.org/c/coreboot/+/56406/comment/129acc32_11f340b9 PS4, Line 589: return 0; nit: `break;`
https://review.coreboot.org/c/coreboot/+/56406/comment/0308c51f_6d3b9203 PS4, Line 602: /* print the timestamp */ is this comment required?
https://review.coreboot.org/c/coreboot/+/56406/comment/2f606636_5f8f90ac PS4, Line 605: /* print event type */ is this comment required?
https://review.coreboot.org/c/coreboot/+/56406/comment/d4867563_b51ecf02 PS4, Line 608: /* print custom data */ is this comment required?