devres: Add functions + migrate Xillybus driver

This patchset consists of new functions to the managed device resource API, followed by patches for the Xillybus driver that now relies on these.

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

* devm_get_free_pages()
* devm_free_pages()
* dmam_map_single()
* dmam_unmap_single()
* pcim_map_single()
* pcim_unmap_single()

After applying these patches, the Xillybus driver uses all six functions, and has been hardware tested on arm and x86_32. A compilation check went cleanly on x86_64 as well.

Dependencies:
Patch #3 relies on patch #2 (quite obviously).
Patch #5 relies on all previous patches.

Thanks,
Eli

Eli Billauer (5):
devres: Add devm_get_free_pages API
dma-mapping: Add devm_ interface for dma_map_single()
dma-mapping: pci: Add devm_ interface for pci_map_single
staging: xillybus: Use devm_ API on probe and remove
staging: xillybus: Use devm_ API for memory allocation and DMA
mapping

 Documentation/driver-model/devres.txt    |    6 +
 drivers/base/devres.c                    |   76 ++++++++++++++
 drivers/base/dma-mapping.c               |   80 +++++++++++++++
 drivers/staging/xillybus/xillybus.h      |   32 +------
 drivers/staging/xillybus/xillybus_core.c |  162 ++++++++----------------------
 drivers/staging/xillybus/xillybus_of.c   |   99 ++----------------
 drivers/staging/xillybus/xillybus_pcie.c |  111 ++++-----------------
 include/asm-generic/pci-dma-compat.h     |   17 +++
 include/linux/device.h                   |    4 +
 include/linux/dma-mapping.h              |    5 +-
 10 files changed, 261 insertions(+), 331 deletions(-)

1.7.2.3

devm_get_free_pages() and devm_free_pages() are the managed counterparts for __get_free_pages() and free_pages().

---
 Documentation/driver-model/devres.txt |    2 +
 drivers/base/devres.c                 |   76 +++++++++++++++++++++++++++++++++
 include/linux/device.h                |    4 ++
 3 files changed, 82 insertions(+), 0 deletions(-)

index 4999518..e1a2707 100644
@@ -237,6 +237,8 @@ MEM
   devm_kzalloc()
   devm_kfree()
   devm_kmemdup()
+  devm_get_free_pages()
+  devm_free_pages()
 
 IIO
   devm_iio_device_alloc()

index ab87158..3dc69a2 100644
@@ -626,6 +626,10 @@ extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp);
 extern void *devm_kmemdup(struct device *dev, const void *src, size_t len,
 			  gfp_t gfp);
 
+extern unsigned long devm_get_free_pages(struct device *dev,
+					 gfp_t gfp_mask, unsigned int order);
+extern void devm_free_pages(struct device *dev, unsigned long addr);
+
 void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
 void __iomem *devm_request_and_ioremap(struct device *dev,
 			struct resource *res);

1.7.2.3

Thanks.

tejun

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

Note that dmam_map_single() returns zero on failure, and not a value to be handled by dma_mapping_error(): The error check is done by dmam_map_single() to avoid the registration of a mapping that failed.

---
 Documentation/driver-model/devres.txt |    2 +
 drivers/base/dma-mapping.c            |   80 +++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h           |    5 ++-
 3 files changed, 86 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..cdb14a8 100644
@@ -233,7 +233,10 @@ static inline void dmam_release_declared_memory(struct device *dev)
 {
 }
 #endif /* ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY */
-
+dma_addr_t dmam_map_single(struct device *dev, void *ptr, size_t size,
+			   enum dma_data_direction direction);
+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

Hello,

tejun

Hello Tejun,

I'll resubmit this patch promptly, with a follow-up patch for the diff to implement dmam_map_single_attrs() instead. Plus a define-statement for dmam_map_single(). I can't test the case of a non-NULL value for @attrs however.

Yucky it is indeed. There are however two problems with keeping the existing API:

* What to do if devres_alloc() fails. How do I signal back an error? The only way I can think of is returning zero. But if the caller should know that zero means failure, I've already broken the API. I might as well return zero for any kind of failure.
* It seems like a lot of dma_mapping_error() implementations always return no-error, since the DMA mapping can't fail on specific architectures. If callers use dma_mapping_error(), the possible devres_alloc() failure will be missed.

By the way, where I've seen dma_mapping_error() doing something, it checks for dma_handle == 0.

Submitting updated patches for the DMA mapping part soon.

Regards,
Eli

Hello, Eli.

What can't it just do the following?

	if (dma_mapping_error(dev, dma_handle)) {
		devres_free(dr);
		return dma_handle;
	}

The caller would have to invoke dma_mapping_error() again but is that a problem?

Thanks.

tejun

Hello, Tejun.

That seems OK to me, but the problem I'm concerned with is this: In devm_get_free_pages() it says

    devres = devres_alloc(devm_pages_release,
                  sizeof(struct pages_devres), GFP_KERNEL);
    if (unlikely(!devres)) {
        free_pages(addr, order);
        return 0;
    }

What should I put instead of this "return 0" to conform with the current API?

And to make things even worse, on some architectures, dma_mapping_error() always returns success, regardless of dma_handle. So if we stick to the current API, the caller of devm_get_free_pages() will never know it failed on these architectures.

So my conclusion was that the caller must be aware that if devm_get_free_pages() returns zero it's an error, regardless of dma_mapping_error(). That breaks the API. Once it's broken, why not return zero on all possible errors?

Maybe I didn't understand what you suggested.

Regards,
Eli

Hello,

Ah, okay. No idea.

What if 0 is a valid return for the architecture? Isn't that the whole point of dma_mapping_error() interface? Maybe the right thing to do is using a separate out argument for dma_handle?

	int dmam_map_single(blah blah, dma_addr_t *out_addr);

So that it can return -errno like other non-crazy functions? We'll probably ask somebody who knows this code better.

Thanks.

tejun

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

index 13b8be0..09b03c9 100644
@@ -272,6 +272,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..444e598 100644
@@ -113,4 +113,21 @@ static inline int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask)
 }
 #endif
 
+/*
+ * Managed DMA API
+ */
+
+static inline dma_addr_t
+pcim_map_single(struct pci_dev *hwdev, void *ptr, size_t size, int direction)
+{
+	return dmam_map_single(hwdev == NULL ? NULL : &hwdev->dev, ptr, size, (enum dma_data_direction)direction);
+}
+
+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

The patch looks fine to me but can you please cc PCI subsystem maintainer and please do so for other patches too.

Thanks.

tejun

---
 drivers/staging/xillybus/xillybus.h      |    1 -
 drivers/staging/xillybus/xillybus_core.c |    2 +-
 drivers/staging/xillybus/xillybus_of.c   |   47 ++++-----------------
 drivers/staging/xillybus/xillybus_pcie.c |   65 ++++++++----------------------
 4 files changed, 27 insertions(+), 88 deletions(-)

index e5e91d6..78a749a 100644
@@ -116,7 +116,6 @@ struct xilly_endpoint {
 	 */
 	struct pci_dev *pdev;
 	struct device *dev;
-	struct resource res; /* OF devices only */
 	struct xilly_endpoint_hardware *ephw;
 
 	struct list_head ep_list;

index b0a6696..fe8f9d2 100644
@@ -2094,7 +2094,7 @@ struct xilly_endpoint *xillybus_init_endpoint(struct pci_dev *pdev,
 {
 	struct xilly_endpoint *endpoint;
 
-	endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL);
+	endpoint = devm_kzalloc(dev, sizeof(*endpoint), GFP_KERNEL);
 	if (!endpoint) {
 		dev_err(dev, "Failed to allocate memory. Aborting.\n");
 		return NULL;

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      |   31 +------
 drivers/staging/xillybus/xillybus_core.c |  160 ++++++++----------------------
 drivers/staging/xillybus/xillybus_of.c   |   54 +----------
 drivers/staging/xillybus/xillybus_pcie.c |   46 +--------
 4 files changed, 48 insertions(+), 243 deletions(-)

index 78a749a..88bb6e5 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,14 @@ struct xilly_endpoint_hardware {
 				       dma_addr_t,
 				       size_t,
 				       int);
-	dma_addr_t (*map_single)(struct xilly_cleanup *,
-				 struct xilly_endpoint *,
+	dma_addr_t (*map_single)(struct xilly_endpoint *,
 				 void *,
 				 size_t,
 				 int);
-	void (*unmap_single)(struct xilly_dma *entry);
 };
 
 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