Hi,
this is version 2 of the patchset which introduces code to debug drivers
usage of the DMA-API. Many thanks to all the reviewers and the useful
comments on the fist version of this patchset. Tests with hardware
IOMMUs have shown several bugs in drivers regarding the usage of that
API. Problems were found especially in network card drivers.
These bugs often don't show up or have any negative impact if there is
no hardware IOMMU in use in the system. But with an hardware IOMMU these
bugs turn the hardware unusable or, in the worst case, cause data
corruption on devices which are managed by other (good) drivers.
With the code these patches introduce driver developers can find several
bugs of misusing the DMA-API in their drivers. But be aware, it can not
find all possible bugs. If it finds a problem it prints out messages
like
------------[ cut here ]------------
WARNING: at /data2/repos/linux.trees.git/lib/dma-debug.c:231 check_unmap+0xab/0x3d9()
Hardware name: Toonie
bnx2 0000:01:00.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x00000000011]
Modules linked in:
Pid: 0, comm: swapper Not tainted 2.6.28 #174
Call Trace:
<IRQ> [<ffffffff8105af3a>] warn_slowpath+0xd3/0xf2
[<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
[<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
[<ffffffff812efd16>] ? usb_hcd_link_urb_to_ep+0x94/0xa0
[<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
[<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
[<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
[<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
[<ffffffff814b25a5>] ? _spin_lock_irqsave+0x69/0x75
[<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
[<ffffffff811e2ff2>] check_unmap+0xab/0x3d9
[<ffffffff8107c9ed>] ? trace_hardirqs_on_caller+0x108/0x14a
[<ffffffff8107ca3c>] ? trace_hardirqs_on+0xd/0xf
[<ffffffff811e3433>] debug_unmap_single+0x3e/0x40
[<ffffffff8128d2d8>] dma_unmap_single+0x3d/0x60
[<ffffffff8128d335>] pci_unmap_page+0x1c/0x1e
[<ffffffff81290759>] bnx2_poll_work+0x626/0x8cb
[<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
[<ffffffff81070100>] ? run_posix_cpu_timers+0x49c/0x603
[<ffffffff81070000>] ? run_posix_cpu_timers+0x39c/0x603
[<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
[<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
[<ffffffff81292804>] bnx2_poll_msix+0x33/0x81
[<ffffffff813b6478>] net_rx_action+0x8a/0x139
[<ffffffff8105ff39>] __do_softirq+0x8b/0x147
[<ffffffff8102933c>] call_softirq+0x1c/0x34
[<ffffffff8102a611>] do_softirq+0x39/0x90
[<ffffffff8105fde8>] irq_exit+0x4e/0x98
[<ffffffff8102a5c2>] do_IRQ+0x11f/0x135
[<ffffffff81028b93>] ret_from_intr+0x0/0xf
<EOI> <4>---[ end trace 4339d58302097423 ]---
This way driver developers get an idea where the problem is in their
code.
I hope I addressed most of the review comments and objections from the
first version. Please give this version also a good review and send me
your comments.
Thanks,
Joerg
Changes from v1 -> v2:
- moved code to lib/ and include/linux to make it usable for all
architectures
- more fine grained hash locking (locking is now per hash bucket, no
global lock anymore)
- dma_debug_entries are preallocated
- per default the code will only print one warning and is silent then
- added a debugfs interface to see some statistics and to enable more
verbose error reporting in the kernel log
- added command line parameter to disable debugging code
- allocation errors are now handled correctly
- added documentation about this facility for driver developers
diffstat:
Documentation/DMA-API.txt | 117 ++++++
Documentation/kernel-parameters.txt | 10 +
arch/Kconfig | 2 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/dma-mapping.h | 30 ++-
arch/x86/kernel/pci-dma.c | 5 +
include/linux/dma-debug.h | 154 ++++++++
lib/Kconfig.debug | 11 +
lib/Makefile | 2 +
lib/dma-debug.c | 726 +++++++++++++++++++++++++++++++++++
10 files changed, 1054 insertions(+), 4 deletions(-)
Impact: add debug callbacks for dma_sync_single_range_for_* functions
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 26 ++++++++++++++++++++++++++
lib/dma-debug.c | 23 +++++++++++++++++++++++
2 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index f39c2a8..b9c221a 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -54,6 +54,16 @@ extern void debug_sync_single_for_device(struct device *dev,
dma_addr_t dma_handle,
size_t size, int direction);
+extern void debug_sync_single_range_for_cpu(struct device *dev,
+ dma_addr_t dma_handle,
+ unsigned long offset, size_t size,
+ int direction);
+
+extern void debug_sync_single_range_for_device(struct device *dev,
+ dma_addr_t dma_handle,
+ unsigned long offset,
+ size_t size, int direction);
+
#else /* CONFIG_DMA_API_DEBUG */
static inline void dma_debug_init(u32 num_entries)
@@ -104,6 +114,22 @@ static inline void debug_sync_single_for_device(struct device *dev,
{
}
+static inline void debug_sync_single_range_for_cpu(struct device *dev,
+ dma_addr_t dma_handle,
+ unsigned long offset,
+ size_t size,
+ int direction)
+{
+}
+
+static inline void debug_sync_single_range_for_device(struct device *dev,
+ dma_addr_t dma_handle,
+ unsigned long offset,
+ size_t size,
+ int direction)
+{
+}
+
#endif /* CONFIG_DMA_API_DEBUG */
#endif /* __DMA_DEBUG_H */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index a7f2369..6f73bed 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -669,3 +669,26 @@ void debug_sync_single_for_device(struct device *dev, dma_addr_t dma_handle,
}
EXPORT_SYMBOL(debug_sync_single_for_device);
+void debug_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
+ unsigned long offset, size_t size,
+ int direction)
+{
+ if (global_disable)
+ return;
+
+ check_sync(dev, dma_handle, size, offset, direction, true);
+}
+EXPORT_SYMBOL(debug_sync_single_range_for_cpu);
+
+void debug_sync_single_range_for_device(struct device *dev,
+ dma_addr_t dma_handle,
+ unsigned long offset,
+ size_t size, int direction)
+{
+ if (global_disable)
+ return;
+
+ check_sync(dev, dma_handle, size, offset, direction, false);
+}
+EXPORT_SYMBOL(debug_sync_single_range_for_device);
+
--
1.5.6.4
Impact: add debugfs interface for configuring DMA-API debugging
Signed-off-by: Joerg Roedel <[email protected]>
---
lib/dma-debug.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 78 insertions(+), 0 deletions(-)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 4e58d09..ca0ccb1 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -19,6 +19,7 @@
#include <linux/dma-debug.h>
#include <linux/spinlock.h>
+#include <linux/debugfs.h>
#include <linux/types.h>
#include <linux/list.h>
#include <linux/slab.h>
@@ -58,12 +59,29 @@ static DEFINE_SPINLOCK(free_entries_lock);
/* Global disable flag - will be set in case of an error */
static bool global_disable __read_mostly;
+/* Global error count */
+static u32 error_count;
+
+/* Global error show enable*/
+static u32 show_all_errors __read_mostly;
+/* Number of errors to show */
+static u32 show_num_errors = 1;
+
static u32 num_free_entries;
static u32 min_free_entries;
/* number of preallocated entries requested by kernel cmdline */
static u32 req_entries;
+/* debugfs dentry's for the stuff above */
+static struct dentry *dma_debug_dent;
+static struct dentry *global_disable_dent;
+static struct dentry *error_count_dent;
+static struct dentry *show_all_errors_dent;
+static struct dentry *show_num_errors_dent;
+static struct dentry *num_free_entries_dent;
+static struct dentry *min_free_entries_dent;
+
/*
* Hash related functions
*
@@ -238,6 +256,58 @@ out_err:
return -ENOMEM;
}
+static int dma_debug_fs_init(void)
+{
+ dma_debug_dent = debugfs_create_dir("dma-api", NULL);
+ if (!dma_debug_dent) {
+ printk(KERN_ERR "DMA-API: can not create debugfs directory\n");
+ return -ENOMEM;
+ }
+
+ global_disable_dent = debugfs_create_bool("disabled", 0444,
+ dma_debug_dent,
+ (u32 *)&global_disable);
+ if (!global_disable_dent)
+ goto out_err;
+
+ error_count_dent = debugfs_create_u32("error_count", 0444,
+ dma_debug_dent, &error_count);
+ if (!error_count_dent)
+ goto out_err;
+
+ show_all_errors_dent = debugfs_create_u32("all_errors", 0644,
+ dma_debug_dent,
+ &show_all_errors);
+ if (!show_all_errors_dent)
+ goto out_err;
+
+ show_num_errors_dent = debugfs_create_u32("num_errors", 0644,
+ dma_debug_dent,
+ &show_num_errors);
+ if (!show_num_errors_dent)
+ goto out_err;
+
+ num_free_entries_dent = debugfs_create_u32("num_free_entries", 0444,
+ dma_debug_dent,
+ &num_free_entries);
+ if (!num_free_entries_dent)
+ goto out_err;
+
+ min_free_entries_dent = debugfs_create_u32("min_free_entries", 0444,
+ dma_debug_dent,
+ &min_free_entries);
+ if (!min_free_entries_dent)
+ goto out_err;
+
+ return 0;
+
+out_err:
+ debugfs_remove_recursive(dma_debug_dent);
+
+ return -ENOMEM;
+}
+
+
/*
* Let the architectures decide how many entries should be preallocated.
*/
@@ -253,6 +323,14 @@ void dma_debug_init(u32 num_entries)
dma_entry_hash[i].lock = SPIN_LOCK_UNLOCKED;
}
+ if (dma_debug_fs_init() != 0) {
+ printk(KERN_ERR "DMA-API: error creating debugfs entries "
+ "- disabling\n");
+ global_disable = true;
+
+ return;
+ }
+
if (req_entries)
num_entries = req_entries;
--
1.5.6.4
Impact: add documentation about DMA-API debugging to DMA-API.txt
Signed-off-by: Joerg Roedel <[email protected]>
---
Documentation/DMA-API.txt | 117 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 117 insertions(+), 0 deletions(-)
diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index b462bb1..e36e85a 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -610,3 +610,120 @@ size is the size (and should be a page-sized multiple).
The return value will be either a pointer to the processor virtual
address of the memory, or an error (via PTR_ERR()) if any part of the
region is occupied.
+
+Part III - Debug drivers use of the DMA-API
+-------------------------------------------
+
+The DMA-API as described above as some constraints. DMA addresses must be
+released with the corresponding function with the same size for example. With
+the advent of hardware IOMMUs it becomes more and more important that drivers
+do not violate those constraints. In the worst case such a violation can
+result in data corruption up to destroyed filesystems.
+
+To debug drivers and find bugs in the usage of the DMA-API checking code can
+be compiled into the kernel which will tell the developer about those
+violations. If your architecture supports it you can select the "Enable
+debugging of DMA-API usage" option in your kernel configuration. Enabling this
+option has a performance impact. Do not enable it in production kernels.
+
+If you boot the resulting kernel will contain code which does some bookkeeping
+about what DMA memory was allocated for which device. If this code detects an
+error it prints a warning message with some details into your kernel log. An
+example warning message may look like this:
+
+------------[ cut here ]------------
+WARNING: at /data2/repos/linux.trees.git/lib/dma-debug.c:231
+ check_unmap+0xab/0x3d9()
+Hardware name: Toonie
+bnx2 0000:01:00.0: DMA-API: device driver tries to free DMA
+ memory it has not allocated [device address=0x00000000011]
+Modules linked in:
+Pid: 0, comm: swapper Not tainted 2.6.28 #174
+Call Trace:
+ <IRQ> [<ffffffff8105af3a>] warn_slowpath+0xd3/0xf2
+ [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
+ [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
+ [<ffffffff812efd16>] ? usb_hcd_link_urb_to_ep+0x94/0xa0
+ [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
+ [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
+ [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
+ [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
+ [<ffffffff814b25a5>] ? _spin_lock_irqsave+0x69/0x75
+ [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
+ [<ffffffff811e2ff2>] check_unmap+0xab/0x3d9
+ [<ffffffff8107c9ed>] ? trace_hardirqs_on_caller+0x108/0x14a
+ [<ffffffff8107ca3c>] ? trace_hardirqs_on+0xd/0xf
+ [<ffffffff811e3433>] debug_unmap_single+0x3e/0x40
+ [<ffffffff8128d2d8>] dma_unmap_single+0x3d/0x60
+ [<ffffffff8128d335>] pci_unmap_page+0x1c/0x1e
+ [<ffffffff81290759>] bnx2_poll_work+0x626/0x8cb
+ [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
+ [<ffffffff81070100>] ? run_posix_cpu_timers+0x49c/0x603
+ [<ffffffff81070000>] ? run_posix_cpu_timers+0x39c/0x603
+ [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
+ [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
+ [<ffffffff81292804>] bnx2_poll_msix+0x33/0x81
+ [<ffffffff813b6478>] net_rx_action+0x8a/0x139
+ [<ffffffff8105ff39>] __do_softirq+0x8b/0x147
+ [<ffffffff8102933c>] call_softirq+0x1c/0x34
+ [<ffffffff8102a611>] do_softirq+0x39/0x90
+ [<ffffffff8105fde8>] irq_exit+0x4e/0x98
+ [<ffffffff8102a5c2>] do_IRQ+0x11f/0x135
+ [<ffffffff81028b93>] ret_from_intr+0x0/0xf
+ <EOI> <4>---[ end trace 4339d58302097423 ]---
+
+The driver developer can find the driver and the device including a stacktrace
+of the DMA-API call which caused this warning.
+
+Per default only the first error will result in a warning message. All other
+errors will only silently counted. This limitation exist to prevent the code
+from flooding your kernel log. To support debugging a device driver this can
+be disabled via debugfs. See the debugfs interface documentation below for
+details.
+
+The debugfs directory for the DMA-API debugging code is called dma-api/. In
+this directory the following files can currently be found:
+
+ dma-api/all_errors This file contains a numeric value. If this
+ value is not equal to zero the debugging code
+ will print a warning for every error it finds
+ into the kernel log. Be carefull with this
+ option. It can easily flood your logs.
+
+ dma-api/disabled This read-only file contains the character 'Y'
+ if the debugging code is disabled. This can
+ happen when it runs out of memory or if it was
+ disabled at boot time
+
+ dma-api/error_count This file is read-only and shows the total
+ numbers of errors found.
+
+ dma-api/num_errors The number in this file shows how many
+ warnings will be printed to the kernel log
+ before it stops. This number is initialized to
+ one at system boot and be set by writing into
+ this file
+
+ dma-api/min_free_entries
+ This read-only file can be read to get the
+ minimum number of free dma_debug_entries the
+ allocator has ever seen. If this value goes
+ down to zero the code will disable itself
+ because it is not longer reliable.
+
+ dma-api/num_free_entries
+ The current number of free dma_debug_entries
+ in the allocator.
+
+If you have this code compiled into your kernel it will be enabled by default.
+If you want to boot without the bookkeeping anyway you can provide
+'dma_debug=off' as a boot parameter. This will disable DMA-API debugging.
+Notice that you can not enable it again at runtime. You have to reboot to do
+so.
+
+When the code disables itself at runtime this is most likely because it ran
+out of dma_debug_entries. These entries are preallocated at boot. The number
+of preallocated entries is defined per architecture. If it is too low for you
+boot with 'dma_debug_entries=<your_desired_number>' to overwrite the
+architectural default.
+
--
1.5.6.4
Impact: add code to initialize dma-debug core data structures
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 14 +++++++++
lib/dma-debug.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index ce4ace7..345d538 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -20,6 +20,20 @@
#ifndef __DMA_DEBUG_H
#define __DMA_DEBUG_H
+#include <linux/types.h>
+
struct device;
+#ifdef CONFIG_DMA_API_DEBUG
+
+extern void dma_debug_init(u32 num_entries);
+
+#else /* CONFIG_DMA_API_DEBUG */
+
+static inline void dma_debug_init(u32 num_entries)
+{
+}
+
+#endif /* CONFIG_DMA_API_DEBUG */
+
#endif /* __DMA_DEBUG_H */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index c5444d4..b932f15 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -21,6 +21,7 @@
#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/list.h>
+#include <linux/slab.h>
#define HASH_SIZE 256
#define HASH_FN_SHIFT 20
@@ -195,3 +196,68 @@ static void dma_entry_free(struct dma_debug_entry *entry)
spin_unlock_irqrestore(&free_entries_lock, flags);
}
+/*
+ * DMA-API debugging init code
+ *
+ * The init code does two things:
+ * 1. Initialize core data structures
+ * 2. Preallocate a given number of dma_debug_entry structs
+ */
+
+static int prealloc_memory(u32 num_entries)
+{
+ struct dma_debug_entry *entry, *next_entry;
+ int i;
+
+ for (i = 0; i < num_entries; ++i) {
+ entry = kmalloc(sizeof(*entry), GFP_KERNEL | __GFP_ZERO);
+ if (!entry)
+ goto out_err;
+
+ list_add_tail(&entry->list, &free_entries);
+ }
+
+ num_free_entries = num_entries;
+ min_free_entries = num_entries;
+
+ printk(KERN_INFO "DMA-API: preallocated %d debug entries\n",
+ num_entries);
+
+ return 0;
+
+out_err:
+
+ list_for_each_entry_safe(entry, next_entry, &free_entries, list) {
+ list_del(&entry->list);
+ kfree(entry);
+ }
+
+ return -ENOMEM;
+}
+
+/*
+ * Let the architectures decide how many entries should be preallocated.
+ */
+void dma_debug_init(u32 num_entries)
+{
+ int i;
+
+ if (global_disable)
+ return;
+
+ for (i = 0; i < HASH_SIZE; ++i) {
+ INIT_LIST_HEAD(&dma_entry_hash[i].list);
+ dma_entry_hash[i].lock = SPIN_LOCK_UNLOCKED;
+ }
+
+ if (prealloc_memory(num_entries) != 0) {
+ printk(KERN_ERR "DMA-API: debugging out of memory error "
+ "- disabled\n");
+ global_disable = true;
+
+ return;
+ }
+
+ printk(KERN_INFO "DMA-API: debugging enabled by kernel config\n");
+}
+
--
1.5.6.4
Impact: implement necessary functions for the core hash
Signed-off-by: Joerg Roedel <[email protected]>
---
lib/dma-debug.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 101 insertions(+), 0 deletions(-)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index d04f8b6..74a0f36 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -18,9 +18,14 @@
*/
#include <linux/dma-debug.h>
+#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/list.h>
+#define HASH_SIZE 256
+#define HASH_FN_SHIFT 20
+#define HASH_FN_MASK 0xffULL
+
enum {
dma_debug_single,
dma_debug_sg,
@@ -37,3 +42,99 @@ struct dma_debug_entry {
int direction;
};
+struct hash_bucket {
+ struct list_head list;
+ spinlock_t lock;
+} ____cacheline_aligned;
+
+/* Hash list to save the allocated dma addresses */
+static struct hash_bucket dma_entry_hash[HASH_SIZE];
+
+/*
+ * Hash related functions
+ *
+ * Every DMA-API request is saved into a struct dma_debug_entry. To
+ * have quick access to these structs they are stored into a hash.
+ */
+static int hash_fn(struct dma_debug_entry *entry)
+{
+ /*
+ * Hash function is based on the dma address.
+ * We use bits 20-27 here as the index into the hash
+ */
+ return (entry->dev_addr >> HASH_FN_SHIFT) & HASH_FN_MASK;
+}
+
+/*
+ * Request exclusive access to a hash bucket for a given dma_debug_entry.
+ */
+static struct hash_bucket *get_hash_bucket(struct dma_debug_entry *entry,
+ unsigned long *flags)
+{
+ int idx = hash_fn(entry);
+ unsigned long __flags;
+
+ spin_lock_irqsave(&dma_entry_hash[idx].lock, __flags);
+ *flags = __flags;
+ return &dma_entry_hash[idx];
+}
+
+/*
+ * Give up exclusive access to the hash bucket
+ */
+static void put_hash_bucket(struct hash_bucket *bucket,
+ unsigned long *flags)
+{
+ unsigned long __flags = *flags;
+
+ spin_unlock_irqrestore(&bucket->lock, __flags);
+}
+
+/*
+ * Search a given entry in the hash bucket list
+ */
+static struct dma_debug_entry *hash_bucket_find(struct hash_bucket *bucket,
+ struct dma_debug_entry *ref)
+{
+ struct dma_debug_entry *entry;
+
+ list_for_each_entry(entry, &bucket->list, list) {
+ if ((entry->dev_addr == ref->dev_addr) &&
+ (entry->dev == ref->dev))
+ return entry;
+ }
+
+ return NULL;
+}
+
+/*
+ * Add an entry to a hash bucket
+ */
+static void hash_bucket_add(struct hash_bucket *bucket,
+ struct dma_debug_entry *entry)
+{
+ list_add_tail(&entry->list, &bucket->list);
+}
+
+/*
+ * Remove entry from a hash bucket list
+ */
+static void hash_bucket_del(struct dma_debug_entry *entry)
+{
+ list_del(&entry->list);
+}
+
+/*
+ * Wrapper function for adding an entry to the hash.
+ * This function takes care of locking itself.
+ */
+static void add_dma_entry(struct dma_debug_entry *entry)
+{
+ struct hash_bucket *bucket;
+ unsigned long flags;
+
+ bucket = get_hash_bucket(entry, &flags);
+ hash_bucket_add(bucket, entry);
+ put_hash_bucket(bucket, &flags);
+}
+
--
1.5.6.4
Impact: add debug callbacks for dma_{un}map_sg
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 15 ++++++++++++
lib/dma-debug.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 82ae9ca..b2131f4 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -34,6 +34,11 @@ extern void debug_map_single(struct device *dev, void *ptr, size_t size,
extern void debug_unmap_single(struct device *dev, dma_addr_t addr,
size_t size, int direction);
+extern void debug_map_sg(struct device *dev, struct scatterlist *sg,
+ int nents, int direction);
+
+extern void debug_unmap_sg(struct device *dev, struct scatterlist *sglist,
+ int nelems, int dir);
#else /* CONFIG_DMA_API_DEBUG */
@@ -52,6 +57,16 @@ static inline void debug_unmap_single(struct device *dev, dma_addr_t addr,
{
}
+static inline void debug_map_sg(struct device *dev, struct scatterlist *sg,
+ int nents, int direction)
+{
+}
+
+static inline void debug_unmap_sg(struct device *dev,
+ struct scatterlist *sglist,
+ int nelems, int dir)
+{
+}
#endif /* CONFIG_DMA_API_DEBUG */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index d4d14e5..e6d45f9 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -17,6 +17,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#include <linux/scatterlist.h>
#include <linux/dma-mapping.h>
#include <linux/dma-debug.h>
#include <linux/spinlock.h>
@@ -551,3 +552,55 @@ void debug_unmap_single(struct device *dev, dma_addr_t addr,
}
EXPORT_SYMBOL(debug_unmap_single);
+void debug_map_sg(struct device *dev, struct scatterlist *sg,
+ int nents, int direction)
+{
+ struct dma_debug_entry *entry;
+ struct scatterlist *s;
+ int i;
+
+ if (global_disable)
+ return;
+
+ for_each_sg(sg, s, nents, i) {
+ entry = dma_entry_alloc();
+ if (!entry)
+ return;
+
+ entry->type = dma_debug_sg;
+ entry->dev = dev;
+ entry->cpu_addr = sg_virt(s);
+ entry->size = s->length;
+ entry->dev_addr = s->dma_address;
+ entry->direction = direction;
+
+ add_dma_entry(entry);
+ }
+}
+EXPORT_SYMBOL(debug_map_sg);
+
+void debug_unmap_sg(struct device *dev, struct scatterlist *sglist,
+ int nelems, int dir)
+{
+ struct scatterlist *s;
+ int i;
+
+ if (global_disable)
+ return;
+
+ for_each_sg(sglist, s, nelems, i) {
+
+ struct dma_debug_entry ref = {
+ .type = dma_debug_sg,
+ .dev = dev,
+ .cpu_addr = sg_virt(s),
+ .dev_addr = s->dma_address,
+ .size = s->length,
+ .direction = dir,
+ };
+
+ check_unmap(&ref);
+ }
+}
+EXPORT_SYMBOL(debug_unmap_sg);
+
--
1.5.6.4
Impact: add functions to check on dma unmap and sync
Signed-off-by: Joerg Roedel <[email protected]>
---
lib/dma-debug.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 133 insertions(+), 0 deletions(-)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index ca0ccb1..9f730a4 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -17,9 +17,11 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#include <linux/dma-mapping.h>
#include <linux/dma-debug.h>
#include <linux/spinlock.h>
#include <linux/debugfs.h>
+#include <linux/device.h>
#include <linux/types.h>
#include <linux/list.h>
#include <linux/slab.h>
@@ -82,6 +84,22 @@ static struct dentry *show_num_errors_dent;
static struct dentry *num_free_entries_dent;
static struct dentry *min_free_entries_dent;
+static char *type2name[3] = { "single", "scather-gather", "coherent" };
+
+static char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",
+ "DMA_FROM_DEVICE", "DMA_NONE" };
+
+#define err_printk(dev, format, arg...) do { \
+ error_count += 1; \
+ if (show_all_errors || show_num_errors > 0) { \
+ WARN(1, "%s %s: " format, \
+ dev_driver_string(dev), \
+ dev_name(dev) , ## arg); \
+ } \
+ if (!show_all_errors && show_num_errors > 0) \
+ show_num_errors -= 1; \
+ } while (0);
+
/*
* Hash related functions
*
@@ -377,3 +395,118 @@ static __init int dma_debug_entries_cmdline(char *str)
__setup("dma_debug=", dma_debug_cmdline);
__setup("dma_debug_entries=", dma_debug_entries_cmdline);
+static void check_unmap(struct dma_debug_entry *ref)
+{
+ struct dma_debug_entry *entry;
+ struct hash_bucket *bucket;
+ unsigned long flags;
+
+ if (dma_mapping_error(ref->dev, ref->dev_addr))
+ return;
+
+ bucket = get_hash_bucket(ref, &flags);
+ entry = hash_bucket_find(bucket, ref);
+
+ if (!entry) {
+ err_printk(ref->dev, "DMA-API: device driver tries "
+ "to free DMA memory it has not allocated "
+ "[device address=0x%016llx] [size=%llu bytes]\n",
+ ref->dev_addr, ref->size);
+ goto out;
+ }
+
+ if (ref->size != entry->size) {
+ err_printk(ref->dev, "DMA-API: device driver frees "
+ "DMA memory with different size "
+ "[device address=0x%016llx] [map size=%llu bytes] "
+ "[unmap size=%llu bytes]\n",
+ ref->dev_addr, entry->size, ref->size);
+ }
+
+ if (ref->type != entry->type) {
+ err_printk(ref->dev, "DMA-API: device driver frees "
+ "DMA memory different that it was allocated "
+ "[device address=0x%016llx] [size=%llu bytes] "
+ "[mapped as %s] [unmapped as %s]\n",
+ ref->dev_addr, ref->size,
+ type2name[entry->type], type2name[ref->type]);
+ } else if ((entry->type == dma_debug_coherent) &&
+ (ref->cpu_addr != entry->cpu_addr)) {
+ err_printk(ref->dev, "DMA-API: device driver frees "
+ "DMA memory with different CPU address "
+ "[device address=0x%016llx] [size=%llu bytes] "
+ "[cpu alloc address=%p] [cpu free address=%p]",
+ ref->dev_addr, ref->size,
+ entry->cpu_addr, ref->cpu_addr);
+ }
+
+ /*
+ * * This may be no bug in reality - but most implementations of the
+ * * DMA API don't handle this properly, so check for it here
+ * */
+ if (ref->direction != entry->direction) {
+ err_printk(ref->dev, "DMA-API: device driver frees "
+ "DMA memory with different direction "
+ "[device address=0x%016llx] [size=%llu bytes] "
+ "[mapped with %s] [unmapped with %s]\n",
+ ref->dev_addr, ref->size,
+ dir2name[entry->direction],
+ dir2name[ref->direction]);
+ }
+
+ hash_bucket_del(entry);
+ dma_entry_free(entry);
+
+out:
+ put_hash_bucket(bucket, &flags);
+}
+
+static void check_sync(struct device *dev, dma_addr_t addr,
+ u64 size, u64 offset, int direction, bool to_cpu)
+{
+ struct dma_debug_entry ref = {
+ .dev = dev,
+ .dev_addr = addr,
+ .size = size,
+ .direction = direction,
+ };
+ struct dma_debug_entry *entry;
+ struct hash_bucket *bucket;
+ unsigned long flags;
+
+ bucket = get_hash_bucket(&ref, &flags);
+
+ entry = hash_bucket_find(bucket, &ref);
+
+ if (!entry) {
+ err_printk(dev, "DMA-API: device driver tries "
+ "to sync DMA memory it has not allocated "
+ "[device address=0x%016llx] [size=%llu bytes]\n",
+ addr, size);
+ goto out;
+ }
+
+ if ((offset + size) > entry->size) {
+ err_printk(dev, "DMA-API: device driver syncs"
+ " DMA memory outside allocated range "
+ "[device address=0x%016llx] "
+ "[allocation size=%llu bytes] [sync offset=%llu] "
+ "[sync size=%llu]\n", entry->dev_addr, entry->size,
+ offset, size);
+ }
+
+ if (direction != entry->direction) {
+ err_printk(dev, "DMA-API: device driver syncs "
+ "DMA memory with different direction "
+ "[device address=0x%016llx] [size=%llu bytes] "
+ "[mapped with %s] [synced with %s]\n",
+ addr, entry->size,
+ dir2name[entry->direction],
+ dir2name[direction]);
+ }
+
+out:
+ put_hash_bucket(bucket, &flags);
+
+}
+
--
1.5.6.4
Impact: make use of DMA-API debugging code in x86
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/include/asm/dma-mapping.h | 30 ++++++++++++++++++++++++++----
arch/x86/kernel/pci-dma.c | 5 +++++
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 862adb9..68a806c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -39,6 +39,7 @@ config X86
select HAVE_GENERIC_DMA_COHERENT if X86_32
select HAVE_EFFICIENT_UNALIGNED_ACCESS
select USER_STACKTRACE_SUPPORT
+ select HAVE_DMA_API_DEBUG
config ARCH_DEFCONFIG
string
diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
index 4035357..939d5b3 100644
--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -7,6 +7,7 @@
*/
#include <linux/scatterlist.h>
+#include <linux/dma-debug.h>
#include <asm/io.h>
#include <asm/swiotlb.h>
#include <asm-generic/dma-coherent.h>
@@ -93,9 +94,12 @@ dma_map_single(struct device *hwdev, void *ptr, size_t size,
int direction)
{
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
+ dma_addr_t addr;
BUG_ON(!valid_dma_direction(direction));
- return ops->map_single(hwdev, virt_to_phys(ptr), size, direction);
+ addr = ops->map_single(hwdev, virt_to_phys(ptr), size, direction);
+ debug_map_single(hwdev, ptr, size, direction, addr);
+ return addr;
}
static inline void
@@ -105,6 +109,7 @@ dma_unmap_single(struct device *dev, dma_addr_t addr, size_t size,
struct dma_mapping_ops *ops = get_dma_ops(dev);
BUG_ON(!valid_dma_direction(direction));
+ debug_unmap_single(dev, addr, size, direction);
if (ops->unmap_single)
ops->unmap_single(dev, addr, size, direction);
}
@@ -114,9 +119,13 @@ dma_map_sg(struct device *hwdev, struct scatterlist *sg,
int nents, int direction)
{
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
+ int ret;
BUG_ON(!valid_dma_direction(direction));
- return ops->map_sg(hwdev, sg, nents, direction);
+ ret = ops->map_sg(hwdev, sg, nents, direction);
+ debug_map_sg(hwdev, sg, ret, direction);
+
+ return ret;
}
static inline void
@@ -126,6 +135,7 @@ dma_unmap_sg(struct device *hwdev, struct scatterlist *sg, int nents,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_unmap_sg(hwdev, sg, nents, direction);
if (ops->unmap_sg)
ops->unmap_sg(hwdev, sg, nents, direction);
}
@@ -137,6 +147,7 @@ dma_sync_single_for_cpu(struct device *hwdev, dma_addr_t dma_handle,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_sync_single_for_cpu(hwdev, dma_handle, size, direction);
if (ops->sync_single_for_cpu)
ops->sync_single_for_cpu(hwdev, dma_handle, size, direction);
flush_write_buffers();
@@ -149,6 +160,7 @@ dma_sync_single_for_device(struct device *hwdev, dma_addr_t dma_handle,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_sync_single_for_device(hwdev, dma_handle, size, direction);
if (ops->sync_single_for_device)
ops->sync_single_for_device(hwdev, dma_handle, size, direction);
flush_write_buffers();
@@ -161,6 +173,8 @@ dma_sync_single_range_for_cpu(struct device *hwdev, dma_addr_t dma_handle,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_sync_single_range_for_cpu(hwdev, dma_handle, offset, size,
+ direction);
if (ops->sync_single_range_for_cpu)
ops->sync_single_range_for_cpu(hwdev, dma_handle, offset,
size, direction);
@@ -175,6 +189,8 @@ dma_sync_single_range_for_device(struct device *hwdev, dma_addr_t dma_handle,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_sync_single_range_for_device(hwdev, dma_handle, offset,
+ size, direction);
if (ops->sync_single_range_for_device)
ops->sync_single_range_for_device(hwdev, dma_handle,
offset, size, direction);
@@ -188,6 +204,7 @@ dma_sync_sg_for_cpu(struct device *hwdev, struct scatterlist *sg,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_sync_sg_for_cpu(hwdev, sg, nelems, direction);
if (ops->sync_sg_for_cpu)
ops->sync_sg_for_cpu(hwdev, sg, nelems, direction);
flush_write_buffers();
@@ -200,6 +217,7 @@ dma_sync_sg_for_device(struct device *hwdev, struct scatterlist *sg,
struct dma_mapping_ops *ops = get_dma_ops(hwdev);
BUG_ON(!valid_dma_direction(direction));
+ debug_sync_sg_for_device(hwdev, sg, nelems, direction);
if (ops->sync_sg_for_device)
ops->sync_sg_for_device(hwdev, sg, nelems, direction);
@@ -267,7 +285,7 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t gfp)
{
struct dma_mapping_ops *ops = get_dma_ops(dev);
- void *memory;
+ void *memory, *addr;
gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
@@ -285,8 +303,11 @@ 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,
+ addr = ops->alloc_coherent(dev, size, dma_handle,
dma_alloc_coherent_gfp_flags(dev, gfp));
+ debug_alloc_coherent(dev, size, *dma_handle, addr);
+
+ return addr;
}
static inline void dma_free_coherent(struct device *dev, size_t size,
@@ -299,6 +320,7 @@ static inline void dma_free_coherent(struct device *dev, size_t size,
if (dma_release_from_coherent(dev, get_order(size), vaddr))
return;
+ debug_free_coherent(dev, size, vaddr, bus);
if (ops->free_coherent)
ops->free_coherent(dev, size, vaddr, bus);
}
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index b254285..c8efbcc 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -44,6 +44,9 @@ struct device x86_dma_fallback_dev = {
};
EXPORT_SYMBOL(x86_dma_fallback_dev);
+/* Number of entries preallocated for DMA-API debugging */
+#define PREALLOC_ENTRIES 8192 /* needs 512kb */
+
int dma_set_mask(struct device *dev, u64 mask)
{
if (!dev->dma_mask || !dma_supported(dev, mask))
@@ -265,6 +268,8 @@ EXPORT_SYMBOL(dma_supported);
static int __init pci_iommu_init(void)
{
+ dma_debug_init(PREALLOC_ENTRIES);
+
calgary_iommu_init();
intel_iommu_init();
--
1.5.6.4
Impact: add groundwork for DMA-API debugging
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 25 +++++++++++++++++++++++++
lib/Makefile | 2 ++
lib/dma-debug.c | 39 +++++++++++++++++++++++++++++++++++++++
3 files changed, 66 insertions(+), 0 deletions(-)
create mode 100644 include/linux/dma-debug.h
create mode 100644 lib/dma-debug.c
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
new file mode 100644
index 0000000..ce4ace7
--- /dev/null
+++ b/include/linux/dma-debug.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2008 Advanced Micro Devices, Inc.
+ *
+ * Author: Joerg Roedel <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef __DMA_DEBUG_H
+#define __DMA_DEBUG_H
+
+struct device;
+
+#endif /* __DMA_DEBUG_H */
diff --git a/lib/Makefile b/lib/Makefile
index 32b0e64..50b48cf 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -84,6 +84,8 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
obj-$(CONFIG_DYNAMIC_PRINTK_DEBUG) += dynamic_printk.o
+obj-$(CONFIG_DMA_API_DEBUG) += dma-debug.o
+
hostprogs-y := gen_crc32table
clean-files := crc32table.h
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
new file mode 100644
index 0000000..d04f8b6
--- /dev/null
+++ b/lib/dma-debug.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2008 Advanced Micro Devices, Inc.
+ *
+ * Author: Joerg Roedel <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/dma-debug.h>
+#include <linux/types.h>
+#include <linux/list.h>
+
+enum {
+ dma_debug_single,
+ dma_debug_sg,
+ dma_debug_coherent,
+};
+
+struct dma_debug_entry {
+ struct list_head list;
+ struct device *dev;
+ int type;
+ void *cpu_addr;
+ u64 dev_addr;
+ u64 size;
+ int direction;
+};
+
--
1.5.6.4
Impact: add debug callbacks for dma_sync_single_for_* functions
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 20 ++++++++++++++++++++
lib/dma-debug.c | 20 ++++++++++++++++++++
2 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index a28a701..f39c2a8 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -46,6 +46,14 @@ extern void debug_alloc_coherent(struct device *dev, size_t size,
extern void debug_free_coherent(struct device *dev, size_t size,
void *virt, dma_addr_t addr);
+extern void debug_sync_single_for_cpu(struct device *dev,
+ dma_addr_t dma_handle, size_t size,
+ int direction);
+
+extern void debug_sync_single_for_device(struct device *dev,
+ dma_addr_t dma_handle,
+ size_t size, int direction);
+
#else /* CONFIG_DMA_API_DEBUG */
static inline void dma_debug_init(u32 num_entries)
@@ -84,6 +92,18 @@ static inline void debug_free_coherent(struct device *dev, size_t size,
{
}
+static inline void debug_sync_single_for_cpu(struct device *dev,
+ dma_addr_t dma_handle,
+ size_t size, int direction)
+{
+}
+
+static inline void debug_sync_single_for_device(struct device *dev,
+ dma_addr_t dma_handle,
+ size_t size, int direction)
+{
+}
+
#endif /* CONFIG_DMA_API_DEBUG */
#endif /* __DMA_DEBUG_H */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index a4a5b0f..a7f2369 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -649,3 +649,23 @@ void debug_free_coherent(struct device *dev, size_t size,
}
EXPORT_SYMBOL(debug_free_coherent);
+void debug_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle,
+ size_t size, int direction)
+{
+ if (global_disable)
+ return;
+
+ check_sync(dev, dma_handle, size, 0, direction, true);
+}
+EXPORT_SYMBOL(debug_sync_single_for_cpu);
+
+void debug_sync_single_for_device(struct device *dev, dma_addr_t dma_handle,
+ size_t size, int direction)
+{
+ if (global_disable)
+ return;
+
+ check_sync(dev, dma_handle, size, 0, direction, false);
+}
+EXPORT_SYMBOL(debug_sync_single_for_device);
+
--
1.5.6.4
Impact: add debug callbacks for dma_{un}map_single
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 19 +++++++++++++++++++
lib/dma-debug.c | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 345d538..82ae9ca 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -28,12 +28,31 @@ struct device;
extern void dma_debug_init(u32 num_entries);
+extern void debug_map_single(struct device *dev, void *ptr, size_t size,
+ int direction, dma_addr_t dma_addr);
+
+extern void debug_unmap_single(struct device *dev, dma_addr_t addr,
+ size_t size, int direction);
+
+
#else /* CONFIG_DMA_API_DEBUG */
static inline void dma_debug_init(u32 num_entries)
{
}
+static inline void debug_map_single(struct device *dev, void *ptr,
+ size_t size, int direction,
+ dma_addr_t dma_addr)
+{
+}
+
+static inline void debug_unmap_single(struct device *dev, dma_addr_t addr,
+ size_t size, int direction)
+{
+}
+
+
#endif /* CONFIG_DMA_API_DEBUG */
#endif /* __DMA_DEBUG_H */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 9f730a4..d4d14e5 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -510,3 +510,44 @@ out:
}
+void debug_map_single(struct device *dev, void *ptr, size_t size,
+ int direction, dma_addr_t dma_addr)
+{
+ struct dma_debug_entry *entry;
+
+ if (global_disable)
+ return;
+
+ entry = dma_entry_alloc();
+ if (!entry)
+ return;
+
+ entry->dev = dev;
+ entry->type = dma_debug_single;
+ entry->cpu_addr = ptr;
+ entry->dev_addr = dma_addr;
+ entry->size = size;
+ entry->direction = direction;
+
+ add_dma_entry(entry);
+}
+EXPORT_SYMBOL(debug_map_single);
+
+void debug_unmap_single(struct device *dev, dma_addr_t addr,
+ size_t size, int direction)
+{
+ struct dma_debug_entry ref = {
+ .type = dma_debug_single,
+ .dev = dev,
+ .dev_addr = addr,
+ .size = size,
+ .direction = direction,
+ };
+
+ if (global_disable)
+ return;
+
+ check_unmap(&ref);
+}
+EXPORT_SYMBOL(debug_unmap_single);
+
--
1.5.6.4
Impact: add dma_debug= and dma_debug_entries= kernel parameters
Signed-off-by: Joerg Roedel <[email protected]>
---
Documentation/kernel-parameters.txt | 10 +++++++++
lib/dma-debug.c | 38 +++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index fb84902..4045f76 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -486,6 +486,16 @@ and is between 256 and 4096 characters. It is defined in the file
Range: 0 - 8192
Default: 64
+ dma_debug=off If the kernel is compiled with DMA_API_DEBUG support
+ this option disables the debugging code at boot.
+
+ dma_debug_entries=<number>
+ This option allows to tune the number of preallocated
+ entries for DMA-API debugging code. One entry is
+ required per DMA-API allocation. Use this if the
+ DMA-API debugging code disables itself because the
+ architectural default is too low.
+
hpet= [X86-32,HPET] option to control HPET usage
Format: { enable (default) | disable | force }
disable: disable HPET and use PIT instead
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index b932f15..4e58d09 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -61,6 +61,9 @@ static bool global_disable __read_mostly;
static u32 num_free_entries;
static u32 min_free_entries;
+/* number of preallocated entries requested by kernel cmdline */
+static u32 req_entries;
+
/*
* Hash related functions
*
@@ -250,6 +253,9 @@ void dma_debug_init(u32 num_entries)
dma_entry_hash[i].lock = SPIN_LOCK_UNLOCKED;
}
+ if (req_entries)
+ num_entries = req_entries;
+
if (prealloc_memory(num_entries) != 0) {
printk(KERN_ERR "DMA-API: debugging out of memory error "
"- disabled\n");
@@ -261,3 +267,35 @@ void dma_debug_init(u32 num_entries)
printk(KERN_INFO "DMA-API: debugging enabled by kernel config\n");
}
+static __init int dma_debug_cmdline(char *str)
+{
+ if (!str)
+ return -EINVAL;
+
+ if (strncmp(str, "off", 3) == 0) {
+ printk(KERN_INFO "DMA-API: debugging disabled on kernel "
+ "command line\n");
+ global_disable = true;
+ }
+
+ return 0;
+}
+
+static __init int dma_debug_entries_cmdline(char *str)
+{
+ int res;
+
+ if (!str)
+ return -EINVAL;
+
+ res = get_option(&str, &req_entries);
+
+ if (!res)
+ req_entries = 0;
+
+ return 0;
+}
+
+__setup("dma_debug=", dma_debug_cmdline);
+__setup("dma_debug_entries=", dma_debug_entries_cmdline);
+
--
1.5.6.4
Impact: add debug callbacks for dma_[alloc|free]_coherent
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 16 ++++++++++++++++
lib/dma-debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index b2131f4..a28a701 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -40,6 +40,12 @@ extern void debug_map_sg(struct device *dev, struct scatterlist *sg,
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 void dma_debug_init(u32 num_entries)
@@ -68,6 +74,16 @@ static inline void debug_unmap_sg(struct device *dev,
{
}
+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 /* __DMA_DEBUG_H */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index e6d45f9..a4a5b0f 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -604,3 +604,48 @@ 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)
+{
+ struct dma_debug_entry *entry;
+
+ if (global_disable)
+ return;
+
+ 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;
+
+ add_dma_entry(entry);
+}
+EXPORT_SYMBOL(debug_alloc_coherent);
+
+void debug_free_coherent(struct device *dev, size_t size,
+ void *virt, dma_addr_t addr)
+{
+ struct dma_debug_entry ref = {
+ .type = dma_debug_coherent,
+ .dev = dev,
+ .cpu_addr = virt,
+ .dev_addr = addr,
+ .size = size,
+ .direction = DMA_BIDIRECTIONAL,
+ };
+
+ if (global_disable)
+ return;
+
+ check_unmap(&ref);
+}
+EXPORT_SYMBOL(debug_free_coherent);
+
--
1.5.6.4
Impact: add a Kconfig entry for DMA-API debugging
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/Kconfig | 2 ++
lib/Kconfig.debug | 11 +++++++++++
2 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 2e13aa2..068554c 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -103,3 +103,5 @@ config HAVE_CLK
The <linux/clk.h> calls support software clock gating and
thus are a key power management tool on many systems.
+config HAVE_DMA_API_DEBUG
+ bool
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2e75478..cba5778 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -899,6 +899,17 @@ config DYNAMIC_PRINTK_DEBUG
debugging for all modules. This mode can be turned off via the above
disable command.
+config DMA_API_DEBUG
+ bool "Enable debugging of DMA-API usage"
+ depends on HAVE_DMA_API_DEBUG
+ help
+ Enable this option to debug the use of the DMA API by device drivers.
+ With this option you will be able to detect common bugs in device
+ drivers like double-freeing of DMA mappings or freeing mappings that
+ were never allocated.
+ This option causes a performance degredation. Use only if you want
+ to debug device drivers. If unsure, say N.
+
source "samples/Kconfig"
source "lib/Kconfig.kgdb"
--
1.5.6.4
Impact: add allocator code for struct dma_debug_entry
Signed-off-by: Joerg Roedel <[email protected]>
---
lib/dma-debug.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 74a0f36..c5444d4 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -49,6 +49,16 @@ struct hash_bucket {
/* Hash list to save the allocated dma addresses */
static struct hash_bucket dma_entry_hash[HASH_SIZE];
+/* List of pre-allocated dma_debug_entry's */
+static LIST_HEAD(free_entries);
+/* Lock for the list above */
+static DEFINE_SPINLOCK(free_entries_lock);
+
+/* Global disable flag - will be set in case of an error */
+static bool global_disable __read_mostly;
+
+static u32 num_free_entries;
+static u32 min_free_entries;
/*
* Hash related functions
@@ -138,3 +148,50 @@ static void add_dma_entry(struct dma_debug_entry *entry)
put_hash_bucket(bucket, &flags);
}
+/* struct dma_entry allocator
+ *
+ * The next two functions implement the allocator for
+ * struct dma_debug_entries.
+ */
+static struct dma_debug_entry *dma_entry_alloc(void)
+{
+ struct dma_debug_entry *entry = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(&free_entries_lock, flags);
+
+ if (list_empty(&free_entries)) {
+ printk(KERN_ERR "DMA-API: debugging out of memory "
+ "- disabling\n");
+ global_disable = true;
+ goto out;
+ }
+
+ entry = list_entry(free_entries.next, struct dma_debug_entry, list);
+ list_del(&entry->list);
+ memset(entry, 0, sizeof(*entry));
+
+ num_free_entries -= 1;
+ if (num_free_entries < min_free_entries)
+ min_free_entries = num_free_entries;
+
+out:
+ spin_unlock_irqrestore(&free_entries_lock, flags);
+
+ return entry;
+}
+
+static void dma_entry_free(struct dma_debug_entry *entry)
+{
+ unsigned long flags;
+
+ /*
+ * add to beginning of the list - this way the entries are
+ * more likely cache hot when they are reallocated.
+ */
+ spin_lock_irqsave(&free_entries_lock, flags);
+ list_add(&entry->list, &free_entries);
+ num_free_entries += 1;
+ spin_unlock_irqrestore(&free_entries_lock, flags);
+}
+
--
1.5.6.4
Impact: add debug callbacks for dma_sync_sg_* functions
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-debug.h | 19 +++++++++++++++++++
lib/dma-debug.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index b9c221a..bdba8c8 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -64,6 +64,13 @@ extern void debug_sync_single_range_for_device(struct device *dev,
unsigned long offset,
size_t size, int direction);
+extern void debug_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
+ int nelems, int direction);
+
+extern void debug_sync_sg_for_device(struct device *dev,
+ struct scatterlist *sg,
+ int nelems, int direction);
+
#else /* CONFIG_DMA_API_DEBUG */
static inline void dma_debug_init(u32 num_entries)
@@ -130,6 +137,18 @@ static inline void debug_sync_single_range_for_device(struct device *dev,
{
}
+static inline void debug_sync_sg_for_cpu(struct device *dev,
+ struct scatterlist *sg,
+ int nelems, int direction)
+{
+}
+
+static inline void debug_sync_sg_for_device(struct device *dev,
+ struct scatterlist *sg,
+ int nelems, int direction)
+{
+}
+
#endif /* CONFIG_DMA_API_DEBUG */
#endif /* __DMA_DEBUG_H */
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 6f73bed..e40c88c 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -692,3 +692,35 @@ void debug_sync_single_range_for_device(struct device *dev,
}
EXPORT_SYMBOL(debug_sync_single_range_for_device);
+void debug_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
+ int nelems, int direction)
+{
+ struct scatterlist *s;
+ int i;
+
+ if (global_disable)
+ return;
+
+ for_each_sg(sg, s, nelems, i) {
+ check_sync(dev, s->dma_address, s->dma_length, 0,
+ direction, true);
+ }
+}
+EXPORT_SYMBOL(debug_sync_sg_for_cpu);
+
+void debug_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
+ int nelems, int direction)
+{
+ struct scatterlist *s;
+ int i;
+
+ if (global_disable)
+ return;
+
+ for_each_sg(sg, s, nelems, i) {
+ check_sync(dev, s->dma_address, s->dma_length, 0,
+ direction, false);
+ }
+}
+EXPORT_SYMBOL(debug_sync_sg_for_device);
+
--
1.5.6.4
Hi Joerg.
On Fri, Jan 09, 2009 at 05:19:17PM +0100, Joerg Roedel ([email protected]) wrote:
> +/*
> + * Request exclusive access to a hash bucket for a given dma_debug_entry.
> + */
> +static struct hash_bucket *get_hash_bucket(struct dma_debug_entry *entry,
> + unsigned long *flags)
> +{
> + int idx = hash_fn(entry);
> + unsigned long __flags;
> +
> + spin_lock_irqsave(&dma_entry_hash[idx].lock, __flags);
> + *flags = __flags;
> + return &dma_entry_hash[idx];
> +}
> +
> +/*
> + * Give up exclusive access to the hash bucket
> + */
> +static void put_hash_bucket(struct hash_bucket *bucket,
> + unsigned long *flags)
> +{
> + unsigned long __flags = *flags;
> +
> + spin_unlock_irqrestore(&bucket->lock, __flags);
> +}
Why do you need such ugly helpers?
> + * Add an entry to a hash bucket
> + */
> +static void hash_bucket_add(struct hash_bucket *bucket,
> + struct dma_debug_entry *entry)
> +{
> + list_add_tail(&entry->list, &bucket->list);
> +}
> +/*
> + * Remove entry from a hash bucket list
> + */
> +static void hash_bucket_del(struct dma_debug_entry *entry)
> +{
> + list_del(&entry->list);
> +}
Do you really need this getting they are called only from single place?
--
Evgeniy Polyakov
On Fri, Jan 09, 2009 at 05:19:19PM +0100, Joerg Roedel ([email protected]) wrote:
+static int prealloc_memory(u32 num_entries)
> +{
> + struct dma_debug_entry *entry, *next_entry;
> + int i;
> +
> + for (i = 0; i < num_entries; ++i) {
> + entry = kmalloc(sizeof(*entry), GFP_KERNEL | __GFP_ZERO);
kzalloc?
--
Evgeniy Polyakov
On Fri, Jan 09, 2009 at 05:19:24PM +0100, Joerg Roedel ([email protected]) wrote:
> +void debug_map_sg(struct device *dev, struct scatterlist *sg,
> + int nents, int direction)
> +{
> + struct dma_debug_entry *entry;
> + struct scatterlist *s;
> + int i;
> +
> + if (global_disable)
> + return;
> +
> + for_each_sg(sg, s, nents, i) {
> + entry = dma_entry_alloc();
> + if (!entry)
> + return;
> +
> + entry->type = dma_debug_sg;
> + entry->dev = dev;
> + entry->cpu_addr = sg_virt(s);
> + entry->size = s->length;
> + entry->dev_addr = s->dma_address;
> + entry->direction = direction;
> +
> + add_dma_entry(entry);
> + }
> +}
> +EXPORT_SYMBOL(debug_map_sg);
> +
> +void debug_unmap_sg(struct device *dev, struct scatterlist *sglist,
> + int nelems, int dir)
> +{
> + struct scatterlist *s;
> + int i;
> +
> + if (global_disable)
> + return;
> +
> + for_each_sg(sglist, s, nelems, i) {
> +
> + struct dma_debug_entry ref = {
> + .type = dma_debug_sg,
> + .dev = dev,
> + .cpu_addr = sg_virt(s),
> + .dev_addr = s->dma_address,
> + .size = s->length,
> + .direction = dir,
> + };
> +
> + check_unmap(&ref);
Will this print false errors if above map debug failed to add an entry
into the list?
--
Evgeniy Polyakov
On Fri, Jan 09, 2009 at 09:08:36PM +0300, Evgeniy Polyakov wrote:
> On Fri, Jan 09, 2009 at 05:19:24PM +0100, Joerg Roedel ([email protected]) wrote:
> > +void debug_map_sg(struct device *dev, struct scatterlist *sg,
> > + int nents, int direction)
> > +{
> > + struct dma_debug_entry *entry;
> > + struct scatterlist *s;
> > + int i;
> > +
> > + if (global_disable)
> > + return;
> > +
> > + for_each_sg(sg, s, nents, i) {
> > + entry = dma_entry_alloc();
> > + if (!entry)
> > + return;
> > +
> > + entry->type = dma_debug_sg;
> > + entry->dev = dev;
> > + entry->cpu_addr = sg_virt(s);
> > + entry->size = s->length;
> > + entry->dev_addr = s->dma_address;
> > + entry->direction = direction;
> > +
> > + add_dma_entry(entry);
> > + }
> > +}
> > +EXPORT_SYMBOL(debug_map_sg);
> > +
> > +void debug_unmap_sg(struct device *dev, struct scatterlist *sglist,
> > + int nelems, int dir)
> > +{
> > + struct scatterlist *s;
> > + int i;
> > +
> > + if (global_disable)
> > + return;
> > +
> > + for_each_sg(sglist, s, nelems, i) {
> > +
> > + struct dma_debug_entry ref = {
> > + .type = dma_debug_sg,
> > + .dev = dev,
> > + .cpu_addr = sg_virt(s),
> > + .dev_addr = s->dma_address,
> > + .size = s->length,
> > + .direction = dir,
> > + };
> > +
> > + check_unmap(&ref);
>
> Will this print false errors if above map debug failed to add an entry
> into the list?
No. The code disables itself if adding an entry fails. This can only
happen when we run out of preallocated dma_debug_entries.
Joerg
On Fri, Jan 09, 2009 at 08:55:42PM +0300, Evgeniy Polyakov wrote:
> Hi Joerg.
>
> On Fri, Jan 09, 2009 at 05:19:17PM +0100, Joerg Roedel ([email protected]) wrote:
> > +/*
> > + * Request exclusive access to a hash bucket for a given dma_debug_entry.
> > + */
> > +static struct hash_bucket *get_hash_bucket(struct dma_debug_entry *entry,
> > + unsigned long *flags)
> > +{
> > + int idx = hash_fn(entry);
> > + unsigned long __flags;
> > +
> > + spin_lock_irqsave(&dma_entry_hash[idx].lock, __flags);
> > + *flags = __flags;
> > + return &dma_entry_hash[idx];
> > +}
> > +
> > +/*
> > + * Give up exclusive access to the hash bucket
> > + */
> > +static void put_hash_bucket(struct hash_bucket *bucket,
> > + unsigned long *flags)
> > +{
> > + unsigned long __flags = *flags;
> > +
> > + spin_unlock_irqrestore(&bucket->lock, __flags);
> > +}
>
> Why do you need such ugly helpers?
Because everything else I thought about here was even more ugly. But
maybe you have a better idea? I tried to lock directly in the debug_
functions. But this is ugly and unnecessary code duplication.
>
> > + * Add an entry to a hash bucket
> > + */
> > +static void hash_bucket_add(struct hash_bucket *bucket,
> > + struct dma_debug_entry *entry)
> > +{
> > + list_add_tail(&entry->list, &bucket->list);
> > +}
>
> > +/*
> > + * Remove entry from a hash bucket list
> > + */
> > +static void hash_bucket_del(struct dma_debug_entry *entry)
> > +{
> > + list_del(&entry->list);
> > +}
>
> Do you really need this getting they are called only from single place?
Hmm, true. I will inline these functions.
Joerg
On Fri, Jan 09, 2009 at 08:58:42PM +0300, Evgeniy Polyakov wrote:
> On Fri, Jan 09, 2009 at 05:19:19PM +0100, Joerg Roedel ([email protected]) wrote:
> +static int prealloc_memory(u32 num_entries)
> > +{
> > + struct dma_debug_entry *entry, *next_entry;
> > + int i;
> > +
> > + for (i = 0; i < num_entries; ++i) {
> > + entry = kmalloc(sizeof(*entry), GFP_KERNEL | __GFP_ZERO);
>
> kzalloc?
True. kzalloc is better. I will change that.
Joerg
On Fri, Jan 09, 2009 at 07:14:46PM +0100, Joerg Roedel ([email protected]) wrote:
> > > +static struct hash_bucket *get_hash_bucket(struct dma_debug_entry *entry,
> > > + unsigned long *flags)
> > > +{
> > > + int idx = hash_fn(entry);
> > > + unsigned long __flags;
> > > +
> > > + spin_lock_irqsave(&dma_entry_hash[idx].lock, __flags);
> > > + *flags = __flags;
> > > + return &dma_entry_hash[idx];
> > > +}
> > > +
> > > +/*
> > > + * Give up exclusive access to the hash bucket
> > > + */
> > > +static void put_hash_bucket(struct hash_bucket *bucket,
> > > + unsigned long *flags)
> > > +{
> > > + unsigned long __flags = *flags;
> > > +
> > > + spin_unlock_irqrestore(&bucket->lock, __flags);
> > > +}
> >
> > Why do you need such ugly helpers?
>
> Because everything else I thought about here was even more ugly. But
> maybe you have a better idea? I tried to lock directly in the debug_
> functions. But this is ugly and unnecessary code duplication.
I believe that having direct locking in the debug_ functions is not a
duplication, anyone will have a direct vision on the locking and hash
array dereference, and this will be just one additional line compared to
the get_* call and the same number of lines for the put :)
--
Evgeniy Polyakov
On Fri, Jan 09, 2009 at 09:23:39PM +0300, Evgeniy Polyakov wrote:
> On Fri, Jan 09, 2009 at 07:14:46PM +0100, Joerg Roedel ([email protected]) wrote:
> > > > +static struct hash_bucket *get_hash_bucket(struct dma_debug_entry *entry,
> > > > + unsigned long *flags)
> > > > +{
> > > > + int idx = hash_fn(entry);
> > > > + unsigned long __flags;
> > > > +
> > > > + spin_lock_irqsave(&dma_entry_hash[idx].lock, __flags);
> > > > + *flags = __flags;
> > > > + return &dma_entry_hash[idx];
> > > > +}
> > > > +
> > > > +/*
> > > > + * Give up exclusive access to the hash bucket
> > > > + */
> > > > +static void put_hash_bucket(struct hash_bucket *bucket,
> > > > + unsigned long *flags)
> > > > +{
> > > > + unsigned long __flags = *flags;
> > > > +
> > > > + spin_unlock_irqrestore(&bucket->lock, __flags);
> > > > +}
> > >
> > > Why do you need such ugly helpers?
> >
> > Because everything else I thought about here was even more ugly. But
> > maybe you have a better idea? I tried to lock directly in the debug_
> > functions. But this is ugly and unnecessary code duplication.
>
> I believe that having direct locking in the debug_ functions is not a
> duplication, anyone will have a direct vision on the locking and hash
> array dereference, and this will be just one additional line compared to
> the get_* call and the same number of lines for the put :)
Even more additional lines because of the additional variables needed in
every function. Anyway, I try it and if it does not look good I will
keep that change ;)
Joerg
On Fri, 9 Jan 2009 17:19:15 +0100 Joerg Roedel wrote:
> Impact: add a Kconfig entry for DMA-API debugging
Surely Impact: is (meant to be) different from the actual patch description... ?
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> arch/Kconfig | 2 ++
> lib/Kconfig.debug | 11 +++++++++++
> 2 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 2e13aa2..068554c 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -103,3 +103,5 @@ config HAVE_CLK
> The <linux/clk.h> calls support software clock gating and
> thus are a key power management tool on many systems.
>
> +config HAVE_DMA_API_DEBUG
> + bool
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 2e75478..cba5778 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -899,6 +899,17 @@ config DYNAMIC_PRINTK_DEBUG
> debugging for all modules. This mode can be turned off via the above
> disable command.
>
> +config DMA_API_DEBUG
> + bool "Enable debugging of DMA-API usage"
> + depends on HAVE_DMA_API_DEBUG
> + help
> + Enable this option to debug the use of the DMA API by device drivers.
> + With this option you will be able to detect common bugs in device
> + drivers like double-freeing of DMA mappings or freeing mappings that
> + were never allocated.
> + This option causes a performance degredation. Use only if you want
> + to debug device drivers. If unsure, say N.
> +
> source "samples/Kconfig"
>
> source "lib/Kconfig.kgdb"
> --
---
~Randy
On Fri, 2009-01-09 at 08:19 -0800, Joerg Roedel wrote:
>
> With the code these patches introduce driver developers can find several
> bugs of misusing the DMA-API in their drivers. But be aware, it can not
> find all possible bugs. If it finds a problem it prints out messages
> like
>
> ------------[ cut here ]------------
> WARNING: at /data2/repos/linux.trees.git/lib/dma-debug.c:231 check_unmap+0xab/0x3d9()
> Hardware name: Toonie
> bnx2 0000:01:00.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x00000000011]
> Modules linked in:
> Pid: 0, comm: swapper Not tainted 2.6.28 #174
> Call Trace:
> <IRQ> [<ffffffff8105af3a>] warn_slowpath+0xd3/0xf2
> [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> [<ffffffff812efd16>] ? usb_hcd_link_urb_to_ep+0x94/0xa0
> [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> [<ffffffff814b25a5>] ? _spin_lock_irqsave+0x69/0x75
> [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> [<ffffffff811e2ff2>] check_unmap+0xab/0x3d9
> [<ffffffff8107c9ed>] ? trace_hardirqs_on_caller+0x108/0x14a
> [<ffffffff8107ca3c>] ? trace_hardirqs_on+0xd/0xf
> [<ffffffff811e3433>] debug_unmap_single+0x3e/0x40
> [<ffffffff8128d2d8>] dma_unmap_single+0x3d/0x60
> [<ffffffff8128d335>] pci_unmap_page+0x1c/0x1e
> [<ffffffff81290759>] bnx2_poll_work+0x626/0x8cb
> [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> [<ffffffff81070100>] ? run_posix_cpu_timers+0x49c/0x603
> [<ffffffff81070000>] ? run_posix_cpu_timers+0x39c/0x603
> [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> [<ffffffff81292804>] bnx2_poll_msix+0x33/0x81
> [<ffffffff813b6478>] net_rx_action+0x8a/0x139
> [<ffffffff8105ff39>] __do_softirq+0x8b/0x147
> [<ffffffff8102933c>] call_softirq+0x1c/0x34
> [<ffffffff8102a611>] do_softirq+0x39/0x90
> [<ffffffff8105fde8>] irq_exit+0x4e/0x98
> [<ffffffff8102a5c2>] do_IRQ+0x11f/0x135
> [<ffffffff81028b93>] ret_from_intr+0x0/0xf
> <EOI> <4>---[ end trace 4339d58302097423 ]---
>
This was triggered during pci_unmap_page() -> dma_unmap_single() where
check_unmap() did not find the entry.
The original mapping was done in bnx2 using pci_map_page(). I did not
see how the debug entry was added to the hash during the call to
pci_map_page() -> dma_map_page(). Did I miss something?
Thanks.
On Fri, Jan 09, 2009 at 01:24:24PM -0800, Michael Chan wrote:
>
> On Fri, 2009-01-09 at 08:19 -0800, Joerg Roedel wrote:
> >
> > With the code these patches introduce driver developers can find several
> > bugs of misusing the DMA-API in their drivers. But be aware, it can not
> > find all possible bugs. If it finds a problem it prints out messages
> > like
> >
> > ------------[ cut here ]------------
> > WARNING: at /data2/repos/linux.trees.git/lib/dma-debug.c:231 check_unmap+0xab/0x3d9()
> > Hardware name: Toonie
> > bnx2 0000:01:00.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x00000000011]
> > Modules linked in:
> > Pid: 0, comm: swapper Not tainted 2.6.28 #174
> > Call Trace:
> > <IRQ> [<ffffffff8105af3a>] warn_slowpath+0xd3/0xf2
> > [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> > [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> > [<ffffffff812efd16>] ? usb_hcd_link_urb_to_ep+0x94/0xa0
> > [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> > [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> > [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> > [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> > [<ffffffff814b25a5>] ? _spin_lock_irqsave+0x69/0x75
> > [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> > [<ffffffff811e2ff2>] check_unmap+0xab/0x3d9
> > [<ffffffff8107c9ed>] ? trace_hardirqs_on_caller+0x108/0x14a
> > [<ffffffff8107ca3c>] ? trace_hardirqs_on+0xd/0xf
> > [<ffffffff811e3433>] debug_unmap_single+0x3e/0x40
> > [<ffffffff8128d2d8>] dma_unmap_single+0x3d/0x60
> > [<ffffffff8128d335>] pci_unmap_page+0x1c/0x1e
> > [<ffffffff81290759>] bnx2_poll_work+0x626/0x8cb
> > [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> > [<ffffffff81070100>] ? run_posix_cpu_timers+0x49c/0x603
> > [<ffffffff81070000>] ? run_posix_cpu_timers+0x39c/0x603
> > [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> > [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> > [<ffffffff81292804>] bnx2_poll_msix+0x33/0x81
> > [<ffffffff813b6478>] net_rx_action+0x8a/0x139
> > [<ffffffff8105ff39>] __do_softirq+0x8b/0x147
> > [<ffffffff8102933c>] call_softirq+0x1c/0x34
> > [<ffffffff8102a611>] do_softirq+0x39/0x90
> > [<ffffffff8105fde8>] irq_exit+0x4e/0x98
> > [<ffffffff8102a5c2>] do_IRQ+0x11f/0x135
> > [<ffffffff81028b93>] ret_from_intr+0x0/0xf
> > <EOI> <4>---[ end trace 4339d58302097423 ]---
> >
> This was triggered during pci_unmap_page() -> dma_unmap_single() where
> check_unmap() did not find the entry.
>
> The original mapping was done in bnx2 using pci_map_page(). I did not
> see how the debug entry was added to the hash during the call to
> pci_map_page() -> dma_map_page(). Did I miss something?
dma_map_page() results in dma_map_single() -> debug_map_single() call on
x86. This way the entry would be added. Maybe the error from a double
free?
Joerg
On Fri, Jan 09, 2009 at 11:33:27PM +0100, Joerg Roedel wrote:
> > This was triggered during pci_unmap_page() -> dma_unmap_single() where
> > check_unmap() did not find the entry.
> >
> > The original mapping was done in bnx2 using pci_map_page(). I did not
> > see how the debug entry was added to the hash during the call to
> > pci_map_page() -> dma_map_page(). Did I miss something?
>
> dma_map_page() results in dma_map_single() -> debug_map_single() call on
> x86. This way the entry would be added. Maybe the error from a double
> free?
Ah, it only calls ops->map_single. Thanks for pointing that out. I will
add a call to debug_map_single to dma_map_page too. So the error above
may be a false positive.
Joerg
* Joerg Roedel <[email protected]> wrote:
> @@ -105,6 +109,7 @@ dma_unmap_single(struct device *dev, dma_addr_t addr, size_t size,
> struct dma_mapping_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(direction));
> + debug_unmap_single(dev, addr, size, direction);
It all looks very nice, i've got one small namespace structure request:
could you please name all the callbacks in a consistent way, so that they
mirror the method they instrument - with a "debug_" prefix?
I.e. the above one would be: debug_dma_unmap_single().
Ingo
* Joerg Roedel <[email protected]> wrote:
> +/* Global error count */
> +static u32 error_count;
> +
> +/* Global error show enable*/
> +static u32 show_all_errors __read_mostly;
> +/* Number of errors to show */
> +static u32 show_num_errors = 1;
> +
> static u32 num_free_entries;
> static u32 min_free_entries;
Small detail: please use native C types for non-hardware variables -
'unsigned int', 'unsigned long', etc.
u32/u64 is typically used for variables where there is a real significance
to the precise width of the variable: there's either a hardware or a
user-space ABI involved.
Ingo
* Joerg Roedel <[email protected]> wrote:
> +#define err_printk(dev, format, arg...) do { \
> + error_count += 1; \
> + if (show_all_errors || show_num_errors > 0) { \
> + WARN(1, "%s %s: " format, \
> + dev_driver_string(dev), \
> + dev_name(dev) , ## arg); \
> + } \
> + if (!show_all_errors && show_num_errors > 0) \
> + show_num_errors -= 1; \
Note that the arithmetics here is SMP-unsafe: we only hold the hash bucket
so if two errors hit at once on two CPUs then the error tracking variables
can be accessed at once.
I'd suggest a simple global lock for this error case (taken inside the
hash bucket lock), to be on the safe side.
Also, please dont use a macro for this - printk details can be passed in
to helper inlines/functions too.
Ingo
* Joerg Roedel <[email protected]> wrote:
> + /*
> + * * This may be no bug in reality - but most implementations of the
> + * * DMA API don't handle this properly, so check for it here
> + * */
You must be using Vim and copy & paste messed up? :)
Ingo
* Joerg Roedel <[email protected]> wrote:
> +static char *type2name[3] = { "single", "scather-gather", "coherent" };
> +
> +static char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",
> + "DMA_FROM_DEVICE", "DMA_NONE" };
Should be const i guess.
Ingo
* Joerg Roedel <[email protected]> wrote:
> +/* debugfs dentry's for the stuff above */
> +static struct dentry *dma_debug_dent;
> +static struct dentry *global_disable_dent;
> +static struct dentry *error_count_dent;
> +static struct dentry *show_all_errors_dent;
> +static struct dentry *show_num_errors_dent;
> +static struct dentry *num_free_entries_dent;
> +static struct dentry *min_free_entries_dent;
should all be __read_mostly.
Ingo
* Joerg Roedel <[email protected]> wrote:
> + printk(KERN_ERR "DMA-API: debugging out of memory "
> + "- disabling\n");
btw., i'd suggest to not break kernel messages mid-string, but do
something like this instead:
> + printk(KERN_ERR
"DMA-API: debugging out of memory - disabling\n");
Also, i'd use WARN() - it might be useful to see what callsite depleted
the pool.
> + entry = list_entry(free_entries.next, struct dma_debug_entry, list);
> + list_del(&entry->list);
> + memset(entry, 0, sizeof(*entry));
> +
> + num_free_entries -= 1;
> + if (num_free_entries < min_free_entries)
> + min_free_entries = num_free_entries;
unlikely() i guess.
Regarding the entry pool locking:
> +static void dma_entry_free(struct dma_debug_entry *entry)
> +{
> + unsigned long flags;
> +
> + /*
> + * add to beginning of the list - this way the entries are
> + * more likely cache hot when they are reallocated.
> + */
> + spin_lock_irqsave(&free_entries_lock, flags);
> + list_add(&entry->list, &free_entries);
> + num_free_entries += 1;
> + spin_unlock_irqrestore(&free_entries_lock, flags);
it might make sense to cache entries in the buckets - hence reuse the
bucket spinlock. This means a somewhat higher effective pool size, but it
also avoids a global lock.
Ingo
* Joerg Roedel <[email protected]> wrote:
> +void debug_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
> + int nelems, int direction)
> +{
> + struct scatterlist *s;
> + int i;
> +
> + if (global_disable)
> + return;
"if (unlikely(global_disable))" i suspect?
Ingo
* Joerg Roedel <[email protected]> wrote:
> @@ -175,6 +189,8 @@ dma_sync_single_range_for_device(struct device *hwdev, dma_addr_t dma_handle,
> struct dma_mapping_ops *ops = get_dma_ops(hwdev);
>
> BUG_ON(!valid_dma_direction(direction));
> + debug_sync_single_range_for_device(hwdev, dma_handle, offset,
whitespace damage here.
Ingo
* Joerg Roedel <[email protected]> wrote:
> Hi,
>
> this is version 2 of the patchset which introduces code to debug drivers
> usage of the DMA-API. Many thanks to all the reviewers and the useful
> comments on the fist version of this patchset. Tests with hardware
> IOMMUs have shown several bugs in drivers regarding the usage of that
> API. Problems were found especially in network card drivers.
>
> These bugs often don't show up or have any negative impact if there is
> no hardware IOMMU in use in the system. But with an hardware IOMMU these
> bugs turn the hardware unusable or, in the worst case, cause data
> corruption on devices which are managed by other (good) drivers.
>
> With the code these patches introduce driver developers can find several
> bugs of misusing the DMA-API in their drivers. But be aware, it can not
> find all possible bugs. If it finds a problem it prints out messages
> like
>
> ------------[ cut here ]------------
> WARNING: at /data2/repos/linux.trees.git/lib/dma-debug.c:231 check_unmap+0xab/0x3d9()
> Hardware name: Toonie
> bnx2 0000:01:00.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x00000000011]
> Modules linked in:
> Pid: 0, comm: swapper Not tainted 2.6.28 #174
> Call Trace:
> <IRQ> [<ffffffff8105af3a>] warn_slowpath+0xd3/0xf2
> [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> [<ffffffff812efd16>] ? usb_hcd_link_urb_to_ep+0x94/0xa0
> [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> [<ffffffff814b25a5>] ? _spin_lock_irqsave+0x69/0x75
> [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> [<ffffffff811e2ff2>] check_unmap+0xab/0x3d9
> [<ffffffff8107c9ed>] ? trace_hardirqs_on_caller+0x108/0x14a
> [<ffffffff8107ca3c>] ? trace_hardirqs_on+0xd/0xf
> [<ffffffff811e3433>] debug_unmap_single+0x3e/0x40
> [<ffffffff8128d2d8>] dma_unmap_single+0x3d/0x60
> [<ffffffff8128d335>] pci_unmap_page+0x1c/0x1e
> [<ffffffff81290759>] bnx2_poll_work+0x626/0x8cb
> [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> [<ffffffff81070100>] ? run_posix_cpu_timers+0x49c/0x603
> [<ffffffff81070000>] ? run_posix_cpu_timers+0x39c/0x603
> [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> [<ffffffff81292804>] bnx2_poll_msix+0x33/0x81
> [<ffffffff813b6478>] net_rx_action+0x8a/0x139
> [<ffffffff8105ff39>] __do_softirq+0x8b/0x147
> [<ffffffff8102933c>] call_softirq+0x1c/0x34
> [<ffffffff8102a611>] do_softirq+0x39/0x90
> [<ffffffff8105fde8>] irq_exit+0x4e/0x98
> [<ffffffff8102a5c2>] do_IRQ+0x11f/0x135
> [<ffffffff81028b93>] ret_from_intr+0x0/0xf
> <EOI> <4>---[ end trace 4339d58302097423 ]---
>
> This way driver developers get an idea where the problem is in their
> code.
>
> I hope I addressed most of the review comments and objections from the
> first version. Please give this version also a good review and send me
> your comments.
Looks pretty good in general - modulo the few observations i just made in
reply to the patches. (Note, the comments apply to all the patches -
there's similar small issues in other places as well.)
Did you have a chance to look at debugobjects and see whether it could be
reused as the dma-debug-entry object management code?
One request: could you please git-base it on the changes in tip/core/iommu
(i only have looked at the patches in email - i presume the current ones
are based on -git)?
That way we'll have it on top of Fujita's very nice DMA-mapping
consolidation code.
Ingo
On Fri, 9 Jan 2009 17:19:25 +0100
Joerg Roedel <[email protected]> wrote:
> Impact: add debug callbacks for dma_[alloc|free]_coherent
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> include/linux/dma-debug.h | 16 ++++++++++++++++
> lib/dma-debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> index b2131f4..a28a701 100644
> --- a/include/linux/dma-debug.h
> +++ b/include/linux/dma-debug.h
> @@ -40,6 +40,12 @@ extern void debug_map_sg(struct device *dev, struct scatterlist *sg,
> 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 void dma_debug_init(u32 num_entries)
> @@ -68,6 +74,16 @@ static inline void debug_unmap_sg(struct device *dev,
> {
> }
>
> +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 /* __DMA_DEBUG_H */
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index e6d45f9..a4a5b0f 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -604,3 +604,48 @@ 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)
> +{
> + struct dma_debug_entry *entry;
> +
> + if (global_disable)
> + return;
> +
> + if (dma_addr == bad_dma_address)
> + return;
Only X86 has 'bad_dma_address' (IA64 also has with some configurations
though). You need to use dma_mapping_error, as I pointed out in the
previous submission.
I recommend you to try this on one non-x86 box (at least). Even if you
don't have non x86, you can use cross compiler.
On Fri, 9 Jan 2009 17:19:16 +0100
Joerg Roedel <[email protected]> wrote:
> Impact: add groundwork for DMA-API debugging
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> include/linux/dma-debug.h | 25 +++++++++++++++++++++++++
> lib/Makefile | 2 ++
> lib/dma-debug.c | 39 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 66 insertions(+), 0 deletions(-)
> create mode 100644 include/linux/dma-debug.h
> create mode 100644 lib/dma-debug.c
>
> diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> new file mode 100644
> index 0000000..ce4ace7
> --- /dev/null
> +++ b/include/linux/dma-debug.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright (C) 2008 Advanced Micro Devices, Inc.
> + *
> + * Author: Joerg Roedel <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef __DMA_DEBUG_H
> +#define __DMA_DEBUG_H
> +
> +struct device;
> +
> +#endif /* __DMA_DEBUG_H */
> diff --git a/lib/Makefile b/lib/Makefile
> index 32b0e64..50b48cf 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -84,6 +84,8 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
>
> obj-$(CONFIG_DYNAMIC_PRINTK_DEBUG) += dynamic_printk.o
>
> +obj-$(CONFIG_DMA_API_DEBUG) += dma-debug.o
> +
> hostprogs-y := gen_crc32table
> clean-files := crc32table.h
>
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> new file mode 100644
> index 0000000..d04f8b6
> --- /dev/null
> +++ b/lib/dma-debug.c
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2008 Advanced Micro Devices, Inc.
> + *
> + * Author: Joerg Roedel <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <linux/dma-debug.h>
> +#include <linux/types.h>
> +#include <linux/list.h>
> +
> +enum {
> + dma_debug_single,
> + dma_debug_sg,
> + dma_debug_coherent,
> +};
> +
> +struct dma_debug_entry {
> + struct list_head list;
> + struct device *dev;
> + int type;
> + void *cpu_addr;
I don't think that cpu addresses are appropriate here.
On Fri, 9 Jan 2009 17:19:29 +0100
Joerg Roedel <[email protected]> wrote:
> Impact: make use of DMA-API debugging code in x86
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> arch/x86/Kconfig | 1 +
> arch/x86/include/asm/dma-mapping.h | 30 ++++++++++++++++++++++++++----
> arch/x86/kernel/pci-dma.c | 5 +++++
> 3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 862adb9..68a806c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -39,6 +39,7 @@ config X86
> select HAVE_GENERIC_DMA_COHERENT if X86_32
> select HAVE_EFFICIENT_UNALIGNED_ACCESS
> select USER_STACKTRACE_SUPPORT
> + select HAVE_DMA_API_DEBUG
>
> config ARCH_DEFCONFIG
> string
> diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
> index 4035357..939d5b3 100644
> --- a/arch/x86/include/asm/dma-mapping.h
> +++ b/arch/x86/include/asm/dma-mapping.h
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/scatterlist.h>
> +#include <linux/dma-debug.h>
> #include <asm/io.h>
> #include <asm/swiotlb.h>
> #include <asm-generic/dma-coherent.h>
> @@ -93,9 +94,12 @@ dma_map_single(struct device *hwdev, void *ptr, size_t size,
> int direction)
> {
> struct dma_mapping_ops *ops = get_dma_ops(hwdev);
> + dma_addr_t addr;
>
> BUG_ON(!valid_dma_direction(direction));
> - return ops->map_single(hwdev, virt_to_phys(ptr), size, direction);
> + addr = ops->map_single(hwdev, virt_to_phys(ptr), size, direction);
> + debug_map_single(hwdev, ptr, size, direction, addr);
> + return addr;
What happens if ops->map_single fails?
Seems that debug_map_single doesn't check the dma mapping fails. So it
allocates a new entries and nobody frees it?
Another problem is that what happens if ops->map_single succeeds but
debug_map_single fails? Seems that it gives a false warning.
On Fri, 9 Jan 2009 23:37:47 +0100
Joerg Roedel <[email protected]> wrote:
> On Fri, Jan 09, 2009 at 11:33:27PM +0100, Joerg Roedel wrote:
> > > This was triggered during pci_unmap_page() -> dma_unmap_single() where
> > > check_unmap() did not find the entry.
> > >
> > > The original mapping was done in bnx2 using pci_map_page(). I did not
> > > see how the debug entry was added to the hash during the call to
> > > pci_map_page() -> dma_map_page(). Did I miss something?
> >
> > dma_map_page() results in dma_map_single() -> debug_map_single() call on
> > x86. This way the entry would be added. Maybe the error from a double
> > free?
>
> Ah, it only calls ops->map_single. Thanks for pointing that out. I will
> add a call to debug_map_single to dma_map_page too. So the error above
> may be a false positive.
Then you will find that the interface of debug_map_single is wrong.
You can't use cpu address here. I explained this to you before:
http://marc.info/?t=123116736500005&r=1&w=2
On Sun, 11 Jan 2009 15:25:42 +0900
FUJITA Tomonori <[email protected]> wrote:
> On Fri, 9 Jan 2009 17:19:25 +0100
> Joerg Roedel <[email protected]> wrote:
>
> > Impact: add debug callbacks for dma_[alloc|free]_coherent
> >
> > Signed-off-by: Joerg Roedel <[email protected]>
> > ---
> > include/linux/dma-debug.h | 16 ++++++++++++++++
> > lib/dma-debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 61 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > index b2131f4..a28a701 100644
> > --- a/include/linux/dma-debug.h
> > +++ b/include/linux/dma-debug.h
> > @@ -40,6 +40,12 @@ extern void debug_map_sg(struct device *dev, struct scatterlist *sg,
> > 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 void dma_debug_init(u32 num_entries)
> > @@ -68,6 +74,16 @@ static inline void debug_unmap_sg(struct device *dev,
> > {
> > }
> >
> > +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 /* __DMA_DEBUG_H */
> > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > index e6d45f9..a4a5b0f 100644
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -604,3 +604,48 @@ 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)
> > +{
> > + struct dma_debug_entry *entry;
> > +
> > + if (global_disable)
> > + return;
> > +
> > + if (dma_addr == bad_dma_address)
> > + return;
>
> Only X86 has 'bad_dma_address' (IA64 also has with some configurations
> though). You need to use dma_mapping_error, as I pointed out in the
> previous submission.
Oops, this is for dma_alloc_coherent so you need to check 'virt'
instead of dma_mapping_error().
On Sun, Jan 11, 2009 at 12:08:26AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > +/* Global error count */
> > +static u32 error_count;
> > +
> > +/* Global error show enable*/
> > +static u32 show_all_errors __read_mostly;
> > +/* Number of errors to show */
> > +static u32 show_num_errors = 1;
> > +
> > static u32 num_free_entries;
> > static u32 min_free_entries;
>
> Small detail: please use native C types for non-hardware variables -
> 'unsigned int', 'unsigned long', etc.
>
> u32/u64 is typically used for variables where there is a real significance
> to the precise width of the variable: there's either a hardware or a
> user-space ABI involved.
Hmm, these variables are exported to debugfs. Thats the reason I made
them u32.
Joerg
On Sun, Jan 11, 2009 at 12:12:22AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > + /*
> > + * * This may be no bug in reality - but most implementations of the
> > + * * DMA API don't handle this properly, so check for it here
> > + * */
>
> You must be using Vim and copy & paste messed up? :)
Ups, true :) Thanks.
Joerg
On Sun, Jan 11, 2009 at 12:11:27AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > +#define err_printk(dev, format, arg...) do { \
> > + error_count += 1; \
> > + if (show_all_errors || show_num_errors > 0) { \
> > + WARN(1, "%s %s: " format, \
> > + dev_driver_string(dev), \
> > + dev_name(dev) , ## arg); \
> > + } \
> > + if (!show_all_errors && show_num_errors > 0) \
> > + show_num_errors -= 1; \
>
> Note that the arithmetics here is SMP-unsafe: we only hold the hash bucket
> so if two errors hit at once on two CPUs then the error tracking variables
> can be accessed at once.
>
> I'd suggest a simple global lock for this error case (taken inside the
> hash bucket lock), to be on the safe side.
>
> Also, please dont use a macro for this - printk details can be passed in
> to helper inlines/functions too.
Yeah, this is not SMP-safe, I know. But debugfs does not support
atomic_t so I made the variables u32. But at least a race condition has
not a too bad impact. What may habben is that error_count misses a error
or the show_num_errors become negative.
But if we really want to avoid this I think its better to add atomic_t
support to debugfs. What do you think?
Joerg
On Sun, Jan 11, 2009 at 03:30:14PM +0900, FUJITA Tomonori wrote:
> On Sun, 11 Jan 2009 15:25:42 +0900
> FUJITA Tomonori <[email protected]> wrote:
>
> > On Fri, 9 Jan 2009 17:19:25 +0100
> > Joerg Roedel <[email protected]> wrote:
> >
> > > Impact: add debug callbacks for dma_[alloc|free]_coherent
> > >
> > > Signed-off-by: Joerg Roedel <[email protected]>
> > > ---
> > > include/linux/dma-debug.h | 16 ++++++++++++++++
> > > lib/dma-debug.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 61 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > > index b2131f4..a28a701 100644
> > > --- a/include/linux/dma-debug.h
> > > +++ b/include/linux/dma-debug.h
> > > @@ -40,6 +40,12 @@ extern void debug_map_sg(struct device *dev, struct scatterlist *sg,
> > > 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 void dma_debug_init(u32 num_entries)
> > > @@ -68,6 +74,16 @@ static inline void debug_unmap_sg(struct device *dev,
> > > {
> > > }
> > >
> > > +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 /* __DMA_DEBUG_H */
> > > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > > index e6d45f9..a4a5b0f 100644
> > > --- a/lib/dma-debug.c
> > > +++ b/lib/dma-debug.c
> > > @@ -604,3 +604,48 @@ 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)
> > > +{
> > > + struct dma_debug_entry *entry;
> > > +
> > > + if (global_disable)
> > > + return;
> > > +
> > > + if (dma_addr == bad_dma_address)
> > > + return;
> >
> > Only X86 has 'bad_dma_address' (IA64 also has with some configurations
> > though). You need to use dma_mapping_error, as I pointed out in the
> > previous submission.
>
> Oops, this is for dma_alloc_coherent so you need to check 'virt'
> instead of dma_mapping_error().
Ah, true. Thanks. I will fix that.
Joerg
On Sun, Jan 11, 2009 at 12:46:41AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > +void debug_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
> > + int nelems, int direction)
> > +{
> > + struct scatterlist *s;
> > + int i;
> > +
> > + if (global_disable)
> > + return;
>
> "if (unlikely(global_disable))" i suspect?
True. I will add unlikely() to all global_disable checks.
Joerg
On Sun, Jan 11, 2009 at 03:25:47PM +0900, FUJITA Tomonori wrote:
> On Fri, 9 Jan 2009 17:19:29 +0100
> Joerg Roedel <[email protected]> wrote:
>
> > Impact: make use of DMA-API debugging code in x86
> >
> > Signed-off-by: Joerg Roedel <[email protected]>
> > ---
> > arch/x86/Kconfig | 1 +
> > arch/x86/include/asm/dma-mapping.h | 30 ++++++++++++++++++++++++++----
> > arch/x86/kernel/pci-dma.c | 5 +++++
> > 3 files changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 862adb9..68a806c 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -39,6 +39,7 @@ config X86
> > select HAVE_GENERIC_DMA_COHERENT if X86_32
> > select HAVE_EFFICIENT_UNALIGNED_ACCESS
> > select USER_STACKTRACE_SUPPORT
> > + select HAVE_DMA_API_DEBUG
> >
> > config ARCH_DEFCONFIG
> > string
> > diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
> > index 4035357..939d5b3 100644
> > --- a/arch/x86/include/asm/dma-mapping.h
> > +++ b/arch/x86/include/asm/dma-mapping.h
> > @@ -7,6 +7,7 @@
> > */
> >
> > #include <linux/scatterlist.h>
> > +#include <linux/dma-debug.h>
> > #include <asm/io.h>
> > #include <asm/swiotlb.h>
> > #include <asm-generic/dma-coherent.h>
> > @@ -93,9 +94,12 @@ dma_map_single(struct device *hwdev, void *ptr, size_t size,
> > int direction)
> > {
> > struct dma_mapping_ops *ops = get_dma_ops(hwdev);
> > + dma_addr_t addr;
> >
> > BUG_ON(!valid_dma_direction(direction));
> > - return ops->map_single(hwdev, virt_to_phys(ptr), size, direction);
> > + addr = ops->map_single(hwdev, virt_to_phys(ptr), size, direction);
> > + debug_map_single(hwdev, ptr, size, direction, addr);
> > + return addr;
>
> What happens if ops->map_single fails?
>
> Seems that debug_map_single doesn't check the dma mapping fails. So it
> allocates a new entries and nobody frees it?
Ah true. The debug_map_single function has to check for
dma_mapping_error. Same is true for debug_map_sg.
> Another problem is that what happens if ops->map_single succeeds but
> debug_map_single fails? Seems that it gives a false warning.
No, this can not happen. If the code fails it disables itself so no more
warnings will be printed.
Joerg
On Sun, Jan 11, 2009 at 12:54:35AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > Hi,
> >
> > this is version 2 of the patchset which introduces code to debug drivers
> > usage of the DMA-API. Many thanks to all the reviewers and the useful
> > comments on the fist version of this patchset. Tests with hardware
> > IOMMUs have shown several bugs in drivers regarding the usage of that
> > API. Problems were found especially in network card drivers.
> >
> > These bugs often don't show up or have any negative impact if there is
> > no hardware IOMMU in use in the system. But with an hardware IOMMU these
> > bugs turn the hardware unusable or, in the worst case, cause data
> > corruption on devices which are managed by other (good) drivers.
> >
> > With the code these patches introduce driver developers can find several
> > bugs of misusing the DMA-API in their drivers. But be aware, it can not
> > find all possible bugs. If it finds a problem it prints out messages
> > like
> >
> > ------------[ cut here ]------------
> > WARNING: at /data2/repos/linux.trees.git/lib/dma-debug.c:231 check_unmap+0xab/0x3d9()
> > Hardware name: Toonie
> > bnx2 0000:01:00.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x00000000011]
> > Modules linked in:
> > Pid: 0, comm: swapper Not tainted 2.6.28 #174
> > Call Trace:
> > <IRQ> [<ffffffff8105af3a>] warn_slowpath+0xd3/0xf2
> > [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> > [<ffffffff8107c36f>] ? find_usage_backwards+0xe2/0x116
> > [<ffffffff812efd16>] ? usb_hcd_link_urb_to_ep+0x94/0xa0
> > [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> > [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> > [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> > [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> > [<ffffffff814b25a5>] ? _spin_lock_irqsave+0x69/0x75
> > [<ffffffff811e2b4b>] ? get_hash_bucket+0x28/0x33
> > [<ffffffff811e2ff2>] check_unmap+0xab/0x3d9
> > [<ffffffff8107c9ed>] ? trace_hardirqs_on_caller+0x108/0x14a
> > [<ffffffff8107ca3c>] ? trace_hardirqs_on+0xd/0xf
> > [<ffffffff811e3433>] debug_unmap_single+0x3e/0x40
> > [<ffffffff8128d2d8>] dma_unmap_single+0x3d/0x60
> > [<ffffffff8128d335>] pci_unmap_page+0x1c/0x1e
> > [<ffffffff81290759>] bnx2_poll_work+0x626/0x8cb
> > [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> > [<ffffffff81070100>] ? run_posix_cpu_timers+0x49c/0x603
> > [<ffffffff81070000>] ? run_posix_cpu_timers+0x39c/0x603
> > [<ffffffff8107c52b>] ? mark_lock+0x1c/0x364
> > [<ffffffff8107d8f7>] ? __lock_acquire+0xaec/0xb55
> > [<ffffffff81292804>] bnx2_poll_msix+0x33/0x81
> > [<ffffffff813b6478>] net_rx_action+0x8a/0x139
> > [<ffffffff8105ff39>] __do_softirq+0x8b/0x147
> > [<ffffffff8102933c>] call_softirq+0x1c/0x34
> > [<ffffffff8102a611>] do_softirq+0x39/0x90
> > [<ffffffff8105fde8>] irq_exit+0x4e/0x98
> > [<ffffffff8102a5c2>] do_IRQ+0x11f/0x135
> > [<ffffffff81028b93>] ret_from_intr+0x0/0xf
> > <EOI> <4>---[ end trace 4339d58302097423 ]---
> >
> > This way driver developers get an idea where the problem is in their
> > code.
> >
> > I hope I addressed most of the review comments and objections from the
> > first version. Please give this version also a good review and send me
> > your comments.
>
> Looks pretty good in general - modulo the few observations i just made in
> reply to the patches. (Note, the comments apply to all the patches -
> there's similar small issues in other places as well.)
>
> Did you have a chance to look at debugobjects and see whether it could be
> reused as the dma-debug-entry object management code?
Yes, I looked at it. But the code looks like it is used to debug object
state and not to provide objects for debugging code.
> One request: could you please git-base it on the changes in tip/core/iommu
> (i only have looked at the patches in email - i presume the current ones
> are based on -git)?
Yes, I will rebase it for the next post.
Joerg
On Sun, Jan 11, 2009 at 08:57:52AM +0100, Joerg Roedel wrote:
> On Sun, Jan 11, 2009 at 12:11:27AM +0100, Ingo Molnar wrote:
> >
> > * Joerg Roedel <[email protected]> wrote:
> >
> > > +#define err_printk(dev, format, arg...) do { \
> > > + error_count += 1; \
> > > + if (show_all_errors || show_num_errors > 0) { \
> > > + WARN(1, "%s %s: " format, \
> > > + dev_driver_string(dev), \
> > > + dev_name(dev) , ## arg); \
> > > + } \
> > > + if (!show_all_errors && show_num_errors > 0) \
> > > + show_num_errors -= 1; \
> >
> > Note that the arithmetics here is SMP-unsafe: we only hold the hash bucket
> > so if two errors hit at once on two CPUs then the error tracking variables
> > can be accessed at once.
> >
> > I'd suggest a simple global lock for this error case (taken inside the
> > hash bucket lock), to be on the safe side.
> >
> > Also, please dont use a macro for this - printk details can be passed in
> > to helper inlines/functions too.
>
> Yeah, this is not SMP-safe, I know. But debugfs does not support
> atomic_t so I made the variables u32. But at least a race condition has
> not a too bad impact. What may habben is that error_count misses a error
> or the show_num_errors become negative.
> But if we really want to avoid this I think its better to add atomic_t
> support to debugfs. What do you think?
Even a global lock will not really help here because show_num_errors and
show_all_errors can be set using debugfs. Either we live with the small
race (with limited impact) or I add atomic_t support to debugfs.
Joerg
On Fri, 9 Jan 2009 17:19:17 +0100 Joerg Roedel <[email protected]> wrote:
> +struct hash_bucket {
> + struct list_head list;
> + spinlock_t lock;
> +} ____cacheline_aligned;
__cacheline_aligned_in_smp.
This all looks like an exotically large amount of code for a debug thingy?
On Tue, 2009-01-13 at 00:51 -0800, Andrew Morton wrote:
> This all looks like an exotically large amount of code for a debug
> thingy?
Perhaps so, but it's a useful debug thingy which lets us debug _very_
common problems in drivers, which many people are unlikely to notice
without this 'assistance'.
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
On Sun, Jan 11, 2009 at 12:11:27AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > +#define err_printk(dev, format, arg...) do { \
> > + error_count += 1; \
> > + if (show_all_errors || show_num_errors > 0) { \
> > + WARN(1, "%s %s: " format, \
> > + dev_driver_string(dev), \
> > + dev_name(dev) , ## arg); \
> > + } \
> > + if (!show_all_errors && show_num_errors > 0) \
> > + show_num_errors -= 1; \
>
> Note that the arithmetics here is SMP-unsafe: we only hold the hash bucket
> so if two errors hit at once on two CPUs then the error tracking variables
> can be accessed at once.
>
> I'd suggest a simple global lock for this error case (taken inside the
> hash bucket lock), to be on the safe side.
As I wrote in a previous email, a race here is no big deal. I add a
comment to document it. Or do we want another global lock here?
> Also, please dont use a macro for this - printk details can be passed in
> to helper inlines/functions too.
Hmm, how does this look like? There is not WARN variant which can use
va_args as a parameter. And it is important that the error message is
logged in the warning itself. So the driver developer can see it when a
user reports the warning.
Joerg
--
| Advanced Micro Devices GmbH
Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
System |
Research | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
| Registergericht München, HRB Nr. 43632
* Andrew Morton <[email protected]> wrote:
> On Fri, 9 Jan 2009 17:19:17 +0100 Joerg Roedel <[email protected]> wrote:
>
> > +struct hash_bucket {
> > + struct list_head list;
> > + spinlock_t lock;
> > +} ____cacheline_aligned;
>
> __cacheline_aligned_in_smp.
>
> This all looks like an exotically large amount of code for a debug
> thingy?
this code checks the DMA usage of ~1 million lines of kernel code - all
the DMA using drivers. I think Joerg's feature is hugely relevant as DMA
scribbles are one of the hardest to debug kernel bugs: they can end up in
permanent data corruption or other hard to find bugs. In fact i think his
patchset is rather simple and even having 10 times as much debug code
would pay for its existence in the long run.
Ingo
* Joerg Roedel <[email protected]> wrote:
> On Sun, Jan 11, 2009 at 12:11:27AM +0100, Ingo Molnar wrote:
> >
> > * Joerg Roedel <[email protected]> wrote:
> >
> > > +#define err_printk(dev, format, arg...) do { \
> > > + error_count += 1; \
> > > + if (show_all_errors || show_num_errors > 0) { \
> > > + WARN(1, "%s %s: " format, \
> > > + dev_driver_string(dev), \
> > > + dev_name(dev) , ## arg); \
> > > + } \
> > > + if (!show_all_errors && show_num_errors > 0) \
> > > + show_num_errors -= 1; \
> >
> > Note that the arithmetics here is SMP-unsafe: we only hold the hash bucket
> > so if two errors hit at once on two CPUs then the error tracking variables
> > can be accessed at once.
> >
> > I'd suggest a simple global lock for this error case (taken inside the
> > hash bucket lock), to be on the safe side.
>
> As I wrote in a previous email, a race here is no big deal. I add a
> comment to document it. Or do we want another global lock here?
we commonly use global locks in debug exception cases - to serialize
console output. But it's certainly no big deal.
Ingo
On Sun, Jan 11, 2009 at 12:08:26AM +0100, Ingo Molnar wrote:
>
> * Joerg Roedel <[email protected]> wrote:
>
> > +/* Global error count */
> > +static u32 error_count;
> > +
> > +/* Global error show enable*/
> > +static u32 show_all_errors __read_mostly;
> > +/* Number of errors to show */
> > +static u32 show_num_errors = 1;
> > +
> > static u32 num_free_entries;
> > static u32 min_free_entries;
>
> Small detail: please use native C types for non-hardware variables -
> 'unsigned int', 'unsigned long', etc.
>
> u32/u64 is typically used for variables where there is a real significance
> to the precise width of the variable: there's either a hardware or a
> user-space ABI involved.
There is some kind of userspace ABI: all these variables are exported
via debugfs. And debugfs relies on these types.
Joerg
--
| Advanced Micro Devices GmbH
Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
System |
Research | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
| Registergericht München, HRB Nr. 43632
On Wed, 14 Jan 2009 12:43:47 +0100 Ingo Molnar <[email protected]> wrote:
>
> * Andrew Morton <[email protected]> wrote:
>
> > On Fri, 9 Jan 2009 17:19:17 +0100 Joerg Roedel <[email protected]> wrote:
> >
> > > +struct hash_bucket {
> > > + struct list_head list;
> > > + spinlock_t lock;
> > > +} ____cacheline_aligned;
> >
> > __cacheline_aligned_in_smp.
> >
> > This all looks like an exotically large amount of code for a debug
> > thingy?
>
> this code checks the DMA usage of ~1 million lines of kernel code - all
> the DMA using drivers. I think Joerg's feature is hugely relevant as DMA
> scribbles are one of the hardest to debug kernel bugs: they can end up in
> permanent data corruption or other hard to find bugs. In fact i think his
> patchset is rather simple and even having 10 times as much debug code
> would pay for its existence in the long run.
>
Have we previously found bugs by other means which this facility would
have detected? I don't recall any...
* Andrew Morton <[email protected]> wrote:
> On Wed, 14 Jan 2009 12:43:47 +0100 Ingo Molnar <[email protected]> wrote:
>
> >
> > * Andrew Morton <[email protected]> wrote:
> >
> > > On Fri, 9 Jan 2009 17:19:17 +0100 Joerg Roedel <[email protected]> wrote:
> > >
> > > > +struct hash_bucket {
> > > > + struct list_head list;
> > > > + spinlock_t lock;
> > > > +} ____cacheline_aligned;
> > >
> > > __cacheline_aligned_in_smp.
> > >
> > > This all looks like an exotically large amount of code for a debug
> > > thingy?
> >
> > this code checks the DMA usage of ~1 million lines of kernel code -
> > all the DMA using drivers. I think Joerg's feature is hugely relevant
> > as DMA scribbles are one of the hardest to debug kernel bugs: they can
> > end up in permanent data corruption or other hard to find bugs. In
> > fact i think his patchset is rather simple and even having 10 times as
> > much debug code would pay for its existence in the long run.
>
> Have we previously found bugs by other means which this facility would
> have detected? I don't recall any...
this facility found a handful of real bugs already - Joerg, do you have
more specifics about that?
Also, such a facility would be useful during driver development, when such
bugs occur en masse. Faster driver development under Linux is certainly
desirable.
This facility basically adds a sandbox to all DMA ops and tracks the
lifetime and validity of DMA operations. Given how ugly DMA bugs can be in
practice and how hard to debug they are, i see this as good step forward.
Ingo
On Wed, 2009-01-14 at 09:39 -0800, Andrew Morton wrote:
> > this code checks the DMA usage of ~1 million lines of kernel code - all
> > the DMA using drivers. I think Joerg's feature is hugely relevant as DMA
> > scribbles are one of the hardest to debug kernel bugs: they can end up in
> > permanent data corruption or other hard to find bugs. In fact i think his
> > patchset is rather simple and even having 10 times as much debug code
> > would pay for its existence in the long run.
> >
>
> Have we previously found bugs by other means which this facility would
> have detected? I don't recall any...
Yes. We've found network driver bugs which only showed up when an IOMMU
has been active -- and were painful to diagnose because they involved
DMA going wrong. It happens when drivers make mistakes with the DMA
mapping APIs.
With this debug facility, we can find such problems on _any_ hardware,
and get a sane report about what's wrong.
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
* Andrew Morton <[email protected]> wrote:
> On Wed, 14 Jan 2009 12:43:47 +0100 Ingo Molnar <[email protected]> wrote:
>
> >
> > * Andrew Morton <[email protected]> wrote:
> >
> > > On Fri, 9 Jan 2009 17:19:17 +0100 Joerg Roedel <[email protected]> wrote:
> > >
> > > > +struct hash_bucket {
> > > > + struct list_head list;
> > > > + spinlock_t lock;
> > > > +} ____cacheline_aligned;
> > >
> > > __cacheline_aligned_in_smp.
> > >
> > > This all looks like an exotically large amount of code for a debug
> > > thingy?
> >
> > this code checks the DMA usage of ~1 million lines of kernel code - all
> > the DMA using drivers. I think Joerg's feature is hugely relevant as DMA
> > scribbles are one of the hardest to debug kernel bugs: they can end up in
> > permanent data corruption or other hard to find bugs. In fact i think his
> > patchset is rather simple and even having 10 times as much debug code
> > would pay for its existence in the long run.
> >
>
> Have we previously found bugs by other means which this facility would
> have detected? I don't recall any...
btw., during the past decade we have had countless very ugly driver DMA
bugs in the past that took vendors months and specialized equipment to
track down.
I cannot give you a number breakdown, only an impression: storage drivers
tended to be the hardest hit (due to the severity of the bugs and due to
their inherent complexity) - but DMA bugs in networking drivers can be
hard to track down too.
Plus there's another benefit: if a driver passes this checking and there's
still DMA related corruption observed, then the hardware / firmware
becomes a stronger suspect. This helps debugging too.
Ingo
On Wed, Jan 14, 2009 at 09:39:18AM -0800, Andrew Morton wrote:
>
> Have we previously found bugs by other means which this facility would
> have detected? I don't recall any...
>
See for example this bugfix:
http://www.spinics.net/lists/netdev/msg83208.html
The bug was found using the first version of this patchset.
Joerg
--
| Advanced Micro Devices GmbH
Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
System |
Research | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
| Registergericht München, HRB Nr. 43632
On Wed, 14 Jan 2009 18:48:04 +0100
Ingo Molnar <[email protected]> wrote:
>
> * Andrew Morton <[email protected]> wrote:
>
> > On Wed, 14 Jan 2009 12:43:47 +0100 Ingo Molnar <[email protected]> wrote:
> >
> > >
> > > * Andrew Morton <[email protected]> wrote:
> > >
> > > > On Fri, 9 Jan 2009 17:19:17 +0100 Joerg Roedel <[email protected]> wrote:
> > > >
> > > > > +struct hash_bucket {
> > > > > + struct list_head list;
> > > > > + spinlock_t lock;
> > > > > +} ____cacheline_aligned;
> > > >
> > > > __cacheline_aligned_in_smp.
> > > >
> > > > This all looks like an exotically large amount of code for a debug
> > > > thingy?
> > >
> > > this code checks the DMA usage of ~1 million lines of kernel code - all
> > > the DMA using drivers. I think Joerg's feature is hugely relevant as DMA
> > > scribbles are one of the hardest to debug kernel bugs: they can end up in
> > > permanent data corruption or other hard to find bugs. In fact i think his
> > > patchset is rather simple and even having 10 times as much debug code
> > > would pay for its existence in the long run.
> > >
> >
> > Have we previously found bugs by other means which this facility would
> > have detected? I don't recall any...
>
> btw., during the past decade we have had countless very ugly driver DMA
> bugs in the past that took vendors months and specialized equipment to
> track down.
>
> I cannot give you a number breakdown, only an impression: storage drivers
> tended to be the hardest hit (due to the severity of the bugs and due to
> their inherent complexity) - but DMA bugs in networking drivers can be
> hard to track down too.
We have had ugly DMA bugs in scsi drivers but I think that this new
feature can find very few of them. I can't think of any SCSI driver
bugs that this could find. This feature can't find any popular DMA
bugs in scsi drivers, such as messing up driver's dma descriptor from
a scatter gather list.
But this can find some kinds of DMA bugs and it's is just about 1,000
lines. I don't see any problem about merging this. 1,000 lines it too
large? Maybe cutting just 1,000 lines up into too many pieces is
deceptive.