Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/67752 )
Change subject: internal.c: Pass `programmer_cfg` to `try_mtd()`
......................................................................
internal.c: Pass `programmer_cfg` to `try_mtd()`
Programmer params are now passed via the `programmer_cfg` struct, but
the internal programmer did not pass them to the `try_mtd()` function
which was still using `NULL`. This problem resulted in a segmentation
fault when trying to use the internal programmer.
TEST=Make sure internal programmer does not segfault on Haswell ULT.
Change-Id: I9e74bd68a1f9509a201dc518dbff96c27d68a3c3
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M include/programmer.h
M internal.c
2 files changed, 24 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/52/67752/1
diff --git a/include/programmer.h b/include/programmer.h
index a7cea5e..17abfd2 100644
--- a/include/programmer.h
+++ b/include/programmer.h
@@ -375,12 +375,15 @@
void probe_superio_ite(void);
int init_superio_ite(const struct programmer_cfg *cfg);
-#if CONFIG_LINUX_MTD == 1
/* trivial wrapper to avoid cluttering internal_init() with #if */
-static inline int try_mtd(void) { return programmer_linux_mtd.init(NULL); };
+static inline int try_mtd(const struct programmer_cfg *cfg)
+{
+#if CONFIG_LINUX_MTD == 1
+ return programmer_linux_mtd.init(cfg);
#else
-static inline int try_mtd(void) { return 1; };
+ return 1;
#endif
+}
/* mcp6x_spi.c */
int mcp6x_spi_init(int want_spi);
diff --git a/internal.c b/internal.c
index 43aa51b..6a8db5e 100644
--- a/internal.c
+++ b/internal.c
@@ -215,7 +215,7 @@
*/
internal_buses_supported = BUS_NONSPI;
- if (try_mtd() == 0) {
+ if (try_mtd(cfg) == 0) {
ret = 0;
goto internal_init_exit;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/67752
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9e74bd68a1f9509a201dc518dbff96c27d68a3c3
Gerrit-Change-Number: 67752
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange
Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67722 )
Change subject: layout.c: Validate _layout_entry_by_name() arguments before use
......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67722/comment/d3fe1992_cdb9c4b0
PS2, Line 9: It maybe
> Done
Hmmm, was an old patchset repushed?
--
To view, visit https://review.coreboot.org/c/flashrom/+/67722
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9
Gerrit-Change-Number: 67722
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 21 Sep 2022 07:11:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67722 )
Change subject: layout.c: Validate _layout_entry_by_name() arguments before use
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/67722
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9
Gerrit-Change-Number: 67722
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 21 Sep 2022 07:11:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Nikolai Artemiev.
Nikolai Artemiev has uploaded a new patch set (#4) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/67722 )
Change subject: layout.c: Validate _layout_entry_by_name() arguments before use
......................................................................
layout.c: Validate _layout_entry_by_name() arguments before use
It maybe the case that a layout could not be derived which
would result in layout logic feed with a NULL pointer. Validate
this case and be defensive to validate the name argument as well.
BUG=b:247055486
TEST=builds
Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M layout.c
1 file changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/67722/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/67722
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a19c0e586f8575b8b3c2c02b5afad312efacfc9
Gerrit-Change-Number: 67722
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nikolai Artemiev.
Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/67725
to look at the new patch set (#3).
Change subject: cli_classic.c: Add error messages for invalid --wp-region use
......................................................................
cli_classic.c: Add error messages for invalid --wp-region use
Print warning if --wp-region is used without a layout file or the layout
file doesn't contain the region.
BUG=b:247055486
TEST=builds
Change-Id: Ie606ba7f8a423405099679ca62169c395d994b5d
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M cli_classic.c
1 file changed, 27 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/67725/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/67725
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie606ba7f8a423405099679ca62169c395d994b5d
Gerrit-Change-Number: 67725
Gerrit-PatchSet: 3
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67723 )
Change subject: cli_classic.c: Early init of layout obscures invalid memory access
......................................................................
Patch Set 2:
(3 comments)
Patchset:
PS2:
n
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/67723/comment/91978e8d_106a8121
PS2, Line 541: struct flashrom_layout *layout = NULL;
> The issue is not that `NULL` means 'no layout' but rather it is the attempted use of a layout before […]
Ultimately `layout` can still end up as NULL if no layout provided/found, so anything that uses it (e.g. get_region_range call below) should check it is non-null.
But we can't really get the compiler to enforce that since it only checks the variable has been initialized, and it doesn't check for initialized-to-non-null.
I'm ok with leaving layout set to NULL here instead of setting it to NULL in the new `else` block below.
An alternative would be to delete `default_layout` in `struct flashctx` and assign `flash->layout` to a default layout in `probe_flash()`. Then users can override the default `flash->layout` value if they want, and flash->layout will always be a valid layout.
Anyway I'm going to move this patch to the end since the other two are sufficient to fix the segfault.
https://review.coreboot.org/c/flashrom/+/67723/comment/0fe60a19_efc92a2a
PS2, Line 1110: msg_gdbg("Valid layout could not be found without image.\n");
> Nik, can you come up with a better string here for your bug. […]
Maybe we should change it to "No layout provided or found in image", it's really just for debugging.
--
To view, visit https://review.coreboot.org/c/flashrom/+/67723
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3df61b873da27f6945d60916a1c3713dedfc3924
Gerrit-Change-Number: 67723
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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-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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 21 Sep 2022 05:13:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67004 )
Change subject: Use chipsize_t (uint32_t) for all range types
......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS4:
> Part of me wants to split this patch into smaller ones. […]
Thanks Anastasia, good point. I have been reworking as a series of patches, my feeble brain cannot handle all the changes in one go either! I will try to get it up today. Easier to squash after review than to split in my opinion.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/67004/comment/a629ef3a_41378f81
PS4, Line 146: *start != tmp
> What is this condition checking? I am confused because the previous line made the assignment and thi […]
Yes this is checking that the user provided value can fit in a chipsize_t. I think I will remove it.
The point here was that the size of start was being hidden inside chipsize_t, so we really ought to parse a large type and then double check that it fits. But actually we arent so much hiding the size which is publicly uint32_t, as just putting all the sizes into a typedef for some documentation.
--
To view, visit https://review.coreboot.org/c/flashrom/+/67004
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I31348806b7ea92a6ea8391a735f722a63aa7e488
Gerrit-Change-Number: 67004
Gerrit-PatchSet: 4
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 21 Sep 2022 00:10:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Evan Benn.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67004 )
Change subject: Use chipsize_t (uint32_t) for all range types
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
Patchset:
PS4:
Part of me wants to split this patch into smaller ones. But I am interested what other people are thinking.
Smaller ones in my mind
1) Use macros for printing format everywhere
2) Delete chipoff and replace it with chipsize
3) Replace size_t with chipsize_t
4) replace unsigned int with chipsize_t
In any case, even if everyone agrees on splitting: probably better do it later when code is mostly reviewed and any other comments resolved.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/67004/comment/77ef926b_b3834a46
PS4, Line 146: *start != tmp
What is this condition checking? I am confused because the previous line made the assignment and this condition seems to check whether the assignment works?
I am guessing this has something to do with changing types? (since it wasn't in the code before)
--
To view, visit https://review.coreboot.org/c/flashrom/+/67004
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I31348806b7ea92a6ea8391a735f722a63aa7e488
Gerrit-Change-Number: 67004
Gerrit-PatchSet: 4
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
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: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Tue, 20 Sep 2022 23:30:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment