Patchwork board_enable selfcheck

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2014-06-01 00:27:18
Message ID <538A7366.3070601@gmx.net>
Download mbox | patch
Permalink /patch/4173/
State Rejected
Headers show

Comments

Carl-Daniel Hailfinger - 2014-06-01 00:27:18
Found this bitrotting away on my disk. Rebased from r1645, last time all
snippets compiled was r1539. If you kill the cli_classic.c hunk, it
should compile. That hunk was in the patch to make sure the msg_gspew()
calls in selfcheck() aren't no-ops by default. Hm. This seems to be a
bug still in HEAD.

Not for direct merge, there are other random changes in there as well.
Stefan, you already wrote a patch doing a board enable array selfcheck,
and AFAICS yours checks some more bits.
AFAICS the most interesting point in this patch are the verbose
messages/comments.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Stefan Tauner - 2014-06-01 02:01:55
On Sun, 01 Jun 2014 02:30:55 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> Am 01.06.2014 02:27 schrieb Carl-Daniel Hailfinger:
> > Found this bitrotting away on my disk. Rebased from r1645, last time all
> > snippets compiled was r1539. If you kill the cli_classic.c hunk, it
> > should compile. That hunk was in the patch to make sure the msg_gspew()
> > calls in selfcheck() aren't no-ops by default. Hm. This seems to be a
> > bug still in HEAD.
> >
> > Not for direct merge, there are other random changes in there as well.
> > Stefan, you already wrote a patch doing a board enable array selfcheck,
> > and AFAICS yours checks some more bits.
> > AFAICS the most interesting point in this patch are the verbose
> > messages/comments.

I decided against detailed messages because it is really bloat... these
checks should only be seen by developers (actually they should never be
seen :) and they ought to understand the rules and if not they should
read the verbose comments of the table.
That said, my patch gives the vendor and board name to indicate the
line much better than your index-based address.

> > +		if (!board->first_card_vendor && !board->dmi_pattern) {
> > +			msg_gerr("Zero first subsystem vendor and no DMI "
> > +				 "pattern in position %i of the board enable "
> > +				 "table\n", i);
> > +			ret = 1;
> > +		}
> > +		if (!board->first_card_device && !board->dmi_pattern) {
> > +			msg_gerr("Zero first subsystem device and no DMI "
> > +				 "pattern in position %i of the board enable "
> > +				 "table\n", i);
> > +			ret = 1;
> > +		}
> > +		if (!board->first_card_vendor && board->dmi_pattern) {
> > +			/* Such boards are usually really old. No error. */
> > +			msg_gspew("Zero first subsystem vendor but with DMI pattern in position %i "
> > +				  "of the board enable table\n", i);
> > +		}
> > +		if (!board->first_card_device && board->dmi_pattern) {
> > +			/* Such boards are usually really old. No error. */
> > +			msg_gspew("Zero first subsystem device but with DMI pattern in position %i "
> > +				  "of the board enable table\n", i);
> > +		}

I had these empty subsystem ID checks too, but I think they are
worthless. As you correctly state such board enables exist and they are
valid - they are for boards that have no subsystem IDs at all AFAIK.
This could happen anytime again and is ok. Maybe we want to use a
dedicated "none" value instead of the valid 0 ID, but I think that
would be overkill. I would just accept them/not warn.

So to sum up, I think your patch is too verbose for the problem at
hand. If you also agree on the 0 subsystem IDs, id rather use my
variant (or something similarly concise).

Patch

Index: flashrom-boardenable_selfcheck/cli_classic.c
===================================================================
--- flashrom-boardenable_selfcheck/cli_classic.c	(Revision 1806)
+++ flashrom-boardenable_selfcheck/cli_classic.c	(Arbeitskopie)
@@ -139,8 +139,10 @@ 
 	print_version();
 	print_banner();
 
+	verbose = 2;
 	if (selfcheck())
 		exit(1);
+	verbose = 0;
 
 	setbuf(stdout, NULL);
 	/* FIXME: Delay all operation_specified checks until after command
Index: flashrom-boardenable_selfcheck/flashrom.c
===================================================================
--- flashrom-boardenable_selfcheck/flashrom.c	(Revision 1806)
+++ flashrom-boardenable_selfcheck/flashrom.c	(Arbeitskopie)
@@ -1303,6 +1303,10 @@ 
 				eraser.eraseblocks[i].size;
 		}
 		/* Empty eraseblock definition with erase function.  */
