[LinuxBIOS] patch to lib/lar.c and include/lar.h

Peter Stuge peter at stuge.se
Mon Dec 17 04:05:33 CET 2007

On Sat, Dec 15, 2007 at 06:00:43PM -0800, ron minnich wrote:
> Support a return from disable_car is a very difficult proposition.


> The intent is that I can do this:
> ret = execute_in_place(&archive, "normal/initram/segment0", main_ram_working);
> or similar and thus chain from initram to the second part of main,
> with a new stack and working dram.

I don't like the patch as is.

It is now becoming non-obvious from function names and parameter
names what the code flow is.

This is a problem in v2 that I would like us to be careful to not
introduce also in v3.

> - * run_address is passed the address of a function taking no parameters and
> + * run_address is passed the address of a function taking a single void * parameter
>   * jumps to it, returning the result. 
>   * @param v the address to call as a function. 
>   * returns value returned by the function. 
>   */
> -int run_address(void *f)
> +int run_address(void *f, void *param)

Please fix up the doxygen comments completely. Note that all
parameters in the doxygen comment are incorrect.

> @@ -263,9 +263,10 @@
>   * Given a file name in the LAR , search for it, and execute it in place. 
>   * @param archive A descriptor for current archive.
>   * @param filename filename to find
> + * @param param void * parameter
>   * returns 0 on success, -1 otherwise
>   */
> -int execute_in_place(const struct mem_file *archive, const char *filename)
> +int execute_in_place(const struct mem_file *archive, const char *filename, void *param)

The name 'param' is really not transparent and the doxygen comment is
not clear enough about how this param will be used - ie. that it is
just passed on as a parameter to the function that actually executes
the file.

Generally speaking I think this arrangement is pretty ugly.
(Passing a parameter several layers into the code.)

In this instance I do think it's a pretty good solution, because it
is actually fairly simple, especially considering the alternatives.

I guess I am proposing that we make the parameter names and possibly
also some function names less generic, because this is something that
will be used exclusively during one specific phase transition.

Good or bad?


More information about the coreboot mailing list