Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37777 )
Change subject: chipset_enable.c: Enable Tiger Lake U support
......................................................................
chipset_enable.c: Enable Tiger Lake U support
Did a basic local test with --flash-name and -r bios and
ran strings to see if the bios contained reasonable data.
BUG=b:146089922
Change-Id: I2c13e0173c9b5e17d2ae197f4a4ab9aa2825c1b3
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M chipset_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/37777/1
diff --git a/chipset_enable.c b/chipset_enable.c
index b55852c..d5a2f03 100644
--- a/chipset_enable.c
+++ b/chipset_enable.c
@@ -1996,6 +1996,7 @@
{0x8086, 0x9d56, B_S, NT, "Intel", "Kaby Lake Y Premium", enable_flash_pch100},
{0x8086, 0x9d58, B_S, NT, "Intel", "Kaby Lake U Premium", enable_flash_pch100},
{0x8086, 0x9d84, B_S, DEP, "Intel", "Cannon Lake U Premium", enable_flash_pch300},
+ {0x8086, 0xa082, B_FS, NT, "Intel", "Tiger Lake U", enable_flash_pch300},
{0x8086, 0xa141, B_S, NT, "Intel", "Sunrise Point Desktop Sample", enable_flash_pch100},
{0x8086, 0xa142, B_S, NT, "Intel", "Sunrise Point Unknown Sample", enable_flash_pch100},
{0x8086, 0xa143, B_S, NT, "Intel", "H110", enable_flash_pch100},
--
To view, visit https://review.coreboot.org/c/flashrom/+/37777
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2c13e0173c9b5e17d2ae197f4a4ab9aa2825c1b3
Gerrit-Change-Number: 37777
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newchange
Miklós Márton has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34641 )
Change subject: usbdev: fix accepting shorter serial numbers than the real one.
......................................................................
usbdev: fix accepting shorter serial numbers than the real one.
Change-Id: Ife8c4e0a957c4345e27ec9ae9e1480ca80fe505c
---
M usbdev.c
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/41/34641/1
diff --git a/usbdev.c b/usbdev.c
index 764ac04..7e41235 100644
--- a/usbdev.c
+++ b/usbdev.c
@@ -104,7 +104,8 @@
msg_pdbg("Serial number is %s\n", myserial);
/* Filter out any serial number that does not commence with serialno */
- return 0 != strncmp(serialno, (char *)myserial, strlen(serialno));
+ return strlen((char *)myserial) == strlen(serialno)
+ && 0 != strncmp(serialno, (char *)myserial, strlen(serialno));
}
struct libusb_device_handle *usb_dev_get_by_vid_pid_serial(
--
To view, visit https://review.coreboot.org/c/flashrom/+/34641
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ife8c4e0a957c4345e27ec9ae9e1480ca80fe505c
Gerrit-Change-Number: 34641
Gerrit-PatchSet: 1
Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-MessageType: newchange
Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/48361 )
Change subject: Fix segfault when iomem=strict
......................................................................
Fix segfault when iomem=strict
As I can see, physmap_common() only returns ERROR_PTR or a real pointer
but never NULL.
Tested on Linux 5.9.12.
Change-Id: I5b11957fc10fd5c7a4fbb7f54fb94d791590f373
Signed-off-by: Evgeny Zinoviev <me(a)ch1p.io>
---
M dmi.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/48361/1
diff --git a/dmi.c b/dmi.c
index c44221c..7488d61 100644
--- a/dmi.c
+++ b/dmi.c
@@ -164,7 +164,7 @@
unsigned int i = 0, j = 0;
uint8_t *dmi_table_mem = physmap_ro("DMI Table", base, len);
- if (dmi_table_mem == NULL) {
+ if (dmi_table_mem == ERROR_PTR) {
msg_perr("Unable to access DMI Table\n");
return;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/48361
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5b11957fc10fd5c7a4fbb7f54fb94d791590f373
Gerrit-Change-Number: 48361
Gerrit-PatchSet: 1
Gerrit-Owner: Evgeny Zinoviev <me(a)ch1p.io>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 )
Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
......................................................................
Patch Set 23:
(1 comment)
Patchset:
PS22:
> This will be a fairly long comment and cover some things mentioned in other comments, but it's proba […]
Disregard typo above, start and len shouldn't be pointers in flashrom_wp_protect_flash(). Also that's just a rough idea for the function name.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58479
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad
Gerrit-Change-Number: 58479
Gerrit-PatchSet: 23
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Wed, 29 Dec 2021 11:36:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 )
Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
......................................................................
Patch Set 23:
(16 comments)
Patchset:
PS22:
> Thinking further about this, there are more reasons to tie […]
This will be a fairly long comment and cover some things mentioned in other comments, but it's probably easiest to put everything here:
I've thought more about what sort of API should be exposed through libflashrom and I don't think exposing the wp_chip_config structure is a good idea, even if the structure is opaque to libflashrom users.
In addition to the config being to tied to the specific chip as you mention, properly sequencing read/modify/write/verify operations is unnecessary work and it increases the risk that introducing a new chip will break existing assumptions in the future.
Another reason to simplify the API is that linux's MTD interface exposes a much simpler protection API. It's really inconvenient to have two APIs for SPI/MTD protection and unifying them makes sense if we don't lost much IMO. Specifically MTD gives us these operations:
- MEMLOCK: Enable HW protection and set range
- MEMUNLOCK: Disable protection / clear range
- MEMISLOCKED: Get current protection range / HW protection status.
The final libdflashrom API / functionality would be something like:
- flashrom_wp_protect_flash(flashctx *, flashsize_t *start, flashsize_t *len) // Read config, set HW protection, set range, write, verify
- flashrom_wp_unprotect_flash(flashctx *) // Read config, unset HW protection, set empty range, write, verify
- flashrom_wp_get_protection_status(flashctx *, flashsize_t *start, flashsize_t *len, bool *hw_prot_enabled) // Read config, decode range / mode, write values to output vars
We can of course expand with additional functions if they're useful, but for now I think these will cover most standard use cases and provide a more flexible API.
The only major thing the new API doesn't allow is setting a protection range without enabling HW protection, but that would just get undone by flashrom or whatever other tool someone next uses to write the flash.
As for migrating the current code, I plan to keep the current writeprotect functions, but keep them inside writeprotect.c and only expose the new functions. Some things that need to be shared will probably get moved to writeprotect.h, like the chip config structure.
What do you think?
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58479/comment/2dc379c2_f68bd077
PS22, Line 131: const
> We should be careful with `const` here. Flash access goes very deep down […]
Done
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58479/comment/e7b9526e_6fae517b
PS22, Line 35: 0
> Will this ever be consumed?
Good point. I thought it would be helpful to ensure it is zeroed if the bit isn't present, but the structure is zero-initialized anyway so we don't need to do that here.
https://review.coreboot.org/c/flashrom/+/58479/comment/cbf7d480_3ccb70ae
PS22, Line 59: *
> No dangling asterisk, please.
I've made the comment a bit bigger, I think the asterisks are ok now since there are leading and trailing comment lines.
https://review.coreboot.org/c/flashrom/+/58479/comment/d9aa7786_76ca184c
PS22, Line 59: intrepret
> int*er*pret
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/b7e234e5_66393683
PS22, Line 61: tmp
> rather `unused` or `ignored`?
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/eefce310_37c9f4b7
PS22, Line 63: *cfg = (struct wp_chip_config) {0};
> Won't this be overwritten where it matters? iow. […]
I think I might have relied on this in an earlier patch set but I don't remember now.
I think it's kind of nice to zero it out before we do anything, else but I can drop it if you want.
https://review.coreboot.org/c/flashrom/+/58479/comment/02149fac_a29c6663
PS22, Line 70: r(
> Missing space.
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/926f9597_d5f22b5d
PS22, Line 70: size_t
> Declarations inside a `for` made trouble in some environment. Please avoid it for now.
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/694d2b85_2eb0d8a4
PS22, Line 70: for(size_t i = 0; bits->bp[i].reg != INVALID_REG; i++) {
> Generally, it's preferred to loop over the target array. This way you […]
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/37c2fc13_d2beeb61
PS22, Line 75: r(
> Missing space.
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/bb2ddb25_6cc35253
PS22, Line 84: *
> No dangling asterisk, please. Or maybe just use the long comment style […]
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/da2b5e4b_56f87daa
PS22, Line 122: INVALID_REG
> This is very confusing. I see it's skipped because the mask would be 0, […]
I've changed it to (INVALID_REG + 1), perhaps STATUS1 would be better though. I'll change it if you want.
https://review.coreboot.org/c/flashrom/+/58479/comment/1b44b521_87138102
PS22, Line 126: = 0
> Why initialize?
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/00045ff9_57888c6e
PS22, Line 144: configuraitons
> configura*ti*ons
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/37c351de_7c726a76
PS22, Line 145: which of two configurations should be used if they both select the same
: * protection range
> Does this really work? How can we say, for instance, that SRP1 being […]
This was kind of an adaptation of an earlier function that just compared ranges so they could be sorted and a preferred range encoding could be chosen.
I realized that it could also be used to compare most bits in the config structure, so I extended it to allow all bits to be compared, which is useful when we want to verify a config we have read back from a chip.
In this context "preferred" is arbitrary, it should only be used to pick between two configurations if both of them are acceptable.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58479
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad
Gerrit-Change-Number: 58479
Gerrit-PatchSet: 23
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Wed, 29 Dec 2021 11:31:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60070 )
Change subject: flashrom.c: Move do_*() helpers into cli_classic.c
......................................................................
Patch Set 4: -Code-Review
--
To view, visit https://review.coreboot.org/c/flashrom/+/60070
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433
Gerrit-Change-Number: 60070
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 29 Dec 2021 00:40:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/60074
to look at the new patch set (#3).
Change subject: build: Remove cli_classic need for internal symbols [WIP]
......................................................................
build: Remove cli_classic need for internal symbols [WIP]
cli_classic should now only use the libflashrom interface.
BUG=b:208132085
TEST=`make && meson/ninja`.
Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M Makefile
M meson.build
2 files changed, 2 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/60074/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/60074
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161
Gerrit-Change-Number: 60074
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset