* [PATCH V2 0/7] swiotlb: changes for powerpc/highmem @ 2009-04-04 1:56 Becky Bruce 2009-04-04 1:56 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Becky Bruce 2009-04-07 2:24 ` [PATCH V2 0/7] swiotlb: changes for powerpc/highmem FUJITA Tomonori 0 siblings, 2 replies; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell This is v2 of a series of fairly minor patches that get swiotlb working on 32-bit powerpc systems with HIGHMEM, plus some cleanup of the outdated comments in the code. I've made a couple of things weak that ppc needs to override, and have changed the prototypes for a couple of functions to include the hwdev pointer, which we need to ppc to convert bus addresses to and from phys/virt addresses. I've also fixed a build warning I've been seeing on ppc. In response to commentary on the previous series, I've also refactored the code a bit, altough I did this slightly differently than was suggested because I noticed we could use the new helper function in 2 places instead of one. I've reformatted a bit of code based on commentary as well. I have not tested this in any way on any non-ppc platforms, so commentary/testing from x86/ia64 folks is, once again, greatly appreciated. I'm going to be offline for the next week, but will respond to commentary as soon as I return. Cheers, Becky diffstat: arch/x86/kernel/pci-swiotlb.c | 2 +- include/linux/swiotlb.h | 3 +- lib/swiotlb.c | 115 ++++++++++++++++++++++------------------- 3 files changed, 64 insertions(+), 56 deletions(-) ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/7] swiotlb: comment corrections (no code changes) 2009-04-04 1:56 [PATCH V2 0/7] swiotlb: changes for powerpc/highmem Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 2009-04-04 1:56 ` [PATCH 2/7] swiotlb: fix compile warning Becky Bruce 2009-04-07 2:24 ` [PATCH V2 0/7] swiotlb: changes for powerpc/highmem FUJITA Tomonori 1 sibling, 1 reply; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce swiotlb_map/unmap_single are now swiotlb_map/unmap_page; trivially change all the comments to reference new names. Also, there were some comments that should have been referring to just plain old map_single, not swiotlb_map_single; fix those as well. Also change a use of the word "pointer", when what is referred to is actually a dma/physical address. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- lib/swiotlb.c | 24 +++++++++++------------- 1 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 32e2bd3..f59cf30 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -60,8 +60,8 @@ enum dma_sync_target { int swiotlb_force; /* - * Used to do a quick range check in swiotlb_unmap_single and - * swiotlb_sync_single_*, to see if the memory was in fact allocated by this + * Used to do a quick range check in unmap_single and + * sync_single_*, to see if the memory was in fact allocated by this * API. */ static char *io_tlb_start, *io_tlb_end; @@ -560,7 +560,6 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, size)) { /* * The allocated memory isn't reachable by the device. - * Fall back on swiotlb_map_single(). */ free_pages((unsigned long) ret, order); ret = NULL; @@ -568,9 +567,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, if (!ret) { /* * We are either out of memory or the device can't DMA - * to GFP_DMA memory; fall back on - * swiotlb_map_single(), which will grab memory from - * the lowest available address range. + * to GFP_DMA memory; fall back on map_single(), which + * will grab memory from the lowest available address range. */ ret = map_single(hwdev, 0, size, DMA_FROM_DEVICE); if (!ret) @@ -634,7 +632,7 @@ swiotlb_full(struct device *dev, size_t size, int dir, int do_panic) * physical address to use is returned. * * Once the device is given the dma address, the device owns this memory until - * either swiotlb_unmap_single or swiotlb_dma_sync_single is performed. + * either swiotlb_unmap_page or swiotlb_dma_sync_single is performed. */ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, unsigned long offset, size_t size, @@ -648,7 +646,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, BUG_ON(dir == DMA_NONE); /* - * If the pointer passed in happens to be in the device's DMA window, + * If the address happens to be in the device's DMA window, * we can safely return the device addr and not worry about bounce * buffering it. */ @@ -679,7 +677,7 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page); /* * Unmap a single streaming mode DMA translation. The dma_addr and size must - * match what was provided for in a previous swiotlb_map_single call. All + * match what was provided for in a previous swiotlb_map_page call. All * other usages are undefined. * * After this call, reads by the cpu to the buffer are guaranteed to see @@ -703,7 +701,7 @@ EXPORT_SYMBOL_GPL(swiotlb_unmap_page); * Make physical memory consistent for a single streaming mode DMA translation * after a transfer. * - * If you perform a swiotlb_map_single() but wish to interrogate the buffer + * If you perform a swiotlb_map_page() but wish to interrogate the buffer * using the cpu, yet do not wish to teardown the dma mapping, you must * call this function before doing so. At the next point you give the dma * address back to the card, you must first perform a @@ -777,7 +775,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device); /* * Map a set of buffers described by scatterlist in streaming mode for DMA. - * This is the scatter-gather version of the above swiotlb_map_single + * This is the scatter-gather version of the above swiotlb_map_page * interface. Here the scatter gather list elements are each tagged with the * appropriate dma address and length. They are obtained via * sg_dma_{address,length}(SG). @@ -788,7 +786,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device); * The routine returns the number of addr/length pairs actually * used, at most nents. * - * Device ownership issues as mentioned above for swiotlb_map_single are the + * Device ownership issues as mentioned above for swiotlb_map_page are the * same here. */ int @@ -836,7 +834,7 @@ EXPORT_SYMBOL(swiotlb_map_sg); /* * Unmap a set of streaming mode DMA translations. Again, cpu read rules - * concerning calls here are the same as for swiotlb_unmap_single() above. + * concerning calls here are the same as for swiotlb_unmap_page() above. */ void swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl, -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 2/7] swiotlb: fix compile warning 2009-04-04 1:56 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 2009-04-04 1:56 ` [PATCH 3/7] swiotlb: map_page fix for highmem systems Becky Bruce 0 siblings, 1 reply; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce Squash a build warning seen on 32-bit powerpc caused by calling min() with 2 different types. Use min_t() instead Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- lib/swiotlb.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index f59cf30..fa62498 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size, unsigned long flags; while (size) { - sz = min(PAGE_SIZE - offset, size); + sz = min_t(size_t, PAGE_SIZE - offset, size); local_irq_save(flags); buffer = kmap_atomic(pfn_to_page(pfn), -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 3/7] swiotlb: map_page fix for highmem systems 2009-04-04 1:56 ` [PATCH 2/7] swiotlb: fix compile warning Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 2009-04-04 1:56 ` [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping Becky Bruce 0 siblings, 1 reply; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce The current code calls virt_to_phys() on address that might be in highmem, which is bad. This wasn't needed, anyway, because we already have the physical address we need. Get rid of the now-unused virtual address as well. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- lib/swiotlb.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index fa62498..eb6dbbd 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -640,7 +640,6 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, struct dma_attrs *attrs) { phys_addr_t phys = page_to_phys(page) + offset; - void *ptr = page_address(page) + offset; dma_addr_t dev_addr = swiotlb_phys_to_bus(dev, phys); void *map; @@ -651,7 +650,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, * buffering it. */ if (!address_needs_mapping(dev, dev_addr, size) && - !range_needs_mapping(virt_to_phys(ptr), size)) + !range_needs_mapping(phys, size)) return dev_addr; /* -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-04 1:56 ` [PATCH 3/7] swiotlb: map_page fix for highmem systems Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 2009-04-04 1:56 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single Becky Bruce 0 siblings, 1 reply; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce Some architectures require additional checking to determine if a device can dma to an address and need to provide their own address_needs_mapping.. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- lib/swiotlb.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index eb6dbbd..af2ec25 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -145,6 +145,12 @@ static void *swiotlb_bus_to_virt(dma_addr_t address) return phys_to_virt(swiotlb_bus_to_phys(address)); } +int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev, + dma_addr_t addr, size_t size) +{ + return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size); +} + int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; @@ -309,10 +315,10 @@ cleanup1: return -ENOMEM; } -static int +static inline int address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size) { - return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size); + return swiotlb_arch_address_needs_mapping(hwdev, addr, size); } static inline int range_needs_mapping(phys_addr_t paddr, size_t size) -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-04 1:56 ` [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 2009-04-04 1:56 ` [PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code Becky Bruce 2009-04-07 2:24 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single FUJITA Tomonori 0 siblings, 2 replies; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce This mirrors the current swiotlb_sync_single() setup where the swiotlb_unmap_single() function is static to this file and contains the logic required to determine if we need to call actual sync_single. Previously, swiotlb_unmap_page and swiotlb_unmap_sg were duplicating very similar code. The duplicated code has also been reformatted for readability. Note that the swiotlb_unmap_sg code was previously doing a complicated comparison to determine if an addresses needed to be unmapped where a simple is_swiotlb_buffer() call would have sufficed. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- lib/swiotlb.c | 36 +++++++++++++++++++++++------------- 1 files changed, 23 insertions(+), 13 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index af2ec25..602315b 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -688,17 +688,30 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page); * After this call, reads by the cpu to the buffer are guaranteed to see * whatever the device wrote there. */ -void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, - size_t size, enum dma_data_direction dir, - struct dma_attrs *attrs) +static void +swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr, + size_t size, int dir) { char *dma_addr = swiotlb_bus_to_virt(dev_addr); BUG_ON(dir == DMA_NONE); - if (is_swiotlb_buffer(dma_addr)) + + if (is_swiotlb_buffer(dma_addr)) { unmap_single(hwdev, dma_addr, size, dir); - else if (dir == DMA_FROM_DEVICE) - dma_mark_clean(dma_addr, size); + return; + } + + if (dir != DMA_FROM_DEVICE) + return; + + dma_mark_clean(dma_addr, size); +} + +void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, + size_t size, enum dma_data_direction dir, + struct dma_attrs *attrs) +{ + swiotlb_unmap_single(hwdev, dev_addr, size, dir); } EXPORT_SYMBOL_GPL(swiotlb_unmap_page); @@ -850,13 +863,10 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl, BUG_ON(dir == DMA_NONE); - for_each_sg(sgl, sg, nelems, i) { - if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg))) - unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address), - sg->dma_length, dir); - else if (dir == DMA_FROM_DEVICE) - dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length); - } + for_each_sg(sgl, sg, nelems, i) + swiotlb_unmap_single(hwdev, sg->dma_address, sg->dma_length, + dir); + } EXPORT_SYMBOL(swiotlb_unmap_sg_attrs); -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code 2009-04-04 1:56 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 2009-04-04 1:56 ` [PATCH 7/7] swiotlb: Change swiotlb_bus_to[phys,virt] prototypes Becky Bruce 2009-04-07 2:24 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single FUJITA Tomonori 1 sibling, 1 reply; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce Right now both swiotlb_sync_single_range and swiotlb_sync_sg were duplicating the code in swiotlb_sync_single. Just call it instead. Also rearrange the sync_single code for readability. Note that the swiotlb_sync_sg code was previously doing a complicated comparison to determine if an addresses needed to be unmapped where a simple is_swiotlb_buffer() call would have sufficed. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- lib/swiotlb.c | 30 ++++++++++++------------------ 1 files changed, 12 insertions(+), 18 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 602315b..c0f185a 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -732,10 +732,16 @@ swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr, char *dma_addr = swiotlb_bus_to_virt(dev_addr); BUG_ON(dir == DMA_NONE); - if (is_swiotlb_buffer(dma_addr)) + + if (is_swiotlb_buffer(dma_addr)) { sync_single(hwdev, dma_addr, size, dir, target); - else if (dir == DMA_FROM_DEVICE) - dma_mark_clean(dma_addr, size); + return; + } + + if (dir != DMA_FROM_DEVICE) + return; + + dma_mark_clean(dma_addr, size); } void @@ -762,13 +768,7 @@ swiotlb_sync_single_range(struct device *hwdev, dma_addr_t dev_addr, unsigned long offset, size_t size, int dir, int target) { - char *dma_addr = swiotlb_bus_to_virt(dev_addr) + offset; - - BUG_ON(dir == DMA_NONE); - if (is_swiotlb_buffer(dma_addr)) - sync_single(hwdev, dma_addr, size, dir, target); - else if (dir == DMA_FROM_DEVICE) - dma_mark_clean(dma_addr, size); + swiotlb_sync_single(hwdev, dev_addr + offset, size, dir, target); } void @@ -892,15 +892,9 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl, struct scatterlist *sg; int i; - BUG_ON(dir == DMA_NONE); - - for_each_sg(sgl, sg, nelems, i) { - if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg))) - sync_single(hwdev, swiotlb_bus_to_virt(sg->dma_address), + for_each_sg(sgl, sg, nelems, i) + swiotlb_sync_single(hwdev, sg->dma_address, sg->dma_length, dir, target); - else if (dir == DMA_FROM_DEVICE) - dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length); - } } void -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 7/7] swiotlb: Change swiotlb_bus_to[phys,virt] prototypes 2009-04-04 1:56 ` [PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code Becky Bruce @ 2009-04-04 1:56 ` Becky Bruce 0 siblings, 0 replies; 44+ messages in thread From: Becky Bruce @ 2009-04-04 1:56 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: Becky Bruce Add a hwdev argument that is needed on some architectures in order to access a per-device offset that is taken into account when producing a physical address (also needed to get from bus address to virtual address because the physical address is an intermediate step). Also make swiotlb_bus_to_virt weak so architectures can override it. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- arch/x86/kernel/pci-swiotlb.c | 2 +- include/linux/swiotlb.h | 3 ++- lib/swiotlb.c | 10 +++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c index 34f12e9..887388a 100644 --- a/arch/x86/kernel/pci-swiotlb.c +++ b/arch/x86/kernel/pci-swiotlb.c @@ -28,7 +28,7 @@ dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr) return paddr; } -phys_addr_t swiotlb_bus_to_phys(dma_addr_t baddr) +phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr) { return baddr; } diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index ac9ff54..cb1a663 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -29,7 +29,8 @@ extern void *swiotlb_alloc(unsigned order, unsigned long nslabs); extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t address); -extern phys_addr_t swiotlb_bus_to_phys(dma_addr_t address); +extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, + dma_addr_t address); extern int swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size); diff --git a/lib/swiotlb.c b/lib/swiotlb.c index c0f185a..d006f6d 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -129,7 +129,7 @@ dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr) return paddr; } -phys_addr_t __weak swiotlb_bus_to_phys(dma_addr_t baddr) +phys_addr_t __weak swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr) { return baddr; } @@ -140,9 +140,9 @@ static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev, return swiotlb_phys_to_bus(hwdev, virt_to_phys(address)); } -static void *swiotlb_bus_to_virt(dma_addr_t address) +void * __weak swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address) { - return phys_to_virt(swiotlb_bus_to_phys(address)); + return phys_to_virt(swiotlb_bus_to_phys(hwdev, address)); } int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev, @@ -692,7 +692,7 @@ static void swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr, size_t size, int dir) { - char *dma_addr = swiotlb_bus_to_virt(dev_addr); + char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr); BUG_ON(dir == DMA_NONE); @@ -729,7 +729,7 @@ static void swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr, size_t size, int dir, int target) { - char *dma_addr = swiotlb_bus_to_virt(dev_addr); + char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr); BUG_ON(dir == DMA_NONE); -- 1.5.6.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-04 1:56 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single Becky Bruce 2009-04-04 1:56 ` [PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code Becky Bruce @ 2009-04-07 2:24 ` FUJITA Tomonori 2009-04-07 6:34 ` Kumar Gala 1 sibling, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-07 2:24 UTC (permalink / raw) To: beckyb; +Cc: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell On Fri, 3 Apr 2009 20:56:47 -0500 Becky Bruce <beckyb@kernel.crashing.org> wrote: > This mirrors the current swiotlb_sync_single() setup > where the swiotlb_unmap_single() function is static to this > file and contains the logic required to determine if we need > to call actual sync_single. Previously, swiotlb_unmap_page > and swiotlb_unmap_sg were duplicating very similar code. > The duplicated code has also been reformatted for > readability. > > Note that the swiotlb_unmap_sg code was previously doing > a complicated comparison to determine if an addresses needed > to be unmapped where a simple is_swiotlb_buffer() call > would have sufficed. > > Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> > --- > lib/swiotlb.c | 36 +++++++++++++++++++++++------------- > 1 files changed, 23 insertions(+), 13 deletions(-) > > diff --git a/lib/swiotlb.c b/lib/swiotlb.c > index af2ec25..602315b 100644 > --- a/lib/swiotlb.c > +++ b/lib/swiotlb.c I don't think 'swiotlb_unmap_single' name is appropriate. swiotlb_unmap_single sounds like an exported function that IOMMUs can use (and it was) however it should not be. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 2:24 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single FUJITA Tomonori @ 2009-04-07 6:34 ` Kumar Gala 2009-04-07 9:09 ` FUJITA Tomonori 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-07 6:34 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: beckyb, linux-kernel, mingo, jeremy, ian.campbell On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: > On Fri, 3 Apr 2009 20:56:47 -0500 > Becky Bruce <beckyb@kernel.crashing.org> wrote: > >> This mirrors the current swiotlb_sync_single() setup >> where the swiotlb_unmap_single() function is static to this >> file and contains the logic required to determine if we need >> to call actual sync_single. Previously, swiotlb_unmap_page >> and swiotlb_unmap_sg were duplicating very similar code. >> The duplicated code has also been reformatted for >> readability. >> >> Note that the swiotlb_unmap_sg code was previously doing >> a complicated comparison to determine if an addresses needed >> to be unmapped where a simple is_swiotlb_buffer() call >> would have sufficed. >> >> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> >> --- >> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- >> 1 files changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c >> index af2ec25..602315b 100644 >> --- a/lib/swiotlb.c >> +++ b/lib/swiotlb.c > > I don't think 'swiotlb_unmap_single' name is appropriate. > > swiotlb_unmap_single sounds like an exported function that IOMMUs can > use (and it was) however it should not be. What do you suggest we call it? __swiotlb_unmap_single. - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 6:34 ` Kumar Gala @ 2009-04-07 9:09 ` FUJITA Tomonori 2009-04-07 15:32 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-07 9:09 UTC (permalink / raw) To: galak; +Cc: fujita.tomonori, beckyb, linux-kernel, mingo, jeremy, ian.campbell On Tue, 7 Apr 2009 01:34:44 -0500 Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: > > > On Fri, 3 Apr 2009 20:56:47 -0500 > > Becky Bruce <beckyb@kernel.crashing.org> wrote: > > > >> This mirrors the current swiotlb_sync_single() setup > >> where the swiotlb_unmap_single() function is static to this > >> file and contains the logic required to determine if we need > >> to call actual sync_single. Previously, swiotlb_unmap_page > >> and swiotlb_unmap_sg were duplicating very similar code. > >> The duplicated code has also been reformatted for > >> readability. > >> > >> Note that the swiotlb_unmap_sg code was previously doing > >> a complicated comparison to determine if an addresses needed > >> to be unmapped where a simple is_swiotlb_buffer() call > >> would have sufficed. > >> > >> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> > >> --- > >> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- > >> 1 files changed, 23 insertions(+), 13 deletions(-) > >> > >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c > >> index af2ec25..602315b 100644 > >> --- a/lib/swiotlb.c > >> +++ b/lib/swiotlb.c > > > > I don't think 'swiotlb_unmap_single' name is appropriate. > > > > swiotlb_unmap_single sounds like an exported function that IOMMUs can > > use (and it was) however it should not be. > > What do you suggest we call it? __swiotlb_unmap_single. I think that __swiotlb_unmap_single is better because the name implies that it's an internal function. It's fine by me. If it is odd that __swiotlb_unmap_single() is just a wrapper function of unmap_single(), which does the real job to unmap a dma mapping, it might be another possible option to rename unmap_single to do_unamp_single and use unmap_single. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 9:09 ` FUJITA Tomonori @ 2009-04-07 15:32 ` Kumar Gala 2009-04-07 16:37 ` FUJITA Tomonori 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-07 15:32 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: beckyb, linux-kernel, mingo, jeremy, ian.campbell On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote: > On Tue, 7 Apr 2009 01:34:44 -0500 > Kumar Gala <galak@kernel.crashing.org> wrote: > >> >> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: >> >>> On Fri, 3 Apr 2009 20:56:47 -0500 >>> Becky Bruce <beckyb@kernel.crashing.org> wrote: >>> >>>> This mirrors the current swiotlb_sync_single() setup >>>> where the swiotlb_unmap_single() function is static to this >>>> file and contains the logic required to determine if we need >>>> to call actual sync_single. Previously, swiotlb_unmap_page >>>> and swiotlb_unmap_sg were duplicating very similar code. >>>> The duplicated code has also been reformatted for >>>> readability. >>>> >>>> Note that the swiotlb_unmap_sg code was previously doing >>>> a complicated comparison to determine if an addresses needed >>>> to be unmapped where a simple is_swiotlb_buffer() call >>>> would have sufficed. >>>> >>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> >>>> --- >>>> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- >>>> 1 files changed, 23 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c >>>> index af2ec25..602315b 100644 >>>> --- a/lib/swiotlb.c >>>> +++ b/lib/swiotlb.c >>> >>> I don't think 'swiotlb_unmap_single' name is appropriate. >>> >>> swiotlb_unmap_single sounds like an exported function that IOMMUs >>> can >>> use (and it was) however it should not be. >> >> What do you suggest we call it? __swiotlb_unmap_single. > > I think that __swiotlb_unmap_single is better because the name implies > that it's an internal function. It's fine by me. > > If it is odd that __swiotlb_unmap_single() is just a wrapper function > of unmap_single(), which does the real job to unmap a dma mapping, it > might be another possible option to rename unmap_single to > do_unamp_single and use unmap_single. I think you lost me here. I'd prefer to just use __swiotlb_unmap_single at this point and get this code into the tree and work on such renaming after the fact (if that's ok). - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 15:32 ` Kumar Gala @ 2009-04-07 16:37 ` FUJITA Tomonori 2009-04-07 16:50 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-07 16:37 UTC (permalink / raw) To: galak; +Cc: fujita.tomonori, beckyb, linux-kernel, mingo, jeremy, ian.campbell On Tue, 7 Apr 2009 10:32:20 -0500 Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote: > > > On Tue, 7 Apr 2009 01:34:44 -0500 > > Kumar Gala <galak@kernel.crashing.org> wrote: > > > >> > >> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: > >> > >>> On Fri, 3 Apr 2009 20:56:47 -0500 > >>> Becky Bruce <beckyb@kernel.crashing.org> wrote: > >>> > >>>> This mirrors the current swiotlb_sync_single() setup > >>>> where the swiotlb_unmap_single() function is static to this > >>>> file and contains the logic required to determine if we need > >>>> to call actual sync_single. Previously, swiotlb_unmap_page > >>>> and swiotlb_unmap_sg were duplicating very similar code. > >>>> The duplicated code has also been reformatted for > >>>> readability. > >>>> > >>>> Note that the swiotlb_unmap_sg code was previously doing > >>>> a complicated comparison to determine if an addresses needed > >>>> to be unmapped where a simple is_swiotlb_buffer() call > >>>> would have sufficed. > >>>> > >>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> > >>>> --- > >>>> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- > >>>> 1 files changed, 23 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c > >>>> index af2ec25..602315b 100644 > >>>> --- a/lib/swiotlb.c > >>>> +++ b/lib/swiotlb.c > >>> > >>> I don't think 'swiotlb_unmap_single' name is appropriate. > >>> > >>> swiotlb_unmap_single sounds like an exported function that IOMMUs > >>> can > >>> use (and it was) however it should not be. > >> > >> What do you suggest we call it? __swiotlb_unmap_single. > > > > I think that __swiotlb_unmap_single is better because the name implies > > that it's an internal function. It's fine by me. > > > > If it is odd that __swiotlb_unmap_single() is just a wrapper function > > of unmap_single(), which does the real job to unmap a dma mapping, it > > might be another possible option to rename unmap_single to > > do_unamp_single and use unmap_single. > > I think you lost me here. I'd prefer to just use > __swiotlb_unmap_single at this point and get this code into the tree > and work on such renaming after the fact (if that's ok). If you are rushing to merge this right now, the original patchset is fine by me (I thought that you missed this merge window). I'll rename it later. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 16:37 ` FUJITA Tomonori @ 2009-04-07 16:50 ` Kumar Gala 2009-04-07 17:22 ` FUJITA Tomonori 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-07 16:50 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: beckyb, linux-kernel, mingo, jeremy, ian.campbell On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote: > On Tue, 7 Apr 2009 10:32:20 -0500 > Kumar Gala <galak@kernel.crashing.org> wrote: > >> >> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote: >> >>> On Tue, 7 Apr 2009 01:34:44 -0500 >>> Kumar Gala <galak@kernel.crashing.org> wrote: >>> >>>> >>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: >>>> >>>>> On Fri, 3 Apr 2009 20:56:47 -0500 >>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote: >>>>> >>>>>> This mirrors the current swiotlb_sync_single() setup >>>>>> where the swiotlb_unmap_single() function is static to this >>>>>> file and contains the logic required to determine if we need >>>>>> to call actual sync_single. Previously, swiotlb_unmap_page >>>>>> and swiotlb_unmap_sg were duplicating very similar code. >>>>>> The duplicated code has also been reformatted for >>>>>> readability. >>>>>> >>>>>> Note that the swiotlb_unmap_sg code was previously doing >>>>>> a complicated comparison to determine if an addresses needed >>>>>> to be unmapped where a simple is_swiotlb_buffer() call >>>>>> would have sufficed. >>>>>> >>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> >>>>>> --- >>>>>> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- >>>>>> 1 files changed, 23 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c >>>>>> index af2ec25..602315b 100644 >>>>>> --- a/lib/swiotlb.c >>>>>> +++ b/lib/swiotlb.c >>>>> >>>>> I don't think 'swiotlb_unmap_single' name is appropriate. >>>>> >>>>> swiotlb_unmap_single sounds like an exported function that IOMMUs >>>>> can >>>>> use (and it was) however it should not be. >>>> >>>> What do you suggest we call it? __swiotlb_unmap_single. >>> >>> I think that __swiotlb_unmap_single is better because the name >>> implies >>> that it's an internal function. It's fine by me. >>> >>> If it is odd that __swiotlb_unmap_single() is just a wrapper >>> function >>> of unmap_single(), which does the real job to unmap a dma mapping, >>> it >>> might be another possible option to rename unmap_single to >>> do_unamp_single and use unmap_single. >> >> I think you lost me here. I'd prefer to just use >> __swiotlb_unmap_single at this point and get this code into the tree >> and work on such renaming after the fact (if that's ok). > > If you are rushing to merge this right now, the original patchset is > fine by me (I thought that you missed this merge window). I'll rename > it later. We probably did, but one can never tell with these things. It seemed like Ingo merged and pushed some swiotlb changes late in the game for . 29 I'm still not clear on what you are suggesting... "rename unmap_single to do_unamp_single and use unmap_single". - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 16:50 ` Kumar Gala @ 2009-04-07 17:22 ` FUJITA Tomonori 2009-04-07 17:32 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-07 17:22 UTC (permalink / raw) To: galak; +Cc: fujita.tomonori, beckyb, linux-kernel, mingo, jeremy, ian.campbell On Tue, 7 Apr 2009 11:50:56 -0500 Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote: > > > On Tue, 7 Apr 2009 10:32:20 -0500 > > Kumar Gala <galak@kernel.crashing.org> wrote: > > > >> > >> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote: > >> > >>> On Tue, 7 Apr 2009 01:34:44 -0500 > >>> Kumar Gala <galak@kernel.crashing.org> wrote: > >>> > >>>> > >>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: > >>>> > >>>>> On Fri, 3 Apr 2009 20:56:47 -0500 > >>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote: > >>>>> > >>>>>> This mirrors the current swiotlb_sync_single() setup > >>>>>> where the swiotlb_unmap_single() function is static to this > >>>>>> file and contains the logic required to determine if we need > >>>>>> to call actual sync_single. Previously, swiotlb_unmap_page > >>>>>> and swiotlb_unmap_sg were duplicating very similar code. > >>>>>> The duplicated code has also been reformatted for > >>>>>> readability. > >>>>>> > >>>>>> Note that the swiotlb_unmap_sg code was previously doing > >>>>>> a complicated comparison to determine if an addresses needed > >>>>>> to be unmapped where a simple is_swiotlb_buffer() call > >>>>>> would have sufficed. > >>>>>> > >>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> > >>>>>> --- > >>>>>> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- > >>>>>> 1 files changed, 23 insertions(+), 13 deletions(-) > >>>>>> > >>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c > >>>>>> index af2ec25..602315b 100644 > >>>>>> --- a/lib/swiotlb.c > >>>>>> +++ b/lib/swiotlb.c > >>>>> > >>>>> I don't think 'swiotlb_unmap_single' name is appropriate. > >>>>> > >>>>> swiotlb_unmap_single sounds like an exported function that IOMMUs > >>>>> can > >>>>> use (and it was) however it should not be. > >>>> > >>>> What do you suggest we call it? __swiotlb_unmap_single. > >>> > >>> I think that __swiotlb_unmap_single is better because the name > >>> implies > >>> that it's an internal function. It's fine by me. > >>> > >>> If it is odd that __swiotlb_unmap_single() is just a wrapper > >>> function > >>> of unmap_single(), which does the real job to unmap a dma mapping, > >>> it > >>> might be another possible option to rename unmap_single to > >>> do_unamp_single and use unmap_single. > >> > >> I think you lost me here. I'd prefer to just use > >> __swiotlb_unmap_single at this point and get this code into the tree > >> and work on such renaming after the fact (if that's ok). > > > > If you are rushing to merge this right now, the original patchset is > > fine by me (I thought that you missed this merge window). I'll rename > > it later. > > We probably did, but one can never tell with these things. It seemed > like Ingo merged and pushed some swiotlb changes late in the game for . > 29 Well, merging patches that have not been tested linux-next late is what we should not do, I guess. I like to see Becky's patch in 2.6.30 because I have some swiotlb changes for 2.6.31 though. > I'm still not clear on what you are suggesting... "rename unmap_single > to do_unamp_single and use unmap_single". This can be applied after Becky's patchset. diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 1c86553..bffe6d7 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -482,7 +482,7 @@ found: * dma_addr is the kernel virtual address of the bounce buffer to unmap. */ static void -unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir) +do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir) { unsigned long flags; int i, count, nslots = ALIGN(size, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT; @@ -591,7 +591,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, (unsigned long long)dev_addr); /* DMA_TO_DEVICE to avoid memcpy in unmap_single */ - unmap_single(hwdev, ret, size, DMA_TO_DEVICE); + do_unmap_single(hwdev, ret, size, DMA_TO_DEVICE); return NULL; } *dma_handle = dev_addr; @@ -608,7 +608,7 @@ swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, free_pages((unsigned long) vaddr, get_order(size)); else /* DMA_TO_DEVICE to avoid memcpy in unmap_single */ - unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE); + do_unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE); } EXPORT_SYMBOL(swiotlb_free_coherent); @@ -688,16 +688,15 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page); * After this call, reads by the cpu to the buffer are guaranteed to see * whatever the device wrote there. */ -static void -swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr, - size_t size, int dir) +static void unmap_single(struct device *hwdev, dma_addr_t dev_addr, + size_t size, int dir) { char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr); BUG_ON(dir == DMA_NONE); if (is_swiotlb_buffer(dma_addr)) { - unmap_single(hwdev, dma_addr, size, dir); + do_unmap_single(hwdev, dma_addr, size, dir); return; } @@ -711,7 +710,7 @@ void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs) { - swiotlb_unmap_single(hwdev, dev_addr, size, dir); + unmap_single(hwdev, dev_addr, size, dir); } EXPORT_SYMBOL_GPL(swiotlb_unmap_page); @@ -864,8 +863,7 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl, BUG_ON(dir == DMA_NONE); for_each_sg(sgl, sg, nelems, i) - swiotlb_unmap_single(hwdev, sg->dma_address, sg->dma_length, - dir); + unmap_single(hwdev, sg->dma_address, sg->dma_length, dir); } EXPORT_SYMBOL(swiotlb_unmap_sg_attrs); ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 17:22 ` FUJITA Tomonori @ 2009-04-07 17:32 ` Kumar Gala 2009-04-07 18:18 ` FUJITA Tomonori 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-07 17:32 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: beckyb, linux-kernel, mingo, jeremy, ian.campbell On Apr 7, 2009, at 12:22 PM, FUJITA Tomonori wrote: > On Tue, 7 Apr 2009 11:50:56 -0500 > Kumar Gala <galak@kernel.crashing.org> wrote: > >> >> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote: >> >>> On Tue, 7 Apr 2009 10:32:20 -0500 >>> Kumar Gala <galak@kernel.crashing.org> wrote: >>> >>>> >>>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote: >>>> >>>>> On Tue, 7 Apr 2009 01:34:44 -0500 >>>>> Kumar Gala <galak@kernel.crashing.org> wrote: >>>>> >>>>>> >>>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: >>>>>> >>>>>>> On Fri, 3 Apr 2009 20:56:47 -0500 >>>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote: >>>>>>> >>>>>>>> This mirrors the current swiotlb_sync_single() setup >>>>>>>> where the swiotlb_unmap_single() function is static to this >>>>>>>> file and contains the logic required to determine if we need >>>>>>>> to call actual sync_single. Previously, swiotlb_unmap_page >>>>>>>> and swiotlb_unmap_sg were duplicating very similar code. >>>>>>>> The duplicated code has also been reformatted for >>>>>>>> readability. >>>>>>>> >>>>>>>> Note that the swiotlb_unmap_sg code was previously doing >>>>>>>> a complicated comparison to determine if an addresses needed >>>>>>>> to be unmapped where a simple is_swiotlb_buffer() call >>>>>>>> would have sufficed. >>>>>>>> >>>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> >>>>>>>> --- >>>>>>>> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- >>>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-) >>>>>>>> >>>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c >>>>>>>> index af2ec25..602315b 100644 >>>>>>>> --- a/lib/swiotlb.c >>>>>>>> +++ b/lib/swiotlb.c >>>>>>> >>>>>>> I don't think 'swiotlb_unmap_single' name is appropriate. >>>>>>> >>>>>>> swiotlb_unmap_single sounds like an exported function that >>>>>>> IOMMUs >>>>>>> can >>>>>>> use (and it was) however it should not be. >>>>>> >>>>>> What do you suggest we call it? __swiotlb_unmap_single. >>>>> >>>>> I think that __swiotlb_unmap_single is better because the name >>>>> implies >>>>> that it's an internal function. It's fine by me. >>>>> >>>>> If it is odd that __swiotlb_unmap_single() is just a wrapper >>>>> function >>>>> of unmap_single(), which does the real job to unmap a dma mapping, >>>>> it >>>>> might be another possible option to rename unmap_single to >>>>> do_unamp_single and use unmap_single. >>>> >>>> I think you lost me here. I'd prefer to just use >>>> __swiotlb_unmap_single at this point and get this code into the >>>> tree >>>> and work on such renaming after the fact (if that's ok). >>> >>> If you are rushing to merge this right now, the original patchset is >>> fine by me (I thought that you missed this merge window). I'll >>> rename >>> it later. >> >> We probably did, but one can never tell with these things. It seemed >> like Ingo merged and pushed some swiotlb changes late in the game >> for . >> 29 > > Well, merging patches that have not been tested linux-next late is > what we should not do, I guess. I like to see Becky's patch in 2.6.30 > because I have some swiotlb changes for 2.6.31 though. Same here. It makes it easier for us to work on the powerpc arch specific changes for .31 if we can get these into .30. What are you looking at for .31? Ingo, any comments on that? >> I'm still not clear on what you are suggesting... "rename >> unmap_single >> to do_unamp_single and use unmap_single". > > This can be applied after Becky's patchset. thanks. I'll merge this into her patch set and repost it. - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 17:32 ` Kumar Gala @ 2009-04-07 18:18 ` FUJITA Tomonori 2009-04-08 12:43 ` Ingo Molnar 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-07 18:18 UTC (permalink / raw) To: galak; +Cc: fujita.tomonori, beckyb, linux-kernel, mingo, jeremy, ian.campbell On Tue, 7 Apr 2009 12:32:12 -0500 Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 7, 2009, at 12:22 PM, FUJITA Tomonori wrote: > > > On Tue, 7 Apr 2009 11:50:56 -0500 > > Kumar Gala <galak@kernel.crashing.org> wrote: > > > >> > >> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote: > >> > >>> On Tue, 7 Apr 2009 10:32:20 -0500 > >>> Kumar Gala <galak@kernel.crashing.org> wrote: > >>> > >>>> > >>>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote: > >>>> > >>>>> On Tue, 7 Apr 2009 01:34:44 -0500 > >>>>> Kumar Gala <galak@kernel.crashing.org> wrote: > >>>>> > >>>>>> > >>>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote: > >>>>>> > >>>>>>> On Fri, 3 Apr 2009 20:56:47 -0500 > >>>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote: > >>>>>>> > >>>>>>>> This mirrors the current swiotlb_sync_single() setup > >>>>>>>> where the swiotlb_unmap_single() function is static to this > >>>>>>>> file and contains the logic required to determine if we need > >>>>>>>> to call actual sync_single. Previously, swiotlb_unmap_page > >>>>>>>> and swiotlb_unmap_sg were duplicating very similar code. > >>>>>>>> The duplicated code has also been reformatted for > >>>>>>>> readability. > >>>>>>>> > >>>>>>>> Note that the swiotlb_unmap_sg code was previously doing > >>>>>>>> a complicated comparison to determine if an addresses needed > >>>>>>>> to be unmapped where a simple is_swiotlb_buffer() call > >>>>>>>> would have sufficed. > >>>>>>>> > >>>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> > >>>>>>>> --- > >>>>>>>> lib/swiotlb.c | 36 +++++++++++++++++++++++------------- > >>>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c > >>>>>>>> index af2ec25..602315b 100644 > >>>>>>>> --- a/lib/swiotlb.c > >>>>>>>> +++ b/lib/swiotlb.c > >>>>>>> > >>>>>>> I don't think 'swiotlb_unmap_single' name is appropriate. > >>>>>>> > >>>>>>> swiotlb_unmap_single sounds like an exported function that > >>>>>>> IOMMUs > >>>>>>> can > >>>>>>> use (and it was) however it should not be. > >>>>>> > >>>>>> What do you suggest we call it? __swiotlb_unmap_single. > >>>>> > >>>>> I think that __swiotlb_unmap_single is better because the name > >>>>> implies > >>>>> that it's an internal function. It's fine by me. > >>>>> > >>>>> If it is odd that __swiotlb_unmap_single() is just a wrapper > >>>>> function > >>>>> of unmap_single(), which does the real job to unmap a dma mapping, > >>>>> it > >>>>> might be another possible option to rename unmap_single to > >>>>> do_unamp_single and use unmap_single. > >>>> > >>>> I think you lost me here. I'd prefer to just use > >>>> __swiotlb_unmap_single at this point and get this code into the > >>>> tree > >>>> and work on such renaming after the fact (if that's ok). > >>> > >>> If you are rushing to merge this right now, the original patchset is > >>> fine by me (I thought that you missed this merge window). I'll > >>> rename > >>> it later. > >> > >> We probably did, but one can never tell with these things. It seemed > >> like Ingo merged and pushed some swiotlb changes late in the game > >> for . > >> 29 > > > > Well, merging patches that have not been tested linux-next late is > > what we should not do, I guess. I like to see Becky's patch in 2.6.30 > > because I have some swiotlb changes for 2.6.31 though. > > Same here. It makes it easier for us to work on the powerpc arch > specific changes for .31 if we can get these into .30. What are you > looking at for .31? I need to finish the dma_mapping_ops cleanups and cross-arch unification stuff: http://marc.info/?l=linux-kernel&m=123827903216314&w=2 But the changes to swiotlb is minor. BTW, I have the patches for powerpc too. > Ingo, any comments on that? ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-07 18:18 ` FUJITA Tomonori @ 2009-04-08 12:43 ` Ingo Molnar 2009-04-08 13:35 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Ingo Molnar @ 2009-04-08 12:43 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, beckyb, linux-kernel, jeremy, ian.campbell * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > > Same here. It makes it easier for us to work on the powerpc arch > > specific changes for .31 if we can get these into .30. What are you > > looking at for .31? > > I need to finish the dma_mapping_ops cleanups and cross-arch > unification stuff: > > http://marc.info/?l=linux-kernel&m=123827903216314&w=2 > > But the changes to swiotlb is minor. BTW, I have the patches for > powerpc too. > > > Ingo, any comments on that? if it's genuine fixes we can do it post-rc1, but it looks a tad wide for that: arch/x86/kernel/pci-swiotlb.c | 2 +- include/linux/swiotlb.h | 3 +- lib/swiotlb.c | 115 ++++++++++++++++++++++------------------- the patches came right in the merge window - that's too late for IOMMU bits, the patch cutoff is generally the beginning of the merge window. (Which was on March 23 in this window) But i can queue them up in the .31 queue if it has all the Acks from you folks. Becky, mind resending the latest version, with all the acks in place? Thanks, Ingo ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-08 12:43 ` Ingo Molnar @ 2009-04-08 13:35 ` Kumar Gala 2009-04-08 14:05 ` Ingo Molnar 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 13:35 UTC (permalink / raw) To: Ingo Molnar; +Cc: FUJITA Tomonori, beckyb, linux-kernel, jeremy, ian.campbell On Apr 8, 2009, at 7:43 AM, Ingo Molnar wrote: > > * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > >>> Same here. It makes it easier for us to work on the powerpc arch >>> specific changes for .31 if we can get these into .30. What are you >>> looking at for .31? >> >> I need to finish the dma_mapping_ops cleanups and cross-arch >> unification stuff: >> >> http://marc.info/?l=linux-kernel&m=123827903216314&w=2 >> >> But the changes to swiotlb is minor. BTW, I have the patches for >> powerpc too. >> >>> Ingo, any comments on that? > > if it's genuine fixes we can do it post-rc1, but it looks a tad wide > for that: > > arch/x86/kernel/pci-swiotlb.c | 2 +- > include/linux/swiotlb.h | 3 +- > lib/swiotlb.c | 115 +++++++++++++++++++++ > +------------------- > > the patches came right in the merge window - that's too late for > IOMMU bits, the patch cutoff is generally the beginning of the merge > window. (Which was on March 23 in this window) Fair. > But i can queue them up in the .31 queue if it has all the Acks from > you folks. Becky, mind resending the latest version, with all the > acks in place? I can do this in Becky's abscense this week. The only "acks" I've seen are from FUJITA Tomonori. Can we ask there be a unique swiotlb branch in your tree to make it easier for us to continue additional swiotlb work as well as arch specific work. - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-08 13:35 ` Kumar Gala @ 2009-04-08 14:05 ` Ingo Molnar 2009-04-08 14:10 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Ingo Molnar @ 2009-04-08 14:05 UTC (permalink / raw) To: Kumar Gala; +Cc: FUJITA Tomonori, beckyb, linux-kernel, jeremy, ian.campbell * Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 8, 2009, at 7:43 AM, Ingo Molnar wrote: > >> >> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: >> >>>> Same here. It makes it easier for us to work on the powerpc arch >>>> specific changes for .31 if we can get these into .30. What are you >>>> looking at for .31? >>> >>> I need to finish the dma_mapping_ops cleanups and cross-arch >>> unification stuff: >>> >>> http://marc.info/?l=linux-kernel&m=123827903216314&w=2 >>> >>> But the changes to swiotlb is minor. BTW, I have the patches for >>> powerpc too. >>> >>>> Ingo, any comments on that? >> >> if it's genuine fixes we can do it post-rc1, but it looks a tad wide >> for that: >> >> arch/x86/kernel/pci-swiotlb.c | 2 +- >> include/linux/swiotlb.h | 3 +- >> lib/swiotlb.c | 115 +++++++++++++++++++++ >> +------------------- >> >> the patches came right in the merge window - that's too late for >> IOMMU bits, the patch cutoff is generally the beginning of the merge >> window. (Which was on March 23 in this window) > > Fair. > >> But i can queue them up in the .31 queue if it has all the Acks from >> you folks. Becky, mind resending the latest version, with all the >> acks in place? > > I can do this in Becky's abscense this week. The only "acks" I've > seen are from FUJITA Tomonori. _your_ ack (or signoff) counts too - so to me it's plural :-) > Can we ask there be a unique swiotlb branch in your tree to make > it easier for us to continue additional swiotlb work as well as > arch specific work. yes - tip:core/iommu is already distinct from tip:x86/iommu and is intended for such more generic patches. It has no patches pending right now (it just got all merged upstream) - so please use an upstream base - 2.6.30-rc1 would be a good starting point. Thanks, Ingo ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single 2009-04-08 14:05 ` Ingo Molnar @ 2009-04-08 14:10 ` Kumar Gala 0 siblings, 0 replies; 44+ messages in thread From: Kumar Gala @ 2009-04-08 14:10 UTC (permalink / raw) To: Ingo Molnar; +Cc: FUJITA Tomonori, beckyb, linux-kernel, jeremy, ian.campbell >>> >>> the patches came right in the merge window - that's too late for >>> IOMMU bits, the patch cutoff is generally the beginning of the merge >>> window. (Which was on March 23 in this window) >> >> Fair. >> >>> But i can queue them up in the .31 queue if it has all the Acks from >>> you folks. Becky, mind resending the latest version, with all the >>> acks in place? >> >> I can do this in Becky's abscense this week. The only "acks" I've >> seen are from FUJITA Tomonori. > > _your_ ack (or signoff) counts too - so to me it's plural :-) I've added my signoff :) >> Can we ask there be a unique swiotlb branch in your tree to make >> it easier for us to continue additional swiotlb work as well as >> arch specific work. > > yes - tip:core/iommu is already distinct from tip:x86/iommu and is > intended for such more generic patches. It has no patches pending > right now (it just got all merged upstream) - so please use an > upstream base - 2.6.30-rc1 would be a good starting point. Just reposted v3 based on 2.6.30-rc1 - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH V2 0/7] swiotlb: changes for powerpc/highmem 2009-04-04 1:56 [PATCH V2 0/7] swiotlb: changes for powerpc/highmem Becky Bruce 2009-04-04 1:56 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Becky Bruce @ 2009-04-07 2:24 ` FUJITA Tomonori 1 sibling, 0 replies; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-07 2:24 UTC (permalink / raw) To: beckyb; +Cc: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell On Fri, 3 Apr 2009 20:56:42 -0500 Becky Bruce <beckyb@kernel.crashing.org> wrote: > This is v2 of a series of fairly minor patches that get swiotlb > working on 32-bit powerpc systems with HIGHMEM, plus some cleanup > of the outdated comments in the code. I've made a couple of things > weak that ppc needs to override, and have changed the prototypes > for a couple of functions to include the hwdev pointer, which > we need to ppc to convert bus addresses to and from phys/virt > addresses. I've also fixed a build warning I've been seeing on > ppc. > > In response to commentary on the previous series, I've also > refactored the code a bit, altough I did this slightly > differently than was suggested because I noticed we could use the > new helper function in 2 places instead of one. I've reformatted > a bit of code based on commentary as well. > > I have not tested this in any way on any non-ppc platforms, > so commentary/testing from x86/ia64 folks is, once again, > greatly appreciated. > > I'm going to be offline for the next week, but will respond to > commentary as soon as I return. > > Cheers, > Becky > > diffstat: > arch/x86/kernel/pci-swiotlb.c | 2 +- > include/linux/swiotlb.h | 3 +- > lib/swiotlb.c | 115 ++++++++++++++++++++++------------------- > 3 files changed, 64 insertions(+), 56 deletions(-) I have a minor comment on 5/7 but the rest looks fine to me. ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH V3 0/7] swiotlb: changes for powerpc/highmem @ 2009-04-08 14:09 Kumar Gala 2009-04-08 14:09 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 14:09 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: beckyb This is a repost of Becky's patches from the v2 version. The only change is to incorporate feedback on "Create swiotlb_unmap_single" an to add Ack by FUJITA Tomonori for patches that have been reviewed. - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/7] swiotlb: comment corrections (no code changes) 2009-04-08 14:09 [PATCH V3 " Kumar Gala @ 2009-04-08 14:09 ` Kumar Gala 2009-04-08 14:09 ` [PATCH 2/7] swiotlb: fix compile warning Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 14:09 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: beckyb From: Becky Bruce <beckyb@kernel.crashing.org> swiotlb_map/unmap_single are now swiotlb_map/unmap_page; trivially change all the comments to reference new names. Also, there were some comments that should have been referring to just plain old map_single, not swiotlb_map_single; fix those as well. Also change a use of the word "pointer", when what is referred to is actually a dma/physical address. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> --- lib/swiotlb.c | 24 +++++++++++------------- 1 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 2b0b5a7..170cf56 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -60,8 +60,8 @@ enum dma_sync_target { int swiotlb_force; /* - * Used to do a quick range check in swiotlb_unmap_single and - * swiotlb_sync_single_*, to see if the memory was in fact allocated by this + * Used to do a quick range check in unmap_single and + * sync_single_*, to see if the memory was in fact allocated by this * API. */ static char *io_tlb_start, *io_tlb_end; @@ -560,7 +560,6 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, size)) { /* * The allocated memory isn't reachable by the device. - * Fall back on swiotlb_map_single(). */ free_pages((unsigned long) ret, order); ret = NULL; @@ -568,9 +567,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, if (!ret) { /* * We are either out of memory or the device can't DMA - * to GFP_DMA memory; fall back on - * swiotlb_map_single(), which will grab memory from - * the lowest available address range. + * to GFP_DMA memory; fall back on map_single(), which + * will grab memory from the lowest available address range. */ ret = map_single(hwdev, 0, size, DMA_FROM_DEVICE); if (!ret) @@ -634,7 +632,7 @@ swiotlb_full(struct device *dev, size_t size, int dir, int do_panic) * physical address to use is returned. * * Once the device is given the dma address, the device owns this memory until - * either swiotlb_unmap_single or swiotlb_dma_sync_single is performed. + * either swiotlb_unmap_page or swiotlb_dma_sync_single is performed. */ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, unsigned long offset, size_t size, @@ -648,7 +646,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, BUG_ON(dir == DMA_NONE); /* - * If the pointer passed in happens to be in the device's DMA window, + * If the address happens to be in the device's DMA window, * we can safely return the device addr and not worry about bounce * buffering it. */ @@ -679,7 +677,7 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page); /* * Unmap a single streaming mode DMA translation. The dma_addr and size must - * match what was provided for in a previous swiotlb_map_single call. All + * match what was provided for in a previous swiotlb_map_page call. All * other usages are undefined. * * After this call, reads by the cpu to the buffer are guaranteed to see @@ -703,7 +701,7 @@ EXPORT_SYMBOL_GPL(swiotlb_unmap_page); * Make physical memory consistent for a single streaming mode DMA translation * after a transfer. * - * If you perform a swiotlb_map_single() but wish to interrogate the buffer + * If you perform a swiotlb_map_page() but wish to interrogate the buffer * using the cpu, yet do not wish to teardown the dma mapping, you must * call this function before doing so. At the next point you give the dma * address back to the card, you must first perform a @@ -777,7 +775,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device); /* * Map a set of buffers described by scatterlist in streaming mode for DMA. - * This is the scatter-gather version of the above swiotlb_map_single + * This is the scatter-gather version of the above swiotlb_map_page * interface. Here the scatter gather list elements are each tagged with the * appropriate dma address and length. They are obtained via * sg_dma_{address,length}(SG). @@ -788,7 +786,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device); * The routine returns the number of addr/length pairs actually * used, at most nents. * - * Device ownership issues as mentioned above for swiotlb_map_single are the + * Device ownership issues as mentioned above for swiotlb_map_page are the * same here. */ int @@ -836,7 +834,7 @@ EXPORT_SYMBOL(swiotlb_map_sg); /* * Unmap a set of streaming mode DMA translations. Again, cpu read rules - * concerning calls here are the same as for swiotlb_unmap_single() above. + * concerning calls here are the same as for swiotlb_unmap_page() above. */ void swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl, -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 2/7] swiotlb: fix compile warning 2009-04-08 14:09 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Kumar Gala @ 2009-04-08 14:09 ` Kumar Gala 2009-04-08 14:09 ` [PATCH 3/7] swiotlb: map_page fix for highmem systems Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 14:09 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: beckyb From: Becky Bruce <beckyb@kernel.crashing.org> Squash a build warning seen on 32-bit powerpc caused by calling min() with 2 different types. Use min_t() instead Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> --- lib/swiotlb.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 170cf56..4fd6a76 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size, unsigned long flags; while (size) { - sz = min(PAGE_SIZE - offset, size); + sz = min_t(size_t, PAGE_SIZE - offset, size); local_irq_save(flags); buffer = kmap_atomic(pfn_to_page(pfn), -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 3/7] swiotlb: map_page fix for highmem systems 2009-04-08 14:09 ` [PATCH 2/7] swiotlb: fix compile warning Kumar Gala @ 2009-04-08 14:09 ` Kumar Gala 2009-04-08 14:09 ` [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 14:09 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: beckyb From: Becky Bruce <beckyb@kernel.crashing.org> The current code calls virt_to_phys() on address that might be in highmem, which is bad. This wasn't needed, anyway, because we already have the physical address we need. Get rid of the now-unused virtual address as well. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> --- lib/swiotlb.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 4fd6a76..e8a47c8 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -640,7 +640,6 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, struct dma_attrs *attrs) { phys_addr_t phys = page_to_phys(page) + offset; - void *ptr = page_address(page) + offset; dma_addr_t dev_addr = swiotlb_phys_to_bus(dev, phys); void *map; @@ -651,7 +650,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, * buffering it. */ if (!address_needs_mapping(dev, dev_addr, size) && - !range_needs_mapping(virt_to_phys(ptr), size)) + !range_needs_mapping(phys, size)) return dev_addr; /* -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 14:09 ` [PATCH 3/7] swiotlb: map_page fix for highmem systems Kumar Gala @ 2009-04-08 14:09 ` Kumar Gala 2009-04-08 20:38 ` Christoph Hellwig 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 14:09 UTC (permalink / raw) To: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell; +Cc: beckyb From: Becky Bruce <beckyb@kernel.crashing.org> Some architectures require additional checking to determine if a device can dma to an address and need to provide their own address_needs_mapping.. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> --- lib/swiotlb.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/swiotlb.c b/lib/swiotlb.c index e8a47c8..d81afab 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -145,6 +145,12 @@ static void *swiotlb_bus_to_virt(dma_addr_t address) return phys_to_virt(swiotlb_bus_to_phys(address)); } +int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev, + dma_addr_t addr, size_t size) +{ + return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size); +} + int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; @@ -309,10 +315,10 @@ cleanup1: return -ENOMEM; } -static int +static inline int address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size) { - return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size); + return swiotlb_arch_address_needs_mapping(hwdev, addr, size); } static inline int range_needs_mapping(phys_addr_t paddr, size_t size) -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 14:09 ` [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping Kumar Gala @ 2009-04-08 20:38 ` Christoph Hellwig 2009-04-08 20:56 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Christoph Hellwig @ 2009-04-08 20:38 UTC (permalink / raw) To: Kumar Gala Cc: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell, beckyb On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote: > From: Becky Bruce <beckyb@kernel.crashing.org> > > Some architectures require additional checking to determine > if a device can dma to an address and need to provide their > own address_needs_mapping.. Shouldn't we just move it completely to the arch? I think that ia64 and x86 currently use the same one is more of an accident. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 20:38 ` Christoph Hellwig @ 2009-04-08 20:56 ` Kumar Gala 2009-04-08 21:15 ` FUJITA Tomonori 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-08 20:56 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell, beckyb On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote: > On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote: >> From: Becky Bruce <beckyb@kernel.crashing.org> >> >> Some architectures require additional checking to determine >> if a device can dma to an address and need to provide their >> own address_needs_mapping.. > > Shouldn't we just move it completely to the arch? I think that ia64 > and > x86 currently use the same one is more of an accident. It seems like the swiotlb code uses __weak for a number of things: lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size, unsigned long nslabs) lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned long nslabs) lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr) lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr) lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address) lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev, lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size) instead of #ifndef HAVE_ARCH_<FOO>. Not sure if there is a historical reason for that. - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 20:56 ` Kumar Gala @ 2009-04-08 21:15 ` FUJITA Tomonori 2009-04-08 21:55 ` Jeremy Fitzhardinge 2009-04-08 22:24 ` Christoph Hellwig 0 siblings, 2 replies; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-08 21:15 UTC (permalink / raw) To: galak Cc: hch, linux-kernel, mingo, jeremy, fujita.tomonori, ian.campbell, beckyb On Wed, 8 Apr 2009 15:56:32 -0500 Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote: > > > On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote: > >> From: Becky Bruce <beckyb@kernel.crashing.org> > >> > >> Some architectures require additional checking to determine > >> if a device can dma to an address and need to provide their > >> own address_needs_mapping.. > > > > Shouldn't we just move it completely to the arch? I think that ia64 > > and > > x86 currently use the same one is more of an accident. > > It seems like the swiotlb code uses __weak for a number of things: > > lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size, > unsigned long nslabs) > lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned > long nslabs) > lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device > *hwdev, phys_addr_t paddr) > lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device > *hwdev, dma_addr_t baddr) > lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev, > dma_addr_t address) > lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct > device *hwdev, > lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t > paddr, size_t size) > > instead of #ifndef HAVE_ARCH_<FOO>. Not sure if there is a historical > reason for that. ia64 and x86_64 use swiotlb but neither need this function. And neither need any above __weak. They were added for dom0 support. Yeah, swiotlb is much cleaner and better if we don't add dom0 support. About this patch, I think that we could do better. What we need to do is allowing each architectures to have is_buffer_dma_capable(). I'm doing the dma_mapping_ops unification. I think that adding something like is_buffer_dma_capable to dma_map_ops struct is cleaner. Then we don't need this __weak function. But this patch is fine by me for now. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 21:15 ` FUJITA Tomonori @ 2009-04-08 21:55 ` Jeremy Fitzhardinge 2009-04-08 22:10 ` FUJITA Tomonori 2009-04-08 22:24 ` Christoph Hellwig 1 sibling, 1 reply; 44+ messages in thread From: Jeremy Fitzhardinge @ 2009-04-08 21:55 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, hch, linux-kernel, mingo, ian.campbell, beckyb FUJITA Tomonori wrote: > On Wed, 8 Apr 2009 15:56:32 -0500 > Kumar Gala <galak@kernel.crashing.org> wrote: > > >> On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote: >> >> >>> On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote: >>> >>>> From: Becky Bruce <beckyb@kernel.crashing.org> >>>> >>>> Some architectures require additional checking to determine >>>> if a device can dma to an address and need to provide their >>>> own address_needs_mapping.. >>>> >>> Shouldn't we just move it completely to the arch? I think that ia64 >>> and >>> x86 currently use the same one is more of an accident. >>> >> It seems like the swiotlb code uses __weak for a number of things: >> >> lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size, >> unsigned long nslabs) >> lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned >> long nslabs) >> lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device >> *hwdev, phys_addr_t paddr) >> lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device >> *hwdev, dma_addr_t baddr) >> lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev, >> dma_addr_t address) >> lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct >> device *hwdev, >> lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t >> paddr, size_t size) >> >> instead of #ifndef HAVE_ARCH_<FOO>. Not sure if there is a historical >> reason for that. >> > > ia64 and x86_64 use swiotlb but neither need this function. And > neither need any above __weak. They were added for dom0 support. > Yeah, swiotlb is much cleaner and better if we don't add dom0 support. > Some architectures need non-trivial bus<->phys conversion routines, etc, so either we can require it that all architectures wishing to use swiotlb define these functions, or have weak default functions that can be overridden by architectures where necessary. This isn't a specific Xen dom0 requirement, except that enabling it in the config will override these functions (but now in a Xen-only file, rather than affecting the normal x86 pci-swiotlb.c). J ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 21:55 ` Jeremy Fitzhardinge @ 2009-04-08 22:10 ` FUJITA Tomonori 2009-04-08 22:36 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-08 22:10 UTC (permalink / raw) To: jeremy Cc: fujita.tomonori, galak, hch, linux-kernel, mingo, ian.campbell, beckyb On Wed, 08 Apr 2009 14:55:55 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote: > FUJITA Tomonori wrote: > > On Wed, 8 Apr 2009 15:56:32 -0500 > > Kumar Gala <galak@kernel.crashing.org> wrote: > > > > > >> On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote: > >> > >> > >>> On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote: > >>> > >>>> From: Becky Bruce <beckyb@kernel.crashing.org> > >>>> > >>>> Some architectures require additional checking to determine > >>>> if a device can dma to an address and need to provide their > >>>> own address_needs_mapping.. > >>>> > >>> Shouldn't we just move it completely to the arch? I think that ia64 > >>> and > >>> x86 currently use the same one is more of an accident. > >>> > >> It seems like the swiotlb code uses __weak for a number of things: > >> > >> lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size, > >> unsigned long nslabs) > >> lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned > >> long nslabs) > >> lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device > >> *hwdev, phys_addr_t paddr) > >> lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device > >> *hwdev, dma_addr_t baddr) > >> lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev, > >> dma_addr_t address) > >> lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct > >> device *hwdev, > >> lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t > >> paddr, size_t size) > >> > >> instead of #ifndef HAVE_ARCH_<FOO>. Not sure if there is a historical > >> reason for that. > >> > > > > ia64 and x86_64 use swiotlb but neither need this function. And > > neither need any above __weak. They were added for dom0 support. > > Yeah, swiotlb is much cleaner and better if we don't add dom0 support. > > > > Some architectures need non-trivial bus<->phys conversion routines, etc, Only Xen needs such conversion for swiotlb. > so either we can require it that all architectures wishing to use > swiotlb define these functions, or have weak default functions that can > be overridden by architectures where necessary. Can you give an example? I don't think IA64, X86_64 or POWER (which will use swiotlb) need any __weak functions. If you say other archs could use swiotlb, please tell me how they need these __weak. > This isn't a specific Xen dom0 requirement, except that enabling it in Yes, it is. > the config will override these functions (but now in a Xen-only file, > rather than affecting the normal x86 pci-swiotlb.c). And again, x86' pci-swiotlb is much cleaner without dom0 support. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 22:10 ` FUJITA Tomonori @ 2009-04-08 22:36 ` Jeremy Fitzhardinge 2009-04-08 23:01 ` FUJITA Tomonori 0 siblings, 1 reply; 44+ messages in thread From: Jeremy Fitzhardinge @ 2009-04-08 22:36 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, hch, linux-kernel, mingo, ian.campbell, beckyb FUJITA Tomonori wrote: >> Some architectures need non-trivial bus<->phys conversion routines, etc, >> > > Only Xen needs such conversion for swiotlb. > Becky's patches of last week also added __weak annotations to swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev parameter to swiotlb_bus_to_phys; and added a weak swiotlb_arch_address_needs_mapping. I assume that was needed because powerpc needs non-trivial implementations for those functions. >> so either we can require it that all architectures wishing to use >> swiotlb define these functions, or have weak default functions that can >> be overridden by architectures where necessary. >> > > Can you give an example? I don't think IA64, X86_64 or POWER (which > will use swiotlb) need any __weak functions. If you say other archs > could use swiotlb, please tell me how they need these __weak. > As I said, Becky's patches added hooks in many of the places we added them for Xen. I assume that's because powerpc needs them; I have not seen the arch/powerpc side of those changes. J ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 22:36 ` Jeremy Fitzhardinge @ 2009-04-08 23:01 ` FUJITA Tomonori 2009-04-08 23:16 ` Jeremy Fitzhardinge 2009-04-09 4:59 ` Kumar Gala 0 siblings, 2 replies; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-08 23:01 UTC (permalink / raw) To: jeremy Cc: fujita.tomonori, galak, hch, linux-kernel, mingo, ian.campbell, beckyb On Wed, 08 Apr 2009 15:36:58 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote: > FUJITA Tomonori wrote: > >> Some architectures need non-trivial bus<->phys conversion routines, etc, > >> > > > > Only Xen needs such conversion for swiotlb. > > > > Becky's patches of last week also added __weak annotations to > swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev > parameter to swiotlb_bus_to_phys; and added a weak > swiotlb_arch_address_needs_mapping. I assume that was needed because > powerpc needs non-trivial implementations for those functions. Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We can remove these and directly use virt_to_bus and bus_to_virt. About __weak address_needs_mapping function, as I said, removing it and using dma_map_ops is a proper solution. > >> so either we can require it that all architectures wishing to use > >> swiotlb define these functions, or have weak default functions that can > >> be overridden by architectures where necessary. > >> > > > > Can you give an example? I don't think IA64, X86_64 or POWER (which > > will use swiotlb) need any __weak functions. If you say other archs > > could use swiotlb, please tell me how they need these __weak. > > > > As I said, Becky's patches added hooks in many of the places we added > them for Xen. I assume that's because powerpc needs them; I have not > seen the arch/powerpc side of those changes. > > J > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 23:01 ` FUJITA Tomonori @ 2009-04-08 23:16 ` Jeremy Fitzhardinge 2009-04-08 23:37 ` FUJITA Tomonori 2009-04-09 4:59 ` Kumar Gala 1 sibling, 1 reply; 44+ messages in thread From: Jeremy Fitzhardinge @ 2009-04-08 23:16 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, hch, linux-kernel, mingo, ian.campbell, beckyb FUJITA Tomonori wrote: >> Becky's patches of last week also added __weak annotations to >> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev >> parameter to swiotlb_bus_to_phys; and added a weak >> swiotlb_arch_address_needs_mapping. I assume that was needed because >> powerpc needs non-trivial implementations for those functions. >> > > Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We > can remove these and directly use virt_to_bus and bus_to_virt. > In general those interfaces are deprecated. Are we un-deprecating them? Or do you mean adding virt<->bus to dma_ops? > About __weak address_needs_mapping function, as I said, removing it > and using dma_map_ops is a proper solution. > Fine. Could swiotlb_alloc() just call dma_alloc_coherent() too? J ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 23:16 ` Jeremy Fitzhardinge @ 2009-04-08 23:37 ` FUJITA Tomonori 2009-04-09 0:09 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-08 23:37 UTC (permalink / raw) To: jeremy Cc: fujita.tomonori, galak, hch, linux-kernel, mingo, ian.campbell, beckyb On Wed, 08 Apr 2009 16:16:17 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote: > FUJITA Tomonori wrote: > >> Becky's patches of last week also added __weak annotations to > >> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev > >> parameter to swiotlb_bus_to_phys; and added a weak > >> swiotlb_arch_address_needs_mapping. I assume that was needed because > >> powerpc needs non-trivial implementations for those functions. > >> > > > > Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We > > can remove these and directly use virt_to_bus and bus_to_virt. > > > > In general those interfaces are deprecated. Are we un-deprecating > them? Or do you mean adding virt<->bus to dma_ops? Hmm, these interfaces are wrong for drivers surely because they can't handle dma mapping properly. However, they are exactly what swiotlb needs (swiotlb doesn't need to care about dma mapping). Until 2.6.28, swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think. > > About __weak address_needs_mapping function, as I said, removing it > > and using dma_map_ops is a proper solution. > > > > Fine. Could swiotlb_alloc() just call dma_alloc_coherent() too? I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 23:37 ` FUJITA Tomonori @ 2009-04-09 0:09 ` Jeremy Fitzhardinge 2009-04-09 4:43 ` Kumar Gala 2009-04-09 18:34 ` FUJITA Tomonori 0 siblings, 2 replies; 44+ messages in thread From: Jeremy Fitzhardinge @ 2009-04-09 0:09 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, hch, linux-kernel, mingo, ian.campbell, beckyb FUJITA Tomonori wrote: > On Wed, 08 Apr 2009 16:16:17 -0700 > Jeremy Fitzhardinge <jeremy@goop.org> wrote: > > >> FUJITA Tomonori wrote: >> >>>> Becky's patches of last week also added __weak annotations to >>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev >>>> parameter to swiotlb_bus_to_phys; and added a weak >>>> swiotlb_arch_address_needs_mapping. I assume that was needed because >>>> powerpc needs non-trivial implementations for those functions. >>>> >>>> >>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We >>> can remove these and directly use virt_to_bus and bus_to_virt. >>> >>> >> In general those interfaces are deprecated. Are we un-deprecating >> them? Or do you mean adding virt<->bus to dma_ops? >> > > Hmm, these interfaces are wrong for drivers surely because they can't > handle dma mapping properly. However, they are exactly what swiotlb > needs (swiotlb doesn't need to care about dma mapping). It needs to care about the mapping from phys to bus. On x86 they're identical, but on powerpc there can be at least an offset between them. > Until 2.6.28, > swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think. > Well, Becky's patches also added the hwdev argument to them, so presumably the powerpc implementation needs that (different devices/buses have differing views of physical memory, I guess). > I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc. > No, its something we need for Xen. I was thinking that swiotlb could allocate its memory with dma_alloc_coherent(NULL, size, ...). That would allocate via x86_fallback_device, which would not have the right behaviour (it would set GFP_DMA, for a start), and would end up hitting the uninitialized dma_ops. So the idea doesn't really work; it would need swiotlb to define another placeholder device who's alloc_coherent operation could be overridden, and it all gets pretty ugly. As an aside, I'm also wondering why there's a distinction between swiotlb_alloc() and swiotlb_alloc_boot(). The latter allocates from bootmem, but I don't see what's wrong with allocating from slab a little bit later, once it has been initialized. The comment mentions something about allocating ISA DMA memory, but the code doesn't make any attempt to allocate the buffer below 16MB (its generally much larger than 16MB anyway). J ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 0:09 ` Jeremy Fitzhardinge @ 2009-04-09 4:43 ` Kumar Gala 2009-04-09 18:34 ` FUJITA Tomonori 1 sibling, 0 replies; 44+ messages in thread From: Kumar Gala @ 2009-04-09 4:43 UTC (permalink / raw) To: Jeremy Fitzhardinge Cc: FUJITA Tomonori, hch, linux-kernel, mingo, ian.campbell, beckyb On Apr 8, 2009, at 7:09 PM, Jeremy Fitzhardinge wrote: > FUJITA Tomonori wrote: >> On Wed, 08 Apr 2009 16:16:17 -0700 >> Jeremy Fitzhardinge <jeremy@goop.org> wrote: >> >> >>> FUJITA Tomonori wrote: >>> >>>>> Becky's patches of last week also added __weak annotations to >>>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the >>>>> hwdev parameter to swiotlb_bus_to_phys; and added a weak >>>>> swiotlb_arch_address_needs_mapping. I assume that was needed >>>>> because powerpc needs non-trivial implementations for those >>>>> functions. >>>>> >>>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We >>>> can remove these and directly use virt_to_bus and bus_to_virt. >>>> >>> In general those interfaces are deprecated. Are we un-deprecating >>> them? Or do you mean adding virt<->bus to dma_ops? >>> >> >> Hmm, these interfaces are wrong for drivers surely because they can't >> handle dma mapping properly. However, they are exactly what swiotlb >> needs (swiotlb doesn't need to care about dma mapping). > > It needs to care about the mapping from phys to bus. On x86 they're > identical, but on powerpc there can be at least an offset between > them. > >> Until 2.6.28, >> swiotlb has used them. They are with IA64, X86_64 and PPC_32, I >> think. >> > > Well, Becky's patches also added the hwdev argument to them, so > presumably the powerpc implementation needs that (different devices/ > buses have differing views of physical memory, I guess). On powerpc we need the hwdev because things vary based on bus. For our SoC chips we don't need any mapping between phys & bus. However something like PCI does have a mapping (a simple offset). - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 0:09 ` Jeremy Fitzhardinge 2009-04-09 4:43 ` Kumar Gala @ 2009-04-09 18:34 ` FUJITA Tomonori 2009-04-09 19:19 ` Jeremy Fitzhardinge 1 sibling, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-09 18:34 UTC (permalink / raw) To: jeremy Cc: fujita.tomonori, galak, hch, linux-kernel, mingo, ian.campbell, beckyb On Wed, 08 Apr 2009 17:09:00 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote: > FUJITA Tomonori wrote: > > On Wed, 08 Apr 2009 16:16:17 -0700 > > Jeremy Fitzhardinge <jeremy@goop.org> wrote: > > > > > >> FUJITA Tomonori wrote: > >> > >>>> Becky's patches of last week also added __weak annotations to > >>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev > >>>> parameter to swiotlb_bus_to_phys; and added a weak > >>>> swiotlb_arch_address_needs_mapping. I assume that was needed because > >>>> powerpc needs non-trivial implementations for those functions. > >>>> > >>>> > >>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We > >>> can remove these and directly use virt_to_bus and bus_to_virt. > >>> > >>> > >> In general those interfaces are deprecated. Are we un-deprecating > >> them? Or do you mean adding virt<->bus to dma_ops? > >> > > > > Hmm, these interfaces are wrong for drivers surely because they can't > > handle dma mapping properly. However, they are exactly what swiotlb > > needs (swiotlb doesn't need to care about dma mapping). > > It needs to care about the mapping from phys to bus. On x86 they're > identical, but on powerpc there can be at least an offset between them. > > > Until 2.6.28, > > swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think. > > > > Well, Becky's patches also added the hwdev argument to them, so > presumably the powerpc implementation needs that (different > devices/buses have differing views of physical memory, I guess). Until I see the ppc specific swiotlb patchset, I'm not sure but I think that we can remove phys_to_bus in swiotlb. Even if we need phys_to_bus, we can remove the rest of __weak tricks for only dom0. And we can make phys_to_bus arch-specific. Then we don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0 support adds many hacks to swiotlb. > > I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc. > > > > No, its something we need for Xen. I was thinking that swiotlb could > allocate its memory with dma_alloc_coherent(NULL, size, ...). That > would allocate via x86_fallback_device, which would not have the right > behaviour (it would set GFP_DMA, for a start), and would end up hitting > the uninitialized dma_ops. So the idea doesn't really work; it would > need swiotlb to define another placeholder device who's alloc_coherent > operation could be overridden, and it all gets pretty ugly. It doesn't work. And swiotlb's alloc_coherent needs to be arch-specific because the page allocator can't handle dma mask. > As an aside, I'm also wondering why there's a distinction between > swiotlb_alloc() and swiotlb_alloc_boot(). The latter allocates from > bootmem, but I don't see what's wrong with allocating from slab a little > bit later, once it has been initialized. The comment mentions something > about allocating ISA DMA memory, but the code doesn't make any attempt > to allocate the buffer below 16MB (its generally much larger than 16MB > anyway). Yeah, ISA DMA comment is misleading. swiotlb can't handle it. And it doesn't need to handle it because the block layer can thanks to the bouncing (the network layer does the similar, I think). As you said, we could remove the latter though I'm not sure. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 18:34 ` FUJITA Tomonori @ 2009-04-09 19:19 ` Jeremy Fitzhardinge 2009-04-09 19:43 ` FUJITA Tomonori 2009-04-09 19:50 ` FUJITA Tomonori 0 siblings, 2 replies; 44+ messages in thread From: Jeremy Fitzhardinge @ 2009-04-09 19:19 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, hch, linux-kernel, mingo, ian.campbell, beckyb FUJITA Tomonori wrote: >> Well, Becky's patches also added the hwdev argument to them, so >> presumably the powerpc implementation needs that (different >> devices/buses have differing views of physical memory, I guess). >> > > Until I see the ppc specific swiotlb patchset, I'm not sure but I > think that we can remove phys_to_bus in swiotlb. > Kumar's comment was: "For our SoC chips we don't need any mapping between phys & bus. However something like PCI does have a mapping (a simple offset)." Kumar, could a single system have different phys<->bus mappings on a single system, or could it differ from device to device (or bus to bus)? > Even if we need phys_to_bus, we can remove the rest of __weak tricks > for only dom0. And we can make phys_to_bus arch-specific. Then we > don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0 > support adds many hacks to swiotlb. > Well, we'd still need a way to do hook the swiotlb_alloc(_boot) allocation. At the moment its effectively arch-specific because x86 only uses swiotlb_alloc_boot(), and ia64 only uses swiotlb_alloc(). One option would be to simply make that function arch-defined, which would remove the need for any kind of override mechanism in lib/swiotlb; that would match the handling of phys_to_bus. And its more appealing if we manage to drop swiotlb_alloc_boot, so there's only a single function for the arches to worry about. > Yeah, ISA DMA comment is misleading. swiotlb can't handle it. And it > doesn't need to handle it because the block layer can thanks to > the bouncing (the network layer does the similar, I think). > > As you said, we could remove the latter though I'm not sure. > It would take a bit of rearranging the x86 swiotlb/iommu init sequence, but I don't think it would be too complex. I'll look into it. J ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 19:19 ` Jeremy Fitzhardinge @ 2009-04-09 19:43 ` FUJITA Tomonori 2009-04-09 19:50 ` FUJITA Tomonori 1 sibling, 0 replies; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-09 19:43 UTC (permalink / raw) To: jeremy Cc: fujita.tomonori, galak, hch, linux-kernel, mingo, ian.campbell, beckyb On Thu, 09 Apr 2009 12:19:19 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote: > FUJITA Tomonori wrote: > >> Well, Becky's patches also added the hwdev argument to them, so > >> presumably the powerpc implementation needs that (different > >> devices/buses have differing views of physical memory, I guess). > >> > > > > Until I see the ppc specific swiotlb patchset, I'm not sure but I > > think that we can remove phys_to_bus in swiotlb. > > > > Kumar's comment was: "For our SoC chips we don't need any mapping > between phys & bus. However something like PCI does have a mapping (a > simple offset)." I meant that swiotlb doesn't need to use phys_to_bus stuff. But I as said, I'm not sure until I see the ppc swiotlb code. > Kumar, could a single system have different phys<->bus mappings on a > single system, or could it differ from device to device (or bus to bus)? > > > Even if we need phys_to_bus, we can remove the rest of __weak tricks > > for only dom0. And we can make phys_to_bus arch-specific. Then we > > don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0 > > support adds many hacks to swiotlb. > > > > Well, we'd still need a way to do hook the swiotlb_alloc(_boot) > allocation. At the moment its effectively arch-specific because x86 > only uses swiotlb_alloc_boot(), and ia64 only uses swiotlb_alloc(). One > option would be to simply make that function arch-defined, which would > remove the need for any kind of override mechanism in lib/swiotlb; that > would match the handling of phys_to_bus. And its more appealing if we > manage to drop swiotlb_alloc_boot, so there's only a single function for > the arches to worry about. > > > Yeah, ISA DMA comment is misleading. swiotlb can't handle it. And it > > doesn't need to handle it because the block layer can thanks to > > the bouncing (the network layer does the similar, I think). > > > > As you said, we could remove the latter though I'm not sure. > > > > It would take a bit of rearranging the x86 swiotlb/iommu init sequence, > but I don't think it would be too complex. I'll look into it. Unfortunately, not that simple. IA64_64 and x86 share the iommu init sequence. So you need to look at both. I put some cleanups on it in 30-rc1. But I need to do more cleanups. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 19:19 ` Jeremy Fitzhardinge 2009-04-09 19:43 ` FUJITA Tomonori @ 2009-04-09 19:50 ` FUJITA Tomonori 2009-04-09 19:54 ` Jeremy Fitzhardinge 1 sibling, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-09 19:50 UTC (permalink / raw) To: jeremy Cc: fujita.tomonori, galak, hch, linux-kernel, mingo, ian.campbell, beckyb On Thu, 09 Apr 2009 12:19:19 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote: > FUJITA Tomonori wrote: > >> Well, Becky's patches also added the hwdev argument to them, so > >> presumably the powerpc implementation needs that (different > >> devices/buses have differing views of physical memory, I guess). > >> > > > > Until I see the ppc specific swiotlb patchset, I'm not sure but I > > think that we can remove phys_to_bus in swiotlb. > > > > Kumar's comment was: "For our SoC chips we don't need any mapping > between phys & bus. However something like PCI does have a mapping (a > simple offset)." > > Kumar, could a single system have different phys<->bus mappings on a > single system, or could it differ from device to device (or bus to bus)? > > > Even if we need phys_to_bus, we can remove the rest of __weak tricks > > for only dom0. And we can make phys_to_bus arch-specific. Then we > > don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0 > > support adds many hacks to swiotlb. > > > > Well, we'd still need a way to do hook the swiotlb_alloc(_boot) > allocation. At the moment its effectively arch-specific because x86 > only uses swiotlb_alloc_boot(), and ia64 only uses swiotlb_alloc(). One Hmm, I think that ia64 use swiotlb_alloc_boot too. ia64 uses swiotlb in two ways; using only swiotlb, using swiotlb and hw iommu. The latter is similar what x86 Calgary does. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 19:50 ` FUJITA Tomonori @ 2009-04-09 19:54 ` Jeremy Fitzhardinge 0 siblings, 0 replies; 44+ messages in thread From: Jeremy Fitzhardinge @ 2009-04-09 19:54 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: galak, hch, linux-kernel, mingo, ian.campbell, beckyb FUJITA Tomonori wrote: > Hmm, I think that ia64 use swiotlb_alloc_boot too. > > ia64 uses swiotlb in two ways; using only swiotlb, using swiotlb and > hw iommu. The latter is similar what x86 Calgary does. > Yes, I see. It also calls swiotlb_init() -> swiotlb_init_with_default_size(), though I got lost in the machvec/platform setup, so I couldn't quite work out where platform_dma_init() gets called from. But I couldn't see any x86 references to swiotlb_late_init_with_default_size(). J ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 23:01 ` FUJITA Tomonori 2009-04-08 23:16 ` Jeremy Fitzhardinge @ 2009-04-09 4:59 ` Kumar Gala 2009-04-09 18:50 ` FUJITA Tomonori 1 sibling, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-09 4:59 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: jeremy, hch, linux-kernel, mingo, ian.campbell, beckyb On Apr 8, 2009, at 6:01 PM, FUJITA Tomonori wrote: >> Becky's patches of last week also added __weak annotations to >> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev >> parameter to swiotlb_bus_to_phys; and added a weak >> swiotlb_arch_address_needs_mapping. I assume that was needed because >> powerpc needs non-trivial implementations for those functions. > > Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We > can remove these and directly use virt_to_bus and bus_to_virt. > > About __weak address_needs_mapping function, as I said, removing it > and using dma_map_ops is a proper solution. Is this something you are looking at doing in the .31 timeframe? I'm looking at the fact that we need to switch over to using struct dma_map_ops on ppc. (I'm guessing this might be the patches you mentioned the other day). If so did you add set_dma_mask() to the generic dma_map_ops? - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 4:59 ` Kumar Gala @ 2009-04-09 18:50 ` FUJITA Tomonori 2009-04-09 20:10 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: FUJITA Tomonori @ 2009-04-09 18:50 UTC (permalink / raw) To: galak Cc: fujita.tomonori, jeremy, hch, linux-kernel, mingo, ian.campbell, beckyb On Wed, 8 Apr 2009 23:59:18 -0500 Kumar Gala <galak@kernel.crashing.org> wrote: > > On Apr 8, 2009, at 6:01 PM, FUJITA Tomonori wrote: > > >> Becky's patches of last week also added __weak annotations to > >> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev > >> parameter to swiotlb_bus_to_phys; and added a weak > >> swiotlb_arch_address_needs_mapping. I assume that was needed because > >> powerpc needs non-trivial implementations for those functions. > > > > Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We > > can remove these and directly use virt_to_bus and bus_to_virt. > > > > About __weak address_needs_mapping function, as I said, removing it > > and using dma_map_ops is a proper solution. > > Is this something you are looking at doing in the .31 timeframe? > > I'm looking at the fact that we need to switch over to using struct > dma_map_ops on ppc. (I'm guessing this might be the patches you > mentioned the other day). If so did you add set_dma_mask() to the > generic dma_map_ops? Yeah, I'll send patches to convert ppc to use dma_map_ops. In .31 timeframe, I plan to: - add a generic dma-mapping.h and convert ia64, x86, and ppc to use it - clean up swiotlb. - try to convert archs supporting multiple dma ops to use dma_map_ops - rewrite ia64 and x86 dma ops initialization BTW, the ppc specific swiotlb patchset is available? ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 18:50 ` FUJITA Tomonori @ 2009-04-09 20:10 ` Kumar Gala 2009-04-09 20:25 ` Kumar Gala 0 siblings, 1 reply; 44+ messages in thread From: Kumar Gala @ 2009-04-09 20:10 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: jeremy, hch, linux-kernel, mingo, ian.campbell, beckyb On Apr 9, 2009, at 1:50 PM, FUJITA Tomonori wrote: > On Wed, 8 Apr 2009 23:59:18 -0500 > Kumar Gala <galak@kernel.crashing.org> wrote: > >> >> On Apr 8, 2009, at 6:01 PM, FUJITA Tomonori wrote: >> >>>> Becky's patches of last week also added __weak annotations to >>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev >>>> parameter to swiotlb_bus_to_phys; and added a weak >>>> swiotlb_arch_address_needs_mapping. I assume that was needed >>>> because >>>> powerpc needs non-trivial implementations for those functions. >>> >>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We >>> can remove these and directly use virt_to_bus and bus_to_virt. >>> >>> About __weak address_needs_mapping function, as I said, removing it >>> and using dma_map_ops is a proper solution. >> >> Is this something you are looking at doing in the .31 timeframe? >> >> I'm looking at the fact that we need to switch over to using struct >> dma_map_ops on ppc. (I'm guessing this might be the patches you >> mentioned the other day). If so did you add set_dma_mask() to the >> generic dma_map_ops? > > Yeah, I'll send patches to convert ppc to use dma_map_ops. In .31 > timeframe, I plan to: > > - add a generic dma-mapping.h and convert ia64, x86, and ppc to use it > > - clean up swiotlb. > > - try to convert archs supporting multiple dma ops to use dma_map_ops > > - rewrite ia64 and x86 dma ops initialization > > > BTW, the ppc specific swiotlb patchset is available? Still cleaning it up, but I'll post a WIP patch so people have a sense of what the changes look like. - k ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-09 20:10 ` Kumar Gala @ 2009-04-09 20:25 ` Kumar Gala 0 siblings, 0 replies; 44+ messages in thread From: Kumar Gala @ 2009-04-09 20:25 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: jeremy, hch, linux-kernel, mingo, ian.campbell, beckyb Here's the WIP patch for the PPC changes to give some sense of what we need out of swiotlb for PPC. - k --- arch/powerpc/include/asm/dma-mapping.h | 18 ++++ arch/powerpc/include/asm/scatterlist.h | 6 +- arch/powerpc/include/asm/swiotlb.h | 15 +++ arch/powerpc/kernel/Makefile | 1 + arch/powerpc/kernel/dma-swiotlb.c | 154 ++++++++++++++++++++++++++++ arch/powerpc/kernel/dma.c | 2 +- arch/powerpc/kernel/pci-common.c | 2 + arch/powerpc/kernel/setup_32.c | 4 + 10 files changed, 211 insertions(+), 7 deletions(-) create mode 100644 arch/powerpc/include/asm/swiotlb.h create mode 100644 arch/powerpc/kernel/dma-swiotlb.c diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h index c69f2b5..db6edfe 100644 --- a/arch/powerpc/include/asm/dma-mapping.h +++ b/arch/powerpc/include/asm/dma-mapping.h @@ -16,8 +16,18 @@ #include <linux/dma-attrs.h> #include <asm/io.h> +#ifdef CONFIG_SWIOTLB +#include <asm/swiotlb.h> +#endif + #define DMA_ERROR_CODE (~(dma_addr_t)0x0) +/* Some dma direct funcs must be visible for use in other dma_ops */ +extern void *dma_direct_alloc_coherent(struct device *dev, size_t size, + dma_addr_t *dma_handle, gfp_t flag); +extern void dma_direct_free_coherent(struct device *dev, size_t size, + void *vaddr, dma_addr_t dma_handle); + #ifdef CONFIG_NOT_COHERENT_CACHE /* * DMA-consistent mapping functions for PowerPCs that don't support @@ -112,11 +122,19 @@ static inline struct dma_mapping_ops *get_dma_ops(struct device *dev) if (unlikely(dev == NULL)) return NULL; + /* BGILL - temp code to look for problems */ + if(dev->archdata.dma_ops == NULL) + printk(KERN_EMERG "dev %s has null dma_ops\n", dev->bus->name); + return dev->archdata.dma_ops; } static inline void set_dma_ops(struct device *dev, struct dma_mapping_ops *ops) { + printk(KERN_EMERG "Setting dma_ops to %s\n", + (ops == &dma_direct_ops) ? "dma_direct_ops" : + ((ops == &swiotlb_dma_ops) ? "swiotlb_dma_ops" : "unknown")); + dev->archdata.dma_ops = ops; } diff --git a/arch/powerpc/include/asm/scatterlist.h b/arch/powerpc/include/asm/scatterlist.h index fcf7d55..912bf59 100644 --- a/arch/powerpc/include/asm/scatterlist.h +++ b/arch/powerpc/include/asm/scatterlist.h @@ -21,7 +21,7 @@ struct scatterlist { unsigned int offset; unsigned int length; - /* For TCE support */ + /* For TCE or SWIOTLB support */ dma_addr_t dma_address; u32 dma_length; }; @@ -34,11 +34,7 @@ struct scatterlist { * is 0. */ #define sg_dma_address(sg) ((sg)->dma_address) -#ifdef __powerpc64__ #define sg_dma_len(sg) ((sg)->dma_length) -#else -#define sg_dma_len(sg) ((sg)->length) -#endif #ifdef __powerpc64__ #define ISA_DMA_THRESHOLD (~0UL) diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/include/asm/swiotlb.h new file mode 100644 index 0000000..cbfce6c --- /dev/null +++ b/arch/powerpc/include/asm/swiotlb.h @@ -0,0 +1,15 @@ +#ifndef __ASM_SWIOTLB_H +#define __ASM_SWIOTLB_H + +#include <linux/swiotlb.h> + +extern int swiotlb_force; +extern int swiotlb; +extern struct dma_mapping_ops swiotlb_dma_ops; + +int swiotlb_arch_address_needs_mapping(struct device *, dma_addr_t, + size_t size); + +static inline void dma_mark_clean(void *addr, size_t size) {} + +#endif /* __ASM_SWIOTLB_H */ diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index 71901fb..34c0a95 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -82,6 +82,7 @@ obj-$(CONFIG_SMP) += smp.o obj-$(CONFIG_KPROBES) += kprobes.o obj-$(CONFIG_PPC_UDBG_16550) += legacy_serial.o udbg_16550.o obj-$(CONFIG_STACKTRACE) += stacktrace.o +obj-$(CONFIG_SWIOTLB) += dma-swiotlb.o pci64-$(CONFIG_PPC64) += pci_dn.o isa-bridge.o obj-$(CONFIG_PCI) += pci_$(CONFIG_WORD_SIZE).o $(pci64-y) \ diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/dma-swiotlb.c new file mode 100644 index 0000000..639f3bc --- /dev/null +++ b/arch/powerpc/kernel/dma-swiotlb.c @@ -0,0 +1,154 @@ +/* + * Copyright (C) 2009 Becky Bruce, Freescale Semiconductor + * + * swiotlb dma ops and functions required by the swiotlb code. + */ + +#include <linux/dma-mapping.h> +#include <linux/pfn.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +#include <asm/machdep.h> +#include <asm/swiotlb.h> +#include <asm/dma.h> +#include <asm/abs_addr.h> + +int swiotlb __read_mostly; + +unsigned long get_dma_direct_offset(struct device *dev); + +void *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t addr) +{ + unsigned long pfn = PFN_DOWN(swiotlb_bus_to_phys(hwdev, addr)); + void *pageaddr = page_address(pfn_to_page(pfn)); + + if(pageaddr != NULL) + return pageaddr + (addr % PAGE_SIZE); + return NULL; +} + +#if 0 /* BGILL - don't need */ +/* This can only be called on pages with a kernel mapping */ +dma_addr_t swiotlb_virt_to_bus(struct device *hwdev, void *addr) +{ + return swiotlb_phys_to_bus(hwdev, virt_to_abs((unsigned long)addr)); +} +#endif + +dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr) +{ + return (paddr + get_dma_direct_offset(hwdev)); +} + +phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr) + +{ + return (baddr - get_dma_direct_offset(hwdev)); +} + +/* + * Eventually, we should really be looking at some per-device + * quantity stored in archdata, and not a global value, since this is + * only needed for devices like PCI that use part of their 32 bit address + * space for something other than mapping memory. + * + * For now, require swiotlb mapping above MAX_32B_DIRECT_DMA_ADDR for devs + * that are not 64-bit capable. This should be determined using the PCI mem + * allocations in the devtree. -beckyb + */ +#define MAX_32B_DIRECT_DMA_ADDR 0x80000000 + +/* determine if an address needs bounce buffering via swiotlb. */ +int +swiotlb_arch_address_needs_mapping(struct device *hwdev, dma_addr_t addr, + size_t size) +{ + dma_addr_t mask = DMA_BIT_MASK(32); + + /* Max dma_address we can access without bounce buffering */ + dma_addr_t max = swiotlb_phys_to_bus(hwdev, MAX_32B_DIRECT_DMA_ADDR); + + /* BGILL - can we get rid of this? */ + if (hwdev && hwdev->dma_mask) + mask = *hwdev->dma_mask; + + if ((mask <= DMA_BIT_MASK(36)) && (addr + size > max)) + return 1; + + return !is_buffer_dma_capable(mask, addr, size); +} + +/* + * At the moment, all platforms that use this code only require + * swiotlb to be used if we're operating on HIGHMEM. Since + * we don't ever call anything other than map_sg, unmap_sg, + * map_page, and unmap_page on highmem, use normal dma_ops + * for everything else. + */ +struct dma_mapping_ops swiotlb_dma_ops = { +#if 0 + .alloc_coherent = dma_direct_alloc_coherent, + .free_coherent = dma_direct_free_coherent, +#else + .alloc_coherent = swiotlb_alloc_coherent, + .free_coherent = swiotlb_free_coherent, +#endif + .map_sg = swiotlb_map_sg_attrs, + .unmap_sg = swiotlb_unmap_sg_attrs, + .dma_supported = swiotlb_dma_supported, + .map_page = swiotlb_map_page, + .unmap_page = swiotlb_unmap_page, + .sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu, + .sync_single_range_for_device = swiotlb_sync_single_range_for_device, + .sync_sg_for_cpu = swiotlb_sync_sg_for_cpu, + .sync_sg_for_device = swiotlb_sync_sg_for_device +}; + +static int ppc_swiotlb_bus_notify(struct notifier_block *nb, + unsigned long action, void *data) +{ + struct device *dev = data; + + /* We are only intereted in device addition */ + if (action != BUS_NOTIFY_ADD_DEVICE) + return 0; + +#if 0 /* BGILL - should look like this; use other form for debug */ + if(dma_get_mask(dev) < DMA_BIT_MASK(36)) + set_dma_ops(dev, &swiotlb_dma_ops); +#else + if(!dev->dma_mask) { + printk(KERN_EMERG "ERROR: no dma_mask set for dev\n"); + set_dma_ops(dev, &swiotlb_dma_ops); + } + else if(*dev->dma_mask < DMA_BIT_MASK(36)) { + set_dma_ops(dev, &swiotlb_dma_ops); + } + else + printk(KERN_EMERG "ERROR; fall thru with direct_ops\n"); +#endif + + return NOTIFY_DONE; +} + +static struct notifier_block ppc_swiotlb_plat_bus_notifier = { + .notifier_call = ppc_swiotlb_bus_notify, + .priority = 0, +}; + +static struct notifier_block ppc_swiotlb_of_bus_notifier = { + .notifier_call = ppc_swiotlb_bus_notify, + .priority = 0, +}; + +static int __init setup_bus_notifier(void) +{ + bus_register_notifier(&platform_bus_type, &ppc_swiotlb_plat_bus_notifier); + bus_register_notifier(&of_platform_bus_type, &ppc_swiotlb_of_bus_notifier); + + return 0; +} + +machine_arch_initcall(mpc86xx_hpcn, setup_bus_notifier); + diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c index 53c7788..62d80c4 100644 --- a/arch/powerpc/kernel/dma.c +++ b/arch/powerpc/kernel/dma.c @@ -19,7 +19,7 @@ * default the offset is PCI_DRAM_OFFSET. */ -static unsigned long get_dma_direct_offset(struct device *dev) +unsigned long get_dma_direct_offset(struct device *dev) { if (dev) return (unsigned long)dev->archdata.dma_data; diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index 9e1ca74..f038b13 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -332,6 +332,10 @@ void __init setup_arch(char **cmdline_p) ppc_md.setup_arch(); if ( ppc_md.progress ) ppc_md.progress("arch: exit", 0x3eab); + /* Allow iotlb to do it's setup */ +#ifdef CONFIG_SWIOTLB + swiotlb_init(); +#endif paging_init(); /* Initialize the MMU context management stuff */ ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping 2009-04-08 21:15 ` FUJITA Tomonori 2009-04-08 21:55 ` Jeremy Fitzhardinge @ 2009-04-08 22:24 ` Christoph Hellwig 1 sibling, 0 replies; 44+ messages in thread From: Christoph Hellwig @ 2009-04-08 22:24 UTC (permalink / raw) To: FUJITA Tomonori Cc: galak, hch, linux-kernel, mingo, jeremy, ian.campbell, beckyb On Thu, Apr 09, 2009 at 06:15:07AM +0900, FUJITA Tomonori wrote: > ia64 and x86_64 use swiotlb but neither need this function. And > neither need any above __weak. They were added for dom0 support. > Yeah, swiotlb is much cleaner and better if we don't add dom0 support. Agreed. All these dom0 support patches look like a cat has been barfing over the source code. ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2009-04-09 20:26 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-04-04 1:56 [PATCH V2 0/7] swiotlb: changes for powerpc/highmem Becky Bruce 2009-04-04 1:56 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Becky Bruce 2009-04-04 1:56 ` [PATCH 2/7] swiotlb: fix compile warning Becky Bruce 2009-04-04 1:56 ` [PATCH 3/7] swiotlb: map_page fix for highmem systems Becky Bruce 2009-04-04 1:56 ` [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping Becky Bruce 2009-04-04 1:56 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single Becky Bruce 2009-04-04 1:56 ` [PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code Becky Bruce 2009-04-04 1:56 ` [PATCH 7/7] swiotlb: Change swiotlb_bus_to[phys,virt] prototypes Becky Bruce 2009-04-07 2:24 ` [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single FUJITA Tomonori 2009-04-07 6:34 ` Kumar Gala 2009-04-07 9:09 ` FUJITA Tomonori 2009-04-07 15:32 ` Kumar Gala 2009-04-07 16:37 ` FUJITA Tomonori 2009-04-07 16:50 ` Kumar Gala 2009-04-07 17:22 ` FUJITA Tomonori 2009-04-07 17:32 ` Kumar Gala 2009-04-07 18:18 ` FUJITA Tomonori 2009-04-08 12:43 ` Ingo Molnar 2009-04-08 13:35 ` Kumar Gala 2009-04-08 14:05 ` Ingo Molnar 2009-04-08 14:10 ` Kumar Gala 2009-04-07 2:24 ` [PATCH V2 0/7] swiotlb: changes for powerpc/highmem FUJITA Tomonori 2009-04-08 14:09 [PATCH V3 " Kumar Gala 2009-04-08 14:09 ` [PATCH 1/7] swiotlb: comment corrections (no code changes) Kumar Gala 2009-04-08 14:09 ` [PATCH 2/7] swiotlb: fix compile warning Kumar Gala 2009-04-08 14:09 ` [PATCH 3/7] swiotlb: map_page fix for highmem systems Kumar Gala 2009-04-08 14:09 ` [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping Kumar Gala 2009-04-08 20:38 ` Christoph Hellwig 2009-04-08 20:56 ` Kumar Gala 2009-04-08 21:15 ` FUJITA Tomonori 2009-04-08 21:55 ` Jeremy Fitzhardinge 2009-04-08 22:10 ` FUJITA Tomonori 2009-04-08 22:36 ` Jeremy Fitzhardinge 2009-04-08 23:01 ` FUJITA Tomonori 2009-04-08 23:16 ` Jeremy Fitzhardinge 2009-04-08 23:37 ` FUJITA Tomonori 2009-04-09 0:09 ` Jeremy Fitzhardinge 2009-04-09 4:43 ` Kumar Gala 2009-04-09 18:34 ` FUJITA Tomonori 2009-04-09 19:19 ` Jeremy Fitzhardinge 2009-04-09 19:43 ` FUJITA Tomonori 2009-04-09 19:50 ` FUJITA Tomonori 2009-04-09 19:54 ` Jeremy Fitzhardinge 2009-04-09 4:59 ` Kumar Gala 2009-04-09 18:50 ` FUJITA Tomonori 2009-04-09 20:10 ` Kumar Gala 2009-04-09 20:25 ` Kumar Gala 2009-04-08 22:24 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).