Re: [flashrom] [PATCH] Implement tested/untested status for board enables

[Carl-Daniel Hailfinger]

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

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"
> 		       "[e-mail].\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
[e-mail].

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 [e-mail]\n");
> }
> return board;
> }

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