Attention is currently required from: Edward O'Callaghan.
Hello build bot (Jenkins), Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/66677
to look at the new patch set (#6).
Change subject: flashrom.c: Make programmer_{un}map_flash_region() static
......................................................................
flashrom.c: Make programmer_{un}map_flash_region() static
Change-Id: Ic02092ce23c5b3233aad38343b888e3fa7e5bcf9
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M flashrom.c
M include/flash.h
2 files changed, 12 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/66677/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/66677
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic02092ce23c5b3233aad38343b888e3fa7e5bcf9
Gerrit-Change-Number: 66677
Gerrit-PatchSet: 6
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/67475 )
Change subject: flashrom.c: Confine is_internal checks under a symbol
......................................................................
flashrom.c: Confine is_internal checks under a symbol
We wish for printers to be parametric on the flag state
of if we are internal or not and not embed this side-effect.
Change-Id: I569811798bfe310c59b8b61afba359bef68969fb
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M flashrom.c
1 file changed, 22 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/67475/1
diff --git a/flashrom.c b/flashrom.c
index 0f07e44..402ba94 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1307,10 +1307,10 @@
#endif
}
-static void nonfatal_help_message(void)
+static void nonfatal_help_message(bool is_internal)
{
msg_gerr("Good, writing to the flash chip apparently didn't do anything.\n");
- if (is_internal_programmer())
+ if (is_internal)
msg_gerr("This means we have to add special support for your board, programmer or flash\n"
"chip. Please report this to the mailing list at flashrom(a)flashrom.org or on\n"
"IRC (see https://www.flashrom.org/Contact for details), thanks!\n"
@@ -1323,10 +1323,10 @@
"https://www.flashrom.org/Contact for details), thanks!\n");
}
-static void emergency_help_message(void)
+static void emergency_help_message(bool is_internal)
{
msg_gerr("Your flash chip is in an unknown state.\n");
- if (is_internal_programmer())
+ if (is_internal)
msg_gerr("Get help on IRC (see https://www.flashrom.org/Contact) or mail\n"
"flashrom(a)flashrom.org with the subject \"FAILED: <your board name>\"!"
"-------------------------------------------------------------------------------\n"
@@ -1609,7 +1609,7 @@
* knows very well that booting won't work.
*/
if (ret)
- emergency_help_message();
+ emergency_help_message(is_internal_programmer());
return ret;
}
@@ -1741,18 +1741,18 @@
if (!flashctx->chip->read(flashctx, curcontents, 0, flash_size)) {
msg_cinfo("done.\n");
if (!memcmp(oldcontents, curcontents, flash_size)) {
- nonfatal_help_message();
+ nonfatal_help_message(is_internal_programmer());
goto _finalize_ret;
}
msg_cerr("Apparently at least some data has changed.\n");
} else
msg_cerr("Can't even read anymore!\n");
- emergency_help_message();
+ emergency_help_message(is_internal_programmer());
goto _finalize_ret;
} else {
msg_cerr("\n");
}
- emergency_help_message();
+ emergency_help_message(is_internal_programmer());
goto _finalize_ret;
}
@@ -1769,7 +1769,7 @@
/* If we tried to write, and verification now fails, we
might have an emergency situation. */
if (ret)
- emergency_help_message();
+ emergency_help_message(is_internal_programmer());
else
msg_cinfo("VERIFIED.\n");
} else {
--
To view, visit https://review.coreboot.org/c/flashrom/+/67475
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I569811798bfe310c59b8b61afba359bef68969fb
Gerrit-Change-Number: 67475
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newchange
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/67474 )
Change subject: flashrom.c: Make emergency_help_message() static local
......................................................................
flashrom.c: Make emergency_help_message() static local
emergency help is relevent only to that of the internal
programmer however the cli logic should be agnostic of
that.
Change-Id: Ib0d390c44e7a851e684014925a25c8b259b810cd
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M cli_classic.c
M flashrom.c
M include/flash.h
3 files changed, 24 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/67474/1
diff --git a/cli_classic.c b/cli_classic.c
index bac31d1..dc4a49e 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -1147,15 +1147,6 @@
ret = do_extract(fill_flash);
else if (erase_it) {
ret = flashrom_flash_erase(fill_flash);
- /*
- * FIXME: Do we really want the scary warning if erase failed?
- * After all, after erase the chip is either blank or partially
- * blank or it has the old contents. A blank chip won't boot,
- * so if the user wanted erase and reboots afterwards, the user
- * knows very well that booting won't work.
- */
- if (ret)
- emergency_help_message();
}
else if (write_it)
ret = do_write(fill_flash, filename, referencefile);
diff --git a/flashrom.c b/flashrom.c
index 81d3f8d..0f07e44 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1323,7 +1323,7 @@
"https://www.flashrom.org/Contact for details), thanks!\n");
}
-void emergency_help_message(void)
+static void emergency_help_message(void)
{
msg_gerr("Your flash chip is in an unknown state.\n");
if (is_internal_programmer())
@@ -1601,6 +1601,15 @@
finalize_flash_access(flashctx);
+ /*
+ * FIXME: Do we really want the scary warning if erase failed?
+ * After all, after erase the chip is either blank or partially
+ * blank or it has the old contents. A blank chip won't boot,
+ * so if the user wanted erase and reboots afterwards, the user
+ * knows very well that booting won't work.
+ */
+ if (ret)
+ emergency_help_message();
return ret;
}
diff --git a/include/flash.h b/include/flash.h
index 79aaa64..0d2dc1b 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -417,7 +417,6 @@
int erase_flash(struct flashctx *flash);
int probe_flash(struct registered_master *mst, int startchip, struct flashctx *fill_flash, int force);
int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len);
-void emergency_help_message(void);
void print_version(void);
void print_buildinfo(void);
void print_banner(void);
--
To view, visit https://review.coreboot.org/c/flashrom/+/67474
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib0d390c44e7a851e684014925a25c8b259b810cd
Gerrit-Change-Number: 67474
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/+/67074 )
Change subject: flashrom.c: Make 'chip_to_probe' a param to probe_flash()
......................................................................
Patch Set 1:
(1 comment)
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/67074/comment/6d60b7b4_f988d953
PS1, Line 326: const char *chip_to_probe; /* workaround for probe_w29ee011(). */
> The alternative ending has been prepared at CB:67091
I have reviewed an alternative ending and then I discovered this one :) Is this one still needed or are you switching to alternative ending (the latter is a part of the larger chain)?
--
To view, visit https://review.coreboot.org/c/flashrom/+/67074
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I22a6c610e3f3c5a26898d4bbfafbb4df3c07ef6e
Gerrit-Change-Number: 67074
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(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: Fri, 09 Sep 2022 09:04:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nikolai Artemiev.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67195 )
Change subject: tree/: Convert flashchip decode range func ptr to enum
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/67195
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If6d08d414d3d1ddadc95ca1d407fc87c23ab543d
Gerrit-Change-Number: 67195
Gerrit-PatchSet: 5
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 09 Sep 2022 08:56:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67240 )
Change subject: tree/: Convert flashchip erase_block func ptr to enumerate
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/67240/comment/8766e367_1e54b3aa
PS5, Line 307: // TODO rename to &jedec_sector_erase;
Just check: my understanding these TODOs and FIXME planned to be addressed in a separate patch later? If yes, and especially if that belongs to "not urgent, but would be nice to do it later" category, can you please create a ticket in bugtracker, and set category "Easy Projects"? Thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/67240
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I58415a4f2589812b26d284c96876e88f8d9acf52
Gerrit-Change-Number: 67240
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
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: Fri, 09 Sep 2022 08:51:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment