- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[master]: erasure_layout: Fix double invocation of erasers
by Anastasia Klimchuk (Code Review) 11 Sep '23

11 Sep '23

Attention is currently required from: Aarya, Edward O'Callaghan. Hello Aarya, Edward O'Callaghan, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/77747?usp=email to look at the new patch set (#3). Change subject: erasure_layout: Fix double invocation of erasers ...................................................................... erasure_layout: Fix double invocation of erasers Erasefn was invoked over the same block of memory twice. Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M erasure_layout.c 1 file changed, 14 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/77747/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/77747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Gerrit-Change-Number: 77747 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[master]: tests: Unit tests for erase function selection algo
by Anastasia Klimchuk (Code Review) 10 Sep '23

10 Sep '23

Attention is currently required from: Aarya, Simon Buhrow, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67535?usp=email ) Change subject: tests: Unit tests for erase function selection algo ...................................................................... Patch Set 11: (1 comment) Patchset: PS8: > I started implementing assertions for expected order of erase blocks invocations, and discovered tha […] Finished the assertions for expected order of erase blocks invocations. I created one more patch (CB:77747) as an attempt to fix double invocations, comments welcome. I will fix the rest of the comments in the next patchset. -- To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Gerrit-Change-Number: 67535 Gerrit-PatchSet: 11 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: 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: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sun, 10 Sep 2023 14:02:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: [WIP] Fix double invocation of erasers
by Anastasia Klimchuk (Code Review) 10 Sep '23

10 Sep '23

Attention is currently required from: Aarya, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77747?usp=email ) Change subject: [WIP] Fix double invocation of erasers ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: This is my attempt to fix the double invocation of every eraser, what do you think about it? I am not sure this is a perfect way, really interested in your thoughts. The issue was: every eraser called twice. This would not be obvious in the real run of flashrom, because at the end the chip is erased and the memory state is correct. But in the test (see next patch CB:67535) I am verifying erasers are called in correct order and the test is recording all invocations, and that's how I discovered the double invocations. -- To view, visit https://review.coreboot.org/c/flashrom/+/77747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Gerrit-Change-Number: 77747 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sun, 10 Sep 2023 13:59:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: [WIP] Fix double invocation of erasers
by Anastasia Klimchuk (Code Review) 10 Sep '23

10 Sep '23

