Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 9:
(4 comments)
thank you!
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@348 PS6, Line 348: return 1;
This error path leaks data. I recommend using a goto error pattern in this function to avoid leaks.
As you mentioned in the later comment, reqister_shutdown() should take care and free data in the case. Also here is the next patch improving init flow: https://review.coreboot.org/c/flashrom/+/48196
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *)
Is this explicit cast really needed? Typically I would no expect this for cast-from-void *.
Yes maybe the assignment makes this cast implicit, but I feel it's better to be explicit here.
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@308 PS8, Line 308: free(data);
nit: In general, this would be a bit more robust using gotos: […]
This whole init flow is improved in the other patch https://review.coreboot.org/c/flashrom/+/48196
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@338 PS8, Line 338: free(data);
on second thought ... if you already called register_shutdown() ... […]
Done