Attention is currently required from: Anastasia Klimchuk, Bill XIE, Peter Marheine.
Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
......................................................................
Patch Set 5:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/7b02b334_dbc4c0c5?us… :
PS1, Line 1843:
: static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
: {
: return find_opcode(curopcodes, opcode) >= 0;
> Merging the masters seems fine to me. […]
Oh, ignore my comment about rebasing on CB:84593, I see it's already the parent of this patch. I think I was confused by looking at an older patchset before.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 5
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
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: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Fri, 04 Oct 2024 07:13:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Bill XIE.
Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84595?usp=email )
Change subject: ichspi: const-correct POSSIBLE_OPCODES[]
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84595?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: I217f8a9e50b9e2e9f2731adec89a46780874c754
Gerrit-Change-Number: 84595
Gerrit-PatchSet: 1
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 07:12:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Anastasia Klimchuk, Bill XIE.
Nikolai Artemiev has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84567?usp=email )
Change subject: ichspi: Change the opcode position for reprogramming on the fly 2->4
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84567?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: I6bc855daedf0af2e8de191f23a3512de3ebc3fef
Gerrit-Change-Number: 84567
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 04 Oct 2024 04:24:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Anastasia Klimchuk, Bill XIE, Peter Marheine.
Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/ca6d3f7b_b5164420?us… :
PS1, Line 1843:
: static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
: {
: return find_opcode(curopcodes, opcode) >= 0;
> I managed to make up a solution to program an erase opcode being probed if it is in the POSSIBLE_OPC […]
Merging the masters seems fine to me. I have reviewed CB:84593 and I'll submit it if there are no objections soon. You can rebase this patch on top of that one.
Also, I assume you originally encountered this problem on an ICH9 device, is that correct? ICH7 shouldn't have had this problem since it didn't have probe_opcode, so flashrom would just assume that all opcodes (including SE) are allowed.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 5
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
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: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Fri, 04 Oct 2024 04:19:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
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:
(1 comment)
Patchset:
PS3:
This looks good to me, it seems like probe_opcode was just accidentally left out of ich7.
--
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:07:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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>