Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4

[Linus Torvalds <torvalds@linux-foundation.org>]

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,