Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@317 PS2, Line 317: ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ this is common code, move it to after the ifdef blocks.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@331 PS2, Line 331: { no need for '{ .. }' for single statements under a branch.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@341 PS2, Line 341: msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported); : /* Check for FWH because IT85 listens to FWH cycles. : * FIXME: The big question is whether FWH cycles are necessary : * for communication even if LPC_IO is defined. : */ : if (internal_buses_supported & BUS_FWH) : msg_pdbg("Registering IT85 SPI.\n"); this whole block constitutes checks and prints of inputs parameters to this function and therefore should be at the beginning of the function - probably after the `if (!(internal_buses_supported & BUS_FWH)) {` block above.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@354 PS2, Line 354: if (!data) { You are checking for nullity _after_ you deference here. Remember the pattern 'check-then-do'.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@360 PS2, Line 360: free(data); It maybe a bit more clear now for you that this just papers over the issue that register_shutdown() should never fail. Notice now that physmap() leaks and that the shutdown callback is responsible for unmapping it. Putting a free() here (not that it matters) is just distracting away from the real leaks that do matter.