December 2022 - flashrom-gerrit

[M] Change in flashrom[master]: tests: Add selfcheck to unit tests
by Anastasia Klimchuk (Code Review) 05 Dec '22

05 Dec '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69620 ) ( 9 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: tests: Add selfcheck to unit tests ...................................................................... tests: Add selfcheck to unit tests Add unit tests for programmer_table, flashchips, and board_matches structs. The tests are derived from the selfcheck function, checking that the required fields have been filled in. BUG=b:140595239 BRANCH=None TEST=meson test Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69620 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M board_enable.c M include/programmer.h M tests/meson.build A tests/selfcheck.c M tests/tests.c M tests/tests.h 6 files changed, 196 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/board_enable.c b/board_enable.c index 4903c0f..bf26173 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2524,6 +2524,7 @@ #endif { 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, P3, NULL, NULL, 0, NT, NULL}, /* end marker */ }; +const size_t board_matches_size = ARRAY_SIZE(board_matches); int selfcheck_board_enables(void) { diff --git a/include/programmer.h b/include/programmer.h index 16448ea..7bcef02 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -193,6 +193,7 @@ }; extern const struct board_match board_matches[]; +extern const size_t board_matches_size; struct board_info { const char *vendor; diff --git a/tests/meson.build b/tests/meson.build index e846d91..b8a5ba8 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -22,6 +22,7 @@ 'layout.c', 'chip.c', 'chip_wp.c', + 'selfcheck.c', ) if not programmer.get('dummy').get('active') diff --git a/tests/selfcheck.c b/tests/selfcheck.c new file mode 100644 index 0000000..aeccf18 --- /dev/null +++ b/tests/selfcheck.c @@ -0,0 +1,156 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2022 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include "flash.h" +#include "string.h" + +#include "include/test.h" +#include "programmer.h" +#include "tests.h" +#include <cmocka.h> + + +#define assert_table(assertion, message, index, name) \ + do { \ + if (!(assertion)) \ + fail_msg(message " for index:%zu name:%s", (index), (name) ? (name) : "unknown"); \ + } while (0) + + +void selfcheck_programmer_table(void **state) +{ + (void)state; /* unused */ + + size_t i; + for (i = 0; i < programmer_table_size; i++) { + const struct programmer_entry *const p = programmer_table[i]; + assert_table(p, "programmer entry is null", i, "unknown"); + assert_table(p->name, "programmer name is null", i, p->name); + bool type_good = false; + switch (p->type) { + case PCI: + case USB: + case OTHER: + type_good = true; + } + assert_table(type_good, "programmer type is invalid", i, p->name); + /* internal has its device list stored separately. */ + if (strcmp("internal", p->name) != 0) + assert_table(p->devs.note, "programmer devs.note is null", i, p->name); + assert_table(p->init, "programmer init is null", i, p->name); + } +} + +void selfcheck_flashchips_table(void **state) +{ + (void)state; /* unused */ + + size_t i; + assert_true(flashchips_size > 1); + assert_true(flashchips[flashchips_size - 1].name == NULL); + for (i = 0; i < flashchips_size - 1; i++) { + const struct flashchip *chip = &flashchips[i]; + assert_table(chip->vendor, "chip vendor is null", i, chip->name); + assert_table(chip->name, "chip name is null", i, chip->name); + assert_table(chip->bustype != BUS_NONE, "chip bustype is BUS_NONE", i, chip->name); + } +} + +void selfcheck_eraseblocks(void **state) +{ + (void)state; /* unused */ + + size_t chip_index; + for (chip_index = 0; chip_index < flashchips_size - 1; chip_index++) { + size_t i, j, k; + const struct flashchip *chip = &flashchips[chip_index]; + unsigned int prev_eraseblock_count = chip->total_size * 1024; + + for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { + unsigned int done = 0; + struct block_eraser eraser = chip->block_erasers[k]; + unsigned int curr_eraseblock_count = 0; + + for (i = 0; i < NUM_ERASEREGIONS; i++) { + /* Blocks with zero size are bugs in flashchips.c. */ + if (eraser.eraseblocks[i].count && !eraser.eraseblocks[i].size) { + fail_msg("Flash chip %s erase function %zu region %zu has size 0", + chip->name, k, i); + } + /* Blocks with zero count are bugs in flashchips.c. */ + if (!eraser.eraseblocks[i].count && eraser.eraseblocks[i].size) { + fail_msg("Flash chip %s erase function %zu region %zu has count 0", + chip->name, k, i); + } + done += eraser.eraseblocks[i].count * eraser.eraseblocks[i].size; + curr_eraseblock_count += eraser.eraseblocks[i].count; + } + /* Empty eraseblock definition with erase function. */ + if (!done && eraser.block_erase) { + printf("Strange: Empty eraseblock definition with non-empty erase function chip %s function %zu. Not an error.\n", + chip->name, k); + } + + if (!done) + continue; + if (done != chip->total_size * 1024) { + fail_msg( + "Flash chip %s erase function %zu region walking resulted in 0x%06x bytes total, expected 0x%06x bytes.", + chip->name, k, done, chip->total_size * 1024); + assert_true(false); + } + + if (!eraser.block_erase) + continue; + /* Check if there are identical erase functions for different + * layouts. That would imply "magic" erase functions. The + * easiest way to check this is with function pointers. + */ + for (j = k + 1; j < NUM_ERASEFUNCTIONS; j++) { + if (eraser.block_erase == chip->block_erasers[j].block_erase) { + fail_msg("Flash chip %s erase function %zu and %zu are identical.", + chip->name, k, j); + } + } + if (curr_eraseblock_count > prev_eraseblock_count) { + fail_msg("Flash chip %s erase function %zu is not in order", chip->name, k); + } + prev_eraseblock_count = curr_eraseblock_count; + } + } +} + +#if CONFIG_INTERNAL == 1 +void selfcheck_board_matches_table(void **state) +{ + (void)state; /* unused */ + + size_t i; + + assert_true(board_matches_size > 1); + assert_true(board_matches[board_matches_size - 1].vendor_name == NULL); + for (i = 0; i < board_matches_size - 1; i++) { + const struct board_match *b = &board_matches[i]; + assert_table(b->vendor_name, "board vendor_name is null", i, b->board_name); + assert_table(b->board_name, "board boad_name is null", i, b->board_name); + if ((!b->first_vendor || !b->first_device || !b->second_vendor || !b->second_device) + || ((!b->lb_vendor) ^ (!b->lb_part)) || (!b->max_rom_decode_parallel && !b->enable)) + fail_msg("Board enable for %s %s is misdefined.\n", b->vendor_name, b->board_name); + } +} + +#else + SKIP_TEST(selfcheck_board_matches_table) +#endif /* CONFIG_INTERNAL */ diff --git a/tests/tests.c b/tests/tests.c index fe70204..abbac64 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -381,6 +381,15 @@ }; ret |= cmocka_run_group_tests_name("helpers.c tests", helpers_tests, NULL, NULL); + const struct CMUnitTest selfcheck[] = { + cmocka_unit_test(selfcheck_programmer_table), + cmocka_unit_test(selfcheck_flashchips_table), + cmocka_unit_test(selfcheck_eraseblocks), + cmocka_unit_test(selfcheck_board_matches_table), + }; + ret |= cmocka_run_group_tests_name("selfcheck.c tests", selfcheck, + NULL, NULL); + const struct CMUnitTest flashrom_tests[] = { cmocka_unit_test(flashbuses_to_text_test_success), }; diff --git a/tests/tests.h b/tests/tests.h index bcca9e8..b36ac70 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -89,4 +89,10 @@ void full_chip_erase_with_wp_dummyflasher_test_success(void **state); void partial_chip_erase_with_wp_dummyflasher_test_success(void **state); +/* selfcheck.c */ +void selfcheck_programmer_table(void **state); +void selfcheck_flashchips_table(void **state); +void selfcheck_eraseblocks(void **state); +void selfcheck_board_matches_table(void **state); + #endif /* TESTS_H */ -- To view, visit https://review.coreboot.org/c/flashrom/+/69620 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a Gerrit-Change-Number: 69620 Gerrit-PatchSet: 11 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: test_build.sh: Switch to meson setup <dir>
by Anastasia Klimchuk (Code Review) 05 Dec '22

