Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78985?usp=email )
Change subject: tests: Add test cases for erasing/writing unaligned layout regions
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce
Gerrit-Change-Number: 78985
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Sun, 19 Nov 2023 23:13:43 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Aarya, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78984?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: erasure_layout: Fix unaligned region end offset by 1
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS1:
> The open question here is whether anything is needed for start region branch. […]
I did more investigation to understand why no fix is needed for the case when start of the region is extended, and yes it seems no fix is needed for start region.
The reason is that the functions used to handle the extension of the region boundaries, ours `read_flash` and not ours `memcpy`, they both are "start-inclusive".
Now, for both start and end boundaries extension, diff is calculated correctly. But then, there is the code which handles the extended area, this area is outside of the initial layout. When start boundary is extended, the code works because `read_flash` and `memcpy` start from the first byte of extended area and go through its length. However, when end boundary is extended, we don't need to start from the last byte of the initial region, we need to start from the first byte after the initial region. Therefore +1 is needed for the end boundary processing.
Patchset:
PS2:
After some investigation I came to conclusion that no fix is needed for start boundary extension (see my other comment). So I won't be adding more to this patch, unless there are other comments of course.
Joursoir, is it possible for you to test the patch on the images that you initially found the bug in ticket 494? It would be great, since you initially reported the bug, so that you can confirm it is fixed.
--
To view, visit https://review.coreboot.org/c/flashrom/+/78984?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I7f78a0090065cd2a952cba1a5d28179483ba4c55
Gerrit-Change-Number: 78984
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Sat, 18 Nov 2023 10:36:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Aarya, Alexander Goncharov, Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78985?usp=email )
Change subject: tests: Add test cases for erasing/writing unaligned layout regions
......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS4:
I have modified test case #16 to heavily test extending the start of the unaligned region. Other test cases in this patch cover extending the end of unaligned region, and #19 has logical layout which is smaller than the total size of chip.
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/78985/comment/abcb5b7a_f626eb52 :
PS2, Line 30: /* How many small chip test cases. */
> Drop the comment too. […]
Oh I completely missed! Done.
--
To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce
Gerrit-Change-Number: 78985
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Sat, 18 Nov 2023 09:58:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Peter Marheine.
Hello Aarya, Alexander Goncharov, Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/78985?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Code-Review+2 by Peter Marheine, Verified+1 by build bot (Jenkins)
Change subject: tests: Add test cases for erasing/writing unaligned layout regions
......................................................................
tests: Add test cases for erasing/writing unaligned layout regions
The test cases from #16 onwards cover the case when layout region is
not aligned with eraseblock region, and therefore either layout start
boundary or end boundary needs to be extended to align with
eraseblock.
Ticket: https://ticket.coreboot.org/issues/494
Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M tests/erase_func_algo.c
1 file changed, 118 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/78985/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce
Gerrit-Change-Number: 78985
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: newpatchset
Name of user not set #1005208 has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/79112?usp=email )
Change subject: serial: Fix sp_flush_incoming for serprog TCP connections
......................................................................
serial: Fix sp_flush_incoming for serprog TCP connections
I was working on an esp32 serprog-compatible SPI programmer pet project,
and when i implemented the TCP over Wi-Fi for serprog,
i discovered that sp_flush_incoming() silently fails
if the underlying sp_fd descriptor is a TCP socket.
I added a check for this case - tcflush returns ENOTTY,
meaning tcflush is not supported for not terminal objects,
in this case a fallback serialport_read_nonblock loop is used.
After that i was able to communicate with my serprog programmer implementation over TCP with 100% success rate.
Signed-off-by: Stanislav Ponomarev <me(a)stasponomarev.com>
Change-Id: I9724a2fcd4a41dede2c15f83877efa6c3b0b7fae
---
M serial.c
1 file changed, 19 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/79112/1
diff --git a/serial.c b/serial.c
index 10d739a..f9bd280 100644
--- a/serial.c
+++ b/serial.c
@@ -377,8 +377,25 @@
#if IS_WINDOWS
PurgeComm(sp_fd, PURGE_RXCLEAR);
#else
- /* FIXME: error handling */
- tcflush(sp_fd, TCIFLUSH);
+ int ret = tcflush(sp_fd, TCIFLUSH);
+ if (ret == 0)
+ return;
+
+ // TCP socket case: sp_fd is not a terminal descriptor - tcflush is not supported
+ if (errno == ENOTTY)
+ {
+ unsigned char c;
+
+ while ((ret = serialport_read_nonblock(&c, 1, 1, NULL)) == 0);
+
+ if (ret > 0) // no more data available
+ return;
+
+ msg_perr("Could not flush serial port incoming buffer: read has failed");
+ return;
+ }
+
+ msg_perr_strerror("Could not flush serial port incoming buffer: ");
#endif
return;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/79112?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I9724a2fcd4a41dede2c15f83877efa6c3b0b7fae
Gerrit-Change-Number: 79112
Gerrit-PatchSet: 1
Gerrit-Owner: Name of user not set #1005208
Gerrit-MessageType: newchange
Attention is currently required from: Anton Samsonov, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77089?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: Remove dependency on C23 __has_include()
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Sorry for such a long delay in response, but finally I got back to this patch, and wow it looks so much better, thank you! :)
I only want to ask add testing info in commit message. I understand you have tested both makefile and meson, but better have the info in commit message. Also if you have tested on different environments (with or without headers), you can list environments that you tested on.
--
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: main
Gerrit-Change-Id: Ic544963ffd29626ae0a21bdddb1c78850cc43ec6
Gerrit-Change-Number: 77089
Gerrit-PatchSet: 4
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Comment-Date: Fri, 17 Nov 2023 10:05:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment