- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Angel Pons (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Simon Buhrow, Alan Green, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 35: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/40477/comment/f9386d01_323a77d0 PS35, Line 15: my As mentioned elsewhere, specify this is in a virtualized environment? -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 35 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Alan Green <avg(a)google.com> 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-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Fri, 25 Jun 2021 12:08:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Angel Pons (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Simon Buhrow, Alan Green, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 35: Code-Review+1 (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/83b434dc_dadf3f44 PS34, Line 317: > Generally right, but it's mimicking the original code and that will […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 35 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Alan Green <avg(a)google.com> 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-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Fri, 25 Jun 2021 12:01:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 9: (1 comment) Patchset: PS4: > We could use both DMI and SVID/SDID checks: […] If we really want to put that much thought into it, we should unify this somehow with `board_enable.c`. For instance, have a common struct and a common function that decides if the host matches. I guess the current rules of `board_enable.c` (when does it match?) are generally debatable, but we should make decisions for the project as a whole and not locally for programmers. I think forcing in the case of mismatching contents is ok. Or even don't check the contents at all (IMHO the user---not necessarily a human, e.g. fwupd---should be responsible for the data). We should have a look how fwupd does things. Forcing to run the programmer code on an unknown board would be new to flashrom. I don't see why we would suddenly need that. If we add forcing options, they should not be global; i.e. never use the classic CLI's `-f` for anything new. Using the same force flag for multiple things turns users blind to it, they'll just always add an `-f` eventually. And then they might force one thing by accident because they wanted to force another. -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 9 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Fri, 25 Jun 2021 11:31:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 35: Code-Review+1 (18 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/40477/comment/15c34477_066e1b05 PS8, Line 17: https://ibb.co/0c1J25d > I wouldn't provide them in a commit message Done Commit Message: https://review.coreboot.org/c/flashrom/+/40477/comment/ba701a0c_52db1638 PS16, Line 25: in a similar way. > The infrastructure is already in place. You just have to implement […] Done Patchset: PS32: > > OK, I understand (I have/had got no idea of VM stuff). […] Done Patchset: PS34: > Thanks for paying attention again. […] I'll take over then. It's only a few nits left after all. File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/95c0abfd_b5e828e6 PS8, Line 542: /* Return to get second op (Program or Erase) without resetting buf nor i*/ > s Line 473 Ack File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/5e48bb31_a0be59e3 PS16, Line 172: .multicommand = default_spi_send_multicommand, > I got it. […] Done https://review.coreboot.org/c/flashrom/+/40477/comment/cf1c0926_7b36aeaa PS16, Line 497: bufsize = max(writecnt + 19, 261 + 19); > > What do you want to drop? Just the change from 260+9 to 261+19 or the whole max()-line? […] Done https://review.coreboot.org/c/flashrom/+/40477/comment/9c523e9b_0f36e9ad PS16, Line 562: if (writearr[0] == JEDEC_WREN) { > You are right, I was assuming JEDEC_WREN in any case. […] Done File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/de4141b9_85c494c0 PS25, Line 189: ft2232_spi_send_command > Maybe we should do this rather early to have more test coverage. Done in a subsequent commit. File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/296c1685_2e073c26 PS33, Line 290: + cmd->readcnt > One tiny mistake left. I also didn't realize this before: The buffer `buf` […] Done File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/f506e043_5965a581 PS34, Line 82: > I guess this space is not intentional. In any case, it should align […] Done https://review.coreboot.org/c/flashrom/+/40477/comment/cfb9f5d4_bfaa60cd PS34, Line 283: const size_t cmd_len = 3; /* same length for any ft2232 command */ > I don't think it's necessary, as the literal value is never used outside of this function. […] I agree with Angel, it should be kept as close as possible to the code it's used in. https://review.coreboot.org/c/flashrom/+/40477/comment/fd7114d0_21441666 PS34, Line 290: + cmd->writecnt + cmd->readcnt The buffer in question does not include the `readcnt` part. https://review.coreboot.org/c/flashrom/+/40477/comment/c9932d66_132940b6 PS34, Line 300: int i = 0, ret = 0; > Shouldn't `i` be `unsigned int` or `size_t`? […] Ack It's used as index for a byte array and as parameter to ft2232_spi_command_fits(), so `size_t`. https://review.coreboot.org/c/flashrom/+/40477/comment/2b8381fb_db0680a9 PS34, Line 307: 65536 > hmmm, max_data_write is 256 That is right, and assuming an overall sound flashrom, we shouldn't hit this case. But it seems a good idea to check this "external" data locally again. Or do you mean we could make the check tighter? I guess we could do so. Probably in a follow up as this just mimicks the original code. https://review.coreboot.org/c/flashrom/+/40477/comment/7934221c_abec5c51 PS34, Line 311: Out of memory! > `Out of memory!` is usually printed when a malloc() or similar function fails. […] Ack "Command does not fit" to align with other error messages that don't have a "FTDI" prefix. https://review.coreboot.org/c/flashrom/+/40477/comment/2da3ab68_bd3f8ff8 PS34, Line 317: > nit: no space between unary operators: […] Generally right, but it's mimicking the original code and that will be reverted later. I hope it's ok to leave it for the moment. https://review.coreboot.org/c/flashrom/+/40477/comment/38a37f06_2b4bf34e PS34, Line 349: /* send_buf */ > I'd say this comment is redundant, and would remove it Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 35 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Fri, 25 Jun 2021 11:06:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Alan Green <avg(a)google.com> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> 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]: ft2232_spi.c: Implement spi_send_multicommand()
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Nico Huber has uploaded a new patch set (#35) to the change originally created by Simon Buhrow. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... ft2232_spi.c: Implement spi_send_multicommand() Every ftdi_write_data() call is quite time consuming as the ftdi-chips seems to take always 2-3ms to respond. This leads to what the comment already says: Minimize USB transfers by packing as many commands as possible together. So I packed the WREN command together with the following operation which can be program or erase operation. This saves about 1 minute when programming a 128MBit Flash with my config! Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a --- M ft2232_spi.c 1 file changed, 90 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/35 -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 35 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Michał Żygowski (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Michael Niewöhner. Hello Felix Singer, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55715 to look at the new patch set (#9). Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops Add a new programmer supporting ITE Embedded Controllers found on TUXEDO laptops. Tested on TUXEDO InfinityBook S 14 Gen6 and 15 Gen6 with EC firmware updates from IBV, EC versions 1.07.02TR1 and 1.07.04TR4. Example standard command to update EC firmware on a TUXEDO laptop: "flashrom -p ite_ec:romsize=128K,autoload=disable -w eL14MU02.TR1" Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 --- M Makefile M flashrom.8.tmpl A ite_ec.c A ite_ec.h M programmer.h M programmer_table.c 6 files changed, 1,053 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/55715/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 9 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Angel Pons (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 8: (1 comment) Patchset: PS4: > Maybe they are unique. However I have this feeling that DMI checks are more reliable. […] We could use both DMI and SVID/SDID checks: - DMI good, SVID/SDID good ---> [1] - DMI good, SVID/SDID bad ---> [2] - DMI bad, SVID/SDID good ---> [3] - DMI bad, SVID/SDID bad ---> [4] [1]: full match, proceed as usual. [2]: unexpected (DMI is more specific than SVID/SDID), abort unless forced [3]: mention that things could work, but abort unless forced [4]: unknown machine, abort unless forced Running into [2] would be very unusual. I'd explain the situation (DMI strings match, but SVID/SDID do not; could be a boot firmware bug or an unsupported system) and warn against continuing unless one is certain that the mismatched SVID/SDID is spurious (not something the mailing list would know). For [3], I'd make flashrom say: - What happened: SVID/SDID match, but DMI strings do not - What the above implies: the machine might just be a rebranded system - What to do: if one can risk a brick, try forcing and see what happens. if it works, then add the DMI strings to flashrom and submit a patch In both cases, log the known DMI strings and SVID/SDID values (print both expected and actual SVID/SDID in [2]). Thoughts? -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 8 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Fri, 25 Jun 2021 10:23:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Edward O'Callaghan (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 ) Change subject: tests: Do not run a test if its driver is not built ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55295/comment/9b7b4150_8dffdd9c PS5, Line 10: however config options can be disabled and in that case test should : not be run. > I'd even say it's better to return back to this later. Why switch […] Agreed. This doesn't need to be resolved to the logical end point right now. The currently solution is perfectly reasonable for the number of tests we currently have. Let's just resolve this and not be distracted by future problems here. I totally agree with Nico's assessment about the weak symbol situation in coreboot!!! Can't stress that one enough and thanks for contrasting that here as it's a excellent case study why weak mostly sucks as well in the general case. My only claim here is that, while I would think weak symbols would be a extremely exceptional to use, I suggest it is perhaps valid in this case. Only for the purposes of a test framework like cmocka that is designed around the weak symbol concept and that is to say it fits the paradigm and problem space. It is very unfortunately we do not have the features of Ada or Rust's traits at our disposal in C however if we did go the route of a clutter of `if (CONFIG(MEC1308))` I would prefer that it be all isolated into one object file in a very narrowly scoped way. Thanks C. -- To view, visit https://review.coreboot.org/c/flashrom/+/55295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Gerrit-Change-Number: 55295 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: 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-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 25 Jun 2021 10:01:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> 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

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Angel Pons (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 ) Change subject: tests: Do not run a test if its driver is not built ...................................................................... Patch Set 5: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55295/comment/d99e4585_b1bd3ef7 PS5, Line 10: however config options can be disabled and in that case test should : not be run. > I'd even say it's better to return back to this later. Why switch > to another scheme before we have seen how this one turns out? I > grasp from the name `init_shutdown.c` that the idea was to have > individual files per tested path. Once we try to add another file, > we'll probably much easier see what the advantages and disadvan- > tages are. > > As stated earlier, missing dependencies might be a reason to favor > per programmer files. For instance if we need some library's > header file to compile the test code. But that's a problem that > I'd try to solve only once it's clear to exist :) I fully agree. Predicting all potential advantages and disadvantages of some approach is hard (at least for me), and I find it easier to wait until the issues manifest themselves. > As for weak functions. I'd rather clutter the code with > > if (CONFIG(MEC1308)) > > than with weak declarations. In the only project I know that makes > heavily use of weak symbols (coreboot) they often turn bad. I concur. Most uses of weak symbols in coreboot cause more trouble than benefits, and this is why I try to get rid of weak symbols where possible. For instance, CB:55812 could have added a weak declaration for the `mb_get_lpddr3_dq_dqs_map()` function instead of a new Kconfig option. However, mainboards with LPDDR3 *must* provide the DQ/DQS map, or else raminit will always fail. If one forgets to define the `mb_get_lpddr3_dq_dqs_map()` function, the current Kconfig approach will result in a build error (linker complains about undefined reference). Using a weak symbol would result in a runtime failure, and finding out what went wrong wouldn't be trivial (raminit is done by the MRC.bin blob, which doesn't log nor return useful error information). -- To view, visit https://review.coreboot.org/c/flashrom/+/55295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Gerrit-Change-Number: 55295 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: 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-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 25 Jun 2021 09:56:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> 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

Change in flashrom[master]: flashrom.c: Add 64KiB write granularity
by Michał Żygowski (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Angel Pons, Michael Niewöhner. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55713 ) Change subject: flashrom.c: Add 64KiB write granularity ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55713/comment/ab501ceb_2a41323c PS2, Line 9: Embedded Controller of : Tuxedo laptops > `ITE Embedded Controller support`? Done -- To view, visit https://review.coreboot.org/c/flashrom/+/55713 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f Gerrit-Change-Number: 55713 Gerrit-PatchSet: 3 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Fri, 25 Jun 2021 09:52:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit