[PATCH] resource/x86: add sticky resource type

Previous thread: sparc build failure by Stephen Rothwell on Wednesday, August 27, 2008 - 6:46 pm. (4 messages)

Next thread: [PATCH 1/2] anondev: init IDR statically by Alexey Dobriyan on Wednesday, August 27, 2008 - 7:25 pm. (1 message)
From: Yinghai Lu
Date: Wednesday, August 27, 2008 - 6:56 pm

Ingo suggest to use sticky resource type to record fixed resource.
so could check that BAR and avoid update it after request_resource failed.

and we could remove tricky code about insert some resource with late_initcall.

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

---
 arch/x86/kernel/acpi/boot.c    |   17 +-----------
 arch/x86/kernel/apic.c         |   30 +++++----------------
 arch/x86/kernel/io_apic.c      |   26 +-----------------
 arch/x86/pci/i386.c            |   38 ++++----------------------
 arch/x86/pci/mmconfig-shared.c |   49 ++--------------------------------
 include/linux/ioport.h         |    3 ++
 kernel/resource.c              |   58 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 83 insertions(+), 138 deletions(-)

Index: linux-2.6/arch/x86/kernel/acpi/boot.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/acpi/boot.c
+++ linux-2.6/arch/x86/kernel/acpi/boot.c
@@ -702,30 +702,17 @@ static int __init acpi_parse_hpet(struct
 	hpet_res = alloc_bootmem(sizeof(*hpet_res) + HPET_RESOURCE_NAME_SIZE);
 
 	hpet_res->name = (void *)&hpet_res[1];
-	hpet_res->flags = IORESOURCE_MEM;
+	hpet_res->flags = IORESOURCE_MEM | IORESOURCE_STICKY;
 	snprintf((char *)hpet_res->name, HPET_RESOURCE_NAME_SIZE, "HPET %u",
 		 hpet_tbl->sequence);
 
 	hpet_res->start = hpet_address;
 	hpet_res->end = hpet_address + (1 * 1024) - 1;
+	insert_resource(&iomem_resource, hpet_res);
 
 	return 0;
 }
 
-/*
- * hpet_insert_resource inserts the HPET resources used into the resource
- * tree.
- */
-static __init int hpet_insert_resource(void)
-{
-	if (!hpet_res)
-		return 1;
-
-	return insert_resource(&iomem_resource, hpet_res);
-}
-
-late_initcall(hpet_insert_resource);
-
 #else
 #define	acpi_parse_hpet	NULL
 #endif
Index: linux-2.6/arch/x86/kernel/apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic.c
+++ ...
From: Ingo Molnar
Date: Thursday, August 28, 2008 - 1:21 am

very nice patch! I've split your patch into two, and applied the 
kernel/resource.c and include/linux/ioresource.h core bits to the 
tip/core/resources topic branch - see the first patch below. (i renamed 
the new API to check_sticky_resource())

Then i've merged this topic branch into irq/sparseirq and applied the 
x86 bits that use this new facility to irq/sparseirq [on which it has 
many dependencies, io_apic.c and apic.c got unified, etc.]. See the 
second patch below.

Especially the second patch shows how much fragile resource code we are 
able to remove this way:

 5 files changed, 22 insertions(+), 138 deletions(-)

nice work.

Does anyone have any suggestions of how to improve this some more? (or 
do it differently)

	Ingo

----------------------->
From 0fec91fc00059401dc756ad635975bea8f813309 Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yhlu.kernel@gmail.com>
Date: Wed, 27 Aug 2008 18:56:23 -0700
Subject: [PATCH] IO resources: add sticky resource type

Ingo suggested to use sticky resource type to record fixed resource.
That way we could check that BAR in the PCI init code and avoid updating
it after request_resource failed.

That way we could remove some tricky code about insert some platform
resources with late_initcall.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/ioport.h |    3 ++
 kernel/resource.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 22d2115..db53613 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -48,6 +48,8 @@ struct resource_list {
 #define IORESOURCE_SIZEALIGN	0x00020000	/* size indicates alignment */
 #define IORESOURCE_STARTALIGN	0x00040000	/* start field is alignment */
 
+#define IORESOURCE_STICKY	0x08000000
+
 #define IORESOURCE_DISABLED	0x10000000
 #define IORESOURCE_UNSET	0x20000000
 #define IORESOURCE_AUTO		0x40000000
@@ -109,6 +111,7 @@ extern ...
From: Linus Torvalds
Date: Thursday, August 28, 2008 - 10:20 am

As far as I can see, THIS IS TOTALLY BROKEN.

There's a reason why we add the broken resources LATE. There's a reason we 
_have_ to add them late.

Trying to come up with these braindamaged schemes to avoid doing it right 
is wrong. Don't do it.

The reason? Those bogus resources that the BIOS reports are simply NOT 
TRUSTWORTHY. They may actually be in all the wrong places, including 
covering a resource half-way, or crossing two real resources.

Yes, it's rare. But it happens. Which is why we should not do these broken 
things that _incorrectly_ assume that the BIOS resources will only ever be 
totally contained within a BAR, or will totally contain one. Think about 
the partial overlap case.

Guys, until you learn that the BIOS resources are _crap_ and at most 
random guesses, you shouldn't touch this code! And it has nothing to do 
with writing "clean" code, or making things "simple", because quite 
frankly, things simply ARE NOT simple!

The fact is, the only reliable way to handle these things has _always_ 
been to ask the hardware first. Add the broken resources from ACPI and 
other BIOS tables _later_. If they conflict, it is the ACPI/BIOS tables 
that should be removed.

Why do I have to tell this to people over and over again?

		Linus
--

From: Ingo Molnar
Date: Thursday, August 28, 2008 - 1:17 pm

i fully agree with that principle, i just messed up implementing it.

'Sticky resources' tried to be exactly the kind of 'untrusted, possibly 
wrong' resources, which should not prevent existing PCI resources from 
being registered - they would at most prevent new PCI resources from 
being allocated over them. (the free space is large enough for us to 
take the small/untrusted hint from the BIOS where not to allocate to)

I missed the possibility of a sticky resource not being wide enough and 
preventing a BAR from being registered, due to partial overlap. That was 
not intended.

I guess this whole patchset has to become a lot wider and a lot more 
involved.

	Ingo
--

Previous thread: sparc build failure by Stephen Rothwell on Wednesday, August 27, 2008 - 6:46 pm. (4 messages)

Next thread: [PATCH 1/2] anondev: init IDR statically by Alexey Dobriyan on Wednesday, August 27, 2008 - 7:25 pm. (1 message)