Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE.
Peter Marheine has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email )
Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803
Gerrit-Change-Number: 84721
Gerrit-PatchSet: 11
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: 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: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 29 Oct 2024 08:03:09 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Aarya, Bill XIE, Peter Marheine.
Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email )
Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed
......................................................................
Patch Set 11:
(2 comments)
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/84721/comment/3b927795_124d3b60?us… :
PS10, Line 23: | [-V[V[V]]] [-o <logfile>] [--progress][--sacrifice-ratio <ratio>]
> Add a space between flags. […]
Done
File doc/release_notes/devel.rst:
https://review.coreboot.org/c/flashrom/+/84721/comment/9b4e0305_28deaaa5?us… :
PS10, Line 57: didn't need to be erased. Which means, the chip, as a hardware, will
: wear faster.
:
: New command line option sets the maximum % memory that is allowed for
: redundant erase. Default is 0, S+1 size block only selected if all the
: area needs to be erased in full. 50 means that if more than a half of
: the area needs to be erased, a S+1 size block can be selected to cover
: all area with one block.
> Small rewording suggestions. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803
Gerrit-Change-Number: 84721
Gerrit-PatchSet: 11
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: 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: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Tue, 29 Oct 2024 05:59:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE.
Anastasia Klimchuk has uploaded a new patch set (#11) to the change originally created by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email )
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed
......................................................................
erasure_layout: Add an option to sacrifice unchanged blocks for speed
The patch adds command line option to handle the following situation:
There is a region which is requested to be erased (or written, because
the write operation uses erase too). Some of the areas inside this
region don't need to be erased, because the bytes already have expected
value. Such areas can be skipped.
The logic selects eraseblocks that can cover the areas which need to be
erased. Suppose there is a region which is partially covered by
eraseblocks of size S (partially because remaining areas don't need to
be erased). Now suppose we can cover the whole region with eraseblock
of larger size, S+1, and erase it all at once. This will run faster:
erase opcode will only be sent once instead of many smaller opcodes.
However, this will run erase over some areas of the chip memory that
didn't need to be erased. Which means, the chip, as a hardware, will
wear faster.
New command line option sets the maximum % memory that is allowed for
redundant erase. Default is 0, S+1 size block only selected if all the
area needs to be erased in full. 50 means that if more than a half of
the area needs to be erased, a S+1 size block can be selected to cover
all area with one block.
The tradeoff is the speed of programming operation VS the longevity of
the chip. Default is longevity.
Change-Id: I154e8a713f626c37dbbe118db700055b96d24803
Co-developed-by: persmule <persmule(a)hardenedlinux.org
Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M cli_classic.c
M doc/classic_cli_manpage.rst
M doc/release_notes/devel.rst
M erasure_layout.c
M include/flash.h
M tests/erase_func_algo.c
6 files changed, 152 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/84721/11
--
To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803
Gerrit-Change-Number: 84721
Gerrit-PatchSet: 11
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: 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: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Matti Finder.
Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
Change subject: cli_classic: Add rpmc command support
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> Matti, wow, that's so much work that you did! Thank you so much for your contribution! […]
Oh one more thing I forgot to mention sorry... it would be great at some point to make a post on the mailing list about new feature.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?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: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 4
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 05:27:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Matti Finder.
Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: cli_classic: Add rpmc command support
......................................................................
Patch Set 4:
(8 comments)
Patchset:
PS4:
Matti, wow, that's so much work that you did! Thank you so much for your contribution!
Sorry I am joining the review a bit later. I added comments around the code, but also I have few comments to the patch in general:
1) What do you think splitting this into two commits: first can add the feature implementation (basically everything except of cli_classic).
Second would be cli client which is using new feature, also manpage update (mentioned in the other comment). Also, see next point ->
2) This cool new feature needs to be added to "Recent development", so that whoever is interested knows what to expect in the next release, the doc is here: `doc/release_notes/devel.rst`
3) You are now the author of lots of code in flashrom, what do you think about becoming a maintainer for your feature? The commitment would be only for your feature, not for everything that is happening on flashrom
We have a doc about the team https://flashrom.org/about_flashrom/team.html
(but you are welcome to get involved into anything interested for you! :)
Commit Message:
https://review.coreboot.org/c/flashrom/+/84840/comment/a1878c01_09f775c9?us… :
PS4, Line 21: usefull
usefull -> useful (typo)
(3 times in commit message :)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/84840/comment/3cedd297_ca38f78d?us… :
PS4, Line 147: #if CONFIG_RPMC_ENABLED == 1
: "RPMC COMMANDS\n"
: " --get-rpmc-status read the extended status\n"
: " --write-root-key write the root key register\n"
: " --update-hmac-key update the hmac key register\n"
: " --increment-counter <previous>\n"
: " increment rpmc counter\n"
: " --get-counter get rpmc counter\n"
: "RPMC OPTIONS\n"
: " --counter-address <number> counter address (default: 0)\n"
: " --rpmc-root-key <keyfile> rpmc root key file\n"
: " --key-data <keydata> hex number to use as key data (default: 0)\n"
: #endif // CONFIG_RPMC_ENABLED
These new commands need to also be documented on the manpage.
Manpage source is here: `doc/classic_cli_manpage.rst` and from this source we generated the manpage and also a webpage (https://flashrom.org/classic_cli_manpage.html)
https://review.coreboot.org/c/flashrom/+/84840/comment/f238b960_f90f467b?us… :
PS4, Line 1323: if (options.rpmc_read_data)
: ret = rpmc_read_data(fill_flash);
I am thinking, and maybe this is more the question to Peter, what do you think,
if we were too add this to libflashrom, would that be easy?
Currently it's only cli feature, but it shouldn't be cli only in the long term: cli should be using libflashrom API as with everything else.
In ideal world :) we would add libflashrom API first, and then cli client which is using it. Not all in one commit of course.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/84840/comment/abd265b2_4fc3aab6?us… :
PS4, Line 553: implement
I know it's a small thing, but if you start with uppercase it would look better.
https://review.coreboot.org/c/flashrom/+/84840/comment/195ab568_f5db14d5?us… :
PS4, Line 575: /*
: * all times in microsecond (us)
: */
Also small comment, but this can be 1-line comment, and starting with uppercase
`/* All times in microsecond (us) */`
https://review.coreboot.org/c/flashrom/+/84840/comment/100f756f_277db26f?us… :
PS4, Line 581: rpmc_ctx
I understand this goes into a flashchip definition? and you mentioned you tested on one chip? It would be great if you could update the definition for the chip you tested, probably better in a separate commit.
It would be a very useful as an real example of how to do it.
File meson.build:
https://review.coreboot.org/c/flashrom/+/84840/comment/daa23c24_d1227966?us… :
PS4, Line 177: if libcrypto.found()
: add_project_arguments('-DCONFIG_RPMC_ENABLED=1', language : 'c')
This ignores the meson option you added. It is auto by default, however it is possible to disable it.
Even if libcrypto found, you also need to check that `rpmc` option is enabled or auto.
For example docs are doing this, in doc/meson.build , lines 9-10 are checking the library, and then the meson option too.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?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: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 4
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 05:25:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE.
Peter Marheine has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email )
Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed
......................................................................
Patch Set 10:
(2 comments)
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/84721/comment/a3d79336_431c45f2?us… :
PS10, Line 23: | [-V[V[V]]] [-o <logfile>] [--progress][--sacrifice-ratio <ratio>]
Add a space between flags.
```suggestion
| [-V[V[V]]] [-o <logfile>] [--progress] [--sacrifice-ratio <ratio>]
```
File doc/release_notes/devel.rst:
https://review.coreboot.org/c/flashrom/+/84721/comment/905d7b53_a095bf8a?us… :
PS10, Line 57: didn't need to be erased. Which means, the chip, as a hardware, will
: wear faster.
:
: New command line option sets the maximum % memory that is allowed for
: redundant erase. Default is 0, S+1 size block only selected if all the
: area needs to be erased in full. 50 means that if more than a half of
: the area needs to be erased, a S+1 size block can be selected to cover
: all area with one block.
Small rewording suggestions.
```suggestion
didn't need to be erased. Which means the physical chip will wear out
faster.
This new option sets the maximum % of memory that is allowed for
redundant erase. Default is 0, S+1 size block only selected if all the
area needs to be erased in full. 50 means that if more than a half of
the area needs to be erased, a S+1 size block can be selected to cover
the entire area with one block.
```
--
To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803
Gerrit-Change-Number: 84721
Gerrit-PatchSet: 10
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: 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: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 29 Oct 2024 02:54:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anastasia Klimchuk, Matti Finder.
Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )
Change subject: cli_classic: Add rpmc command support
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/84840?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: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 4
Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 02:49:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Arnaud Ferraris, Maximilian Brune.
Anastasia Klimchuk has posted comments on this change by Arnaud Ferraris. ( https://review.coreboot.org/c/flashrom/+/84856?usp=email )
Change subject: linux_mtd: fix build with clang >= 19
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
File linux_mtd.c:
https://review.coreboot.org/c/flashrom/+/84856/comment/bfe82da6_8d0c4b02?us… :
PS2, Line 52: char path[sizeof(LINUX_MTD_SYSFS_ROOT) + 32];
Oh I got worried that I missed something.
Size has changed because, it's +1 longer because now it counts `\0` char, is that correct? and it's fine because the same data still fits in.
Yes, maybe 1-line in commit message:
> The result is the array init with +1 longer size, because sizeof in the case counts \0 char. (which is fine)
or something like that
--
To view, visit https://review.coreboot.org/c/flashrom/+/84856?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: If470a65702e9ae08e4303123a0014e53a1fee56e
Gerrit-Change-Number: 84856
Gerrit-PatchSet: 2
Gerrit-Owner: Arnaud Ferraris <arnaud.ferraris(a)collabora.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arnaud Ferraris <arnaud.ferraris(a)collabora.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 01:06:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Attention is currently required from: Nikolai Artemiev, Stefan Reinauer.
Victor Lim has posted comments on this change by Victor Lim. ( https://review.coreboot.org/c/flashrom/+/84827?usp=email )
Change subject: flashchips: add GD25F128F
......................................................................
Patch Set 2:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/84827/comment/4e074d5a_b88cf16e?us… :
PS1, Line 8199: OTP: 1024B total, 256B reserved
> I see that you updated, so I mark it as resolved, right? […]
thanks. resolved
--
To view, visit https://review.coreboot.org/c/flashrom/+/84827?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: I14c0905f50e92492287d50f8377790b997c4acfe
Gerrit-Change-Number: 84827
Gerrit-PatchSet: 2
Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 18:49:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Victor Lim <vlim(a)gigadevice.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Arnaud Ferraris.
Maximilian Brune has posted comments on this change by Arnaud Ferraris. ( https://review.coreboot.org/c/flashrom/+/84856?usp=email )
Change subject: linux_mtd: fix build with clang >= 19
......................................................................
Patch Set 2:
(1 comment)
File linux_mtd.c:
https://review.coreboot.org/c/flashrom/+/84856/comment/00e51fff_ab27b6e0?us… :
PS2, Line 52: char path[sizeof(LINUX_MTD_SYSFS_ROOT) + 32];
You have changed the size of the path variable. It doesn't look like to be a problem here, but a notable difference nonethless.
nit:
either keep the previous size or mention it in the commit msg.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84856?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: If470a65702e9ae08e4303123a0014e53a1fee56e
Gerrit-Change-Number: 84856
Gerrit-PatchSet: 2
Gerrit-Owner: Arnaud Ferraris <arnaud.ferraris(a)collabora.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arnaud Ferraris <arnaud.ferraris(a)collabora.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 18:41:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No