[LinuxBIOS] [PATCH] Fix various compiler warnings and potential bugs
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Feb 1 19:52:36 CET 2007
Ed Swierk wrote:
> This patch fixes a bunch of compiler warnings I ran into while
> building an MCP55 target using gcc 4.1.1.
>
> A few are real bugs, like the misspelled "default" in raminit_f.c and
> the truncated 16-bit argument to delayx() in mcp55_early_setup_car.c.
> The uninitialized variables in raminit_f_dqs.c are potential bugs.
Ack.
> gcc 4.1.x is famously paranoid about certain things, and some of its
> warnings can only be considered compiler bugs, so I ignored those. The
> minor warnings I fixed are mainly improper int/pointer casts and
> unused variables.
Hmmm. You know that the signedness of char is not defined?
Besides that, we definitely should enable -fno-strict-aliasing in the
gcc flags until we have audited all casts.
> --- LinuxBIOSv2-2540.orig/util/options/build_opt_tbl.c
> +++ LinuxBIOSv2-2540/util/options/build_opt_tbl.c
> @@ -332,7 +332,7 @@ int main(int argc, char **argv)
> fprintf(stderr, "Error - Length is to long in line \n%s\n",line);
> exit(1);
> }
> - if (!is_ident(ce->name)) {
> + if (!is_ident((char *) ce->name)) {
> fprintf(stderr,
> "Error - Name %s is an invalid identifier in line\n %s\n",
> ce->name, line);
Nack. Fix struct cmos_entries instead. is_ident_* needs a lot
of cleaning, too.
> @@ -341,7 +341,7 @@ int main(int argc, char **argv)
> /* put in the record type */
> ce->tag=LB_TAG_OPTION;
> /* calculate and save the record length */
> - len=strlen(ce->name)+1;
> + len=strlen((char *) ce->name)+1;
> /* make the record int aligned */
> if(len%4)
> len+=(4-(len%4));
ditto
> @@ -540,7 +540,7 @@ int main(int argc, char **argv)
> if (ce->config == 'r') {
> continue;
> }
> - if (!is_ident(ce->name)) {
> + if (!is_ident((char *) ce->name)) {
> fprintf(stderr, "Invalid identifier: %s\n",
> ce->name);
> exit(1);
ditto
> --- LinuxBIOSv2-2540.orig/src/cpu/amd/model_fxx/model_fxx_init.c
> +++ LinuxBIOSv2-2540/src/cpu/amd/model_fxx/model_fxx_init.c
> @@ -17,6 +17,7 @@
> #include <cpu/x86/pae.h>
> #include <pc80/mc146818rtc.h>
> #include <cpu/x86/lapic.h>
> +#include <part/hard_reset.h>
>
> #include "../../../northbridge/amd/amdk8/amdk8.h"
>
> @@ -473,7 +474,7 @@ static void amd_set_name_string_f(device
> unsigned nN;
> unsigned unknown = 1;
>
> - uint8_t str[48];
> + char str[48];
> uint32_t *p;
>
> msr_t msr;
Please remove the magic constant 48 when you change this line. Same for
all other magic 48.
> @@ -533,7 +534,7 @@ static void amd_set_name_string_f(device
> #endif
> }
>
> - p = str;
> + p = (uint32_t *) str;
> for(i=0;i<6;i++) {
> msr.lo = *p; p++; msr.hi = *p; p++;
> wrmsr(0xc0010030+i, msr);
While I agree with this cast, gcc is free to break this code completely
unless we use -fno-strict-aliasing.
> Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/coherent_ht_car.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/coherent_ht_car.c
> +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/coherent_ht_car.c
> @@ -1662,7 +1662,6 @@ static int apply_cpu_errata_fixes(unsign
> int needs_reset = 0;
> for(node = 0; node < nodes; node++) {
> device_t dev;
> - uint32_t cmd;
> dev = NODE_MC(node);
> #if K8_REV_F_SUPPORT == 0
> if (is_cpu_pre_c0()) {
> Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/misc_control.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/misc_control.c
> +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/misc_control.c
> @@ -110,7 +110,10 @@ static void misc_control_init(struct dev
> {
> uint32_t cmd, cmd_ref;
> int needs_reset;
> - struct device *f0_dev, *f2_dev;
> + struct device *f0_dev;
> +#if K8_REV_F_SUPPORT == 0
> + struct device *f2_dev;
> +#endif
>
> printk_debug("NB: Function 3 Misc Control.. ");
> needs_reset = 0;
> Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/raminit_f.c
> +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f.c
> @@ -2506,7 +2506,7 @@ static void set_misc_timing(const struct
> case 0x00:
> dwordx = 0x002b2220; //x8 double Rank
> break;
> - defalut:
> + default:
> dwordx = 0x002a2220; //x8 single Rank and double Rank mixed
> }
> } else if((meminfo->x4_mask == 0) && (meminfo->x16_mask == 0x01) && (meminfo->single_rank_mask == 0x01)) {
Nice spot.
> @@ -2865,7 +2865,7 @@ static void sdram_enable(int controllers
>
> /* Before enabling memory start the memory clocks */
> for(i = 0; i < controllers; i++) {
> - uint32_t dtl, dch;
> + uint32_t dch;
> if (!sysinfo->ctrl_present[ i ])
> continue;
> dch = pci_read_config32(ctrl[i].f2, DRAM_CONFIG_HIGH);
> @@ -2938,7 +2938,7 @@ static void sdram_enable(int controllers
> }
>
> for(i = 0; i < controllers; i++) {
> - uint32_t dcl, dch, dcm;
> + uint32_t dcl, dcm;
> if (!sysinfo->ctrl_present[ i ])
> continue;
> /* Skip everything if I don't have any memory on this controller */
> Index: LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f_dqs.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/northbridge/amd/amdk8/raminit_f_dqs.c
> +++ LinuxBIOSv2-2540/src/northbridge/amd/amdk8/raminit_f_dqs.c
> @@ -551,13 +551,15 @@ static unsigned TrainRcvrEn(const struct
>
> unsigned TestAddr0, TestAddr0B, TestAddr1, TestAddr1B;
>
> - unsigned CurrRcvrCHADelay;
> + unsigned CurrRcvrCHADelay = 0;
>
> unsigned tmp;
>
> unsigned is_Width128 = sysinfo->meminfo[ctrl->node_id].is_Width128;
>
> +#if K8_REV_F_SUPPORT_F0_F1_WORKAROUND == 1
> unsigned cpu_f0_f1;
> +#endif
>
> if(Pass == DQS_FIRST_PASS) {
> InitDQSPos4RcvrEn(ctrl);
> @@ -1205,6 +1207,7 @@ static unsigned TrainDQSPos(const struct
>
> LastTest = DQS_FAIL;
> RnkDlySeqPassMax = 0;
> + RnkDlySeqPassMin = 0;
> RnkDlyFilterMax = 0;
> RnkDlyFilterMin = 0;
> for(DQSDelay=0; DQSDelay<48; DQSDelay++) {
> Index: LinuxBIOSv2-2540/src/pc80/mc146818rtc.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/pc80/mc146818rtc.c
> +++ LinuxBIOSv2-2540/src/pc80/mc146818rtc.c
> @@ -123,8 +123,10 @@ static void rtc_set_checksum(int range_s
>
> void rtc_init(int invalid)
> {
> +#if HAVE_OPTION_TABLE
> unsigned char x;
> int cmos_invalid, checksum_invalid;
> +#endif
>
> printk_debug("RTC Init\n");
>
> Index: LinuxBIOSv2-2540/src/ram/ramtest.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/ram/ramtest.c
> +++ LinuxBIOSv2-2540/src/ram/ramtest.c
> @@ -94,7 +94,6 @@ static void ram_verify(unsigned long sta
>
> void ram_check(unsigned long start, unsigned long stop)
> {
> - int result;
> /*
> * This is much more of a "Is my DRAM properly configured?"
> * test than a "Is my DRAM faulty?" test. Not all bits
> Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_aza.c
> +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_aza.c
> @@ -27,6 +27,7 @@
> #include <device/pci_ids.h>
> #include <device/pci_ops.h>
> #include <arch/io.h>
> +#include <delay.h>
> #include "mcp55.h"
>
> static int set_bits(uint8_t *port, uint32_t mask, uint32_t val)
> @@ -228,7 +229,7 @@ static void aza_init(struct device *dev)
> if(!res)
> return;
>
> - base =(uint8_t *) res->base;
> + base =(uint8_t *) ((uint32_t) res->base);
> printk_debug("base = %08x\n", base);
>
> codec_mask = codec_detect(base);
Are you really sure you want to cast a uint64_t to uint32_t to uint8_t * ?
> Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
> +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
> @@ -175,7 +175,8 @@ static void mcp55_early_pcie_setup(unsig
> pci_write_config32(dev, 0xe4, dword);
>
> // need to wait 100ms
> - delayx(1000);
> + for (i=0; i<10; i++)
> + delayx(100);
> }
>
> static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn, unsigned *devn, unsigned *io_base, unsigned *pci_e_x)
Nack. You increased the delay by a factor of 10.
> Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_lpc.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_lpc.c
> +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_lpc.c
> @@ -243,7 +243,6 @@ static void lpc_init(device_t dev)
> static void mcp55_lpc_read_resources(device_t dev)
> {
> struct resource *res;
> - unsigned long index;
>
> /* Get the normal pci resources of this device */
> pci_dev_read_resources(dev); // We got one for APIC, or one more for TRAP
> Index: LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_nic.c
> ===================================================================
> --- LinuxBIOSv2-2540.orig/src/southbridge/nvidia/mcp55/mcp55_nic.c
> +++ LinuxBIOSv2-2540/src/southbridge/nvidia/mcp55/mcp55_nic.c
> @@ -28,6 +28,7 @@
> #include <device/pci_ids.h>
> #include <device/pci_ops.h>
> #include <arch/io.h>
> +#include <delay.h>
> #include "mcp55.h"
>
> static int phy_read(uint8_t *base, unsigned phy_addr, unsigned phy_reg)
> @@ -55,7 +56,7 @@ static int phy_read(uint8_t *base, unsig
>
> }
>
> -static int phy_detect(uint8_t *base)
> +static void phy_detect(uint8_t *base)
> {
> uint32_t dword;
> int i;
> @@ -94,7 +95,6 @@ static int phy_detect(uint8_t *base)
> }
> static void nic_init(struct device *dev)
> {
> - uint32_t dword, old;
> uint32_t mac_h, mac_l;
> int eeprom_valid = 0;
> struct southbridge_nvidia_mcp55_config *conf;
> @@ -108,7 +108,7 @@ static void nic_init(struct device *dev)
>
> if(!res) return;
>
> - base = res->base;
> + base = (uint8_t *) ((uint32_t) res->base);
>
> phy_detect(base);
>
> @@ -154,8 +154,7 @@ static void nic_init(struct device *dev)
> }
> // if that is invalid we will read that from romstrap
> if(!eeprom_valid) {
> - unsigned long mac_pos;
> - mac_pos = 0xffffffd0; // refer to romstrap.inc and romstrap.lds
> + uint32_t *mac_pos = (uint32_t *) 0xffffffd0; // refer to romstrap.inc and romstrap.lds
> mac_l = readl(mac_pos) + nic_index; // overflow?
> mac_h = readl(mac_pos + 4);
>
Hmmm. I have to reread that.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list