diff mbox

x86/pci: make pci_mem_start to be aligned only -v4

Message ID alpine.LFD.2.00.0904181142530.4042@localhost.localdomain (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Linus Torvalds April 18, 2009, 6:50 p.m. UTC 
On Sat, 18 Apr 2009, Yinghai Lu wrote:
> 
> except need to change 
> > +		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
> ==> > +		reserve_region_with_split(&iomem_resource, start, end - 1, "RAM buffer");

Yes, I sent out a later email pointing that out.

> it will make sure dynmical allocating code will not use those range.
> 
> and could make e820_setup_gap much simple.

ACK. In fact:

> Index: linux-2.6/arch/x86/kernel/e820.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/e820.c
> +++ linux-2.6/arch/x86/kernel/e820.c
> @@ -635,14 +635,12 @@ __init void e820_setup_gap(void)
>  #endif
>  
>  	/*
> -	 * See how much we want to round up: start off with
> -	 * rounding to the next 1MB area.
> +	 * e820_reserve_resources_late will protect stolen RAM
> +	 * so just round it to 1M
>  	 */
>  	round = 0x100000;
> -	while ((gapsize >> 4) > round)
> -		round += round;
> -	/* Fun with two's complement */
> -	pci_mem_start = (gapstart + round) & -round;
> +
> +	pci_mem_start = roundup(gapstart, round);

You can just remove "round" entirely. It's no longer a variable, it's just 
an odd way of saying 1M ;)

> Ingo, can you put those two patches in tip?

I would suggest that we first change "reserve_region_with_split()" to not 
recurse into the region.

That function isn't used by anything else (we ended up using 
"expand_to_fit()" instead in the one place that migth have used it), and 
now th eone caller we do have would not want the recursion - if there 
already exists a resource at the top level, we want to just avoid it.

This - again TOTALLY UNTESTED - patch removes the "recurse into conflicts" 
code. Comments? Testing?

			Linus
---
 kernel/resource.c |   46 ++++++++++++----------------------------------
 1 files changed, 12 insertions(+), 34 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yinghai Lu April 18, 2009, 10:44 p.m. UTC | #1 
