dma-debug: New interfaces to debug dma mapping errors

lib/dma-debug.c +29 -0

+void debug_dma_dump_map_errors(struct device *dev, int all)
+{
+	if (all) {
+		dev_info(dev,
+			 "DMA-API: DMA map error summary:\n"
+			 "DMA map errors returned = %d\n"
+			 "DMA map errors not checked = %d\n"
+			 "DMA unmap errors = %d\n"
+			 "SWIOTLB overflow triggers = %d\n",
+			 dev->dma_map_errors,
+			 dev->dma_map_errors_not_checked,
+			 dev->dma_unmap_errors,
+			 dev->iotlb_overflow_cnt);
+		return;
+	}
+
+	if (dev->dma_map_errors_not_checked) {
+		dev_err(dev,
+			"DMA-API: Driver failed to check\n"
+			"%d out of a total of %d DMA map errors returned\n"
+			"by DMA mapping interfaces\n",
+			dev->dma_map_errors_not_checked,
+			dev->dma_map_errors);
+	}
+
+	if (dev->dma_unmap_errors) {
+		dev_err(dev,
+			"DMA-API: Driver tried to free invalid\n"
+			"DMA addresses %d times\n",

Quoted 34 lines · Shuah Khan

lib/dma-debug.c +34 -0

+void debug_dma_dump_map_errors(struct device *dev, int all)
+{
+	if (all) {
+		dev_info(dev,
+			 "DMA-API: DMA map error summary:\n"
+			 "DMA map errors returned = %d\n"
+			 "DMA map errors not checked = %d\n"
+			 "DMA unmap errors = %d\n"
+			 "SWIOTLB overflow triggers = %d\n",
+			 dev->dma_map_errors,
+			 dev->dma_map_errors_not_checked,
+			 dev->dma_unmap_errors,
+			 dev->iotlb_overflow_cnt);
+		return;
+	}
+
+	if (dev->dma_map_errors_not_checked) {
+		dev_err(dev,
+			"DMA-API: Driver failed to check\n"
+			"%d out of a total of %d DMA map errors returned\n"
+			"by DMA mapping interfaces\n",
+			dev->dma_map_errors_not_checked,
+			dev->dma_map_errors);
+	}
+
+	if (dev->dma_unmap_errors) {
+		dev_err(dev,
+			"DMA-API: Driver tried to free invalid\n"
+			"DMA addresses %d times\n",
+			dev->dma_unmap_errors);
+	}
+
+	if (dev->iotlb_overflow_cnt) {
+		dev_err(dev,

lib/dma-debug.c static void hash_bucket_del(struct dma_debug_entry *entry) · line 416

Quoted 5 lines

lib/dma-debug.c +5 -0

+			"DMA-API: SWIOTLB overflow buffer triggered %d times\n",
+			dev->iotlb_overflow_cnt);
+	}
+}
+EXPORT_SYMBOL(debug_dma_dump_map_errors);

Don't use syslog for stuff like this, that's not good. Why not use debugfs?

Quoted 35 lines · Greg KH

Quoted 34 lines · Shuah Khan

lib/dma-debug.c +34 -0

+void debug_dma_dump_map_errors(struct device *dev, int all)
+{
+	if (all) {
+		dev_info(dev,
+			 "DMA-API: DMA map error summary:\n"
+			 "DMA map errors returned = %d\n"
+			 "DMA map errors not checked = %d\n"
+			 "DMA unmap errors = %d\n"
+			 "SWIOTLB overflow triggers = %d\n",
+			 dev->dma_map_errors,
+			 dev->dma_map_errors_not_checked,
+			 dev->dma_unmap_errors,
+			 dev->iotlb_overflow_cnt);
+		return;
+	}
+
+	if (dev->dma_map_errors_not_checked) {
+		dev_err(dev,
+			"DMA-API: Driver failed to check\n"
+			"%d out of a total of %d DMA map errors returned\n"
+			"by DMA mapping interfaces\n",
+			dev->dma_map_errors_not_checked,
+			dev->dma_map_errors);
+	}
+
+	if (dev->dma_unmap_errors) {
+		dev_err(dev,
+			"DMA-API: Driver tried to free invalid\n"
+			"DMA addresses %d times\n",
+			dev->dma_unmap_errors);
+	}
+
+	if (dev->iotlb_overflow_cnt) {
+		dev_err(dev,

Quoted 8 lines

lib/dma-debug.c static void hash_bucket_del(struct dma_debug_entry *entry) · line 416

Quoted 5 lines

lib/dma-debug.c +5 -0

+			"DMA-API: SWIOTLB overflow buffer triggered %d times\n",
+			dev->iotlb_overflow_cnt);
+	}
+}
+EXPORT_SYMBOL(debug_dma_dump_map_errors);

Don't use syslog for stuff like this, that's not good. Why not use debugfs?

This debug interface debug_dma_dump_map_errors() is enabled only when CONFIG_DMA_API_DEBUG is enabled, however I agree that debugfs is a better choice than logging to syslog. I created debugfs interfaces for these error counts at system level. Are recommending the same approach for per device counts as well?

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

Four new fields are added to struct device when CONFIG_DMA_API_DEBUG is enabled, to track the following:

dma_map_errors:
Total number of dma mapping errors returned by the dma mapping interfaces, in response to mapping requests from this device.
dma_map_errors_not_checked:
Total number of dma mapping errors the device failed to check before using the returned address.
dma_unmap_errors:
Total number of times the device tried to unmap or free an invalid dma address.
iotlb_overflow_cnt:
Tracks how many times a swiotlb overflow buffer is returned to this device when regular iotlb is full.

Enhancements to dma-debug api are made to add new debugfs interfaces to report total dma errors, dma errors that are not checked, and unmap errors for the entire system. Please note that these are counts for all devices in the system.

The following new dma-debug interfaces are added:

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr):
Tracks dma mapping errors checked by the device. It decrements the dma_map_errors_not_checked counter that was incremented by debug_dma_map_page() when it checked for errors.
debug_dma_dump_map_errors(struct device *dev, int all):
Allows dump of dma mapping error summary or just the errors if any.

The following existing dma-debug api are changed to support this feature:
debug_dma_map_page():
Increments dma_map_errors and dma_map_errors_not_checked errors for the current device as well as totals for the system, dma-debug api keeps track of, when dma_addr is invalid. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling this routine at the time dma error unchecked state is registered, will not help if state gets changed right away.
check_unmap():

This is an existing internal routines that checks for unmap errors, changed to increment dma_unmap_errors for the current device, as well as the dma_unmap_errors counter for the system, dma-debug api keeps track of, when a device requests an invalid address to be unmapped. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling dma_mapping_error() from this routine will decrement dma_map_errors_not_checked counter incorrectly.

The following new swiotlb interface is changed:
swiotlb_map_page():
Increments iotlb_overflow_cnt for the device when iotlb overflow buffer is returned when swiotlb is full.

Changed arch/x86/include/asm/dma-mapping.h to call debug_dma_mapping_error() to validate these new interfaces on x86_64. Other architectures will be changed in a subsequent patch.

The current dma-debug infrastructure is designed to track dma mappings, and debug entries are added only for correctly mapped addresses and not when mapping fails. Enhancing the current infrastructure to track failed mappings will result in unnecessary complexity. The approach to add counters to

struct device eliminates the need for maintaining failed mappings in dma-debug infrastructure and is cleaner and simpler without impacting the existing dma-debug infrastructure.

Quoted 9 lines

check_unmap():
This is an existing internal routines that checks for unmap errors, changed to increment dma_unmap_errors for the current device, as well as the dma_unmap_errors counter for the system, dma-debug api keeps track of, when a device requests an invalid address to be unmapped. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling dma_mapping_error() from this routine will decrement dma_map_errors_not_checked counter incorrectly.

I like the direction of this patch. That said I am wondering why you choose to do it this way? Was there no way to have all of the logic within debug dma file, and within check_unmap?

What is the extra complexity? Can you explain as if I was a newbie to debug DMA API - perhaps there is still some hope in doing it there?

Quoted 3 lines

struct device eliminates the need for maintaining failed mappings in dma-debug infrastructure and is cleaner and simpler without impacting the existing dma-debug infrastructure.

Could you explain please why it would be more difficult to do it in the existing dma-debug infrastructure?

Quoted 18 lines · Konrad Rzeszutek Wilk

Quoted 9 lines

check_unmap():
This is an existing internal routines that checks for unmap errors, changed to increment dma_unmap_errors for the current device, as well as the dma_unmap_errors counter for the system, dma-debug api keeps track of, when a device requests an invalid address to be unmapped. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling dma_mapping_error() from this routine will decrement dma_map_errors_not_checked counter incorrectly.

I like the direction of this patch. That said I am wondering why you choose to do it this way? Was there no way to have all of the logic within debug dma file, and within check_unmap?

What is the extra complexity? Can you explain as if I was a newbie to debug DMA API - perhaps there is still some hope in doing it there?

Quoted 6 lines

Quoted 3 lines

struct device eliminates the need for maintaining failed mappings in dma-debug infrastructure and is cleaner and simpler without impacting the existing dma-debug infrastructure.

Could you explain please why it would be more difficult to do it in the existing dma-debug infrastructure?

I started out with a goal to provide a debug infrastructure to track all the cases where dma mapping errors go unchecked.

I could have gone the route to track system wide counts and not be concerned about per device counts. In which case, it would be a sub-set of the functionality in this pacth. i.e debug_dma_map_page() increments dma_map_errors and dma_map_errors_not_checked. The new interface debug_dma_mapping_error() simply decrements dma_map_errors_not_checked.

check_unmap() can increment the third system wide counter, dma_unmap_errors.

However, system wide counters are of limited use, per device counters wil gine us the ability to identify the drivers that need fixing and fix them and have a way to regression test old drivers and sanity check the new in the future.

Having decided on per device counters, my first approach was looking into enhancing dma-debug infrastructure and contain this logic within that module by enhancing the existing dma_debug_entry to track these errors.

One issue with this approach is that the current dma-debug infrastructure tracks only the successful mappings. Entries are added to the dma_debug_entry able from mapping interfaces for good maps. This table is hashed using the mapped address (dma_addr). When dma mapping error is detected by the debug interfaces debug_dma_map_page() namely, nothing gets added to the dma_debug_entry table.

Tracking failed mappings would require either changing the current table usage to include failed maps and change the hash function to use some other key instead of the mapped address. I didn't want to go that route. One option I considered was to maintain device list with dma-debug module and at that point adding fields to struct device sounded like one way to go instead of adding another set of parallel data structures to maintain the association between these counts and devices.

But from what I am hearing as feedback "changing struct device for this purpose is not a desirable." :)

I will go back and take a look at another approach to not disturb the existing dma_debug_entry usage, still provide per device counts contained within the dma-debug module. I have couple of ideas to pursue further and see if they work.

-- Shuah

Quoted 49 lines · Shuah Khan

Quoted 18 lines · Konrad Rzeszutek Wilk

Quoted 9 lines

check_unmap():
This is an existing internal routines that checks for unmap errors, changed to increment dma_unmap_errors for the current device, as well as the dma_unmap_errors counter for the system, dma-debug api keeps track of, when a device requests an invalid address to be unmapped. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling dma_mapping_error() from this routine will decrement dma_map_errors_not_checked counter incorrectly.

I like the direction of this patch. That said I am wondering why you choose to do it this way? Was there no way to have all of the logic within debug dma file, and within check_unmap?

What is the extra complexity? Can you explain as if I was a newbie to debug DMA API - perhaps there is still some hope in doing it there?

Quoted 6 lines

Quoted 3 lines

struct device eliminates the need for maintaining failed mappings in dma-debug infrastructure and is cleaner and simpler without impacting the existing dma-debug infrastructure.

Could you explain please why it would be more difficult to do it in the existing dma-debug infrastructure?

I started out with a goal to provide a debug infrastructure to track all the cases where dma mapping errors go unchecked.

I could have gone the route to track system wide counts and not be concerned about per device counts. In which case, it would be a sub-set of the functionality in this pacth. i.e debug_dma_map_page() increments dma_map_errors and dma_map_errors_not_checked. The new interface debug_dma_mapping_error() simply decrements dma_map_errors_not_checked.

check_unmap() can increment the third system wide counter, dma_unmap_errors.

However, system wide counters are of limited use, per device counters wil gine us the ability to identify the drivers that need fixing and fix them and have a way to regression test old drivers and sanity check the new in the future.

Having decided on per device counters, my first approach was looking into enhancing dma-debug infrastructure and contain this logic within that module by enhancing the existing dma_debug_entry to track these errors.

Quoted 6 lines

One issue with this approach is that the current dma-debug infrastructure tracks only the successful mappings. Entries are added to the dma_debug_entry able from mapping interfaces for good maps. This table is hashed using the mapped address (dma_addr). When dma mapping error is detected by the debug interfaces debug_dma_map_page() namely, nothing gets added to the dma_debug_entry table.

The check for the violations you are trying to find is to find that during the life-time of 'map_page' -> 'unmap_page' that 'dma_mapping_error' has been called. Presumarily part of that are good maps?

So what would it take to keep that state for that scenario? Could you use the existing system of lookup?

For the scenario where the result of 'map_page' is invalid it seems that you would need to use a completely different hash key anyway, as:

extern dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
unsigned long offset, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs);

on the input side you get 'struct device','struct page'... and that is it. The DMA API is responsible for providing you with the 'dma_addr' which is going to be zero or -1, or some valid DMA scratch address, depending on the IOMMU.

On the later invocation, so 'unmap_page', you have:

extern void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, struct dma_attrs *attrs);

you are feed the 'dev_addr' that 'map_page' came up with (-1 or 0) and the 'struct device'.

Perhaps you could use just 'struct device' and 'dma_addr' and life with the possiblity of the device doing multiple of these map_page where it gets a invalid address and does nothing about it. If it does the 'dma_mapping_error' you could deduct the count of invalid DMA address?

Quoted 11 lines

Tracking failed mappings would require either changing the current table usage to include failed maps and change the hash function to use some other key instead of the mapped address. I didn't want to go that route. One option I considered was to maintain device list with dma-debug module and at that point adding fields to struct device sounded like one way to go instead of adding another set of parallel data structures to maintain the association between these counts and devices.

Quoted 2 lines

But from what I am hearing as feedback "changing struct device for this purpose is not a desirable." :)

Yup.

Quoted 1 line

Quoted 4 lines

I will go back and take a look at another approach to not disturb the existing dma_debug_entry usage, still provide per device counts contained within the dma-debug module. I have couple of ideas to pursue further and see if they work.

OK. Would it help if we suggested some ideas or do you want to try to mull some of your ideas first?

Yeah. I will firm up my ideas a bit and summarize in a day or two. Would like to hear your ideas as well at that time, so we can pick the one that works the best.

Quoted 3 lines · Shuah Khan

Yeah. I will firm up my ideas a bit and summarize in a day or two. Would like to hear your ideas as well at that time, so we can pick the one that works the best.

I think the best approach for this functionality is to add a flag to 'struct dma_debug_entry' which tells whether the address has been checked with dma_mapping error or not. On unmap or driver unload you can then check for that flag and print a warning when an unchecked address is detected.

Quoted 10 lines · Joerg Roedel

Quoted 3 lines · Shuah Khan

Yeah. I will firm up my ideas a bit and summarize in a day or two. Would like to hear your ideas as well at that time, so we can pick the one that works the best.

I think the best approach for this functionality is to add a flag to 'struct dma_debug_entry' which tells whether the address has been checked with dma_mapping error or not. On unmap or driver unload you can then check for that flag and print a warning when an unchecked address is detected.

Was hoping to get comments from you as well. You are original author for this dam-debug module.

Are you ok with the system wide and per device error counts I added? Any comments on the overall approach?

The approach you suggested will cover the cases where drivers fail to check good map cases. We won't able to catch failed maps that get used without checks. Are you not concerned about these cases? These could cause a silent error with wild writes or could bring the system down. Or are you recommending changing the infrastructure to track failed maps as well?

I am still pursuing a way to track failed map cases. I combined the flag idea with one of the ideas I am looking into. Details below: (if this sounds like a reasonable approach, I can do v2 patch and we can discuss the code)

. Add new fields dma_map_errors, dma_map_errors_not_checked, dma_unmap_errors, iotlb_overflow_cnt, and flag to struct dma_debug_entry. Maybe flag is not even needed if dma_map_errors_not_checked can double as status.

. Enhance dma_debug_init() to create a second table to track failed maps with PREALLOC_DMA_DEBUG_ENTRIES/64 = 64. 64 devices probably is good enough.

. Entries added to this new table when debug_dma_map_page() detects error when mapping error is detected for the first time. Subsequent errors, will increment dma_map_errors, dma_map_errors_not_checked for that the device that is tracked by this entry. Note: paddr field could work as an index into this table (existing table uses dma_addr)

. Decrement dma_map_errors_not_checked from debug_dma_mapping_error(), clear the flag.

. check_unmap() when it detects mapping error, checks flag (status) and prints warn message.

-- Shuah

Quoted 30 lines · Shuah Khan

Quoted 10 lines · Joerg Roedel

Quoted 3 lines · Shuah Khan

Yeah. I will firm up my ideas a bit and summarize in a day or two. Would like to hear your ideas as well at that time, so we can pick the one that works the best.

I think the best approach for this functionality is to add a flag to 'struct dma_debug_entry' which tells whether the address has been checked with dma_mapping error or not. On unmap or driver unload you can then check for that flag and print a warning when an unchecked address is detected.

Was hoping to get comments from you as well. You are original author for this dam-debug module.

Are you ok with the system wide and per device error counts I added? Any comments on the overall approach?

The approach you suggested will cover the cases where drivers fail to check good map cases. We won't able to catch failed maps that get used without checks. Are you not concerned about these cases? These could cause a silent error with wild writes or could bring the system down. Or are you recommending changing the infrastructure to track failed maps as well?

I am still pursuing a way to track failed map cases. I combined the flag idea with one of the ideas I am looking into. Details below: (if this sounds like a reasonable approach, I can do v2 patch and we can discuss the code)

Quoted 4 lines

. Add new fields dma_map_errors, dma_map_errors_not_checked, dma_unmap_errors, iotlb_overflow_cnt, and flag to struct dma_debug_entry. Maybe flag is not even needed if dma_map_errors_not_checked can double as status.

Not sure if you need the iotlb_overflow_cnt anymore. Just having dma_map_errors_not_checked and the dma_map_errors (which you can increment/decrement) would suffice. Unless you were thinking to check that dma_map_errors == dma_unmap_errors and if they != then produce a warning?

Are you ok with the system wide and per device error counts I added? Any comments on the overall approach?

The approach you suggested will cover the cases where drivers fail to check good map cases. We won't able to catch failed maps that get used without checks. Are you not concerned about these cases? These could cause a silent error with wild writes or could bring the system down. Or are you recommending changing the infrastructure to track failed maps as well?

I am still pursuing a way to track failed map cases. I combined the flag idea with one of the ideas I am looking into. Details below: (if this sounds like a reasonable approach, I can do v2 patch and we can discuss the code)

Quoted 2 lines · Shuah Khan

Are you ok with the system wide and per device error counts I added? Any comments on the overall approach?

The general approach of having error counters is fine. But the addresses allocated/addresses checked thing should be done per allocation and not with counter comparison for several reasons:

1. When doing it per-allocation we know exactly which allocation was not checked and can tell the driver developer. The code saves stack-traces for that. This is much more useful than telling the developer 'somewhere you do not check your dma-handles'

2. Checking this per-allocation gives you the per-device and also the per-driver checking you want.

3. You don't need to change 'struct device' for that.

There are more reasons, like that this approach fits a lot better to the general idea of the DMA-API debugging code.

Quoted 6 lines

The approach you suggested will cover the cases where drivers fail to check good map cases. We won't able to catch failed maps that get used without checks. Are you not concerned about these cases? These could cause a silent error with wild writes or could bring the system down. Or are you recommending changing the infrastructure to track failed maps as well?

It is fine to only check the good-map cases. Think about what DMA-debugging is good for: It is a tool for driver developers to find bugs in their code they wouldn't notice otherwise. An unchecked bad-map case is a bug they would notice otherwise. So if we check only the good-map cases and warn the driver developers about non-checked addresses they fix it and make the drivers more robust against failed allocations, fixing also the bad-map cases.

Quoted 5 lines

I am still pursuing a way to track failed map cases. I combined the flag idea with one of the ideas I am looking into. Details below: (if this sounds like a reasonable approach, I can do v2 patch and we can discuss the code)

Why do you want to track the bad-map cases?

Enhancements to dma-debug API are made to add new debugfs interfaces to report total dma errors, dma errors that are not checked, and unmap errors for the entire system. Please note that these are system wide counters for all devices in the system.

The following new dma-debug interface is added:

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr):
Sets dma map error checked status for the dma map entry if one is found. Decrements the system wide dma_map_errors_not_checked counter that is incremented by debug_dma_map_page() when it checks for mapping error before adding it to the dma debug entry table.

The following existing dma-debug APIs are changed to support this feature:

debug_dma_map_page():
Increments dma_map_errors and dma_map_errors_not_checked error totals for the system when dma_addr is invalid. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling this routine at the time dma error unchecked state is registered, will not help if state gets changed right away.

check_unmap():
This is an existing internal routine that checks for various mapping errors. Changed to increment system wide dma_unmap_errors, when a device requests an invalid address to be unmapped. Please note that this routine can no longer call dma_mapping_error(), because of the newly added debug_dma_mapping_error() interface. Calling dma_mapping_error() from this routine will change the dma map error flag erroneously.

Changed arch/x86/include/asm/dma-mapping.h to call debug_dma_mapping_error() to validate these new interfaces on x86_64. Other architectures will be changed in a subsequent patch.

Tested: Intel iommu and swiotlb (iommu=soft) on x86-64 with
CONFIG_DMA_API_DEBUG enabled and disabled.

Signed-off-by Shuah Khan <[email protected]>

---
 Documentation/DMA-API.txt          |   13 +++++
 arch/x86/include/asm/dma-mapping.h |    1 +
 include/linux/dma-debug.h          |    7 +++
 lib/dma-debug.c                    |   92 ++++++++++++++++++++++++++++++++++--
 4 files changed, 109 insertions(+), 4 deletions(-)

Documentation/DMA-API.txt +13 -0

index 66bd97a..59d58b9 100644
@@ -638,6 +638,19 @@ this directory the following files can currently be found:
 	dma-api/error_count	This file is read-only and shows the total
 				numbers of errors found.
 
+	dma-api/dma_map_errors  This file is read-only and shows the total
+				number of dma mapping errors detected.
+
+	dma-api/dma_map_errors_not_checked
+				This file is read-only and shows the total
+				number of dma mapping errors, device drivers
+				failed to check prior to using the returned
+				address.
+
+	dma-api/dma_unmap_errors
+				This file is read-only and shows the total
+				number of invalid dma unmapping attempts.
+
 	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

arch/x86/include/asm/dma-mapping.h +1 -0

index f7b4c79..808dae6 100644
@@ -47,6 +47,7 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
+	debug_dma_mapping_error(dev, dma_addr);
 	if (ops->mapping_error)
 		return ops->mapping_error(dev, dma_addr);
 

include/linux/dma-debug.h +7 -0

index 171ad8a..fc0e34c 100644
@@ -39,6 +39,8 @@ extern void debug_dma_map_page(struct device *dev, struct page *page,
 			       int direction, dma_addr_t dma_addr,
 			       bool map_single);
 
+extern void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
+
 extern void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 				 size_t size, int direction, bool map_single);
 

@@ -105,6 +107,11 @@ static inline void debug_dma_map_page(struct device *dev, struct page *page,
 {
 }
 
+static inline void debug_dma_mapping_error(struct device *dev,
+					  dma_addr_t dma_addr)
+{
+}
+
 static inline void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 					size_t size, int direction,
 					bool map_single)

lib/dma-debug.c +0 -0

index 66ce414..70724a5 100644

lib/dma-debug.c +3 -2

 /* little merge helper - remove it after the merge window */
 #ifndef BUS_NOTIFY_UNBOUND_DRIVER
 #define BUS_NOTIFY_UNBOUND_DRIVER 0x0005

@@ -381,11 +399,12 @@ void debug_dma_dump_mappings(struct device *dev)
 		list_for_each_entry(entry, &bucket->list, list) {
 			if (!dev || dev == entry->dev) {
 				dev_info(entry->dev,
-					 "%s idx %d P=%Lx D=%Lx L=%Lx %s\n",
+					 "%s idx %d P=%Lx D=%Lx L=%Lx %s %s\n",
 					 type2name[entry->type], idx,
 					 (unsigned long long)entry->paddr,
 					 entry->dev_addr, entry->size,
-					 dir2name[entry->direction]);
+					 dir2name[entry->direction],
+					 maperr2str[entry->dma_map_error_flag]);
 			}
 		}
 

lib/dma-debug.c +8 -0

+		err_printk(ref->dev, entry,
+			   "DMA-API: device driver failed to check map error"
+			   "[device address=0x%016llx] [size=%llu bytes] "
+			   "[mapped as %s]",
+			   ref->dev_addr, ref->size,
+			   type2name[entry->type]);
+	}
+
 	hash_bucket_del(entry);
 	dma_entry_free(entry);
 

lib/dma-debug.c +1 -0

 	if (map_single)
 		entry->type = dma_debug_single;

@@ -1050,6 +1105,35 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 }
 EXPORT_SYMBOL(debug_dma_map_page);
 
+void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)

lib/dma-debug.c +8 -0

