Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52773 )
Change subject: pickit2_spi.c: Separate shutdown from failed init cleanup ......................................................................
Patch Set 1:
(1 comment)
File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/52773/comment/f84d14d9_bd9f498e PS1, Line 482: goto init_err_cleanup_exit;
At the same time, what was before: if failure happened between pickit2_get_firmware_version() and register_spi_master() then pickit2_shutdown() would be called (because at the point when pickit2_get_firmware_version() run, shutdown function has already been registered). This stays the same: all failures before pickit2_get_firmware_version() do cleanup for themselves, all failures after pickit2_get_firmware_version() rely on pickit2_shutdown().
I think you are right wrt. the state of the hardware. But software-wise, in case register_shutdown() failed, we'd have left dangling resources (e.g. libusb_exit() wasn't called).
What do you think? I can change code here if you think I should do that, no problem!
The patch is fine, hence the +2. I only wanted to point our that behavioral changes usually have priority in commit messages. Generally, this can also be achieved by splitting them out into separate commits.
Nothing to do here.
-
Nico Huber (Code Review)