[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

-- 
http://www.hailfinger.org/