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

Ed Swierk eswierk at arastra.com
Thu Feb 1 20:46:02 CET 2007


On 2/1/07, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> Hmmm. You know that the signedness of char is not defined?

Yes, but if functions like strlen() take a char *, then shouldn't the
parameter be signed rather than unsigned?

> Besides that, we definitely should enable -fno-strict-aliasing in the
> gcc flags until we have audited all casts.

Agreed.

> Please remove the magic constant 48 when you change this line. Same for
> all other magic 48.

Oh boy. You're asking me to understand the code I'm tidying? :-)

> > 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 * ?

I'm not sure. The current implementation truncates base to 32 bits, so
I suppose there ought to be a check that base doesn't exceed
0xffffffff. But in this case the double-cast is still necessary to
avoid gcc's complaint about truncating while casting to a pointer.

> > ===================================================================
> > --- 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.

This alters the existing behavior, yes. But if the comments in the
code are to be believed, delayx(1) waits 100 usec, so for a 100-msec
delay we have to call delayx(100) 10 times.

--Ed




More information about the coreboot mailing list