Re: [flashrom] [PATCH] Refactor the -p internal:mainboard handling.

[Carl-Daniel Hailfinger]

Am 13.08.2012 02:51 schrieb Stefan Tauner:
> This patch gets rid of some global variables and makes lots of bits along
> the code path that control the board enable execution more generic and
> clearer. From now on flashrom also abort on a few occasions that should be
> safer for the user. For example we abort if he specifies the mainboard (enable)
> but we can not find the enable function.
>
> Parts of the board_match_cbname refactoring were done by Carl-Daniel.
>
> FIXME: manpage
>
> Signed-off-by: Stefan Tauner 
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Carl-Daniel Hailfinger 
with a few comments and if you fix the manpage.

BIG FAT NOTE: You do not have to change all the stuff mentioned in the
review (because that would mean I have to review the patch again), but
it would be nice to have the review comments addressed in a followup patch.
> This might be kinda unreviewable...
> i can probably split it up if need be, just tell me please.

Big patch, but still... splitting it wouldn't have helped.
> diff --git a/board_enable.c b/board_enable.c
> index 9c16905..22d4072 100644
> --- a/board_enable.c
> +++ b/board_enable.c
> @@ -25,6 +25,7 @@
>   */
>
> #include <string.h>
> +#include <stdlib.h>
>  #include "flash.h"
>  #include "programmer.h"
>  #include "hwaccess.h"
> @@ -2443,41 +2444,63 @@ const struct board_match board_matches[] = {
>    {     0,      0,      0,      0,       0,      0,      0,      0, NULL,         NULL, NULL,           P3, NULL,          NULL,                    0,   NT, NULL}, /* end marker */
>  };
>
> +/* Parse the <vendor>:<board> string specified by the user as part of -p internal:mainboard=<vendor>:<board>.
> Parameters vendor and model will be overwritten. Returns 0 on success.
> Note: strtok modifies the original string, so we work on a copy and allocate memory for the results.
> */
>
> +int board_parse_parameter(const char *boardstring, const char **vendor, const char **model)
> +{
> /* strtok may modify the original string. */
> char *tempstr = strdup(boardstring);
> char *tempstr2 = NULL;
> strtok(tempstr, ":");
> tempstr2 = strtok(NULL, ":");
> if (tempstr == NULL || tempstr2 == NULL) {
> free(tempstr);

While freeing only tempstr is correct, it's not obvious.
> msg_pinfo("Please supply the board vendor and model name with the "
> 	  "-p internal:mainboard=<vendor>:<model> option.\n");
> return 1;
> }
> *vendor = strdup(tempstr);
> *model = strdup(tempstr2);
> msg_pspew("-p internal:mainboard: vendor="%s", model="%s"\n", tempstr, tempstr2);
> free(tempstr);
> return 0;
>
> +}
>
> /*
> Match boards on coreboot table gathered vendor and part name.
> Match boards on vendor and model name.
> Hint: the parameters can come either from the coreboot table or the command line (i.e. the user).

Please add the following comment:
vendor and model must be non-NULL.
> Require main PCI IDs to match too as extra safety.
>
> */
> -static const struct board_match *board_match_cbname(const char *vendor,
> 				    const char *part)
>
> +static const struct board_match *board_match_name(const char *vendor, const char *model)
>  {
>    const struct board_match *board = board_matches;
>    const struct board_match *partmatch = NULL;
>
> for (; board->vendor_name; board++) {
> if (vendor && (!board->lb_vendor
> 	       || strcasecmp(board->lb_vendor, vendor)))
> if (!board->lb_vendor || strcasecmp(board->lb_vendor, vendor))
> continue;
>
> if (!board->lb_part || strcasecmp(board->lb_part, part))
> if (!board->lb_part || strcasecmp(board->lb_part, model))
> continue;
>
> if (!pci_dev_find(board->first_vendor, board->first_device))
> if (!pci_dev_find(board->first_vendor, board->first_device)) {
> 	msg_pdbg("Odd. Board name \"%s\":\"%s\" matches, but first PCI device %04x:%04x "
> 		 "doesn't.\n", vendor, model, board->first_vendor, board->first_device);

Should we ask for a report here? Not sure, personal preference is not to
ask. If yes, at which message level?
> 	continue;
> }
>
> if (board->second_vendor &&
>     !pci_dev_find(board->second_vendor, board->second_device))
> if (!pci_dev_find(board->second_vendor, board->second_device)) {
> 	msg_pdbg("Odd. Board name \"%s\":\"%s\" matches, but second PCI device %04x:%04x "
> 		 "doesn't.\n", vendor, model, board->second_vendor, board->second_device);
> continue;
> if (vendor)
> 	return board;
> }
>
> if (partmatch) {
> 	/* a second entry has a matching part name */
> 	msg_pinfo("AMBIGUOUS BOARD NAME: %s\n", part);
> 	msg_pinfo("At least vendors '%s' and '%s' match.\n",
> 		  partmatch->lb_vendor, board->lb_vendor);
> 	msg_perr("Please use the full -p internal:mainboard="
> 		 "vendor:part syntax.\n");
> 	/* More than one entry has a matching name. */
> 	msg_perr("Board name \"%s\":\"%s\" and PCI IDs matched more than one board enable "
> 		 "entry. Please report a bug at [e-mail]\n", vendor, model);
> return NULL;
> }
> partmatch = board;
>
> @@ -2486,15 +2509,7 @@ static const struct board_match *board_match_cbname(const char *vendor,
>    if (partmatch)
>    	return partmatch;
> if (!partvendor_from_cbtable) {
> /* Only warn if the mainboard type was not gathered from the
>  * coreboot table. If it was, the coreboot implementor is
>  * expected to fix flashrom, too.
>  */
> msg_perr("\nUnknown vendor:board from -p internal:mainboard="
> 	 " programmer parameter:\n%s:%s\n\n",
> 	 vendor, part);
> }
> msg_perr("No board enable found for vendor="%s", model="%s".\n", vendor, model);
> return NULL;
>
> }
>
> @@ -2534,9 +2549,10 @@ const static struct board_match *board_match_pci_ids(enum board_match_phase phas
> if (board->dmi_pattern) {
> 	if (!has_dmi_support) {
> 		msg_perr("WARNING: Can't autodetect %s %s,"
> 			 " DMI info unavailable.\n",
> 		msg_perr("WARNING: Can't autodetect %s %s, DMI info unavailable.\n",
> 		 board->vendor_name, board->board_name);
> 		msg_pinfo("Please supply the board vendor and model name with the "
> 			  "-p internal:mainboard=<vendor>:<model> option.\n");
> 	continue;
> } else {
> 	if (!dmi_match(board->dmi_pattern))
>
> @@ -2569,8 +2585,7 @@ static int unsafe_board_handler(const struct board_match *board)
>    	return 1;
>    }
>    msg_pinfo("NOTE: Running an untested board enable procedure.\n"
>   "Please report success/failure to [e-mail]\n"
>   "with your board name and SUCCESS or FAILURE in the subject.\n");
>   "Please report success/failure to [e-mail].\n");
> return 0;
>
> }
>
> @@ -2606,29 +2621,29 @@ void board_handle_before_laptop(void)
>    board_handle_phase(P2);
>  }
>
> -int board_flash_enable(const char *vendor, const char *part)
> +int board_flash_enable(const char *vendor, const char *model)
>  {
>    const struct board_match *board = NULL;
>    int ret = 0;
> if (part)
> board = board_match_cbname(vendor, part);
> if (!board)
> if (vendor && model) {
> board = board_match_name(vendor, model);
> if (!board) /* if a board was given it has to match, else we abort here. */
> 	return 1;
> } else {
> board = board_match_pci_ids(P3);
> if (!board) /* i.e. there is just no board enable available for this board */
> 	return 0;
> }
> if (unsafe_board_handler(board))
> board = NULL;
> if (board) {
> ret = unsafe_board_handler(board);

Side note: unsafe_board_handler is a pretty dumb name. Yes, it's my
fault. Still... check_unsafe_board_enable() or something similar would
be a better name.
> if (!ret) {
> if (board->max_rom_decode_parallel)
> 	max_rom_decode.parallel =
> 		board->max_rom_decode_parallel * 1024;
> 	max_rom_decode.parallel = board->max_rom_decode_parallel * 1024;
>
> if (board->enable != NULL) {
> 	msg_pinfo("Disabling flash write protection for "
> 		  "board \"%s %s\"... ", board->vendor_name,
> 		  board->board_name);
> 	msg_pinfo("Enabling full flash access for board \"%s %s\"... ",
> 		  board->vendor_name, board->board_name);
>
> ret = board->enable();
> if (ret)
>
> diff --git a/cbtable.c b/cbtable.c
> index dde12ac..1f8b1c3 100644
> --- a/cbtable.c
> +++ b/cbtable.c
> @@ -24,57 +24,33 @@
>
> #include <unistd.h>
>  #include <stdio.h>
> -#include <stdlib.h>
>  #include <ctype.h>
>  #include <string.h>
>  #include "flash.h"
>  #include "programmer.h"
>  #include "coreboot_tables.h"
>
> -char *lb_part = NULL, *lb_vendor = NULL;
> -int partvendor_from_cbtable = 0;
> -static char *def_name = "DEFAULT";
> -static char *mainboard_vendor = NULL;
> -static char *mainboard_part = NULL;
>
> -/* Parse the [<vendor>:]<board> string specified by the user as part of
> -p internal:mainboard=[<vendor>:]<board> and set lb_vendor and lb_part
> to the extracted values.
> Note: strtok modifies the original string, so we work on a copy and allocate
> memory for lb_vendor and lb_part with strdup.
> */
>
> -void lb_vendor_dev_from_string(const char *boardstring)
> -{
> /* strtok may modify the original string. */
> char *tempstr = strdup(boardstring);
> char *tempstr2 = NULL;
> strtok(tempstr, ":");
> tempstr2 = strtok(NULL, ":");
> if (tempstr2) {
> lb_vendor = strdup(tempstr);
> lb_part = strdup(tempstr2);
> } else {
> lb_vendor = NULL;
> lb_part = strdup(tempstr);
> }
> free(tempstr);
>
> -}
> +static char *cb_vendor = NULL, *cb_model = NULL;
>
> -int show_id(uint8_t *bios, int size)
> +/* Tries to find coreboot IDs in the supplied image and compares them to the current IDs.
> Returns...
> -1	if IDs in the image do not match the IDs embedded in the current firmware,
> 0	if the IDs could not be found in the image or if they match correctly.
> */
>
> +int cb_check_image(uint8_t *image, int size)

cb_check_image_matches_board would be a better name IMHO.
> {
> const char *image_vendor = NULL;
> const char *image_model = NULL;
> unsigned int *walk;
> unsigned int mb_part_offset, mb_vendor_offset;
> char *mb_part, *mb_vendor;
> mainboard_vendor = def_name;
> mainboard_part = def_name;
> walk = (unsigned int *)(bios + size - 0x10);
> walk = (unsigned int *)(image + size - 0x10);
> walk--;

The two lines above made me scream WTF?!?
Since you're already fixing that code, can you please merge them into one?
walk = (unsigned int *)(image + size - 0x14);
> if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) {
> /* We might have an NVIDIA chipset BIOS which stores the ID information somewhere else. */
> walk = (unsigned int *)(bios + size - 0x80);
> /* We might have an NVIDIA chipset which stores the ID information somewhere else. */

Actually, "BIOS" wasn't that wrong... let me rephrase the comment so
that others may have a chance to understand the reason:

/* Some NVIDIA chipsets store chipset soft straps (IIRC Hypertransport
init info etc.) in flash at exactly the location where coreboot image
size, coreboot vendor name pointer and coreboot board name pointer are
usually stored. In this case coreboot uses an alternate location for the
coreboot image data. */
> walk = (unsigned int *)(image + size - 0x80);
> walk--;

And the two lines above need to be
walk = (unsigned int *)(image + size - 0x84);
> }
>
> @@ -88,49 +64,37 @@ int show_id(uint8_t *bios, int size)
>    mb_vendor_offset = *(walk - 2);
>    if ((*walk) == 0 || ((*walk) & 0x3ff) != 0 || (*walk) > size ||
>        mb_part_offset > size || mb_vendor_offset > size) {
> msg_pinfo("Flash image seems to be a legacy BIOS. Disabling coreboot-related checks.\n");
> msg_pdbg("Flash image seems to be a legacy BIOS. Disabling coreboot-related checks.\n");
> return 0;
> }
> mb_part = (char *)(bios + size - mb_part_offset);
> mb_vendor = (char *)(bios + size - mb_vendor_offset);
> mb_part = (char *)(image + size - mb_part_offset);
> mb_vendor = (char *)(image + size - mb_vendor_offset);
> if (!isprint((unsigned char)*mb_part) ||
>   !isprint((unsigned char)*mb_vendor)) {
> msg_pinfo("Flash image seems to have garbage in the ID location. Disabling checks.\n");
> msg_pdbg("Flash image seems to have garbage in the ID location. Disabling checks.\n");

...garbage in the coreboot ID location. Disabling coreboot-related checks.
> return 0;
>
> }
>
> msg_pdbg("coreboot last image size (not ROM size) is %d bytes.\n", *walk);
> mainboard_part = strdup(mb_part);
> mainboard_vendor = strdup(mb_vendor);
> msg_pdbg("Manufacturer: %s\n", mainboard_vendor);
> msg_pdbg("Mainboard ID: %s\n", mainboard_part);
> image_vendor = strdup(mb_vendor);
> image_model = strdup(mb_part);

If the image looks like a coreboot image according to our checks, but
it's malformed (or not a coreboot image), checking string length with
if (strnlen(mb_vendor, mb_vendor_offset) < mb_vendor_offset)
and aborting otherwise may be a good idea to avoid segfaults. Can be
done in a followup patch, though.
> msg_pdbg("Manufacturer: %s\n", image_vendor);
>
> msg_pdbg("Mainboard ID: %s\n", image_model);
>
> /* If these are not set, the coreboot table was not found. */
> if (!lb_vendor || !lb_part) {
> msg_pinfo("Note: If the following flash access fails, try "
> 	  "-p internal:mainboard=<vendor>:<mainboard>.\n");
> if (!cb_vendor || !cb_model)
> return 0;
>
> }
>
> /* These comparisons are case insensitive to make things a little less user^Werror prone. */
>
> if (!strcasecmp(mainboard_vendor, lb_vendor) &&
>    !strcasecmp(mainboard_part, lb_part)) {
> msg_pdbg("This firmware image matches this mainboard.\n");
> if (!strcasecmp(image_vendor, cb_vendor) && !strcasecmp(image_model, cb_model)) {
> msg_pdbg2("This coreboot image matches this mainboard.\n");
> } else {
> if (force_boardmismatch) {
> 	msg_pinfo("WARNING: This firmware image does not "
> 	       "seem to fit to this machine - forcing it.\n");
> } else {
> 	msg_pinfo("ERROR: Your firmware image (%s:%s) does not appear to\n"
> 		  "       be correct for the detected mainboard (%s:%s)\n\n"
> 		  "Override with -p internal:boardmismatch=force to ignore the board name\n"
> 		  "in the firmware image or override the detected mainboard with\n"
> 		  "-p internal:mainboard=<vendor>:<mainboard>.\n\n",
> 		  mainboard_vendor, mainboard_part, lb_vendor, lb_part);
> 	return 1;
> }
> msg_pinfo("WARNING: This coreboot image (%s:%s) does not appear to\n"
> 	  "         be correct for the detected mainboard (%s:%s).\n",
> 	  image_vendor, image_model, cb_vendor, cb_model);
> return -1;
>
> }
>
> return 0;
>
> @@ -242,22 +206,15 @@ static void find_mainboard(struct lb_record *ptr, unsigned long addr)

find_mainboard is a misnomer... it should be
get_mainboard_from_cb_record or something like that, and the addr
parameter was never used since the code was first committed. Followup
patch, no need to complicate this one.
> rec = (struct lb_mainboard *)ptr;
>    max_size = rec->size - sizeof(*rec);
>    msg_pdbg("Vendor ID: %.*s, part ID: %.*s\n",
>       max_size - rec->vendor_idx,
>       rec->strings + rec->vendor_idx,
>       max_size - rec->part_number_idx,
>       rec->strings + rec->part_number_idx);
> snprintf(vendor, 255, "%.*s", max_size - rec->vendor_idx,
>  rec->strings + rec->vendor_idx);
> snprintf(part, 255, "%.*s", max_size - rec->part_number_idx,
>  rec->strings + rec->part_number_idx);
> if (lb_part) {
> msg_pdbg("Overwritten by command line, vendor ID: %s, part ID: %s.\n", lb_vendor, lb_part);
> } else {
> partvendor_from_cbtable = 1;
> lb_part = strdup(part);
> lb_vendor = strdup(vendor);
> }
>         max_size - rec->vendor_idx,
>         rec->strings + rec->vendor_idx,
>         max_size - rec->part_number_idx,
>         rec->strings + rec->part_number_idx);
> snprintf(vendor, 255, "%.*s", max_size - rec->vendor_idx, rec->strings + rec->vendor_idx);
> snprintf(part, 255, "%.*s", max_size - rec->part_number_idx, rec->strings + rec->part_number_idx);
> cb_vendor = strdup(vendor);
> cb_model = strdup(part);
>
> }
>
> static struct lb_record *next_record(struct lb_record *rec)
> @@ -265,8 +222,7 @@ static struct lb_record *next_record(struct lb_record *rec)
>    return (struct lb_record *)(((char *)rec) + rec->size);
>  }
>
> -static void search_lb_records(struct lb_record *rec, struct lb_record *last,
> 	      unsigned long addr)
>
> +static void search_lb_records(struct lb_record *rec, struct lb_record *last, unsigned long addr)
>  {
>    struct lb_record *next;
>    int count;
> @@ -284,7 +240,8 @@ static void search_lb_records(struct lb_record *rec, struct lb_record *last,
>  }
>
> #define BYTES_TO_MAP (1024*1024)
> -int coreboot_init(void)
> +/* returns 0 if the table was parsed successfully and cb_vendor/cb_model have been set. */
> +int cb_parse_table(const char **vendor, const char **model)
>  {
>    uint8_t *table_area;
>    unsigned long addr, start;
> @@ -317,8 +274,7 @@ int coreboot_init(void)
>    		physunmap(table_area, BYTES_TO_MAP);
>    		table_area = physmap_try_ro("high tables", start, BYTES_TO_MAP);
>    		if (ERROR_PTR == table_area) {
> 		msg_perr("Failed getting access to coreboot "
> 			 "high tables.\n");
> 		msg_perr("Failed getting access to coreboot high tables.\n");
> 	return -1;
> }
> lb_table = find_lb_table(table_area, 0x00000, 0x1000);
>
> @@ -340,6 +296,7 @@ int coreboot_init(void)
>         lb_table->table_bytes, lb_table->table_checksum,
>         lb_table->table_entries);
>    search_lb_records(rec, last, addr + lb_table->header_bytes);
> *vendor = cb_vendor;
> *model = cb_model;

I'm not totally happy with those static variables, but I can't offer a
better suggestion (unless we want to extend struct flashctx with
board/device matching info, but anything like that should be a separate
patch.
> return 0;
>  }
> diff --git a/flashrom.c b/flashrom.c
> index 9544155..c39c12c 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1814,11 +1814,15 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>    	}
>
> #if CONFIG_INTERNAL == 1
> if (programmer == PROGRAMMER_INTERNAL)
> 	if (show_id(newcontents, size)) {
> if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) {
> 	if (force_boardmismatch) {
> 		msg_pinfo("Proceeding anyway because user forced us to.\n");
> 	} else {
> 		msg_perr("Aborting. You can override this with -p internal:boardmismatch=force.\n");

112 column limit...
> 		ret = 1;
> 		goto out;
> 	}
> }
>
> #endif
>    }
>
> diff --git a/internal.c b/internal.c
> index 3ecc348..b1686aa 100644
> --- a/internal.c
> +++ b/internal.c
> @@ -249,10 +256,20 @@ int internal_init(void)
>    }
>
> #if defined(__i386__) || defined(__x86_64__)
> /* We look at the cbtable first to see if we need a
> * mainboard specific flash enable sequence.
> */
> coreboot_init();
> if (cb_parse_table(&cb_vendor, &cb_model) == 0) { /* coreboot IDs valid */
> /* if no -p internal:mainboard was given but there are valid coreboot IDs then use those */

Please capitalize first word and finish with full stop. Otherwise you'll
see an instant followup commit by Uwe changing it. ;-)
> if (board_vendor == NULL || board_model == NULL) {

Is it even possible that only one of board_vendor/board_model is NULL?
> 	board_vendor = cb_vendor;
> 	board_model = cb_model;
> } else if (strcasecmp(board_vendor, cb_vendor) || strcasecmp(board_model, cb_model)) {
> 	msg_pinfo("WARNING: The mainboard IDs set by -p internal:mainboard (%s:%s) do not\n"
> 		  "         match the current coreboot IDs of the mainboard (%s:%s).\n",
> 		  board_vendor, board_model, cb_vendor, cb_model);
> 	if (!force_boardmismatch)
> 		return 1;
> 	msg_pinfo("Continuing anyway.\n");
> }
>
> }
>
> dmi_init();
>
> @@ -321,12 +338,11 @@ int internal_init(void)
>    init_superio_ite();
>  #endif
> board_flash_enable(lb_vendor, lb_part);
> if (board_flash_enable(board_vendor, board_model)) {
> msg_perr("Aborting to be safe.\n");
> return 1;
> }
> /* Even if chipset init returns an error code, we don't want to abort.
> * The error code might have been a warning only.

The two lines above still make sense AFAICS.
> * Besides that, we don't check the board enable return code either.

The line above can be killed as you did.
> */
>
> #if defined(__i386__) || defined(__x86_64__) || defined (__mips)
>    register_par_programmer(&par_programmer_internal, internal_buses_supported);
>    return 0;

Thanks for cleaning up this code!

Regards,
Carl-Daniel

[PATCH] Refactor the -p internal:mainboard handling.

[Stefan Tauner]

On Wed, 15 Aug 2012 02:03:29 +0200
Carl-Daniel Hailfinger  wrote:
> Am 13.08.2012 02:51 schrieb Stefan Tauner:
> > This patch gets rid of some global variables and makes lots of bits along
> > the code path that control the board enable execution more generic and
> > clearer. From now on flashrom also abort on a few occasions that should be
> > safer for the user. For example we abort if he specifies the mainboard (enable)
> > but we can not find the enable function.
> >
> > Parts of the board_match_cbname refactoring were done by Carl-Daniel.
> >
> > FIXME: manpage
> >
> > Signed-off-by: Stefan Tauner 
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Carl-Daniel Hailfinger 
> with a few comments and if you fix the manpage.
>
> BIG FAT NOTE: You do not have to change all the stuff mentioned in the
> review (because that would mean I have to review the patch again), but
> it would be nice to have the review comments addressed in a followup patch.
> > diff --git a/board_enable.c b/board_enable.c
> > index 9c16905..22d4072 100644
> > --- a/board_enable.c
> > +++ b/board_enable.c
> > @@ -25,6 +25,7 @@
> >   */
> >
> > #include <string.h>
> > +#include <stdlib.h>
> >  #include "flash.h"
> >  #include "programmer.h"
> >  #include "hwaccess.h"
> > @@ -2443,41 +2444,63 @@ const struct board_match board_matches[] = {
> >    {     0,      0,      0,      0,       0,      0,      0,      0, NULL,         NULL, NULL,           P3, NULL,          NULL,                    0,   NT, NULL}, /* end marker */
> >  };
> >
> > +/* Parse the <vendor>:<board> string specified by the user as part of -p internal:mainboard=<vendor>:<board>.
> > Parameters vendor and model will be overwritten. Returns 0 on success.
> > Note: strtok modifies the original string, so we work on a copy and allocate memory for the results.
> > */
> >
> > +int board_parse_parameter(const char *boardstring, const char **vendor, const char **model)
> > +{
> > /* strtok may modify the original string. */
> > char *tempstr = strdup(boardstring);
> > char *tempstr2 = NULL;
> > strtok(tempstr, ":");
> > tempstr2 = strtok(NULL, ":");
> > if (tempstr == NULL || tempstr2 == NULL) {
> > free(tempstr);
>
> While freeing only tempstr is correct, it's not obvious.

it is not?
then maybe my naivety guided me well :)
tempstr is the only one that is newly allocated up to this point.
strtok does modify the given string but not allocate a new one... i
thought?
> > msg_pinfo("Please supply the board vendor and model name with the "
> > 	  "-p internal:mainboard=<vendor>:<model> option.\n");
> > return 1;
> > }
> > *vendor = strdup(tempstr);
> > *model = strdup(tempstr2);
> > msg_pspew("-p internal:mainboard: vendor="%s", model="%s"\n", tempstr, tempstr2);
> > free(tempstr);
> > return 0;
> >
> > +}
> >
> > /*
> > Match boards on coreboot table gathered vendor and part name.
> > Match boards on vendor and model name.
> > Hint: the parameters can come either from the coreboot table or the command line (i.e. the user).
>
> Please add the following comment:
> vendor and model must be non-NULL.

i had a check for this in it already, but since it is not widely used
it does not make too much sense, but a comment is probably a good idea!
done.
> > Require main PCI IDs to match too as extra safety.
> >
> > */
> > -static const struct board_match *board_match_cbname(const char *vendor,
> > 				    const char *part)
> >
> > +static const struct board_match *board_match_name(const char *vendor, const char *model)
> >  {
> >    const struct board_match *board = board_matches;
> >    const struct board_match *partmatch = NULL;
> >
> > for (; board->vendor_name; board++) {
> > if (vendor && (!board->lb_vendor
> > 	       || strcasecmp(board->lb_vendor, vendor)))
> > if (!board->lb_vendor || strcasecmp(board->lb_vendor, vendor))
> > continue;
> >
> > if (!board->lb_part || strcasecmp(board->lb_part, part))
> > if (!board->lb_part || strcasecmp(board->lb_part, model))
> > continue;
> >
> > if (!pci_dev_find(board->first_vendor, board->first_device))
> > if (!pci_dev_find(board->first_vendor, board->first_device)) {
> > 	msg_pdbg("Odd. Board name \"%s\":\"%s\" matches, but first PCI device %04x:%04x "
> > 		 "doesn't.\n", vendor, model, board->first_vendor, board->first_device);
>
> Should we ask for a report here? Not sure, personal preference is not to
> ask. If yes, at which message level?

No, most likely it is running on a new unknown board revision with
different PCI IDs. The code is only executed if the user (or the cb
table) suggests the board name. If the PCI IDs do not match the user
will be informed that no match has been found and flashrom will abort.
We will request a -V log then anyway.

In case we encounter such a case we will probably not add the new
revision with the same name, but another one, so the case when there
are multiple enables for board with equal names but different pci ids
should not happen.
> […]
> > ret = unsafe_board_handler(board);
>
> Side note: unsafe_board_handler is a pretty dumb name. Yes, it's my
> fault. Still... check_unsafe_board_enable() or something similar would
> be a better name.

is_board_enable_(un)safe()?
> > -int show_id(uint8_t *bios, int size)
> > +/* Tries to find coreboot IDs in the supplied image and compares them to the current IDs.
> > Returns...
> > -1	if IDs in the image do not match the IDs embedded in the current firmware,
> > 0	if the IDs could not be found in the image or if they match correctly.
> > */
> >
> > +int cb_check_image(uint8_t *image, int size)
>
> cb_check_image_matches_board would be a better name IMHO.

it reflects the current behavior better, yes.
but OTOH it seems to have started as a function that was just showing
the ID and the name was not changed until now, although the behavior
changed a lot. i dont like to name functions too precisely because
it can easily become wrong :)
also, it is much longer and now there is a comment explaining exactly
what it does in case someone does not know.
> > {
> > const char *image_vendor = NULL;
> > const char *image_model = NULL;
> > unsigned int *walk;
> > unsigned int mb_part_offset, mb_vendor_offset;
> > char *mb_part, *mb_vendor;
> > mainboard_vendor = def_name;
> > mainboard_part = def_name;
> > walk = (unsigned int *)(bios + size - 0x10);
> > walk = (unsigned int *)(image + size - 0x10);
> > walk--;
>
> The two lines above made me scream WTF?!?
> Since you're already fixing that code, can you please merge them into one?
> walk = (unsigned int *)(image + size - 0x14);

I am deliberately not *fixing* the cbtale.c code (and i wont in the near
future). in this case i have no idea if your change is even correct.
what if sizeof(int) != 4? is the next item still at x - 0x14 then and
the current code is wrong or would your change break the now correct
code? or am i missing something? :)
> > if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) {
> > /* We might have an NVIDIA chipset BIOS which stores the ID information somewhere else. */
> > walk = (unsigned int *)(bios + size - 0x80);
> > /* We might have an NVIDIA chipset which stores the ID information somewhere else. */
>
> Actually, "BIOS" wasn't that wrong... let me rephrase the comment so
> that others may have a chance to understand the reason:
>
> /* Some NVIDIA chipsets store chipset soft straps (IIRC Hypertransport
> init info etc.) in flash at exactly the location where coreboot image
> size, coreboot vendor name pointer and coreboot board name pointer are
> usually stored. In this case coreboot uses an alternate location for the
> coreboot image data. */

i dont see how this makes BIOS less wrong, but thank you very much for
the clarification!
> > return 0;
> >
> > }
> >
> > msg_pdbg("coreboot last image size (not ROM size) is %d bytes.\n", *walk);
> > mainboard_part = strdup(mb_part);
> > mainboard_vendor = strdup(mb_vendor);
> > msg_pdbg("Manufacturer: %s\n", mainboard_vendor);
> > msg_pdbg("Mainboard ID: %s\n", mainboard_part);
> > image_vendor = strdup(mb_vendor);
> > image_model = strdup(mb_part);
>
> If the image looks like a coreboot image according to our checks, but
> it's malformed (or not a coreboot image), checking string length with
> if (strnlen(mb_vendor, mb_vendor_offset) < mb_vendor_offset)
> and aborting otherwise may be a good idea to avoid segfaults. Can be
> done in a followup patch, though.

yes, indeed.
as mentioned above i dont want to touch any of this code. i have seen
enough of it that i know that i would have to study coreboot's code and
then rewrite half of this file before i am satisfied. do not want. i
deem it almost useless... who uses coreboot, flashes the wrong image
and is not able to recover afterwards? - someone who deserves it. ;)
> > msg_pdbg("Manufacturer: %s\n", image_vendor);
> >
> > msg_pdbg("Mainboard ID: %s\n", image_model);
> >
> > /* If these are not set, the coreboot table was not found. */
> > if (!lb_vendor || !lb_part) {
> > msg_pinfo("Note: If the following flash access fails, try "
> > 	  "-p internal:mainboard=<vendor>:<mainboard>.\n");
> > if (!cb_vendor || !cb_model)
> > return 0;
> >
> > }
> >
> > /* These comparisons are case insensitive to make things a little less user^Werror prone. */
> >
> > if (!strcasecmp(mainboard_vendor, lb_vendor) &&
> >    !strcasecmp(mainboard_part, lb_part)) {
> > msg_pdbg("This firmware image matches this mainboard.\n");
> > if (!strcasecmp(image_vendor, cb_vendor) && !strcasecmp(image_model, cb_model)) {
> > msg_pdbg2("This coreboot image matches this mainboard.\n");
> > } else {
> > if (force_boardmismatch) {
> > 	msg_pinfo("WARNING: This firmware image does not "
> > 	       "seem to fit to this machine - forcing it.\n");
> > } else {
> > 	msg_pinfo("ERROR: Your firmware image (%s:%s) does not appear to\n"
> > 		  "       be correct for the detected mainboard (%s:%s)\n\n"
> > 		  "Override with -p internal:boardmismatch=force to ignore the board name\n"
> > 		  "in the firmware image or override the detected mainboard with\n"
> > 		  "-p internal:mainboard=<vendor>:<mainboard>.\n\n",
> > 		  mainboard_vendor, mainboard_part, lb_vendor, lb_part);
> > 	return 1;
> > }
> > msg_pinfo("WARNING: This coreboot image (%s:%s) does not appear to\n"
> > 	  "         be correct for the detected mainboard (%s:%s).\n",
> > 	  image_vendor, image_model, cb_vendor, cb_model);
> > return -1;
> >
> > }
> >
> > return 0;
> >
> > @@ -242,22 +206,15 @@ static void find_mainboard(struct lb_record *ptr, unsigned long addr)
>
> find_mainboard is a misnomer... it should be
> get_mainboard_from_cb_record or something like that, and the addr
> parameter was never used since the code was first committed. Followup
> patch, no need to complicate this one.

true, but OTOH it is just a static method...
> […]
> > *vendor = cb_vendor;
> > *model = cb_model;
>
> I'm not totally happy with those static variables, but I can't offer a
> better suggestion (unless we want to extend struct flashctx with
> board/device matching info, but anything like that should be a separate
> patch.

yes... but the problem is that cb_check_image is called in doit() and
there is no easy and sane way to get that information there.
OTOH i dont like that special case in doit anyway. it is necessary
because the image is read in there and hence is not available earlier
(at programmer init time).
any changes in this regard would conflict with the layout patch set so
let's postpone this discussion for now. and the situation improved a lot
already anyway (Eigenlob duftet ausgezeichnet! :)
> > diff --git a/flashrom.c b/flashrom.c
> > index 9544155..c39c12c 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1814,11 +1814,15 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
> >    	}
> >
> > #if CONFIG_INTERNAL == 1
> > if (programmer == PROGRAMMER_INTERNAL)
> > 	if (show_id(newcontents, size)) {
> > if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) {
> > 	if (force_boardmismatch) {
> > 		msg_pinfo("Proceeding anyway because user forced us to.\n");
> > 	} else {
> > 		msg_perr("Aborting. You can override this with -p internal:boardmismatch=force.\n");
>
> 112 column limit...

fixed
> > diff --git a/internal.c b/internal.c
> > index 3ecc348..b1686aa 100644
> > --- a/internal.c
> > +++ b/internal.c
> > @@ -249,10 +256,20 @@ int internal_init(void)
> >    }
> >
> > #if defined(__i386__) || defined(__x86_64__)
> > /* We look at the cbtable first to see if we need a
> > * mainboard specific flash enable sequence.
> > */
> > coreboot_init();
> > if (cb_parse_table(&cb_vendor, &cb_model) == 0) { /* coreboot IDs valid */
> > /* if no -p internal:mainboard was given but there are valid coreboot IDs then use those */
>
> Please capitalize first word and finish with full stop. Otherwise you'll
> see an instant followup commit by Uwe changing it. ;-)

if it motivates him to look at flashrom code again, then maybe i should
keep it that way :P
> > if (board_vendor == NULL || board_model == NULL) {
>
> Is it even possible that only one of board_vendor/board_model is NULL?

no, it should not be possible. OTOH '&&' wouldnt be 'more' correct
either, but i want to check both variables... mostly for
consistency/symmetry i guess :)
> > 	board_vendor = cb_vendor;
> > 	board_model = cb_model;
> > } else if (strcasecmp(board_vendor, cb_vendor) || strcasecmp(board_model, cb_model)) {
> > 	msg_pinfo("WARNING: The mainboard IDs set by -p internal:mainboard (%s:%s) do not\n"
> > 		  "         match the current coreboot IDs of the mainboard (%s:%s).\n",
> > 		  board_vendor, board_model, cb_vendor, cb_model);
> > 	if (!force_boardmismatch)
> > 		return 1;
> > 	msg_pinfo("Continuing anyway.\n");
> > }
> >
> > }
> >
> > dmi_init();
> >
> > @@ -321,12 +338,11 @@ int internal_init(void)
> >    init_superio_ite();
> >  #endif
> > board_flash_enable(lb_vendor, lb_part);
> > if (board_flash_enable(board_vendor, board_model)) {
> > msg_perr("Aborting to be safe.\n");
> > return 1;
> > }
> > /* Even if chipset init returns an error code, we don't want to abort.
> > * The error code might have been a warning only.
>
> The two lines above still make sense AFAICS.

no, but it did not change with this patch. it has been wrong for a
while i guess. first of all the location is wrong. secondly it is not
true: we DO abort in some cases. and there is a comment explaining this
there already:
    /* try to enable it. Failure IS an option, since not all motherboards
     * really need this to be done, etc., etc.
     */
    ret = chipset_flash_enable();

