[flashrom] [PATCH] board_enable selfcheck

Stefan Tauner stefan.tauner at alumni.tuwien.ac.at
Sun Jun 1 04:01:55 CEST 2014


On Sun, 01 Jun 2014 02:30:55 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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).

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list