x86: Fix CONFIG_PCI=n compile failure

PATCH In review James Bottomley 4 participants 8 messages Oct 28, 2008 – Oct 30, 2008 x86

lore

Share & tools

lore

James Bottomley

author

2008-10-28 21:22 UTC #0 lore raw

PATCH 1/1 x86: Fix CONFIG_PCI=n compile failure

This:

commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899

Author: Glauber Costa <[email protected]>
Date:   Tue Apr 8 13:20:56 2008 -0300

x86: merge iommu initialization parameters

Moved the forbid_dac parameter into pci-dma.c but forgot that it's a PCI only symbol thus causing a compile failure if CONFIG_PCI=N. Fix by surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI

Signed-off-by James Bottomley <[email protected]>

---

arch/x86/kernel/pci-dma.c +2 -0

index 1972266..47c5a7a 100644
@@ -202,6 +202,7 @@ static __init int iommu_setup(char *p)
 			iommu_merge = 0;
 		if (!strncmp(p, "forcesac", 8))
 			iommu_sac_force = 1;
+#ifdef CONFIG_PCI
 		if (!strncmp(p, "allowdac", 8))
 			forbid_dac = 0;
 		if (!strncmp(p, "nodac", 5))

@@ -210,6 +211,7 @@ static __init int iommu_setup(char *p)
 			forbid_dac = -1;
 			return 1;
 		}
+#endif
 #ifdef CONFIG_SWIOTLB
 		if (!strncmp(p, "soft", 4))
 			swiotlb = 1;

FUJITA Tomonori

2008-10-28 22:42 UTC #1 lore raw

Quoted 6 lines · On Tue, 28 Oct 2008 16:22:23 -0500 James Bottomley…

This:
commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
Author: Glauber Costa <[email protected]>
Date:   Tue Apr 8 13:20:56 2008 -0300

Quoted 7 lines

x86: merge iommu initialization parameters
Moved the forbid_dac parameter into pci-dma.c but forgot that it's a PCI only symbol thus causing a compile failure if CONFIG_PCI=N. Fix by surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI
Signed-off-by James Bottomley <[email protected]>

http://marc.info/?l=linux-kernel&m=122480590627590&w=2

I guess that this patch is in the PCI tree.

Can you please send this quickly? I've seen bug reports due to this too many times.

Ingo Molnar

2008-10-29 07:37 UTC #2 lore raw

In reply to James Bottomley

Quoted 11 lines · James Bottomley

This:
commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
Author: Glauber Costa <[email protected]>
Date:   Tue Apr 8 13:20:56 2008 -0300
x86: merge iommu initialization parameters
Moved the forbid_dac parameter into pci-dma.c but forgot that it's a PCI only symbol thus causing a compile failure if CONFIG_PCI=N. Fix by surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI

uhm, that's wrong James, the above commit you refer to is half a year old and has been released in v2.6.26 and v2.6.27 ;-)

The patch that broke the !CONFIG_PCI build in this cycle is this one:

| From 5b6985ce8ec7127b4d60ad450b64ca8b82748a3b Mon Sep 17 00:00:00 2001
| From: Fenghua Yu <[email protected]>
| Date: Thu, 16 Oct 2008 18:02:32 -0700
| Subject: [PATCH] intel-iommu: IA64 support
|
| The current Intel IOMMU code assumes that both host page size and 
| Intel IOMMU page size are 4KiB. The first patch supports variable 
| page size. This provides support for IA64 which has multiple page 
| sizes.
|
| This patch also adds some other code hooks for IA64 platform 
| including DMAR_OPERATION_TIMEOUT definition.
|
| [dwmw2: some cleanup]
| Signed-off-by: Fenghua Yu <[email protected]>
| Signed-off-by: Tony Luck <[email protected]>
| Signed-off-by: David Woodhouse <[email protected]>

Quoted 17 lines

Signed-off-by James Bottomley <[email protected]>

---

arch/x86/kernel/pci-dma.c +1 -0

index 1972266..47c5a7a 100644
@@ -202,6 +202,7 @@ static __init int iommu_setup(char *p)
 			iommu_merge = 0;
 		if (!strncmp(p, "forcesac", 8))
 			iommu_sac_force = 1;
+#ifdef CONFIG_PCI
 		if (!strncmp(p, "allowdac", 8))
 			forbid_dac = 0;
 		if (!strncmp(p, "nodac", 5))

arch/x86/kernel/pci-dma.c static __init int iommu_setup(char *p) · line 214

Quoted 4 lines

arch/x86/kernel/pci-dma.c +1 -0

