Attention is currently required from: Edward O'Callaghan, Angel Pons. Anastasia Klimchuk 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/88bfc4ef_89b58987 PS1, Line 482: goto init_err_cleanup_exit;
Just one comment to make this review not too boring: This actually changes […]
I can explain my thinking process on this, because very oddly, the answer to the question whether this changes behaviour is yes and no at the same time :/ You explained why the answer is yes, and you are right of course! 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().
It seems odd, because register_shutdown used to be on a dividing line between pickit2_get_firmware_version() and the rest, and now it's in the deep. I don't know what is ideal solution here. One thing I do know is that register_shutdown will disappear from here soon, so whatever we decide to do it's not for long.
What do you think? I can change code here if you think I should do that, no problem!
-
Anastasia Klimchuk (Code Review)