January 2023 - flashrom-gerrit

[L] Change in flashrom[master]: flashrom.c: Add helper functions for new erase selction algorithm
by Aarya (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/65844 to look at the new patch set (#82). Change subject: flashrom.c: Add helper functions for new erase selction algorithm ...................................................................... flashrom.c: Add helper functions for new erase selction algorithm 1) Add function to flatten out the addresses of the flash chip as per the different erase functions. This function will return a list of layouts which is dynamically allocated. So after use all the layouts as well as the list itself should be freed. The free_erase_layout function does that. 2) Add function to align start and end address of the region (in struct walk_info) to some erase sector boundaries and modify the region start and end addresses to match nearest erase sector boundaries. This function will be used in the new algorithm for erase function selection. 3) Add function that returns a list of sectors (as seen by the first erase function) that need erasing. Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> --- A erasure_layout.c A erasure_layout.h 2 files changed, 286 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/65844/82 -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 82 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashrom.c: Add new erase_by_layout and walk_bylayout implementations
by Aarya (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen, Edward O'Callaghan, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71173 to look at the new patch set (#15). Change subject: flashrom.c: Add new erase_by_layout and walk_bylayout implementations ...................................................................... flashrom.c: Add new erase_by_layout and walk_bylayout implementations Add [erase,walk]_by_layout_new to use optimised implementations of the erase function selection algorithm. Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> --- M flashrom.c 1 file changed, 94 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/71173/15 -- To view, visit https://review.coreboot.org/c/flashrom/+/71173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Gerrit-Change-Number: 71173 Gerrit-PatchSet: 15 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[master]: flashrom.c:Add helper functions for new erase selction algorithm
by Aarya (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/65844 to look at the new patch set (#81). Change subject: flashrom.c:Add helper functions for new erase selction algorithm ...................................................................... flashrom.c:Add helper functions for new erase selction algorithm 1) Add function to flatten out the addresses of the flash chip as per the different erase functions. This function will return a list of layouts which is dynamically allocated. So after use all the layouts as well as the list itself should be freed. The free_erase_layout function does that. 2) Add function to align start and end address of the region (in struct walk_info) to some erase sector boundaries and modify the region start and end addresses to match nearest erase sector boundaries. This function will be used in the new algorithm for erase function selection. 3) Add function that returns a list of sectors (as seen by the first erase function) that need erasing. Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> --- A erasure_layout.c A erasure_layout.h 2 files changed, 286 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/65844/81 -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 81 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[master]: flashrom.c:Add helper functions needed for new erase selction algorithm
by Aarya (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Anastasia Klimchuk. Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c:Add helper functions needed for new erase selction algorithm ...................................................................... Patch Set 80: (7 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/65844/comment/8ca640a7_2d315840 PS80, Line 7: flashrom.c:Add helper functions needed for new erase selction algorithm > There are few small things (missing space and one typo), having in mind 72 char limit I suggest to r […] Done https://review.coreboot.org/c/flashrom/+/65844/comment/882837f7_0c8096b8 PS80, Line 9: 1)Add a function > Add space between number and text (and same for below). Done Patchset: PS78: > Looks like this comment resolved? Edward what do you think? I see this patch is end result of squash […] Done File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/65844/comment/9f0cc4f8_52340ece PS80, Line 115: > Remove one empty line (there are two empty lines in a row) Done https://review.coreboot.org/c/flashrom/+/65844/comment/e5c3a12e_71518dec PS80, Line 173: !! > I think one exclamation mark is enough :) […] Done https://review.coreboot.org/c/flashrom/+/65844/comment/c2eac7dd_7311b679 PS80, Line 198: if (ll->start_addr >= rstart && : ll->end_addr <= rend) { > This can be one line (will be more readable) Done https://review.coreboot.org/c/flashrom/+/65844/comment/3bb723ec_1c2fbffd PS80, Line 219: if (ll->start_addr >= rstart && : ll->end_addr <= rend) { > Here too: make this one line, for better readability Done -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 80 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 19 Jan 2023 20:14:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Add new erase_by_layout and walk_bylayout implementations
by Aarya (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71173 ) Change subject: flashrom.c: Add new erase_by_layout and walk_bylayout implementations ...................................................................... Patch Set 14: (6 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71173/comment/03f8b138_5acbb338 PS14, Line 1547: //erase layout creation failed > Insert new line before this Done https://review.coreboot.org/c/flashrom/+/71173/comment/43f421fe_108ab7aa PS14, Line 1552: //not enough memory > new line before this Done https://review.coreboot.org/c/flashrom/+/71173/comment/570df891_22ea3962 PS14, Line 1557: memset(curcontents, ~ERASED_VALUE(flashctx), flash_size); > new line before this Done https://review.coreboot.org/c/flashrom/+/71173/comment/f74991f9_9a166781 PS14, Line 1559: const struct flashrom_layout *const flash_layout = get_layout(flashctx); > new line before this Done https://review.coreboot.org/c/flashrom/+/71173/comment/b070fdd2_936b18ed PS14, Line 1569: _ret: > new line before this Done https://review.coreboot.org/c/flashrom/+/71173/comment/aeed7e72_071bec3b PS14, Line 1711: if (!flash_layout) { > new line before this Done -- To view, visit https://review.coreboot.org/c/flashrom/+/71173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Gerrit-Change-Number: 71173 Gerrit-PatchSet: 14 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 19 Jan 2023 20:14:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree/: Change chip restore data type from uint8_t to void ptr
by Edward O'Callaghan (Code Review) 19 Jan '23

19 Jan '23

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70349 ) Change subject: tree/: Change chip restore data type from uint8_t to void ptr ...................................................................... tree/: Change chip restore data type from uint8_t to void ptr Chip restore callbacks currently are used by - spi25_statusreg.c unlock functions to restore status register 1. - s25f.c to restore config register 3. Both of these cases only need to save a single uint8_t value to restore the original chip state, however storing a void pointer will allow more flexible chip restore behaviour. In particular, it will allow flashrom_wp_cfg objects to be saved and restored, enabling writeprotect-based unlocking. BUG=b:237485865,b:247421511 BRANCH=none TEST=Tested on grunt DUT (prog: sb600spi, flash: W25Q128.W): `flashrom --wp-range 0x0,0x1000000 \ flashrom --wp-status # Result: range=0x0,0x1000000 \ flashrom -w random.bin # Result: success \ flashrom -v random.bin # Result: success \ flashrom --wp-status # Result: range=0x0,0x1000000` Change-Id: I311b468a4b0349f4da9584c12b36af6ec2394527 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/70349 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M flashrom.c M include/flash.h M s25f.c M spi25_statusreg.c 4 files changed, 66 insertions(+), 10 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved Sergii Dmytruk: Looks good to me, but someone else must approve diff --git a/flashrom.c b/flashrom.c index 957d4aa..67b71f0 100644 --- a/flashrom.c +++ b/flashrom.c @@ -94,7 +94,7 @@ } int register_chip_restore(chip_restore_fn_cb_t func, - struct flashctx *flash, uint8_t status) + struct flashctx *flash, void *data) { if (flash->chip_restore_fn_count >= MAX_CHIP_RESTORE_FUNCTIONS) { msg_perr("Tried to register more than %i chip restore" @@ -102,7 +102,7 @@ return 1; } flash->chip_restore_fn[flash->chip_restore_fn_count].func = func; - flash->chip_restore_fn[flash->chip_restore_fn_count].status = status; + flash->chip_restore_fn[flash->chip_restore_fn_count].data = data; flash->chip_restore_fn_count++; return 0; @@ -115,7 +115,7 @@ while (flash->chip_restore_fn_count > 0) { int i = --flash->chip_restore_fn_count; rc |= flash->chip_restore_fn[i].func( - flash, flash->chip_restore_fn[i].status); + flash, flash->chip_restore_fn[i].data); } return rc; diff --git a/include/flash.h b/include/flash.h index fcefc42..3db38d2 100644 --- a/include/flash.h +++ b/include/flash.h @@ -509,7 +509,7 @@ enum decode_range_func decode_range; }; -typedef int (*chip_restore_fn_cb_t)(struct flashctx *flash, uint8_t status); +typedef int (*chip_restore_fn_cb_t)(struct flashctx *flash, void *data); struct flashrom_flashctx { struct flashchip *chip; @@ -544,7 +544,7 @@ int chip_restore_fn_count; struct chip_restore_func_data { chip_restore_fn_cb_t func; - uint8_t status; + void *data; } chip_restore_fn[MAX_CHIP_RESTORE_FUNCTIONS]; /* Progress reporting */ flashrom_progress_callback *progress_callback; @@ -618,7 +618,7 @@ int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename); int prepare_flash_access(struct flashctx *, bool read_it, bool write_it, bool erase_it, bool verify_it); void finalize_flash_access(struct flashctx *); -int register_chip_restore(chip_restore_fn_cb_t func, struct flashctx *flash, uint8_t status); +int register_chip_restore(chip_restore_fn_cb_t func, struct flashctx *flash, void *data); int check_block_eraser(const struct flashctx *flash, int k, int log); unsigned int count_usable_erasers(const struct flashctx *flash); int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, const uint8_t erased_value); diff --git a/s25f.c b/s25f.c index 605a560..549883b 100644 --- a/s25f.c +++ b/s25f.c @@ -21,6 +21,7 @@ * TODO: Implement fancy hybrid sector architecture helpers. */ +#include <stdlib.h> #include <string.h> #include "chipdrivers.h" @@ -230,10 +231,13 @@ return s25f_poll_status(flash); } -static int s25fs_restore_cr3nv(struct flashctx *flash, uint8_t cfg) +static int s25fs_restore_cr3nv(struct flashctx *flash, void *data) { int ret = 0; + uint8_t cfg = *(uint8_t *)data; + free(data); + msg_cdbg("Restoring CR3NV value to 0x%02x\n", cfg); ret |= s25fs_write_cr(flash, CR3NV_ADDR, cfg); ret |= s25fs_software_reset(flash); @@ -285,8 +289,15 @@ msg_cdbg("\n%s: CR3NV updated (0x%02x -> 0x%02x)\n", __func__, cfg, s25fs_read_cr(flash, CR3NV_ADDR)); + /* Restore CR3V when flashrom exits */ - register_chip_restore(s25fs_restore_cr3nv, flash, cfg); + uint8_t *data = calloc(sizeof(uint8_t), 1); + if (!data) { + msg_cerr("Out of memory!\n"); + return 1; + } + *data = cfg; + register_chip_restore(s25fs_restore_cr3nv, flash, data); } cr3nv_checked = 1; diff --git a/spi25_statusreg.c b/spi25_statusreg.c index 726ca8c..ec2c101 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -17,6 +17,8 @@ * GNU General Public License for more details. */ +#include <stdlib.h> + #include "flash.h" #include "chipdrivers.h" #include "programmer.h" @@ -263,8 +265,11 @@ return 0; } -static int spi_restore_status(struct flashctx *flash, uint8_t status) +static int spi_restore_status(struct flashctx *flash, void *data) { + uint8_t status = *(uint8_t *)data; + free(data); + msg_cdbg("restoring chip status (0x%02x)\n", status); return spi_write_register(flash, STATUS1, status); } @@ -304,7 +309,13 @@ } /* Restore status register content upon exit in finalize_flash_access(). */ - register_chip_restore(spi_restore_status, flash, status); + uint8_t *data = calloc(sizeof(uint8_t), 1); + if (!data) { + msg_cerr("Out of memory!\n"); + return 1; + } + *data = status; + register_chip_restore(spi_restore_status, flash, data); msg_cdbg("Some block protection in effect, disabling... "); if ((status & lock_mask) != 0) { -- To view, visit https://review.coreboot.org/c/flashrom/+/70349 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I311b468a4b0349f4da9584c12b36af6ec2394527 Gerrit-Change-Number: 70349 Gerrit-PatchSet: 10 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: flashrom.c: Supplement `chip->unlock()` calls with wp unlocking
by Edward O'Callaghan (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69517 ) Change subject: flashrom.c: Supplement `chip->unlock()` calls with wp unlocking ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69517 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I74b3f5d3a17749ea60485b916b2d87467a5d8b2f Gerrit-Change-Number: 69517 Gerrit-PatchSet: 12 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Thu, 19 Jan 2023 12:15:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Add new erase_by_layout and walk_bylayout implementations
by Anastasia Klimchuk (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Thomas Heijligen, Edward O'Callaghan, Aarya. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71173 ) Change subject: flashrom.c: Add new erase_by_layout and walk_bylayout implementations ...................................................................... Patch Set 14: (6 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71173/comment/3feec79b_40fa849d PS14, Line 1547: //erase layout creation failed Insert new line before this https://review.coreboot.org/c/flashrom/+/71173/comment/1e8ad176_41bd54bd PS14, Line 1552: //not enough memory new line before this https://review.coreboot.org/c/flashrom/+/71173/comment/3c124e28_0db8fc15 PS14, Line 1557: memset(curcontents, ~ERASED_VALUE(flashctx), flash_size); new line before this https://review.coreboot.org/c/flashrom/+/71173/comment/06f6109c_4dec647b PS14, Line 1559: const struct flashrom_layout *const flash_layout = get_layout(flashctx); new line before this https://review.coreboot.org/c/flashrom/+/71173/comment/82b0e2ea_a23d5319 PS14, Line 1569: _ret: new line before this https://review.coreboot.org/c/flashrom/+/71173/comment/44e6c4aa_77f2dbb9 PS14, Line 1711: if (!flash_layout) { new line before this -- To view, visit https://review.coreboot.org/c/flashrom/+/71173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Gerrit-Change-Number: 71173 Gerrit-PatchSet: 14 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Thu, 19 Jan 2023 07:58:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Add wrapper function to use the erase algorithm
by Anastasia Klimchuk (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Nico Huber, Edward O'Callaghan, Aarya. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66104 ) Change subject: flashrom.c: Add wrapper function to use the erase algorithm ...................................................................... Patch Set 98: (2 comments) Patchset: PS97: > Do you mean CB:65844 Yes, the correct patch to merge into is CB:65844. It already contains 3 patches squashed, and let's add 4th patch there too. The idea is to have one patch with everything for `erasure_layout.h` and `erasure_layout.c`. I have few style comments to `erase_write` function code, but I think it would be easier for you if I add them after you squash. So that the comments and resolution are in the same patch. File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/66104/comment/5dcb442b_73ac737d PS91, Line 195: //execute erase : erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser); : ret = erasefn(flashctx, start_addr, block_len); : if (ret) { : msg_cerr("Failed to execute erase command for offset %#x to %#x.\n", start_addr, start_addr + block_len); : ret = -1; : goto _end; : } : //adjust curcontents : memset(curcontents+start_addr, erased_value, block_len); : //after erase make it unselected again : erase_layout[i].layout_list[j].selected = false; : msg_cdbg("E(%x:%x)", start_addr, start_addr + block_len - 1); : //verify erase : ret = check_erased_range(flashctx, start_addr, block_len); : if (ret) { : msg_cerr("Verifying flash. Erase failed for range %#x : %#x, Abort.\n", start_addr, start_addr + block_len - 1); : goto _end; : } > What is the `region` in that patch? Given that CB:70517 is merged, I guess the comment can be resolved. -- To view, visit https://review.coreboot.org/c/flashrom/+/66104 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8 Gerrit-Change-Number: 66104 Gerrit-PatchSet: 98 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Thu, 19 Jan 2023 07:52:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: flashrom.c:Add helper functions needed for new erase selction algorithm
by Anastasia Klimchuk (Code Review) 19 Jan '23

19 Jan '23

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Aarya. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c:Add helper functions needed for new erase selction algorithm ...................................................................... Patch Set 80: Code-Review+1 (8 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/65844/comment/8bf9aead_02a5757b PS80, Line 7: flashrom.c:Add helper functions needed for new erase selction algorithm There are few small things (missing space and one typo), having in mind 72 char limit I suggest to rephrase: flashrom.c: Add helper functions for new erase selection algorithm https://review.coreboot.org/c/flashrom/+/65844/comment/37bc6124_a6f9fbc2 PS80, Line 9: 1)Add a function Add space between number and text (and same for below). Patchset: PS78: > This patch already contains the code from CB:65879. […] Looks like this comment resolved? Edward what do you think? I see this patch is end result of squashing 3 earlier patches. Patchset: PS80: I have few minor style things. Aarya, thank you so much for your work! We are almost there! :) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/65844/comment/c7413505_238178b1 PS80, Line 115: Remove one empty line (there are two empty lines in a row) https://review.coreboot.org/c/flashrom/+/65844/comment/1d93b7b1_ea77511a PS80, Line 173: !! I think one exclamation mark is enough :) (and same below) https://review.coreboot.org/c/flashrom/+/65844/comment/f09ac04e_f2917350 PS80, Line 198: if (ll->start_addr >= rstart && : ll->end_addr <= rend) { This can be one line (will be more readable) https://review.coreboot.org/c/flashrom/+/65844/comment/81d95fe9_9f3b5592 PS80, Line 219: if (ll->start_addr >= rstart && : ll->end_addr <= rend) { Here too: make this one line, for better readability -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 80 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Thu, 19 Jan 2023 07:32:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2023