On Sat, 18 Aug 2012 22:33:30 +0200
Carl-Daniel Hailfinger <[email protected]> wrote:

> Am 15.08.2012 04:29 schrieb Stefan Tauner:
> > On Wed, 15 Aug 2012 02:03:29 +0200
> > Carl-Daniel Hailfinger <[email protected]> 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
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From 8e35049a90ac8a89e843c637912181dcadb978cb Mon Sep 17 00:00:00 2001
From: Stefan Tauner <[email protected]>
Date: Sun, 19 Aug 2012 02:09:23 +0200
Subject: [PATCH] Refactor the -p internal:mainboard handling.

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 aborts on a few more occasions that should be
safer for the user. For example it aborts if the enable function for the
specified mainboard (enable) can not be found.

Parts of the board_match_cbname refactoring were done by Carl-Daniel.

Signed-off-by: Stefan Tauner <[email protected]>
Signed-off-by: Carl-Daniel Hailfinger <[email protected]>
Acked-by: Carl-Daniel Hailfinger <[email protected]>
---
 board_enable.c |  137 +++++++++++++++++++++++++++++++-------------------------
 cbtable.c      |  131 +++++++++++++++++++----------------------------------
 flashrom.8     |    2 +-
 flashrom.c     |   20 +++++----
 internal.c     |   33 +++++++++++---
 programmer.h   |   10 ++---
 6 files changed, 166 insertions(+), 167 deletions(-)

diff --git a/board_enable.c b/board_enable.c
index 9c16905..d9aa676 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,64 @@ 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);
+		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).
  * Require main PCI IDs to match too as extra safety.
+ * vendor and model must be non-NULL!
  */
-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);
 			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 [email protected]\n", vendor, model);
 			return NULL;
 		}
 		partmatch = board;
@@ -2486,15 +2510,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 +2550,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))
@@ -2550,13 +2567,13 @@ const static struct board_match *board_match_pci_ids(enum board_match_phase phas
 	return NULL;
 }
 
-static int unsafe_board_handler(const struct board_match *board)
+static int is_board_enable_safe(const struct board_match *board)
 {
 	if (!board)
-		return 1;
+		return 0;
 
 	if (board->status == OK)
-		return 0;
+		return 1;
 
 	if (!force_boardenable) {
 		msg_pinfo("WARNING: Your mainboard is %s %s, but the mainboard-specific\n"
@@ -2566,12 +2583,11 @@ static int unsafe_board_handler(const struct board_match *board)
 			  "Please see the man page (section PROGRAMMER SPECIFIC INFO, subsection\n"
 			  "\"internal programmer\") for details.\n",
 			  board->vendor_name, board->board_name);
-		return 1;
+		return 0;
 	}
 	msg_pinfo("NOTE: Running an untested board enable procedure.\n"
-		  "Please report success/failure to [email protected]\n"
-		  "with your board name and SUCCESS or FAILURE in the subject.\n");
-	return 0;
+		  "Please report success/failure to [email protected].\n");
+	return 1;
 }
 
 /* FIXME: Should this be identical to board_flash_enable? */
@@ -2581,12 +2597,12 @@ static int board_handle_phase(enum board_match_phase phase)
 
 	board = board_match_pci_ids(phase);
 
-	if (unsafe_board_handler(board))
-		board = NULL;
-
 	if (!board)
 		return 0;
 
+	if (!is_board_enable_safe(board))
+		return 0;
+
 	if (!board->enable) {
 		/* Not sure if there is a valid case for this. */
 		msg_perr("Board match found, but nothing to do?\n");
@@ -2606,36 +2622,37 @@ 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 (!is_board_enable_safe(board))
+		return 1;
 
-	if (board) {
-		if (board->max_rom_decode_parallel)
-			max_rom_decode.parallel =
-				board->max_rom_decode_parallel * 1024;
+	/* limit the maximum size of the parallel bus */
+	if (board->max_rom_decode_parallel)
+		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);
+	if (board->enable != NULL) {
+		msg_pinfo("Enabling full flash access for board \"%s %s\"... ",
+			  board->vendor_name, board->board_name);
 
-			ret = board->enable();
-			if (ret)
-				msg_pinfo("FAILED!\n");
-			else
-				msg_pinfo("OK.\n");
-		}
+		ret = board->enable();
+		if (ret)
+			msg_pinfo("FAILED!\n");
+		else
+			msg_pinfo("OK.\n");
 	}
 
 	return ret;
diff --git a/cbtable.c b/cbtable.c
index dde12ac..bd83ea5 100644
--- a/cbtable.c
+++ b/cbtable.c
@@ -24,57 +24,36 @@
 
 #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)
 {
+	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--;
 
 	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);
