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

[Carl-Daniel Hailfinger]

Ed Swierk wrote:
> 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?

For functions like strlen, we have no choice but to use naked char.
So yes, my complaint was superfluous in this case.

>> 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? :-)

Yes. Sacrificing a chicken would be nice, too.

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

Agreed.

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

The code says something different.

static void delayx(uint8_t value) {
        int i;
        for(i=0;i<0x8000;i++) {
                outb(value, 0x80);
        }
}

The value is the POST code and has nothing to do with the delay.
So your patch changed the POST code from E8 (1000 mod 256) to 64 (100)
and called the delay function ten times.

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/