devres: dma-mapping: Introducing new functions

This patchset consists of new functions to the managed device resource API, followed by a patch for the Xillybus driver, which is my motivation and what I tested with.

This is a resubmission after changing the API slightly.

Rationale: While migrating the staging/xillybus driver to rely completely on managed resources, some functionalities were missing, and hence added:

* dmam_map_single()
* dmam_unmap_single()
* pcim_map_single()
* pcim_unmap_single()

Tejun suggested that dma_map_single_attrs() should have a managed version as well. The second patch in this set turns dmam_map_single() into dma_map_single_attrs(), and implements the former as a macro. Functions added:

* dmam_map_single_attrs()
* dmam_unmap_single_attrs()

Xillybus' driver works with and without this patch (depends on patches #1 and #3 only).

Thanks,
Eli

Eli Billauer (4):
dma-mapping: Add devm_ interface for dma_map_single()
dma-mapping: Add devm_ interface for dma_map_single_attrs()
dma-mapping: pci: Add devm_ interface for pci_map_single
staging: xillybus: Use devm_ API for memory allocation and DMA
mapping

 Documentation/driver-model/devres.txt    |    6 +
 drivers/base/dma-mapping.c               |  106 +++++++++++++++++
 drivers/staging/xillybus/xillybus.h      |   38 +------
 drivers/staging/xillybus/xillybus_core.c |  186 +++++++++---------------------
 drivers/staging/xillybus/xillybus_of.c   |   61 +---------
 drivers/staging/xillybus/xillybus_pcie.c |   54 ++--------
 include/asm-generic/dma-mapping-common.h |    3 +
 include/asm-generic/pci-dma-compat.h     |   18 +++
 include/linux/dma-mapping.h              |    8 +-
 9 files changed, 214 insertions(+), 266 deletions(-)

1.7.2.3

dmam_map_single() and dmam_unmap_single() are the managed counterparts for the respective dma_* functions.

Note that dmam_map_single() returns a status value rather than the DMA handle. The DMA handle is passed to the caller through a pointer in the arguments.

The reason for this API change is that dmam_map_single() allocates memory, which may fail even if the DMA mapping is successful. On the other hand, it seems like there's no platform-independent value for dma_handle that can be used to indicate an error.

This leaves dmam_map_single() with no safe way to tell its caller that the memory allocation has failed, except for returning a status as an int. Trying to keep close to the non-devres API could also tempt programmers into using dma_mapping_error() on the dmam_* functions. This would work incorrectly on platforms, for which dma_mapping_error() ignores its argument, and always returns success.

Thanks to Tejun Heo for suggesting this API.

---
 Documentation/driver-model/devres.txt |    2 +
 drivers/base/dma-mapping.c            |   86 +++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h           |    6 ++-
 3 files changed, 93 insertions(+), 1 deletions(-)

index e1a2707..13b8be0 100644
@@ -266,6 +266,8 @@ DMA
   dmam_declare_coherent_memory()
   dmam_pool_create()
   dmam_pool_destroy()
+  dmam_map_single()
+  dmam_unmap_single()
 
 PCI
   pcim_enable_device()	: after success, all PCI ops become managed

index fd4aee2..cad63de 100644
@@ -233,7 +233,11 @@ static inline void dmam_release_declared_memory(struct device *dev)
 {
 }
 #endif /* ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY */
-
+int dmam_map_single(struct device *dev, void *ptr, size_t size,
+		    enum dma_data_direction direction,
+		    dma_addr_t *ret_dma_handle);
+void dmam_unmap_single(struct device *dev, dma_addr_t dma_handle,
+		       size_t size, enum dma_data_direction direction);
 #ifndef CONFIG_HAVE_DMA_ATTRS
 struct dma_attrs;
 

1.7.2.3

I see the value of this interface in unmap case, this type of wrapper can release dma buffers, drivers neglected to release leaving dangling buffers.

However, driver writers should give it some thought before switching from conventional dma_map_*() interfaces to these proposed managed dma mapping interfaces. These new interfaces shouldn't be treated as drop in replacements to dma_map_*() interfaces.

The reasons are:

1. This interface adds an overhead in allocation memory for devres to compared to other dma_map_* wrappers. Users need to be aware of that. This would be okay in the cases where a driver allocates several buffers at init time and uses them. However, several drivers allocate during run-time and release as soon as it is no longer needed. This overhead is going to be in the performance path.

2. It adds a failure case even when dma buffers are available. i.e if if devres alloc fails, it will return failure even if dma map could have succeeded. This is a new failure case for DMA-API.