this could be improved though. :)

i have put it back anyway for now because it is unrelated and can
easily be removed in my tested_stuff branch... just tell me if you are
convinced please.
> Thanks for cleaning up this code!

i would lie if i would write "my pleasure", but i am sure reviewing
this was similarly dreadful. :) thanks for the review!
i still need to refine the manpage... so no commit yet.

[PATCH] Refactor the -p internal:mainboard handling.

[Carl-Daniel Hailfinger]

Am 15.08.2012 04:29 schrieb Stefan Tauner:
> On Wed, 15 Aug 2012 02:03:29 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 13.08.2012 02:51 schrieb Stefan Tauner:
> > > This patch gets rid of some global variables and makes lots of bits along
> > > the code path that control the board enable execution more generic and
> > > clearer. From now on flashrom also abort on a few occasions that should be
> > > safer for the user. For example we abort if he specifies the mainboard (enable)
> > > but we can not find the enable function.
> > >
> > > Parts of the board_match_cbname refactoring were done by Carl-Daniel.
> > >
> > > FIXME: manpage
> > >
> > > Signed-off-by: Stefan Tauner 
> > > Signed-off-by: Carl-Daniel Hailfinger
> >
> > Acked-by: Carl-Daniel Hailfinger 
> > with a few comments and if you fix the manpage.
> >
> > BIG FAT NOTE: You do not have to change all the stuff mentioned in the
> > review (because that would mean I have to review the patch again), but
> > it would be nice to have the review comments addressed in a followup patch.
> > > diff --git a/board_enable.c b/board_enable.c
> > > index 9c16905..22d4072 100644
> > > --- a/board_enable.c
> > > +++ b/board_enable.c
> > > @@ -25,6 +25,7 @@
> > >   */
> > >
> > > #include <string.h>
> > > +#include <stdlib.h>
> > >  #include "flash.h"
> > >  #include "programmer.h"
> > >  #include "hwaccess.h"
> > > @@ -2443,41 +2444,63 @@ const struct board_match board_matches[] = {
> > >    {     0,      0,      0,      0,       0,      0,      0,      0, NULL,         NULL, NULL,           P3, NULL,          NULL,                    0,   NT, NULL}, /* end marker */
> > >  };
> > >
> > > +/* Parse the <vendor>:<board> string specified by the user as part of -p internal:mainboard=<vendor>:<board>.
> > > Parameters vendor and model will be overwritten. Returns 0 on success.
> > > Note: strtok modifies the original string, so we work on a copy and allocate memory for the results.
> > > */
> > >
> > > +int board_parse_parameter(const char *boardstring, const char **vendor, const char **model)
> > > +{
> > > /* strtok may modify the original string. */
> > > char *tempstr = strdup(boardstring);
> > > char *tempstr2 = NULL;
> > > strtok(tempstr, ":");
> > > tempstr2 = strtok(NULL, ":");
> > > if (tempstr == NULL || tempstr2 == NULL) {
> > > free(tempstr);
> >
> > While freeing only tempstr is correct, it's not obvious.
>
> it is not?
> then maybe my naivety guided me well :)
> tempstr is the only one that is newly allocated up to this point.
> strtok does modify the given string but not allocate a new one... i
> thought?

Well, it was not obvious when I reviewed the code late at night ;-)
> > […]
> > > ret = unsafe_board_handler(board);
> >
> > Side note: unsafe_board_handler is a pretty dumb name. Yes, it's my
> > fault. Still... check_unsafe_board_enable() or something similar would
> > be a better name.
>
> is_board_enable_(un)safe()?

Yes. I would pick is_board_enable_safe(), but I'm fine with either of
your suggestions.
> > > -int show_id(uint8_t *bios, int size)
> > > +/* Tries to find coreboot IDs in the supplied image and compares them to the current IDs.
> > > Returns...
> > > -1	if IDs in the image do not match the IDs embedded in the current firmware,
> > > 0	if the IDs could not be found in the image or if they match correctly.
> > > */
> > >
> > > +int cb_check_image(uint8_t *image, int size)
> >
> > cb_check_image_matches_board would be a better name IMHO.
>
> it reflects the current behavior better, yes.
> but OTOH it seems to have started as a function that was just showing
> the ID and the name was not changed until now, although the behavior
> changed a lot. i dont like to name functions too precisely because
> it can easily become wrong :)
> also, it is much longer and now there is a comment explaining exactly
> what it does in case someone does not know.

I envision lots of similar functions in the future, e.g. for checking
the PCI ID embedded in an option ROM file against the PCI ID of the
target device. check_image_matches_optionrom_pcidevice() and
check_image_matches_board_awardbios() and
check_image_matches_board_coreboot()... so the name I suggested isn't
optimal either.
If we use check_image_matches_ as prefix, we'd have a consistent prefix
for any such future functions.
Your choice.
> > > {
> > > const char *image_vendor = NULL;
> > > const char *image_model = NULL;
> > > unsigned int *walk;
> > > unsigned int mb_part_offset, mb_vendor_offset;
> > > char *mb_part, *mb_vendor;
> > > mainboard_vendor = def_name;
> > > mainboard_part = def_name;
> > > walk = (unsigned int *)(bios + size - 0x10);
> > > walk = (unsigned int *)(image + size - 0x10);
> > > walk--;
> >
> > The two lines above made me scream WTF?!?
> > Since you're already fixing that code, can you please merge them into one?
> > walk = (unsigned int *)(image + size - 0x14);
>
> I am deliberately not *fixing* the cbtale.c code (and i wont in the near
> future). in this case i have no idea if your change is even correct.
> what if sizeof(int) != 4? is the next item still at x - 0x14 then and
> the current code is wrong or would your change break the now correct
> code? or am i missing something? :)

OK, I can fix this in a followup patch.
> > > if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) {
> > > /* We might have an NVIDIA chipset BIOS which stores the ID information somewhere else. */
> > > walk = (unsigned int *)(bios + size - 0x80);
> > > /* We might have an NVIDIA chipset which stores the ID information somewhere else. */
> >
> > Actually, "BIOS" wasn't that wrong... let me rephrase the comment so
> > that others may have a chance to understand the reason:
> >
> > /* Some NVIDIA chipsets store chipset soft straps (IIRC Hypertransport
> > init info etc.) in flash at exactly the location where coreboot image
> > size, coreboot vendor name pointer and coreboot board name pointer are
> > usually stored. In this case coreboot uses an alternate location for the
> > coreboot image data. */
>
> i dont see how this makes BIOS less wrong, but thank you very much for
> the clarification!

You could leave the comment here alone, and I could send a separate
patch with the offset fixups I suggested and with the new comment I
suggested. Your choice.
> > > msg_pdbg("coreboot last image size (not ROM size) is %d bytes.\n", *walk);
> > > mainboard_part = strdup(mb_part);
> > > mainboard_vendor = strdup(mb_vendor);
> > > msg_pdbg("Manufacturer: %s\n", mainboard_vendor);
> > > msg_pdbg("Mainboard ID: %s\n", mainboard_part);
> > > image_vendor = strdup(mb_vendor);
> > > image_model = strdup(mb_part);
> >
> > If the image looks like a coreboot image according to our checks, but
> > it's malformed (or not a coreboot image), checking string length with
> > if (strnlen(mb_vendor, mb_vendor_offset) < mb_vendor_offset)
> > and aborting otherwise may be a good idea to avoid segfaults. Can be
> > done in a followup patch, though.
>
> yes, indeed.
> as mentioned above i dont want to touch any of this code. i have seen
> enough of it that i know that i would have to study coreboot's code and
> then rewrite half of this file before i am satisfied. do not want. i
> deem it almost useless... who uses coreboot, flashes the wrong image
> and is not able to recover afterwards? - someone who deserves it. ;)

