Re: [LinuxBIOS] [PATCH] Fix various compiler warnings and potential bugs

[Carl-Daniel Hailfinger]

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