On 27.02.2010 19:30, Michael Karcher wrote:
The message printing code greatly exceed the 80 character limit. I can reformat it on request to obey the limit.
Heh, this is one of the places where our coding standard says nothing. Detailed comment below.
Intended behaviour: on untested boards an explanation of that status is printed and the board enable code is not run, unless the option "boardenable=force" has been passed to the internal programmer.
Signed-off-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks a lot for this patch. It will allow us to get the number of pending patches down to a manageable level.
diff --git a/board_enable.c b/board_enable.c index 4278b6d..8f76b9b 100644 --- a/board_enable.c +++ b/board_enable.c @@ -1349,6 +1349,30 @@ static struct board_pciid_enable *board_match_pci_card_ids(void) } }
if (board->status == NT) {
IMHO this check should live in board_flash_enable() instead. Otherwise the board status is completely ignored if someone uses the coreboot IDs to match.
if (!force_boardenable){printf("WARNING: Your mainboard is %s %s, but the mainboard-specific\n""code has not been marked as working. To help flashrom development, please\n"
s/has not been marked as working/has not been tested/
"test flashrom on your board. As the code to support your board is untested,\n""we strongly recommend that as an additional safety measure you make\n""store backup of your current ROM contents (obtained by flashrom -r) on\n"
"...make store backup..." looks like a few words are missing here.
"a medium that can be accessed from a different computer (like an USB\n""drive or a network share of another system) before you try to erase or\n""write.\n""The untested code does not run unless you specify the\n"" \"-p internal:boardenable=force\" command line option. Depending on your\n""hardware environment, erasing, writing or even probing can fail without\n""running the board specific code. Running the board-specific code might\n""cause your computer to behave erratically if it is wrong.\n""Please report the results of running the board enable code to\n""flashrom@flashrom.org.\n",
I'm tempted to make this message way shorter and point people to the man page instead.
WARNING: Your mainboard has been detected as %s %s, and it needs mainboard specific code to write or erase your chip. This code is present in flashrom, but it has not been tested. Please see the flashrom man page for details how to enable it. Please report your results to flashrom@flashrom.org.
Either way, we should think about throwing the whole message text into some #define and align that to the left, or maybe break indentation rules for such messages. Ther's no standard for long message coding style, and every part of the code does it in a different way. Each possibility sucks, but it would be great if the suckage was standard.
board->vendor_name, board->board_name);continue;}printf("NOTE: Running an untested board enable procedure.\n""Please report success/failure to flashrom@flashrom.org\n");}
Rest looks good. Explain of fix the points above (message formatting can be postponed until after 0.9.2), and you get an ack.
Regards, Carl-Daniel
This also checks the testedness of boards in all cases, not just for PCI/DMI detection.
Signed-off-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de --- Hello Carl-Daniel,
This should address all the issues you raised in your review.
Regards, Michael Karcher
board_enable.c | 41 +++++++++++++++++------------------------ flashrom.8 | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 24 deletions(-)
diff --git a/board_enable.c b/board_enable.c index d28490c..7381940 100644 --- a/board_enable.c +++ b/board_enable.c @@ -1411,30 +1411,6 @@ static struct board_pciid_enable *board_match_pci_card_ids(void) } }
- if (board->status == NT) { - if (!force_boardenable) - { - printf("WARNING: Your mainboard is %s %s, but the mainboard-specific\n" - "code has not been marked as working. To help flashrom development, please\n" - "test flashrom on your board. As the code to support your board is untested,\n" - "we strongly recommend that as an additional safety measure you make\n" - "store backup of your current ROM contents (obtained by flashrom -r) on\n" - "a medium that can be accessed from a different computer (like an USB\n" - "drive or a network share of another system) before you try to erase or\n" - "write.\n" - "The untested code does not run unless you specify the\n" - " "-p internal:boardenable=force" command line option. Depending on your\n" - "hardware environment, erasing, writing or even probing can fail without\n" - "running the board specific code. Running the board-specific code might\n" - "cause your computer to behave erratically if it is wrong.\n" - "Please report the results of running the board enable code to\n" - "flashrom@flashrom.org.\n", - board->vendor_name, board->board_name); - continue; - } - printf("NOTE: Running an untested board enable procedure.\n" - "Please report success/failure to flashrom@flashrom.org\n"); - } return board; }
@@ -1452,6 +1428,23 @@ int board_flash_enable(const char *vendor, const char *part) if (!board) board = board_match_pci_card_ids();
+ if (board->status == NT) { + if (!force_boardenable) + { + printf("WARNING: Your mainboard is %s %s, but the mainboard-specific\n" + "code has not been tested, and thus will not not be executed by default.\n" + "Depending on your hardware environment, erasing, writing or even probing\n" + "can fail without running the board specific code.\n\n" + "Please see the man page (section PROGRAMMER SPECIFIC INFO, subsection\n" + ""internal programmer" for details\n", + board->vendor_name, board->board_name); + board = NULL; + } + else + printf("NOTE: Running an untested board enable procedure.\n" + "Please report success/failure to flashrom@flashrom.org\n"); + } + if (board) { if (board->max_rom_decode_parallel) max_rom_decode.parallel = diff --git a/flashrom.8 b/flashrom.8 index 696aba2..46dca21 100644 --- a/flashrom.8 +++ b/flashrom.8 @@ -175,6 +175,46 @@ colon. While some programmers take arguments at fixed positions, other programmers use a key/value interface in which the key and value is separated by an equal sign and different pairs are separated by a comma or a colon. .TP +.BR "internal " programmer +Some mainboards require to run mainboard specific code to enable erase and +write support (on old systems with parallel flash even probe support). The +type of the mainboard (if it requires specific code) is usually autodetected +using one of the following mechanisms: If your system is running coreboot, +the mainboard type is determined from the coreboot table, otherwise, the +mainboard is detected by examining the onboard PCI devices and possibly DMI +info. If PCI and DMI do not contain information to uniquely identify the +mainboard (which is the exception), it might be needed to specify the +mainboard using the -m switch (see above). +.sp +Some of these board-specific flash enabling functions (called board enables) +in flashrom have not yet been tested. If your mainboard is detected needing an untested +untested board-enable function, a warning message is printed and the board +enable is not executed, because a wrong board enable function might cause the +system to behave erratically, as board enable functions touch the low-level +internals of a mainboard. This might cause detection or erasing +failure. If your board protects only part of the flash (commonly the top +end, called boot block), flashrom might encounter the error only after +erasing the other part, so running without the board-enable function might +be dangerous for erase and write (which includes erase). +.sp +The suggested procedure for a mainboard with untested board specific code is +to first try to probe the ROM (just invoke flashrom and check that it detects +your flash chip type) without running the board enable code (i.e. without any +parameters). If it finds your chip, fine, otherwise, retry probing your chip +with the board-enable code running, using +.sp +.B "flashrom -p internal:boardenable=force" +.sp +If your chip is still not detected, the board enable code seems to be broken +or the chip unsupported. Otherwise, make a backup of your current ROM +contents (using -r) and store it to a medium outside of your computer, like +an USB drive or a network share. If you needed to run the board enable code +already for probing, use it for reading too. Now you can try to write the +new image. You should enable the board enable code in any case now, as it +has been written because it is known that writing/erasing without the board +enable is going to fail. In any case (success or failure), please report +to the flashrom mailing list, see below. +.sp .BR "dummy " programmer An optional parameter specifies the bus types it should support. For that you have to use the
On 28.02.2010 02:37, Carl-Daniel Hailfinger wrote:
On 27.02.2010 19:30, Michael Karcher wrote:
Intended behaviour: on untested boards an explanation of that status is printed and the board enable code is not run, unless the option "boardenable=force" has been passed to the internal programmer.
Signed-off-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
This was committed in r919. Sean Nelson acked it (with a reference to the patchwork URL) in another thread.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Michael Karcher