OK, I will send a patch for it.
> > > msg_pdbg("Manufacturer: %s\n", image_vendor);
> > >
> > > msg_pdbg("Mainboard ID: %s\n", image_model);
> > >
> > > /* If these are not set, the coreboot table was not found. */
> > > if (!lb_vendor || !lb_part) {
> > > msg_pinfo("Note: If the following flash access fails, try "
> > > 	  "-p internal:mainboard=<vendor>:<mainboard>.\n");
> > > if (!cb_vendor || !cb_model)
> > > return 0;
> > >
> > > }
> > >
> > > /* These comparisons are case insensitive to make things a little less user^Werror prone. */
> > >
> > > if (!strcasecmp(mainboard_vendor, lb_vendor) &&
> > >    !strcasecmp(mainboard_part, lb_part)) {
> > > msg_pdbg("This firmware image matches this mainboard.\n");
> > > if (!strcasecmp(image_vendor, cb_vendor) && !strcasecmp(image_model, cb_model)) {
> > > msg_pdbg2("This coreboot image matches this mainboard.\n");
> > > } else {
> > > if (force_boardmismatch) {
> > > 	msg_pinfo("WARNING: This firmware image does not "
> > > 	       "seem to fit to this machine - forcing it.\n");
> > > } else {
> > > 	msg_pinfo("ERROR: Your firmware image (%s:%s) does not appear to\n"
> > > 		  "       be correct for the detected mainboard (%s:%s)\n\n"
> > > 		  "Override with -p internal:boardmismatch=force to ignore the board name\n"
> > > 		  "in the firmware image or override the detected mainboard with\n"
> > > 		  "-p internal:mainboard=<vendor>:<mainboard>.\n\n",
> > > 		  mainboard_vendor, mainboard_part, lb_vendor, lb_part);
> > > 	return 1;
> > > }
> > > msg_pinfo("WARNING: This coreboot image (%s:%s) does not appear to\n"
> > > 	  "         be correct for the detected mainboard (%s:%s).\n",
> > > 	  image_vendor, image_model, cb_vendor, cb_model);
> > > return -1;
> > >
> > > }
> > >
> > > return 0;
> > >
> > > @@ -242,22 +206,15 @@ static void find_mainboard(struct lb_record *ptr, unsigned long addr)
> >
> > find_mainboard is a misnomer... it should be
> > get_mainboard_from_cb_record or something like that, and the addr
> > parameter was never used since the code was first committed. Followup
> > patch, no need to complicate this one.
>
> true, but OTOH it is just a static method...

My concern was code readability, not public interfaces. You're right
that this code fortunately isn't called from outside this file. To be
honest, this file is probably (except layout.c) the least modified file
since flashrom development started.
> > […]
> > > *vendor = cb_vendor;
> > > *model = cb_model;
> >
> > I'm not totally happy with those static variables, but I can't offer a
> > better suggestion (unless we want to extend struct flashctx with
> > board/device matching info, but anything like that should be a separate
> > patch.
>
> yes... but the problem is that cb_check_image is called in doit() and
> there is no easy and sane way to get that information there.
> OTOH i dont like that special case in doit anyway. it is necessary
> because the image is read in there and hence is not available earlier
> (at programmer init time).

Mh yes.
> any changes in this regard would conflict with the layout patch set so
> let's postpone this discussion for now. and the situation improved a lot
> already anyway (Eigenlob duftet ausgezeichnet! :)

Aaaaaargh, don't remind me of the layout patches. That will be the most
painful review ever.
> > > diff --git a/flashrom.c b/flashrom.c
> > > index 9544155..c39c12c 100644
> > > --- a/flashrom.c
> > > +++ b/flashrom.c
> > > @@ -1814,11 +1814,15 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
> > >    	}
> > >
> > > #if CONFIG_INTERNAL == 1
> > > if (programmer == PROGRAMMER_INTERNAL)
> > > 	if (show_id(newcontents, size)) {
> > > if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) {
> > > 	if (force_boardmismatch) {
> > > 		msg_pinfo("Proceeding anyway because user forced us to.\n");
> > > 	} else {
> > > 		msg_perr("Aborting. You can override this with -p internal:boardmismatch=force.\n");
> >
> > 112 column limit...
>
> fixed
> > > diff --git a/internal.c b/internal.c
> > > index 3ecc348..b1686aa 100644
> > > --- a/internal.c
> > > +++ b/internal.c
> > > @@ -249,10 +256,20 @@ int internal_init(void)
> > >    }
> > >
> > > #if defined(__i386__) || defined(__x86_64__)
> > > /* We look at the cbtable first to see if we need a
> > > * mainboard specific flash enable sequence.
> > > */
> > > coreboot_init();
> > > if (cb_parse_table(&cb_vendor, &cb_model) == 0) { /* coreboot IDs valid */
> > > /* if no -p internal:mainboard was given but there are valid coreboot IDs then use those */
> >
> > Please capitalize first word and finish with full stop. Otherwise you'll
> > see an instant followup commit by Uwe changing it. ;-)
>
> if it motivates him to look at flashrom code again, then maybe i should
> keep it that way :P

Heh.
> > > if (board_vendor == NULL || board_model == NULL) {
> >
> > Is it even possible that only one of board_vendor/board_model is NULL?
>
> no, it should not be possible. OTOH '&&' wouldnt be 'more' correct
> either, but i want to check both variables... mostly for
> consistency/symmetry i guess :)

Good idea.
> > > 	board_vendor = cb_vendor;
> > > 	board_model = cb_model;
> > > } else if (strcasecmp(board_vendor, cb_vendor) || strcasecmp(board_model, cb_model)) {
> > > 	msg_pinfo("WARNING: The mainboard IDs set by -p internal:mainboard (%s:%s) do not\n"
> > > 		  "         match the current coreboot IDs of the mainboard (%s:%s).\n",
> > > 		  board_vendor, board_model, cb_vendor, cb_model);
> > > 	if (!force_boardmismatch)
> > > 		return 1;
> > > 	msg_pinfo("Continuing anyway.\n");
> > > }
> > >
> > > }
> > >
> > > dmi_init();
> > >
> > > @@ -321,12 +338,11 @@ int internal_init(void)
> > >    init_superio_ite();
> > >  #endif
> > > board_flash_enable(lb_vendor, lb_part);
> > > if (board_flash_enable(board_vendor, board_model)) {
> > > msg_perr("Aborting to be safe.\n");
> > > return 1;
> > > }
> > > /* Even if chipset init returns an error code, we don't want to abort.
> > > * The error code might have been a warning only.
> >
> > The two lines above still make sense AFAICS.
>
> no, but it did not change with this patch. it has been wrong for a
> while i guess. first of all the location is wrong. secondly it is not
> true: we DO abort in some cases. and there is a comment explaining this
> there already:
>    /* try to enable it. Failure IS an option, since not all motherboards
>     * really need this to be done, etc., etc.
>     */
>    ret = chipset_flash_enable();
>
> this could be improved though. :)
>
> i have put it back anyway for now because it is unrelated and can
> easily be removed in my tested_stuff branch... just tell me if you are
> convinced please.