Attention is currently required from: Aarya. Hello Aarya, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/77747?usp=email to look at the new patch set (#2). Change subject: [WIP] Fix double invocation of erasers ...................................................................... [WIP] Fix double invocation of erasers Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M erasure_layout.c 1 file changed, 14 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/77747/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/77747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Gerrit-Change-Number: 77747 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: [WIP] Fix double invocation of erasers
by Anastasia Klimchuk (Code Review) 10 Sep '23

10 Sep '23

Attention is currently required from: Aarya. Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/77747?usp=email ) Change subject: [WIP] Fix double invocation of erasers ...................................................................... [WIP] Fix double invocation of erasers Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M erasure_layout.c 1 file changed, 15 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/77747/1 diff --git a/erasure_layout.c b/erasure_layout.c index c9ac44b..06626c3 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -304,6 +304,7 @@ goto _end; } + unsigned int last_addr = start_addr; unsigned int len; for (unsigned int addr = start_addr; addr < start_addr + block_len; addr += len) { struct flash_region region; @@ -337,15 +338,22 @@ msg_cerr("ERASE FAILED!\n"); goto _end; } + + last_addr = addr + len; } - ret = erasefn(flashctx, start_addr, block_len); - if (ret) { - msg_cerr("Failed to execute erase command " - "for offset %#"PRIx32" to %#"PRIx32".\n", - start_addr, start_addr + block_len); - ret = -1; - goto _end; + if (last_addr < start_addr + block_len) { + // Something is left to erase + printf("Left to erase from %#"PRIx32" to %#"PRIx32".\n", last_addr, start_addr + block_len); + unsigned int remaining_len = start_addr + block_len - last_addr; + ret = erasefn(flashctx, last_addr, remaining_len); + if (ret) { + msg_cerr("Failed to execute erase command " + "for offset %#"PRIx32" to %#"PRIx32".\n", + last_addr, last_addr + remaining_len); + ret = -1; + goto _end; + } } // adjust curcontents -- To view, visit https://review.coreboot.org/c/flashrom/+/77747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Gerrit-Change-Number: 77747 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newchange

1 0

[L] Change in flashrom[master]: tests: Unit tests for erase function selection algo
by Anastasia Klimchuk (Code Review) 10 Sep '23

10 Sep '23

Attention is currently required from: Aarya, Simon Buhrow, Thomas Heijligen. Hello Aarya, Edward O'Callaghan, Simon Buhrow, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67535?usp=email to look at the new patch set (#10). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: tests: Unit tests for erase function selection algo ...................................................................... tests: Unit tests for erase function selection algo The test checks that algorithm for erase functions selection works and there are no regressions. Specifically, test contains a number of test cases (14 at the time of writing, but more can be added). Each case initialises a given initial state of the memory for the mock chip, and layout regions on the chip, and then performs erase and write operations. At the end of operation, test asserts the following: - the state of mock chip memory is as expected, i.e. properly erased or written - erase blocks are invoked in expected order and expected number of them - chip operation (erase or write) returned 0. Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- A tests/erase_func_algo.c M tests/meson.build M tests/tests.c M tests/tests.h 4 files changed, 611 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/35/67535/10 -- To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Gerrit-Change-Number: 67535 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: 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: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: Remove dependency on C23 __has_include()
by Anastasia Klimchuk (Code Review) 08 Sep '23

08 Sep '23

Attention is currently required from: Alexander Goncharov, Anton Samsonov, Anton Samsonov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77089?usp=email ) Change subject: Remove dependency on C23 __has_include() ...................................................................... Patch Set 1: (1 comment) File include/cli_classic.h: https://review.coreboot.org/c/flashrom/+/77089/comment/da0c1e4b_a9ffa4f9 : PS1, Line 18: #if HAVE_GETOPT_H : #include <getopt.h> : #elif !defined(HAVE_GETOPT_H) : #if !defined(__has_include) && (__STDC_VERSION__ < 202300L) : #include <getopt.h> : #define HAVE_GETOPT_H 1 : #elif __has_include(<getopt.h>) : #include <getopt.h> : #define HAVE_GETOPT_H 1 : #else : #define HAVE_GETOPT_H 0 : #endif /* __has_include() */ : #endif /* HAVE_GETOPT_H */ : : #if !HAVE_GETOPT_H > Our and coreboot code styles say nothing about directives. […] Sorry it took me some time to get to this patch. It's hard to read yes, but I suspect 15 lines of pre-processor would be hard to read anyway :\ I would be so happy to remove pre-processor altogether, as I said in my other comment. With or without style guide, I would first try to resolve this with build system. -- To view, visit https://review.coreboot.org/c/flashrom/+/77089?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic544963ffd29626ae0a21bdddb1c78850cc43ec6 Gerrit-Change-Number: 77089 Gerrit-PatchSet: 1 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 08 Sep 2023 11:00:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: Remove dependency on C23 __has_include()
by Anastasia Klimchuk (Code Review) 08 Sep '23

08 Sep '23

Attention is currently required from: Anton Samsonov, Anton Samsonov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77089?usp=email ) Change subject: Remove dependency on C23 __has_include() ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I would avoid introducing new pre-processor conditionals, because we are planning to go in the opposite direction and git rid of them all. Instead resolve this with the help of build system. Meson build file already has a few of examples of `cc.has_header` and `cc.has_function` and it can decide to use different source files/headers depending on whether header present or not. This effort (removing pre-processor conditionals) needs makefile to be removed first, which in turn can only happen after the release is done. But, all of that is on the near future plans. Anton, how do you feel about it? -- To view, visit https://review.coreboot.org/c/flashrom/+/77089?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic544963ffd29626ae0a21bdddb1c78850cc43ec6 Gerrit-Change-Number: 77089 Gerrit-PatchSet: 1 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Comment-Date: Fri, 08 Sep 2023 10:53:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[master]: doc: Add html_logo and html_title to sphinx config
by Anastasia Klimchuk (Code Review) 08 Sep '23

08 Sep '23

Attention is currently required from: Alexander Goncharov, Stefan Reinauer, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77438?usp=email ) Change subject: doc: Add html_logo and html_title to sphinx config ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > What's the point of displaying logo on every page? It's not a commercial logo, we're not selling any […] I guess I was trying to do the same as wiki (re displaying logo on every page). But maybe it's not necessary. You are right about alignment, it hurts eyes a little bit. From a quick look, I didn't find the easy way to change alignment (apart from providing custom css files, which I don't want to do). Maybe I will leave this patch hanging for some time, I will read documentation again to see what are all available config options to display logo. -- To view, visit https://review.coreboot.org/c/flashrom/+/77438?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a530f7143880393fffc2a97327416f2cd688586 Gerrit-Change-Number: 77438 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 08 Sep 2023 08:06:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[master]: doc: Add html_logo and html_title to sphinx config
by Alexander Goncharov (Code Review) 05 Sep '23

05 Sep '23

Attention is currently required from: Anastasia Klimchuk, Stefan Reinauer, Thomas Heijligen. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77438?usp=email ) Change subject: doc: Add html_logo and html_title to sphinx config ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I missed the elephant in the room, there is a way to add logo in sphinx config. […] What's the point of displaying logo on every page? It's not a commercial logo, we're not selling anything. And to be honest, I don't like the way it displays. Toctree is left-aligned, but the logo is centered, so it looks kind of weird. At least the "flashrom" title should be centered as well, but I have no idea whether that's possible to do via the standard sphinx configuration. -- To view, visit https://review.coreboot.org/c/flashrom/+/77438?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a530f7143880393fffc2a97327416f2cd688586 Gerrit-Change-Number: 77438 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Sep 2023 10:17:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit