Re: [coreboot] [PATCH] Fix the NULL dev resource usage

[Carl-Daniel Hailfinger]

On 25.03.2009 23:17, Stefan Reinauer wrote:
> > Index: src/northbridge/amd/amdk8/northbridge.c
> >
> > --- src/northbridge/amd/amdk8/northbridge.c	(revision 4026)
> > +++ src/northbridge/amd/amdk8/northbridge.c	(working copy)
> > @@ -291,11 +291,12 @@
> >    unsigned nodeid, link;
> >    int result;
> >    res = 0;
> > for(nodeid = 0; !res && (nodeid < 8); nodeid++) {
> > for(nodeid = 0; !res && (nodeid < FX_DEVS); nodeid++) {
> > device_t dev;
> > dev = __f0_dev[nodeid];
> > for(link = 0; !res && (link < 3); link++) {
> > 	res = probe_resource(dev, 0x100 + (reg | link));
> > 	if (dev)
> > 		res = probe_resource(dev, 0x100 + (reg | link));
>
> Going through the loop and checking dev every time is a bit nasty. What
> about

I was about writing the same comment, but then I stumbled upon a
condition which should be watched. See below.
> for(nodeid = 0; !res && (nodeid < FX_DEVS); nodeid++) {
>    device_t dev = __f0_dev[nodeid];
>    if (!dev) continue;
>    for(link = 0; !res && (link < 3); link++) {
>    	res = probe_resource(dev, 0x100 + (reg | link));
>     ...

Is link accessed in read mode after the loop? If yes, this changes the
semantics.
I don't have access to the full source code right now, so I can't check.
> > }
> >
> > }
> >    result = 2;
> > @@ -760,19 +761,20 @@
> >    	mem_hole.hole_startk = HW_MEM_HOLE_SIZEK;
> >    	mem_hole.node_id = -1;
> > for (i = 0; i < 8; i++) {
> > for (i = 0; i < FX_DEVS; i++) {
> > uint32_t base;
> > uint32_t hole;
> > base  = f1_read_config32(0x40 + (i << 3));
> > if ((base & ((1<<1)|(1<<0))) != ((1<<1)|(1<<0))) {
> > 	continue;
> > }
> > 	hole = pci_read_config32(__f1_dev[i], 0xf0);
> > 	if(hole & 1) { // we find the hole
> > 		mem_hole.hole_startk = (hole & (0xff<<24)) >> 10;
> > 		mem_hole.node_id = i; // record the node No with hole
> > 		break; // only one hole
> > 	if (__f1_dev[i]) {
>
> Same as above. Just add another continue under the base check above.

Same here.
> > 		hole = pci_read_config32(__f1_dev[i], 0xf0);
> > 		if(hole & 1) { // we find the hole
> > 			mem_hole.hole_startk = (hole & (0xff<<24)) >> 10;
> > 			mem_hole.node_id = i; // record the node No with hole
> > 			break; // only one hole
> > 		}
> > }
> > }
> >
> > @@ -834,15 +836,15 @@
> >    limit = f1_read_config32(0x44 + (i << 3));
> >    f1_write_config32(0x44 + (i << 3),limit - (hole_sizek << 2));
> >    dev = __f1_dev[i];
> > hoist = pci_read_config32(dev, 0xf0);
> > if(hoist & 1) {
> > pci_write_config32(dev, 0xf0, 0);
> > if (dev) {
>
> And the same as above.

Same here.
> > hoist = pci_read_config32(dev, 0xf0);
> > if(hoist & 1) {
> > 	pci_write_config32(dev, 0xf0, 0);
> > } else {
> > 	base = pci_read_config32(dev, 0x40 + (i << 3));
> > 	f1_write_config32(0x40 + (i << 3),base - (hole_sizek << 2));
> > }
> > }
> > else {
> > base = pci_read_config32(dev, 0x40 + (i << 3));
> > f1_write_config32(0x40 + (i << 3),base - (hole_sizek << 2));
> > }
> >
> > }
>
> ...
> > Index: src/northbridge/amd/amdfam10/northbridge.c
> >
> > --- src/northbridge/amd/amdfam10/northbridge.c	(revision 4026)
> > +++ src/northbridge/amd/amdfam10/northbridge.c	(working copy)
> > @@ -339,7 +339,8 @@
> >    	device_t dev;
> >    	dev = __f0_dev[nodeid];
> >    	for(link = 0; !res && (link < 8); link++) {
> > 	res = probe_resource(dev, 0x1000 + reg + (link<<16)); // 8 links, 0x1000 man f1,
> > 	if (dev)
> > 		res = probe_resource(dev, 0x1000 + reg + (link<<16)); // 8 links, 0x1000 man f1,
>
> And the same.

Same here.
> > }
> >
> > }
> >    result = 2;

Regards,
Carl-Daniel