Re: [flashrom] [PATCH] Move ntested board enable documentation to manpage

[Carl-Daniel Hailfinger]

Hi Michael,

On 07.03.2010 17:24, Michael Karcher wrote:
> This also checks the testedness of boards in all cases, not just for
> PCI/DMI detection.
>
> Signed-off-by: Michael Karcher

Acked-by: Carl-Daniel Hailfinger
> Hello Carl-Daniel,
>
> This should address all the issues you raised in your review.

Thanks! One minor comment, otherwise it looks OK. No need to repost,
just commit the updated version.
> Regards,
>   Michael Karcher
>
> 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"
> 		       "[e-mail].\n",
> 		       board->vendor_name, board->board_name);
> 		continue;
> 	}
> 	printf("NOTE: Running an untested board enable procedure.\n"
> 	       "Please report success/failure to [e-mail]\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 [e-mail]\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

... to enable chip erase and ...
> +write support (on old systems with parallel flash even probe support). The

... (and probe support on old systems with parallel flash).
> +type of the mainboard (if it requires specific code) is usually autodetected

The mainboard brand and model (....
> +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

... might be necessary to specify ...
> +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 untested?
[Kill one of them]
> +untested board-enable function, a warning message is printed and the board

... untested board enable ...
[remove -]
> +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

Not executing a board enable function (if one is needed) might cause ...
> +failure. If your board protects only part of the flash (commonly the top
> +end, called boot block), flashrom might encounter the error only after

...encounter an error ...
> +erasing the other part, so running without the board-enable function might

...erasing the unprotected part...
> +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

... or the flash chip is unsupported.
> +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

Looks good otherwise. I'm not a native speaker, so I might have
overlooked something. Still, your text is way better than what we have
now, so go ahead.

Regards,
Carl-Daniel

[PATCH] Move ntested board enable documentation to manpage

[Michael Karcher]

Am Sonntag, den 07.03.2010, 21:40 +0100 schrieb Carl-Daniel Hailfinger:
> > Signed-off-by: Michael Karcher
>
> Acked-by: Carl-Daniel Hailfinger

Thanks for your review! Committed as r926, after incorporating most of
the requested changes.

Regards,
  Michael Karcher