Author: hailfinger
Date: Wed Oct 20 00:06:20 2010
New Revision: 1215
URL: http://flashrom.org/trac/flashrom/changeset/1215
Log:
Always read the flash chip before writing. This will allow flashrom to
skip erase of already-erased blocks and to skip write of blocks which
already have the wanted contents.
Avoid emergency messages by checking if the chip contents after a failed
write operation (erase/write) are unchanged.
Keep the emergency messages after a failed pure erase. That part is
debatable because if someone wants erase, he pretty sure doesn't care
about the flash contents anymore.
Please note that this introduces additional overhead of a full chip read
before write. This is frowned upon by people with slow programmers.
A followup patch will make this configurable.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Acked-by: Stefan Reinauer <stepan(a)coreboot.org>
Modified:
trunk/flash.h
trunk/flashrom.c
trunk/layout.c
Modified: trunk/flash.h
==============================================================================
--- trunk/flash.h Tue Oct 19 00:32:03 2010 (r1214)
+++ trunk/flash.h Wed Oct 20 00:06:20 2010 (r1215)
@@ -239,7 +239,7 @@
/* layout.c */
int read_romlayout(char *name);
int find_romentry(char *name);
-int handle_romentries(uint8_t *buffer, struct flashchip *flash);
+int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents);
/* spi.c */
struct spi_command {
Modified: trunk/flashrom.c
==============================================================================
--- trunk/flashrom.c Tue Oct 19 00:32:03 2010 (r1214)
+++ trunk/flashrom.c Wed Oct 20 00:06:20 2010 (r1215)
@@ -1343,6 +1343,19 @@
return ret;
}
+void nonfatal_help_message(void)
+{
+ msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
+ "This means we have to add special support for your board, "
+ "programmer or flash chip.\n"
+ "Please report this on IRC at irc.freenode.net (channel "
+ "#flashrom) or\n"
+ "mail flashrom(a)flashrom.org!\n"
+ "-------------------------------------------------------------"
+ "------------------\n"
+ "You may now reboot or simply leave the machine running.\n");
+}
+
void emergency_help_message(void)
{
msg_gerr("Your flash chip is in an unknown state.\n"
@@ -1602,9 +1615,10 @@
*/
int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it)
{
- uint8_t *buf;
+ uint8_t *oldcontents;
+ uint8_t *newcontents;
int ret = 0;
- unsigned long size;
+ unsigned long size = flash->total_size * 1024;
if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it)) {
msg_cerr("Aborting.\n");
@@ -1612,9 +1626,6 @@
return 1;
}
- size = flash->total_size * 1024;
- buf = (uint8_t *) calloc(size, sizeof(char));
-
/* Given the existence of read locks, we want to unlock for read,
* erase and write.
*/
@@ -1627,6 +1638,12 @@
return ret;
}
if (erase_it) {
+ /* 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 (erase_flash(flash)) {
emergency_help_message();
programmer_shutdown();
@@ -1634,33 +1651,71 @@
}
}
+ newcontents = (uint8_t *) calloc(size, sizeof(char));
+
if (write_it || verify_it) {
- if (read_buf_from_file(buf, flash->total_size * 1024, filename)) {
+ if (read_buf_from_file(newcontents, size, filename)) {
programmer_shutdown();
return 1;
}
#if CONFIG_INTERNAL == 1
- show_id(buf, size, force);
+ if (programmer == PROGRAMMER_INTERNAL)
+ show_id(newcontents, size, force);
#endif
}
+ oldcontents = (uint8_t *) calloc(size, sizeof(char));
+
+ /* Read the whole chip to be able to check whether regions need to be
+ * erased and to give better diagnostics in case write fails.
+ * The alternative would be to read only the regions which are to be
+ * preserved, but in that case we might perform unneeded erase which
+ * takes time as well.
+ */
+ msg_cdbg("Reading old flash chip contents...\n");
+ if (flash->read(flash, oldcontents, 0, size)) {
+ programmer_shutdown();
+ return 1;
+ }
+
// This should be moved into each flash part's code to do it
// cleanly. This does the job.
- handle_romentries(buf, flash);
+ handle_romentries(flash, oldcontents, newcontents);
// ////////////////////////////////////////////////////////////
if (write_it) {
if (erase_flash(flash)) {
+ msg_cerr("Uh oh. Erase failed. Checking if anything "
+ "changed.\n");
+ if (!flash->read(flash, newcontents, 0, size)) {
+ if (!memcmp(oldcontents, newcontents, size)) {
+ msg_cinfo("Good. It seems nothing was "
+ "changed.\n");
+ nonfatal_help_message();
+ programmer_shutdown();
+ return 1;
+ }
+ }
emergency_help_message();
programmer_shutdown();
return 1;
}
msg_cinfo("Writing flash chip... ");
- ret = flash->write(flash, buf, 0, flash->total_size * 1024);
+ ret = flash->write(flash, newcontents, 0, size);
if (ret) {
- msg_cerr("FAILED!\n");
+ msg_cerr("Uh oh. Write failed. Checking if anything "
+ "changed.\n");
+ if (!flash->read(flash, newcontents, 0, size)) {
+ if (!memcmp(oldcontents, newcontents, size)) {
+ msg_cinfo("Good. It seems nothing was "
+ "changed.\n");
+ nonfatal_help_message();
+ programmer_shutdown();
+ return 1;
+ }
+ }
emergency_help_message();
programmer_shutdown();
return 1;
@@ -1673,7 +1728,7 @@
/* Work around chips which need some time to calm down. */
if (write_it)
programmer_delay(1000*1000);
- ret = verify_flash(flash, buf);
+ ret = verify_flash(flash, newcontents);
/* If we tried to write, and verification now fails, we
* might have an emergency situation.
*/
Modified: trunk/layout.c
==============================================================================
--- trunk/layout.c Tue Oct 19 00:32:03 2010 (r1214)
+++ trunk/layout.c Wed Oct 20 00:06:20 2010 (r1215)
@@ -205,7 +205,7 @@
return -1;
}
-int handle_romentries(uint8_t *buffer, struct flashchip *flash)
+int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)
{
int i;
@@ -225,13 +225,12 @@
// normal will be updated and the rest will be kept.
for (i = 0; i < romimages; i++) {
-
if (rom_entries[i].included)
continue;
- flash->read(flash, buffer + rom_entries[i].start,
- rom_entries[i].start,
- rom_entries[i].end - rom_entries[i].start + 1);
+ memcpy(newcontents + rom_entries[i].start,
+ oldcontents + rom_entries[i].start,
+ rom_entries[i].end - rom_entries[i].start + 1);
}
return 0;
I noticed that chipset_enable.c has #define _LARGEFILE64_SOURCE but it
does not perform any open/seek, whereas other files (especially
physmap.c) have no indication of large file support, yet they do use
open/seek.
Looking at the code history this seems to be an accident.
What's the correct way to get large file support, and do we need it at all?
Image files will probably stay smaller than 16 MB, so I see no real
problems there, but MSR support and physmap support may need this.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
The cbtable code in flashrom is ancient, and although it grew new
features over time, it is not exactly one of the most readable pieces of
code in flashrom. This is in part due to the complicated integration of
the cbtable code in flashrom, and in part due to the way mapping the
cbtable has to deal with OS constraints.
The goal for cbtable and DMI code is to be well-encapsulated with equal
rights for both.
I believe the following patch is a bugfix, but I'd love to get a very
thorough review for this.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-cbtable_mapping_fixup/cbtable.c
===================================================================
--- flashrom-cbtable_mapping_fixup/cbtable.c (Revision 1199)
+++ flashrom-cbtable_mapping_fixup/cbtable.c (Arbeitskopie)
@@ -203,8 +203,11 @@
struct lb_record *rec, *last;
#ifdef __DARWIN__
- /* This is a hack. DirectHW fails to map physical address 0x00000000.
- * Why?
+ /* This is a hack. DirectHW apparently fails to map physical address
+ * 0x00000000.
+ * FIXME: Modify DirectHW so that it returns (void *)-1 as error
+ * instead of NULL. Maybe the mapping works and earlier failure reports
+ * were just side effects of a direct mapping at 0x00000000.
*/
start = 0x400;
#else
@@ -216,16 +219,25 @@
return -1;
}
- lb_table = find_lb_table(table_area, 0x00000, 0x1000);
+ /* (start - start) is used to make it explicit that the search begins
+ * at start which is mapped at offset (start - start) == 0.
+ */
+ lb_table = find_lb_table(table_area, start - start, 0x1000 - start);
if (!lb_table)
lb_table = find_lb_table(table_area, 0xf0000 - start, BYTES_TO_MAP - start);
if (lb_table) {
struct lb_forward *forward = (struct lb_forward *)
(((char *)lb_table) + lb_table->header_bytes);
if (forward->tag == LB_TAG_FORWARD) {
- start = forward->forward;
- start &= ~(getpagesize() - 1);
- physunmap(table_area, BYTES_TO_MAP);
+ unsigned long newstart;
+ newstart = forward->forward;
+ newstart &= ~(getpagesize() - 1);
+ /* We know the location of the new table, unmap the
+ * previous table.
+ */
+ physunmap(table_area, BYTES_TO_MAP - start);
+ start = newstart;
+ /* FIXME: Do we really want to map 1 MB? */
table_area = physmap_try_ro("high tables", start, BYTES_TO_MAP);
if (ERROR_PTR == table_area) {
msg_perr("Failed getting access to coreboot "
@@ -252,5 +264,7 @@
lb_table->table_entries);
search_lb_records(rec, last, addr + lb_table->header_bytes);
+ /* FIXME: unmap the table area here or will any code access it later? */
+
return 0;
}
--
http://www.hailfinger.org/
Hi flashrom devs!
My attempt to flash BIOS with http://dlcdnet.asus.com/pub/ASUS/Barebone/P1-P5945GC/P5L8L-SE_0705.zip failed miserably:
Writing flash chip... Erasing flash chip... ERASE FAILED at 0x000000f0! Expected=0xff, Read=0x50, failed byte count from 0x00000000-0x00000fff: 0xee7
As told on #flashrom, I'm attaching output of following commands:
# superiotool -deV >superiotool.txt
# lspci -nnvvvxxx >lspci.txt
# flashrom -V >flashrom.txt
<facepalm>
Unfortunately, I did not make any backup image.
</facepalm>
Any help is appreciated.
Thanks in advance,
--
Jollyroger
patch attached.
Tested on a W25Q80...
Signed-off-by: David Hendricks <dhendrix(a)google.com>
--
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
Add a function which checks erase and write operations and makes sure
that neither touch anything outside the desired region.
Runtime is O(n^2).
A less paranoid version can have O(n) runtime, but I think it is
valuable to have the paranoid option.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-test_chip_infrastructure/flash.h
===================================================================
--- flashrom-test_chip_infrastructure/flash.h (Revision 1212)
+++ flashrom-test_chip_infrastructure/flash.h (Arbeitskopie)
@@ -37,6 +37,8 @@
/* Error codes */
#define TIMEOUT_ERROR -101
+#define VERIFY_ERROR -201
+#define COMMAND_ERROR -202
typedef unsigned long chipaddr;
Index: flashrom-test_chip_infrastructure/flashrom.c
===================================================================
--- flashrom-test_chip_infrastructure/flashrom.c (Revision 1212)
+++ flashrom-test_chip_infrastructure/flashrom.c (Arbeitskopie)
@@ -1271,6 +1271,65 @@
return ret;
}
+/* This function expects the previous flash contents in oldcontents, and the
+ * wanted flash contents in newcontents. Usually both are test patterns, and
+ * they should be distinct. The region from start to start+len-1 is expected
+ * to be already erased.
+ */
+int autotest_write_eraseregion_paranoid(struct flashchip *flash,
+ unsigned int start, unsigned int len,
+ uint8_t *oldcontents,
+ uint8_t *newcontents)
+{
+ /* Checking integrity of just erased range is not needed, the erase
+ * function will do that itself. Do it anyway for nicer diagnostics.
+ */
+ if (check_erased_range(flash, start, len)) {
+ msg_cerr("Error in just erased range 0x%x-0x%x\n",
+ start, start + len - 1);
+ return VERIFY_ERROR;
+ }
+ /* Check integrity of previously written range. */
+ if (verify_range(flash, newcontents, 0, start, NULL)) {
+ msg_cerr("Error in previously written range 0x%x-0x%x\n",
+ 0, start - 1);
+ return VERIFY_ERROR;
+ }
+ /* Check integrity of not yet written range. */
+ if (verify_range(flash, oldcontents + start + len, start + len,
+ flash->total_size * 1024 - start - len, NULL)) {
+ msg_cerr("Error in not yet written range 0x%x-0x%x\n",
+ start + len, flash->total_size * 1024 - 1);
+ return VERIFY_ERROR;
+ }
+ /* Check if the write function fails. */
+ if (flash->write(flash, newcontents + start, start, len)) {
+ msg_cerr("Error during write in range 0x%x-0x%x\n",
+ start, start + len - 1);
+ return COMMAND_ERROR;
+ }
+ /* Check integrity of just written range. */
+ if (verify_range(flash, newcontents + start, start, len, NULL)) {
+ msg_cerr("Error in just written range 0x%x-0x%x\n",
+ start, start + len - 1);
+ return VERIFY_ERROR;
+ }
+ /* Recheck integrity of previously written range. */
+ if (verify_range(flash, newcontents, 0, start, NULL)) {
+ msg_cerr("Error in previously written range 0x%x-0x%x\n",
+ 0, start - 1);
+ return VERIFY_ERROR;
+ }
+ /* Recheck integrity of not yet written range. */
+ if (verify_range(flash, oldcontents + start + len, start + len,
+ flash->total_size * 1024 - start - len, NULL)) {
+ msg_cerr("Error in not yet written range 0x%x-0x%x\n",
+ start + len, flash->total_size * 1024 - 1);
+ return VERIFY_ERROR;
+ }
+ return 0;
+}
+
static int walk_eraseregions(struct flashchip *flash, int erasefunction,
int (*do_something) (struct flashchip *flash,
unsigned int addr,
--
http://www.hailfinger.org/