Disallow erase/write for known bad chips so people won't try without a clear understanding. If we want them to test, we can tell them to patch/edit flashchips.c.
If write/erase failed, warn the user to get help and not shutdown/reboot the computer.
Warn that the result of a forced read is often garbage. Too many users believed that a forced read meant that everything was fine.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-errormessages/flashrom.c =================================================================== --- flashrom-errormessages/flashrom.c (Revision 676) +++ flashrom-errormessages/flashrom.c (Arbeitskopie) @@ -221,6 +221,7 @@ { size_t size = flash->total_size * 1024; /* Flash registers live 4 MByte below the flash. */ + /* FIXME: This is incorrect for nonstandard flashbase. */ flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); }
@@ -475,6 +476,15 @@ return 0; }
+void emergency_help_message() +{ + fprintf(stderr, "Your flash chip is in an unknown state.\n" + "Get help on IRC at irc.freenode.net channel #flashrom or\n" + "mail flashrom@flashrom.org\n" + "------------------------------------------------------------\n" + "DO NOT REBOOT OR POWEROFF!\n"); +} + void usage(const char *name) { printf("usage: %s [-VfLzhR] [-E|-r file|-w file|-v file] [-c chipname]\n" @@ -743,6 +753,7 @@ printf("Run flashrom -L to view the hardware supported in this flashrom version.\n"); exit(1); } + printf("Please note that forced reads most likely contain garbage.\n"); return read_flash(flashes[0], filename); } // FIXME: flash writes stay enabled! @@ -808,14 +819,31 @@ buf = (uint8_t *) calloc(size, sizeof(char));
if (erase_it) { - if (erase_flash(flash)) + if (flash->tested & TEST_BAD_ERASE) { + fprintf(stderr, "Erase is not working on this chip. " + "Aborting.\n"); return 1; + } + if (erase_flash(flash)) { + emergency_help_message(); + return 1; + } } else if (read_it) { if (read_flash(flash, filename)) return 1; } else { struct stat image_stat;
+ if (flash->tested & TEST_BAD_ERASE) { + fprintf(stderr, "Erase is not working on this chip " + "and erase is needed for write. Aborting.\n"); + return 1; + } + if (flash->tested & TEST_BAD_WRITE) { + fprintf(stderr, "Write is not working on this chip. " + "Aborting.\n"); + return 1; + } if ((image = fopen(filename, "r")) == NULL) { perror(filename); exit(1); @@ -854,14 +882,18 @@ ret = flash->write(flash, buf); if (ret) { fprintf(stderr, "FAILED!\n"); + emergency_help_message(); return 1; } else { printf("COMPLETE.\n"); } }
- if (verify_it) + if (verify_it) { ret = verify_flash(flash, buf); + if (ret) + emergency_help_message(); + }
programmer_shutdown();
On 8/11/09 5:25 PM, Carl-Daniel Hailfinger wrote:
if (flash->tested & TEST_BAD_ERASE) {fprintf(stderr, "Erase is not working on this chip ""and erase is needed for write. Aborting.\n");return 1;}if (flash->tested & TEST_BAD_WRITE) {fprintf(stderr, "Write is not working on this chip. ""Aborting.\n");return 1;}
I would agree to this as long as there's a --force option
I think the IRC note should go to the documentation, not the code.
On 12.08.2009 11:39, Stefan Reinauer wrote:
On 8/11/09 5:25 PM, Carl-Daniel Hailfinger wrote:
if (flash->tested & TEST_BAD_ERASE) {fprintf(stderr, "Erase is not working on this chip ""and erase is needed for write. Aborting.\n");return 1;}if (flash->tested & TEST_BAD_WRITE) {fprintf(stderr, "Write is not working on this chip. ""Aborting.\n");return 1;}I would agree to this as long as there's a --force option
Updated patch attached.
I think the IRC note should go to the documentation, not the code.
Looking at the experience of most failed flashes where the users came to us after it was too late or rebooted directly after joining IRC, I'd say directing them with a program message to a realtime support channel instead of some piece of text in the documentation is the right way to go. After all, quite a few distro packages do not install the README.
New version: - Allow BAD override with --force - Wait 1 second between erase and verify. This fixes a few reports where verify directly after erase had unpleasant side effects like corrupting flash or at least getting incorrect verify results.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-errormessages/flashrom.c =================================================================== --- flashrom-errormessages/flashrom.c (Revision 678) +++ flashrom-errormessages/flashrom.c (Arbeitskopie) @@ -222,6 +222,7 @@ { size_t size = flash->total_size * 1024; /* Flash registers live 4 MByte below the flash. */ + /* FIXME: This is incorrect for nonstandard flashbase. */ flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); }
@@ -476,6 +477,15 @@ return 0; }
+void emergency_help_message() +{ + fprintf(stderr, "Your flash chip is in an unknown state.\n" + "Get help on IRC at irc.freenode.net channel #flashrom or\n" + "mail flashrom@flashrom.org\n" + "------------------------------------------------------------\n" + "DO NOT REBOOT OR POWEROFF!\n"); +} + void usage(const char *name) { printf("usage: %s [-VfLzhR] [-E|-r file|-w file|-v file] [-c chipname]\n" @@ -744,6 +754,7 @@ printf("Run flashrom -L to view the hardware supported in this flashrom version.\n"); exit(1); } + printf("Please note that forced reads most likely contain garbage.\n"); return read_flash(flashes[0], filename); } // FIXME: flash writes stay enabled! @@ -809,14 +820,44 @@ buf = (uint8_t *) calloc(size, sizeof(char));
if (erase_it) { - if (erase_flash(flash)) + if (flash->tested & TEST_BAD_ERASE) { + fprintf(stderr, "Erase is not working on this chip. "); + if (!force) { + fprintf(stderr, "Aborting.\n"); + return 1; + } else { + fprintf(stderr, "Continuing anyway.\n"); + } + } + if (erase_flash(flash)) { + emergency_help_message(); return 1; + } } else if (read_it) { if (read_flash(flash, filename)) return 1; } else { struct stat image_stat;
+ if (flash->tested & TEST_BAD_ERASE) { + fprintf(stderr, "Erase is not working on this chip " + "and erase is needed for write. "); + if (!force) { + fprintf(stderr, "Aborting.\n"); + return 1; + } else { + fprintf(stderr, "Continuing anyway.\n"); + } + } + if (flash->tested & TEST_BAD_WRITE) { + fprintf(stderr, "Write is not working on this chip. "); + if (!force) { + fprintf(stderr, "Aborting.\n"); + return 1; + } else { + fprintf(stderr, "Continuing anyway.\n"); + } + } if ((image = fopen(filename, "r")) == NULL) { perror(filename); exit(1); @@ -855,14 +896,21 @@ ret = flash->write(flash, buf); if (ret) { fprintf(stderr, "FAILED!\n"); + emergency_help_message(); return 1; } else { printf("COMPLETE.\n"); } }
- if (verify_it) + if (verify_it) { + /* Work around chips which need some time to calm down. */ + if (write_it) + programmer_delay(1000*1000); ret = verify_flash(flash, buf); + if (ret) + emergency_help_message(); + }
programmer_shutdown();
On 12.08.2009 14:06, Carl-Daniel Hailfinger wrote:
New version:
- Allow BAD override with --force
- Wait 1 second between erase and verify. This fixes a few reports where
verify directly after erase had unpleasant side effects like corrupting flash or at least getting incorrect verify results.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
All review comments were addressed. This is 0.9.1 material. An ack would be appreciated.
Regards, Carl-Daniel
On 8/12/09 2:06 PM, Carl-Daniel Hailfinger wrote:
- if (verify_it) {
/* Work around chips which need some time to calm down. */if (write_it)programmer_delay(1000*1000);if (ret)emergency_help_message();- }
This should be
/* If we tried to write, and now we don't properly verify, we might have an emergency situation */ if (write_it && ret) emergency_help_message();
Otherwise
Acked-by: Stefan Reinauer stepan@coresystems.de
On 19.08.2009 14:03, Stefan Reinauer wrote:
This should be
/* If we tried to write, and now we don't properly verify, we might have an emergency situation */ if (write_it && ret) emergency_help_message();
Indeed, thanks! New patch below.
Otherwise
Acked-by: Stefan Reinauer stepan@coresystems.de
Disallow erase/write for known bad chips so people won't try without a clear understanding. Allow override with --force.
If write/erase failed, warn the user to get help and not shutdown/reboot the computer.
Warn that the result of a forced read is often garbage. Too many users believed that a forced read meant that everything was fine.
Wait 1 second between erase and verify. This fixes a few reports where verify directly after erase had unpleasant side effects like corrupting flash or at least getting incorrect verify results.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Stefan Reinauer stepan@coresystems.de
Index: flashrom-errormessages/flashrom.c =================================================================== --- flashrom-errormessages/flashrom.c (Revision 691) +++ flashrom-errormessages/flashrom.c (Arbeitskopie) @@ -234,6 +234,7 @@ { size_t size = flash->total_size * 1024; /* Flash registers live 4 MByte below the flash. */ + /* FIXME: This is incorrect for nonstandard flashbase. */ flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size); }
@@ -488,6 +489,15 @@ return 0; }
+void emergency_help_message() +{ + fprintf(stderr, "Your flash chip is in an unknown state.\n" + "Get help on IRC at irc.freenode.net channel #flashrom or\n" + "mail flashrom@flashrom.org\n" + "------------------------------------------------------------\n" + "DO NOT REBOOT OR POWEROFF!\n"); +} + void usage(const char *name) { const char *pname; @@ -792,6 +802,7 @@ printf("Run flashrom -L to view the hardware supported in this flashrom version.\n"); exit(1); } + printf("Please note that forced reads most likely contain garbage.\n"); return read_flash(flashes[0], filename); } // FIXME: flash writes stay enabled! @@ -857,14 +868,44 @@ buf = (uint8_t *) calloc(size, sizeof(char));
if (erase_it) { - if (erase_flash(flash)) + if (flash->tested & TEST_BAD_ERASE) { + fprintf(stderr, "Erase is not working on this chip. "); + if (!force) { + fprintf(stderr, "Aborting.\n"); + return 1; + } else { + fprintf(stderr, "Continuing anyway.\n"); + } + } + if (erase_flash(flash)) { + emergency_help_message(); return 1; + } } else if (read_it) { if (read_flash(flash, filename)) return 1; } else { struct stat image_stat;
+ if (flash->tested & TEST_BAD_ERASE) { + fprintf(stderr, "Erase is not working on this chip " + "and erase is needed for write. "); + if (!force) { + fprintf(stderr, "Aborting.\n"); + return 1; + } else { + fprintf(stderr, "Continuing anyway.\n"); + } + } + if (flash->tested & TEST_BAD_WRITE) { + fprintf(stderr, "Write is not working on this chip. "); + if (!force) { + fprintf(stderr, "Aborting.\n"); + return 1; + } else { + fprintf(stderr, "Continuing anyway.\n"); + } + } if ((image = fopen(filename, "r")) == NULL) { perror(filename); exit(1); @@ -903,14 +944,24 @@ ret = flash->write(flash, buf); if (ret) { fprintf(stderr, "FAILED!\n"); + emergency_help_message(); return 1; } else { printf("COMPLETE.\n"); } }
- if (verify_it) + if (verify_it) { + /* Work around chips which need some time to calm down. */ + if (write_it) + programmer_delay(1000*1000); ret = verify_flash(flash, buf); + /* If we tried to write, and now we don't properly verify, we + * might have an emergency situation. + */ + if (ret && write_it) + emergency_help_message(); + }
programmer_shutdown();
On 19.08.2009 15:54, Carl-Daniel Hailfinger wrote:
Disallow erase/write for known bad chips so people won't try without a clear understanding. Allow override with --force.
If write/erase failed, warn the user to get help and not shutdown/reboot the computer.
Warn that the result of a forced read is often garbage. Too many users believed that a forced read meant that everything was fine.
Wait 1 second between erase and verify. This fixes a few reports where verify directly after erase had unpleasant side effects like corrupting flash or at least getting incorrect verify results.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Stefan Reinauer stepan@coresystems.de
Committed in r692. Thanks for the reviews!
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Reinauer