| Message ID | 1227284770-19215-8-git-send-email-joerg.roedel@amd.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
please use this nice and clean structure init style: > + entry->type = DMA_DEBUG_COHERENT; > + entry->dev = dev; > + entry->cpu_addr = virt; > + entry->size = size; > + entry->dev_addr = dma_addr; > + entry->direction = DMA_BIDIRECTIONAL; here too: > + struct dma_debug_entry ref = { > + .type = DMA_DEBUG_COHERENT, > + .dev = dev, > + .cpu_addr = virt, > + .dev_addr = addr, > + .size = size, > + .direction = DMA_BIDIRECTIONAL, > + }; Ingo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 21 Nov 2008 17:26:07 +0100 Joerg Roedel <joerg.roedel@amd.com> wrote: > Impact: detect bugs in alloc/free_coherent usage > > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> > --- > arch/x86/include/asm/dma-mapping.h | 10 +++++- > arch/x86/include/asm/dma_debug.h | 20 +++++++++++++ > arch/x86/kernel/pci-dma-debug.c | 56 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 84 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h > index c7bdb75..2893adb 100644 > --- a/arch/x86/include/asm/dma-mapping.h > +++ b/arch/x86/include/asm/dma-mapping.h > @@ -304,8 +304,12 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, > if (!ops->alloc_coherent) > return NULL; > > - return ops->alloc_coherent(dev, size, dma_handle, > - dma_alloc_coherent_gfp_flags(dev, gfp)); > + memory = ops->alloc_coherent(dev, size, dma_handle, > + dma_alloc_coherent_gfp_flags(dev, gfp)); > + > + debug_alloc_coherent(dev, size, *dma_handle, memory); > + > + return memory; > } > > static inline void dma_free_coherent(struct device *dev, size_t size, > @@ -320,6 +324,8 @@ static inline void dma_free_coherent(struct device *dev, size_t size, > > if (ops->free_coherent) > ops->free_coherent(dev, size, vaddr, bus); > + > + debug_free_coherent(dev, size, vaddr, bus); > } > > #endif > diff --git a/arch/x86/include/asm/dma_debug.h b/arch/x86/include/asm/dma_debug.h > index ff06d1c..7245e27 100644 > --- a/arch/x86/include/asm/dma_debug.h > +++ b/arch/x86/include/asm/dma_debug.h > @@ -59,6 +59,14 @@ extern > void debug_unmap_sg(struct device *dev, struct scatterlist *sglist, > int nelems, int dir); > > +extern > +void debug_alloc_coherent(struct device *dev, size_t size, > + dma_addr_t dma_addr, void *virt); > + > +extern > +void debug_free_coherent(struct device *dev, size_t size, > + void *virt, dma_addr_t addr); > + > #else /* CONFIG_DMA_API_DEBUG */ > > static inline > @@ -90,6 +98,18 @@ void debug_unmap_sg(struct device *dev, struct scatterlist *sglist, > { > } > > +static inline > +void debug_alloc_coherent(struct device *dev, size_t size, > + dma_addr_t dma_addr, void *virt) > +{ > +} > + > +static inline > +void debug_free_coherent(struct device *dev, size_t size, > + void *virt, dma_addr_t addr) > +{ > +} > + > #endif /* CONFIG_DMA_API_DEBUG */ > > #endif /* __ASM_X86_DMA_DEBUG */ > diff --git a/arch/x86/kernel/pci-dma-debug.c b/arch/x86/kernel/pci-dma-debug.c > index 55ef69a..db5ef9a 100644 > --- a/arch/x86/kernel/pci-dma-debug.c > +++ b/arch/x86/kernel/pci-dma-debug.c > @@ -352,3 +352,59 @@ void debug_unmap_sg(struct device *dev, struct scatterlist *sglist, > } > EXPORT_SYMBOL(debug_unmap_sg); > > +void debug_alloc_coherent(struct device *dev, size_t size, > + dma_addr_t dma_addr, void *virt) > +{ > + unsigned long flags; > + struct dma_debug_entry *entry; > + > + if (dma_addr == bad_dma_address) > + return; > + > + entry = dma_entry_alloc(); > + if (!entry) > + return; > + > + entry->type = DMA_DEBUG_COHERENT; > + entry->dev = dev; > + entry->cpu_addr = virt; > + entry->size = size; > + entry->dev_addr = dma_addr; > + entry->direction = DMA_BIDIRECTIONAL; > + > + spin_lock_irqsave(&dma_lock, flags); > + add_dma_entry(entry); > + spin_unlock_irqrestore(&dma_lock, flags); > +} > +EXPORT_SYMBOL(debug_alloc_coherent); Can you clean up the duplication in debug_map_single, debug_map_sg, and debug_alloc_coherent? The higher-level helper functions might help. > +void debug_free_coherent(struct device *dev, size_t size, > + void *virt, dma_addr_t addr) > +{ > + unsigned long flags; > + struct dma_debug_entry ref = { > + .type = DMA_DEBUG_COHERENT, > + .dev = dev, > + .cpu_addr = virt, > + .dev_addr = addr, > + .size = size, > + .direction = DMA_BIDIRECTIONAL, > + }; > + struct dma_debug_entry *entry; > + > + if (addr == bad_dma_address) > + return; > + > + spin_lock_irqsave(&dma_lock, flags); > + > + entry = find_dma_entry(&ref); > + > + if (check_unmap(&ref, entry)) { > + remove_dma_entry(entry); > + dma_entry_free(entry); > + } > + > + spin_unlock_irqrestore(&dma_lock, flags); > +} > +EXPORT_SYMBOL(debug_free_coherent); Ditto. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 22, 2008 at 12:27:42PM +0900, FUJITA Tomonori wrote: > On Fri, 21 Nov 2008 17:26:07 +0100 > Joerg Roedel <joerg.roedel@amd.com> wrote: > > +void debug_alloc_coherent(struct device *dev, size_t size, > > + dma_addr_t dma_addr, void *virt) > > +{ > > + unsigned long flags; > > + struct dma_debug_entry *entry; > > + > > + if (dma_addr == bad_dma_address) > > + return; > > + > > + entry = dma_entry_alloc(); > > + if (!entry) > > + return; > > + > > + entry->type = DMA_DEBUG_COHERENT; > > + entry->dev = dev; > > + entry->cpu_addr = virt; > > + entry->size = size; > > + entry->dev_addr = dma_addr; > > + entry->direction = DMA_BIDIRECTIONAL; > > + > > + spin_lock_irqsave(&dma_lock, flags); > > + add_dma_entry(entry); > > + spin_unlock_irqrestore(&dma_lock, flags); > > +} > > +EXPORT_SYMBOL(debug_alloc_coherent); > > Can you clean up the duplication in debug_map_single, debug_map_sg, > and debug_alloc_coherent? The higher-level helper functions might > help. Hmm, lets see. For me it makes only sense if it won't result in helper functions with tons of parameters. This is worse than little code duplication. But lets see. Joerg -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h index c7bdb75..2893adb 100644 --- a/arch/x86/include/asm/dma-mapping.h +++ b/arch/x86/include/asm/dma-mapping.h @@ -304,8 +304,12 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, if (!ops->alloc_coherent) return NULL; - return ops->alloc_coherent(dev, size, dma_handle, - dma_alloc_coherent_gfp_flags(dev, gfp)); + memory = ops->alloc_coherent(dev, size, dma_handle, + dma_alloc_coherent_gfp_flags(dev, gfp)); + + debug_alloc_coherent(dev, size, *dma_handle, memory); + + return memory; } static inline void dma_free_coherent(struct device *dev, size_t size, @@ -320,6 +324,8 @@ static inline void dma_free_coherent(struct device *dev, size_t size, if (ops->free_coherent) ops->free_coherent(dev, size, vaddr, bus); + + debug_free_coherent(dev, size, vaddr, bus); } #endif diff --git a/arch/x86/include/asm/dma_debug.h b/arch/x86/include/asm/dma_debug.h index ff06d1c..7245e27 100644 --- a/arch/x86/include/asm/dma_debug.h +++ b/arch/x86/include/asm/dma_debug.h @@ -59,6 +59,14 @@ extern void debug_unmap_sg(struct device *dev, struct scatterlist *sglist, int nelems, int dir); +extern +void debug_alloc_coherent(struct device *dev, size_t size, + dma_addr_t dma_addr, void *virt); + +extern +void debug_free_coherent(struct device *dev, size_t size, + void *virt, dma_addr_t addr); + #else /* CONFIG_DMA_API_DEBUG */ static inline @@ -90,6 +98,18 @@ void debug_unmap_sg(struct device *dev, struct scatterlist *sglist, { } +static inline +void debug_alloc_coherent(struct device *dev, size_t size, + dma_addr_t dma_addr, void *virt) +{ +} + +static inline +void debug_free_coherent(struct device *dev, size_t size, + void *virt, dma_addr_t addr) +{ +} + #endif /* CONFIG_DMA_API_DEBUG */ #endif /* __ASM_X86_DMA_DEBUG */ diff --git a/arch/x86/kernel/pci-dma-debug.c b/arch/x86/kernel/pci-dma-debug.c index 55ef69a..db5ef9a 100644 --- a/arch/x86/kernel/pci-dma-debug.c +++ b/arch/x86/kernel/pci-dma-debug.c @@ -352,3 +352,59 @@ void debug_unmap_sg(struct device *dev, struct scatterlist *sglist, } EXPORT_SYMBOL(debug_unmap_sg); +void debug_alloc_coherent(struct device *dev, size_t size, + dma_addr_t dma_addr, void *virt) +{ + unsigned long flags; + struct dma_debug_entry *entry; + + if (dma_addr == bad_dma_address) + return; + + entry = dma_entry_alloc(); + if (!entry) + return; + + entry->type = DMA_DEBUG_COHERENT; + entry->dev = dev; + entry->cpu_addr = virt; + entry->size = size; + entry->dev_addr = dma_addr; + entry->direction = DMA_BIDIRECTIONAL; + + spin_lock_irqsave(&dma_lock, flags); + add_dma_entry(entry); + spin_unlock_irqrestore(&dma_lock, flags); +} +EXPORT_SYMBOL(debug_alloc_coherent); + +void debug_free_coherent(struct device *dev, size_t size, + void *virt, dma_addr_t addr) +{ + unsigned long flags; + struct dma_debug_entry ref = { + .type = DMA_DEBUG_COHERENT, + .dev = dev, + .cpu_addr = virt, + .dev_addr = addr, + .size = size, + .direction = DMA_BIDIRECTIONAL, + }; + struct dma_debug_entry *entry; + + if (addr == bad_dma_address) + return; + + spin_lock_irqsave(&dma_lock, flags); + + entry = find_dma_entry(&ref); + + if (check_unmap(&ref, entry)) { + remove_dma_entry(entry); + dma_entry_free(entry); + } + + spin_unlock_irqrestore(&dma_lock, flags); +} +EXPORT_SYMBOL(debug_free_coherent); +
Impact: detect bugs in alloc/free_coherent usage Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> --- arch/x86/include/asm/dma-mapping.h | 10 +++++- arch/x86/include/asm/dma_debug.h | 20 +++++++++++++ arch/x86/kernel/pci-dma-debug.c | 56 ++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-)