January 2023 - flashrom-gerrit

[S] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Anastasia Klimchuk (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Edward O'Callaghan, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 2: (1 comment) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72408/comment/5b9de1af_4a670d17 PS2, Line 910: What if you try: if (!emu_data) return 0; The reason I thought about it, is that on first run of shutdown emu_data is freed. And then emu_data->anything is going to segfault right? emu_data is shared between masters if multiple are registered. hope I am not missing anything The good thing about dummyflasher, it's [relatively] easy to add new unit tests for it. If there is any combination of buses that is not covered by unit test, it can be added, can be added in this patch. And then the best thing is: unit tests can fully test this patch! and unit tests are checking there is no memory leaks. -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 25 Jan 2023 10:34:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: dummyflasher: fix propagation of register_*_master() return values
by Anastasia Klimchuk (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Edward O'Callaghan, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72430 ) Change subject: dummyflasher: fix propagation of register_*_master() return values ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72430 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3a7eeb3cdd814db18b0717ae8b40ecadb4c32f7c Gerrit-Change-Number: 72430 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 25 Jan 2023 10:27:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Supplement `chip->unlock()` calls with wp unlocking
by Anastasia Klimchuk (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk 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 14: 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: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 25 Jan 2023 04:08:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: dummyflasher.c: Speed up programmer ops without delay
by Sam McNally (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Edward O'Callaghan. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72423 ) Change subject: dummyflasher.c: Speed up programmer ops without delay ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I43411b6d1558d1a5da4c7508618f8a6ec6d9bf95 Gerrit-Change-Number: 72423 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 25 Jan 2023 03:21:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: programmers: Allow opaque programmers to specify custom delay
by Sam McNally (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Edward O'Callaghan. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72424 ) Change subject: programmers: Allow opaque programmers to specify custom delay ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72424 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia86ad21f011214abc2aa0891a43559f91e2a9591 Gerrit-Change-Number: 72424 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 25 Jan 2023 03:21:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: programmers: Allow opaque programmers to specify custom delay
by Edward O'Callaghan (Code Review) Jan. 25, 2023

Jan. 25, 2023

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/72424 ) Change subject: programmers: Allow opaque programmers to specify custom delay ...................................................................... programmers: Allow opaque programmers to specify custom delay The dummyprogrammer in the opaque case can have a NOP delay to avoid wasting CPU time and energy. BUG=b:266014935 TEST=builds Change-Id: Ia86ad21f011214abc2aa0891a43559f91e2a9591 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c M include/programmer.h 2 files changed, 20 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/72424/1 diff --git a/flashrom.c b/flashrom.c index 67b71f0..09172f6 100644 --- a/flashrom.c +++ b/flashrom.c @@ -277,6 +277,9 @@ } else if (flash->mst->buses_supported & BUS_PARALLEL) { if (flash->mst->par.delay) return flash->mst->par.delay(flash, usecs); + } else if (flash->mst->buses_supported & BUS_PROG) { + if (flash->mst->opaque.delay) + return flash->mst->opaque.delay(flash, usecs); } return default_delay(usecs); diff --git a/include/programmer.h b/include/programmer.h index 9e706d5..db32b2c 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -418,6 +418,7 @@ enum flashrom_wp_result (*wp_get_ranges)(struct flashrom_wp_ranges **, struct flashctx *); void (*get_region)(const struct flashctx *flash, unsigned int addr, struct flash_region *region); int (*shutdown)(void *data); + void (*delay) (const struct flashctx *flash, unsigned int usecs); void *data; }; int register_opaque_master(const struct opaque_master *mst, void *data); -- To view, visit https://review.coreboot.org/c/flashrom/+/72424 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia86ad21f011214abc2aa0891a43559f91e2a9591 Gerrit-Change-Number: 72424 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: dummyflasher.c: Speed up programmer ops without delay
by Edward O'Callaghan (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Sam McNally. Hello Sam McNally, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/72423 to look at the new patch set (#2). Change subject: dummyflasher.c: Speed up programmer ops without delay ...................................................................... dummyflasher.c: Speed up programmer ops without delay There is no need to perform delays for the dummyflasher as there is no chipsets to settle down. BUG=b:266014935 TEST=builds Change-Id: I43411b6d1558d1a5da4c7508618f8a6ec6d9bf95 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M dummyflasher.c 1 file changed, 23 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/72423/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I43411b6d1558d1a5da4c7508618f8a6ec6d9bf95 Gerrit-Change-Number: 72423 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: dummyflasher.c: Speed up programmer ops without delay
by Edward O'Callaghan (Code Review) Jan. 25, 2023

Jan. 25, 2023

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/72423 ) Change subject: dummyflasher.c: Speed up programmer ops without delay ...................................................................... dummyflasher.c: Speed up programmer ops without delay There is no need to perform delays for the dummyflasher as there is no chipsets to settle down. BUG=b:266014935 TEST=builds Change-Id: I43411b6d1558d1a5da4c7508618f8a6ec6d9bf95 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M dummyflasher.c 1 file changed, 22 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/72423/1 diff --git a/dummyflasher.c b/dummyflasher.c index 1960e92..70f4d7b 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -919,6 +919,10 @@ return 0; } +static void dummy_nop_delay(const struct flashctx *flash, unsigned int usecs) +{ +} + static const struct spi_master spi_master_dummyflasher = { .map_flash_region = dummy_map, .unmap_flash_region = dummy_unmap, @@ -930,6 +934,7 @@ .read = default_spi_read, .write_256 = dummy_spi_write_256, .probe_opcode = dummy_spi_probe_opcode, + .delay = dummy_nop_delay, }; static const struct par_master par_master_dummyflasher = { @@ -943,6 +948,7 @@ .chip_writew = dummy_chip_writew, .chip_writel = dummy_chip_writel, .chip_writen = dummy_chip_writen, + .delay = dummy_nop_delay, }; static const struct opaque_master opaque_master_dummyflasher = { -- To view, visit https://review.coreboot.org/c/flashrom/+/72423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I43411b6d1558d1a5da4c7508618f8a6ec6d9bf95 Gerrit-Change-Number: 72423 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

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

Jan. 25, 2023

Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk. 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 14: 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: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 25 Jan 2023 01:03:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Supplement `chip->unlock()` calls with wp unlocking
by Nikolai Artemiev (Code Review) Jan. 25, 2023

Jan. 25, 2023

Attention is currently required from: Edward O'Callaghan, Sergii Dmytruk. Nikolai Artemiev 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 14: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/69517/comment/c9661551_f2c5e6ab PS13, Line 1931: restore_flash_wp(flash, initial_wp_cfg); > Indirectly calling `flashrom_wp_write_cfg` is intentional here? I guess it won't hurt, just looks w […] Good point, I hadn't looked at this CL in a while and I thought we needed to restore in this case since the restore function won't be called if registration fails. But this all happens before we disable flash protection in `unlock_flash_wp()` so we just need to free the reference to `initial_wp_cfg` like you said in your first comment. -- 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: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 25 Jan 2023 00:34:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0