January 2024 - flashrom-gerrit

[L] Change in flashrom[main]: doc: Add docs how to add and update test status of flashchips
by Sergii Dmytruk (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[L] Change in flashrom[main]: doc: Add docs how to add and update test status of flashchips
by Anastasia Klimchuk (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[L] Change in flashrom[main]: doc: Add docs how to add and update test status of flashchips
by Anastasia Klimchuk (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[S] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
by Anton Samsonov (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[S] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
by Anton Samsonov (Code Review) 27 Jan '24

27 Jan '24

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/7996 https://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

1 0

[S] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx 3.x
by Anastasia Klimchuk (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[S] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx 3.x
by Anastasia Klimchuk (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[M] Change in flashrom[main]: Replace all flashchips accesses with get functions
by Anastasia Klimchuk (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[M] Change in flashrom[main]: Replace all flashchips accesses with get functions
by Stefan Reinauer (Code Review) 27 Jan '24

27 Jan '24

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

1 0

[M] Change in flashrom[main]: Replace all flashchips accesses with get functions
by Stefan Reinauer (Code Review) 27 Jan '24

27 Jan '24

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

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2024