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

Change in flashrom[master]: ichspi.c: Implement read_write_status for wp
by Subrata Banik (Code Review) 30 May '22

30 May '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64540 ) Change subject: ichspi.c: Implement read_write_status for wp ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/64540 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ecbe8491ecea3697922c91af26ca62276e86317 Gerrit-Change-Number: 64540 Gerrit-PatchSet: 7 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 30 May 2022 04:24:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Implement read_write_status for wp
by Edward O'Callaghan (Code Review) 30 May '22

30 May '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Hello build bot (Jenkins), Subrata Banik, Nikolai Artemiev, Sergii Dmytruk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/64540 to look at the new patch set (#7). Change subject: ichspi.c: Implement read_write_status for wp ...................................................................... ichspi.c: Implement read_write_status for wp The ichspi hwseq path has a opaque master specialisation that allows for reading and writing STATUS1 registers. Hook the callbacks with a implementation to allow for this so that writeprotect maybe supported though this path. Change-Id: I7ecbe8491ecea3697922c91af26ca62276e86317 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M ichspi.c 1 file changed, 74 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/64540/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/64540 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ecbe8491ecea3697922c91af26ca62276e86317 Gerrit-Change-Number: 64540 Gerrit-PatchSet: 7 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
by Edward O'Callaghan (Code Review) 30 May '22

30 May '22

Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 ) Change subject: writeprotect.c: Allow opaque masters to hook spi_{rw}_register() ...................................................................... Patch Set 6: (1 comment) File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/64375/comment/0ee45d4c_3eec94f2 PS5, Line 33: if ((flash->mst->buses_supported & BUS_PROG) && flash->mst->opaque.write_register) { : return flash->mst->opaque.write_register(flash, reg, value); : } > I am inclined to move this specalisation to actually within writeprotect. […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/64375 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Gerrit-Change-Number: 64375 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Mon, 30 May 2022 01:52:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
by Edward O'Callaghan (Code Review) 30 May '22

30 May '22

Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 ) Change subject: writeprotect.c: Allow opaque masters to hook spi_{rw}_register() ...................................................................... Patch Set 6: (1 comment) File include/programmer.h: https://review.coreboot.org/c/flashrom/+/64375/comment/adbb6ce2_ca72ef45 PS6, Line 405: int (*read_register)(const struct flashctx *flash, enum flash_reg reg, uint8_t *value); : int (*write_register)(const struct flashctx *flash, enum flash_reg reg, uint8_t value); > If these are meant to stay WP only, they should have "wp_" prefix. That is a fair point however this was intentional since r/w register is not necessarily specific to only writeprotect.c it is just that the use-case happens to be that right now. The hooks are intended to be general purpose, another use-case I have potentially in mind is for libflashrom to facilitate dumping register state. -- To view, visit https://review.coreboot.org/c/flashrom/+/64375 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Gerrit-Change-Number: 64375 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Mon, 30 May 2022 01:52:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_output.c: Format progress output to show in single line
by Anastasia Klimchuk (Code Review) 30 May '22

30 May '22

Attention is currently required from: Daniel Campello, Light. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64668 ) Change subject: cli_output.c: Format progress output to show in single line ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Patchset: PS1: Makes sense to me. Daniel, Richard, does this look good for you as authors of progress API? -- To view, visit https://review.coreboot.org/c/flashrom/+/64668 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If982193f9857f0da47cd71706ce6cebaa60d4323 Gerrit-Change-Number: 64668 Gerrit-PatchSet: 1 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Richard Hughes <richard(a)hughsie.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Mon, 30 May 2022 00:37:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: Allow getting the progress_state from the flashctx
by Anastasia Klimchuk (Code Review) 30 May '22

30 May '22

Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64663 ) Change subject: libflashrom: Allow getting the progress_state from the flashctx ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: I think I can answer some of these, let me try! :) > have you tested if the current implementation works? There is a testing information in commit message, I asked to add earlier. > I'm a bit confused about when update_progress() should get called. Previous patch CB:49643 introduces update_progress() calls into many programmers. For me, it was easier to understand when looking at CB:49643 > Does it do anything on a modern Intel system at all? This question, maybe someone can confirm, but what I see is that CB:49643 is not changing ichspi.c at all, so I guess Intel systems are not touched? > There are odd paths like spi_chip_read() calls into the master, which may > call spi_read_chunked() which calls update_progress(). But spi_chip_read() > calls it as well? I wonder what was the reason to do it at the chip and > programmer levels. This is interesting, I had a closer look into `spi_chip_read()` and `spi_read_chunked()` and I see now what you mean. It is possible that update_progress is called by both functions, however not always, but only in some cases. I looked into flashrom_progress_cb in cli_output.c and it calculates percent progress, and then it produces output only of percent has changed. So it won't produce the same repeated lines like READ 66% complete... READ 66% complete... It will be only line in such case. What would be a better way to wire update_progress into spi infra, what do you think? File include/libflashrom.h: https://review.coreboot.org/c/flashrom/+/64663/comment/d262150c_4a0fb9d9 PS2, Line 85: typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx); > Why doesn't it get the progress passed directly? I think I can answer this one. There is a progress_state in flashctx, so no need to pass another one. The earlier versions in CB:49643 had two arguments: flashctx and progress_state and it was confusing on which exactly state is used. The patch owner was asked to remove the second argument during review. I can understand because there is a progress_state in flashctx already. I hope I answered your question! At least, to the extent how I understand the question. -- To view, visit https://review.coreboot.org/c/flashrom/+/64663 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I322bf56ff92f7b4d0ffc92768e9f0cdf7cb82010 Gerrit-Change-Number: 64663 Gerrit-PatchSet: 2 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Comment-Date: Mon, 30 May 2022 00:23:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
by Sergii Dmytruk (Code Review) 29 May '22

29 May '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 ) Change subject: writeprotect.c: Allow opaque masters to hook spi_{rw}_register() ...................................................................... Patch Set 6: (1 comment) File include/programmer.h: https://review.coreboot.org/c/flashrom/+/64375/comment/b2805ef9_e950968d PS6, Line 405: int (*read_register)(const struct flashctx *flash, enum flash_reg reg, uint8_t *value); : int (*write_register)(const struct flashctx *flash, enum flash_reg reg, uint8_t value); If these are meant to stay WP only, they should have "wp_" prefix. -- To view, visit https://review.coreboot.org/c/flashrom/+/64375 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Gerrit-Change-Number: 64375 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 29 May 2022 13:18:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[refs/meta/config]: Add reviewers group to allow +1, -1 changes.
by Angel Pons (Code Review) 29 May '22

29 May '22

Attention is currently required from: Felix Singer, Nico Huber, Martin L Roth. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64499 ) Change subject: Add reviewers group to allow +1, -1 changes. ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Patchset: PS2: Looks reasonable. -- To view, visit https://review.coreboot.org/c/flashrom/+/64499 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-Change-Id: I6af85c58d9a8c37c8bf17181c83720332a474cc8 Gerrit-Change-Number: 64499 Gerrit-PatchSet: 2 Gerrit-Owner: Martin L Roth <gaumless(a)tutanota.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin L Roth <gaumless(a)tutanota.com> Gerrit-Comment-Date: Sun, 29 May 2022 11:44:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect.c: Allow opaque masters to hook spi_{rw}_register()
by Edward O'Callaghan (Code Review) 29 May '22

29 May '22

Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Subrata Banik, Nikolai Artemiev, Sergii Dmytruk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/64375 to look at the new patch set (#6). Change subject: writeprotect.c: Allow opaque masters to hook spi_{rw}_register() ...................................................................... writeprotect.c: Allow opaque masters to hook spi_{rw}_register() Allow specialisation in opaque masters, such as ichspi hwseq, to write to status registers. Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M include/programmer.h M writeprotect.c 2 files changed, 26 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/64375/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/64375 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Gerrit-Change-Number: 64375 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: spi25_statusreg.c: Allow opaque masters to hook spi_{rw}_register()
by Edward O'Callaghan (Code Review) 29 May '22

29 May '22

Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64375 ) Change subject: spi25_statusreg.c: Allow opaque masters to hook spi_{rw}_register() ...................................................................... Patch Set 5: Code-Review-1 (1 comment) File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/64375/comment/e19c8ce1_3afee909 PS5, Line 33: if ((flash->mst->buses_supported & BUS_PROG) && flash->mst->opaque.write_register) { : return flash->mst->opaque.write_register(flash, reg, value); : } I am inclined to move this specalisation to actually within writeprotect.c I feel this layer violates as-is and that flashrom probably should be refactored some to wrap up the spi command issuing semantics and master selection behind some kind of internal API. -- To view, visit https://review.coreboot.org/c/flashrom/+/64375 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ab0d7f5f48338c8ecb118a69651c203fbc516ac Gerrit-Change-Number: 64375 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 29 May 2022 02:05:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2022