The reason dma_map_*() routines fail now is because there are no buffers available. Drivers handle this error as a retry case.

Drivers using dmam_map_single() will have to handle the failure cases differently.

Since the return values are different for dmam_map_*(), that is plus that these interfaces can't be drop in replacements to the dma_map_*() interfaces.

3. Similarly, it adds an overhead in releasing memory for devres to compared to other dma_unmap_* wrappers. Users need to be aware of that. This overhead is going to be in the performance path when drivers unmap buffers during run-time.

Adding Joerg Roedel to the thread.

-- Shuah

Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
[email protected] | (970) 672-0658

I fully agree with the points Shuah brought up here. I don't think it is a good idea to add this kind of resource management to runtime-allocated (and de-allocated) resources of device drivers.

Also DMA handles are not something that could be garbage collected at driver unload time. They are a limited resource that may be used up at some point. And the whole point of a devm-API is that code can be simpler because we don't need to de-allocate everything on the error-path or at unload time, no?

Besides that, we already have DMA-API debug to find drivers that do not release all their DMA buffers.

Joerg

Hello,

Hmmm? Don't we have drivers which map dma buffers on device init and release them on exit? For dynamic usages, its usefulness is limited especially given that dynamic tracking of buffers usually would involve tracking of other information too in addition to dma buffer pointer themselves. If alloc on init and free on exit is a very rare usage pattern, I have no objection against not adding devm interface for dma mappings.

Thanks.

tejun

Yes, but those drivers usually get DMA buffers at init time with the dma_alloc_* interfaces. The dma_map_* interfaces discussed here belong to the streaming DMA-API, so they are usually used for only one DMA transaction before dma_unmap_* is called on them.

A devm interface for the dma_alloc_* family of functions would actually make sense, but not for the dma_map_* functions.

Joerg

Hello,

Ah, okay. Fair enough.

Thanks.

tejun

Hi,

I believe that I need a managed dma_map_single() my own driver, which doesn't fall in the case of a single use: The driver allocates its buffers with __get_free_pages() (or the to-be managed version of it). Then it cuts the allocated memory into smaller buffers (in some cases, and with certain alignment rules), and then calls dma_map_single() to do the DMA mapping for each. The buffers are held along the driver's lifetime, with DMA sync API juggling control back and forth to the hardware. Note that the DMA is noncoherent.

Once could argue, that since dma_map_noncoherent() calls __get_free_pages() under the hood, I could request the larger chunk from dma_map_noncoherent(), and cut it into smaller DMA buffers. My concern is that the dma_sync_single_*() functions expect a DMA handle, not a physical address I've made up with my own buffer splitting. I don't see any problem with this trick on platforms I've worked with, but I'm not sure if this is the proper way to go. dma_map_single(), on the other hand, returns a DMA handle.

The DMA pool functions could be interesting, but I understand that they would supply me with coherent memory only.

Anything I missed?

Thanks,
Eli

Hi,

What you are trying to do should work with dma_alloc_noncoherent(). The API allows partial syncs on this memory, so you should be fine.

The problem with a devm variant of dma_map_* is that it is too easy to misuse or to use it wrong so that a driver could eat up all available DMA handles on some platforms.

Joerg

Hello Joerg.

Please try to put yourself in my position: I have a driver that I care about, which works fine for a few years. It's based upon dma_map_single(), which seems to be the common way to get non-coherent memory, even for the driver's entire lifespan. I realize that dma_alloc_* was the intended way to do it, but fact is that dma_map_* has become the common choice.

Now I need to switch to dma_alloc_noncoherent(), which isn't used by many drivers, it seems. It should work the same, but there's always the worry if I'll cover all the corners. So will I take this risk of a nasty DMA bug on some esoteric platform, just to cut some lines in the code?

And if I choose to keep the unmanaged dma_map_single(), maybe I'll mess up if I convert other allocations to the managed API? Hmmm, maybe it's best to forget all about it.

I suppose you mean that those who use dma_map_* for a one-off DMA session will drop the unmapping, thinking that it's "managed anyhow"...? Well, you can say that about any of the managed functions. For example, when devm_kzalloc() was introduced, someone maybe argued that people would drop kfree()s where they shouldn't, causing memory leaks.

So I think it boils down to whether devres is a good idea or not. Someone who thinks it's bad, will reject any new API, referring to memory efficiency, additional causes of failure and the risk of misleading the herd.

But if devres is to become commonly used in the future, I think drop-in replacements are necessary. In my opinion, telling people to adopt another methodology (e.g. dma_alloc_noncoherent vs. mapping), even if functionally equivalent, is a good way to make sure devres is never adopted.

Regards,
Eli

Is your driver in the kernel tree? If not, you really are on your own :(

greg k-h

It's the Xillybus driver in the staging area. I don't know if this counts for being in the kernel tree...

The suggested patchset would allow replacing my use of dma_map_single() with a managed version of that function. This will clean the driver's code considerably.

But I think that the discussion here is if it's valid to use dma_map_single() for a device-permanent DMA mapping, or if dma_alloc_noncoherent() is the only way. If the answer is no, there's quite obviously no point in a devres version for that function.

Regards,
Eli

Eli,

dma_map_single() and dma_unmap_single() are streaming DMA APIs. These are intended for one DMA transfer and unmapped right after it is done.

dma buffers are limited shared resources for streaming that are shared by several drivers. Hence the need for use and release model.

Please refer to the Using Streaming DMA mappings in DMA-API-HOWTO.txt

"- Streaming DMA mappings which are usually mapped for one DMA
transfer, unmapped right after it (unless you use dma_sync_* below) and for which hardware can optimize for sequential accesses.

This of "streaming" as "asynchronous" or "outside the coherency domain".

Good examples of what to use streaming mappings for are:

- Networking buffers transmitted/received by a device.
- Filesystem buffers written/read by a SCSI device."

If I understand your intended usage correctly, you are looking to allocate and hold the buffers for the lifetime of the driver. For such cases, dma_alloc_*() interfaces are the ones to use.

Please also refer to DMA-API.txt as well. Hope this helps.

-- Shuah

Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
[email protected] | (970) 672-0658

Hello Shuah,

We agree that the streaming API was originally *intended* for short map-unmap DMA sessions, and that dma_alloc_noncoherent() was the *intended* API for those who want to hold the DMA during a device's lifetime.

We also agree that on some platforms, DMA mappings are precious, and therefore any driver should unmap a region as soon as it's not needed anymore.

But if we stick to the citation you gave, it says "...unmapped right after it (unless you use dma_sync_* below)". So even in the streaming API's definition, there's an understanding, that the "streaming" DMA buffer can be held for more than a single session. And a good sync tool for that is made available.

Using cross-reference on Linux' code, I get a strong impression, that dma_alloc_NONcoherent() is pretty much unused (I counted 8 drivers). The streaming API's sync functions are heavily used, on the other hand. So one gets a hunch, that there's a lot of use of the streaming API in the kernel tree for long-term DMA mappings.

This wasn't the original intention -- we agree on that. But why is it wrong? Assuming that a driver needs to hold a DMA mapping for a long while, why does it matter if it was done with dma_alloc_noncoherent() or with dma_map_*()? They are equally wasteful, aren't they?

Why maintaining two API sets doing the same thing? Or is there a subtle functional difference I'm not aware of?

Thanks,
Eli

dmam_map_single_attrs() and dmam_unmap_single_attrs() replace the non-*_attrs functions, which are implemented as defines instead.

The case of a non-NULL @attrs parameter has not been tested.

---
 Documentation/driver-model/devres.txt    |    2 +
 drivers/base/dma-mapping.c               |   46 +++++++++++++++++++++--------
 include/asm-generic/dma-mapping-common.h |    3 ++
 include/linux/dma-mapping.h              |   12 ++++---
 4 files changed, 45 insertions(+), 18 deletions(-)

index 13b8be0..2112a00 100644
@@ -268,6 +268,8 @@ DMA
   dmam_pool_destroy()
   dmam_map_single()
   dmam_unmap_single()
+  dmam_map_single_attrs()
+  dmam_unmap_single_attrs()
 
 PCI
   pcim_enable_device()	: after success, all PCI ops become managed

index de8bf89..1af27b3 100644
@@ -175,6 +175,9 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
 #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, NULL)
 #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, NULL)
 #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, NULL)
+#define dmam_map_single(d, a, s, r, p) \
+	dmam_map_single_attrs(d, a, s, r, NULL, p)
+#define dmam_unmap_single(d, a, s, r) dmam_unmap_single_attrs(d, a, s, r, NULL)
 
 extern int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
 			   void *cpu_addr, dma_addr_t dma_addr, size_t size);

index cad63de..4ca9134 100644
@@ -233,11 +233,13 @@ static inline void dmam_release_declared_memory(struct device *dev)
 {
 }
 #endif /* ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY */
-int dmam_map_single(struct device *dev, void *ptr, size_t size,
-		    enum dma_data_direction direction,
-		    dma_addr_t *ret_dma_handle);
-void dmam_unmap_single(struct device *dev, dma_addr_t dma_handle,
-		       size_t size, enum dma_data_direction direction);
+int dmam_map_single_attrs(struct device *dev, void *ptr, size_t size,
+			  enum dma_data_direction direction,
+			  struct dma_attrs *attrs,
+			  dma_addr_t *ret_dma_handle);
+void dmam_unmap_single_attrs(struct device *dev, dma_addr_t dma_handle,
+			     size_t size, enum dma_data_direction direction,
+			     struct dma_attrs *attrs);
 #ifndef CONFIG_HAVE_DMA_ATTRS
 struct dma_attrs;
 

1.7.2.3

---
 Documentation/driver-model/devres.txt |    2 ++
 include/asm-generic/pci-dma-compat.h  |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

index 2112a00..fea2c69 100644
@@ -274,6 +274,8 @@ DMA
 PCI
   pcim_enable_device()	: after success, all PCI ops become managed
   pcim_pin_device()	: keep PCI device enabled after release
+  pcim_map_single()
+  pcim_unmap_single()
 
 IOMAP
   devm_ioport_map()

index 1437b7d..796a892 100644
@@ -113,4 +113,22 @@ static inline int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask)
 }
 #endif
 
+/*
+ * Managed DMA API
+ */
+
+static inline int
+pcim_map_single(struct pci_dev *hwdev, void *ptr, size_t size, int direction,
+		dma_addr_t *ret_dma_handle)
+{
+	return dmam_map_single(hwdev == NULL ? NULL : &hwdev->dev, ptr, size, (enum dma_data_direction)direction, ret_dma_handle);
+}
+
+static inline void
+pcim_unmap_single(struct pci_dev *hwdev, dma_addr_t dma_addr,
+		 size_t size, int direction)
+{
+	dmam_unmap_single(hwdev == NULL ? NULL : &hwdev->dev, dma_addr, size, (enum dma_data_direction)direction);
+}
+
 #endif

1.7.2.3

Managed device resource API replaces code that reinvents it for memory allocation, page allocation and DMA mapping.

---
 drivers/staging/xillybus/xillybus.h      |   38 +------
 drivers/staging/xillybus/xillybus_core.c |  186 +++++++++---------------------
 drivers/staging/xillybus/xillybus_of.c   |   61 +---------
 drivers/staging/xillybus/xillybus_pcie.c |   54 ++--------
 4 files changed, 74 insertions(+), 265 deletions(-)

index 78a749a..d88e458 100644
@@ -25,33 +25,12 @@
 
 struct xilly_endpoint_hardware;
 
-struct xilly_page {
-	struct list_head node;
-	unsigned long addr;
-	unsigned int order;
-};
-
-struct xilly_dma {
-	struct list_head node;
-	struct pci_dev *pdev;
-	struct device *dev;
-	dma_addr_t dma_addr;
-	size_t size;
-	int direction;
-};
-
 struct xilly_buffer {
 	void *addr;
 	dma_addr_t dma_addr;
 	int end_offset; /* Counting elements, not bytes */
 };
 
-struct xilly_cleanup {
-	struct list_head to_kfree;
-	struct list_head to_pagefree;
-	struct list_head to_unmap;
-};
-
 struct xilly_idt_handle {
 	unsigned char *chandesc;
 	unsigned char *idt;

@@ -126,9 +105,6 @@ struct xilly_endpoint {
 	struct mutex register_mutex;
 	wait_queue_head_t ep_wait;
 
-	/* List of memory allocations, to make release easy */
-	struct xilly_cleanup cleanup;
-
 	/* Channels and message handling */
 	struct cdev cdev;
 

@@ -156,19 +132,15 @@ struct xilly_endpoint_hardware {
 				       dma_addr_t,
 				       size_t,
 				       int);
-	dma_addr_t (*map_single)(struct xilly_cleanup *,
-				 struct xilly_endpoint *,
-				 void *,
-				 size_t,
-				 int);
-	void (*unmap_single)(struct xilly_dma *entry);
+	int (*map_single)(struct xilly_endpoint *,
+			  void *,
+			  size_t,
+			  int,
+			  dma_addr_t *);
 };
 
 irqreturn_t xillybus_isr(int irq, void *data);
 
-void xillybus_do_cleanup(struct xilly_cleanup *mem,
-			 struct xilly_endpoint *endpoint);
-
 struct xilly_endpoint *xillybus_init_endpoint(struct pci_dev *pdev,
 					      struct device *dev,
 					      struct xilly_endpoint_hardware

1.7.2.3