Attention is currently required from: Anastasia Klimchuk, Miklós Márton.
Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email )
Change subject: Fix FEATURE_NO_ERASE chips and add test for them ......................................................................
Patch Set 3:
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/84203/comment/a73930a0_5943095a?usp... : PS3, Line 487: for (int i = 0; opcode[i]; i++) { This will always crash if `spi_get_opcode_from_erasefn` doesn't know the eraser, which should be probably be fixed to gracefully bail out.
`FEATURE_NO_ERASE` seems to more correctly indicate that the chip doesn't need to be erased before writing, so I don't think it's correct to check that flag when seeing which erase options are supported: it's still meaningful to want to erase a chip without writing it, and some chips might have more efficient erase commands than rewriting with "erased" values which we'd want to support (and probe for support).
Since `spi_get_opcode_from_erasefn` might not know the opcode for a custom erasefn (including `BLOCK_ERASE_EMULATION`) and chips that don't require erase on write might still have usable erase opcodes, I think we should just skip this loop if `opcode == NULL`. Possibly also a comment should be added to `FEATURE_NO_ERASE` to document that it controls erase before write specifically.
-
Peter Marheine (Code Review)