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

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Anastasia Klimchuk (Code Review) 30 Jun '21

30 Jun '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/+/55934 ) Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... Patch Set 3: (3 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/55934/comment/311bd164_1859cc78 PS2, Line 108: /* Manual voltage setting */ > It does more than that. Probably doesn't need a comment. Done (comment removed) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/eb921630_dfedd186 PS2, Line 52: */ > You could also declare it above. […] Done, it works! but I'm not sure I understand how it works... magic? :) I thought I understand why void* worked - because any pointer can be casted to void*, and real function with some pointer was replaced with wrap function with void*. But now there is an entirely new type, which happens to have the same symbol as the old one? and they can replace each other? File tests/tests.c: https://review.coreboot.org/c/flashrom/+/55934/comment/5021a62f_d6ea2d1a PS2, Line 91: void __wrap_test_outb(unsigned char value, unsigned short port) : { : /* LOG_ME; */ : if (current_io && current_io->outb) : current_io->outb(current_io->state, value, port); : } : : unsigned char __wrap_test_inb(unsigned short port) : { : /* LOG_ME; */ : if (current_io && current_io->inb) : return current_io->inb(current_io->state, port); : return 0; : } : : void __wrap_test_outw(unsigned short value, unsigned short port) : { : /* LOG_ME; */ : if (current_io && current_io->outw) : current_io->outw(current_io->state, value, port); : } : : unsigned short __wrap_test_inw(unsigned short port) : { : /* LOG_ME; */ : if (current_io && current_io->inw) : return current_io->inw(current_io->state, port); : return 0; : } : : void __wrap_test_outl(unsigned int value, unsigned short port) : { : /* LOG_ME; */ : if (current_io && current_io->outl) : current_io->outl(current_io->state, value, port); : } : : unsigned int __wrap_test_inl(unsigned short port) : { > this style fix that sneaked in here is a separate patch Done CB:55974 -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 Gerrit-PatchSet: 3 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-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: Wed, 30 Jun 2021 00:51:42 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests.c: Move opening brace on new line for function body
by Anastasia Klimchuk (Code Review) 30 Jun '21

30 Jun '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/55974 ) Change subject: tests.c: Move opening brace on new line for function body ...................................................................... tests.c: Move opening brace on new line for function body BUG=b:181803212 TEST=builds and ninja test Change-Id: Ia1b00bb68ec2426161bfc183d93d2c0719a949ec Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/tests.c 1 file changed, 12 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/55974/1 diff --git a/tests/tests.c b/tests/tests.c index 44bbbdb..dcbd5af 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -88,39 +88,45 @@ return 0; } -void __wrap_test_outb(unsigned char value, unsigned short port) { +void __wrap_test_outb(unsigned char value, unsigned short port) +{ /* LOG_ME; */ if (current_io && current_io->outb) current_io->outb(current_io->state, value, port); } -unsigned char __wrap_test_inb(unsigned short port) { +unsigned char __wrap_test_inb(unsigned short port) +{ /* LOG_ME; */ if (current_io && current_io->inb) return current_io->inb(current_io->state, port); return 0; } -void __wrap_test_outw(unsigned short value, unsigned short port) { +void __wrap_test_outw(unsigned short value, unsigned short port) +{ /* LOG_ME; */ if (current_io && current_io->outw) current_io->outw(current_io->state, value, port); } -unsigned short __wrap_test_inw(unsigned short port) { +unsigned short __wrap_test_inw(unsigned short port) +{ /* LOG_ME; */ if (current_io && current_io->inw) return current_io->inw(current_io->state, port); return 0; } -void __wrap_test_outl(unsigned int value, unsigned short port) { +void __wrap_test_outl(unsigned int value, unsigned short port) +{ /* LOG_ME; */ if (current_io && current_io->outl) current_io->outl(current_io->state, value, port); } -unsigned int __wrap_test_inl(unsigned short port) { +unsigned int __wrap_test_inl(unsigned short port) +{ /* LOG_ME; */ if (current_io && current_io->inl) return current_io->inl(current_io->state, port); -- To view, visit https://review.coreboot.org/c/flashrom/+/55974 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia1b00bb68ec2426161bfc183d93d2c0719a949ec Gerrit-Change-Number: 55974 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Anastasia Klimchuk (Code Review) 30 Jun '21

30 Jun '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55934 to look at the new patch set (#3). Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... dediprog: Init-shutdown test for dediprog This patch adds mocks for libusb functions. To avoid dependency on libusb.h, libusb symbols for context and device handle are redefined. Real libusb functions are never called in tests anyway, cmocka wraps work with this without complaints. BUG=b:181803212 TEST=builds and ninja test Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/init_shutdown.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 107 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/55934/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 Gerrit-PatchSet: 3 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-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: [RFC] Makefile: Deflate dependency handling
by Nico Huber (Code Review) 29 Jun '21

29 Jun '21

Nico Huber has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/52024 ) Change subject: [RFC] Makefile: Deflate dependency handling ...................................................................... Abandoned Split it and forgot to re-use the change-id... -- To view, visit https://review.coreboot.org/c/flashrom/+/52024 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I33049474a95d288c9a84e412633c99fbc251ac03 Gerrit-Change-Number: 52024 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: abandon

1 0

Change in flashrom[master]: [RFC] tests: Abstract port I/O and factor mec1308 specifics out
by Nico Huber (Code Review) 29 Jun '21

29 Jun '21

Nico Huber has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/52604 ) Change subject: [RFC] tests: Abstract port I/O and factor mec1308 specifics out ...................................................................... Abandoned Integrated in other changes. -- To view, visit https://review.coreboot.org/c/flashrom/+/52604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia3a424478456a7c968b3c736ba371d755c494f30 Gerrit-Change-Number: 52604 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: abandon

1 0

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Nico Huber (Code Review) 29 Jun '21

29 Jun '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55934 ) Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... Patch Set 2: (2 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/55934/comment/7a6ab449_5ae300c3 PS2, Line 108: /* Manual voltage setting */ It does more than that. Probably doesn't need a comment. File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/a239c0c4_44634e79 PS2, Line 52: */ You could also declare it above. struct libusb_device_handle; typedef struct libusb_device_handle libusb_device_handle; Not more clutter than the comment ^^ -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 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-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 29 Jun 2021 18:46:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] spi_master: Add shutdown function in spi_master struct
by Nico Huber (Code Review) 29 Jun '21

29 Jun '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55932 ) Change subject: [RFC] spi_master: Add shutdown function in spi_master struct ...................................................................... Patch Set 1: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55932/comment/b8cca1cc_04e78950 PS1, Line 14: adding another argument to register_spi_master. The new version seems better! If the shutdown function can be decided at compile time, just like any other master function, this makes things much cleaner. Patchset: PS1: > I wasn't sure which one of the structs is the right place for the shutdown function, and even more, why there are two structs describing the same thing? I guess `master` is the right place as that is what the `data` is tied to. They are not the same thing. And there is even a 1:n relation. A programmer driver can register multiple masters. A most intricate example is the `internal` programmer. It can add multiple masters for the chipset alone; because there are different buses where the BIOS flash could be connected. It could even be indirectly connected e.g. through a super-i/o chip on the LPC bus, so `internal` might add a master for the super-i/o as well. `linux_mtd` is also considered part of the `internal` programmer on plat- forms where that combination is possible. If one thinks it through, even the EC masters should be part of it (something that was missed in the chromium fork, it seems). > Especially now, when both structs are in the same file. Both in the same file, however they are separate and cannot share information between each other. Only in the same file for the "few" masters that you have been looking at so far ;) > > Programmer_entry has init function, so I was thinking of adding a shutdown there as well. But then I cannot access it from register_spi_master. Alternatively, I thought to add `register_shutdown(programmer->shutdown, data)` after flashrom.c#159, but data is not available on that level. It's a hierarchy. In fact, we already have a programmer shutdown. Just the same for all programmers which simply calls all the master's shutdown functions. This may change in the future. Same as it was for the masters, there is global state cluttered over programmers. If that is to change, we'll need per-programmer shutdown pointers as well. More work in that direction will not cease to pop up soon. The libflashrom API provides a hint: `struct flashrom_programmer`. Where is that defined? Ooops, nobody did it (yet) ;) This should probably con- tain a pointer to the `struct programmer_entry` and one to its runtime `data`. And if that would ever be done: There is also a library-level context missing. That's not even declared yet (API needs to change /o\). > > Data is needed for shutdown, but also data is something that is re-created for every new run. With that, maybe adding shutdown into spi_master is ok? Data is there already. +1 > > Another thing I don't know, why data is added into spi/par/opaque master struct (3 times), and not in registered_master struct. I added shutdown function where the data is. Now that we pass `data` to the registration functions, it might be easy to move it to `registered_master`. File linux_spi.c: https://review.coreboot.org/c/flashrom/+/55932/comment/461477af_084711c5 PS1, Line 243: if (register_spi_master(&spi_master_linux, spi_data)) : return 1; /* core does cleanup */ : return 0; : Or just return register_spi_master(&spi_master_linux, spi_data); File programmer.h: https://review.coreboot.org/c/flashrom/+/55932/comment/59c7b8fb_8388a9aa PS1, Line 359: shutdown > I am not sure how others feel but I would vote for this to be ideally called `deinit` and logically […] I think we'll need shutdown functions for both eventually. Starting with the masters seems like a good next step. -- To view, visit https://review.coreboot.org/c/flashrom/+/55932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Gerrit-Change-Number: 55932 Gerrit-PatchSet: 1 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 29 Jun 2021 18:16:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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]: ich_descriptors: Normalize chipset detection
by Michał Żygowski (Code Review) 29 Jun '21

29 Jun '21

Attention is currently required from: Nico Huber. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5a5fee870202173b3a9813b03ec261e8ee45155 Gerrit-Change-Number: 55651 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> 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-Comment-Date: Tue, 29 Jun 2021 14:53:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors: Don't base chipset detection on `freq_read`
by Michał Żygowski (Code Review) 29 Jun '21

29 Jun '21

Attention is currently required from: Nico Huber. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55650 ) Change subject: ich_descriptors: Don't base chipset detection on `freq_read` ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55650 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f Gerrit-Change-Number: 55650 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> 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-Comment-Date: Tue, 29 Jun 2021 14:51:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors: Revise detection for chipsets w/ ICCRIBA
by Michał Żygowski (Code Review) 29 Jun '21

29 Jun '21

Attention is currently required from: Nico Huber. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55647 ) Change subject: ich_descriptors: Revise detection for chipsets w/ ICCRIBA ...................................................................... Patch Set 4: Code-Review+2 (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55647/comment/25aa0429_7f082b81 PS3, Line 946: } else if (upper->MDTBA == 0x00) { > Ping Nah, I guess it is not worth it. -- To view, visit https://review.coreboot.org/c/flashrom/+/55647 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Gerrit-Change-Number: 55647 Gerrit-PatchSet: 4 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> 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-Comment-Date: Tue, 29 Jun 2021 14:51:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021