+		goto out;
+
+	entry->dma_map_error_flag = dma_map_error_checked;
+out:
+	put_hash_bucket(bucket, &flags);
+}
+EXPORT_SYMBOL(debug_dma_mapping_error);
+
 void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 			  size_t size, int direction, bool map_single)
 {

Signature · 1 line

1.7.9.5

Quoted 1 line

Thanks for improving this patch. It is looking more and more ready for the kernel. With that in mind, I've some comments below.

lib/dma-debug.c struct dma_debug_entry { · line 60

Quoted 10 lines

lib/dma-debug.c +1 -0

@@ -57,6 +57,7 @@ struct dma_debug_entry {
 	int              direction;
 	int		 sg_call_ents;
 	int		 sg_mapped_ents;
+	int		 dma_map_error_flag;

I don't think you need 'dma_map' as a prefix a this is in an dma_debug structure. Perhaps 'map_error_cnt' ?

But looking at the implementation this is actually an enum? Can you do this instead:

	enum map_err_type	map_err_type;

?

Quoted 7 lines

lib/dma-debug.c +0 -0

 #ifdef CONFIG_STACKTRACE
 	struct		 stack_trace stacktrace;
 	unsigned long	 st_entries[DMA_DEBUG_STACKTRACE_ENTRIES];

@@ -83,6 +84,11 @@ static u32 global_disable __read_mostly;
 /* Global error count */
 static u32 error_count;
 

lib/dma-debug.c static u32 global_disable __read_mostly; · line 90

Quoted 4 lines

lib/dma-debug.c +4 -0

+/* dma mapping error counts */
+static u32 dma_map_errors;
+static u32 dma_map_errors_not_checked;
+static u32 dma_unmap_errors;

s/dma//

lib/dma-debug.c static struct dentry *show_num_errors_dent __read_mostly; · line 115

Quoted 12 lines

lib/dma-debug.c +4 -0

+
 /* Global error show enable*/
 static u32 show_all_errors __read_mostly;
 /* Number of errors to show */

@@ -104,6 +110,9 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *dma_map_errors_dent   __read_mostly;
+static struct dentry *dma_map_errors_not_checked_dent   __read_mostly;
+static struct dentry *dma_unmap_errors_dent __read_mostly;

Ditto.

lib/dma-debug.c static const char *type2name[4] = { "single", "page", · line 136

Quoted 13 lines

lib/dma-debug.c +5 -0

 /* per-driver filter related state */
 

@@ -120,6 +129,15 @@ static const char *type2name[4] = { "single", "page",
 static const char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",
 				   "DMA_FROM_DEVICE", "DMA_NONE" };
 
+enum {
+	dma_map_error_check_not_applicable,
+	dma_map_error_not_checked,
+	dma_map_error_checked,
+};

s/dma//
s/error/err//

And perhaps make them uppercase?

Can you name the enum? Say 'map_err_types'

lib/dma-debug.c static const char *type2name[4] = { "single", "page", · line 140

Quoted 4 lines

lib/dma-debug.c +4 -0

+static const char *maperr2str[3] = { "dma map error check not applicable",
+				     "dma map error not checked",
+				     "dma map error checked" };
+

Just do this:

static const char *const names[] = {
		[err_check_na] = {"check n/a"},
		[err_not_checked] = {"not checked"},
		.. snip..
		};

That way you don't have to worry about the size and can just use ARRAY_SIZE(names) to check for valid enum size.

Quoted 19 lines

lib/dma-debug.c +3 -2

 /* little merge helper - remove it after the merge window */
 #ifndef BUS_NOTIFY_UNBOUND_DRIVER
 #define BUS_NOTIFY_UNBOUND_DRIVER 0x0005

@@ -381,11 +399,12 @@ void debug_dma_dump_mappings(struct device *dev)
 		list_for_each_entry(entry, &bucket->list, list) {
 			if (!dev || dev == entry->dev) {
 				dev_info(entry->dev,
-					 "%s idx %d P=%Lx D=%Lx L=%Lx %s\n",
+					 "%s idx %d P=%Lx D=%Lx L=%Lx %s %s\n",
 					 type2name[entry->type], idx,
 					 (unsigned long long)entry->paddr,
 					 entry->dev_addr, entry->size,
-					 dir2name[entry->direction]);
+					 dir2name[entry->direction],
+					 maperr2str[entry->dma_map_error_flag]);
 			}
 		}
 

lib/dma-debug.c static int dma_debug_fs_init(void) · line 717

Quoted 4 lines

lib/dma-debug.c +1 -0

@@ -695,6 +714,28 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	dma_map_errors_dent = debugfs_create_u32("dma_map_errors", 0444,

Just call it 'map_errors'

lib/dma-debug.c static int dma_debug_fs_init(void) · line 725

Quoted 8 lines

lib/dma-debug.c +8 -0

+			dma_debug_dent,
+			&dma_map_errors);
+
+	if (!dma_map_errors_dent)
+		goto out_err;
+
+	dma_map_errors_not_checked_dent = debugfs_create_u32(
+			"dma_map_errors_not_checked",

Ditto. s/dma//

lib/dma-debug.c static int dma_debug_fs_init(void) · line 733

Quoted 8 lines

lib/dma-debug.c +8 -0

+			0444,
+			dma_debug_dent,
+			&dma_map_errors_not_checked);
+
+	if (!dma_map_errors_not_checked_dent)
+		goto out_err;
+
+	dma_unmap_errors_dent = debugfs_create_u32("dma_unmap_errors", 0444,

s/dma//

lib/dma-debug.c static int dma_debug_fs_init(void) · line 738

Quoted 5 lines

lib/dma-debug.c +5 -0

+			dma_debug_dent,
+			&dma_unmap_errors);
+	if (!dma_unmap_errors_dent)
+		goto out_err;
+

This whole function could use a a loop to set this up instead of doing one by one... But that is another patch that can be done later.

Quoted 8 lines

lib/dma-debug.c +0 -0

 	return 0;
 
 out_err:

@@ -849,7 +890,8 @@ static void check_unmap(struct dma_debug_entry *ref)
 	struct hash_bucket *bucket;
 	unsigned long flags;
 

lib/dma-debug.c static void check_unmap(struct dma_debug_entry *ref) · line 893

Quoted 2 lines

lib/dma-debug.c +1 -1

-	if (dma_mapping_error(ref->dev, ref->dev_addr)) {
+	if (ref->dev_addr == DMA_ERROR_CODE) {

The DMA_ERROR_CODE is not exported on every architecture. Worst yet, it is not used universally on all IOMMUs - if you look in the GART (gart_mapping_error) it does not check for the DMA_ERROR_CODE but for its own bad_dma_addr address. I think using the dma_mapping_errors here is still a good idea.

lib/dma-debug.c static void check_unmap(struct dma_debug_entry *ref) · line 960

Quoted 10 lines

lib/dma-debug.c +2 -0

+		dma_unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;

@@ -915,6 +957,15 @@ static void check_unmap(struct dma_debug_entry *ref)
 			   dir2name[ref->direction]);
 	}
 
+	if (entry->dma_map_error_flag == dma_map_error_not_checked) {

Wait. Aren't you using dma_map_error_flag to only be up to 3 - as you are using it to lookup in the string table for its proper name?

Perhaps the string table lookup should use a different flag??

Quoted 12 lines

lib/dma-debug.c +8 -0

+		err_printk(ref->dev, entry,
+			   "DMA-API: device driver failed to check map error"
+			   "[device address=0x%016llx] [size=%llu bytes] "
+			   "[mapped as %s]",
+			   ref->dev_addr, ref->size,
+			   type2name[entry->type]);
+	}
+
 	hash_bucket_del(entry);
 	dma_entry_free(entry);
 

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1076

Quoted 5 lines

lib/dma-debug.c +1 -1

@@ -1022,8 +1073,11 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	if (unlikely(global_disable))
 		return;
 
-	if (unlikely(dma_mapping_error(dev, dma_addr)))
+	if (dma_addr == DMA_ERROR_CODE) {

Ditto

Quoted 5 lines

lib/dma-debug.c +3 -0

+		dma_map_errors += 1;
+		dma_map_errors_not_checked += 1;
 		return;
+	}
 

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1092

Quoted 7 lines

lib/dma-debug.c +1 -0

 	entry = dma_entry_alloc();
 	if (!entry)

@@ -1035,6 +1089,7 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	entry->dev_addr  = dma_addr;
 	entry->size      = size;
 	entry->direction = direction;
+	entry->dma_map_error_flag = dma_map_error_not_checked;

So if it is greater than three, then maperr2str[entry->dma_map_error_flag] is going to blow up, right?

Quoted 9 lines

lib/dma-debug.c +1 -0

 	if (map_single)
 		entry->type = dma_debug_single;

@@ -1050,6 +1105,35 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 }
 EXPORT_SYMBOL(debug_dma_map_page);
 
+void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1118

Quoted 10 lines

lib/dma-debug.c +10 -0

+{
+	struct dma_debug_entry ref;
+	struct dma_debug_entry *entry;
+	struct hash_bucket *bucket;
+	unsigned long flags;
+
+	if (unlikely(global_disable))
+		return;
+
+	if (dma_addr == DMA_ERROR_CODE) {

Don't use that... Just the IOMMUs' dma_mapping_error call..

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1119

Quoted 1 line

lib/dma-debug.c +1 -0

+		dma_map_errors_not_checked -= 1;

Should you check in case the user calls dma_mapping_error more than once on the same dma_addr?

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1128

Quoted 10 lines

lib/dma-debug.c +9 -0

+		return;
+	}
+
+	ref.dev = dev;
+	ref.dev_addr = dma_addr;
+	bucket = get_hash_bucket(&ref, &flags);
+	entry = bucket_find_exact(bucket, &ref);
+
+	if (!entry) /* very likley dma-api didn't call debug_dma_map_page() */

Ok, so should we re-adjust dma_map_errors_not_checked? Or we don't care?

Quoted 16 lines

lib/dma-debug.c +8 -0

+		goto out;
+
+	entry->dma_map_error_flag = dma_map_error_checked;
+out:
+	put_hash_bucket(bucket, &flags);
+}
+EXPORT_SYMBOL(debug_dma_mapping_error);
+
 void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 			  size_t size, int direction, bool map_single)
 {

Signature · 1 line

1.7.9.5

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr):
Sets dma map error checked status for the dma map entry if one is found. Decrements the system wide dma_map_errors_not_checked counter that is incremented by debug_dma_map_page() when it checks for mapping error before adding it to the dma debug entry table.

Hi Shuah,

the patch looks better then the older versions. It comes closer to a merge, but I see one issue here:

Quoted 5 lines · Shuah Khan

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr):
Sets dma map error checked status for the dma map entry if one is found. Decrements the system wide dma_map_errors_not_checked counter that is incremented by debug_dma_map_page() when it checks for mapping error before adding it to the dma debug entry table.

It is problematic that the DMA debug code can no longer call dma_mapping_error because of this. How about adding a special version of that function which does no checking and use it in the DMA debug code instead?

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

Quoted 10 lines · On Wed, 03 Oct 2012 08:55:59 -0600 Shuah Khan <[email protected]> wrote:

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

Quoted 4 lines

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

This seems to be a strange way of addressing kernel programming errors. Instead of fixing them up, we generate lots of statistics about how often they happen!

Would it not be better to find and fix the buggy code sites? A coccinelle script wold probably help here.

And let's also look at *why* we keep doing this. Partly it's because these things are self-propagating - people copy-n-paste bad code so we get more bad code.

Another reason surely is the poor documentation. Suppose our diligent programmer goes to the dma_map_single() definition site:

#define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL)

No documentation at all. Because it's a stupid macro it doesn't even give the types and names of the arguments or the type of the return value.

So he goes to dma_map_single_attrs() and finds that is altogether undocmented.

So he goes into Documentation/DMA-API-HOWTO.txt, searches for "dma_map_single" and finds

: To map a single region, you do:
:
: struct device *dev = &my_dev->dev;
: dma_addr_t dma_handle;
: void *addr = buffer->ptr;
: size_t size = buffer->len;
:
: dma_handle = dma_map_single(dev, addr, size, direction);
:
: and to unmap it:
:
: dma_unmap_single(dev, dma_handle, size, direction);

So it is hardly surprising that we keep screwing this up!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 18 lines · Andrew Morton

Quoted 14 lines · On Wed, 03 Oct 2012 08:55:59 -0600 Shuah Khan <[email protected]> wrote:

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

Quoted 3 lines

This seems to be a strange way of addressing kernel programming errors. Instead of fixing them up, we generate lots of statistics about how often they happen!

And by using this we can fix the drivers.

Quoted 3 lines

Would it not be better to find and fix the buggy code sites? A coccinelle script wold probably help here.

That is the end goal (fixing the buggy code sites). Shuah has identified the bad culprits.

Quoted 5 lines

And let's also look at *why* we keep doing this. Partly it's because these things are self-propagating - people copy-n-paste bad code so we get more bad code.

And this patch will now tell the poor engineers that write new code that they pasted bad code.

Quoted 31 lines

Another reason surely is the poor documentation. Suppose our diligent programmer goes to the dma_map_single() definition site:

#define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL)

No documentation at all. Because it's a stupid macro it doesn't even give the types and names of the arguments or the type of the return value.

So he goes to dma_map_single_attrs() and finds that is altogether undocmented.

So he goes into Documentation/DMA-API-HOWTO.txt, searches for "dma_map_single" and finds

: To map a single region, you do:
:
: struct device *dev = &my_dev->dev;
: dma_addr_t dma_handle;
: void *addr = buffer->ptr;
: size_t size = buffer->len;
:
: dma_handle = dma_map_single(dev, addr, size, direction);
:
: and to unmap it:
:
: dma_unmap_single(dev, dma_handle, size, direction);

Quoted 1 line

So it is hardly surprising that we keep screwing this up!

Right, so that should be also modified (Thank you for looking at that)!

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

Patch v3: Addresses review and design comments from Patch v2. Patch v2: Addressed design issues from Patch v1.

Enhance dma-debug infrastructure to track dma mapping, and unmapping errors.

map_errors: (system wide counter)
Total number of dma mapping errors returned by the dma mapping interfaces, in response to mapping requests from all devices in the system.
map_errors_not_checked: (system wide counter)
Total number of dma mapping errors devices failed to check before using the returned address.
unmap_errors: (system wide counter)
Total number of times devices tried to unmap or free an invalid dma address.
map_err_type: (new field added to dma_debug_entry structure)
New field to maintain dma mapping error check status. This error type is applicable to the dma map page and dma map single entries tracked by dma-debug api. This status indicates whether or not a good mapping is checked by the device before its use. dma_map_single() and dma_map_page() could fail to create a mapping in some cases, and drivers are expected to call dma_mapping_error() to check for errors. Please note that this is not counter.

Enhancements to dma-debug api are made to add new debugfs interfaces to report total dma errors, dma errors that are not checked, and unmap errors for the entire system. Please note that these are system wide counters for all devices in the system.

The following new dma-debug interface is added:

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
Sets dma map error checked status for the dma map entry if one is found. Decrements the system wide dma_map_errors_not_checked counter that is incremented by debug_dma_map_page() when it checks for mapping error before adding it to the dma debug entry table.

New dma-debug internal interface:
check_mapping_error(struct device *dev, dma_addr_t dma_addr) Calling dma_mapping_error() from dma-debug api will result in dma mapping check when it shouldn't. This internal routine checks dma mapping error without any debug checks.

The following existing dma-debug api are changed to support this feature:
debug_dma_map_page()
Increments dma_map_errors and dma_map_errors_not_checked errors totals for the system, dma-debug api keeps track of, when dma_addr is invalid. This routine now calls internal check_mapping_error() interface to avoid doing dma mapping debug checks from dma-debug internal mapping error checks.
check_unmap()
This is an existing internal routines that checks for various mapping errors. Changed to increment system wide dma_unmap_errors, when a device requests an invalid address to be unmapped. This routine now calls internal check_mapping_error() interface to avoid doing dma mapping debug checks from dma-debug internal mapping error checks.

Changed arch/x86/include/asm/dma-mapping.h to call debug_dma_mapping_error() to validate these new interfaces on x86_64. Other architectures will be changed in a subsequent patch.

Tested: Intel iommu and swiotlb (iommu=soft) on x86-64 with
CONFIG_DMA_API_DEBUG enabled and disabled.

Signed-off-by Shuah Khan <[email protected]>

---
 Documentation/DMA-API.txt          |   13 ++++
 arch/x86/include/asm/dma-mapping.h |    1 +
 include/linux/dma-debug.h          |    7 ++
 lib/dma-debug.c                    |  128 ++++++++++++++++++++++++++++++++----
 4 files changed, 136 insertions(+), 13 deletions(-)

Documentation/DMA-API.txt +13 -0

index 66bd97a..886aaf9 100644
@@ -638,6 +638,19 @@ this directory the following files can currently be found:
 	dma-api/error_count	This file is read-only and shows the total
 				numbers of errors found.
 
+	dma-api/map_errors	This file is read-only and shows the total
+				number of dma mapping errors detected.
+
+	dma-api/map_errors_not_checked
+				This file is read-only and shows the total
+				number of dma mapping errors, device drivers
+				failed to check prior to using the returned
+				address.
+
+	dma-api/unmap_errors
+				This file is read-only and shows the total
+				number of invalid dma unmapping attempts.
+
 	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

arch/x86/include/asm/dma-mapping.h +1 -0

index f7b4c79..808dae6 100644
@@ -47,6 +47,7 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
+	debug_dma_mapping_error(dev, dma_addr);
 	if (ops->mapping_error)
 		return ops->mapping_error(dev, dma_addr);
 

include/linux/dma-debug.h +7 -0

index 171ad8a..fc0e34c 100644
@@ -39,6 +39,8 @@ extern void debug_dma_map_page(struct device *dev, struct page *page,
 			       int direction, dma_addr_t dma_addr,
 			       bool map_single);
 
+extern void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
+
 extern void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 				 size_t size, int direction, bool map_single);
 

@@ -105,6 +107,11 @@ static inline void debug_dma_map_page(struct device *dev, struct page *page,
 {
 }
 
+static inline void debug_dma_mapping_error(struct device *dev,
+					  dma_addr_t dma_addr)
+{
+}
+
 static inline void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 					size_t size, int direction,
 					bool map_single)

lib/dma-debug.c +6 -9

index 66ce414..1708a10 100644
@@ -45,18 +45,25 @@ enum {
 	dma_debug_coherent,
 };
 
+enum map_err_types {
+	MAP_ERR_CHECK_NOT_APPLICABLE,
+	MAP_ERR_NOT_CHECKED,
+	MAP_ERR_CHECKED,
+};
+
 #define DMA_DEBUG_STACKTRACE_ENTRIES 5
 
 struct dma_debug_entry {
-	struct list_head list;
-	struct device    *dev;
-	int              type;
-	phys_addr_t      paddr;
-	u64              dev_addr;
-	u64              size;
-	int              direction;
-	int		 sg_call_ents;
-	int		 sg_mapped_ents;

lib/dma-debug.c +99 -4

 #ifdef CONFIG_STACKTRACE
 	struct		 stack_trace stacktrace;
 	unsigned long	 st_entries[DMA_DEBUG_STACKTRACE_ENTRIES];

@@ -83,6 +90,11 @@ static u32 global_disable __read_mostly;
 /* Global error count */
 static u32 error_count;
 
+/* dma mapping error counts */
+static u32 map_errors;
+static u32 map_errors_not_checked;
+static u32 unmap_errors;
+
 /* Global error show enable*/
 static u32 show_all_errors __read_mostly;
 /* Number of errors to show */

@@ -104,6 +116,9 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *map_errors_dent   __read_mostly;
+static struct dentry *map_errors_not_checked_dent   __read_mostly;
+static struct dentry *unmap_errors_dent __read_mostly;
 
 /* per-driver filter related state */
 

@@ -114,6 +129,12 @@ static struct device_driver *current_driver                    __read_mostly;
 
 static DEFINE_RWLOCK(driver_name_lock);
 
+static const char *const maperr2str[] = {
+	[MAP_ERR_CHECK_NOT_APPLICABLE] = "dma map error check not applicable",
+	[MAP_ERR_NOT_CHECKED] = "dma map error not checked",
+	[MAP_ERR_CHECKED] = "dma map error checked",
+};
+
 static const char *type2name[4] = { "single", "page",
 				    "scather-gather", "coherent" };
 

@@ -381,11 +402,12 @@ void debug_dma_dump_mappings(struct device *dev)
 		list_for_each_entry(entry, &bucket->list, list) {
 			if (!dev || dev == entry->dev) {
 				dev_info(entry->dev,
-					 "%s idx %d P=%Lx D=%Lx L=%Lx %s\n",
+					 "%s idx %d P=%Lx D=%Lx L=%Lx %s %s\n",
 					 type2name[entry->type], idx,
 					 (unsigned long long)entry->paddr,
 					 entry->dev_addr, entry->size,
-					 dir2name[entry->direction]);
+					 dir2name[entry->direction],
+					 maperr2str[entry->map_err_type]);
 			}
 		}
 

@@ -695,6 +717,28 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	map_errors_dent = debugfs_create_u32("map_errors", 0444,
+			dma_debug_dent,
+			&map_errors);
+
+	if (!map_errors_dent)
+		goto out_err;
+
+	map_errors_not_checked_dent = debugfs_create_u32(
+			"map_errors_not_checked",
+			0444,
+			dma_debug_dent,
+			&map_errors_not_checked);
+
+	if (!map_errors_not_checked_dent)
+		goto out_err;
+
+	unmap_errors_dent = debugfs_create_u32("unmap_errors", 0444,
+			dma_debug_dent,
+			&unmap_errors);
+	if (!unmap_errors_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:

@@ -843,13 +887,29 @@ static __init int dma_debug_entries_cmdline(char *str)
 __setup("dma_debug=", dma_debug_cmdline);
 __setup("dma_debug_entries=", dma_debug_entries_cmdline);
 
+/* Calling dma_mapping_error() from dma-debug api will result in calling
+   debug_dma_mapping_error() - need internal mapping error routine to
+   avoid debug checks */
+#ifndef DMA_ERROR_CODE
+#define DMA_ERROR_CODE 0
+#endif
+static inline int check_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+	if (ops->mapping_error)
+		return ops->mapping_error(dev, dma_addr);
+
+	return (dma_addr == DMA_ERROR_CODE);
+}
+
 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)) {
+	if (unlikely(check_mapping_error(ref->dev, ref->dev_addr))) {
+		unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;

@@ -915,6 +975,15 @@ static void check_unmap(struct dma_debug_entry *ref)
 			   dir2name[ref->direction]);
 	}
 
+	if (entry->map_err_type == MAP_ERR_NOT_CHECKED) {
+		err_printk(ref->dev, entry,
+			   "DMA-API: device driver failed to check map error"
+			   "[device address=0x%016llx] [size=%llu bytes] "
+			   "[mapped as %s]",
+			   ref->dev_addr, ref->size,
+			   type2name[entry->type]);
+	}
+
 	hash_bucket_del(entry);
 	dma_entry_free(entry);
 

@@ -1022,8 +1091,11 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	if (unlikely(global_disable))
 		return;
 
-	if (unlikely(dma_mapping_error(dev, dma_addr)))
+	if (unlikely(check_mapping_error(dev, dma_addr))) {
+		map_errors += 1;
+		map_errors_not_checked += 1;
 		return;
+	}
 
 	entry = dma_entry_alloc();
 	if (!entry)

@@ -1035,6 +1107,7 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	entry->dev_addr  = dma_addr;
 	entry->size      = size;
 	entry->direction = direction;
+	entry->map_err_type = MAP_ERR_NOT_CHECKED;
 
 	if (map_single)
 		entry->type = dma_debug_single;

@@ -1050,6 +1123,35 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 }
 EXPORT_SYMBOL(debug_dma_map_page);
 
+void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	struct dma_debug_entry ref;
+	struct dma_debug_entry *entry;
+	struct hash_bucket *bucket;
+	unsigned long flags;
+
+	if (unlikely(global_disable))
+		return;
+
+	ref.dev = dev;
+	ref.dev_addr = dma_addr;
+	bucket = get_hash_bucket(&ref, &flags);
+	entry = bucket_find_exact(bucket, &ref);
+
+	if (!entry) {
+		/* very likley dma-api didn't call debug_dma_map_page() or
+		   debug_dma_map_page() detected mapping error */
+		if (map_errors_not_checked)
+			map_errors_not_checked -= 1;
+		goto out;
+	}
+
+	entry->map_err_type = MAP_ERR_CHECKED;
+out:
+	put_hash_bucket(bucket, &flags);
+}
+EXPORT_SYMBOL(debug_dma_mapping_error);
+
 void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 			  size_t size, int direction, bool map_single)
 {

Signature · 9 lines

1.7.9.5



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 178 lines · Shuah Khan

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

Patch v3: Addresses review and design comments from Patch v2. Patch v2: Addressed design issues from Patch v1.

Enhance dma-debug infrastructure to track dma mapping, and unmapping errors.

map_errors: (system wide counter)
Total number of dma mapping errors returned by the dma mapping interfaces, in response to mapping requests from all devices in the system.
map_errors_not_checked: (system wide counter)
Total number of dma mapping errors devices failed to check before using the returned address.
unmap_errors: (system wide counter)
Total number of times devices tried to unmap or free an invalid dma address.
map_err_type: (new field added to dma_debug_entry structure)
New field to maintain dma mapping error check status. This error type is applicable to the dma map page and dma map single entries tracked by dma-debug api. This status indicates whether or not a good mapping is checked by the device before its use. dma_map_single() and dma_map_page() could fail to create a mapping in some cases, and drivers are expected to call dma_mapping_error() to check for errors. Please note that this is not counter.

Enhancements to dma-debug api are made to add new debugfs interfaces to report total dma errors, dma errors that are not checked, and unmap errors for the entire system. Please note that these are system wide counters for all devices in the system.

The following new dma-debug interface is added:

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
Sets dma map error checked status for the dma map entry if one is found. Decrements the system wide dma_map_errors_not_checked counter that is incremented by debug_dma_map_page() when it checks for mapping error before adding it to the dma debug entry table.

New dma-debug internal interface:
check_mapping_error(struct device *dev, dma_addr_t dma_addr) Calling dma_mapping_error() from dma-debug api will result in dma mapping check when it shouldn't. This internal routine checks dma mapping error without any debug checks.

The following existing dma-debug api are changed to support this feature:
debug_dma_map_page()
Increments dma_map_errors and dma_map_errors_not_checked errors totals for the system, dma-debug api keeps track of, when dma_addr is invalid. This routine now calls internal check_mapping_error() interface to avoid doing dma mapping debug checks from dma-debug internal mapping error checks.
check_unmap()
This is an existing internal routines that checks for various mapping errors. Changed to increment system wide dma_unmap_errors, when a device requests an invalid address to be unmapped. This routine now calls internal check_mapping_error() interface to avoid doing dma mapping debug checks from dma-debug internal mapping error checks.

Changed arch/x86/include/asm/dma-mapping.h to call debug_dma_mapping_error() to validate these new interfaces on x86_64. Other architectures will be changed in a subsequent patch.

Tested: Intel iommu and swiotlb (iommu=soft) on x86-64 with
CONFIG_DMA_API_DEBUG enabled and disabled.

Signed-off-by Shuah Khan <[email protected]>

---
 Documentation/DMA-API.txt          |   13 ++++
 arch/x86/include/asm/dma-mapping.h |    1 +
 include/linux/dma-debug.h          |    7 ++
 lib/dma-debug.c                    |  128 ++++++++++++++++++++++++++++++++----
 4 files changed, 136 insertions(+), 13 deletions(-)

Documentation/DMA-API.txt +13 -0

index 66bd97a..886aaf9 100644
@@ -638,6 +638,19 @@ this directory the following files can currently be found:
 	dma-api/error_count	This file is read-only and shows the total
 				numbers of errors found.
 
+	dma-api/map_errors	This file is read-only and shows the total
+				number of dma mapping errors detected.
+
+	dma-api/map_errors_not_checked
+				This file is read-only and shows the total
+				number of dma mapping errors, device drivers
+				failed to check prior to using the returned
+				address.
+
+	dma-api/unmap_errors
+				This file is read-only and shows the total
+				number of invalid dma unmapping attempts.
+
 	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

arch/x86/include/asm/dma-mapping.h +1 -0

index f7b4c79..808dae6 100644
@@ -47,6 +47,7 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
+	debug_dma_mapping_error(dev, dma_addr);
 	if (ops->mapping_error)
 		return ops->mapping_error(dev, dma_addr);
 

include/linux/dma-debug.h +7 -0

index 171ad8a..fc0e34c 100644
@@ -39,6 +39,8 @@ extern void debug_dma_map_page(struct device *dev, struct page *page,
 			       int direction, dma_addr_t dma_addr,
 			       bool map_single);
 
+extern void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
+
 extern void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 				 size_t size, int direction, bool map_single);
 

@@ -105,6 +107,11 @@ static inline void debug_dma_map_page(struct device *dev, struct page *page,
 {
 }
 
+static inline void debug_dma_mapping_error(struct device *dev,
+					  dma_addr_t dma_addr)
+{
+}
+
 static inline void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 					size_t size, int direction,
 					bool map_single)

lib/dma-debug.c +11 -9

index 66ce414..1708a10 100644
@@ -45,18 +45,25 @@ enum {
 	dma_debug_coherent,
 };
 
+enum map_err_types {
+	MAP_ERR_CHECK_NOT_APPLICABLE,
+	MAP_ERR_NOT_CHECKED,
+	MAP_ERR_CHECKED,
+};
+
 #define DMA_DEBUG_STACKTRACE_ENTRIES 5
 
 struct dma_debug_entry {
-	struct list_head list;
-	struct device    *dev;
-	int              type;
-	phys_addr_t      paddr;
-	u64              dev_addr;
-	u64              size;
-	int              direction;
-	int		 sg_call_ents;
-	int		 sg_mapped_ents;
+	struct list_head    list;
+	struct device       *dev;
+	int                 type;
+	phys_addr_t         paddr;
+	u64                 dev_addr;

lib/dma-debug.c enum { · line 66

Quoted 5 lines

lib/dma-debug.c +5 -0

+	u64                 size;
+	int                 direction;
+	int		    sg_call_ents;
+	int		    sg_mapped_ents;
+	enum map_err_types  map_err_type;

Something gone wild with your tabs/space. It should be only one extra field added.

Otherwise:

Reviewed-by Konrad Rzeszutek Wilk <[email protected]>

Quoted 198 lines

lib/dma-debug.c +99 -4

 #ifdef CONFIG_STACKTRACE
 	struct		 stack_trace stacktrace;
 	unsigned long	 st_entries[DMA_DEBUG_STACKTRACE_ENTRIES];

@@ -83,6 +90,11 @@ static u32 global_disable __read_mostly;
 /* Global error count */
 static u32 error_count;
 
+/* dma mapping error counts */
+static u32 map_errors;
+static u32 map_errors_not_checked;
+static u32 unmap_errors;
+
 /* Global error show enable*/
 static u32 show_all_errors __read_mostly;
 /* Number of errors to show */

@@ -104,6 +116,9 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *map_errors_dent   __read_mostly;
+static struct dentry *map_errors_not_checked_dent   __read_mostly;
+static struct dentry *unmap_errors_dent __read_mostly;
 
 /* per-driver filter related state */
 

@@ -114,6 +129,12 @@ static struct device_driver *current_driver                    __read_mostly;
 
 static DEFINE_RWLOCK(driver_name_lock);
 
+static const char *const maperr2str[] = {
+	[MAP_ERR_CHECK_NOT_APPLICABLE] = "dma map error check not applicable",
+	[MAP_ERR_NOT_CHECKED] = "dma map error not checked",
+	[MAP_ERR_CHECKED] = "dma map error checked",
+};
+
 static const char *type2name[4] = { "single", "page",
 				    "scather-gather", "coherent" };
 

@@ -381,11 +402,12 @@ void debug_dma_dump_mappings(struct device *dev)
 		list_for_each_entry(entry, &bucket->list, list) {
 			if (!dev || dev == entry->dev) {
 				dev_info(entry->dev,
-					 "%s idx %d P=%Lx D=%Lx L=%Lx %s\n",
+					 "%s idx %d P=%Lx D=%Lx L=%Lx %s %s\n",
 					 type2name[entry->type], idx,
 					 (unsigned long long)entry->paddr,
 					 entry->dev_addr, entry->size,
-					 dir2name[entry->direction]);
+					 dir2name[entry->direction],
+					 maperr2str[entry->map_err_type]);
 			}
 		}
 

@@ -695,6 +717,28 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	map_errors_dent = debugfs_create_u32("map_errors", 0444,
+			dma_debug_dent,
+			&map_errors);
+
+	if (!map_errors_dent)
+		goto out_err;
+
+	map_errors_not_checked_dent = debugfs_create_u32(
+			"map_errors_not_checked",
+			0444,
+			dma_debug_dent,
+			&map_errors_not_checked);
+
+	if (!map_errors_not_checked_dent)
+		goto out_err;
+
+	unmap_errors_dent = debugfs_create_u32("unmap_errors", 0444,
+			dma_debug_dent,
+			&unmap_errors);
+	if (!unmap_errors_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:

@@ -843,13 +887,29 @@ static __init int dma_debug_entries_cmdline(char *str)
 __setup("dma_debug=", dma_debug_cmdline);
 __setup("dma_debug_entries=", dma_debug_entries_cmdline);
 
+/* Calling dma_mapping_error() from dma-debug api will result in calling
+   debug_dma_mapping_error() - need internal mapping error routine to
+   avoid debug checks */
+#ifndef DMA_ERROR_CODE
+#define DMA_ERROR_CODE 0
+#endif
+static inline int check_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+	if (ops->mapping_error)
+		return ops->mapping_error(dev, dma_addr);
+
+	return (dma_addr == DMA_ERROR_CODE);
+}
+
 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)) {
+	if (unlikely(check_mapping_error(ref->dev, ref->dev_addr))) {
+		unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;

@@ -915,6 +975,15 @@ static void check_unmap(struct dma_debug_entry *ref)
 			   dir2name[ref->direction]);
 	}
 
+	if (entry->map_err_type == MAP_ERR_NOT_CHECKED) {
+		err_printk(ref->dev, entry,
+			   "DMA-API: device driver failed to check map error"
+			   "[device address=0x%016llx] [size=%llu bytes] "
+			   "[mapped as %s]",
+			   ref->dev_addr, ref->size,
+			   type2name[entry->type]);
+	}
+
 	hash_bucket_del(entry);
 	dma_entry_free(entry);
 

@@ -1022,8 +1091,11 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	if (unlikely(global_disable))
 		return;
 
-	if (unlikely(dma_mapping_error(dev, dma_addr)))
+	if (unlikely(check_mapping_error(dev, dma_addr))) {
+		map_errors += 1;
+		map_errors_not_checked += 1;
 		return;
+	}
 
 	entry = dma_entry_alloc();
 	if (!entry)

@@ -1035,6 +1107,7 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	entry->dev_addr  = dma_addr;
 	entry->size      = size;
 	entry->direction = direction;
+	entry->map_err_type = MAP_ERR_NOT_CHECKED;
 
 	if (map_single)
 		entry->type = dma_debug_single;

@@ -1050,6 +1123,35 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 }
 EXPORT_SYMBOL(debug_dma_map_page);
 
+void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	struct dma_debug_entry ref;
+	struct dma_debug_entry *entry;
+	struct hash_bucket *bucket;
+	unsigned long flags;
+
+	if (unlikely(global_disable))
+		return;
+
+	ref.dev = dev;
+	ref.dev_addr = dma_addr;
+	bucket = get_hash_bucket(&ref, &flags);
+	entry = bucket_find_exact(bucket, &ref);
+
+	if (!entry) {
+		/* very likley dma-api didn't call debug_dma_map_page() or
+		   debug_dma_map_page() detected mapping error */
+		if (map_errors_not_checked)
+			map_errors_not_checked -= 1;
+		goto out;
+	}
+
+	entry->map_err_type = MAP_ERR_CHECKED;
+out:
+	put_hash_bucket(bucket, &flags);
+}
+EXPORT_SYMBOL(debug_dma_mapping_error);
+
 void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 			  size_t size, int direction, bool map_single)
 {

Signature · 9 lines

1.7.9.5



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

A recent dma mapping error analysis effort showed that a large percentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors.

Reference:
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

Patch v4: Addresses extra tab review comments from Patch v3. Patch v3: Addresses review and design comments from Patch v2. Patch v2: Addressed design issues from Patch v1.

Enhance dma-debug infrastructure to track dma mapping, and unmapping errors.

map_errors: (system wide counter)
Total number of dma mapping errors returned by the dma mapping interfaces, in response to mapping requests from all devices in the system.
map_errors_not_checked: (system wide counter)
Total number of dma mapping errors devices failed to check before using the returned address.
unmap_errors: (system wide counter)
Total number of times devices tried to unmap or free an invalid dma address.
map_err_type: (new field added to dma_debug_entry structure)
New field to maintain dma mapping error check status. This error type is applicable to the dma map page and dma map single entries tracked by dma-debug api. This status indicates whether or not a good mapping is checked by the device before its use. dma_map_single() and dma_map_page() could fail to create a mapping in some cases, and drivers are expected to call dma_mapping_error() to check for errors. Please note that this is not counter.

Enhancements to dma-debug api are made to add new debugfs interfaces to report total dma errors, dma errors that are not checked, and unmap errors for the entire system. Please note that these are system wide counters for all devices in the system.

The following new dma-debug interface is added:

debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
Sets dma map error checked status for the dma map entry if one is found. Decrements the system wide dma_map_errors_not_checked counter that is incremented by debug_dma_map_page() when it checks for mapping error before adding it to the dma debug entry table.

New dma-debug internal interface:
check_mapping_error(struct device *dev, dma_addr_t dma_addr) Calling dma_mapping_error() from dma-debug api will result in dma mapping check when it shouldn't. This internal routine checks dma mapping error without any debug checks.

The following existing dma-debug api are changed to support this feature:
debug_dma_map_page()
Increments dma_map_errors and dma_map_errors_not_checked errors totals for the system, dma-debug api keeps track of, when dma_addr is invalid. This routine now calls internal check_mapping_error() interface to avoid doing dma mapping debug checks from dma-debug internal mapping error checks.
check_unmap()
This is an existing internal routines that checks for various mapping errors. Changed to increment system wide dma_unmap_errors, when a device requests an invalid address to be unmapped. This routine now calls internal check_mapping_error() interface to avoid doing dma mapping debug checks from dma-debug internal mapping error checks.

Changed arch/x86/include/asm/dma-mapping.h to call debug_dma_mapping_error() to validate these new interfaces on x86_64. Other architectures will be changed in a subsequent patch.

Tested: Intel iommu and swiotlb (iommu=soft) on x86-64 with
CONFIG_DMA_API_DEBUG enabled and disabled.

From my reading of the code, this patch indeed does that, along with a

 Documentation/DMA-API.txt          |   13 +++++
 arch/x86/include/asm/dma-mapping.h |    1 +
 include/linux/dma-debug.h          |    7 +++
 lib/dma-debug.c                    |  110 ++++++++++++++++++++++++++++++++++--

static void check_unmap(struct dma_debug_entry *ref) {

lib/dma-debug.c +2 -1

 	struct dma_debug_entry *entry;
 	struct hash_bucket *bucket;
 	unsigned long flags;
 
-	if (dma_mapping_error(ref->dev, ref->dev_addr)) {
+	if (unlikely(check_mapping_error(ref->dev, ref->dev_addr))) {
+		unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;

@@ -915,6 +975,15 @@ static void check_unmap(struct dma_debug_entry *ref)
 			   dir2name[ref->direction]);

lib/dma-debug.c +18 -0

 	hash_bucket_del(entry);
 	dma_entry_free(entry);
 
 
...
 
+void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	struct dma_debug_entry ref;
+	struct dma_debug_entry *entry;
+	struct hash_bucket *bucket;
+	unsigned long flags;
+
+	if (unlikely(global_disable))
+		return;
+
+	ref.dev = dev;
+	ref.dev_addr = dma_addr;
+	bucket = get_hash_bucket(&ref, &flags);
+	entry = bucket_find_exact(bucket, &ref);
+
+	if (!entry) {
+		/* very likley dma-api didn't call debug_dma_map_page() or
+		   debug_dma_map_page() detected mapping error */

...

Still seems overly complicated to me, but whatev.

I think the way to handle this is pretty simple: set a flag in the dma entry when someone runs dma_mapping_error() and, if that flag wasn't set at unmap time, emit a loud warning.

From my reading of the code, this patch indeed does that, along with a bunch of other (unnecessary?) stuff. But boy, the changelog conceals this information well!

Quoted 5 lines

 Documentation/DMA-API.txt          |   13 +++++
 arch/x86/include/asm/dma-mapping.h |    1 +
 include/linux/dma-debug.h          |    7 +++
 lib/dma-debug.c                    |  110 ++++++++++++++++++++++++++++++++++--

Please, go through Documentation/DMA-API-HOWTO.txt with a toothcomb and ensure that all this is appropriately covered and that the examples are completed for error checking.

Quoted 3 lines

...

lib/dma-debug.c static __init int dma_debug_entries_cmdline(char *str) · line 903

Quoted 8 lines

lib/dma-debug.c +8 -0

+static inline int check_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+	if (ops->mapping_error)
+		return ops->mapping_error(dev, dma_addr);
+
+	return (dma_addr == DMA_ERROR_CODE);
+}

I'm not a fan of functions called check_foo() because I never know whether their return value means "foo is true" or "foo is false". It doesn't help that the return value here is undocumented. Names such as has_mapping_error() or mapping_has_error() will remove this question, thereby making the call sites more readable.

Quoted 15 lines

static void check_unmap(struct dma_debug_entry *ref) {

lib/dma-debug.c +2 -1

 	struct dma_debug_entry *entry;
 	struct hash_bucket *bucket;
 	unsigned long flags;
 
-	if (dma_mapping_error(ref->dev, ref->dev_addr)) {
+	if (unlikely(check_mapping_error(ref->dev, ref->dev_addr))) {
+		unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;

@@ -915,6 +975,15 @@ static void check_unmap(struct dma_debug_entry *ref)
 			   dir2name[ref->direction]);

lib/dma-debug.c static void check_unmap(struct dma_debug_entry *ref) · line 985

Quoted 10 lines

lib/dma-debug.c +8 -0

 	}
 
+	if (entry->map_err_type == MAP_ERR_NOT_CHECKED) {
+		err_printk(ref->dev, entry,
+			   "DMA-API: device driver failed to check map error"
+			   "[device address=0x%016llx] [size=%llu bytes] "
+			   "[mapped as %s]",
+			   ref->dev_addr, ref->size,
+			   type2name[entry->type]);
+	}

It's important that this warning be associated with a backtrace so we can identify the offending call site in the usual fashion. err_printk() does include a backtrace under some circumstances, but those circumstances hurt my brain.

Is it guaranteed that we'll have that backtrace? If not, I'd suggest making it so.

Quoted 30 lines

lib/dma-debug.c +23 -0

 	hash_bucket_del(entry);
 	dma_entry_free(entry);
 
 
...
 
+void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	struct dma_debug_entry ref;
+	struct dma_debug_entry *entry;
+	struct hash_bucket *bucket;
+	unsigned long flags;
+
+	if (unlikely(global_disable))
+		return;
+
+	ref.dev = dev;
+	ref.dev_addr = dma_addr;
+	bucket = get_hash_bucket(&ref, &flags);
+	entry = bucket_find_exact(bucket, &ref);
+
+	if (!entry) {
+		/* very likley dma-api didn't call debug_dma_map_page() or
+		   debug_dma_map_page() detected mapping error */
+		if (map_errors_not_checked)
+			map_errors_not_checked -= 1;
+		goto out;
+	}
+

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1153

Quoted 5 lines

lib/dma-debug.c +5 -0

+	entry->map_err_type = MAP_ERR_CHECKED;
+out:
+	put_hash_bucket(bucket, &flags);
+}
+EXPORT_SYMBOL(debug_dma_mapping_error);

Well, it's a global, exported-to-modules symbol. Some formal documentation is appropriate for such things.

Quoted 4 lines

...

Quoted 10 lines

Still seems overly complicated to me, but whatev.

I think the way to handle this is pretty simple: set a flag in the dma entry when someone runs dma_mapping_error() and, if that flag wasn't set at unmap time, emit a loud warning.

From my reading of the code, this patch indeed does that, along with a bunch of other (unnecessary?) stuff. But boy, the changelog conceals this information well!

Are you referring to the system wide error counters when you say unnecessary stuff. The reason I added those was to catch errors when drivers don't do unmap. Several drivers fail to do unmap. Checking flag from unmap debug interfaces, doesn't cover these cases. However, I think the value of system wide counters is limited in the sense that they don't really identify the driver that needs fixing. In that sense it can be deemed unnecessary. I dropped them in v5 patch, which I am sending out.

Quoted 11 lines · On Mon, 08 Oct 2012 11:07:20 -0600 Shuah Khan <[email protected]> wrote:

Quoted 10 lines

Still seems overly complicated to me, but whatev.

I think the way to handle this is pretty simple: set a flag in the dma entry when someone runs dma_mapping_error() and, if that flag wasn't set at unmap time, emit a loud warning.

From my reading of the code, this patch indeed does that, along with a bunch of other (unnecessary?) stuff. But boy, the changelog conceals this information well!

Quoted 8 lines

Are you referring to the system wide error counters when you say unnecessary stuff. The reason I added those was to catch errors when drivers don't do unmap. Several drivers fail to do unmap. Checking flag from unmap debug interfaces, doesn't cover these cases. However, I think the value of system wide counters is limited in the sense that they don't really identify the driver that needs fixing. In that sense it can be deemed unnecessary. I dropped them in v5 patch, which I am sending out.

hm. Could we keep a counter of the number of map/unmap calls within the dma object and then emit a warning if that is non-zero at teardown time? That should identify the offending driver.