Attention is currently required from: Edward O'Callaghan.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70026 )
Change subject: internal.c: laptop_ok global state can become stale
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/70026/comment/373be7cd_708db168
PS1, Line 30: BUG=b:260518132,b:260518132
These are both the same.
--
To view, visit https://review.coreboot.org/c/flashrom/+/70026
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2c00c351904307eeb1488c5dfaffc91d6468ee25
Gerrit-Change-Number: 70026
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 28 Nov 2022 00:37:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70026 )
Change subject: internal.c: laptop_ok global state can become stale
......................................................................
internal.c: laptop_ok global state can become stale
Craask and similar DUT's are erroneously probing random second chips.
```
Found chipset "Intel Alder Lake-N".
Enabling flash write... SPI Configuration is locked down.
FREG0: Flash Descriptor region (0x00000000-0x00000fff) is read-write.
FREG1: BIOS region (0x003a0000-0x00ffffff) is read-write.
FREG2: Management Engine region (0x00001000-0x0039ffff) is read-write.
OK.
Found Winbond flash chip "W25Q128.V..M" (16384 kB, Programmer-specific) on host.
Warning: Setting BIOS Control at 0xdc from 0x8b to 0x89 failed.
New value is 0x8b.
Found MoselVitelic flash chip "V29C51000T" (64 kB, Parallel) mapped at physical address 0x00000000ffff0000.
```
This seems to be due to `laptop_ok` becoming a stale global state
after the first operation leading to probing on unrelated buses.
Therefore unconditionally reset the global state upon entry into
the internal driver.
BUG=b:260518132,b:260518132
TEST=Craask reportly no longer finds duplicate chip.
Change-Id: I2c00c351904307eeb1488c5dfaffc91d6468ee25
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M internal.c
1 file changed, 37 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/70026/1
diff --git a/internal.c b/internal.c
index 4f68456..9b80f37 100644
--- a/internal.c
+++ b/internal.c
@@ -211,6 +211,9 @@
if (ret)
return ret;
+ /* Unconditionally reset global state from previous operation. */
+ laptop_ok = false;
+
/* Default to Parallel/LPC/FWH flash devices. If a known host controller
* is found, the host controller init routine sets the
* internal_buses_supported bitfield.
--
To view, visit https://review.coreboot.org/c/flashrom/+/70026
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2c00c351904307eeb1488c5dfaffc91d6468ee25
Gerrit-Change-Number: 70026
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70003 )
Change subject: cli, manpage: Clean up occurrences of old image argument
......................................................................
Patch Set 3:
(2 comments)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/70003/comment/5e636031_8fd240f2
PS1, Line 621: include
> Oh no... did I just do the blasphemy of changing the public API! how could I... %( […]
Done
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/70003/comment/8c68c31e_beb280ae
PS2, Line 622: {"image", 1, NULL, 'i'},
> Add a comment here that this is deprecated
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/70003
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134
Gerrit-Change-Number: 70003
Gerrit-PatchSet: 3
Gerrit-Owner: 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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sun, 27 Nov 2022 23:57:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/70003
to look at the new patch set (#3).
Change subject: cli, manpage: Clean up occurrences of old image argument
......................................................................
cli, manpage: Clean up occurrences of old image argument
--include argument was introduced and replaced --image argument in
commit 45d50a101e8073191e6d88143990ed91d3bfe815
This patch cleans up remaining few places where old `--image`
argument was mentioned so that now all the documentation has
`--include`. --image is deprecated.
Both old --image and new --include have the same short version -i
and it remains the same. The code remains the same since the code
handles --include already.
Tested by running
flashrom -h
man ./flashrom.8.tmpl
Ticket: https://ticket.coreboot.org/issues/372
Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M cli_classic.c
M flashrom.8.tmpl
2 files changed, 32 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/70003/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/70003
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134
Gerrit-Change-Number: 70003
Gerrit-PatchSet: 3
Gerrit-Owner: 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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Angel Pons.
Liam Flaherty has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69713 )
Change subject: flashchips.c: Add 4BA write to XM25Qx256C
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69713/comment/c89afefe_aa1afb81
PS2, Line 11:
> I don't have access to these chips, so I have left the `tested` field as `TEST_UNTESTED`. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/69713
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I96c80762fcda2af6028c7a53d8c545b0c6565cbd
Gerrit-Change-Number: 69713
Gerrit-PatchSet: 3
Gerrit-Owner: Liam Flaherty <liamflaherty(a)chromium.org>
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Sun, 27 Nov 2022 23:18:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Liam Flaherty <liamflaherty(a)chromium.org>
Gerrit-MessageType: comment
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69518 )
Change subject: linux_mtd: Mark Opaque chip as tested for WP
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69518/comment/075da238_fe4ef79a
PS2, Line 9: Since linux_mtd supports write-protect, its probe function needs
Yes, I spent some time thinking about this, before producing this patch and CB:69842.
> What happens if the chip doesn't support write-protect?
Not entirely sure, I will check. But I don't see programmer code ever returning errors like "wp unsuported". Perhaps this is handled at a different level, or maybe everything that goes through linux_mtd can support wp.
The programmer (and all the other opaque master) do not check whether any of the operations are supported. They just set the chip as "everything tested". I guess, by design of opaque masters?
So this is why I made CB:69842 too. Similar motivation, although I made a different commit message for CB:69842 , to be very specific.
In any case, showing to the user "your Opaque chip is not tested, please report on the mailing list" is not much help: what do they report, "Opaque chip"? My understanding this is also by design, that users do not [need to] know which exacty chip that is.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69518
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Icc0521c28555a93f26ce66bdbeaa68590f10c358
Gerrit-Change-Number: 69518
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Comment-Date: Sun, 27 Nov 2022 22:22:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70025 )
Change subject: flashrom.c: Factor out setup_curcontents() from flashrom_image_write()
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
RFC?
--
To view, visit https://review.coreboot.org/c/flashrom/+/70025
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4baf09700d7efcbca3a6ba0cfda3231837e0a34a
Gerrit-Change-Number: 70025
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Comment-Date: Sun, 27 Nov 2022 10:15:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment