v3 still has many places where config variables are tested with #if in v2 style. That fails silently in v3. Fix up the most obvious ones.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-if0crud-2/include/spinlock.h =================================================================== --- LinuxBIOSv3-if0crud-2/include/spinlock.h (Revision 596) +++ LinuxBIOSv3-if0crud-2/include/spinlock.h (Arbeitskopie) @@ -23,7 +23,7 @@ #ifndef SPINLOCK_H #define SPINLOCK_H
-#if defined(CONFIG_SMP) && CONFIG_SMP == 1 +#ifdef CONFIG_SMP
#include <arch/spinlock.h>
Index: LinuxBIOSv3-if0crud-2/device/device.c =================================================================== --- LinuxBIOSv3-if0crud-2/device/device.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/device/device.c (Arbeitskopie) @@ -534,7 +534,7 @@ base, bridge->size, bridge->align, bridge->gran); }
-#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN struct device *vga_pri = 0; int vga_inited = 0; static void allocate_vga_resource(void) @@ -580,8 +580,7 @@ if (!vga) { vga = vga_first; } -#if defined(CONFIG_INITIALIZE_ONBOARD_VGA_FIRST) && \ - CONFIG_INITIALIZE_ONBOARD_VGA_FIRST == 1 +#ifdef CONFIG_INITIALIZE_ONBOARD_VGA_FIRST if (vga_onboard) // Will use on board VGA as pri. #else if (!vga) // Will use last add on adapter as pri. @@ -919,7 +918,7 @@ mem->flags |= IORESOURCE_ASSIGNED; mem->flags &= ~IORESOURCE_STORED;
-#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN /* Allocate the VGA I/O resource. */ allocate_vga_resource(); #endif Index: LinuxBIOSv3-if0crud-2/device/pci_rom.c =================================================================== --- LinuxBIOSv3-if0crud-2/device/pci_rom.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/device/pci_rom.c (Arbeitskopie) @@ -105,7 +105,7 @@
static void *pci_ram_image_start = (void *)PCI_RAM_IMAGE_START;
-#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN extern int vga_inited; // Defined in vga_console.c. #ifndef CONFIG_MULTIPLE_VGA_INIT extern struct device *vga_pri; // The primary VGA device, defined in device.c. @@ -135,7 +135,7 @@ rom_size = rom_header->size * 512;
if (PCI_CLASS_DISPLAY_VGA == rom_data->class_hi) { -#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN #ifndef CONFIG_MULTIPLE_VGA_INIT if (dev != vga_pri) return NULL; // Only one VGA supported. Index: LinuxBIOSv3-if0crud-2/device/pci_device.c =================================================================== --- LinuxBIOSv3-if0crud-2/device/pci_device.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/device/pci_device.c (Arbeitskopie) @@ -702,7 +702,7 @@ void pci_dev_init(struct device *dev) { printk(BIOS_SPEW, "PCI: pci_dev_init\n"); -#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN void run_bios(struct device *dev, unsigned long addr); struct rom_header *rom, *ram;
Index: LinuxBIOSv3-if0crud-2/lib/console.c =================================================================== --- LinuxBIOSv3-if0crud-2/lib/console.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/lib/console.c (Arbeitskopie) @@ -89,7 +89,7 @@ #ifdef CONFIG_CONSOLE_SERIAL if (byte == '\n') { uart8250_tx_byte(TTYSx_BASE, '\r'); -#if defined(CONFIG_CONSOLE_PREFIX) && (CONFIG_CONSOLE_PREFIX == 1) +#ifdef CONFIG_CONSOLE_PREFIX uart8250_tx_byte(TTYSx_BASE, '\n'); uart8250_tx_byte(TTYSx_BASE, '('); uart8250_tx_byte(TTYSx_BASE, 'L'); Index: LinuxBIOSv3-if0crud-2/arch/x86/mc146818rtc.c =================================================================== --- LinuxBIOSv3-if0crud-2/arch/x86/mc146818rtc.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/arch/x86/mc146818rtc.c (Arbeitskopie) @@ -43,7 +43,7 @@ outb((val),RTC_PORT(1)); \ })
-#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE
static int rtc_checksum_valid(int range_start, int range_end, int cks_loc) { @@ -78,14 +78,14 @@
void rtc_init(int invalid) { -#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE unsigned char x; int cmos_invalid, checksum_invalid; #endif
printk(BIOS_DEBUG, "Initializing realtime clock.\n");
-#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE
/* See if there has been a CMOS power problem. */ x = CMOS_READ(RTC_VALID); @@ -127,7 +127,7 @@ /* Setup the frequency it operates at */ CMOS_WRITE(RTC_FREQ_SELECT_DEFAULT, RTC_FREQ_SELECT);
-#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE /* See if there is a coreboot CMOS checksum error */ checksum_invalid = !rtc_checksum_valid(CB_CKS_RANGE_START, CB_CKS_RANGE_END,CB_CKS_LOC); @@ -224,7 +224,7 @@ if(!rtc_checksum_valid(CB_CKS_RANGE_START, CB_CKS_RANGE_END,CB_CKS_LOC)) return(-4); -#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE return(0); #else return -2; Index: LinuxBIOSv3-if0crud-2/arch/x86/speaker.c =================================================================== --- LinuxBIOSv3-if0crud-2/arch/x86/speaker.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/arch/x86/speaker.c (Arbeitskopie) @@ -93,7 +93,7 @@ */ void beep_short(void) { -#if defined(CONFIG_BEEPS) && (CONFIG_BEEPS == 1) +#ifdef CONFIG_BEEPS speaker_tone(1760, 500); /* 1760 == note A6. */ #endif } @@ -103,7 +103,7 @@ */ void beep_long(void) { -#if defined(CONFIG_BEEPS) && (CONFIG_BEEPS == 1) +#ifdef CONFIG_BEEPS speaker_tone(1760, 2000); /* 1760 == note A6. */ #endif }
Carl-Daniel Hailfinger wrote:
v3 still has many places where config variables are tested with #if in v2 style. That fails silently in v3. Fix up the most obvious ones.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
While the patch itself is discussable, it is cosmetics and not a fix. In fact you are removing the check of the CONFIG_ variable's value, and so if for some reason someone sets a defined value to 0, THEN the code would fail silently after applying your diff.
Can you please elaborate how any of the lines you touch would cause silent failure in v3?
Index: LinuxBIOSv3-if0crud-2/include/spinlock.h
--- LinuxBIOSv3-if0crud-2/include/spinlock.h (Revision 596) +++ LinuxBIOSv3-if0crud-2/include/spinlock.h (Arbeitskopie) @@ -23,7 +23,7 @@ #ifndef SPINLOCK_H #define SPINLOCK_H
-#if defined(CONFIG_SMP) && CONFIG_SMP == 1 +#ifdef CONFIG_SMP
#include <arch/spinlock.h>
Index: LinuxBIOSv3-if0crud-2/device/device.c
--- LinuxBIOSv3-if0crud-2/device/device.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/device/device.c (Arbeitskopie) @@ -534,7 +534,7 @@ base, bridge->size, bridge->align, bridge->gran); }
-#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN struct device *vga_pri = 0; int vga_inited = 0; static void allocate_vga_resource(void) @@ -580,8 +580,7 @@ if (!vga) { vga = vga_first; } -#if defined(CONFIG_INITIALIZE_ONBOARD_VGA_FIRST) && \
- CONFIG_INITIALIZE_ONBOARD_VGA_FIRST == 1
+#ifdef CONFIG_INITIALIZE_ONBOARD_VGA_FIRST if (vga_onboard) // Will use on board VGA as pri. #else if (!vga) // Will use last add on adapter as pri. @@ -919,7 +918,7 @@ mem->flags |= IORESOURCE_ASSIGNED; mem->flags &= ~IORESOURCE_STORED;
-#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN /* Allocate the VGA I/O resource. */ allocate_vga_resource(); #endif Index: LinuxBIOSv3-if0crud-2/device/pci_rom.c =================================================================== --- LinuxBIOSv3-if0crud-2/device/pci_rom.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/device/pci_rom.c (Arbeitskopie) @@ -105,7 +105,7 @@
static void *pci_ram_image_start = (void *)PCI_RAM_IMAGE_START;
-#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN extern int vga_inited; // Defined in vga_console.c. #ifndef CONFIG_MULTIPLE_VGA_INIT extern struct device *vga_pri; // The primary VGA device, defined in device.c. @@ -135,7 +135,7 @@ rom_size = rom_header->size * 512;
if (PCI_CLASS_DISPLAY_VGA == rom_data->class_hi) { -#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN #ifndef CONFIG_MULTIPLE_VGA_INIT if (dev != vga_pri) return NULL; // Only one VGA supported. Index: LinuxBIOSv3-if0crud-2/device/pci_device.c =================================================================== --- LinuxBIOSv3-if0crud-2/device/pci_device.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/device/pci_device.c (Arbeitskopie) @@ -702,7 +702,7 @@ void pci_dev_init(struct device *dev) { printk(BIOS_SPEW, "PCI: pci_dev_init\n"); -#if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1 +#ifdef CONFIG_PCI_OPTION_ROM_RUN void run_bios(struct device *dev, unsigned long addr); struct rom_header *rom, *ram;
Index: LinuxBIOSv3-if0crud-2/lib/console.c
--- LinuxBIOSv3-if0crud-2/lib/console.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/lib/console.c (Arbeitskopie) @@ -89,7 +89,7 @@ #ifdef CONFIG_CONSOLE_SERIAL if (byte == '\n') { uart8250_tx_byte(TTYSx_BASE, '\r'); -#if defined(CONFIG_CONSOLE_PREFIX) && (CONFIG_CONSOLE_PREFIX == 1) +#ifdef CONFIG_CONSOLE_PREFIX uart8250_tx_byte(TTYSx_BASE, '\n'); uart8250_tx_byte(TTYSx_BASE, '('); uart8250_tx_byte(TTYSx_BASE, 'L'); Index: LinuxBIOSv3-if0crud-2/arch/x86/mc146818rtc.c =================================================================== --- LinuxBIOSv3-if0crud-2/arch/x86/mc146818rtc.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/arch/x86/mc146818rtc.c (Arbeitskopie) @@ -43,7 +43,7 @@ outb((val),RTC_PORT(1)); \ })
-#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE
static int rtc_checksum_valid(int range_start, int range_end, int cks_loc) { @@ -78,14 +78,14 @@
void rtc_init(int invalid) { -#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE unsigned char x; int cmos_invalid, checksum_invalid; #endif
printk(BIOS_DEBUG, "Initializing realtime clock.\n");
-#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE
/* See if there has been a CMOS power problem. */ x = CMOS_READ(RTC_VALID); @@ -127,7 +127,7 @@ /* Setup the frequency it operates at */ CMOS_WRITE(RTC_FREQ_SELECT_DEFAULT, RTC_FREQ_SELECT);
-#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE /* See if there is a coreboot CMOS checksum error */ checksum_invalid = !rtc_checksum_valid(CB_CKS_RANGE_START, CB_CKS_RANGE_END,CB_CKS_LOC); @@ -224,7 +224,7 @@ if(!rtc_checksum_valid(CB_CKS_RANGE_START, CB_CKS_RANGE_END,CB_CKS_LOC)) return(-4); -#if defined(CONFIG_OPTION_TABLE) && (CONFIG_OPTION_TABLE == 1) +#ifdef CONFIG_OPTION_TABLE return(0); #else return -2; Index: LinuxBIOSv3-if0crud-2/arch/x86/speaker.c =================================================================== --- LinuxBIOSv3-if0crud-2/arch/x86/speaker.c (Revision 596) +++ LinuxBIOSv3-if0crud-2/arch/x86/speaker.c (Arbeitskopie) @@ -93,7 +93,7 @@ */ void beep_short(void) { -#if defined(CONFIG_BEEPS) && (CONFIG_BEEPS == 1) +#ifdef CONFIG_BEEPS speaker_tone(1760, 500); /* 1760 == note A6. */ #endif } @@ -103,7 +103,7 @@ */ void beep_long(void) { -#if defined(CONFIG_BEEPS) && (CONFIG_BEEPS == 1) +#ifdef CONFIG_BEEPS speaker_tone(1760, 2000); /* 1760 == note A6. */ #endif }
The whole #ifdef vs. #if is a very old argument in linuxbios/coreboot.
initially, it was #ifdef in old v1. Then, later, there was a feeling it should be linux style, #if.
The change from #ifdef to #if caused lots of silent failures. Now we're changing back.
The main thing is that we have to be consistent. I am not particular either way, but it is important to understand that this argument has been ongoing, with strong discussion over the years.
ron
On 14.02.2008 19:31, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
v3 still has many places where config variables are tested with #if in v2 style. That fails silently in v3. Fix up the most obvious ones.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
While the patch itself is discussable, it is cosmetics and not a fix. In fact you are removing the check of the CONFIG_ variable's value, and so if for some reason someone sets a defined value to 0, THEN the code would fail silently after applying your diff.
Can you please elaborate how any of the lines you touch would cause silent failure in v3?
All of them. These values are Kconfig bools and if they are set, they are set to "y" and not 1.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 14.02.2008 19:31, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
v3 still has many places where config variables are tested with #if in v2 style. That fails silently in v3. Fix up the most obvious ones.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
While the patch itself is discussable, it is cosmetics and not a fix. In fact you are removing the check of the CONFIG_ variable's value, and so if for some reason someone sets a defined value to 0, THEN the code would fail silently after applying your diff.
Can you please elaborate how any of the lines you touch would cause silent failure in v3?
All of them. These values are Kconfig bools and if they are set, they are set to "y" and not 1.
That's wrong, so the answer is indeed "none of them" as I expected.
Here's an excerpt from config.h as generated by Kconfig:
/*
- Compression
*/ #define CONFIG_COMPRESSION_LZMA 1 #undef CONFIG_COMPRESSION_NRV2B #define CONFIG_DEFAULT_COMPRESSION_LZMA 1 #undef CONFIG_DEFAULT_COMPRESSION_NRV2B #undef CONFIG_DEFAULT_COMPRESSION_NONE
Stefan
On 14.02.2008 21:59, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 14.02.2008 19:31, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
v3 still has many places where config variables are tested with #if in v2 style. That fails silently in v3. Fix up the most obvious ones.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
While the patch itself is discussable, it is cosmetics and not a fix. In fact you are removing the check of the CONFIG_ variable's value, and so if for some reason someone sets a defined value to 0, THEN the code would fail silently after applying your diff.
Can you please elaborate how any of the lines you touch would cause silent failure in v3?
All of them. These values are Kconfig bools and if they are set, they are set to "y" and not 1.
That's wrong, so the answer is indeed "none of them" as I expected.
Here's an excerpt from config.h as generated by Kconfig:
I stand corrected. However, the test for the #define being 1 is superfluous if the #define is a bool.
New patch description: Remove superfluous checks for boolean CONFIG_* variables where we tested CONFIG_* == 1. If those variables are set, they will always be 1.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 14.02.2008 21:59, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 14.02.2008 19:31, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
v3 still has many places where config variables are tested with #if in v2 style. That fails silently in v3. Fix up the most obvious ones.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
While the patch itself is discussable, it is cosmetics and not a fix. In fact you are removing the check of the CONFIG_ variable's value, and so if for some reason someone sets a defined value to 0, THEN the code would fail silently after applying your diff.
Can you please elaborate how any of the lines you touch would cause silent failure in v3?
All of them. These values are Kconfig bools and if they are set, they are set to "y" and not 1.
That's wrong, so the answer is indeed "none of them" as I expected.
Here's an excerpt from config.h as generated by Kconfig:
I stand corrected. However, the test for the #define being 1 is superfluous if the #define is a bool.
New patch description: Remove superfluous checks for boolean CONFIG_* variables where we tested CONFIG_* == 1. If those variables are set, they will always be 1.
Acked-by: Stefan Reinauer stepan@coresystems.de
On 14.02.2008 22:23, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Remove superfluous checks for boolean CONFIG_* variables where we tested CONFIG_* == 1. If those variables are set, they will always be 1.
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks, r599.
Regards, Carl-Daniel
On Thu, Feb 14, 2008 at 10:23:56PM +0100, Stefan Reinauer wrote:
I stand corrected. However, the test for the #define being 1 is superfluous if the #define is a bool.
Acked-by: Stefan Reinauer stepan@coresystems.de
I'd like to nak. But already committed.
I would like to be more tolerant to #defines being 0.
//Peter
On 15.02.2008 01:26, Peter Stuge wrote:
On Thu, Feb 14, 2008 at 10:23:56PM +0100, Stefan Reinauer wrote:
I stand corrected. However, the test for the #define being 1 is superfluous if the #define is a bool.
Acked-by: Stefan Reinauer stepan@coresystems.de
I'd like to nak. But already committed.
Sorry.
I would like to be more tolerant to #defines being 0.
If these #defines were 0, we would have a Kconfig bug (at least that's how I understand it). For non-bool Kconfig variables, #defines still can be 0 or anything else.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich -
Stefan Reinauer