Re: [PATCH] x86: split e820 reserved entries record to late v4 - fix v7

Previous thread: Re: Linux 2.6.27-rc5 by Francois Romieu on Monday, September 1, 2008 - 12:12 am. (1 message)

Next thread: [PATCH] fs/gfs2: Use an IS_ERR test rather than a NULL test by Julien Brunel on Monday, September 1, 2008 - 1:51 am. (2 messages)
From: Yinghai Lu
Date: Monday, September 1, 2008 - 12:31 am

try to insert_resource second time, by expand the resource...

for case: e820 reserved entry is partially overlapped with bar res...

hope it will never happen

v2: according to Linus, add insert_resource_expand_to_fit, and change
	__insert_resource to static without lock

v3: use reserve_region_with_split() instead to hand overlapping
	with test case by extend 0xe0000000 - 0xeffffff to 0xdd800000 -
	get
		e0000000-efffffff : PCI MMCONFIG 0
			 e0000000-efffffff : reserved
	in /proc/iomem
	get
		found conflict for reserved [dd800000, efffffff], try to reserve with split
		    __reserve_region_with_split: (PCI Bus #80) [dd000000, ddffffff], res: (reserved) [dd800000, efffffff]
		    __reserve_region_with_split: (PCI Bus #00) [de000000, dfffffff], res: (reserved) [de000000, efffffff]
		initcall pci_subsys_init+0x0/0x121 returned 0 after 381 msecs
	in dmesg

v4: take out __insert_resource and insert_resource_expand_to_fit : Linus already check in.
    use reserve_region_with_split at the first point
    use const char *name

v5: fix checking on overlapping

v6: only reserve common area in conflict
	so got sth in /proc/iomem
		d8000000-dfffffff : PCI Bus #00
		  dc000000-dfffffff : GART
		    dc000000-dfffffff : reserved
		e0000000-efffffff : PCI MMCONFIG 0
		  e0000000-efffffff : reserved
	when we have big range in e820
		00000000dc000000-00000000f0000000 (reserved)

v7: remove wrong removing of write_unlock(&resource_lock) in adjust_resource()

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

---
 arch/x86/kernel/e820.c |    2 -
 include/linux/ioport.h |    3 ++
 kernel/resource.c      |   68 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)

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
@@ -1320,7 +1320,7 @@ void __init e820_reserve_resources_late(
 	res = e820_res;
 ...
From: Ingo Molnar
Date: Thursday, September 4, 2008 - 12:04 pm

i've applied the generic commit below to tip/core/resources, and merged 
tip/core/resources into tip/x86/core, and added the remaining e820.c 
oneliner change (attached below as well).

thanks Yinghai!

	Ingo

-------------------->
From 268364a0f48aee2f851f9d1ef8a6cda0f3039ef1 Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yhlu.kernel@gmail.com>
Date: Thu, 4 Sep 2008 21:02:44 +0200
Subject: [PATCH] IO resources: add reserve_region_with_split()

add reserve_region_with_split() to not lose e820 reserved entries if
they overlap with existing IO regions:

with test case by extend 0xe0000000 - 0xeffffff to 0xdd800000 -
we get:
	e0000000-efffffff : PCI MMCONFIG 0
		 e0000000-efffffff : reserved

and in /proc/iomem we get:
	found conflict for reserved [dd800000, efffffff], try to reserve with split
	    __reserve_region_with_split: (PCI Bus #80) [dd000000, ddffffff], res: (reserved) [dd800000, efffffff]
	    __reserve_region_with_split: (PCI Bus #00) [de000000, dfffffff], res: (reserved) [de000000, efffffff]
	initcall pci_subsys_init+0x0/0x121 returned 0 after 381 msecs
in dmesg

various fixes and improvements suggested by Linus.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/ioport.h |    3 ++
 kernel/resource.c      |   68 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8d3b7a9..fded376 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -108,6 +108,9 @@ extern struct resource iomem_resource;
 
 extern int request_resource(struct resource *root, struct resource *new);
 extern int release_resource(struct resource *new);
+extern void reserve_region_with_split(struct resource *root,
+			     resource_size_t start, resource_size_t end,
+			     const char *name);
 extern int insert_resource(struct resource *parent, struct resource *new);
 extern void ...
From: Andrew Morton
Date: Thursday, September 4, 2008 - 12:16 pm

On Thu, 4 Sep 2008 21:04:57 +0200

start and end have type resource_size_t.  Such types CANNOT be printed
unless cast to a known type.

Because there is a %s following an incorrect %lld, the above code will
crash the machine.

--

From: Yinghai Lu
Date: Thursday, September 4, 2008 - 12:22 pm

On Thu, Sep 4, 2008 at 12:16 PM, Andrew Morton

should just remove those lines.

YH
--

From: Tony Luck
Date: Monday, October 13, 2008 - 1:32 pm

This didn't happen.  These lines are still in the version that went into
Linus' tree over the weekend for 2.6.28.  On the ia64 build they spit
out a bunch of warnings:

kernel/resource.c:554: warning: format '%llx' expects type 'long long
unsigned int', but argument 3 has type 'resource_size_t'

Ditto for args 4, 6 and 7 on the same line.

-Tony
--

From: H. Peter Anvin
Date: Monday, October 13, 2008 - 2:14 pm

Here is a fix... currently running standard tests on it.

	-hpa

From: Luck, Tony
Date: Monday, October 13, 2008 - 2:17 pm

PiBIZXJlIGlzIGEgZml4Li4uIGN1cnJlbnRseSBydW5uaW5nIHN0YW5kYXJkIHRlc3RzIG9uIGl0
Lg0KDQpUaGF0IHNodXRzIHVwIHRoZSBjb21waWxlciBmb3IgdGhlIGlhNjQgYnVpbGQuDQoNClRo
YW5rcw0KDQotVG9ueQ0K
--

From: Linus Torvalds
Date: Monday, October 13, 2008 - 2:46 pm

Or we could do what Andrew suggested some time ago, and extend %p to do 
resource printing, like %pS and %pF.

TOTALLY UNTESTED! But something like this might allow

	printk(KERN_DEBUG "  reserve_region: (%s) %pR\n"
		res->name, res);

and if I did things right it should print

	reserve_region: (name) [xx-xx]

and maybe it's worth it. We certainly do seem to have a fair number of 
those irritating casts for resource printouts.

		Linus
---
 lib/vsprintf.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c399bc1..dd62557 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
+#include <linux/ioport.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -528,6 +529,21 @@ static char *symbol_string(char *buf, char *end, void *ptr, int field_width, int
 #endif
 }
 
+static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
+{
+	char sym[4*sizeof(resource_size_t) + 4];
+	char *p = sym, *pend = sym + sizeof(sym);
+
+	*p++ = '[';
+	p = number(p, pend, res->start, 16, -1, -1, 0);
+	*p++ = '-';
+	p = number(p, pend, res->end, 16, -1, -1, 0);
+	*p++ = ']';
+	*p = 0;
+
+	return string(buf, end, sym, field_width, precision, flags);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -549,6 +565,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		/* Fallthrough */
 	case 'S':
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
+	case 'R':
+		return resource_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {
--

From: H. Peter Anvin
Date: Monday, October 13, 2008 - 2:54 pm

There is certainly a lot to be said for this.  Using higher-level 
interfaces keeps formatting consistent and gives us one place to change 
things if desired.

	-hpa
--

From: Yinghai Lu
Date: Monday, October 13, 2008 - 3:40 pm

On Mon, Oct 13, 2008 at 2:46 PM, Linus Torvalds

can we have
       printk(KERN_DEBUG "  reserve_region: (%s) %04pR\n"
               res->name, res);
or
       printk(KERN_DEBUG "  reserve_region: (%s) %08pR\n"
               res->name, res);

because io port resource will only 4 digi only.

YH
--

From: H. Peter Anvin
Date: Monday, October 13, 2008 - 3:48 pm

That should be picked up from res->flags instead of being hardcoded in 
the format string.

	-hpa
--

From: Yinghai Lu
Date: Monday, October 13, 2008 - 10:29 pm

Ingo, how come following patch getting lost?

commit 1cf44baad76b6f20f95ece397c6f643320aa44c9
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Sep 4 21:26:06 2008 +0200

    IO resources: fix/remove printk

    Andrew Morton noticed that the printk in kernel/resource.c was buggy:

    | start and end have type resource_size_t.  Such types CANNOT be printed
    | unless cast to a known type.
    |
    | Because there is a %s following an incorrect %lld, the above code will
    | crash the machine.

    ... and it's probably quite unneeded as well, so remove it.

    Reported-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/kernel/resource.c b/kernel/resource.c
index 414d6fc..fc59dcc 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -549,13 +549,9 @@ static void __init
__reserve_region_with_split(struct resource *root,
        }

        if (!res) {
-               printk(KERN_DEBUG "    __reserve_region_with_split:
(%s) [%llx, %llx], res: (%s) [%llx, %llx]\n",
-                        conflict->name, conflict->start, conflict->end,
-                        name, start, end);
-
                /* failed, split and try again */

-               /* conflict coverred whole area */
+               /* conflict covered whole area */
                if (conflict->start <= start && conflict->end >= end)
                        return;
--

From: Yinghai Lu
Date: Friday, September 5, 2008 - 1:52 am

wonder if need to put

  x86: split e820 reserved entries record to late v4
  IO resources: add reserve_region_with_split()
  x86: split e820 reserved entries record to late, v7

into 2.6.27 for fix hang on David's system.

YH
--

From: Ingo Molnar
Date: Friday, September 5, 2008 - 4:45 am

too late i think. It hung on v2.6.26 too, right? So v2.6.28 would be 
more appropriate, and linux-next or tip/msater can be used as a 
practical kernel in the meanwhile.

	Ingo
--

Previous thread: Re: Linux 2.6.27-rc5 by Francois Romieu on Monday, September 1, 2008 - 12:12 am. (1 message)

Next thread: [PATCH] fs/gfs2: Use an IS_ERR test rather than a NULL test by Julien Brunel on Monday, September 1, 2008 - 1:51 am. (2 messages)