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]>
Index: linux-work/arch/ppc64/kernel/prom.c
===================================================================
--- linux-work.orig/arch/ppc64/kernel/prom.c 2004-10-26 13:15:54.000000000 +1000
+++ linux-work/arch/ppc64/kernel/prom.c 2004-10-26 17:22:28.397358448 +1000
@@ -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);
Index: linux-work/include/asm-ppc64/iommu.h
===================================================================
--- linux-work.orig/include/asm-ppc64/iommu.h 2004-10-26 13:15:55.000000000 +1000
+++ linux-work/include/asm-ppc64/iommu.h 2004-10-26 17:19:17.086442128 +1000
@@ -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 */
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 :).
> - 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.
> - 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]>
diff -Nru a/arch/ppc64/kernel/iommu.c b/arch/ppc64/kernel/iommu.c
--- a/arch/ppc64/kernel/iommu.c Tue Oct 26 11:36:42 2004
+++ b/arch/ppc64/kernel/iommu.c Tue Oct 26 11:36:42 2004
@@ -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
diff -Nru a/arch/ppc64/kernel/pSeries_iommu.c b/arch/ppc64/kernel/pSeries_iommu.c
--- a/arch/ppc64/kernel/pSeries_iommu.c Tue Oct 26 11:36:42 2004
+++ b/arch/ppc64/kernel/pSeries_iommu.c Tue Oct 26 11:36:42 2004
@@ -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;
diff -Nru a/arch/ppc64/kernel/prom.c b/arch/ppc64/kernel/prom.c
--- a/arch/ppc64/kernel/prom.c Tue Oct 26 11:36:42 2004
+++ b/arch/ppc64/kernel/prom.c Tue Oct 26 11:36:42 2004
@@ -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);
On Tue, 2004-10-26 at 11:41 -0500, John Rose wrote:
> 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.