[PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Shuah Khan]

dma-debug checks to verify if driver validated the address returned by
dma mapping routines when driver does unmap. If a driver doesn't call
unmap, failure to check mapping errors isn't detected and reported.

Enhancing existing bus notifier_call dma_debug_device_change() to check
for mapping errors at the same time it detects leaked dma buffers for
BUS_NOTIFY_UNBOUND_DRIVER event. It scans for mapping errors and if any
found, prints one warning message that includes mapping error count.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 lib/dma-debug.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index d87a17a..e34865e 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -717,6 +717,7 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
 	struct dma_debug_entry *entry;
 	unsigned long flags;
 	int count = 0, i;
+	int map_err_cnt = 0;
 
 	local_irq_save(flags);
 
@@ -724,6 +725,8 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
 		spin_lock(&dma_entry_hash[i].lock);
 		list_for_each_entry(entry, &dma_entry_hash[i].list, list) {
 			if (entry->dev == dev) {
+				if (entry->map_err_type == MAP_ERR_NOT_CHECKED)
+					map_err_cnt += 1;
 				count += 1;
 				*out_entry = entry;
 			}
@@ -733,6 +736,10 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
 
 	local_irq_restore(flags);
 
+	if (map_err_cnt)
+		dev_warn(entry->dev,
+			"DMA-API: device driver failed to check map errors: "
+			"[count] = %d\n", map_err_cnt);
 	return count;
 }
 
-- 
1.8.3.2

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Joerg Roedel]

On Wed, Nov 13, 2013 at 01:56:08PM -0700, Shuah Khan wrote:
> dma-debug checks to verify if driver validated the address returned by
> dma mapping routines when driver does unmap. If a driver doesn't call
> unmap, failure to check mapping errors isn't detected and reported.
> 
> Enhancing existing bus notifier_call dma_debug_device_change() to check
> for mapping errors at the same time it detects leaked dma buffers for
> BUS_NOTIFY_UNBOUND_DRIVER event. It scans for mapping errors and if any
> found, prints one warning message that includes mapping error count.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> ---
>  lib/dma-debug.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index d87a17a..e34865e 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -717,6 +717,7 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
>  	struct dma_debug_entry *entry;
>  	unsigned long flags;
>  	int count = 0, i;
> +	int map_err_cnt = 0;
>  
>  	local_irq_save(flags);
>  
> @@ -724,6 +725,8 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
>  		spin_lock(&dma_entry_hash[i].lock);
>  		list_for_each_entry(entry, &dma_entry_hash[i].list, list) {
>  			if (entry->dev == dev) {
> +				if (entry->map_err_type == MAP_ERR_NOT_CHECKED)
> +					map_err_cnt += 1;
>  				count += 1;
>  				*out_entry = entry;

I think it is better to check for this in a seperate function and use
err_printk instead of dev_warn in the end to print the errors.
The new function can then be called in the dma_debug_device_change
callback like device_dma_allocations is.


	Joerg

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Shuah Khan]

On 12/30/2013 07:15 AM, Joerg Roedel wrote:
> On Wed, Nov 13, 2013 at 01:56:08PM -0700, Shuah Khan wrote:
>> dma-debug checks to verify if driver validated the address returned by
>> dma mapping routines when driver does unmap. If a driver doesn't call
>> unmap, failure to check mapping errors isn't detected and reported.
>>
>> Enhancing existing bus notifier_call dma_debug_device_change() to check
>> for mapping errors at the same time it detects leaked dma buffers for
>> BUS_NOTIFY_UNBOUND_DRIVER event. It scans for mapping errors and if any
>> found, prints one warning message that includes mapping error count.
>>
>> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
>> ---
>>   lib/dma-debug.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
>> index d87a17a..e34865e 100644
>> --- a/lib/dma-debug.c
>> +++ b/lib/dma-debug.c
>> @@ -717,6 +717,7 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
>>   	struct dma_debug_entry *entry;
>>   	unsigned long flags;
>>   	int count = 0, i;
>> +	int map_err_cnt = 0;
>>
>>   	local_irq_save(flags);
>>
>> @@ -724,6 +725,8 @@ static int device_dma_allocations(struct device *dev, struct dma_debug_entry **o
>>   		spin_lock(&dma_entry_hash[i].lock);
>>   		list_for_each_entry(entry, &dma_entry_hash[i].list, list) {
>>   			if (entry->dev == dev) {
>> +				if (entry->map_err_type == MAP_ERR_NOT_CHECKED)
>> +					map_err_cnt += 1;
>>   				count += 1;
>>   				*out_entry = entry;
>
> I think it is better to check for this in a seperate function and use
> err_printk instead of dev_warn in the end to print the errors.
> The new function can then be called in the dma_debug_device_change
> callback like device_dma_allocations is.
>

I did explore separate function option and backed off from it since the 
new routine will have to duplicate what device_dma_allocations() does 
except that it checks for entry->map_err_type.

I still have the patch that does that saved away. If you still prefer 
that approach, I can rework the patch and send it.

-- Shuah


-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Joerg Roedel]

On Fri, Jan 03, 2014 at 11:26:04AM -0700, Shuah Khan wrote:
> On 12/30/2013 07:15 AM, Joerg Roedel wrote:
> >I think it is better to check for this in a seperate function and use
> >err_printk instead of dev_warn in the end to print the errors.
> >The new function can then be called in the dma_debug_device_change
> >callback like device_dma_allocations is.
> >
> 
> I did explore separate function option and backed off from it since
> the new routine will have to duplicate what device_dma_allocations()
> does except that it checks for entry->map_err_type.
> 
> I still have the patch that does that saved away. If you still
> prefer that approach, I can rework the patch and send it.

You can get rid of the code duplication by defining for_each macros for
the list traversals. So I would still prefer to have this check in a
seperate function.


	Joerg

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Shuah Khan]

On 01/07/2014 07:26 AM, Joerg Roedel wrote:
> On Fri, Jan 03, 2014 at 11:26:04AM -0700, Shuah Khan wrote:
>> On 12/30/2013 07:15 AM, Joerg Roedel wrote:
>>> I think it is better to check for this in a seperate function and use
>>> err_printk instead of dev_warn in the end to print the errors.
>>> The new function can then be called in the dma_debug_device_change
>>> callback like device_dma_allocations is.
>>>
>>
>> I did explore separate function option and backed off from it since
>> the new routine will have to duplicate what device_dma_allocations()
>> does except that it checks for entry->map_err_type.
>>
>> I still have the patch that does that saved away. If you still
>> prefer that approach, I can rework the patch and send it.
>
> You can get rid of the code duplication by defining for_each macros for
> the list traversals. So I would still prefer to have this check in a
> seperate function.
>
>
> 	Joerg
>
>

Joerg,

This patch and a follow-on cocinelli warning fix patch are in 
linux-next. Would you like me to send a patch relative to the change in 
linux-next or cut a new patch against the latest Linus's git. I can go 
either way. We just have to remember to drop those two patches from 
linux-next.

-- Shuah

-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Joerg Roedel]

On Tue, Jan 07, 2014 at 08:00:33AM -0700, Shuah Khan wrote:
> This patch and a follow-on cocinelli warning fix patch are in
> linux-next. Would you like me to send a patch relative to the change
> in linux-next or cut a new patch against the latest Linus's git. I
> can go either way. We just have to remember to drop those two
> patches from linux-next.

Please do a patch against linus.git and drop the two patches from
linux-next. Over which tree did they go into linux-next anyway?


	Joerg

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Shuah Khan]

On 01/07/2014 08:12 AM, Joerg Roedel wrote:
> On Tue, Jan 07, 2014 at 08:00:33AM -0700, Shuah Khan wrote:
>> This patch and a follow-on cocinelli warning fix patch are in
>> linux-next. Would you like me to send a patch relative to the change
>> in linux-next or cut a new patch against the latest Linus's git. I
>> can go either way. We just have to remember to drop those two
>> patches from linux-next.
>
> Please do a patch against linus.git and drop the two patches from
> linux-next. Over which tree did they go into linux-next anyway?
>
>

ok. It went through mm tree.

-- Shuah


-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH v2] dma-debug: enhance dma_debug_device_change() to check for mapping errors

[Shuah Khan]

On 01/07/2014 10:22 AM, Shuah Khan wrote:
> On 01/07/2014 08:12 AM, Joerg Roedel wrote:
>> On Tue, Jan 07, 2014 at 08:00:33AM -0700, Shuah Khan wrote:
>>> This patch and a follow-on cocinelli warning fix patch are in
>>> linux-next. Would you like me to send a patch relative to the change
>>> in linux-next or cut a new patch against the latest Linus's git. I
>>> can go either way. We just have to remember to drop those two
>>> patches from linux-next.
>>
>> Please do a patch against linus.git and drop the two patches from
>> linux-next. Over which tree did they go into linux-next anyway?
>>
>>
>
> ok. It went through mm tree.
>

I have a separate routine now for scanning for entries with map error 
flag set and I also have a new err_printk() call to print the entry from 
dma_debug_device_change() right after device_dma_allocations() check and 
corresponding err_printk(). Snippet of the diff below. Only one warning 
gets printed. Looking at err_printk(): if (!show_all_errors && 
show_num_errors > 0) conditional, it appears that is the way it is 
supposed to work, unless I am missing something

One way to not miss the second warning is to just generate just a 
pr_err() for the mapping error instead of err_printk(). Leaked entries 
is a bigger problem that should really be shown and the same entry might 
not have the map error flag set. Thoughts?

  static int dma_debug_device_change(struct notifier_block *nb, unsigned 
long action, void *data)
  {
         struct device *dev = data;
@@ -758,6 +784,18 @@ static int dma_debug_device_change(struct 
notifier_block *nb, unsigned long acti
                                 "[mapped with %s] [mapped as %s]\n",
                         count, entry->dev_addr, entry->size,
                         dir2name[entry->direction], 
type2name[entry->type]);
+
+               count = device_dma_map_errors(dev, &entry);
+               if (count == 0)
+                       break;
+               err_printk(dev, entry,
+                               "DMA-API: device driver failed to check 
map err"
+                               ": [count=%d]\n"
+                               "Details of an entry with map error flag 
set: "
+                               "[device address=0x%016llx] [size=%llu 
bytes] "
+                               "[mapped with %s] [mapped as %s]\n",
+                       count, entry->dev_addr, entry->size,
+                       dir2name[entry->direction], type2name[entry->type]);
                 break;


-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658