Patchwork option_table.h race

login
register
about
Submitter Stefan Reinauer
Date 2010-09-01 11:30:55
Message ID <4C7E396F.5080307@coresystems.de>
Download mbox | patch
Permalink /patch/1843/
State Superseded
Headers show

Comments

Stefan Reinauer - 2010-09-01 11:30:55
See patch.
Fix race condition in option_table.h generation by moving the include statement
to those files that actually need it. This significantly reduces the number of
dependencies, so it's no longer extremely ugly to specify them manually (see 
the src/pc80/Makefile.inc portion)
Also, drop the AMD DBM690T work around for the issue.

Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Myles Watson - 2010-09-01 15:26:50
The only thing that worries me is this include.

from src/include/pc80/mc146818rtc.h:
#include <pc80/mc146818rtc_early.c>

It seems like usually when we make initobj, we stop including the c file.

Acked-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Stefan Reinauer - 2010-09-01 16:20:38
On 9/1/10 5:26 PM, Myles Watson wrote:
> The only thing that worries me is this include.
>
> from src/include/pc80/mc146818rtc.h:
> #include <pc80/mc146818rtc_early.c>
>
> It seems like usually when we make initobj, we stop including the c file.
Yes... Another problem though.

It's guarded by #ifdef __ROMCC__ so it only gets included on romcc
targets. We could remove the C include from there and move it to the
romstage.c files, to make it more obvious which .c files get sucked in
on the targets. Not sure what else we could do..

There are 4 other places where a .c file is included in a .h file:

./src/arch/i386/include/bootblock_common.h:#include
<cpu/x86/lapic/boot_cpu.c>
./src/include/console/console.h:#include <pc80/serial.c>
./src/include/console/console.h:#include "lib/ne2k.c"
./src/include/console/console.h:#include <console/console.c>
./src/include/pc80/mc146818rtc.h:#include <pc80/mc146818rtc_early.c>

In addition:

- We have an unknown number of .h files that contain functions

- We include .c files in 148 romstage.c files

and in these:

./cpu/amd/car/post_cache_as_ram.c
./cpu/amd/dualcore/amd_sibling.c
./cpu/amd/dualcore/dualcore.c
./cpu/amd/model_10xxx/init_cpus.c
./cpu/amd/quadcore/amd_sibling.c
./cpu/amd/quadcore/quadcore.c
./cpu/amd/sc520/raminit.c
./drivers/ati/ragexl/xlinit.c
./lib/lzma.c
./mainboard/getac/p470/mainboard.c
./mainboard/getac/p470/mainboard_smi.c
./northbridge/amd/amdfam10/bootblock.c
./northbridge/amd/amdfam10/debug.c
./northbridge/amd/amdfam10/northbridge.c
./northbridge/amd/amdfam10/raminit_amdmct.c
./northbridge/amd/amdht/h3finit.c
./northbridge/amd/amdht/ht_wrapper.c
./northbridge/amd/amdk8/raminit_f.c
./northbridge/amd/amdk8/raminit_test.c
./northbridge/intel/i82830/raminit.c
./northbridge/intel/i945/early_init.c
./northbridge/intel/i945/raminit.c
./northbridge/via/vx800/raminit.c
./pc80/usbdebug_serial.c
./pc80/vga/vga.c
./southbridge/amd/amd8111/amd8111_reset.c
./southbridge/amd/amd8111/bootblock.c
./southbridge/amd/cs5530/cs5530_vga.c
./southbridge/amd/sb600/sb600_early_setup.c
./southbridge/amd/sb600/sb600_reset.c
./southbridge/amd/sb600/sb600_sm.c
./southbridge/amd/sb700/sb700_early_setup.c
./southbridge/amd/sb700/sb700_reset.c
./southbridge/amd/sb700/sb700_sm.c
./southbridge/broadcom/bcm5785/bcm5785_early_setup.c
./southbridge/broadcom/bcm5785/bcm5785_reset.c
./southbridge/broadcom/bcm5785/bootblock.c
./southbridge/intel/i82801dx/i82801dx_smihandler.c
./southbridge/intel/i82801gx/i82801gx_smihandler.c
./southbridge/nvidia/ck804/ck804_reset.c
./southbridge/nvidia/mcp55/mcp55_reset.c
./southbridge/sis/sis966/sis966_reset.c



Stefan
Myles Watson - 2010-09-01 16:28:28
On Wed, Sep 1, 2010 at 10:20 AM, Stefan Reinauer
<stefan.reinauer@coresystems.de> wrote:
>  On 9/1/10 5:26 PM, Myles Watson wrote:
>> The only thing that worries me is this include.
>>
>> from src/include/pc80/mc146818rtc.h:
>> #include <pc80/mc146818rtc_early.c>
>>
>> It seems like usually when we make initobj, we stop including the c file.
> Yes... Another problem though.
Yes.  A problem for another day.  I was just noting that it made it
harder to follow.

Thanks,
Myles

Patch

Index: src/include/pc80/mc146818rtc.h
===================================================================
--- src/include/pc80/mc146818rtc.h	(revision 5761)
+++ src/include/pc80/mc146818rtc.h	(working copy)
@@ -81,14 +81,6 @@ 
 #define PC_CKS_RANGE_END	45
 #define PC_CKS_LOC		46
 
-/* coreboot cmos checksum is usually only built over bytes 49..125
- * LB_CKS_RANGE_START, LB_CKS_RANGE_END and LB_CKS_LOC are defined
- * in option_table.h
- */
-#if CONFIG_HAVE_OPTION_TABLE
-#include <option_table.h>
-#endif
-
 #ifndef UTIL_BUILD_OPTION_TABLE
 #include <arch/io.h>
 static inline unsigned char cmos_read(unsigned char addr)
Index: src/mainboard/amd/dbm690t/Kconfig
===================================================================
--- src/mainboard/amd/dbm690t/Kconfig	(revision 5761)
+++ src/mainboard/amd/dbm690t/Kconfig	(working copy)
@@ -25,12 +25,6 @@ 
 	string
 	default amd/dbm690t
 
-# This is a temporary fix, and should be removed when the race condition for
-# building option_table.h is fixed.
-config WARNINGS_ARE_ERRORS
-	bool
-	default n
-
 config DCACHE_RAM_BASE
 	hex
 	default 0xc8000
Index: src/pc80/mc146818rtc_early.c
===================================================================
--- src/pc80/mc146818rtc_early.c	(revision 5761)
+++ src/pc80/mc146818rtc_early.c	(working copy)
@@ -1,6 +1,10 @@ 
 #include <pc80/mc146818rtc.h>
 #include <fallback.h>
 
+#if CONFIG_USE_OPTION_TABLE
+#include <option_table.h>
+#endif
+
 #ifndef CONFIG_MAX_REBOOT_CNT
 #error "CONFIG_MAX_REBOOT_CNT not defined"
 #endif
Index: src/pc80/Makefile.inc
===================================================================
--- src/pc80/Makefile.inc	(revision 5761)
+++ src/pc80/Makefile.inc	(working copy)
@@ -8,3 +8,4 @@ 
 subdirs-y += vga
 
 $(obj)/pc80/mc146818rtc.o : $(OPTION_TABLE_H)
+$(obj)/pc80/mc146818rtc_early.initobj.o : $(OPTION_TABLE_H)
Index: src/pc80/mc146818rtc.c
===================================================================
--- src/pc80/mc146818rtc.c	(revision 5761)
+++ src/pc80/mc146818rtc.c	(working copy)
@@ -2,6 +2,9 @@ 
 #include <pc80/mc146818rtc.h>
 #include <boot/coreboot_tables.h>
 #include <string.h>
+#if CONFIG_USE_OPTION_TABLE
+#include <option_table.h>
+#endif
 
 /* control registers - Moto names
  */