+		/* FIXME: This message will never trigger because spew and debug
+		 * messages are not printed at this stage before debug variable
+		 * initialization.
+		 */
 		if (!done && eraser.block_erase)
 			msg_gspew("Strange: Empty eraseblock definition with "
 				  "non-empty erase function. Not an error.\n");
@@ -1768,7 +1772,11 @@ 
 				ret = 1;
 			}
 		}
+		ret = 1;
 	}
+	if (selfcheck_boardenables()) {
+		msg_gerr("Board enable self check failed!\n");
+	}
 
 	/* TODO: implement similar sanity checks for other arrays where deemed necessary. */
 	return ret;
Index: flashrom-boardenable_selfcheck/programmer.h
===================================================================
--- flashrom-boardenable_selfcheck/programmer.h	(Revision 1806)
+++ flashrom-boardenable_selfcheck/programmer.h	(Arbeitskopie)
@@ -263,6 +263,7 @@ 
 uint8_t sio_read(uint16_t port, uint8_t reg);
 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);
+int selfcheck_boardenables(void);
 void board_handle_before_superio(void);
 void board_handle_before_laptop(void);
 int board_flash_enable(const char *vendor, const char *model, const char *cb_vendor, const char *cb_model);
Index: flashrom-boardenable_selfcheck/board_enable.c
===================================================================
--- flashrom-boardenable_selfcheck/board_enable.c	(Revision 1806)
+++ flashrom-boardenable_selfcheck/board_enable.c	(Arbeitskopie)
@@ -2532,9 +2532,72 @@ 
 	return NULL;
 }
 
+int selfcheck_boardenables(void)
+{
+	struct board_pciid_enable *board = board_pciid_enables;
+	int ret = 0;
+	int i = 0;
+
+	for (; board->vendor_name; board++) {
+		if (!board->first_vendor) {
+			/* This would only be valid for boards which don't have
+			 * any onboard PCI devices at all.
+			 */
+			msg_gerr("Zero first vendor in position %i "
+				 "of the board enable table\n", i);
+			ret = 1;
+		}
+		if (!board->first_device) {
+			/* This would only be valid for boards which don't have
+			 * any onboard PCI devices at all.
+			 */
+			msg_gerr("Zero first device in position %i "
+				 "of the board enable table\n", i);
+			ret = 1;
+		}
+		if (!board->board_name) {
+			/* A nameless board in the list is a maintenance
+			 * nightmare.
+			 */
+			msg_gerr("Missing board name in position %i "
+				 "of the board enable table\n", i);
+			ret = 1;
+		}
+		if (!board->first_card_vendor && !board->dmi_pattern) {
+			msg_gerr("Zero first subsystem vendor and no DMI "
+				 "pattern in position %i of the board enable "
+				 "table\n", i);
+			ret = 1;
+		}
+		if (!board->first_card_device && !board->dmi_pattern) {
+			msg_gerr("Zero first subsystem device and no DMI "
+				 "pattern in position %i of the board enable "
+				 "table\n", i);
+			ret = 1;
+		}
+		if (!board->first_card_vendor && board->dmi_pattern) {
+			/* Such boards are usually really old. No error. */
+			msg_gspew("Zero first subsystem vendor but with DMI pattern in position %i "
+				  "of the board enable table\n", i);
+		}
+		if (!board->first_card_device && board->dmi_pattern) {
+			/* Such boards are usually really old. No error. */
+			msg_gspew("Zero first subsystem device but with DMI pattern in position %i "
+				  "of the board enable table\n", i);
+		}
+
+		i++;
+	}
+	
+	return ret;
+}
+
 /*
  * Match boards on PCI IDs and subsystem IDs.
  * Second set of IDs can be either main+subsystem IDs, main IDs or no IDs.
+ *
+ * Logic of this function: First PCI dev has to match ven/dev/subven/subdev.
+ * The second PCI dev is completely optional (bad idea).
  */
 const static struct board_match *board_match_pci_ids(enum board_match_phase phase)
 {