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
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) {
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.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
selfcheck_board_enables() needs an additional bugfix to compile. See below. Sorry.
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.*/@@ -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;
Sorry. This should have been struct board_match *board = board_matches;
- 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) {
Regards, Carl-Daniel
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).
-
Carl-Daniel Hailfinger -
Stefan Tauner