On 21.04.2009 17:57, Patrick Georgi wrote: > coreboot-scanbuild.diff eliminates various issues brought up by > scan-build. > > Signed-off-by: Patrick Georgi <[email protected]>
Review follows. If you address my points (either explain why I'm wrong or fix), the patch is Acked-by: Carl-Daniel Hailfinger <[email protected]> > Index: src/devices/pci_rom.c > =================================================================== > --- src/devices/pci_rom.c (.../branches/upstream/coreboot-v2) > +++ src/devices/pci_rom.c (.../trunk/coreboot-v2) > @@ -42,7 +42,7 @@ > printk_debug("In cbfs, rom address for %s = %lx\n", > dev_path(dev), rom_address); > if (v) { > - dev->rom_address = v; > + dev->rom_address = (u32)v; > dev->on_mainboard = 1; > } > } > Index: src/devices/pci_device.c > =================================================================== > --- src/devices/pci_device.c (.../branches/upstream/coreboot-v2) > +++ src/devices/pci_device.c (.../trunk/coreboot-v2) > @@ -10,7 +10,7 @@ > * Copyright (C) 2004-2005 Li-Ta Lo <[email protected]> > * Copyright (C) 2005-2006 Tyan > * (Written by Yinghai Lu <[email protected]> for Tyan) > - * Copyright (C) 2005-2007 Stefan Reinauer <[email protected]> > + * Copyright (C) 2005-2009 coresystems GmbH > That's a bit unusual. I'd have expected the author name to remain: Copyright (C) 2005-2009 coresystems GmbH Written by Stefan Reinauer for coresystems GmbH > */ > > /* > @@ -271,7 +271,7 @@ > { > struct resource *resource; > unsigned long value; > - resource_t moving, limit; > + resource_t moving; > > if ((dev->on_mainboard) && (dev->rom_address == 0)) { > //skip it if rom_address is not set in MB Config.lb > @@ -296,8 +296,6 @@ > * - Limit is all of the bits that move plus all of the lower bits. > * See PCI Spec 6.2.5.1 ... > */ > - limit = 0; > - > if (moving) { > resource->size = 1; > resource->align = resource->gran = 0; > @@ -306,7 +304,7 @@ > resource->align += 1; > resource->gran += 1; > } > - resource->limit = limit = moving | (resource->size - 1); > + resource->limit = moving | (resource->size - 1); > } > > if (moving == 0) { > @@ -927,7 +925,7 @@ > if ( (id == 0xffffffff) || (id == 0x00000000) || > (id == 0x0000ffff) || (id == 0xffff0000)) > { > - printk_spew("%s, bad id 0x%x\n", dev_path(&dummy), id); > + // printk_spew("PCI: devfn 0x%x, bad id 0x%x\n", devfn, > id); > That change seems unrelated. Please revert. > return NULL; > } > dev = alloc_dev(bus, &dummy.path); > Index: src/include/console/console.h > =================================================================== > --- src/include/console/console.h (.../branches/upstream/coreboot-v2) > +++ src/include/console/console.h (.../trunk/coreboot-v2) > @@ -10,7 +10,7 @@ > unsigned char console_rx_byte(void); > int console_tst_byte(void); > void post_code(uint8_t value); > -void die(const char *msg); > +void __attribute__ ((noreturn)) die(const char *msg); > IIRC I sent that one ages ago. Definitely something we want. > > struct console_driver { > void (*init)(void); > Index: src/console/usbdebug_direct_console.c > =================================================================== > --- src/console/usbdebug_direct_console.c > (.../branches/upstream/coreboot-v2) > +++ src/console/usbdebug_direct_console.c (.../trunk/coreboot-v2) > @@ -1,3 +1,4 @@ > +#include <string.h> > #include <console/console.h> > #include <usbdebug_direct.h> > #include <pc80/mc146818rtc.h> > Index: src/boot/elfboot.c > =================================================================== > --- src/boot/elfboot.c (.../branches/upstream/coreboot-v2) > +++ src/boot/elfboot.c (.../trunk/coreboot-v2) > @@ -362,9 +362,6 @@ > seg->phdr_next->phdr_prev = new; > seg->phdr_next = new; > > - /* compute the new value of end */ > - end = start + len; > - > printk_spew(" late: [0x%016lx, 0x%016lx, 0x%016lx)\n", > new->s_addr, > new->s_addr + new->s_filesz, > Index: src/arch/i386/boot/coreboot_table.c > =================================================================== > --- src/arch/i386/boot/coreboot_table.c > (.../branches/upstream/coreboot-v2) > +++ src/arch/i386/boot/coreboot_table.c (.../trunk/coreboot-v2) > @@ -93,9 +93,8 @@ > > void add_console(struct lb_header *header, u16 consoletype) > { > - struct lb_record *rec; > struct lb_console *console; > - rec = lb_new_record(header); > I believe this is incorrect. The removed call modified *header and that change is now missing. > + > console = (struct lb_console *)lb_new_record(header); > console->tag = LB_TAG_CONSOLE; > console->size = sizeof(*console); > Index: util/nrv2b/nrv2b.c > =================================================================== > --- util/nrv2b/nrv2b.c (.../branches/upstream/coreboot-v2) > +++ util/nrv2b/nrv2b.c (.../trunk/coreboot-v2) > @@ -65,7 +65,7 @@ > #define BITSIZE 32 > #endif > > -static __inline__ void Error(char *message) > +static __inline__ __attribute__((noreturn)) void Error(char *message) > { > Fprintf((stderr, "\n%s\n", message)); > exit(EXIT_FAILURE); > Index: util/buildrom/buildrom.c > =================================================================== > --- util/buildrom/buildrom.c (.../branches/upstream/coreboot-v2) > +++ util/buildrom/buildrom.c (.../trunk/coreboot-v2) > @@ -24,7 +24,7 @@ > exit(1); > } > > -void fatal(char *s) > +void __attribute__((noreturn)) fatal(char *s) > { > perror(s); > exit(2); > > > Regards, Carl-Daniel -- http://www.hailfinger.org/ -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
