April 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: tests: Add unit test to run init/shutdown for linux_spi.c
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52498 ) Change subject: tests: Add unit test to run init/shutdown for linux_spi.c ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52498 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4911fbb6f04371283f0e62d2196bdd691a227584 Gerrit-Change-Number: 52498 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 07:47:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for dummyflasher.c
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52497 ) Change subject: tests: Add unit test to run init/shutdown for dummyflasher.c ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/52497/comment/da71e440_574114d0 PS2, Line 9: I plan to do this for all drivers in programmer table, : one at a time. Dummy is the first and it does not need : mocks. : This explains a overarching strategy however is spare on the detail of the commit itself. May I recommend something along the lines of; ``` " Introduce test to exercise that init and teardown of drivers correctly manage the drivers life-time state. We constrain ourselves to dummyflasher in particular here as it does not need any mocking. " ``` https://review.coreboot.org/c/flashrom/+/52497/comment/a42a2a1a_3a233fdf PS2, Line 13: After this was rebased on the top of memory checks : https://review.coreboot.org/c/flashrom/+/51243 , : dummy has been discovered to leak memory on shutdown, : so I fix it here. separate patch as it is not related to the introduction of the test code itself just that a previously introduced test found a bug. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/52497/comment/060d6f22_432457ff PS2, Line 653: free(data); Fix the memory leak in a separate patch as not to be conflated with the introduction of test code. The fix stands upon it's own merit. -- To view, visit https://review.coreboot.org/c/flashrom/+/52497 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3c0ef73397f00c1db7aabb5f9f00cb43525af29c Gerrit-Change-Number: 52497 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 07:44:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: implement set_wp_region operation
by Nikolai Artemiev (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Daniel Campello. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52531 ) Change subject: cli_classic.c: implement set_wp_region operation ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52531 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibad68a038ab38b9986b0d8b5f5eb6c73b20ef381 Gerrit-Change-Number: 52531 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 04:17:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: implement set_wp_region operation
by Nikolai Artemiev (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Daniel Campello. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52531 ) Change subject: cli_classic.c: implement set_wp_region operation ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Hey Nikolai, could you take a look over this please as it overlaps with your writeprotect work. I don't think there's any harm in adding this, the external writeprotect.c interface should be fairly stable. But we should hold off actually enabling wp functionality while writeprotect.c is being rewritten. -- To view, visit https://review.coreboot.org/c/flashrom/+/52531 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibad68a038ab38b9986b0d8b5f5eb6c73b20ef381 Gerrit-Change-Number: 52531 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 04:17:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c
by Anastasia Klimchuk (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c ...................................................................... Patch Set 6: (5 comments) Patchset: PS2: > I split this into 5 patches, and first three are ready, these ones: […] And now all 5 patches are ready. Patchset: PS6: thank you for reviews! File hwaccess_io_unittest.h: PS2: > Move into `tests/`? This file is referenced in meson.build in root directory (and the same situation is with unittest_env.h) and I am worried that root build file would depend on the file in tests/ (other way around is normal, tests depend on root). What do you think about it? If you say this is fine, I can move into tests/ https://review.coreboot.org/c/flashrom/+/51487/comment/c0ab8615_898a68f5 PS2, Line 24: #ifndef __HWACCESS_IO_H__ > Maybe leave a comment that we intentionally use the same guard. Done File tests/meson.build: https://review.coreboot.org/c/flashrom/+/51487/comment/6218850f_e861c5c6 PS2, Line 36: '-Wl,--wrap=test_outb', : '-Wl,--wrap=test_inb', : '-Wl,--wrap=test_outw', : '-Wl,--wrap=test_inw', : '-Wl,--wrap=test_outl', : '-Wl,--wrap=test_inl', > I guess as these are not implemented anywhere, we don't need to "wrap" […] I would prefer to have everything as wraps, then from the point of view of someone working on tests all wraps are the same, they all can return mock() etc etc. If half of functions is wraps, and other half is test_ then for someone not familiar the question is: what is the difference? But there is no difference. -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 04:11:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c
by Anastasia Klimchuk (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Hello build bot (Jenkins), Nico Huber, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/51487 to look at the new patch set (#6). Change subject: tests: Add unit test to run init/shutdown for mec1308.c ...................................................................... tests: Add unit test to run init/shutdown for mec1308.c This patch includes mocks for io operations in hwaccess_io.h because those are needed to test lifecycle of mec1308.c BUG=b:181803212 TEST=builds and ninja test Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- A hwaccess_io_unittest.h M meson.build M tests/init_shutdown.c M tests/meson.build M tests/tests.c M tests/tests.h 6 files changed, 137 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/51487/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: hwaccess.c: Fix PowerPC memory barrier
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Stefan Reinauer, Angel Pons, Arthur Heymans. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52470 ) Change subject: hwaccess.c: Fix PowerPC memory barrier ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52470/comment/0a7a1c48_15ce5019 PS1, Line 10: rotten link. > My best guess: CB:47908 That seems extremely likely Angel, good spot! It is very unfortunate the toolchain defaults of a given release are being leaned on leaning to a build up of brokeness being papered over. Nico, do you happen to have a SPARC box or similar to ssh into for investigation? I suspect we have sadly accrued an eclectic series of pitfalls over time due to the missing toolchain constraints. It would surely nice to see these fixed. Stefan, is it somehow possible to get a SPARC box off ebay and include it as a buildbot? I don't know how that side of the infra works? Google is certainly a hard environment to work outside the confines of the world being entirely x86+ARM just because of the realities no matter how well intended the developer is. -- To view, visit https://review.coreboot.org/c/flashrom/+/52470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Gerrit-Change-Number: 52470 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 20 Apr 2021 03:00:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: s25f.c: Fix mismatched function definitions
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Angel Pons, Arthur Heymans. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52473 ) Change subject: s25f.c: Fix mismatched function definitions ...................................................................... Patch Set 1: Code-Review-2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52473/comment/7aa176bd_f344cbb9 PS1, Line 8: : This was missed because `uint32_t` is `unsigned int` in most cases. : However, it is not the case for DJGPP 6.1.0 for some reason. this does not sit right with me. I believe the correct fix here is to simply correct the prototypes in chipdrivers.h which seem to be wrong: ``` git grep -E '(s25fs_block_erase_d8|s25fl_block_erase)' chipdrivers.h chipdrivers.h:int s25fl_block_erase(struct flashctx *flash, unsigned int addr, unsigned int blocklen); chipdrivers.h:int s25fs_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen); ``` uint32_t seems to be the correct type. -- To view, visit https://review.coreboot.org/c/flashrom/+/52473 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I656a72b85d4c70b57f6ff9268186a4a60933f8a9 Gerrit-Change-Number: 52473 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:49:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Makefile: Explicitly set '-std=c99'"
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Stefan Reinauer, Angel Pons, Arthur Heymans, Anastasia Klimchuk, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52425 ) Change subject: Revert "Makefile: Explicitly set '-std=c99'" ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > My understanding is: if original commit breaks so many things then it needs to be reverted now. […] I agree. However, we need to get to the bottom of this as having non-explicit compiler arguments is papering over a deep fundamental issue where "working" essentially equates to "by luck" which, obviously, isn't a good mode of operation. -- To view, visit https://review.coreboot.org/c/flashrom/+/52425 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I57b5125207de3fd156dface67cba605da893d6aa Gerrit-Change-Number: 52425 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:44:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Makefile: Explicitly set '-std=c99'"
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Stefan Reinauer, Angel Pons, Arthur Heymans, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52425 ) Change subject: Revert "Makefile: Explicitly set '-std=c99'" ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52425 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I57b5125207de3fd156dface67cba605da893d6aa Gerrit-Change-Number: 52425 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:42:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021