Convinced.
> > Thanks for cleaning up this code!
>
> i would lie if i would write "my pleasure", but i am sure reviewing
> this was similarly dreadful. :) thanks for the review!
> i still need to refine the manpage... so no commit yet.

Thanks for all the work you did!

Regards,
Carl-Daniel

[PATCH] Refactor the -p internal:mainboard handling.

[Stefan Tauner]

On Sat, 18 Aug 2012 22:33:30 +0200
Carl-Daniel Hailfinger  wrote:
> Am 15.08.2012 04:29 schrieb Stefan Tauner:
> > On Wed, 15 Aug 2012 02:03:29 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 13.08.2012 02:51 schrieb Stefan Tauner:
> > > > -int show_id(uint8_t *bios, int size)
> > > > +/* Tries to find coreboot IDs in the supplied image and compares them to the current IDs.
> > > > Returns...
> > > > -1	if IDs in the image do not match the IDs embedded in the current firmware,
> > > > 0	if the IDs could not be found in the image or if they match correctly.
> > > > */
> > > >
> > > > +int cb_check_image(uint8_t *image, int size)
> > >
> > > cb_check_image_matches_board would be a better name IMHO.
> >
> > it reflects the current behavior better, yes.
> > but OTOH it seems to have started as a function that was just showing
> > the ID and the name was not changed until now, although the behavior
> > changed a lot. i dont like to name functions too precisely because
> > it can easily become wrong :)
> > also, it is much longer and now there is a comment explaining exactly
> > what it does in case someone does not know.
>
> I envision lots of similar functions in the future, e.g. for checking
> the PCI ID embedded in an option ROM file against the PCI ID of the
> target device. check_image_matches_optionrom_pcidevice() and
> check_image_matches_board_awardbios() and
> check_image_matches_board_coreboot()... so the name I suggested isn't
> optimal either.
> If we use check_image_matches_ as prefix, we'd have a consistent prefix
> for any such future functions.
> Your choice.

renaming should be done in the patch that adds the first other function
then (i am not sure that those function will (all) be part of flashrom
itself).
> > > > if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) {
> > > > /* We might have an NVIDIA chipset BIOS which stores the ID information somewhere else. */
> > > > walk = (unsigned int *)(bios + size - 0x80);
> > > > /* We might have an NVIDIA chipset which stores the ID information somewhere else. */
> > >
> > > Actually, "BIOS" wasn't that wrong... let me rephrase the comment so
> > > that others may have a chance to understand the reason:
> > >
> > > /* Some NVIDIA chipsets store chipset soft straps (IIRC Hypertransport
> > > init info etc.) in flash at exactly the location where coreboot image
> > > size, coreboot vendor name pointer and coreboot board name pointer are
> > > usually stored. In this case coreboot uses an alternate location for the
> > > coreboot image data. */
> >
> > i dont see how this makes BIOS less wrong, but thank you very much for
> > the clarification!
>
> You could leave the comment here alone, and I could send a separate
> patch with the offset fixups I suggested and with the new comment I
> suggested. Your choice.

since i have already added you comment instead of the previous, changed
comment, and because it does make sense even without the offset change
ill commit it now.
> > > > @@ -242,22 +206,15 @@ static void find_mainboard(struct lb_record *ptr, unsigned long addr)
> > >
> > > find_mainboard is a misnomer... it should be
> > > get_mainboard_from_cb_record or something like that, and the addr
> > > parameter was never used since the code was first committed. Followup
> > > patch, no need to complicate this one.
> >
> > true, but OTOH it is just a static method...
>
> My concern was code readability, not public interfaces. You're right
> that this code fortunately isn't called from outside this file. To be
> honest, this file is probably (except layout.c) the least modified file
> since flashrom development started.

oh i figured that out... it is a pity that code dragons just dont die
from old age. :)

changes since the last iteration:
- trivial manpage change
- renaming unsafe_board_handler to is_board_enable_safe.
  this changes the truth! ;) so there are a few related changes
  including a small fix/sanitation of board_handle_phase

whee almost done

[PATCH] Refactor the -p internal:mainboard handling.

[Stefan Tauner]

On Sun, 19 Aug 2012 02:09:45 +0200
Stefan Tauner  wrote:
> changes since the last iteration:
> trivial manpage change
> renaming unsafe_board_handler to is_board_enable_safe.
> this changes the truth! ;) so there are a few related changes
> including a small fix/sanitation of board_handle_phase
>
> whee almost done

committed in r1577 after debating about the name and respective return
values of the board enable safety check function.
thanks for the review!