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

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Nico Huber (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Thank you, this is exactly what I wanted to know! […] As you can see from the amount of tests, it's a rather new topic for flashrom. As long as your work has no or only a marginal effect on the actual program code, don't expect any strong opinions :) Personally, I think testing graceful termination, same behavior from multiple calls and things like memory leaks is a very good idea. There are other things where I have mixed feelings: For instance flashbuses_to_text_test_success() needs knowledge about exact behavior. This can increase the maintenance burden (i.e. when the strings change, the test needs to be changed too). -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: 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, 23 Mar 2021 01:15:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Anastasia Klimchuk (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I usually give a +1 when I haven't noticed anything blatantly wrong with a change, but I'm not sure […] Thank you, this is exactly what I wanted to know! Splitting into smaller pieces would not be hard, not a problem at all. Dummy goes first, and then every next test can be a separate patch. -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 23 Mar 2021 00:30:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Enable dynamic memory allocation checks for cmocka unit tests
by Nico Huber (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: David Hendricks, Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 ) Change subject: Enable dynamic memory allocation checks for cmocka unit tests ...................................................................... Patch Set 2: Code-Review+1 (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/51243/comment/dce00df4_ebcd51e1 PS2, Line 10: files and tests. Nit. If these are paragraphs, split them with empty lines. If they aren't, don't wrap the lines after the full stop. Patchset: PS2: The preprocessor solution to override stdlib functions looks a bit fragile. If an inline function in another header file would use malloc(), would that still be caught? There may be a way out by always including `stdlib.h` first and then `unittest_env.h` before anything else. That would be easy from the command line with `-include` directives. And it wouldn't need `UNIT_TEST_ENV`. File tests/flashrom.c: https://review.coreboot.org/c/flashrom/+/51243/comment/e435f40d_3c35633e PS2, Line 27: #define assert_string_not_equal_with_free(text, expected) \ NB. I known this is probably just following cmocka, but I find it very hard to visually distinguish the two assert_string_equal_with_free assert_string_not_equal_with_free The longer the boilerplate around the *not* becomes, the harder it is to "see" it (without fully reading the identifier). -- To view, visit https://review.coreboot.org/c/flashrom/+/51243 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f Gerrit-Change-Number: 51243 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com> Gerrit-Reviewer: David Hendricks <david.hendricks(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: Daniel Kurtz <djkurtz(a)chromium.org> Gerrit-CC: Victor Ding <victording(a)chromium.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 23 Mar 2021 00:27:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Angel Pons (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Hello! Just to check on this: with a +1 can I assume this is moving to the right direction, and I ca […] I usually give a +1 when I haven't noticed anything blatantly wrong with a change, but I'm not sure enough about it to give a +2. In this case, I don't know enough about testing to give a +2 without having heard from others first. But yes, I'd say this is moving in the right direction and I'd look into adding more tests. If the change would grow very large, I'd split it into several smaller changes. -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 23 Mar 2021 00:10:03 +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]: Add unit test to run init/shutdown for enabled drivers
by Anastasia Klimchuk (Code Review) 23 Mar '21

23 Mar '21

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Hello! Just to check on this: with a +1 can I assume this is moving to the right direction, and I can go ahead and add init/shutdown tests for all the other drivers in programmer table? I am looking forward to that! Or in other words: if I add tests for all enabled drivers, would that help to *consider* turning +1 into +2 ? Also I plan to build more on the top of this. For example, first thing if this comes through will be to run init/shutdown twice (means init->shutdown->init->shutdown). And, combining this with https://review.coreboot.org/c/flashrom/+/51243 . Thank you! -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Mon, 22 Mar 2021 23:57:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Nico Huber (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Patch Set 1: (1 comment) File mec1308.c: https://review.coreboot.org/c/flashrom/+/51487/comment/77005351_b647cc0b PS1, Line 115: #endif /* UNIT_TEST_ENV */ I know it won't be as easy, but I'd much prefer such mocks in separate compilation units. Instead of the macros, you'd have to provide implementations for inb() and outb(), I guess. -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 22 Mar 2021 23:44:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog.c: Split up compound conditional statement
by Nico Huber (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51706 ) Change subject: dediprog.c: Split up compound conditional statement ...................................................................... Patch Set 1: (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/51706/comment/64012864_48848732 PS1, Line 1276: if (register_spi_master(&spi_master_dediprog)) : return 1; : : return dediprog_set_leds(LED_NONE); > I've preserved the original behaviour of the code, just moved the `dediprog_set_leds` call out of th […] Probably not worth to merge this as is if we are going to change the behavior anyway. The order seems to have grown into this state before there were return values to check. And then somebody decided to check them. Switching the statements shouldn't make a difference for the hardware (register_spi_master() just performs software-internal setup). So I'm in favor to switch the two. -- To view, visit https://review.coreboot.org/c/flashrom/+/51706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib7e0179da39279e32a8497466b044b69ec836da8 Gerrit-Change-Number: 51706 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 22 Mar 2021 23:27:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog.c: Split up compound conditional statement
by Anastasia Klimchuk (Code Review) 23 Mar '21

23 Mar '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/+/51706 ) Change subject: dediprog.c: Split up compound conditional statement ...................................................................... Patch Set 1: (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/51706/comment/39f0a0ba_afd1319b PS1, Line 1276: if (register_spi_master(&spi_master_dediprog)) : return 1; : : return dediprog_set_leds(LED_NONE); > I've preserved the original behaviour of the code, just moved the `dediprog_set_leds` call out of th […] Yeah, I understand this is the original behaviour, but I also think it can be improved. Returning an error when LED fails is of course justified, but registering the master when LED fails and init fails is probably not. I agree this can be a separate patch though. -- To view, visit https://review.coreboot.org/c/flashrom/+/51706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib7e0179da39279e32a8497466b044b69ec836da8 Gerrit-Change-Number: 51706 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: 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: Mon, 22 Mar 2021 23:25:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog.c: Split up compound conditional statement
by Angel Pons (Code Review) 23 Mar '21

23 Mar '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51706 ) Change subject: dediprog.c: Split up compound conditional statement ...................................................................... Patch Set 1: (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/51706/comment/2668b417_49bbe04a PS1, Line 1276: if (register_spi_master(&spi_master_dediprog)) : return 1; : : return dediprog_set_leds(LED_NONE); > I think register_spi_master can be the very last line, means we register something only after all in […] I've preserved the original behaviour of the code, just moved the `dediprog_set_leds` call out of the if-statement. In any case, if setting the LEDs fails, then something is very wrong with the programmer, and returning an error is justified. -- To view, visit https://review.coreboot.org/c/flashrom/+/51706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib7e0179da39279e32a8497466b044b69ec836da8 Gerrit-Change-Number: 51706 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 22 Mar 2021 23:07:42 +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]: dediprog.c: Split up compound conditional statement
by Anastasia Klimchuk (Code Review) 23 Mar '21

23 Mar '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/+/51706 ) Change subject: dediprog.c: Split up compound conditional statement ...................................................................... Patch Set 1: Code-Review+1 (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/51706/comment/41b98bd7_bac247fd PS1, Line 1276: if (register_spi_master(&spi_master_dediprog)) : return 1; : : return dediprog_set_leds(LED_NONE); I think register_spi_master can be the very last line, means we register something only after all initialisation steps are completed successfully? Otherwise here register_xxx can succeed, but then set_leds fails and init returns 1, this seems like a contradiction. -- To view, visit https://review.coreboot.org/c/flashrom/+/51706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib7e0179da39279e32a8497466b044b69ec836da8 Gerrit-Change-Number: 51706 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: 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: Mon, 22 Mar 2021 23:03:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2021