+		/* 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--;
 	}
 
@@ -88,49 +67,38 @@ 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 coreboot-related checks.\n");
 		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);
+	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 +210,15 @@ static void find_mainboard(struct lb_record *ptr, unsigned long addr)
 	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 +226,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 +244,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 +278,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 +300,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;
 	return 0;
 }
diff --git a/flashrom.8 b/flashrom.8
index f37d5e9..33e6e23 100644
--- a/flashrom.8
+++ b/flashrom.8
@@ -259,7 +259,7 @@ and possibly DMI info. If PCI and DMI do not contain information to uniquely
 identify the mainboard (which is the exception), or if you want to override
 the detected mainboard model, you can specify the mainboard using the
 .sp
-.B "  flashrom \-p internal:mainboard=[<vendor>:]<board>"
+.B "  flashrom \-p internal:mainboard=<vendor>:<board>"
 syntax.
 .sp
 See the 'Known boards' or 'Known laptops' section in the output
diff --git a/flashrom.c b/flashrom.c
index 9544155..ee01e60 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1012,12 +1012,11 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
 					  "work, but to support all possible "
 					  "features");
 
-			msg_cinfo(" we need to add them manually.\nYou "
-				  "can help us by mailing us the output of "
-				  "the following command to flashrom@flashrom."
-				  "org: \n'flashrom -VV [plus the "
-				  "-p/--programmer parameter (if needed)]"
-				  "'\nThanks for your help!\n"
+			msg_cinfo(" we need to add them manually.\n"
+				  "You can help us by mailing us the output of the following command to "
+				  "[email protected]:\n"
+				  "'flashrom -VV [plus the -p/--programmer parameter]'\n"
+				  "Thanks for your help!\n"
 				  "===\n");
 		}
 
@@ -1814,11 +1813,16 @@ 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");
 				ret = 1;
 				goto out;
 			}
+		}
 #endif
 	}
 
diff --git a/internal.c b/internal.c
index 3ecc348..f3ccbde 100644
--- a/internal.c
+++ b/internal.c
@@ -169,6 +169,10 @@ int internal_init(void)
 #endif
 	int force_laptop = 0;
 	int not_a_laptop = 0;
+	const char *board_vendor = NULL;
+	const char *board_model = NULL;
+	const char *cb_vendor = NULL;
+	const char *cb_model = NULL;
 	char *arg;
 
 	arg = extract_programmer_param("boardenable");
@@ -217,7 +221,10 @@ int internal_init(void)
 
 	arg = extract_programmer_param("mainboard");
 	if (arg && strlen(arg)) {
-		lb_vendor_dev_from_string(arg);
+		if (board_parse_parameter(arg, &board_vendor, &board_model)) {
+			free(arg);
+			return 1;
+		}
 	} else if (arg && !strlen(arg)) {
 		msg_perr("Missing argument for mainboard.\n");
 		free(arg);
@@ -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. */
+		if (board_vendor == NULL || board_model == 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,11 +338,13 @@ 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.
-	 * Besides that, we don't check the board enable return code either.
 	 */
 #if defined(__i386__) || defined(__x86_64__) || defined (__mips)
 	register_par_programmer(&par_programmer_internal, internal_buses_supported);
diff --git a/programmer.h b/programmer.h
index 7d06be3..a32cd79 100644
--- a/programmer.h
+++ b/programmer.h
@@ -246,6 +246,7 @@ void print_supported_pcidevs(const struct pcidev_status *devs);
 
 #if CONFIG_INTERNAL == 1
 /* board_enable.c */
+int board_parse_parameter(const char *boardstring, const char **vendor, const char **model);
 void w836xx_ext_enter(uint16_t port);
 void w836xx_ext_leave(uint16_t port);
 void probe_superio_winbond(void);
@@ -255,7 +256,7 @@ void sio_write(uint16_t port, uint8_t reg, uint8_t data);
 void sio_mask(uint16_t port, uint8_t reg, uint8_t data, uint8_t mask);
 void board_handle_before_superio(void);
 void board_handle_before_laptop(void);
-int board_flash_enable(const char *vendor, const char *part);
+int board_flash_enable(const char *vendor, const char *model);
 
 /* chipset_enable.c */
 int chipset_flash_enable(void);
@@ -273,11 +274,8 @@ int setup_cpu_msr(int cpu);
 void cleanup_cpu_msr(void);
 
 /* cbtable.c */
-void lb_vendor_dev_from_string(const char *boardstring);
-int show_id(uint8_t *bios, int size);
-int coreboot_init(void);
-extern char *lb_part, *lb_vendor;
-extern int partvendor_from_cbtable;
+int cb_parse_table(const char **vendor, const char **model);
+int cb_check_image(uint8_t *bios, int size);
 
 /* dmi.c */
 extern int has_dmi_support;
-- 
Kind regards, Stefan Tauner

_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom

Reply via email to