Re: [flashrom] [PATCH] board_enable selfcheck

[Carl-Daniel Hailfinger]

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

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.
>  */
> 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;

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