January 2023 - flashrom-gerrit

[L] Change in flashrom[master]: Add AMD Ryzen Support & VIA VL805 Support
by Arthur Heymans (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: ChrisEric1 CECL, Thomas Heijligen. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72020 ) Change subject: Add AMD Ryzen Support & VIA VL805 Support ...................................................................... Patch Set 6: (3 comments) Patchset: PS6: Please drop the AMD changes and just add in VL805 in one patch. File sb600spi.c: https://review.coreboot.org/c/flashrom/+/72020/comment/7ad4595c_2f5a0f66 PS6, Line 56: uint8_t RZN32BM = 0; Accessing larger than 16M is indeed a problem. https://review.coreboot.org/c/flashrom/+/71565 fixes it. https://review.coreboot.org/c/flashrom/+/72020/comment/de025556_ec0d118f PS6, Line 128: (rev == 0x51 || rev == 0x59 || rev == 0x61) Those cases are covered below... -- To view, visit https://review.coreboot.org/c/flashrom/+/72020 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7452b169ad89e0a884a46acc6a0499e97bb6ff7e Gerrit-Change-Number: 72020 Gerrit-PatchSet: 6 Gerrit-Owner: ChrisEric1 CECL <christopherericlentocha(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: ChrisEric1 CECL <christopherericlentocha(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Wed, 18 Jan 2023 06:35:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Use Range and usize for all ranges
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72004 ) Change subject: flashrom_tester: Use Range and usize for all ranges ...................................................................... Patch Set 6: (2 comments) File util/flashrom_tester/flashrom/src/cmd.rs: https://review.coreboot.org/c/flashrom/+/72004/comment/9f65720e_71bfde2d PS5, Line 56: usize > I think `u64` is more appropriate, since we don't really care how this relates to the system's point […] yes this is where we get into the mess of the flashrom API types. getsize returns a `size_t`. Other parts of the API do other stuff, including `int`, `unsigned int` uint32_t and more :'). usize is big enough on 32 bit platforms to hold flashrom range types, because those dont even get to 32 bits afaict. The advantage of usize is that I can index into arrays with it without annoying type conversions `buf[range]`. I have deja vu, I think we had this conversation last time round! https://review.coreboot.org/c/flashrom/+/72004/comment/1d664a3b_e3223234 PS5, Line 56: start,length > It's confusing to describe this as `(start,length)` just because the range itself is using the usual […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/72004 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic09be66c3598acf45fbe8952093006a9b185810a Gerrit-Change-Number: 72004 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Jan 2023 06:12:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Check WP range after setting it
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71975 ) Change subject: flashrom_tester: Check WP range after setting it ...................................................................... Patch Set 7: (4 comments) File util/flashrom_tester/flashrom/src/cmd.rs: https://review.coreboot.org/c/flashrom/+/71975/comment/111f5c46_32ee18cf PS6, Line 207: Range<usize> > Is it possible for multiple ranges to be protected at the same time? No, although the stuff nikolai is working on might introduce this, it doesnt exist yet. https://evanbenn.users.x20web.corp.google.com/www/libflashrom-doc/html/grou… https://review.coreboot.org/c/flashrom/+/71975/comment/50ff5b4b_3ddbdb2b PS6, Line 227: \d > `[[:xdigit:]]` ! lucky that worked in my test? https://review.coreboot.org/c/flashrom/+/71975/comment/6605ec4d_e7c82de3 PS6, Line 231: range_match.get(1).unwrap().as_str() > You can use `range_match[1]` to be more concise. Done File util/flashrom_tester/flashrom/src/lib.rs: https://review.coreboot.org/c/flashrom/+/71975/comment/7b6dc926_5e088838 PS6, Line 123: range > "range of addresses that are protected" seems a little clearer. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/71975 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I464b1f1393a275a0e636654c6aec7b7b94bcb16c Gerrit-Change-Number: 71975 Gerrit-PatchSet: 7 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Jan 2023 06:11:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Check WP range after setting it
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Evan Benn. Hello build bot (Jenkins), Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71975 to look at the new patch set (#8). Change subject: flashrom_tester: Check WP range after setting it ...................................................................... flashrom_tester: Check WP range after setting it Double check that fetching the software write protect range after we set it matches what we tried to set it to. BUG=b:236085139 BRANCH=None TEST=None Change-Id: I464b1f1393a275a0e636654c6aec7b7b94bcb16c Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/flashrom/Cargo.toml M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tester.rs 5 files changed, 63 insertions(+), 26 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/71975/8 -- To view, visit https://review.coreboot.org/c/flashrom/+/71975 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I464b1f1393a275a0e636654c6aec7b7b94bcb16c Gerrit-Change-Number: 71975 Gerrit-PatchSet: 8 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[master]: flashrom_tester: Rewrite IOOpts to support more operations
by Peter Marheine (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Evan Benn. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71973 ) Change subject: flashrom_tester: Rewrite IOOpts to support more operations ...................................................................... Patch Set 5: (1 comment) File util/flashrom_tester/flashrom/src/cmd.rs: https://review.coreboot.org/c/flashrom/+/71973/comment/b79c4b9e_194537a7 PS4, Line 82: // an operation must provide a file or a region.file or both. > I tried it a bit like this way initially, I didn't like that the things inside the enum variants don […] If each operation can take both a region and file path (or more generally, if every operation can take the same options), then it seems okay to not put fields in the `Operation` and instead put it alongside a `Region` like I've outlined; the bigger win seems like how the `Region` better expresses where data might come from. If you want named fields you can always use struct variants (so you don't need to define more structs): ``` enum Operation { Read { region: Region, path: Path } ... } ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/71973 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1cb46bb1b26949fd9c19949c43708a8b652e00da Gerrit-Change-Number: 71973 Gerrit-PatchSet: 5 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 18 Jan 2023 06:05:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: flashrom_tester: Rewrite IOOpts to support more operations
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71973 ) Change subject: flashrom_tester: Rewrite IOOpts to support more operations ...................................................................... Patch Set 5: (2 comments) File util/flashrom_tester/flashrom/src/cmd.rs: https://review.coreboot.org/c/flashrom/+/71973/comment/4acc3667_be24a538 PS4, Line 82: // an operation must provide a file or a region.file or both. > Does this vary with what operation is being performed? It might be more obvious (regarding correct u […] I tried it a bit like this way initially, I didn't like that the things inside the enum variants dont have names. I suppose I could put a struct in each though. I did not notice that Erase can have a region, but I'm not using it yet so I'll leave it off. https://review.coreboot.org/c/flashrom/+/71973/comment/4ebcb97b_beebed73 PS4, Line 148: #[allow(clippy::needless_update)] // clippy doesnt like Default when all fields are filled > Seems okay to omit the `default` then, unless you anticipate adding more fields that would require a […] I had thought the point of it was to save me if I add a field back in and forget to modify this site. I think I am misunderstanding -- To view, visit https://review.coreboot.org/c/flashrom/+/71973 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1cb46bb1b26949fd9c19949c43708a8b652e00da Gerrit-Change-Number: 71973 Gerrit-PatchSet: 5 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Jan 2023 06:00:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom_tester: Rename simplify lock_test hwwp_locks_swwp_test
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71972 ) Change subject: flashrom_tester: Rename simplify lock_test hwwp_locks_swwp_test ...................................................................... Patch Set 2: (1 comment) File util/flashrom_tester/src/tests.rs: https://review.coreboot.org/c/flashrom/+/71972/comment/bb1c40ec_6dd3326f PS1, Line 208: .set_sw(true)?; > This changes the ordering to now enable HW before SW. […] it did still work on the duts I tried it on. I'll just leave it how it was, chestertons fence and all -- To view, visit https://review.coreboot.org/c/flashrom/+/71972 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6080622755ff16d8fba7044b38f9e09db0c62f97 Gerrit-Change-Number: 71972 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Jan 2023 05:59:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Check WP range after setting it
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Evan Benn. Hello build bot (Jenkins), Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71975 to look at the new patch set (#7). Change subject: flashrom_tester: Check WP range after setting it ...................................................................... flashrom_tester: Check WP range after setting it Double check that fetching the software write protect range after we set it matches what we tried to set it to. BUG=b:236085139 BRANCH=None TEST=None Change-Id: I464b1f1393a275a0e636654c6aec7b7b94bcb16c Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/flashrom/Cargo.toml M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tester.rs 5 files changed, 63 insertions(+), 26 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/71975/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/71975 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I464b1f1393a275a0e636654c6aec7b7b94bcb16c Gerrit-Change-Number: 71975 Gerrit-PatchSet: 7 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashrom_tester: Use Range and usize for all ranges
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Evan Benn. Hello build bot (Jenkins), Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/72004 to look at the new patch set (#6). Change subject: flashrom_tester: Use Range and usize for all ranges ...................................................................... flashrom_tester: Use Range and usize for all ranges i64 was being used for various range and size types. Use an unsigned type as returned by libflashrom. Replace the undocumented i64 pair types representing start,end with a built in range to eliminate ambiguity. BUG=b:236085139 BRANCH=None TEST=None Change-Id: Ic09be66c3598acf45fbe8952093006a9b185810a Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs M util/flashrom_tester/src/utils.rs 6 files changed, 71 insertions(+), 55 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/72004/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/72004 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic09be66c3598acf45fbe8952093006a9b185810a Gerrit-Change-Number: 72004 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashrom_tester: Add positive check to verify_fail_test
by Evan Benn (Code Review) 18 Jan '23

18 Jan '23

Attention is currently required from: Peter Marheine. Hello build bot (Jenkins), Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71974 to look at the new patch set (#6). Change subject: flashrom_tester: Add positive check to verify_fail_test ...................................................................... flashrom_tester: Add positive check to verify_fail_test In verify_fail_test test that verify works when expected, as well as fails when expected. A verify_region_from_file function is added to support this. BUG=b:235916336 BRANCH=None TEST=None Change-Id: Ibbcc97086466b67cfab4f6c32140bb5f2c456beb Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tests.rs 4 files changed, 72 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/71974/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/71974 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibbcc97086466b67cfab4f6c32140bb5f2c456beb Gerrit-Change-Number: 71974 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2023