Re: [PATCH 11/19] powerpc: iommu enablement for CMO
From: Olof Johansson <hidden>
Date: 2008-06-13 01:23:37
Hi, Some comments and questions below. -Olof On Thu, Jun 12, 2008 at 05:19:36PM -0500, Robert Jennings wrote:
quoted hunk ↗ jump to hunk
Index: b/arch/powerpc/kernel/iommu.c ===================================================================--- a/arch/powerpc/kernel/iommu.c +++ b/arch/powerpc/kernel/iommu.c@@ -183,6 +183,49 @@ static unsigned long iommu_range_alloc(s return n; } +/** iommu_undo - Clear iommu_table bits without calling platform tce_free. + * + * @tbl - struct iommu_table to alter + * @dma_addr - DMA address to free entries for + * @npages - number of pages to free entries for + * + * This is the same as __iommu_free without the call to ppc_md.tce_free();
__iommu_free has the __ prepended to indicate that it's not locking. Since this does the same, please keep the __. Also see comments below.
+ *
+ * To clean up after ppc_md.tce_build() errors we need to clear bits
+ * in the table without calling the ppc_md.tce_free() method; calling
+ * ppc_md.tce_free() could alter entries that were not touched due to a
+ * premature failure in ppc_md.tce_build().
+ *
+ * The ppc_md.tce_build() needs to perform its own clean up prior to
+ * returning its error.
+ */
+static void iommu_undo(struct iommu_table *tbl, dma_addr_t dma_addr,
+ unsigned int npages)
+{
+ unsigned long entry, free_entry;
+
+ entry = dma_addr >> IOMMU_PAGE_SHIFT;
+ free_entry = entry - tbl->it_offset;
+
+ if (((free_entry + npages) > tbl->it_size) ||
+ (entry < tbl->it_offset)) {
+ if (printk_ratelimit()) {
+ printk(KERN_INFO "iommu_undo: invalid entry\n");
+ printk(KERN_INFO "\tentry = 0x%lx\n", entry);
+ printk(KERN_INFO "\tdma_addr = 0x%lx\n", (u64)dma_addr);
+ printk(KERN_INFO "\tTable = 0x%lx\n", (u64)tbl);
+ printk(KERN_INFO "\tbus# = 0x%lx\n", tbl->it_busno);
+ printk(KERN_INFO "\tsize = 0x%lx\n", tbl->it_size);
+ printk(KERN_INFO "\tstartOff = 0x%lx\n", tbl->it_offset);
+ printk(KERN_INFO "\tindex = 0x%lx\n", tbl->it_index);
+ WARN_ON(1);
+ }
+ return;
+ }
+
+ iommu_area_free(tbl->it_map, free_entry, npages);
+}Ick, This should just be refactored to reuse code together with iommu_free() instead of duplicating it. Also, the error checking shouldn't be needed here. Actually, is there harm in calling tce_free for these cases anyway? I'm guessing it's not a performance critical path.
quoted hunk ↗ jump to hunk
@@ -275,7 +330,7 @@ int iommu_map_sg(struct device *dev, str dma_addr_t dma_next = 0, dma_addr; unsigned long flags; struct scatterlist *s, *outs, *segstart; - int outcount, incount, i; + int outcount, incount, i, rc = 0; unsigned int align; unsigned long handle; unsigned int max_seg_size;@@ -336,7 +391,10 @@ int iommu_map_sg(struct device *dev, str npages, entry, dma_addr); /* Insert into HW table */ - ppc_md.tce_build(tbl, entry, npages, vaddr & IOMMU_PAGE_MASK, direction); + rc = ppc_md.tce_build(tbl, entry, npages, + vaddr & IOMMU_PAGE_MASK, direction); + if(unlikely(rc)) + goto failure; /* If we are in an open segment, try merging */ if (segstart != s) {@@ -399,7 +457,10 @@ int iommu_map_sg(struct device *dev, str vaddr = s->dma_address & IOMMU_PAGE_MASK; npages = iommu_num_pages(s->dma_address, s->dma_length); - __iommu_free(tbl, vaddr, npages); + if (!rc) + __iommu_free(tbl, vaddr, npages); + else + iommu_undo(tbl, vaddr, npages);
'rc' is a quite generic name to carry state this far away from where it's set. Either a more descriptive name (build_fail, whatever), or if the above is true, just call __iommu_free here as well.
quoted hunk ↗ jump to hunk
-static void tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum, +static void tce_free_pSeriesLP(struct iommu_table*, long, long); +static void tce_freemulti_pSeriesLP(struct iommu_table*, long, long); + +static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages, unsigned long uaddr, enum dma_data_direction direction) { - u64 rc; + u64 rc = 0; u64 proto_tce, tce; u64 rpn; + int sleep_msecs, ret = 0; + long tcenum_start = tcenum, npages_start = npages; rpn = (virt_to_abs(uaddr)) >> TCE_SHIFT; proto_tce = TCE_PCI_READ;@@ -108,7 +115,21 @@ static void tce_build_pSeriesLP(struct i while (npages--) { tce = proto_tce | (rpn & TCE_RPN_MASK) << TCE_RPN_SHIFT; - rc = plpar_tce_put((u64)tbl->it_index, (u64)tcenum << 12, tce); + do { + rc = plpar_tce_put((u64)tbl->it_index, + (u64)tcenum << 12, tce); + if (unlikely(H_IS_LONG_BUSY(rc))) { + sleep_msecs = plpar_get_longbusy_msecs(rc); + mdelay(sleep_msecs);
Ouch! You're holding locks and stuff here. Do you really want this right here?
+ } + } while (unlikely(H_IS_LONG_BUSY(rc)));
Do you also want to keep doing this forever, or eventually just fail instead?
quoted hunk ↗ jump to hunk
+ if (unlikely(rc == H_NOT_ENOUGH_RESOURCES)) { + ret = (int)rc; + tce_free_pSeriesLP(tbl, tcenum_start, + (npages_start - (npages + 1))); + break; + } if (rc && printk_ratelimit()) { printk("tce_build_pSeriesLP: plpar_tce_put failed. rc=%ld\n", rc);@@ -121,19 +142,22 @@ static void tce_build_pSeriesLP(struct i tcenum++; rpn++; } + return ret; } static DEFINE_PER_CPU(u64 *, tce_page) = NULL; -static void tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum, +static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages, unsigned long uaddr, enum dma_data_direction direction) { - u64 rc; + u64 rc = 0; u64 proto_tce; u64 *tcep; u64 rpn; long l, limit; + long tcenum_start = tcenum, npages_start = npages; + int sleep_msecs, ret = 0; if (npages == 1) return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,@@ -171,15 +195,26 @@ static void tce_buildmulti_pSeriesLP(str rpn++; } - rc = plpar_tce_put_indirect((u64)tbl->it_index, - (u64)tcenum << 12, - (u64)virt_to_abs(tcep), - limit); + do { + rc = plpar_tce_put_indirect(tbl->it_index, tcenum << 12, + virt_to_abs(tcep), limit); + if (unlikely(H_IS_LONG_BUSY(rc))) { + sleep_msecs = plpar_get_longbusy_msecs(rc); + mdelay(sleep_msecs); + } + } while (unlikely(H_IS_LONG_BUSY(rc))); npages -= limit; tcenum += limit; } while (npages > 0 && !rc); + if (unlikely(rc == H_NOT_ENOUGH_RESOURCES)) { + ret = (int)rc; + tce_freemulti_pSeriesLP(tbl, tcenum_start, + (npages_start - (npages + limit))); + return ret; + } + if (rc && printk_ratelimit()) { printk("tce_buildmulti_pSeriesLP: plpar_tce_put failed. rc=%ld\n", rc); printk("\tindex = 0x%lx\n", (u64)tbl->it_index);@@ -187,14 +222,23 @@ static void tce_buildmulti_pSeriesLP(str printk("\ttce[0] val = 0x%lx\n", tcep[0]); show_stack(current, (unsigned long *)__get_SP()); } + return ret; } static void tce_free_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages) { + int sleep_msecs; u64 rc; while (npages--) { - rc = plpar_tce_put((u64)tbl->it_index, (u64)tcenum << 12, 0); + do { + rc = plpar_tce_put((u64)tbl->it_index, + (u64)tcenum << 12, 0); + if (unlikely(H_IS_LONG_BUSY(rc))) { + sleep_msecs = plpar_get_longbusy_msecs(rc); + mdelay(sleep_msecs); + } + } while (unlikely(H_IS_LONG_BUSY(rc)));
Can this ever happen? I would hope that any entry that's got an active mapping is actually pinned in memory, what other than paging in from disk can result in long busy?
quoted hunk ↗ jump to hunk
@@ -210,9 +254,17 @@ static void tce_free_pSeriesLP(struct io static void tce_freemulti_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages) { + int sleep_msecs; u64 rc; - rc = plpar_tce_stuff((u64)tbl->it_index, (u64)tcenum << 12, 0, npages); + do { + rc = plpar_tce_stuff((u64)tbl->it_index, + (u64)tcenum << 12, 0, npages); + if (unlikely(H_IS_LONG_BUSY(rc))) { + sleep_msecs = plpar_get_longbusy_msecs(rc); + mdelay(sleep_msecs); + } + } while (unlikely(H_IS_LONG_BUSY(rc))); if (rc && printk_ratelimit()) { printk("tce_freemulti_pSeriesLP: plpar_tce_stuff failed\n");