@@ -210,6 +211,7 @@ static __init int iommu_setup(char *p)
 			forbid_dac = -1;
 			return 1;
 		}
+#endif

that's the wrong fix, the right fix from Fenghua Yu (from about a week ago) is at:

http://marc.info/?l=linux-kernel&m=122480590627590&w=2

and it is in the PCI tree already.

Ingo

James Bottomley

author

2008-10-29 14:02 UTC #3 lore raw

Quoted 28 lines · Ingo Molnar

Quoted 11 lines · James Bottomley
This:
commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
Author: Glauber Costa <[email protected]>
Date:   Tue Apr 8 13:20:56 2008 -0300
x86: merge iommu initialization parameters
Moved the forbid_dac parameter into pci-dma.c but forgot that it's a PCI only symbol thus causing a compile failure if CONFIG_PCI=N. Fix by surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI
uhm, that's wrong James, the above commit you refer to is half a year old and has been released in v2.6.26 and v2.6.27 ;-)
The patch that broke the !CONFIG_PCI build in this cycle is this one:
| From 5b6985ce8ec7127b4d60ad450b64ca8b82748a3b Mon Sep 17 00:00:00 2001
| From: Fenghua Yu <[email protected]>
| Date: Thu, 16 Oct 2008 18:02:32 -0700
| Subject: [PATCH] intel-iommu: IA64 support
|
| The current Intel IOMMU code assumes that both host page size and 
| Intel IOMMU page size are 4KiB. The first patch supports variable 
| page size. This provides support for IA64 which has multiple page 
| sizes.

Quoted 8 lines

|
| This patch also adds some other code hooks for IA64 platform 
| including DMAR_OPERATION_TIMEOUT definition.
|
| [dwmw2: some cleanup]
| Signed-off-by: Fenghua Yu <[email protected]>
| Signed-off-by: Tony Luck <[email protected]>
| Signed-off-by: David Woodhouse <[email protected]>

Hmm, well, the triage could be wrong ... however the prior patch seemed to move forbid_dac out from under the PCI defines.

Quoted 25 lines

Quoted 17 lines
Signed-off-by James Bottomley <[email protected]>
---
arch/x86/kernel/pci-dma.c +1 -0
index 1972266..47c5a7a 100644
@@ -202,6 +202,7 @@ static __init int iommu_setup(char *p)
 			iommu_merge = 0;
 		if (!strncmp(p, "forcesac", 8))
 			iommu_sac_force = 1;
+#ifdef CONFIG_PCI
 		if (!strncmp(p, "allowdac", 8))
 			forbid_dac = 0;
 		if (!strncmp(p, "nodac", 5))
arch/x86/kernel/pci-dma.c static __init int iommu_setup(char *p) · line 214
Quoted 4 lines
arch/x86/kernel/pci-dma.c +1 -0
@@ -210,6 +211,7 @@ static __init int iommu_setup(char *p)
 			forbid_dac = -1;
 			return 1;
 		}
+#endif
that's the wrong fix, the right fix from Fenghua Yu (from about a week ago) is at:

Quoted 3 lines

http://marc.info/?l=linux-kernel&m=122480590627590&w=2
and it is in the PCI tree already.

This is obviously some strange definition of the word "right" of which I was previously unaware. That patch moves forbid_dac plus a load of quirk processing (also for a PCI bus) out from under CONFIG_PCI only ... which will fix the compile error, sure.

However, if you'd be so kind, please explain how a DAC (meaning Dual Addressing Cycle on the PCI bus) is useful (or even can be effected) without a PCI bus?

All its really doing is contaminating pci-dma.c with clutter that only needs to be there because someone can't get the separation right.

James

Arjan van de Ven

2008-10-29 14:08 UTC #4 lore raw

Quoted 5 lines · On Wed, 29 Oct 2008 09:01:46 -0500 James Bottomley…

This is obviously some strange definition of the word "right" of which I was previously unaware. That patch moves forbid_dac plus a load of quirk processing (also for a PCI bus) out from under CONFIG_PCI only ... which will fix the compile error, sure.

Quoted 7 lines

However, if you'd be so kind, please explain how a DAC (meaning Dual Addressing Cycle on the PCI bus) is useful (or even can be effected) without a PCI bus?
All its really doing is contaminating pci-dma.c with clutter that only needs to be there because someone can't get the separation right.o

why do you even want that file for CONFIG_PCI=n ??

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

James Bottomley

author

2008-10-29 14:18 UTC #5 lore raw

Quoted 12 lines · Arjan van de Ven