Linus Torvalds wrote:
> 
> On Sat, 18 Apr 2009, Yinghai Lu wrote:
>> except need to change 
>>> +		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
>> ==> > +		reserve_region_with_split(&iomem_resource, start, end - 1, "RAM buffer");
> 
> Yes, I sent out a later email pointing that out.
> 
>> it will make sure dynmical allocating code will not use those range.
>>
>> and could make e820_setup_gap much simple.
> 
> ACK. In fact:
> 
>> Index: linux-2.6/arch/x86/kernel/e820.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/e820.c
>> +++ linux-2.6/arch/x86/kernel/e820.c
>> @@ -635,14 +635,12 @@ __init void e820_setup_gap(void)
>>  #endif
>>  
>>  	/*
>> -	 * See how much we want to round up: start off with
>> -	 * rounding to the next 1MB area.
>> +	 * e820_reserve_resources_late will protect stolen RAM
>> +	 * so just round it to 1M
>>  	 */
>>  	round = 0x100000;
>> -	while ((gapsize >> 4) > round)
>> -		round += round;
>> -	/* Fun with two's complement */
>> -	pci_mem_start = (gapstart + round) & -round;
>> +
>> +	pci_mem_start = roundup(gapstart, round);
> 
> You can just remove "round" entirely. It's no longer a variable, it's just 
> an odd way of saying 1M ;)
> 
>> Ingo, can you put those two patches in tip?
> 
> I would suggest that we first change "reserve_region_with_split()" to not 
> recurse into the region.
> 
> That function isn't used by anything else (we ended up using 
> "expand_to_fit()" instead in the one place that migth have used it), and 
> now th eone caller we do have would not want the recursion - if there 
> already exists a resource at the top level, we want to just avoid it.
> 
> This - again TOTALLY UNTESTED - patch removes the "recurse into conflicts" 
> code. Comments? Testing?
> 
> 			Linus
> ---
>  kernel/resource.c |   46 ++++++++++++----------------------------------
>  1 files changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index fd5d7d5..ac5f3a3 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -533,43 +533,21 @@ static void __init __reserve_region_with_split(struct resource *root,
>  	res->end = end;
>  	res->flags = IORESOURCE_BUSY;
>  
> -	for (;;) {
> -		conflict = __request_resource(parent, res);
> -		if (!conflict)
> -			break;
> -		if (conflict != parent) {
> -			parent = conflict;
> -			if (!(conflict->flags & IORESOURCE_BUSY))
> -				continue;
> -		}
> -
> -		/* Uhhuh, that didn't work out.. */
> -		kfree(res);
> -		res = NULL;
> -		break;
> -	}
> -
> -	if (!res) {
> -		/* failed, split and try again */
> -
> -		/* conflict covered whole area */
> -		if (conflict->start <= start && conflict->end >= end)
> -			return;
> +	conflict = __request_resource(parent, res);
> +	if (!conflict)
> +		return;
>  
> -		if (conflict->start > start)
> -			__reserve_region_with_split(root, start, conflict->start-1, name);
> -		if (!(conflict->flags & IORESOURCE_BUSY)) {
> -			resource_size_t common_start, common_end;
> +	/* failed, split and try again */
> +	kfree(res);
>  
> -			common_start = max(conflict->start, start);
> -			common_end = min(conflict->end, end);
> -			if (common_start < common_end)
> -				__reserve_region_with_split(root, common_start, common_end, name);
> -		}
> -		if (conflict->end < end)
> -			__reserve_region_with_split(root, conflict->end+1, end, name);
> -	}
> +	/* conflict covered whole area */
> +	if (conflict->start <= start && conflict->end >= end)
> +		return;
>  
> +	if (conflict->start > start)
> +		__reserve_region_with_split(root, start, conflict->start-1, name);
> +	if (conflict->end < end)
> +		__reserve_region_with_split(root, conflict->end+1, end, name);
>  }
>  
>  void __init reserve_region_with_split(struct resource *root,

with
[    0.000000] BIOS-provided physical RAM map:
[    0.000000]  BIOS-e820: 0000000000000100 - 0000000000097400 (usable)
[    0.000000]  BIOS-e820: 0000000000097400 - 00000000000a0000 (reserved)
[    0.000000]  BIOS-e820: 0000000000100000 - 00000000b7fa0000 (usable)
[    0.000000]  BIOS-e820: 00000000b7fae000 - 00000000b7fb0000 (usable)
[    0.000000]  BIOS-e820: 00000000b7fb0000 - 00000000b7fbe000 (ACPI data)
[    0.000000]  BIOS-e820: 00000000b7fbe000 - 00000000b7ff0000 (ACPI NVS)
[    0.000000]  BIOS-e820: 00000000b7ff0000 - 00000000b8000000 (reserved)
[    0.000000]  BIOS-e820: 00000000e0000000 - 00000000f0000000 (reserved)
[    0.000000]  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
[    0.000000]  BIOS-e820: 00000000fee00000 - 00000000fef00000 (reserved)
[    0.000000]  BIOS-e820: 00000000ff700000 - 0000000100000000 (reserved)
[    0.000000]  BIOS-e820: 0000000100000000 - 0000002048000000 (usable)

got

00000100-000973ff : System RAM
00097400-0009ffff : reserved
000a0000-000bffff : PCI Bus #00
000c0000-000cffff : pnp 00:0c
000e0000-000fffff : pnp 00:0c
00100000-b7f9ffff : System RAM
  00200000-00c68f6b : Kernel code
  00c68f6c-01332f7f : Kernel data
  015a6000-01fcaa57 : Kernel bss
  20000000-23ffffff : GART
b7fa0000-b7fadfff : RAM buffer
b7fae000-b7faffff : System RAM
b7fb0000-b7fbdfff : ACPI Tables
b7fbe000-b7feffff : ACPI Non-volatile Storage
b7ff0000-b7ffffff : reserved
...

YH
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu April 18, 2009, 11:01 p.m. UTC | #2 
Yinghai Lu wrote:
> Linus Torvalds wrote:
>> On Sat, 18 Apr 2009, Yinghai Lu wrote:
>>> except need to change 
>>>> +		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
>>> ==> > +		reserve_region_with_split(&iomem_resource, start, end - 1, "RAM buffer");
>> Yes, I sent out a later email pointing that out.
>>
>>> it will make sure dynmical allocating code will not use those range.
>>>
>>> and could make e820_setup_gap much simple.
>> ACK. In fact:
>>
>>> Index: linux-2.6/arch/x86/kernel/e820.c
>>> ===================================================================
>>> --- linux-2.6.orig/arch/x86/kernel/e820.c
>>> +++ linux-2.6/arch/x86/kernel/e820.c
>>> @@ -635,14 +635,12 @@ __init void e820_setup_gap(void)
>>>  #endif
>>>  
>>>  	/*
>>> -	 * See how much we want to round up: start off with
>>> -	 * rounding to the next 1MB area.
>>> +	 * e820_reserve_resources_late will protect stolen RAM
>>> +	 * so just round it to 1M
>>>  	 */
>>>  	round = 0x100000;
>>> -	while ((gapsize >> 4) > round)
>>> -		round += round;
>>> -	/* Fun with two's complement */
>>> -	pci_mem_start = (gapstart + round) & -round;
>>> +
>>> +	pci_mem_start = roundup(gapstart, round);
>> You can just remove "round" entirely. It's no longer a variable, it's just 
>> an odd way of saying 1M ;)
>>
>>> Ingo, can you put those two patches in tip?
>> I would suggest that we first change "reserve_region_with_split()" to not 
>> recurse into the region.
>>
>> That function isn't used by anything else (we ended up using 
>> "expand_to_fit()" instead in the one place that migth have used it), and 
>> now th eone caller we do have would not want the recursion - if there 
>> already exists a resource at the top level, we want to just avoid it.
>>
>> This - again TOTALLY UNTESTED - patch removes the "recurse into conflicts" 
>> code. Comments? Testing?
>>
>> 			Linus
>> ---
>>  kernel/resource.c |   46 ++++++++++++----------------------------------
>>  1 files changed, 12 insertions(+), 34 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index fd5d7d5..ac5f3a3 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -533,43 +533,21 @@ static void __init __reserve_region_with_split(struct resource *root,
>>  	res->end = end;
>>  	res->flags = IORESOURCE_BUSY;
>>  
>> -	for (;;) {
>> -		conflict = __request_resource(parent, res);
>> -		if (!conflict)
>> -			break;
>> -		if (conflict != parent) {
>> -			parent = conflict;
>> -			if (!(conflict->flags & IORESOURCE_BUSY))
>> -				continue;
>> -		}
>> -
>> -		/* Uhhuh, that didn't work out.. */
>> -		kfree(res);
>> -		res = NULL;
>> -		break;
>> -	}
>> -
>> -	if (!res) {
>> -		/* failed, split and try again */
>> -
>> -		/* conflict covered whole area */
>> -		if (conflict->start <= start && conflict->end >= end)
>> -			return;
>> +	conflict = __request_resource(parent, res);
>> +	if (!conflict)
>> +		return;
>>  
>> -		if (conflict->start > start)
>> -			__reserve_region_with_split(root, start, conflict->start-1, name);
>> -		if (!(conflict->flags & IORESOURCE_BUSY)) {
>> -			resource_size_t common_start, common_end;
>> +	/* failed, split and try again */
>> +	kfree(res);
>>  
>> -			common_start = max(conflict->start, start);
>> -			common_end = min(conflict->end, end);
>> -			if (common_start < common_end)
>> -				__reserve_region_with_split(root, common_start, common_end, name);
>> -		}
>> -		if (conflict->end < end)
>> -			__reserve_region_with_split(root, conflict->end+1, end, name);
>> -	}
>> +	/* conflict covered whole area */
>> +	if (conflict->start <= start && conflict->end >= end)
>> +		return;
>>  
>> +	if (conflict->start > start)
>> +		__reserve_region_with_split(root, start, conflict->start-1, name);
>> +	if (conflict->end < end)
>> +		__reserve_region_with_split(root, conflict->end+1, end, name);
>>  }
>>  
>>  void __init reserve_region_with_split(struct resource *root,
> 
> with
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  BIOS-e820: 0000000000000100 - 0000000000097400 (usable)
> [    0.000000]  BIOS-e820: 0000000000097400 - 00000000000a0000 (reserved)
> [    0.000000]  BIOS-e820: 0000000000100000 - 00000000b7fa0000 (usable)
> [    0.000000]  BIOS-e820: 00000000b7fae000 - 00000000b7fb0000 (usable)
> [    0.000000]  BIOS-e820: 00000000b7fb0000 - 00000000b7fbe000 (ACPI data)
> [    0.000000]  BIOS-e820: 00000000b7fbe000 - 00000000b7ff0000 (ACPI NVS)
> [    0.000000]  BIOS-e820: 00000000b7ff0000 - 00000000b8000000 (reserved)
> [    0.000000]  BIOS-e820: 00000000e0000000 - 00000000f0000000 (reserved)
> [    0.000000]  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
> [    0.000000]  BIOS-e820: 00000000fee00000 - 00000000fef00000 (reserved)
> [    0.000000]  BIOS-e820: 00000000ff700000 - 0000000100000000 (reserved)
> [    0.000000]  BIOS-e820: 0000000100000000 - 0000002048000000 (usable)
> 
> got
> 
> 00000100-000973ff : System RAM
> 00097400-0009ffff : reserved
> 000a0000-000bffff : PCI Bus #00
> 000c0000-000cffff : pnp 00:0c
> 000e0000-000fffff : pnp 00:0c
> 00100000-b7f9ffff : System RAM
>   00200000-00c68f6b : Kernel code
>   00c68f6c-01332f7f : Kernel data
>   015a6000-01fcaa57 : Kernel bss
>   20000000-23ffffff : GART
> b7fa0000-b7fadfff : RAM buffer
> b7fae000-b7faffff : System RAM
> b7fb0000-b7fbdfff : ACPI Tables
> b7fbe000-b7feffff : ACPI Non-volatile Storage
> b7ff0000-b7ffffff : reserved
> ...
> 
without your patch got
00000100-000973ff : System RAM
00097400-0009ffff : reserved
000a0000-000bffff : PCI Bus #00
000c0000-000cffff : pnp 00:0c
000e0000-000fffff : pnp 00:0c
00100000-b7f9ffff : System RAM
  00200000-00c68f6b : Kernel code
  00c68f6c-01332f7f : Kernel data
  015a6000-01fcaa57 : Kernel bss
  20000000-23ffffff : GART
b7fa0000-b7fadfff : RAM buffer
b7fae000-b7faffff : System RAM
b7fb0000-b7fbdfff : ACPI Tables
b7fbe000-b7feffff : ACPI Non-volatile Storage
b7ff0000-b7ffffff : reserved
  b7ff0000-b7ffffff : RAM buffer
b8000000-beffffff : PCI Bus #00
bf000000-bfffffff : PCI Bus #80

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds April 18, 2009, 11:04 p.m. UTC | #3 
On Sat, 18 Apr 2009, Yinghai Lu wrote:
> 
> with
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  BIOS-e820: 0000000000000100 - 0000000000097400 (usable)
> [    0.000000]  BIOS-e820: 0000000000097400 - 00000000000a0000 (reserved)
> [    0.000000]  BIOS-e820: 0000000000100000 - 00000000b7fa0000 (usable)
> [    0.000000]  BIOS-e820: 00000000b7fae000 - 00000000b7fb0000 (usable)
> [    0.000000]  BIOS-e820: 00000000b7fb0000 - 00000000b7fbe000 (ACPI data)
> [    0.000000]  BIOS-e820: 00000000b7fbe000 - 00000000b7ff0000 (ACPI NVS)
> [    0.000000]  BIOS-e820: 00000000b7ff0000 - 00000000b8000000 (reserved)
> [    0.000000]  BIOS-e820: 00000000e0000000 - 00000000f0000000 (reserved)
> [    0.000000]  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
> [    0.000000]  BIOS-e820: 00000000fee00000 - 00000000fef00000 (reserved)
> [    0.000000]  BIOS-e820: 00000000ff700000 - 0000000100000000 (reserved)
> [    0.000000]  BIOS-e820: 0000000100000000 - 0000002048000000 (usable)
> 
> got
> 
> 00000100-000973ff : System RAM
> 00097400-0009ffff : reserved
> 000a0000-000bffff : PCI Bus #00
> 000c0000-000cffff : pnp 00:0c
> 000e0000-000fffff : pnp 00:0c
> 00100000-b7f9ffff : System RAM
>   00200000-00c68f6b : Kernel code
>   00c68f6c-01332f7f : Kernel data
>   015a6000-01fcaa57 : Kernel bss
>   20000000-23ffffff : GART
> b7fa0000-b7fadfff : RAM buffer
> b7fae000-b7faffff : System RAM
> b7fb0000-b7fbdfff : ACPI Tables
> b7fbe000-b7feffff : ACPI Non-volatile Storage
> b7ff0000-b7ffffff : reserved

Hmm. That looks correct to me. We filled in that odd area between 
b7fa0000-b7fadfff that went unmentioned in the e820 tables.

And that _is_ a really odd hole. I wonder what it is all about. But the 
approach does seem to have done the right thing.

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds April 18, 2009, 11:06 p.m. UTC | #4 
On Sat, 18 Apr 2009, Yinghai Lu wrote:
>
> without your patch got
>
> 00000100-000973ff : System RAM
> 00097400-0009ffff : reserved
> 000a0000-000bffff : PCI Bus #00
> 000c0000-000cffff : pnp 00:0c
> 000e0000-000fffff : pnp 00:0c
> 00100000-b7f9ffff : System RAM
>   00200000-00c68f6b : Kernel code
>   00c68f6c-01332f7f : Kernel data
>   015a6000-01fcaa57 : Kernel bss
>   20000000-23ffffff : GART
> b7fa0000-b7fadfff : RAM buffer
> b7fae000-b7faffff : System RAM
> b7fb0000-b7fbdfff : ACPI Tables
> b7fbe000-b7feffff : ACPI Non-volatile Storage
> b7ff0000-b7ffffff : reserved
>   b7ff0000-b7ffffff : RAM buffer
> b8000000-beffffff : PCI Bus #00
> bf000000-bfffffff : PCI Bus #80

Yeah, that "RAM buffer" inside the b7ff0000-b7ffffff reserved area is 
obviously crap.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu April 18, 2009, 11:26 p.m. UTC | #5 
Linus Torvalds wrote:
> 
> On Sat, 18 Apr 2009, Yinghai Lu wrote:
>> without your patch got
>>
>> 00000100-000973ff : System RAM
>> 00097400-0009ffff : reserved
>> 000a0000-000bffff : PCI Bus #00
>> 000c0000-000cffff : pnp 00:0c
>> 000e0000-000fffff : pnp 00:0c
>> 00100000-b7f9ffff : System RAM
>>   00200000-00c68f6b : Kernel code
>>   00c68f6c-01332f7f : Kernel data
>>   015a6000-01fcaa57 : Kernel bss
>>   20000000-23ffffff : GART
>> b7fa0000-b7fadfff : RAM buffer
>> b7fae000-b7faffff : System RAM
>> b7fb0000-b7fbdfff : ACPI Tables
>> b7fbe000-b7feffff : ACPI Non-volatile Storage
>> b7ff0000-b7ffffff : reserved
>>   b7ff0000-b7ffffff : RAM buffer
>> b8000000-beffffff : PCI Bus #00
>> bf000000-bfffffff : PCI Bus #80
> 
> Yeah, that "RAM buffer" inside the b7ff0000-b7ffffff reserved area is 
> obviously crap.

reserved from 0xb7ff0000 - 0xffffffff ranges are registered via e820_reserve_resources_late
didn't have IORESOURCE_BUSY

but RAM buffer area has that IORESOURCE_BUSY

YH
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu April 18, 2009, 11:30 p.m. UTC | #6 
Yinghai Lu wrote:
> Linus Torvalds wrote:
>> On Sat, 18 Apr 2009, Yinghai Lu wrote:
>>> without your patch got
>>>
>>> 00000100-000973ff : System RAM
>>> 00097400-0009ffff : reserved
>>> 000a0000-000bffff : PCI Bus #00
>>> 000c0000-000cffff : pnp 00:0c
>>> 000e0000-000fffff : pnp 00:0c
>>> 00100000-b7f9ffff : System RAM
>>>   00200000-00c68f6b : Kernel code
>>>   00c68f6c-01332f7f : Kernel data
>>>   015a6000-01fcaa57 : Kernel bss
>>>   20000000-23ffffff : GART
>>> b7fa0000-b7fadfff : RAM buffer
>>> b7fae000-b7faffff : System RAM
>>> b7fb0000-b7fbdfff : ACPI Tables
>>> b7fbe000-b7feffff : ACPI Non-volatile Storage
>>> b7ff0000-b7ffffff : reserved
>>>   b7ff0000-b7ffffff : RAM buffer
>>> b8000000-beffffff : PCI Bus #00
>>> bf000000-bfffffff : PCI Bus #80
>> Yeah, that "RAM buffer" inside the b7ff0000-b7ffffff reserved area is 
>> obviously crap.
> 
> reserved from 0xb7ff0000 - 0xffffffff ranges are registered via e820_reserve_resources_late
> didn't have IORESOURCE_BUSY
> 
> but RAM buffer area has that IORESOURCE_BUSY
> 
aka those "reserved" is not really reserved.

YH
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
H. Peter Anvin April 19, 2009, 12:32 a.m. UTC | #7 
Linus Torvalds wrote:
>>
>> 00000100-000973ff : System RAM
>> 00097400-0009ffff : reserved
>> 000a0000-000bffff : PCI Bus #00
>> 000c0000-000cffff : pnp 00:0c
>> 000e0000-000fffff : pnp 00:0c
>> 00100000-b7f9ffff : System RAM
>>   00200000-00c68f6b : Kernel code
>>   00c68f6c-01332f7f : Kernel data
>>   015a6000-01fcaa57 : Kernel bss
>>   20000000-23ffffff : GART
>> b7fa0000-b7fadfff : RAM buffer
>> b7fae000-b7faffff : System RAM
>> b7fb0000-b7fbdfff : ACPI Tables
>> b7fbe000-b7feffff : ACPI Non-volatile Storage
>> b7ff0000-b7ffffff : reserved
> 
> Hmm. That looks correct to me. We filled in that odd area between 
> b7fa0000-b7fadfff that went unmentioned in the e820 tables.
> 
> And that _is_ a really odd hole. I wonder what it is all about. But the 
> approach does seem to have done the right thing.
> 

Looks to me as through the BIOS rounded the end of available memory to a
64K boundary after subtracting the ACPI storage.  That might have been
done to work around an OS loader bug somewhere.

	-hpa
Linus Torvalds April 19, 2009, 4:50 a.m. UTC | #8 
On Sat, 18 Apr 2009, Linus Torvalds wrote:
> 
> And that _is_ a really odd hole. I wonder what it is all about. But the 
> approach does seem to have done the right thing.

I'll commit the reserve_region_with_split() change. There are no actual 
users of it now, so committing that change doesn't really do anything, but 
I like removing code, and with the only current potential user actively 
wanting just the simpler behavior, why keep the code around?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar April 19, 2009, 9:02 a.m. UTC | #9 
* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 18 Apr 2009, Linus Torvalds wrote:
> > 
> > And that _is_ a really odd hole. I wonder what it is all about. 
> > But the approach does seem to have done the right thing.
> 
> I'll commit the reserve_region_with_split() change. There are no 
> actual users of it now, so committing that change doesn't really 
> do anything, but I like removing code, and with the only current 
> potential user actively wanting just the simpler behavior, why 
> keep the code around?

Cool! Yinghai, mind (re-)sending the latest version of the remaining 
two patches, so what we can pick this up into the x86 tree and get 
it tested? I'd say it's for v2.6.31. (unless someone can think of a 
strong reason to do this sooner.)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar April 19, 2009, 9:06 a.m. UTC | #10 
* Ingo Molnar <mingo@elte.hu> wrote:

> Cool! Yinghai, mind (re-)sending the latest version of the 
> remaining two patches, so what we can pick this up into the x86 
> tree and get it tested? I'd say it's for v2.6.31. (unless someone 
> can think of a strong reason to do this sooner.)

Hm, there's one patch in that lot that does:

 drivers/pci/bus.c	 |    8 +++++++-
 drivers/pci/probe.c     |    8 ++++++--
 drivers/pci/setup-bus.c |   40 +++++++++++++++++++++++++++++++---------
 3 files changed, 44 insertions(+), 12 deletions(-)

Which should go via the PCI tree.

I can set up an isolated x86/pci-gap topic that i'll send to Jesse 
to pull (once it looks to be stable), as the other patches modify 
the e820 code which we'd like to test in the x86 tree first.

Jesse, Linus, Yinghai, does that look like a good plan to you?

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Barnes April 19, 2009, 5:52 p.m. UTC | #11 
On Sun, 19 Apr 2009 11:06:15 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > Cool! Yinghai, mind (re-)sending the latest version of the 
> > remaining two patches, so what we can pick this up into the x86 
> > tree and get it tested? I'd say it's for v2.6.31. (unless someone 
> > can think of a strong reason to do this sooner.)
> 
> Hm, there's one patch in that lot that does:
> 
>  drivers/pci/bus.c	 |    8 +++++++-
>  drivers/pci/probe.c     |    8 ++++++--
>  drivers/pci/setup-bus.c |   40
> +++++++++++++++++++++++++++++++--------- 3 files changed, 44
> insertions(+), 12 deletions(-)
> 
> Which should go via the PCI tree.
> 
> I can set up an isolated x86/pci-gap topic that i'll send to Jesse 
> to pull (once it looks to be stable), as the other patches modify 
> the e820 code which we'd like to test in the x86 tree first.
> 
> Jesse, Linus, Yinghai, does that look like a good plan to you?

Yep, that's fine with me.
diff mbox

Patch

diff --git a/kernel/resource.c b/kernel/resource.c
index fd5d7d5..ac5f3a3 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -533,43 +533,21 @@  static void __init __reserve_region_with_split(struct resource *root,
 	res->end = end;
 	res->flags = IORESOURCE_BUSY;
 
-	for (;;) {
-		conflict = __request_resource(parent, res);
-		if (!conflict)
-			break;
-		if (conflict != parent) {
-			parent = conflict;
-			if (!(conflict->flags & IORESOURCE_BUSY))
-				continue;
-		}
-
-		/* Uhhuh, that didn't work out.. */
-		kfree(res);
-		res = NULL;
-		break;
-	}
-
-	if (!res) {
-		/* failed, split and try again */
-
-		/* conflict covered whole area */
-		if (conflict->start <= start && conflict->end >= end)
-			return;
+	conflict = __request_resource(parent, res);
+	if (!conflict)
+		return;
 
-		if (conflict->start > start)
-			__reserve_region_with_split(root, start, conflict->start-1, name);
-		if (!(conflict->flags & IORESOURCE_BUSY)) {
-			resource_size_t common_start, common_end;
+	/* failed, split and try again */
+	kfree(res);
 
-			common_start = max(conflict->start, start);
-			common_end = min(conflict->end, end);
-			if (common_start < common_end)
-				__reserve_region_with_split(root, common_start, common_end, name);
-		}
-		if (conflict->end < end)
-			__reserve_region_with_split(root, conflict->end+1, end, name);
-	}
+	/* conflict covered whole area */
+	if (conflict->start <= start && conflict->end >= end)
+		return;
 
+	if (conflict->start > start)
+		__reserve_region_with_split(root, start, conflict->start-1, name);
+	if (conflict->end < end)
+		__reserve_region_with_split(root, conflict->end+1, end, name);
 }
 
 void __init reserve_region_with_split(struct resource *root,