05 Dec '22

Attention is currently required from: Felix Singer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70242 ) Change subject: test_build.sh: Switch to meson setup <dir> ...................................................................... Patch Set 3: Code-Review+2 (1 comment) Patchset: PS3: I assume this patch is fully tested by jenkins :) -- To view, visit https://review.coreboot.org/c/flashrom/+/70242 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6e84997f910928d3973a4e5826a2d4196bdb2916 Gerrit-Change-Number: 70242 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Mon, 05 Dec 2022 03:28:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
by Eric Lai (Code Review) 05 Dec '22

05 Dec '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Nikolai Artemiev. Eric Lai has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70342 ) Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70342/comment/f57a51b0_1c574ce3 PS1, Line 9: CB:35480 please use commit hash. -- To view, visit https://review.coreboot.org/c/flashrom/+/70342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Gerrit-Change-Number: 70342 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 05 Dec 2022 02:37:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
by Anastasia Klimchuk (Code Review) 05 Dec '22

05 Dec '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70342 ) Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/70342/comment/6ed53cd6_7c97f7a5 PS1, Line 15: Coreboot issue Ticket (instead of "Coreboot issue") Patchset: PS1: Thank you so much Nikolai! Could you please add a topic "for_1.3.x" . -- To view, visit https://review.coreboot.org/c/flashrom/+/70342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Gerrit-Change-Number: 70342 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 05 Dec 2022 02:22:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: cli, manpage: Clean up occurrences of old image argument
by Anastasia Klimchuk (Code Review) 05 Dec '22

05 Dec '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70003 ) ( 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cli, manpage: Clean up occurrences of old image argument ...................................................................... cli, manpage: Clean up occurrences of old image argument --include argument was introduced and replaced --image argument in commit 45d50a101e8073191e6d88143990ed91d3bfe815 This patch cleans up remaining few places where old `--image` argument was mentioned so that now all the documentation has `--include`. --image is deprecated. Both old --image and new --include have the same short version -i and it remains the same. The code remains the same since the code handles --include already. Tested by running flashrom -h man ./flashrom.8.tmpl Ticket: https://ticket.coreboot.org/issues/372 Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/70003 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M cli_classic.c M flashrom.8.tmpl 2 files changed, 36 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/cli_classic.c b/cli_classic.c index 5cc14a0..0b0944f 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -71,8 +71,9 @@ " --fmap read ROM layout from fmap embedded in ROM\n" " --fmap-file <fmapfile> read ROM layout from fmap in <fmapfile>\n" " --ifd read layout from an Intel Firmware Descriptor\n" - " -i | --image <region>[:<file>] only read/write image <region> from layout\n" + " -i | --include <region>[:<file>] only read/write image <region> from layout\n" " (optionally with data from <file>)\n" + " --image <region>[:<file>] deprecated, please use --include\n" " -o | --output <logfile> log output to <logfile>\n" " --flash-contents <ref-file> assume flash contents to be <ref-file>\n" " -L | --list-supported print supported devices\n" @@ -619,7 +620,8 @@ {"ifd", 0, NULL, OPTION_IFD}, {"fmap", 0, NULL, OPTION_FMAP}, {"fmap-file", 1, NULL, OPTION_FMAP_FILE}, - {"image", 1, NULL, 'i'}, + {"image", 1, NULL, 'i'}, // (deprecated): back compatibility. + {"include", 1, NULL, 'i'}, {"flash-contents", 1, NULL, OPTION_FLASH_CONTENTS}, {"flash-name", 0, NULL, OPTION_FLASH_NAME}, {"flash-size", 0, NULL, OPTION_FLASH_SIZE}, diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index e672869..4c43323 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -49,7 +49,7 @@ (\fB\-\-flash\-name\fR|\fB\-\-flash\-size\fR| [\fB\-E\fR|\fB\-x\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] [(\fB\-l\fR <file>|\fB\-\-ifd\fR|\fB \-\-fmap\fR|\fB\-\-fmap-file\fR <file>) - [\fB\-i\fR <image>[:<file>]]] + [\fB\-i\fR <include>[:<file>]]] [\fB\-\-wp\-status\fR] [\fB\-\-wp\-list\fR] [\fB\-\-wp\-enable\fR|\fB\-\-wp\-disable\fR] [\fB\-\-wp\-range\fR <start>,<length>|\fB\-\-wp\-region\fR <region>] [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR])] -- To view, visit https://review.coreboot.org/c/flashrom/+/70003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134 Gerrit-Change-Number: 70003 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
by Nikolai Artemiev (Code Review) 05 Dec '22

