This is 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.
I have not tested this in any way on any non-ppc platforms,
so commentary/testing from x86/ia64 folks is greatly
appreciated.
diffstat:
arch/x86/kernel/pci-swiotlb.c | 2 +-
include/linux/swiotlb.h | 4 ++-
lib/swiotlb.c | 70 +++++++++++++++++++++++-----------------
3 files changed, 44 insertions(+), 32 deletions(-)
Cheers,
Becky
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 <[email protected]>
---
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
Squash a build warning seen on 32-bit powerpc caused by calling min()
with 2 different types. Cast the first arg to size_t, which is the
type of the second, and should be portable across architectures.
Signed-off-by: Becky Bruce <[email protected]>
---
lib/swiotlb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index f59cf30..62f5f75 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((size_t)(PAGE_SIZE - offset), size);
local_irq_save(flags);
buffer = kmap_atomic(pfn_to_page(pfn),
--
1.5.6.6
Make these functions take the hwdev as an argument because on some
platforms it contains a per-device offset that is used to convert
from bus addresses to/from other types of addresses.
Also, make these weak so architectures can override the default
behavior (for example, by adding an offset in the hwdev).
Signed-off-by: Becky Bruce <[email protected]>
---
arch/x86/kernel/pci-swiotlb.c | 2 +-
include/linux/swiotlb.h | 4 +++-
lib/swiotlb.c | 31 +++++++++++++++++++------------
3 files changed, 23 insertions(+), 14 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..a27bca3 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -29,7 +29,9 @@ 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 void *swiotlb_bus_to_virt(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 62f5f75..c152f17 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -129,20 +129,20 @@ 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;
}
-static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
+dma_addr_t __weak swiotlb_virt_to_bus(struct device *hwdev,
volatile void *address)
{
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_range_needs_mapping(phys_addr_t paddr, size_t size)
@@ -687,7 +687,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)
{
- char *dma_addr = swiotlb_bus_to_virt(dev_addr);
+ char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
if (is_swiotlb_buffer(dma_addr))
@@ -711,7 +711,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);
if (is_swiotlb_buffer(dma_addr))
@@ -744,7 +744,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;
+ char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr) + offset;
BUG_ON(dir == DMA_NONE);
if (is_swiotlb_buffer(dma_addr))
@@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
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);
+ unmap_single(
+ hwdev,
+ swiotlb_bus_to_virt(hwdev, 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);
+ dma_mark_clean(
+ swiotlb_bus_to_virt(hwdev, sg->dma_address),
+ sg->dma_length);
}
}
EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
@@ -881,10 +885,13 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
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),
+ sync_single(hwdev,
+ swiotlb_bus_to_virt(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);
+ dma_mark_clean(
+ swiotlb_bus_to_virt(hwdev, sg->dma_address),
+ sg->dma_length);
}
}
--
1.5.6.6
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 <[email protected]>
---
lib/swiotlb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index c152f17..b33180e 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
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 <[email protected]>
---
lib/swiotlb.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index b33180e..f365735 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -145,6 +145,12 @@ void __weak *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
return phys_to_virt(swiotlb_bus_to_phys(hwdev, 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
On Tue, 24 Mar 2009 16:28:44 -0500
Becky Bruce <[email protected]> wrote:
> Make these functions take the hwdev as an argument because on some
> platforms it contains a per-device offset that is used to convert
> from bus addresses to/from other types of addresses.
>
> Also, make these weak so architectures can override the default
> behavior (for example, by adding an offset in the hwdev).
>
> Signed-off-by: Becky Bruce <[email protected]>
> ---
> arch/x86/kernel/pci-swiotlb.c | 2 +-
> include/linux/swiotlb.h | 4 +++-
> lib/swiotlb.c | 31 +++++++++++++++++++------------
> 3 files changed, 23 insertions(+), 14 deletions(-)
> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
>
> 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);
> + unmap_single(
> + hwdev,
> + swiotlb_bus_to_virt(hwdev, 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);
> + dma_mark_clean(
> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
> + sg->dma_length);
> }
The coding style looks a bit odd to me. How about something like this?
for_each_sg(sgl, sg, nelems, i) {
void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
unmap_single(hwdev, virt, sg->dma_length, dir);
else if (dir == DMA_FROM_DEVICE)
dma_mark_clean(virt, sg->dma_length);
> }
> EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
> @@ -881,10 +885,13 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
>
> 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),
> + sync_single(hwdev,
> + swiotlb_bus_to_virt(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);
> + dma_mark_clean(
> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
> + sg->dma_length);
> }
Ditto.
The rest looks fine to me.
On Tue, 24 Mar 2009 16:28:43 -0500
Becky Bruce <[email protected]> wrote:
> Squash a build warning seen on 32-bit powerpc caused by calling min()
> with 2 different types. Cast the first arg to size_t, which is the
> type of the second, and should be portable across architectures.
>
> Signed-off-by: Becky Bruce <[email protected]>
> ---
> lib/swiotlb.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index f59cf30..62f5f75 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((size_t)(PAGE_SIZE - offset), size);
>
> local_irq_save(flags);
> buffer = kmap_atomic(pfn_to_page(pfn),
?
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),
On Tue, 24 Mar 2009 16:28:45 -0500
Becky Bruce <[email protected]> wrote:
> 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 <[email protected]>
> ---
> lib/swiotlb.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index c152f17..b33180e 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;
Acked-by: FUJITA Tomonori <[email protected]>
On Tue, 24 Mar 2009 16:28:46 -0500
Becky Bruce <[email protected]> wrote:
> 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 <[email protected]>
> ---
> lib/swiotlb.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index b33180e..f365735 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -145,6 +145,12 @@ void __weak *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
> return phys_to_virt(swiotlb_bus_to_phys(hwdev, 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)
Looks fine to me. Though I guess that we could clean up
swiotlb_arch_range_needs_mapping and
swiotlb_arch_address_needs_mapping a bit (at least, the names are
confusing).
On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
> On Tue, 24 Mar 2009 16:28:43 -0500
> Becky Bruce <[email protected]> wrote:
>
>> Squash a build warning seen on 32-bit powerpc caused by calling min()
>> with 2 different types. Cast the first arg to size_t, which is the
>> type of the second, and should be portable across architectures.
>>
>> Signed-off-by: Becky Bruce <[email protected]>
>> ---
>> lib/swiotlb.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>> index f59cf30..62f5f75 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((size_t)(PAGE_SIZE - offset), size);
>>
>> local_irq_save(flags);
>> buffer = kmap_atomic(pfn_to_page(pfn),
>
> ?
>
> 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),
OK, we're clearly pointed at different trees here, and it looks like
I'm behind - this patch is based on Ingo's master. Which tree has
this change?
Thanks,
Becky
On Tue, 24 Mar 2009 22:42:33 -0500
Becky Bruce <[email protected]> wrote:
>
> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
>
> > On Tue, 24 Mar 2009 16:28:43 -0500
> > Becky Bruce <[email protected]> wrote:
> >
> >> Squash a build warning seen on 32-bit powerpc caused by calling min()
> >> with 2 different types. Cast the first arg to size_t, which is the
> >> type of the second, and should be portable across architectures.
> >>
> >> Signed-off-by: Becky Bruce <[email protected]>
> >> ---
> >> lib/swiotlb.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >> index f59cf30..62f5f75 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((size_t)(PAGE_SIZE - offset), size);
> >>
> >> local_irq_save(flags);
> >> buffer = kmap_atomic(pfn_to_page(pfn),
> >
> > ?
> >
> > 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),
>
> OK, we're clearly pointed at different trees here, and it looks like
> I'm behind - this patch is based on Ingo's master. Which tree has
> this change?
I also use tip/master. I think that we are on the same page. No tree
has this change. I just send it because I'm not sure why you don't use
min_t().
On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
> On Tue, 24 Mar 2009 16:28:44 -0500
> Becky Bruce <[email protected]> wrote:
>
>> Make these functions take the hwdev as an argument because on some
>> platforms it contains a per-device offset that is used to convert
>> from bus addresses to/from other types of addresses.
>>
>> Also, make these weak so architectures can override the default
>> behavior (for example, by adding an offset in the hwdev).
>>
>> Signed-off-by: Becky Bruce <[email protected]>
>> ---
>> arch/x86/kernel/pci-swiotlb.c | 2 +-
>> include/linux/swiotlb.h | 4 +++-
>> lib/swiotlb.c | 31 +++++++++++++++++++------------
>> 3 files changed, 23 insertions(+), 14 deletions(-)
>
>> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev,
>> struct scatterlist *sgl,
>>
>> 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);
>> + unmap_single(
>> + hwdev,
>> + swiotlb_bus_to_virt(hwdev, 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);
>> + dma_mark_clean(
>> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> + sg->dma_length);
>> }
>
> The coding style looks a bit odd to me. How about something like this?
>
>
> for_each_sg(sgl, sg, nelems, i) {
> void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
> if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
> unmap_single(hwdev, virt, sg->dma_length, dir);
> else if (dir == DMA_FROM_DEVICE)
> dma_mark_clean(virt, sg->dma_length);
Heh, I was trying to avoid > 80 character lines there, and it ended up
looking a bit gross. I originally had *exactly* what you suggest in
my tree, but changed it, so I'm more than happy to change this back.
>
>
>
>> }
>> EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
>> @@ -881,10 +885,13 @@ swiotlb_sync_sg(struct device *hwdev, struct
>> scatterlist *sgl,
>>
>> 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),
>> + sync_single(hwdev,
>> + swiotlb_bus_to_virt(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);
>> + dma_mark_clean(
>> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> + sg->dma_length);
>> }
>
> Ditto.
>
>
> The rest looks fine to me.
Thanks for the review!
Cheers,
-Becky
* Becky Bruce <[email protected]> wrote:
>
> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
>
>> On Tue, 24 Mar 2009 16:28:44 -0500
>> Becky Bruce <[email protected]> wrote:
>>
>>> Make these functions take the hwdev as an argument because on some
>>> platforms it contains a per-device offset that is used to convert
>>> from bus addresses to/from other types of addresses.
>>>
>>> Also, make these weak so architectures can override the default
>>> behavior (for example, by adding an offset in the hwdev).
>>>
>>> Signed-off-by: Becky Bruce <[email protected]>
>>> ---
>>> arch/x86/kernel/pci-swiotlb.c | 2 +-
>>> include/linux/swiotlb.h | 4 +++-
>>> lib/swiotlb.c | 31 +++++++++++++++++++------------
>>> 3 files changed, 23 insertions(+), 14 deletions(-)
>>
>>> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev,
>>> struct scatterlist *sgl,
>>>
>>> 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);
>>> + unmap_single(
>>> + hwdev,
>>> + swiotlb_bus_to_virt(hwdev, 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);
>>> + dma_mark_clean(
>>> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
>>> + sg->dma_length);
>>> }
>>
>> The coding style looks a bit odd to me. How about something like this?
>
>>
>>
>> for_each_sg(sgl, sg, nelems, i) {
>> void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>> unmap_single(hwdev, virt, sg->dma_length, dir);
>> else if (dir == DMA_FROM_DEVICE)
>> dma_mark_clean(virt, sg->dma_length);
that looks odd too.
> Heh, I was trying to avoid > 80 character lines there, and it
> ended up looking a bit gross. I originally had *exactly* what you
> suggest in my tree, but changed it, so I'm more than happy to
> change this back.
The proper solution is to split out the loop body into a helper
function, __swiotlb_unmap_sg_attrs() or so. Something like:
__swiotlb_unmap_sg_attrs()
{
if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
unmap_single(hwdev,
swiotlb_bus_to_virt(hwdev, sg->dma_address),
sg->dma_length, dir);
return;
}
if (dir != DMA_FROM_DEVICE)
return;
dma_mark_clean(swiotlb_bus_to_virt(hwdev, sg->dma_address),
sg->dma_length);
}
Ingo
* FUJITA Tomonori <[email protected]> wrote:
> On Tue, 24 Mar 2009 22:42:33 -0500
> Becky Bruce <[email protected]> wrote:
>
> >
> > On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
> >
> > > On Tue, 24 Mar 2009 16:28:43 -0500
> > > Becky Bruce <[email protected]> wrote:
> > >
> > >> Squash a build warning seen on 32-bit powerpc caused by calling min()
> > >> with 2 different types. Cast the first arg to size_t, which is the
> > >> type of the second, and should be portable across architectures.
> > >>
> > >> Signed-off-by: Becky Bruce <[email protected]>
> > >> ---
> > >> lib/swiotlb.c | 2 +-
> > >> 1 files changed, 1 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> > >> index f59cf30..62f5f75 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((size_t)(PAGE_SIZE - offset), size);
> > >>
> > >> local_irq_save(flags);
> > >> buffer = kmap_atomic(pfn_to_page(pfn),
> > >
> > > ?
> > >
> > > 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),
> >
> > OK, we're clearly pointed at different trees here, and it looks like
> > I'm behind - this patch is based on Ingo's master. Which tree has
> > this change?
>
> I also use tip/master. I think that we are on the same page. No
> tree has this change. I just send it because I'm not sure why you
> don't use min_t().
yes, min_t() is safer and cleaner than min()+cast.
Ingo
On Mar 25, 2009, at 7:45 AM, Ingo Molnar wrote:
>
> * FUJITA Tomonori <[email protected]> wrote:
>
>> On Tue, 24 Mar 2009 22:42:33 -0500
>> Becky Bruce <[email protected]> wrote:
>>
>>>
>>> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
>>>
>>>> On Tue, 24 Mar 2009 16:28:43 -0500
>>>> Becky Bruce <[email protected]> wrote:
>>>>
>>>>> Squash a build warning seen on 32-bit powerpc caused by calling
>>>>> min()
>>>>> with 2 different types. Cast the first arg to size_t, which is
>>>>> the
>>>>> type of the second, and should be portable across architectures.
>>>>>
>>>>> Signed-off-by: Becky Bruce <[email protected]>
>>>>> ---
>>>>> lib/swiotlb.c | 2 +-
>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>>> index f59cf30..62f5f75 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((size_t)(PAGE_SIZE - offset), size);
>>>>>
>>>>> local_irq_save(flags);
>>>>> buffer = kmap_atomic(pfn_to_page(pfn),
>>>>
>>>> ?
>>>>
>>>> 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),
>>>
>>> OK, we're clearly pointed at different trees here, and it looks like
>>> I'm behind - this patch is based on Ingo's master. Which tree has
>>> this change?
>>
>> I also use tip/master. I think that we are on the same page. No
>> tree has this change. I just send it because I'm not sure why you
>> don't use min_t().
>
> yes, min_t() is safer and cleaner than min()+cast.
Ahh, then, that's just an oversight on my part, and I'll fix it. I'll
send out a respun set of patches as soon as I'm out of today's endless
set of meetings.
Thanks,
Becky
Becky Bruce wrote:
> This is 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.
>
> I have not tested this in any way on any non-ppc platforms,
> so commentary/testing from x86/ia64 folks is greatly
> appreciated.
>
This works fine for me under Xen.
Acked-by: Jeremy Fitzhardinge <[email protected]>
J