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

[Patrick Georgi]

Hi,

coreboot-scanbuild.diff eliminates various issues brought up by scan-build.

coreboot-loglevel.diff adds an "-l <num>" argument to abuild that sets 
the LOGLEVEL variables to the specified value.

coreboot-psd.diff adds a helper function to acpigen to create _PSD tables.

All of them are
Signed-off-by: Patrick Georgi

Regards,
Patrick

[Stefan Reinauer]

On 21.04.2009 17:57 Uhr, Patrick Georgi wrote:
> Hi,
>
> coreboot-scanbuild.diff eliminates various issues brought up by
> scan-build.
>
> coreboot-loglevel.diff adds an "-l <num>" argument to abuild that sets
> the LOGLEVEL variables to the specified value.
>
> coreboot-psd.diff adds a helper function to acpigen to create _PSD
> tables.
>
> All of them are
> Signed-off-by: Patrick Georgi

Acked-by: Stefan Reinauer

[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

Review follows.
If you address my points (either explain why I'm wrong or fix), the patch is
Acked-by: Carl-Daniel Hailfinger
> 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
> Copyright (C) 2005-2006 Tyan
> (Written by Yinghai Lu  for Tyan)
> Copyright (C) 2005-2007 Stefan Reinauer
> 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

[Patrick Georgi]

Am 21.04.2009 18:18, schrieb Carl-Daniel Hailfinger:
> On 21.04.2009 17:57, Patrick Georgi wrote:
> > Copyright (C) 2005-2007 Stefan Reinauer
> > 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

I'll add that.
> > @@ -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.

will do
> > @@ -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.

That header is an empty one, see the line below.
> > console = (struct lb_console *)lb_new_record(header);
> >  console->tag = LB_TAG_CONSOLE;
> >  console->size = sizeof(*console);

Patrick

[Carl-Daniel Hailfinger]

On 21.04.2009 17:57, Patrick Georgi wrote:
> coreboot-loglevel.diff adds an "-l <num>" argument to abuild that sets
> the LOGLEVEL variables to the specified value.

Why did you only change the boards having Config-abuild.lb? Other boards
can be compiled with abuild as well.
If you have an answer for this, the patch is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel