| Message ID | 1334055852-19500-5-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 10 April 2012, Marek Szyprowski wrote: > This patch removes the need for offset parameter in dma bounce > functions. This is required to let dma-mapping framework on ARM > architecture use common, generic dma-mapping helpers. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> This one worries me a little. I always thought that the range sync functions were specifically needed for the dmabounce code. At the very least, I would expect the changeset comment to have an explanation of why this was initially done this way and why it's now safe to do do it otherwise. Arnd
Hi Arnd, On Tuesday, April 10, 2012 1:43 PM Arnd Bergmann wrote: > On Tuesday 10 April 2012, Marek Szyprowski wrote: > > This patch removes the need for offset parameter in dma bounce > > functions. This is required to let dma-mapping framework on ARM > > architecture use common, generic dma-mapping helpers. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > > This one worries me a little. I always thought that the range sync > functions were specifically needed for the dmabounce code. At the > very least, I would expect the changeset comment to have an explanation > of why this was initially done this way and why it's now safe to do > do it otherwise. Well, range sync functions are available from the early days of the dma mapping api (at least that's what I've found reading the change log and old patches). They are the correct way of doing a partial syncs on the buffer (usually used by the network device drivers). This patch changes only the internal implementation of the dma bounce functions to let them tunnel through dma_map_ops structure. The driver api stays unchanged, so driver are obliged to call dma_*_range_* functions to keep code clean and easy to understand. The only drawback I can see from this patch is reduced detection of the dma api abuse. Let us consider the following code: dma_addr = dma_map_single(dev, ptr, 64, DMA_TO_DEVICE); dma_sync_single_range_for_cpu(dev, dma_addr+16, 0, 32, DMA_TO_DEVICE); Without the patch such code fails, because dma bounce code is unable to find the bounce buffer for the given dma_address. After the patch the sync call will be equivalent to: dma_sync_single_range_for_cpu(dev, dma_addr, 16, 32, DMA_TO_DEVICE); which succeeds. I don't consider this as a real problem. DMA API abuse should be caught by debug_dma_* function family, so we can simplify the internal low-level implementation without losing anything. Best regards
On Wednesday 11 April 2012, Marek Szyprowski wrote: > Well, range sync functions are available from the early days of the dma > mapping api (at least that's what I've found reading the change log and > old patches). They are the correct way of doing a partial syncs on the > buffer (usually used by the network device drivers). This patch changes > only the internal implementation of the dma bounce functions to let > them tunnel through dma_map_ops structure. The driver api stays > unchanged, so driver are obliged to call dma_*_range_* functions to > keep code clean and easy to understand. > > The only drawback I can see from this patch is reduced detection of > the dma api abuse. Let us consider the following code: > > dma_addr = dma_map_single(dev, ptr, 64, DMA_TO_DEVICE); > dma_sync_single_range_for_cpu(dev, dma_addr+16, 0, 32, DMA_TO_DEVICE); > > Without the patch such code fails, because dma bounce code is unable > to find the bounce buffer for the given dma_address. After the patch > the sync call will be equivalent to: > > dma_sync_single_range_for_cpu(dev, dma_addr, 16, 32, DMA_TO_DEVICE); > > which succeeds. > > I don't consider this as a real problem. DMA API abuse should be caught > by debug_dma_* function family, so we can simplify the internal low-level > implementation without losing anything. > Ok, fair enough. Can you put the above text into the changelog? Arnd
Hi Arnd, On Wednesday, April 11, 2012 2:19 PM Arnd Bergmann wrote: > On Wednesday 11 April 2012, Marek Szyprowski wrote: > > Well, range sync functions are available from the early days of the dma > > mapping api (at least that's what I've found reading the change log and > > old patches). They are the correct way of doing a partial syncs on the > > buffer (usually used by the network device drivers). This patch changes > > only the internal implementation of the dma bounce functions to let > > them tunnel through dma_map_ops structure. The driver api stays > > unchanged, so driver are obliged to call dma_*_range_* functions to > > keep code clean and easy to understand. > > > > The only drawback I can see from this patch is reduced detection of > > the dma api abuse. Let us consider the following code: > > > > dma_addr = dma_map_single(dev, ptr, 64, DMA_TO_DEVICE); > > dma_sync_single_range_for_cpu(dev, dma_addr+16, 0, 32, DMA_TO_DEVICE); > > > > Without the patch such code fails, because dma bounce code is unable > > to find the bounce buffer for the given dma_address. After the patch > > the sync call will be equivalent to: > > > > dma_sync_single_range_for_cpu(dev, dma_addr, 16, 32, DMA_TO_DEVICE); > > > > which succeeds. > > > > I don't consider this as a real problem. DMA API abuse should be caught > > by debug_dma_* function family, so we can simplify the internal low-level > > implementation without losing anything. > > > > Ok, fair enough. Can you put the above text into the changelog? Yes, I will update it in the next release. Best regards
diff --git a/arch/arm/common/dmabounce.c b/arch/arm/common/dmabounce.c index a1abdc9..c9f54b6 100644 --- a/arch/arm/common/dmabounce.c +++ b/arch/arm/common/dmabounce.c @@ -173,7 +173,8 @@ find_safe_buffer(struct dmabounce_device_info *device_info, dma_addr_t safe_dma_ read_lock_irqsave(&device_info->lock, flags); list_for_each_entry(b, &device_info->safe_buffers, node) - if (b->safe_dma_addr == safe_dma_addr) { + if (b->safe_dma_addr <= safe_dma_addr && + b->safe_dma_addr + b->size > safe_dma_addr) { rb = b; break; } @@ -362,9 +363,10 @@ void __dma_unmap_page(struct device *dev, dma_addr_t dma_addr, size_t size, EXPORT_SYMBOL(__dma_unmap_page); int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr, - unsigned long off, size_t sz, enum dma_data_direction dir) + size_t sz, enum dma_data_direction dir) { struct safe_buffer *buf; + unsigned long off; dev_dbg(dev, "%s(dma=%#x,off=%#lx,sz=%zx,dir=%x)\n", __func__, addr, off, sz, dir); @@ -373,6 +375,8 @@ int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr, if (!buf) return 1; + off = addr - buf->safe_dma_addr; + BUG_ON(buf->direction != dir); dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x) mapped to %p (dma=%#x)\n", @@ -391,9 +395,10 @@ int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr, EXPORT_SYMBOL(dmabounce_sync_for_cpu); int dmabounce_sync_for_device(struct device *dev, dma_addr_t addr, - unsigned long off, size_t sz, enum dma_data_direction dir) + size_t sz, enum dma_data_direction dir) { struct safe_buffer *buf; + unsigned long off; dev_dbg(dev, "%s(dma=%#x,off=%#lx,sz=%zx,dir=%x)\n", __func__, addr, off, sz, dir); @@ -402,6 +407,8 @@ int dmabounce_sync_for_device(struct device *dev, dma_addr_t addr, if (!buf) return 1; + off = addr - buf->safe_dma_addr; + BUG_ON(buf->direction != dir); dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x) mapped to %p (dma=%#x)\n", diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index 3dbec1d..02d651f 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -266,19 +266,17 @@ extern void __dma_unmap_page(struct device *, dma_addr_t, size_t, /* * Private functions */ -int dmabounce_sync_for_cpu(struct device *, dma_addr_t, unsigned long, - size_t, enum dma_data_direction); -int dmabounce_sync_for_device(struct device *, dma_addr_t, unsigned long, - size_t, enum dma_data_direction); +int dmabounce_sync_for_cpu(struct device *, dma_addr_t, size_t, enum dma_data_direction); +int dmabounce_sync_for_device(struct device *, dma_addr_t, size_t, enum dma_data_direction); #else static inline int dmabounce_sync_for_cpu(struct device *d, dma_addr_t addr, - unsigned long offset, size_t size, enum dma_data_direction dir) + size_t size, enum dma_data_direction dir) { return 1; } static inline int dmabounce_sync_for_device(struct device *d, dma_addr_t addr, - unsigned long offset, size_t size, enum dma_data_direction dir) + size_t size, enum dma_data_direction dir) { return 1; } @@ -401,6 +399,33 @@ static inline void dma_unmap_page(struct device *dev, dma_addr_t handle, __dma_unmap_page(dev, handle, size, dir); } + +static inline void dma_sync_single_for_cpu(struct device *dev, + dma_addr_t handle, size_t size, enum dma_data_direction dir) +{ + BUG_ON(!valid_dma_direction(dir)); + + debug_dma_sync_single_for_cpu(dev, handle, size, dir); + + if (!dmabounce_sync_for_cpu(dev, handle, size, dir)) + return; + + __dma_single_dev_to_cpu(dma_to_virt(dev, handle), size, dir); +} + +static inline void dma_sync_single_for_device(struct device *dev, + dma_addr_t handle, size_t size, enum dma_data_direction dir) +{ + BUG_ON(!valid_dma_direction(dir)); + + debug_dma_sync_single_for_device(dev, handle, size, dir); + + if (!dmabounce_sync_for_device(dev, handle, size, dir)) + return; + + __dma_single_cpu_to_dev(dma_to_virt(dev, handle), size, dir); +} + /** * dma_sync_single_range_for_cpu * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices @@ -423,40 +448,14 @@ static inline void dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t handle, unsigned long offset, size_t size, enum dma_data_direction dir) { - BUG_ON(!valid_dma_direction(dir)); - - debug_dma_sync_single_for_cpu(dev, handle + offset, size, dir); - - if (!dmabounce_sync_for_cpu(dev, handle, offset, size, dir)) - return; - - __dma_single_dev_to_cpu(dma_to_virt(dev, handle) + offset, size, dir); + dma_sync_single_for_cpu(dev, handle + offset, size, dir); } static inline void dma_sync_single_range_for_device(struct device *dev, dma_addr_t handle, unsigned long offset, size_t size, enum dma_data_direction dir) { - BUG_ON(!valid_dma_direction(dir)); - - debug_dma_sync_single_for_device(dev, handle + offset, size, dir); - - if (!dmabounce_sync_for_device(dev, handle, offset, size, dir)) - return; - - __dma_single_cpu_to_dev(dma_to_virt(dev, handle) + offset, size, dir); -} - -static inline void dma_sync_single_for_cpu(struct device *dev, - dma_addr_t handle, size_t size, enum dma_data_direction dir) -{ - dma_sync_single_range_for_cpu(dev, handle, 0, size, dir); -} - -static inline void dma_sync_single_for_device(struct device *dev, - dma_addr_t handle, size_t size, enum dma_data_direction dir) -{ - dma_sync_single_range_for_device(dev, handle, 0, size, dir); + dma_sync_single_for_device(dev, handle + offset, size, dir); } /* diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 8762d5a..ed16a7b 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -657,7 +657,7 @@ void dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int i; for_each_sg(sg, s, nents, i) { - if (!dmabounce_sync_for_cpu(dev, sg_dma_address(s), 0, + if (!dmabounce_sync_for_cpu(dev, sg_dma_address(s), sg_dma_len(s), dir)) continue; @@ -683,7 +683,7 @@ void dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, int i; for_each_sg(sg, s, nents, i) { - if (!dmabounce_sync_for_device(dev, sg_dma_address(s), 0, + if (!dmabounce_sync_for_device(dev, sg_dma_address(s), sg_dma_len(s), dir)) continue;