Attention is currently required from: Bill XIE.
Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84593?usp=email )
Change subject: ichspi: Merge spi_master implementations for Intel ich
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84593?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I6a65c97e910622a55da7cef8a10de3af6a99c9e8
Gerrit-Change-Number: 84593
Gerrit-PatchSet: 3
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Comment-Date: Fri, 04 Oct 2024 04:03:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Aarya, Anastasia Klimchuk.
Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email )
Change subject: erasure_layout: Erase larger block only when all sub-block need erase
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
After CB:84253 gets nearly completed, another bug introducing erase overheads surfaces. Fortunately, this one is hardware-independent.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84614?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I9e10749186e395da67ec80e296119f33c3f83122
Gerrit-Change-Number: 84614
Gerrit-PatchSet: 1
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 01 Oct 2024 14:42:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Aarya.
Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email )
Change subject: erasure_layout: Erase larger block only when all sub-block need erase
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/84614/comment/eccd8c79_ce0529d4?us… :
PS1, Line 15: More severely, since this logic only selects a block and delects
: its sub-blocks when half of sub-blocks need erase, but this logic
: does not deselect "double sub-blocks (sub-blocks of sub-block)" not
: reach the limit under this block, the logic may cause duplicated
: erase.
Test case for this could be generate with the following shell script:
>#!/bin/sh
>dd if=/dev/urandom bs=16M count=1 of=0.rom
>cp 0.rom 1.rom
>#! Overwrite first 130 64k blocks to exceed half
>dd if=/dev/urandom bs=64k count=130 conv=notrunc of=1.rom
>#! Overwrite last 2 4k blocks
>dd if=/dev/urandom bs=4k count=2 seek=4094 conv=notrunc of=1.rom
Test with:
> $ /path/to/flashrom -p dummy:emulate=W25Q128FV,image=chip.rom -Vw 0.rom
$ /path/to/flashrom -p dummy:emulate=W25Q128FV,image=chip.rom -Vw 1.rom
Result:
> Reading old flash chip contents... read_flash: region (00000000..0xffffff) is readable, reading range (00000000..0xffffff).
done.
erase_write: region (00000000..0xffffff) is writable, erasing range (00000000..0xffffff).
0xffe000..0xffefff verify_range: Verifying region (00000000..0xffffff)
read_flash: region (00000000..0xffffff) is readable, reading range (0xffe000..0xffefff).
E(ffe000:ffefff)0xfff000..0xffffff verify_range: Verifying region (00000000..0xffffff)
read_flash: region (00000000..0xffffff) is readable, reading range (0xfff000..0xffffff).
E(fff000:ffffff)000000..0xffffff verify_range: Verifying region (00000000..0xffffff)
read_flash: region (00000000..0xffffff) is readable, reading range (00000000..0xffffff).
E(0:ffffff)write_flash: region (00000000..0xffffff) is writable, writing range (00000000..0xffffff).
W(0:ffffff)Erase/write done from 0 to ffffff
Verifying flash... read_flash: region (00000000..0xffffff) is readable, reading range (00000000..0xffffff).
VERIFIED.
Writing chip.rom
(ffe000:ffefff) and (fff000:ffffff) get erased twice, and (820000:ffdfff) also gets erased but should not. This commit will fix this.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84614?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I9e10749186e395da67ec80e296119f33c3f83122
Gerrit-Change-Number: 84614
Gerrit-PatchSet: 1
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2024 14:05:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Patch Set 6:
(3 comments)
File cli_output.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/fd006211_ba7b831f?us… :
PS4, Line 119: 16 * FLASHROM_PROGRESS_NR
> For 100% it will be 13 chars which I rounded to 16. `\b \b` is: […]
Oh this is a backspace that is not deleting characters! Sorry I got so confused.
I made an attempt to introduce the flag. It is a bit sad that I added a global state, but it seems this is how cli_output currently works :\
To test , running the same commands as is in your commit message will backspace and print on the top. But when I added -VVV , I got lots of messages in between progress indicator %, so I could see in debugger that new line is printed instead of backspace.
(the terminal explodes when you give -VVV to dummy, it prints every byte)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/39344173_d76e25fd?us… :
PS5, Line 1235: /* const size_t flash_size = flashctx->chip->total_size * 1024; */
: /* const size_t page_size = flashctx->chip->page_size; */
> Sure.
Done
https://review.coreboot.org/c/flashrom/+/84102/comment/5eeadbe0_4a7edd57?us… :
PS5, Line 1958: init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); // WTF?
> I think it was before addressing progress reporting on SPI reads, so adding this line caused progres […]
Okay I removed the comment.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 6
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Tue, 01 Oct 2024 13:29:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev.
Anastasia Klimchuk has uploaded a new patch set (#6) to the change originally created by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Complete and fix progress feature implementation for all operations
Original progress reporting implemented in CB:49643 and it has some
issues, for example:
size_t start_address = start;
size_t end_address = len - start;
End address is anything but length minus start address.
update_progress(flash,
FLASHROM_PROGRESS_READ,
/*current*/ start - start_address + to_read,
/*total*/ end_address);
Total should just be length if that's how current value is computed.
---
libflashrom needs to know total size ahead of time.
That's init_progress() and changed update_progress().
It also needs to store the last current value to be able to update it.
That's stage_progress in flashrom_flashctx.
Measuring accurately amount of data which will be read/erased/written
isn't easy because things can be skipped as optimizations. The next
patch in the chain aims to address this, there are TODO/FIXME
comments there.
---
CLI shares terminal with the rest of the code and has to maintain more
state to handle that reasonably well.
The progress doesn't just dump lots of stuff on the screen which
probably won't fly because of CB:64668, but it's not hard to adjust
this.
---
A script to test the CLI:
\#!/bin/bash
t=${1:-rewW}
shift
if [[ $t =~ r ]]; then
echo ">>> READ"
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -r dump.rom --progress "$@"
echo
fi
if [[ $t =~ e ]]; then
echo ">>> ERASE"
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -E --progress "$@"
echo
fi
if [[ $t =~ w ]]; then
echo ">>> WRITE (without erase)"
dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -w zero.rom --progress "$@"
echo
fi
if [[ $t =~ W ]]; then
echo ">>> WRITE (with erase)"
dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null
dd if=/dev/random of=random.rom bs=1M count=16 2> /dev/null
./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz,image=random.rom -w zero.rom --progress "$@"
echo
fi
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M 82802ab.c
M at45db.c
M cli_classic.c
M cli_output.c
M dediprog.c
M doc/release_notes/devel.rst
M en29lv640b.c
M erasure_layout.c
M flashrom.c
M include/flash.h
M it87spi.c
M jedec.c
M libflashrom.c
M linux_mtd.c
M parade_lspcon.c
M realtek_mst_i2c_spi.c
M spi.c
M spi25.c
M sst28sf040.c
M tests/chip.c
M tests/spi25.c
M tests/tests.c
M tests/tests.h
23 files changed, 296 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/02/84102/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 6
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Bill XIE has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email )
Change subject: erasure_layout: Erase larger block only when all sub-block need erase
......................................................................
erasure_layout: Erase larger block only when all sub-block need erase
A larger (not the smallest) erase block used to get erased when half
of sub-blocks it contains need erase, which has at least 2 issues:
1. The rest half of sub-blocks that do not need erase are also erased,
introducing some erase overheads.
2. More severely, since this logic only selects a block and delects
its sub-blocks when half of sub-blocks need erase, but this logic
does not deselect "double sub-blocks (sub-blocks of sub-block)" not
reach the limit under this block, the logic may cause duplicated
erase. For example, if a erase block (often the largest one
corresponding to the whole chip) has half of its sub-blocks and
some incontiguous double sub-blocks needing erase, these double
sub-blocks will end up being erased twice, introducing even more
erase overheads than whole-chip erase.
The older behavior of flashrom before adding erasure_layout.c, when no
communicational error occurs, will neither erase blocks that do not
need erase, nor cause duplicated erase. Higher efficiency should be
achieved without introducing extra erase overheads, by allowing
combining contiguous small erase blocks only when they can
coincidently form a larger erase block.
Change-Id: I9e10749186e395da67ec80e296119f33c3f83122
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
---
M erasure_layout.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/84614/1
diff --git a/erasure_layout.c b/erasure_layout.c
index c3a415b..56841ab 100644
--- a/erasure_layout.c
+++ b/erasure_layout.c
@@ -228,7 +228,7 @@
}
const int total_blocks = sub_block_end - sub_block_start + 1;
- if (count && count > total_blocks/2) {
+ if (count == total_blocks) {
if (ll->start_addr >= rstart && ll->end_addr <= rend) {
for (int j = sub_block_start; j <= sub_block_end; j++)
layout[findex - 1].layout_list[j].selected = false;
--
To view, visit https://review.coreboot.org/c/flashrom/+/84614?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I9e10749186e395da67ec80e296119f33c3f83122
Gerrit-Change-Number: 84614
Gerrit-PatchSet: 1
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Attention is currently required from: Balázs Vinarz, Keith Hui, Nico Huber.
Anastasia Klimchuk has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/flashrom/+/23053?usp=email )
Change subject: Fwd: Software info for Willem/EZOflash
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2:
> have the user specify the protocol via programmer options
If this is fine for the user, this is a good option. Would the user know which protocol they need? For example, if you were the user?
dummyflasher.c can initialise more than one bus, and the desired option is set via programmer param. dummyflasher is an emulator-programmer, but it will do as an example how to init one bus or the other depending on the param.
Also, this can be done step-by-step, doesn't have to be in one commit. First commit adds programmer and support for spi. Second commit can add programmer param, and more options (parallel). If default option stays the same (default what happens without param provided), this should be fine.
Adding new programmer will be a large commit already, and lots of work, I think planning multiple commits will help.
File meson.build:
https://review.coreboot.org/c/flashrom/+/23053/comment/071e5f0a_231e3b68?us… :
PS2, Line 500:
> we might need to use the ezo here as well to maintain consistency, but that wouldn't mention that th […]
yes, let's use `ezo`
it seems it can be made to work with not only SPI
--
To view, visit https://review.coreboot.org/c/flashrom/+/23053?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Idc4d98593076c80fd1d6ac4596940d1d7977343c
Gerrit-Change-Number: 23053
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Balázs Vinarz <vinibali1(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-CC: Keith Hui <buurin(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Balázs Vinarz <vinibali1(a)gmail.com>
Gerrit-Attention: Keith Hui <buurin(a)gmail.com>
Gerrit-Comment-Date: Tue, 01 Oct 2024 05:26:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Balázs Vinarz <vinibali1(a)gmail.com>
Comment-In-Reply-To: Keith Hui <buurin(a)gmail.com>