October 2022 - flashrom-gerrit

[S] Change in flashrom[master]: s25f.c: Fix undefined behaviour on shift
by Evan Benn (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Nikolai Artemiev, Evan Benn. Hello build bot (Jenkins), Angel Pons, Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68127 to look at the new patch set (#2). Change subject: s25f.c: Fix undefined behaviour on shift ...................................................................... s25f.c: Fix undefined behaviour on shift dev_id, a uint8_t, was shifted left by 24 bits. After promotion to int, this results in shifting into the sign bit, which is undefined behaviour. Cast to uint32_t to prevent the promotion to signed int. BUG=None BRANCH=None TEST=None Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M s25f.c 1 file changed, 22 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/68127/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Gerrit-Change-Number: 68127 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: ps25f.c: Fix UB on shift
by Angel Pons (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Nikolai Artemiev, Evan Benn. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68127 ) Change subject: ps25f.c: Fix UB on shift ...................................................................... Patch Set 1: Code-Review+2 (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/68127/comment/c5abe207_329b2275 PS1, Line 7: UB There's enough space to use the full form: `undefined behavior` (or `undefined behaviour`, pick your poison 😄) https://review.coreboot.org/c/flashrom/+/68127/comment/f51c73f9_d7164d33 PS1, Line 7: p spurious `p` https://review.coreboot.org/c/flashrom/+/68127/comment/9a62eaa2_4b8b8d7d PS1, Line 9: 24 We had a teacher that would say "24 what? Potatoes?". Maybe reword this sentence as follows: dev_id, a u8, was shifted left 24 bits. -- To view, visit https://review.coreboot.org/c/flashrom/+/68127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Gerrit-Change-Number: 68127 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 01:02:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
by Angel Pons (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Jonathon Hall, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ...................................................................... Patch Set 8: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/67695/comment/2c345e9d_b5a0819b PS8, Line 869: #if CONFIG_INTERNAL == 1 > Angel Pons suggested this to me elsewhere but maybe drop the pre-processor if branch here && (see ab […] Not sure if `physical_memory` is always available. If it is, sure. -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 8 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 00:59:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ps25f.c: Fix UB on shift
by Evan Benn (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Nikolai Artemiev. Hello Nikolai Artemiev, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/68127 to review the following change. Change subject: ps25f.c: Fix UB on shift ...................................................................... ps25f.c: Fix UB on shift dev_id, a u8, was shifted left 24. After promotion to int, this results in shifting into the sign bit, which is undefined behaviour. Cast to uint32 to prevent the promotion to signed int. BUG=None BRANCH=None TEST=None Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M s25f.c 1 file changed, 22 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/68127/1 diff --git a/s25f.c b/s25f.c index 67715ed..1133ef8 100644 --- a/s25f.c +++ b/s25f.c @@ -380,10 +380,10 @@ */ uint32_t model_id = - dev_id[1] << 24 | - dev_id[2] << 16 | - dev_id[4] << 8 | - dev_id[5] << 0; + (uint32_t)dev_id[1] << 24 | + (uint32_t)dev_id[2] << 16 | + (uint32_t)dev_id[4] << 8 | + (uint32_t)dev_id[5] << 0; if (dev_id[0] == flash->chip->manufacture_id && model_id == flash->chip->model_id) return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/68127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Gerrit-Change-Number: 68127 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
by Edward O'Callaghan (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Jonathon Hall, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ...................................................................... Patch Set 8: Code-Review+2 (7 comments) Patchset: PS2: > Got this reworked, I agree that it's a lot cleaner. […] I am ok to resolve here. Patchset: PS5: > I brought back the SPI master mappers, it was pretty straightforward. […] I am ok to resolve here. File flashrom.c: https://review.coreboot.org/c/flashrom/+/67695/comment/c76496f0_f208b98f PS8, Line 222: ( nit: `if (` https://review.coreboot.org/c/flashrom/+/67695/comment/d507cc79_0db159fb PS8, Line 249: #if CONFIG_INTERNAL == 1 drop this || move inside symbol and make the `master_uses_physmap()` always available (see comment below)? https://review.coreboot.org/c/flashrom/+/67695/comment/2fd12929_a49b35bf PS8, Line 869: #if CONFIG_INTERNAL == 1 Angel Pons suggested this to me elsewhere but maybe drop the pre-processor if branch here && (see above comment). File ichspi.c: https://review.coreboot.org/c/flashrom/+/67695/comment/06d1f5dc_30431b0d PS2, Line 1865: &mapper_phys > Alrighty this is a lot more understandable now! I split this up into several patches and rebased on […] I am ok to resolve here. File serprog.c: https://review.coreboot.org/c/flashrom/+/67695/comment/1983d826_9c8ae4f1 PS5, Line 919: 1024 > We have `MiB` too apparently! Done Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 8 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 00:02:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Remove custom mappers from opaque_master
by Edward O'Callaghan (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68092 ) Change subject: flashrom.c: Remove custom mappers from opaque_master ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68092 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I36f05154edda371b51f8ff416f019837ff1c243d Gerrit-Change-Number: 68092 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Comment-Date: Tue, 04 Oct 2022 23:50:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: dummyflasher.c: Remove custom mapper from opaque_master
by Edward O'Callaghan (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68091 ) Change subject: dummyflasher.c: Remove custom mapper from opaque_master ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76ae3e0c2e91ecba4fd320941bd1eff038050731 Gerrit-Change-Number: 68091 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Comment-Date: Tue, 04 Oct 2022 23:49:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ichspi: Do not attempt to map physical memory for hwseq
by Edward O'Callaghan (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68090 ) Change subject: ichspi: Do not attempt to map physical memory for hwseq ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68090 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie698071c3181e988f10b750b0e50c9700efaa1a3 Gerrit-Change-Number: 68090 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Comment-Date: Tue, 04 Oct 2022 23:48:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: [WIP] test_build.sh: Rework programmer selection using Meson
by Felix Singer (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Nico Huber, Angel Pons. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68068 ) Change subject: [WIP] test_build.sh: Rework programmer selection using Meson ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > > Patch Set 3: […] Maybe we should just go with the programmer list in test_build.sh. Everything else seems overly complex to me and might introduce other problems. -- To view, visit https://review.coreboot.org/c/flashrom/+/68068 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9f41ce2ff219c70c3c05a90134291b01a084c859 Gerrit-Change-Number: 68068 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 04 Oct 2022 19:01:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: [WIP] test_build.sh: Rework programmer selection using Meson
by Felix Singer (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Nico Huber, Angel Pons. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68068 ) Change subject: [WIP] test_build.sh: Rework programmer selection using Meson ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > Patch Set 3: > > I would prefer using the `option('programmer'` of `meson_options.txt` and parse it with awk or so to generate the list. This may be more stable than the summary generated by meson. > And this parser could then also be used to test other options in the future Me too, but I don't like inventing a parser for it. -- To view, visit https://review.coreboot.org/c/flashrom/+/68068 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9f41ce2ff219c70c3c05a90134291b01a084c859 Gerrit-Change-Number: 68068 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 04 Oct 2022 18:44:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2022