Skip to contentSkip to conversation

ppc64: Fix g5-only build

PATCH In review Benjamin Herrenschmidt 2 participants 3 messages Oct 26, 2004

lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/1 ppc64: Fix g5-only build

Hi !

The iommu_free_table() patch broke g5 only build by adding back some incestuous relationship between generic code and pSeries code.
This patch wraps this in #ifdef as a quick fix until the original author of the patch comes up with a better solution.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
arch/ppc64/kernel/prom.c +5 -0
Index: linux-work/arch/ppc64/kernel/prom.c
===================================================================
@@ -1818,8 +1818,13 @@
 		return -EBUSY;
 	}
 
+	/* XXX This is a layering violation, should be moved to the caller
+	 * --BenH.
+	 */
+#ifdef CONFIG_PPC_PSERIES
 	if (np->iommu_table)
 		iommu_free_table(np);
+#endif /* CONFIG_PPC_PSERIES */
 
 	write_lock(&devtree_lock);
 	OF_MARK_STALE(np);
include/asm-ppc64/iommu.h +8 -0
Index: linux-work/include/asm-ppc64/iommu.h
===================================================================
@@ -111,9 +111,17 @@
 extern void iommu_setup_u3(void);
 
 /* Creates table for an individual device node */
+/* XXX: This isn't generic, please name it accordingly or add
+ * some ppc_md. hooks for iommu implementations to do what they
+ * need to do. --BenH.
+ */
 extern void iommu_devnode_init(struct device_node *dn);
 
 /* Frees table for an individual device node */
+/* XXX: This isn't generic, please name it accordingly or add
+ * some ppc_md. hooks for iommu implementations to do what they
+ * need to do. --BenH.
+ */
 extern void iommu_free_table(struct device_node *dn);
 
 #endif /* CONFIG_PPC_MULTIPLATFORM */
#1 lore raw

Forgive me for the cross-post, but I'm trying to answer two list messages on the same topic. I think it's more productive to just fix the bug than to commit a giant comment pointing out a small bug, so I've attached an alternate fix (build tested for g5 :).

Quoted 7 lines

- It breaks build without CONFIG_PPC_PSERIES (try a pmac-only build). There is, more generally, a tendency at calling things in pSeries_iommu.c with the prefix "iommu_" without any mention of "pSeries" in the name. Hey guys ! pSeries isn't alone anymore ! So please call those pSeries-specific things pSeries_* or tce_* or whatever, but don't add back confusion where I had such a hard time splitting things.

Apologies for the build break. I mistakenly placed the function in a pSeries file. In our view, this is a generic function, complementary to iommu_init_table(), so I've moved it to iommu.c.

Quoted 5 lines

- It seems that any call to of_remove_node() will call iommu_free_table() on np->iommu_table. That sounds bad. The iommu_table pointer is copied at init time from the parent to all child nodes. So if we add a phb, and then remove a device from that bus, we end up disposing of the phb's iommu table ...

Good catch, although table allocation doesn't always happen at the PHB level. On POWER5, it happens at the EADS level. My fix checks for the ibm,dma-window property before calling the free function. This is the criterion for which the table is alloc'ed in the first place.

Thanks-
John

Signed-off-by John Rose <[email protected]>
arch/ppc64/kernel/iommu.c +33 -0
diff -Nru a/arch/ppc64/kernel/iommu.c b/arch/ppc64/kernel/iommu.c
@@ -425,6 +425,39 @@
 	return tbl;
 }
 
+void iommu_free_table(struct device_node *dn)
+{
+	struct iommu_table *tbl = dn->iommu_table;
+	unsigned long bitmap_sz, i;
+	unsigned int order;
+
+	if (!tbl || !tbl->it_map) {
+		printk(KERN_ERR "%s: expected TCE map for %s\n", __FUNCTION__,
+				dn->full_name);
+		return;
+	}
+
+	/* verify that table contains no entries */
+	/* it_mapsize is in entries, and we're examining 64 at a time */
+	for (i = 0; i < (tbl->it_mapsize/64); i++) {
+		if (tbl->it_map[i] != 0) {
+			printk(KERN_WARNING "%s: Unexpected TCEs for %s\n",
+				__FUNCTION__, dn->full_name);
+			break;
+		}
+	}
+
+	/* calculate bitmap size in bytes */
+	bitmap_sz = (tbl->it_mapsize + 7) / 8;
+
+	/* free bitmap */
+	order = get_order(bitmap_sz);
+	free_pages((unsigned long) tbl->it_map, order);
+
+	/* free table */
+	kfree(tbl);
+}
+
 /* Creates TCEs for a user provided buffer.  The user buffer must be
  * contiguous real kernel storage (not vmalloc).  The address of the buffer
  * passed here is the kernel (virtual) address of the buffer.  The buffer
arch/ppc64/kernel/pSeries_iommu.c +0 -33
diff -Nru a/arch/ppc64/kernel/pSeries_iommu.c b/arch/ppc64/kernel/pSeries_iommu.c
@@ -412,39 +412,6 @@
 	dn->iommu_table = iommu_init_table(tbl);
 }
 
-void iommu_free_table(struct device_node *dn)
-{
-	struct iommu_table *tbl = dn->iommu_table;
-	unsigned long bitmap_sz, i;
-	unsigned int order;
-
-	if (!tbl || !tbl->it_map) {
-		printk(KERN_ERR "%s: expected TCE map for %s\n", __FUNCTION__,
-				dn->full_name);
-		return;
-	}
-
-	/* verify that table contains no entries */
-	/* it_mapsize is in entries, and we're examining 64 at a time */
-	for (i = 0; i < (tbl->it_mapsize/64); i++) {
-		if (tbl->it_map[i] != 0) {
-			printk(KERN_WARNING "%s: Unexpected TCEs for %s\n",
-				__FUNCTION__, dn->full_name);
-			break;
-		}
-	}
-
-	/* calculate bitmap size in bytes */
-	bitmap_sz = (tbl->it_mapsize + 7) / 8;
-
-	/* free bitmap */
-	order = get_order(bitmap_sz);
-	free_pages((unsigned long) tbl->it_map, order);
-
-	/* free table */
-	kfree(tbl);
-}
-
 void iommu_setup_pSeries(void)
 {
 	struct pci_dev *dev = NULL;
arch/ppc64/kernel/prom.c +2 -1
diff -Nru a/arch/ppc64/kernel/prom.c b/arch/ppc64/kernel/prom.c
@@ -1818,8 +1818,9 @@
 		return -EBUSY;
 	}
 
-	if (np->iommu_table)
+	if ((np->iommu_table) && get_property(np, "ibm,dma-window", NULL)) {
 		iommu_free_table(np);
+	}
 
 	write_lock(&devtree_lock);
 	OF_MARK_STALE(np);
author
#2 lore raw
Quoted 4 lines · John Rose

Forgive me for the cross-post, but I'm trying to answer two list messages on the same topic. I think it's more productive to just fix the bug than to commit a giant comment pointing out a small bug, so I've attached an alternate fix (build tested for g5 :).

I replied the other list, let's stop this thread here.

Ben.