Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Angel Pons, Nikolai Artemiev, Peter Marheine, Stefan Reinauer, Thomas Heijligen.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80205?usp=email )
Change subject: doc: Add docs how to add and update test status of flashchips
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/80205?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: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec
Gerrit-Change-Number: 80205
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Sat, 27 Jan 2024 12:40:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Nikolai Artemiev, Peter Marheine, Sergii Dmytruk, Stefan Reinauer, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80205?usp=email )
Change subject: doc: Add docs how to add and update test status of flashchips
......................................................................
Patch Set 4:
(1 comment)
File doc/contrib_howtos/how_to_add_new_chip.rst:
https://review.coreboot.org/c/flashrom/+/80205/comment/b229119a_4e6d03ad :
PS3, Line 86: abc
> nit: replace with `alphabetic`?
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/80205?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: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec
Gerrit-Change-Number: 80205
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Sat, 27 Jan 2024 12:03:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Angel Pons, Nikolai Artemiev, Peter Marheine, Sergii Dmytruk, Stefan Reinauer, Thomas Heijligen.
Hello Angel Pons, Nikolai Artemiev, Peter Marheine, Sergii Dmytruk, Stefan Reinauer, Thomas Heijligen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/80205?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Code-Review+2 by Sergii Dmytruk, Code-Review+2 by Stefan Reinauer, Verified+1 by build bot (Jenkins)
The change is no longer submittable: Code-Review and Verified are unsatisfied now.
Change subject: doc: Add docs how to add and update test status of flashchips
......................................................................
doc: Add docs how to add and update test status of flashchips
For reference, link to doc about adding new chip
on the old website:
https://wiki.flashrom.org/Development_Guidelines#Adding/reviewing_a_new_fla…
Docs on how to update test status of flashchips never existed before.
Change-Id: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
A doc/contrib_howtos/how_to_add_new_chip.rst
A doc/contrib_howtos/how_to_mark_chip_tested.rst
A doc/contrib_howtos/index.rst
M doc/index.rst
4 files changed, 269 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/80205/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/80205?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: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec
Gerrit-Change-Number: 80205
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Peter Marheine, Thomas Heijligen.
Anton Samsonov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email )
Change subject: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
......................................................................
Patch Set 6:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/77778/comment/0a8fb8eb_d3f23d71 :
PS3, Line 7: 3
> Oh I see! Maybe you can rename the title as […]
Acknowledged
File doc/sphinx-wrapper.sh:
https://review.coreboot.org/c/flashrom/+/77778/comment/95a8f73a_86adf847 :
PS5, Line 8: "$@"
> `$@` means all the command-line parameters, but the parameters needed for this invocation are starting from $5 and until the end?
Yes, `$@` means all the parameters [that are still left]. The script's self path, (denoted by 'sphinx_wrapper' in meson.build), is `$0`. Then there are 2 additional parameters we capture as 'OUTPUT_HOME' and 'MAN_OUTPUTS' in the script, and then `sphinx-build` full path (denoted by 'sphinx' in meson.build), — all 3 are cut from `$@` by `shift 3`, so that only arguments for `sphinx-build` are left.
https://review.coreboot.org/c/flashrom/+/77778/comment/b11042fc_95c948aa :
PS5, Line 14:
> I know this info is in commit message…
I thought this was self-explanatory from `[ "${SPHINXBUILD_MAJOR}" -ge 4 ]`, but you are right in that there is no such thing as a over-commenting. OK, I'll add it a bit later.
--
To view, visit https://review.coreboot.org/c/flashrom/+/77778?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: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Gerrit-Change-Number: 77778
Gerrit-PatchSet: 6
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Sat, 27 Jan 2024 09:48:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anton Samsonov, Anton Samsonov, Peter Marheine, Thomas Heijligen.
Anton Samsonov has uploaded a new patch set (#6) to the change originally created by Anton Samsonov. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email )
Change subject: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
......................................................................
Makefile,meson.build: Add support for Sphinx versions prior to 4.x
As of version 3.x, `sphinx-build` outputs man pages to "8" directory
instead of "man8" expected by Makefile and doc/meson.build. See:
https://github.com/sphinx-doc/sphinx/issues/7996https://github.com/sphinx-doc/sphinx/issues/9217
Current solution is to rename "8" to "man8" after documentation build.
That enables successful build and installation, as well as dependency
tracking at build-system level, but not on `sphinx-build` own level
upon which `meson` build blindly relies.
Change-Id: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Signed-off-by: Anton Samsonov <devel(a)zxlab.ru>
---
M Makefile
M doc/meson.build
A doc/sphinx-wrapper.sh
3 files changed, 41 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/77778/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/77778?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: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Gerrit-Change-Number: 77778
Gerrit-PatchSet: 6
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anton Samsonov, Anton Samsonov, Peter Marheine, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email )
Change subject: Makefile,meson.build: Add support for Sphinx 3.x
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5:
Peter, maybe you too can have a look at the patch? Thank you so much!
--
To view, visit https://review.coreboot.org/c/flashrom/+/77778?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: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Gerrit-Change-Number: 77778
Gerrit-PatchSet: 5
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Sat, 27 Jan 2024 09:13:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Anton Samsonov, Anton Samsonov, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email )
Change subject: Makefile,meson.build: Add support for Sphinx 3.x
......................................................................
Patch Set 5:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/77778/comment/af38fdce_96f0466e :
PS3, Line 7: 3
> The patch adds support for older 3.x versions, since 4. […]
Oh I see! Maybe you can rename the title as
`Makefile,meson: Add support for Sphinx versions prior to 4.0`
so that it's more clear that it's adding backward compatibility
https://review.coreboot.org/c/flashrom/+/77778/comment/1c9505ba_03d7e996 :
PS3, Line 13: but not on `sphinx-build` own level
: upon which `meson` build relies
> > Alternatively, a symlink "8" may be created pointing at "man8" after the directory renaming occurs […]
Oh sorry I did not react on your previous message! but yes, I agree simlink makes sense, lets keep it (especially that you wrote the code already).
Patchset:
PS5:
I discovered that at the moment jenkins skips sphinx-build :\ I will try to find out why, but meanwhile
I assume you have tested on your environment (which is something prior 4.0), can you add test info into commit message (the sphinx version)?
I will also test locally, and see maybe jenkins can have sphinx build again.
File doc/sphinx-wrapper.sh:
https://review.coreboot.org/c/flashrom/+/77778/comment/7b4b0eb2_b72d3c46 :
PS5, Line 8: "$@"
I have a question, just want to understand how it works.
`$@` means all the command-line parameters, but I compare the old and new code, it seems to me that the parameters needed for this invocation of SPHINXBUILD (which is `sphinx` right?) are starting from $5 and until the end?
do I miss something?
https://review.coreboot.org/c/flashrom/+/77778/comment/3fd00061_7b322755 :
PS5, Line 14:
Can you add a comment here, saying: for sphinx versions prior to 4.0 this block renames directory from 8 to man8, since man8 expected by makefile and meson builds.
I know this info is in commit message, and this is right, but for future people looking into the code, it is useful to have a comment right here too.
--
To view, visit https://review.coreboot.org/c/flashrom/+/77778?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: I9cd280551a1ba4d17edb2e857d56f80431b61e1b
Gerrit-Change-Number: 77778
Gerrit-PatchSet: 5
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Comment-Date: Sat, 27 Jan 2024 09:12:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75646?usp=email )
Change subject: Replace all flashchips accesses with get functions
......................................................................
Patch Set 7:
(6 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/75646/comment/f0206653_74431d8c :
PS7, Line 7: Replace all flashchips accesses with get functions
If you are in a good mood, you can add `tree: ` prefix to commit title, which we usually do, so that prefix gives an idea what is changing in commit.
This is a big change across the tree.
Patchset:
PS7:
Great, thank you!
The build error seems legit, and when it's fixed, unit tests will also run. Which is what we want, especially the selfcheck unit test is very relevant.
Did you run tests locally?
Also I am thinking whether unit tests have coverage for probing, I need to check. If not, I will try to add a unit test which does probing (with dummy). This would be useful, in addition to selfcheck. But that would be separate patch.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/9f6c3fd5_d8fbead3 :
PS7, Line 1936: * They are all defined as externs in this compilation unit so we don't know their sizes which vary
: * depending on compiler flags, e.g. the target architecture, and can sometimes be 0.
: * For 'flashchips' we export the size explicitly to work around this and to be able to implement the
: * checks below.
I think this comment needs to be rewritten.
`They are all defined as externs in this compilation unit` does not fully apply, and `'flashchips' we export the size explicitly` neither.
I still want some comment, but I want it to be correct. For the rest of constant arrays situation hasn't changed, but for flashchips it had.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/ef0caac1_d476710b :
PS7, Line 1472: onst struct flashchip *chip = get_flashchips()[0];
Jenkins complains on this, seems like a legit build error:
> ichspi.c:1472:40: error: incompatible types when initializing type 'const struct
flashchip *' using type 'struct flashchip'
1472 | const struct flashchip *chip = get_flashchips()[0];
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/e46469f3_ea194950 :
PS7, Line 98: return NULL;
Could you please add `msg_gerr` before returning here, too
File tests/selfcheck.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/7554d0b8_1a450006 :
PS7, Line 80: flashchips = get_flashchips();
: flashchips_size = get_flashchips_size();
Can this be immediately initialised (like the above?)
--
To view, visit https://review.coreboot.org/c/flashrom/+/75646?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: Ibc89e32c83975e01c958b8cf0d11dad73c461a53
Gerrit-Change-Number: 75646
Gerrit-PatchSet: 7
Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sat, 27 Jan 2024 07:48:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan.
Hello Anastasia Klimchuk, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/75646?usp=email
to look at the new patch set (#6).
Change subject: Replace all flashchips accesses with get functions
......................................................................
Replace all flashchips accesses with get functions
Abstract access to struct flashchip flashchips[].
Change-Id: Ibc89e32c83975e01c958b8cf0d11dad73c461a53
Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
---
M cli_classic.c
M flashchips.c
M flashrom.c
M ichspi.c
M include/flash.h
M libflashrom.c
M print.c
M print_wiki.c
M tests/selfcheck.c
9 files changed, 48 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/75646/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/75646?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: Ibc89e32c83975e01c958b8cf0d11dad73c461a53
Gerrit-Change-Number: 75646
Gerrit-PatchSet: 6
Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75646?usp=email )
Change subject: Replace all flashchips accesses with get functions
......................................................................
Patch Set 5:
(15 comments)
Patchset:
PS5:
> I realise this patch was a intermediate step so the comments are just how it should probably look fo […]
Done
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/31223b9d_eaf0ba69 :
PS5, Line 28: * Temporarily, this file is sorted alphabetically by vendor and name to
: * assist with merging the Chromium fork of flashrom.
> Can we remove this comment by now? (maybe in a separate patch). […]
Done
https://review.coreboot.org/c/flashrom/+/75646/comment/afb19e6f_1f31d463 :
PS5, Line 34: internal_
> I agree that "internal" almost always means internal programmer. It is flashromspeak :) […]
Done
https://review.coreboot.org/c/flashrom/+/75646/comment/79a33fb2_da183bd0 :
PS5, Line 20844: get_flashchips(void)
> Where would the caller get the pointer to flashchip db? and why the caller needs to have the pointer […]
Any code that needs to access the supported flashchips would use this function instead of accessing the array directly. This allows to later load the database of supported flashchips dynamically rather than hard coding it in the binary.
https://review.coreboot.org/c/flashrom/+/75646/comment/b58c418a_34e7a568 :
PS5, Line 20849: unsigned int
> maybe so. it was unsigned int before, so i didn't change it. […]
Done
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/217b37be_b2fa90d2 :
PS5, Line 1099: return -1;
> Maybe you can add `msg_gerr` before returning. […]
Done
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/cc185715_06991b98 :
PS5, Line 1469: const struct flashchip *chip;
:
: struct flashchip *flashchips=get_flashchips();
:
: if (!flashchips)
: return NULL;
> very clever. and doesn't check the return value. […]
Done
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/ad9a70ce_cf7e3c48 :
PS5, Line 95: unsigned int
> shouldn't then i downstairs also be size_t? not against this, but it seems that is a larger consiste […]
Done
File print.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/e4138b87_af71dddb :
PS5, Line 56: const struct flashchip *chip;
> delete
This won't work because we walk the whole array twice and chip is modified the first time.
https://review.coreboot.org/c/flashrom/+/75646/comment/1cf56450_5101611f :
PS5, Line 62: flashchips
> chip
ditto
https://review.coreboot.org/c/flashrom/+/75646/comment/9a2e313e_0260c79b :
PS5, Line 65: chip = flashchips
> delete
ditto.
File print_wiki.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/a2bdff40_14e6d3e5 :
PS5, Line 294: flashchips
> get_flashchips()
Given that get_flashchips() might do more in the future, it doesn't make sense to optimize two lines of code saved by making two function calls in my opinion.
https://review.coreboot.org/c/flashrom/+/75646/comment/c12e89f3_e7f98e63 :
PS5, Line 310: flashchips
> get_flashchips()
ditto
File tests/selfcheck.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/94c28057_65adc2ea :
PS5, Line 80: /* unused */
> But it is used two lines below?
Done
https://review.coreboot.org/c/flashrom/+/75646/comment/83be1367_b595402e :
PS5, Line 83: flashchips = get_flashchips();
: flashchips_size = get_flashchips_size();
> Can this be immediately initialised? (and the same above). This is what non-test code is doing.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/75646?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: Ibc89e32c83975e01c958b8cf0d11dad73c461a53
Gerrit-Change-Number: 75646
Gerrit-PatchSet: 5
Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 26 Jan 2024 21:57:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment