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

Change in flashrom[master]: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Edward O'Callaghan (Code Review) 04 May '21

04 May '21

Attention is currently required from: Nico Huber, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52883/comment/1f113254_fa6eac11 PS1, Line 13: FEATAURE_QPI what did the datasheets say between 'C' and 'B' ? -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 04 May 2021 10:06:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

04 May '21

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, 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 33: (1 comment) Patchset: PS32: > OK, I understand (I have/had got no idea of VM stuff). > > So in case of not using VM, the speed we gain because of less USB communication is consumed by the increased processing time needed to pack the packages? The processing time is negligible. Just prefix your flashrom command with `time`, you'll see that it actually does nothing but waiting in either case. If you could provide a total number how long it takes to fully read/erase/write a chip, that could give us a clue (sorry if this was already given and I missed it). I suspect the VM case to be slower in general, so the speed increase of this patch would just be time re-gained that was lost due to the virtuali- zation in the first place. So far my theory. I expect, but don't know, the individual packets to reach the FTDI with some delay. I have not looked into libftdi deeply enough. Maybe it waits for confirmation that the write reached the device before ftdi_write_data() returns. If this is the case, we'd get a lot syn- chronization overhead. Consider this: send WREN packet --> send WP packet --> wait for acks <-- vs. send WREN packet --> wait for ack <-- send WP packet --> wait for ack <-- Ignoring bandwidth, in the second example, it would take 2 round- trip times instead of one. And the round-trip is influenced by the virtualization. It depends a lot how the lower layers are implemen- ted. What flashrom leaves to libftdi. > > Feel free to comment if you think this patch is worth to get merged. I think it's nice work and even if it wouldn't ever increase the speed, it would still be a nice example how to implement spi_send_multicommand 😊 But that it helps your use case without affecting others would be enough reason to get it in anyway. From my side, I'll just have to find the time for a final good look at it, then it could be merged. -- 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: 33 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Tue, 04 May 2021 10:03:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Nikolai Artemiev (Code Review) 04 May '21

04 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Further explanation of the reasons for this change: These chip entries are some of the last differences between cros and upstream flashrom - resolving them will get us extremely close to fully deforking the flashchips.{c,h} files. We also tried bringing the duplicate chip into cros flashrom, but that caused issues with flashchip identification, and resolving it would be likely to significantly delay resyncing flashchips.c. This patch does remove the FEATURE_QPI feature bit (since only 'C' chips had it), however (AFAIK) that feature flag is currently unused, and the chip's capabilities are still recorded in the comments. An additional advantage of this approach is that it generally makes flashrom easier to use. -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> 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: Tue, 04 May 2021 09:24:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Nikolai Artemiev (Code Review) 04 May '21

04 May '21

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C Both the 'B' and 'C' chips have the same vendor / device ID, leading to flashrom not being able to automatically identify them. The only difference between the definitions is that the 'C' chips have FEATAURE_QPI and 'B' chips don't. For now this has been resolved by removing that feature bit. Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c 1 file changed, 9 insertions(+), 44 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/52883/1 diff --git a/flashchips.c b/flashchips.c index 6482f67..81ee9fb 100644 --- a/flashchips.c +++ b/flashchips.c @@ -6271,13 +6271,18 @@ { .vendor = "GigaDevice", - .name = "GD25B128B/GD25Q128B", + .name = "GD25B128B/GD25Q128B/GD25Q127C/GD25Q128C", .bustype = BUS_SPI, .manufacture_id = GIGADEVICE_ID, .model_id = GIGADEVICE_GD25Q128, .total_size = 16384, .page_size = 256, - /* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44 */ + /* OTP: read 0x48; write 0x42; erase 0x44 + GD25B128B/GD25Q128B: 1024B total, 256B reserved + GD25Q127C/GD25Q128C: 1536B total, 256B reserved + + QPI: GD25B128B/GD25Q128B: QPI support unknown + GD25Q127C/GD25Q128C: enable 0x38, disable 0xFF */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP, .tested = TEST_OK_PREW, .probe = probe_spi_rdid, @@ -6301,8 +6306,9 @@ .block_erase = spi_block_erase_c7, } }, + /* TODO: 2nd status reg (read 0x35, write 0x31) and 3rd status reg (read 0x15, write 0x11) */ .printlock = spi_prettyprint_status_register_bp4_srwd, - .unlock = spi_disable_blockprotect_bp4_srwd, /* TODO: 2nd status reg (read with 0x35) */ + .unlock = spi_disable_blockprotect_bp4_srwd, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {2700, 3600}, @@ -6582,47 +6588,6 @@ { .vendor = "GigaDevice", - .name = "GD25Q127C/GD25Q128C", - .bustype = BUS_SPI, - .manufacture_id = GIGADEVICE_ID, - .model_id = GIGADEVICE_GD25Q128, - .total_size = 16384, - .page_size = 256, - /* OTP: 1536B total; read 0x48; write 0x42, erase 0x44 */ - /* QPI: enable 0x38, disable 0xFF */ - .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI, - .tested = TEST_OK_PREW, - .probe = probe_spi_rdid, - .probe_timing = TIMING_ZERO, - .block_erasers = - { - { - .eraseblocks = { {4 * 1024, 4096} }, - .block_erase = spi_block_erase_20, - }, { - .eraseblocks = { {32 * 1024, 512} }, - .block_erase = spi_block_erase_52, - }, { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = spi_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - } - }, - /* TODO: 2nd status reg (read 0x35, write 0x31) and 3rd status reg (read 0x15, write 0x11) */ - .printlock = spi_prettyprint_status_register_bp4_srwd, - .unlock = spi_disable_blockprotect_bp4_srwd, - .write = spi_chip_write_256, - .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ - .voltage = {2700, 3600}, - }, - - { - .vendor = "GigaDevice", .name = "GD25Q16(B)", .bustype = BUS_SPI, .manufacture_id = GIGADEVICE_ID, -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Simon Buhrow (Code Review) 04 May '21

04 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Samir Ibradžić. Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 33: (1 comment) Patchset: PS32: > > * I´m running flashrom on a virutal machine (don´t think this makes any difference) […] OK, I understand (I have/had got no idea of VM stuff). So in case of not using VM, the speed we gain because of less USB communication is consumed by the increased processing time needed to pack the packages? Feel free to comment if you think this patch is worth to get merged. -- 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: 33 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Tue, 04 May 2021 08:50:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Simon Buhrow (Code Review) 04 May '21

04 May '21

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Samir Ibradžić. Hello build bot (Jenkins), Alan Green, Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Samir Ibradžić, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/40477 to look at the new patch set (#33). 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 when using flashrom inside a virtual machine. This leads to what the comment already says: Minimize USB transfers by packing as many commands as possible together. This saves about 1 minute when programming a 128MBit Flash with my VM 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/33 -- 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: 33 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: libflashrom: Return progress state to the library user
by Edward O'Callaghan (Code Review) 04 May '21

04 May '21

Attention is currently required from: Richard Hughes, Angel Pons, Anastasia Klimchuk, Patrick Rudolph. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 ) Change subject: libflashrom: Return progress state to the library user ...................................................................... Patch Set 4: (1 comment) File libflashrom.c: https://review.coreboot.org/c/flashrom/+/49643/comment/ae954e24_dea0062a PS4, Line 113: private > This function doesn't look to be private, it is called outside of this file. private from the libflashrom API, as in "internal" to the flashrom implementation. Not to mean static local. private is a bit of a overloaded term [sic. java/c++]. -- To view, visit https://review.coreboot.org/c/flashrom/+/49643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c Gerrit-Change-Number: 49643 Gerrit-PatchSet: 4 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Richard Hughes <hughsient(a)gmail.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 04 May 2021 08:15:15 +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]: libflashrom: Return progress state to the library user
by Anastasia Klimchuk (Code Review) 04 May '21

04 May '21

Attention is currently required from: Richard Hughes, Angel Pons, Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 ) Change subject: libflashrom: Return progress state to the library user ...................................................................... Patch Set 4: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/49643/comment/19007be0_a919d018 PS4, Line 9: Include test for the dummy spi25 device. If you could elaborate more on what this commit is doing, it would be great. Maybe add a paragraph explaining the change in more details? File libflashrom.h: https://review.coreboot.org/c/flashrom/+/49643/comment/4b938cc4_968242e6 PS4, Line 106: void * Is there a reason why last argument doesn't have a name here? all other arguments have names. File libflashrom.c: https://review.coreboot.org/c/flashrom/+/49643/comment/6676a19e_d98d9810 PS4, Line 113: private This function doesn't look to be private, it is called outside of this file. File tests/spi25.c: https://review.coreboot.org/c/flashrom/+/49643/comment/c5032b8f_f4aa73cb PS4, Line 54: size_t current, Thank you for the test! Is there any way to assert what current value is? it should change (unlike total) right? -- To view, visit https://review.coreboot.org/c/flashrom/+/49643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c Gerrit-Change-Number: 49643 Gerrit-PatchSet: 4 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Richard Hughes <hughsient(a)gmail.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 04 May 2021 06:43:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Separate shutdown from failed init cleanup
by Anastasia Klimchuk (Code Review) 04 May '21

04 May '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/+/52877 ) Change subject: buspirate_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 1: (1 comment) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/52877/comment/9ef3d665_9008d396 PS1, Line 672: if (register_shutdown(buspirate_spi_shutdown, NULL) != 0) { : ret = 1; : goto init_err_cleanup_exit; : } After moving register_shutdown below this has the same situation as CB:52773 , specifically if register_shutdown fails than full cleanup is performed. I use the same approach as we agreed on CB:52773 -- To view, visit https://review.coreboot.org/c/flashrom/+/52877 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Gerrit-Change-Number: 52877 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: Miklós Márton <martonmiklosqdev(a)gmail.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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 04 May 2021 04:58:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Separate shutdown from failed init cleanup
by Anastasia Klimchuk (Code Review) 04 May '21

04 May '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52877 ) Change subject: buspirate_spi.c: Separate shutdown from failed init cleanup ...................................................................... buspirate_spi.c: Separate shutdown from failed init cleanup Shutdown function was covering two different jobs here: 1) the actual shutdown which is run at the end of the driver's lifecycle and 2) cleanup in cases when initialisation failed. Now, shutdown is only doing its main job (#1), and the driver itself is doing cleanup when init fails (#2). The good thing is that now resources are released/closed immediately in cases when init fails (vs shutdown function which was run at some point later), and the driver leaves clean space after itself if init fails. And very importantly this unlocks API change which plans to move register_shutdown inside register master API, see https://review.coreboot.org/c/flashrom/+/51761 BUG=b:185191942 TEST=builds Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M buspirate_spi.c 1 file changed, 71 insertions(+), 49 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/52877/1 diff --git a/buspirate_spi.c b/buspirate_spi.c index eee8daa..31d36cd 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -385,13 +385,6 @@ return ret; } - if (register_shutdown(buspirate_spi_shutdown, NULL) != 0) { - bp_commbufsize = 0; - free(bp_commbuf); - bp_commbuf = NULL; - return 1; - } - /* This is the brute force version, but it should work. * It is likely to fail if a previous flashrom run was aborted during a write with the new SPI commands * in firmware v5.5 because that firmware may wait for up to 4096 bytes of input before responding to @@ -403,7 +396,7 @@ /* Send the command, don't read the response. */ ret = buspirate_sendrecv(bp_commbuf, 1, 0); if (ret) - return ret; + goto init_err_cleanup_exit; /* The old way to handle responses from a Bus Pirate already in BBIO mode was to flush any * response which came in over serial. Unfortunately that does not work reliably on Linux * with FTDI USB-serial. @@ -416,19 +409,19 @@ } /* We know that 20 commands of \0 should elicit at least one BBIO1 response. */ if ((ret = buspirate_wait_for_string(bp_commbuf, "BBIO"))) - return ret; + goto init_err_cleanup_exit; /* Reset the Bus Pirate. */ bp_commbuf[0] = 0x0f; /* Send the command, don't read the response. */ if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, "irate "))) - return ret; + goto init_err_cleanup_exit; /* Read the hardware version string. Last byte of the buffer is reserved for \0. */ for (i = 0; i < DEFAULT_BUFSIZE - 1; i++) { if ((ret = buspirate_sendrecv(bp_commbuf + i, 0, 1))) - return ret; + goto init_err_cleanup_exit; if (strchr("\r\n\t ", bp_commbuf[i])) break; } @@ -449,11 +442,11 @@ msg_pdbg("\n"); if ((ret = buspirate_wait_for_string(bp_commbuf, "irmware "))) - return ret; + goto init_err_cleanup_exit; /* Read the firmware version string. Last byte of the buffer is reserved for \0. */ for (i = 0; i < DEFAULT_BUFSIZE - 1; i++) { if ((ret = buspirate_sendrecv(bp_commbuf + i, 0, 1))) - return ret; + goto init_err_cleanup_exit; if (strchr("\r\n\t ", bp_commbuf[i])) break; } @@ -474,21 +467,24 @@ msg_pdbg("\n"); if ((ret = buspirate_wait_for_string(bp_commbuf, "HiZ>"))) - return ret; + goto init_err_cleanup_exit; /* Tell the user about missing SPI binary mode in firmware 2.3 and older. */ if (BP_FWVERSION(fw_version_major, fw_version_minor) < BP_FWVERSION(2, 4)) { msg_pinfo("Bus Pirate firmware 2.3 and older does not support binary SPI access.\n"); msg_pinfo("Please upgrade to the latest firmware (at least 2.4).\n"); - return SPI_PROGRAMMER_ERROR; + ret = SPI_PROGRAMMER_ERROR; + goto init_err_cleanup_exit; } /* Use fast SPI mode in firmware 5.5 and newer. */ if (BP_FWVERSION(fw_version_major, fw_version_minor) >= BP_FWVERSION(5, 5)) { msg_pdbg("Using SPI command set v2.\n"); /* Sensible default buffer size. */ - if (buspirate_commbuf_grow(260 + 5)) - return ERROR_OOM; + if (buspirate_commbuf_grow(260 + 5)) { + ret = ERROR_OOM; + goto init_err_cleanup_exit; + } spi_master_buspirate.max_data_read = 2048; spi_master_buspirate.max_data_write = 256; spi_master_buspirate.command = buspirate_spi_send_command_v2; @@ -497,8 +493,10 @@ msg_pinfo("Reading/writing a flash chip may take hours.\n"); msg_pinfo("It is recommended to upgrade to firmware 5.5 or newer.\n"); /* Sensible default buffer size. */ - if (buspirate_commbuf_grow(16 + 3)) - return ERROR_OOM; + if (buspirate_commbuf_grow(16 + 3)) { + ret = ERROR_OOM; + goto init_err_cleanup_exit; + } spi_master_buspirate.max_data_read = 12; spi_master_buspirate.max_data_write = 12; spi_master_buspirate.command = buspirate_spi_send_command_v1; @@ -538,37 +536,37 @@ /* Enter baud rate configuration mode */ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "b\n"); if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, ">"))) - return ret; + goto init_err_cleanup_exit; /* Enter manual clock divisor entry mode */ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "10\n"); if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, ">"))) - return ret; + goto init_err_cleanup_exit; /* Set the clock divisor to the value calculated from the user's input */ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "%d\n", BP_DIVISOR(serialspeeds[serialspeed_index].speed)); if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0))) - return ret; + goto init_err_cleanup_exit; sleep(1); /* Reconfigure the host's serial baud rate to the new value */ if ((ret = serialport_config(sp_fd, serialspeeds[serialspeed_index].speed))) { msg_perr("Unable to configure system baud rate to specified value."); - return ret; + goto init_err_cleanup_exit; } /* Return to the main prompt */ bp_commbuf[0] = ' '; if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, "HiZ>"))) - return ret; + goto init_err_cleanup_exit; msg_pdbg("Serial speed is %d baud\n", serialspeeds[serialspeed_index].speed); } @@ -581,30 +579,34 @@ for (i = 0; i < 20; i++) { bp_commbuf[0] = 0x00; if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0))) - return ret; + goto init_err_cleanup_exit; } if ((ret = buspirate_wait_for_string(bp_commbuf, "BBIO"))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_sendrecv(bp_commbuf, 0, 1))) - return ret; + goto init_err_cleanup_exit; msg_pdbg("Raw bitbang mode version %c\n", bp_commbuf[0]); if (bp_commbuf[0] != '1') { msg_perr("Can't handle raw bitbang mode version %c!\n", bp_commbuf[0]); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Enter raw SPI mode */ bp_commbuf[0] = 0x01; ret = buspirate_sendrecv(bp_commbuf, 1, 0); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if ((ret = buspirate_wait_for_string(bp_commbuf, "SPI"))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_sendrecv(bp_commbuf, 0, 1))) - return ret; + goto init_err_cleanup_exit; msg_pdbg("Raw SPI mode version %c\n", bp_commbuf[0]); if (bp_commbuf[0] != '1') { msg_perr("Can't handle raw SPI mode version %c!\n", bp_commbuf[0]); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Initial setup (SPI peripherals config): Enable power, CS high, AUX */ @@ -614,21 +616,27 @@ msg_pdbg("Enabling pull-up resistors.\n"); } ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting power/CS/AUX(/Pull-up resistors)!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Set SPI speed */ bp_commbuf[0] = 0x60 | spispeed; ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI speed!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Set SPI config: output type, idle, clock edge, sample */ @@ -638,24 +646,38 @@ msg_pdbg("Pull-ups enabled, so using HiZ pin output! (Open-Drain mode)\n"); } ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI config!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* De-assert CS# */ bp_commbuf[0] = 0x03; ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while raising CS#!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } + if (register_shutdown(buspirate_spi_shutdown, NULL) != 0) { + ret = 1; + goto init_err_cleanup_exit; + } register_spi_master(&spi_master_buspirate); return 0; + +init_err_cleanup_exit: + buspirate_spi_shutdown(NULL); + return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/52877 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Gerrit-Change-Number: 52877 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021