05 Dec '22

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70342 ) Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence ...................................................................... flashchips.c: remove WREN from GD25Q256D enter 4BA sequence As noted in a comment on CB:35480, the GD25Q256D datasheet indicates that the chip does not requre a WREN command to enter 4BA mode. Testing has confirmed that a WREN command is not requried, so change the flashchip feature flags from FEATURE_4BA_WREN to FEATURE_4BA. Coreboot issue: https://ticket.coreboot.org/issues/356 BUG=none BRANCH=none TEST=read/write/erase/verify GD25Q256D flash with FT2232H programmer Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c 1 file changed, 23 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/70342/1 diff --git a/flashchips.c b/flashchips.c index 6f65389..7ea8601 100644 --- a/flashchips.c +++ b/flashchips.c @@ -6849,7 +6849,7 @@ .model_id = GIGADEVICE_GD25Q256D, .total_size = 32768, .page_size = 256, - .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_WREN | + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA | FEATURE_WRSR_EXT2 | FEATURE_WRSR2 | FEATURE_WRSR3, .tested = TEST_OK_PREWB, .probe = PROBE_SPI_RDID, -- To view, visit https://review.coreboot.org/c/flashrom/+/70342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Gerrit-Change-Number: 70342 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Anastasia Klimchuk (Code Review) 05 Dec '22