Quoted 9 lines · On Wed, 29 Oct 2008 09:01:46 -0500 James Bottomley…
This is obviously some strange definition of the word "right" of which I was previously unaware. That patch moves forbid_dac plus a load of quirk processing (also for a PCI bus) out from under CONFIG_PCI only ... which will fix the compile error, sure.
However, if you'd be so kind, please explain how a DAC (meaning Dual Addressing Cycle on the PCI bus) is useful (or even can be effected) without a PCI bus?

Quoted 5 lines

Quoted 3 lines
All its really doing is contaminating pci-dma.c with clutter that only needs to be there because someone can't get the separation right.o
why do you even want that file for CONFIG_PCI=n ??

Um, my fault from long ago. It's contaminated with the dma_ API pieces that are bus generic.

The correct solution, I think, is to split it out into a bus generic piece and a PCI specific piece. I think all the intel/amd IOMMU stuff should be in the PCI specific piece ... although I know there's a theoretical case where the AMD iommu can be ht bus only with no PCI bus, I don't think anyone's built such a beast, in which case it's safe to condition iommu presence on CONFIG_PCI?

James

Arjan van de Ven

2008-10-29 15:14 UTC #6 lore raw

Quoted 18 lines · On Wed, 29 Oct 2008 09:18:03 -0500 James Bottomley…

Quoted 17 lines · Arjan van de Ven
Quoted 13 lines · On Wed, 29 Oct 2008 09:01:46 -0500 James Bottomley…
This is obviously some strange definition of the word "right" of which I was previously unaware. That patch moves forbid_dac plus a load of quirk processing (also for a PCI bus) out from under CONFIG_PCI only ... which will fix the compile error, sure.
However, if you'd be so kind, please explain how a DAC (meaning Dual Addressing Cycle on the PCI bus) is useful (or even can be effected) without a PCI bus?
All its really doing is contaminating pci-dma.c with clutter that only needs to be there because someone can't get the separation right.o

Quoted 7 lines

Quoted 1 line
why do you even want that file for CONFIG_PCI=n ??
Um, my fault from long ago. It's contaminated with the dma_ API pieces that are bus generic.
The correct solution, I think, is to split it out into a bus generic piece and a PCI specific piece.

agreed

Quoted 5 lines

I think all the intel/amd IOMMU stuff
should be in the PCI specific piece ... although I know there's a theoretical case where the AMD iommu can be ht bus only with no PCI bus, I don't think anyone's built such a beast, in which case it's safe to condition iommu presence on CONFIG_PCI?

I would think that's not relevant; AMD seems to have PCI in it's cpu package already, never mind what it is connected to.

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

James Bottomley

author

2008-10-30 20:13 UTC #7 lore raw

PATCH 1/1 x86 separate PCI out from DMA operations

This splits pci-dma.c into two pieces (a PCI specific pci-dma.c and a generic dma.c) and conditions the compile of the PCI specific piece on CONFIG_PCI.

This has the benefit of sweeping all the IOMMU glue into PCI specific files.

Signed-off-by James Bottomley <[email protected]>

---

Note: it's possible to merge pci-nommu into this if desired, since that file really has nothing to do with PCI and everything to do with generic dma operations.

Also, I used the CONFIG_PCI conditioning dma_supported() rather than weak symbol trickery because of the EXPORT_SYMBOL() requirements.

James

arch/x86/kernel/Makefile +2 -1

index 58814cc..be44178 100644
@@ -34,7 +34,8 @@ obj-$(CONFIG_X86_32)	+= sys_i386_32.o i386_ksyms_32.o
 obj-$(CONFIG_X86_64)	+= sys_x86_64.o x8664_ksyms_64.o
 obj-$(CONFIG_X86_64)	+= syscall_64.o vsyscall_64.o
 obj-y			+= bootflag.o e820.o
-obj-y			+= pci-dma.o quirks.o i8237.o topology.o kdebugfs.o
+obj-$(CONFIG_PCI)	+= pci-dma.o
+obj-y			+= dma.o quirks.o i8237.o topology.o kdebugfs.o
 obj-y			+= alternative.o i8253.o pci-nommu.o
 obj-y			+= tsc.o io_delay.o rtc.o

arch/x86/kernel/pci-dma.c +4 ~~-67~~

125 lines not on this page · raw message

arch/x86/kernel/pci-nommu.c +0 -2

index c70ab5a..5e7dd85 100644
@@ -90,7 +90,5 @@ void __init no_iommu_init(void)
 {
 	if (dma_ops)
 		return;
-
-	force_iommu = 0; /* no HW IOMMU */
 	dma_ops = &nommu_dma_ops;
 }

arch/x86/kernel/dma.c new file +104 -0

104 lines not on this page · raw message