[flashrom] [PATCH] Refactor the -p internal:mainboard handling.

[Stefan Tauner]

On Sat, 18 Aug 2012 22:33:30 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 15.08.2012 04:29 schrieb Stefan Tauner:
> > On Wed, 15 Aug 2012 02:03:29 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Refactor-the-p-internal-mainboard-handling.patch
Type: text/x-patch
Size: 22802 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20120819/88478d08/attachment.patch>