05 Dec '22

Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... Patch Set 4: (4 comments) File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/f07643a8_a4ca4b9a PS2, Line 139: || file == NULL > Line 144 causes a memory leak now. […] Oh yes, thanks for spotting memory leak! I missed that, fixed by `goto error`. The option you said prefer (moving the code) does not compile, I tried that at some point earlier locally (between patchset 2 and 3), the reason is: ``` ../layout.c: In function ‘register_include_arg’: ../layout.c:170:9: error: ‘name’ may be used uninitialized [-Werror=maybe-uninitialized] 170 | free(name); | ^~~~~~~~~~ ``` (line number might be out of date). Handling the `name` first, `file` second solves the issue. This also means all error paths (for name, file and below) are treated by `goto error`, looks nice and consistent. File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/b2316f42_28504fd3 PS3, Line 135: if (name == NULL) { > nit: `!name` Done https://review.coreboot.org/c/flashrom/+/70006/comment/cee1d3e0_a50fe7c7 PS3, Line 142: if(!colon[1]) { > mising space `if (` Done https://review.coreboot.org/c/flashrom/+/70006/comment/a4347779_12ec7822 PS3, Line 148: if (file == NULL) { > nit: `!file` Done -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 05 Dec 2022 02:03:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Anastasia Klimchuk (Code Review) 05 Dec '22

05 Dec '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello Felix Singer, build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70006 to look at the new patch set (#4). Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... layout: Check return values for strdup in register_include_arg strdup return values should be checked for NULL to catch the potential error case of out of memory. Follow up on commit 45d50a101e8073191e6d88143990ed91d3bfe815 Ticket: https://ticket.coreboot.org/issues/372 Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M layout.c 1 file changed, 39 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/70006/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
by Edward O'Callaghan (Code Review) 04 Dec '22

04 Dec '22

Attention is currently required from: Subrata Banik, Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69195 ) Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip` ...................................................................... Patch Set 9: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/69195/comment/ae975696_a7b26501 PS2, Line 7: ichspi.c: Read chip ID and use it to identify chip > > > Maybe we should just print a warning if >1 chip is detected? […] In flashrom.c the top level of the CFG is: ``` @@ -915,7 +915,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f if (!w29ee011_can_override(flash->chip->name, chip_to_probe)) goto notfound; - if (probe_func(flash) != 1) ``` you could return '2' from this function to mean two chips found and handle it in the above. The actionable here, however, is likely just read the descriptor to find the number of flashes and print that when it is >0 (i.e., >1chip) but just assume the first chip. -- To view, visit https://review.coreboot.org/c/flashrom/+/69195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia408e1e45dc6f53c0934afd6558e301abfa48ee6 Gerrit-Change-Number: 69195 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 04 Dec 2022 23:45:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tree/: Make probe_opcode() flashctx argument const
by Edward O'Callaghan (Code Review) 04 Dec '22

04 Dec '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70030 ) ( 2 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: tree/: Make probe_opcode() flashctx argument const ...................................................................... tree/: Make probe_opcode() flashctx argument const Probing an opcode generally shouldn't involve mutating the flashctx state and currently no probe_opcode functions do that. Make the flashctx arg const so that call sites don't need to have a non-const pointer. BUG=b:253715389,b:253713774 BRANCH=none TEST=ninja test Change-Id: I19e98be50d682de2d2715417f8b7b8c62b871617 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/70030 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M dummyflasher.c M ichspi.c M include/programmer.h M spi.c 4 files changed, 30 insertions(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/dummyflasher.c b/dummyflasher.c index 35cdfef..da4efc7 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -122,10 +122,10 @@ emu_data->spi_write_256_chunksize); } -static bool dummy_spi_probe_opcode(struct flashctx *flash, uint8_t opcode) +static bool dummy_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) { size_t i; - struct emu_data *emu_data = flash->mst->spi.data; + const struct emu_data *emu_data = flash->mst->spi.data; for (i = 0; i < emu_data->spi_blacklist_size; i++) { if (emu_data->spi_blacklist[i] == opcode) return false; diff --git a/ichspi.c b/ichspi.c index 62d1799..4588502 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1661,7 +1661,7 @@ return ret; } -static bool ich_spi_probe_opcode(struct flashctx *flash, uint8_t opcode) +static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) { return find_opcode(curopcodes, opcode) >= 0; } diff --git a/include/programmer.h b/include/programmer.h index 55e300a..16448ea 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -312,7 +312,7 @@ int (*write_256)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*write_aai)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*shutdown)(void *data); - bool (*probe_opcode)(struct flashctx *flash, uint8_t opcode); + bool (*probe_opcode)(const struct flashctx *flash, uint8_t opcode); void *data; }; @@ -322,7 +322,7 @@ int default_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int default_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int default_spi_write_aai(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); -bool default_spi_probe_opcode(struct flashctx *flash, uint8_t opcode); +bool default_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode); int register_spi_master(const struct spi_master *mst, void *data); /* The following enum is needed by ich_descriptor_tool and ich* code as well as in chipset_enable.c. */ diff --git a/spi.c b/spi.c index 38d8d01..eeb1362 100644 --- a/spi.c +++ b/spi.c @@ -134,7 +134,7 @@ return flash->mst->spi.write_aai(flash, buf, start, len); } -bool default_spi_probe_opcode(struct flashctx *flash, uint8_t opcode) +bool default_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) { return true; } -- To view, visit https://review.coreboot.org/c/flashrom/+/70030 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I19e98be50d682de2d2715417f8b7b8c62b871617 Gerrit-Change-Number: 70030 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022