[coreboot] [PATCH]es: abuild, acpigen, scan-build results

[Carl-Daniel Hailfinger]

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 <patrick.georgi at coresystems.de>

Review follows.
If you address my points (either explain why I'm wrong or fix), the patch is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

> 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 <ollie at lanl.gov>
>   * Copyright (C) 2005-2006 Tyan
>   * (Written by Yinghai Lu <yhlu at tyan.com> for Tyan)
> - * Copyright (C) 2005-2007 Stefan Reinauer <stepan at openbios.org>
> + * 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/