Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email )
Change subject: Fix FEATURE_NO_ERASE chips and add test for them ......................................................................
Fix FEATURE_NO_ERASE chips and add test for them
New check was added to `check_block_eraser` in commit 0f389aea9e630c3b21547a5dd8dbe572a8502853 but it was not handling FEATURE_NO_ERASE chips.
This patch fixes processing such chips and adds test to run write and verify with dummyflasher for FEATURE_NO_ERASE chips.
Ticket: https://ticket.coreboot.org/issues/553
Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Signed-off-by: Anastasia Klimchuk aklm@flashrom.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/84203 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Peter Marheine pmarheine@chromium.org --- M flashrom.c M include/flash.h M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 80 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Peter Marheine: Looks good to me, approved
diff --git a/flashrom.c b/flashrom.c index 01a41c4..e2c60f4 100644 --- a/flashrom.c +++ b/flashrom.c @@ -484,14 +484,15 @@
if (flash->mst->buses_supported & BUS_SPI) { const uint8_t *opcode = spi_get_opcode_from_erasefn(eraser.block_erase); - for (int i = 0; opcode[i]; i++) { - if (!spi_probe_opcode(flash, opcode[i])) { - if (log) - msg_cdbg("block erase function and layout found " - "but SPI master doesn't support the function. "); - return 1; + if (opcode) + for (int i = 0; opcode[i]; i++) { + if (!spi_probe_opcode(flash, opcode[i])) { + if (log) + msg_cdbg("block erase function and layout found " + "but SPI master doesn't support the function. "); + return 1; + } } - } } // TODO: Once erase functions are annotated with allowed buses, check that as well. return 0; diff --git a/include/flash.h b/include/flash.h index d9a20ad..d0e55af 100644 --- a/include/flash.h +++ b/include/flash.h @@ -148,6 +148,14 @@ * other flash chips, such as the ENE KB9012 internal flash, work the opposite way. */ #define FEATURE_ERASED_ZERO (1 << 18) +/* + * Feature indicates that the chip does not require erase before writing: + * write operations can set any bit to any value without first doing an erase, + * but bulk erase operations may still be supported. + * + * EEPROMs usually behave this way (compare to Flash, which requires erase), + * for example the ST M95M02. + */ #define FEATURE_NO_ERASE (1 << 19)
#define FEATURE_WRSR_EXT2 (1 << 20) diff --git a/tests/chip.c b/tests/chip.c index 3e1ae3b..604d79e 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -27,6 +27,7 @@
#include <include/test.h> #include <stdio.h> +#include <stdlib.h> #include <string.h>
#include "tests.h" @@ -132,6 +133,25 @@ }}, };
+/* Chip expected to be processed with dummyflasher, so using real op functions. */ +static const struct flashchip chip_no_erase = { + .vendor = "aklm&dummyflasher", + .total_size = 16 * 1024, + .tested = TEST_OK_PREW, + .read = SPI_CHIP_READ, + .write = SPI_CHIP_WRITE256, + .page_size = 256, + .feature_bits = FEATURE_NO_ERASE | FEATURE_ERASED_ZERO, + .block_erasers = + { + { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + /* Special erase fn for chips without erase capability. */ + .block_erase = SPI_BLOCK_ERASE_EMULATION, + } + }, +}; + /* Setup the struct for W25Q128.V, all values come from flashchips.c */ static const struct flashchip chip_W25Q128_V = { .vendor = "aklm&dummyflasher", @@ -384,6 +404,48 @@ free(newcontents); }
+void write_chip_feature_no_erase(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + + /* + * Tricking the dummyflasher by asking to emulate W25Q128FV but giving to it + * mock chip with FEATURE_NO_ERASE. + * As long as chip size is the same, this is fine. + */ + struct flashchip mock_chip = chip_no_erase; + const char *param_dup = "bus=spi,emulate=W25Q128FV"; + + setup_chip(&flashctx, &layout, &mock_chip, param_dup, &chip_io); + + /* See comment in write_chip_test_success */ + const char *const filename = "-"; + unsigned long size = mock_chip.total_size * 1024; + uint8_t *const newcontents = malloc(size); + assert_non_null(newcontents); + + printf("Write chip operation started.\n"); + assert_int_equal(0, read_buf_from_file(newcontents, size, filename)); + assert_int_equal(0, flashrom_image_write(&flashctx, newcontents, size, NULL)); + assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, size)); + printf("Write chip operation done.\n"); + + teardown(&layout); + + free(newcontents); +} + void write_nonaligned_region_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/tests.c b/tests/tests.c index 165390a..78f1b47 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -498,6 +498,7 @@ cmocka_unit_test(read_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_test_success), cmocka_unit_test(write_chip_with_dummyflasher_test_success), + cmocka_unit_test(write_chip_feature_no_erase), cmocka_unit_test(write_nonaligned_region_with_dummyflasher_test_success), cmocka_unit_test(verify_chip_test_success), cmocka_unit_test(verify_chip_with_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index 8cf8be0..3536d1d 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -86,6 +86,7 @@ void read_chip_with_dummyflasher_test_success(void **state); void write_chip_test_success(void **state); void write_chip_with_dummyflasher_test_success(void **state); +void write_chip_feature_no_erase(void **state); void write_nonaligned_region_with_dummyflasher_test_success(void **state); void verify_chip_test_success(void **state); void verify_chip_with_dummyflasher_test_success(void **state);
-
Anastasia Klimchuk (Code Review)