[PATCH 0/5] devres: Add functions + migrate Xillybus driver

[PATCH 0/5] devres: Add functions + migrate Xillybus driver

[Eli Billauer]

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

[PATCH 1/5] devres: Add devm_get_free_pages API

[Eli Billauer]

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

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 Documentation/driver-model/devres.txt |    2 +
 drivers/base/devres.c                 |   76 +++++++++++++++++++++++++++++++++
 include/linux/device.h                |    4 ++
 3 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 4999518..e1a2707 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -237,6 +237,8 @@ MEM
   devm_kzalloc()
   devm_kfree()
   devm_kmemdup()
+  devm_get_free_pages()
+  devm_free_pages()
 
 IIO
   devm_iio_device_alloc()
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index d0914cb..5230294 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -852,3 +852,79 @@ void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
 	return p;
 }
 EXPORT_SYMBOL_GPL(devm_kmemdup);
+
+struct pages_devres {
+	unsigned long addr;
+	unsigned int order;
+};
+
+static int devm_pages_match(struct device *dev, void *res, void *p)
+{
+	struct pages_devres *devres = res;
+	struct pages_devres *target = p;
+
+	return devres->addr == target->addr;
+}
+
+static void devm_pages_release(struct device *dev, void *res)
+{
+	struct pages_devres *devres = res;
+
+	free_pages(devres->addr, devres->order);
+}
+
+/**
+ * devm_get_free_pages - Resource-managed __get_free_pages
+ * @dev: Device to allocate memory for
+ * @gfp_mask: Allocation gfp flags
+ * @order: Allocation size is (1 << order) pages
+ *
+ * Managed get_free_pages.  Memory allocated with this function is
+ * automatically freed on driver detach.
+ *
+ * RETURNS:
+ * Address of allocated memory on success, 0 on failure.
+ */
+
+unsigned long devm_get_free_pages(struct device *dev,
+				  gfp_t gfp_mask, unsigned int order)
+{
+	struct pages_devres *devres;
+	unsigned long addr;
+
+	addr = __get_free_pages(gfp_mask, order);
+
+	if (unlikely(!addr))
+		return 0;
+
+	devres = devres_alloc(devm_pages_release,
+			      sizeof(struct pages_devres), GFP_KERNEL);
+	if (unlikely(!devres)) {
+		free_pages(addr, order);
+		return 0;
+	}
+
+	devres->addr = addr;
+	devres->order = order;
+
+	devres_add(dev, devres);
+	return addr;
+}
+EXPORT_SYMBOL_GPL(devm_get_free_pages);
+
+/**
+ * devm_free_pages - Resource-managed free_pages
+ * @dev: Device this memory belongs to
+ * @addr: Memory to free
+ *
+ * Free memory allocated with devm_get_free_pages(). Unlike free_pages,
+ * there is no need to supply the @order.
+ */
+void devm_free_pages(struct device *dev, unsigned long addr)
+{
+	struct pages_devres devres = { .addr = addr };
+
+	WARN_ON(devres_release(dev, devm_pages_release, devm_pages_match,
+			       &devres));
+}
+EXPORT_SYMBOL_GPL(devm_free_pages);
diff --git a/include/linux/device.h b/include/linux/device.h
index ab87158..3dc69a2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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

[PATCH 2/5] dma-mapping: Add devm_ interface for dma_map_single()

[Eli Billauer]

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.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 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(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index e1a2707..13b8be0 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -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
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 0ce39a3..db1c496 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -19,6 +19,7 @@ struct dma_devres {
 	size_t		size;
 	void		*vaddr;
 	dma_addr_t	dma_handle;
+	enum dma_data_direction direction;
 };
 
 static void dmam_coherent_release(struct device *dev, void *res)
@@ -267,3 +268,82 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
 	return ret;
 }
 EXPORT_SYMBOL(dma_common_mmap);
+
+static int dmam_map_match(struct device *dev, void *res, void *match_data)
+{
+	struct dma_devres *this = res, *match = match_data;
+
+	if (this->dma_handle == match->dma_handle) {
+		WARN_ON(this->size != match->size ||
+			this->direction != match->direction);
+		return 1;
+	}
+	return 0;
+}
+
+static void dmam_map_single_release(struct device *dev, void *res)
+{
+	struct dma_devres *this = res;
+
+	dma_unmap_single(dev, this->dma_handle, this->size, this->direction);
+}
+
+/**
+ * dmam_map_single - Managed dma_map_single()
+ * @dev: Device to map DMA region for
+ * @ptr: Pointer to region
+ * @size: Size to map
+ * @direction: The mapping's direction
+ *
+ * Managed dma_map_single().  The region mapped using this
+ * function will be automatically unmapped on driver detach.
+ *
+ * RETURNS:
+ * The DMA handle of the mapped region upon success, 0 otherwise.
+ */
+dma_addr_t dmam_map_single(struct device *dev, void *ptr, size_t size,
+			   enum dma_data_direction direction)
+
+{
+	struct dma_devres *dr;
+	dma_addr_t dma_handle;
+
+	dr = devres_alloc(dmam_map_single_release, sizeof(*dr), GFP_KERNEL);
+	if (!dr)
+		return 0;
+
+	dma_handle = dma_map_single(dev, ptr, size, direction);
+	if (dma_mapping_error(dev, dma_handle)) {
+		devres_free(dr);
+		return 0;
+	}
+
+	dr->vaddr = ptr;
+	dr->dma_handle = dma_handle;
+	dr->size = size;
+	dr->direction = direction;
+
+	devres_add(dev, dr);
+
+	return dma_handle;
+}
+EXPORT_SYMBOL(dmam_map_single);
+
+/**
+ * dmam_unmap_single - Managed dma_unmap_single()
+ * @dev: Device to map DMA region for
+ * @dma_handle: DMA handle of the region to unmap
+ * @size: Size to unmap
+ * @direction: The mapping's direction
+ *
+ * Managed dma_unmap_single().
+ */
+void dmam_unmap_single(struct device *dev, dma_addr_t dma_handle,
+		       size_t size, enum dma_data_direction direction)
+{
+	struct dma_devres match_data = { size, NULL, dma_handle, direction };
+
+	WARN_ON(devres_release(dev, dmam_map_single_release, dmam_map_match,
+			       &match_data));
+}
+EXPORT_SYMBOL(dmam_unmap_single);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index fd4aee2..cdb14a8 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -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

[PATCH 3/5] dma-mapping: pci: Add devm_ interface for pci_map_single

[Eli Billauer]

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 Documentation/driver-model/devres.txt |    2 ++
 include/asm-generic/pci-dma-compat.h  |   17 +++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 13b8be0..09b03c9 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -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()
diff --git a/include/asm-generic/pci-dma-compat.h b/include/asm-generic/pci-dma-compat.h
index 1437b7d..444e598 100644
--- a/include/asm-generic/pci-dma-compat.h
+++ b/include/asm-generic/pci-dma-compat.h
@@ -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

[PATCH 4/5] staging: xillybus: Use devm_ API on probe and remove

[Eli Billauer]

Suggested-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 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(-)

diff --git a/drivers/staging/xillybus/xillybus.h b/drivers/staging/xillybus/xillybus.h
index e5e91d6..78a749a 100644
--- a/drivers/staging/xillybus/xillybus.h
+++ b/drivers/staging/xillybus/xillybus.h
@@ -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;
diff --git a/drivers/staging/xillybus/xillybus_core.c b/drivers/staging/xillybus/xillybus_core.c
index b0a6696..fe8f9d2 100644
--- a/drivers/staging/xillybus/xillybus_core.c
+++ b/drivers/staging/xillybus/xillybus_core.c
@@ -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;
diff --git a/drivers/staging/xillybus/xillybus_of.c b/drivers/staging/xillybus/xillybus_of.c
index 23a609b..46ea010 100644
--- a/drivers/staging/xillybus/xillybus_of.c
+++ b/drivers/staging/xillybus/xillybus_of.c
@@ -19,6 +19,7 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
+#include <linux/err.h>
 #include "xillybus.h"
 
 MODULE_DESCRIPTION("Xillybus driver for Open Firmware");
@@ -123,6 +124,7 @@ static int xilly_drv_probe(struct platform_device *op)
 	struct xilly_endpoint *endpoint;
 	int rc = 0;
 	int irq;
+	struct resource res;
 	struct xilly_endpoint_hardware *ephw = &of_hw;
 
 	if (of_property_read_bool(dev->of_node, "dma-coherent"))
@@ -135,38 +137,26 @@ static int xilly_drv_probe(struct platform_device *op)
 
 	dev_set_drvdata(dev, endpoint);
 
-	rc = of_address_to_resource(dev->of_node, 0, &endpoint->res);
+	rc = of_address_to_resource(dev->of_node, 0, &res);
 	if (rc) {
 		dev_warn(endpoint->dev,
 			 "Failed to obtain device tree resource\n");
-		goto failed_request_regions;
+		return rc;
 	}
 
-	if  (!request_mem_region(endpoint->res.start,
-				 resource_size(&endpoint->res), xillyname)) {
-		dev_err(endpoint->dev,
-			"request_mem_region failed. Aborting.\n");
-		rc = -EBUSY;
-		goto failed_request_regions;
-	}
+	endpoint->registers = devm_ioremap_resource(dev, &res);
 
-	endpoint->registers = of_iomap(dev->of_node, 0);
-	if (!endpoint->registers) {
-		dev_err(endpoint->dev,
-			"Failed to map I/O memory. Aborting.\n");
-		rc = -EIO;
-		goto failed_iomap0;
-	}
+	if (IS_ERR(endpoint->registers))
+		return PTR_ERR(endpoint->registers);
 
 	irq = irq_of_parse_and_map(dev->of_node, 0);
 
-	rc = request_irq(irq, xillybus_isr, 0, xillyname, endpoint);
+	rc = devm_request_irq(dev, irq, xillybus_isr, 0, xillyname, endpoint);
 
 	if (rc) {
 		dev_err(endpoint->dev,
 			"Failed to register IRQ handler. Aborting.\n");
-		rc = -ENODEV;
-		goto failed_register_irq;
+		return -ENODEV;
 	}
 
 	rc = xillybus_endpoint_discovery(endpoint);
@@ -174,18 +164,8 @@ static int xilly_drv_probe(struct platform_device *op)
 	if (!rc)
 		return 0;
 
-	free_irq(irq, endpoint);
-
-failed_register_irq:
-	iounmap(endpoint->registers);
-failed_iomap0:
-	release_mem_region(endpoint->res.start,
-			   resource_size(&endpoint->res));
-
-failed_request_regions:
 	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
 
-	kfree(endpoint);
 	return rc;
 }
 
@@ -193,20 +173,11 @@ static int xilly_drv_remove(struct platform_device *op)
 {
 	struct device *dev = &op->dev;
 	struct xilly_endpoint *endpoint = dev_get_drvdata(dev);
-	int irq = irq_of_parse_and_map(dev->of_node, 0);
 
 	xillybus_endpoint_remove(endpoint);
 
-	free_irq(irq, endpoint);
-
-	iounmap(endpoint->registers);
-	release_mem_region(endpoint->res.start,
-			   resource_size(&endpoint->res));
-
 	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
 
-	kfree(endpoint);
-
 	return 0;
 }
 
diff --git a/drivers/staging/xillybus/xillybus_pcie.c b/drivers/staging/xillybus/xillybus_pcie.c
index 51426d8..a4fe51c 100644
--- a/drivers/staging/xillybus/xillybus_pcie.c
+++ b/drivers/staging/xillybus/xillybus_pcie.c
@@ -141,38 +141,32 @@ static int xilly_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, endpoint);
 
-	rc = pci_enable_device(pdev);
-
-	/* L0s has caused packet drops. No power saving, thank you. */
-
-	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
+	rc = pcim_enable_device(pdev);
 
 	if (rc) {
 		dev_err(endpoint->dev,
-			"pci_enable_device() failed. Aborting.\n");
-		goto no_enable;
+			"pcim_enable_device() failed. Aborting.\n");
+		return rc;
 	}
 
+	/* L0s has caused packet drops. No power saving, thank you. */
+
+	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
+
 	if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
 		dev_err(endpoint->dev,
 			"Incorrect BAR configuration. Aborting.\n");
-		rc = -ENODEV;
-		goto bad_bar;
+		return -ENODEV;
 	}
 
-	rc = pci_request_regions(pdev, xillyname);
+	rc = pcim_iomap_regions(pdev, 0x01, xillyname);
 	if (rc) {
 		dev_err(endpoint->dev,
-			"pci_request_regions() failed. Aborting.\n");
-		goto failed_request_regions;
+			"pcim_iomap_regions() failed. Aborting.\n");
+		return rc;
 	}
 
-	endpoint->registers = pci_iomap(pdev, 0, 128);
-	if (!endpoint->registers) {
-		dev_err(endpoint->dev, "Failed to map BAR 0. Aborting.\n");
-		rc = -EIO;
-		goto failed_iomap0;
-	}
+	endpoint->registers = pcim_iomap_table(pdev)[0];
 
 	pci_set_master(pdev);
 
@@ -180,16 +174,15 @@ static int xilly_probe(struct pci_dev *pdev,
 	if (pci_enable_msi(pdev)) {
 		dev_err(endpoint->dev,
 			"Failed to enable MSI interrupts. Aborting.\n");
-		rc = -ENODEV;
-		goto failed_enable_msi;
+		return -ENODEV;
 	}
-	rc = request_irq(pdev->irq, xillybus_isr, 0, xillyname, endpoint);
+	rc = devm_request_irq(&pdev->dev, pdev->irq, xillybus_isr, 0,
+			      xillyname, endpoint);
 
 	if (rc) {
 		dev_err(endpoint->dev,
 			"Failed to register MSI handler. Aborting.\n");
-		rc = -ENODEV;
-		goto failed_register_msi;
+		return -ENODEV;
 	}
 
 	/*
@@ -203,8 +196,7 @@ static int xilly_probe(struct pci_dev *pdev,
 		endpoint->dma_using_dac = 0;
 	else {
 		dev_err(endpoint->dev, "Failed to set DMA mask. Aborting.\n");
-		rc = -ENODEV;
-		goto failed_dmamask;
+		return -ENODEV;
 	}
 
 	rc = xillybus_endpoint_discovery(endpoint);
@@ -212,22 +204,8 @@ static int xilly_probe(struct pci_dev *pdev,
 	if (!rc)
 		return 0;
 
-failed_dmamask:
-	free_irq(pdev->irq, endpoint);
-failed_register_msi:
-	pci_disable_msi(pdev);
-failed_enable_msi:
-	/* pci_clear_master(pdev); Nobody else seems to do this */
-	pci_iounmap(pdev, endpoint->registers);
-failed_iomap0:
-	pci_release_regions(pdev);
-failed_request_regions:
-bad_bar:
-	pci_disable_device(pdev);
-no_enable:
 	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
 
-	kfree(endpoint);
 	return rc;
 }
 
@@ -237,16 +215,7 @@ static void xilly_remove(struct pci_dev *pdev)
 
 	xillybus_endpoint_remove(endpoint);
 
-	free_irq(pdev->irq, endpoint);
-
-	pci_disable_msi(pdev);
-	pci_iounmap(pdev, endpoint->registers);
-	pci_release_regions(pdev);
-	pci_disable_device(pdev);
-
 	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
-
-	kfree(endpoint);
 }
 
 MODULE_DEVICE_TABLE(pci, xillyids);
-- 
1.7.2.3

[PATCH 5/5] staging: xillybus: Use devm_ API for memory allocation and DMA mapping

[Eli Billauer]

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

Suggested-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 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(-)

diff --git a/drivers/staging/xillybus/xillybus.h b/drivers/staging/xillybus/xillybus.h
index 78a749a..88bb6e5 100644
--- a/drivers/staging/xillybus/xillybus.h
+++ b/drivers/staging/xillybus/xillybus.h
@@ -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
diff --git a/drivers/staging/xillybus/xillybus_core.c b/drivers/staging/xillybus/xillybus_core.c
index fe8f9d2..ff78937 100644
--- a/drivers/staging/xillybus/xillybus_core.c
+++ b/drivers/staging/xillybus/xillybus_core.c
@@ -311,85 +311,14 @@ EXPORT_SYMBOL(xillybus_isr);
  * no locks are applied!
  */
 
-void xillybus_do_cleanup(struct xilly_cleanup *mem,
-			 struct xilly_endpoint *endpoint)
-{
-	struct list_head *this, *next;
-
-	list_for_each_safe(this, next, &mem->to_unmap) {
-		struct xilly_dma *entry =
-			list_entry(this, struct xilly_dma, node);
-
-		endpoint->ephw->unmap_single(entry);
-		kfree(entry);
-	}
-
-	INIT_LIST_HEAD(&mem->to_unmap);
-
-	list_for_each_safe(this, next, &mem->to_kfree)
-		kfree(this);
-
-	INIT_LIST_HEAD(&mem->to_kfree);
-
-	list_for_each_safe(this, next, &mem->to_pagefree) {
-		struct xilly_page *entry =
-			list_entry(this, struct xilly_page, node);
-
-		free_pages(entry->addr, entry->order);
-		kfree(entry);
-	}
-	INIT_LIST_HEAD(&mem->to_pagefree);
-}
-EXPORT_SYMBOL(xillybus_do_cleanup);
-
-static void *xilly_malloc(struct xilly_cleanup *mem, size_t size)
-{
-	void *ptr;
-
-	ptr = kzalloc(sizeof(struct list_head) + size, GFP_KERNEL);
-
-	if (!ptr)
-		return ptr;
-
-	list_add_tail((struct list_head *) ptr, &mem->to_kfree);
-
-	return ptr + sizeof(struct list_head);
-}
-
-static unsigned long xilly_pagealloc(struct xilly_cleanup *mem,
-				     unsigned long order)
-{
-	unsigned long addr;
-	struct xilly_page *this;
-
-	this = kmalloc(sizeof(struct xilly_page), GFP_KERNEL);
-	if (!this)
-		return 0;
-
-	addr =  __get_free_pages(GFP_KERNEL | __GFP_DMA32 | __GFP_ZERO, order);
-
-	if (!addr) {
-		kfree(this);
-		return 0;
-	}
-
-	this->addr = addr;
-	this->order = order;
-
-	list_add_tail(&this->node, &mem->to_pagefree);
-
-	return addr;
-}
-
-
 static void xillybus_autoflush(struct work_struct *work);
 
 static int xilly_setupchannels(struct xilly_endpoint *ep,
-			       struct xilly_cleanup *mem,
 			       unsigned char *chandesc,
 			       int entries
 	)
 {
+	struct device *dev = ep->dev;
 	int i, entry, wr_nbuffer, rd_nbuffer;
 	struct xilly_channel *channel;
 	int channelnum, bufnum, bufsize, format, is_writebuf;
@@ -402,17 +331,19 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 	int left_of_rd_salami = 0;
 	dma_addr_t dma_addr;
 	int msg_buf_done = 0;
+	const gfp_t gfp_mask = GFP_KERNEL | __GFP_DMA32 | __GFP_ZERO;
 
 	struct xilly_buffer *this_buffer = NULL; /* Init to silence warning */
 
-	channel = xilly_malloc(mem, ep->num_channels *
-			       sizeof(struct xilly_channel));
+	channel = devm_kzalloc(dev, ep->num_channels *
+			       sizeof(struct xilly_channel), GFP_KERNEL);
 
 	if (!channel)
 		goto memfail;
 
-	ep->channels = xilly_malloc(mem, (ep->num_channels + 1) *
-				    sizeof(struct xilly_channel *));
+	ep->channels = devm_kzalloc(dev, (ep->num_channels + 1) *
+				    sizeof(struct xilly_channel *),
+				    GFP_KERNEL);
 
 	if (!ep->channels)
 		goto memfail;
@@ -501,16 +432,16 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 			channel->rd_exclusive_open = exclusive_open;
 			channel->seekable = seekable;
 
-			channel->rd_buffers = xilly_malloc(
-				mem,
-				bufnum * sizeof(struct xilly_buffer *));
+			channel->rd_buffers = devm_kzalloc(dev,
+				bufnum * sizeof(struct xilly_buffer *),
+				GFP_KERNEL);
 
 			if (!channel->rd_buffers)
 				goto memfail;
 
-			this_buffer = xilly_malloc(
-				mem,
-				bufnum * sizeof(struct xilly_buffer));
+			this_buffer = devm_kzalloc(dev,
+				bufnum * sizeof(struct xilly_buffer),
+				GFP_KERNEL);
 
 			if (!this_buffer)
 				goto memfail;
@@ -530,16 +461,16 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 			channel->wr_synchronous = synchronous;
 			channel->wr_exclusive_open = exclusive_open;
 
-			channel->wr_buffers = xilly_malloc(
-				mem,
-				bufnum * sizeof(struct xilly_buffer *));
+			channel->wr_buffers = devm_kzalloc(dev,
+				bufnum * sizeof(struct xilly_buffer *),
+				GFP_KERNEL);
 
 			if (!channel->wr_buffers)
 				goto memfail;
 
-			this_buffer = xilly_malloc(
-				mem,
-				bufnum * sizeof(struct xilly_buffer));
+			this_buffer = devm_kzalloc(dev,
+				bufnum * sizeof(struct xilly_buffer),
+				GFP_KERNEL);
 
 			if (!this_buffer)
 				goto memfail;
@@ -576,15 +507,16 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 					}
 
 					wr_salami = (void *)
-						xilly_pagealloc(mem,
-								allocorder);
+						devm_get_free_pages(
+							dev, gfp_mask,
+							allocorder);
+
 					if (!wr_salami)
 						goto memfail;
 					left_of_wr_salami = allocsize;
 				}
 
 				dma_addr = ep->ephw->map_single(
-					mem,
 					ep,
 					wr_salami,
 					bytebufsize,
@@ -654,8 +586,8 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 					}
 
 					rd_salami = (void *)
-						xilly_pagealloc(
-							mem,
+						devm_get_free_pages(
+							dev, gfp_mask,
 							allocorder);
 
 					if (!rd_salami)
@@ -664,7 +596,6 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 				}
 
 				dma_addr = ep->ephw->map_single(
-					mem,
 					ep,
 					rd_salami,
 					bytebufsize,
@@ -2103,9 +2034,6 @@ struct xilly_endpoint *xillybus_init_endpoint(struct pci_dev *pdev,
 	endpoint->pdev = pdev;
 	endpoint->dev = dev;
 	endpoint->ephw = ephw;
-	INIT_LIST_HEAD(&endpoint->cleanup.to_kfree);
-	INIT_LIST_HEAD(&endpoint->cleanup.to_pagefree);
-	INIT_LIST_HEAD(&endpoint->cleanup.to_unmap);
 	endpoint->msg_counter = 0x0b;
 	endpoint->failed_messages = 0;
 	endpoint->fatal_error = 0;
@@ -2131,7 +2059,7 @@ static int xilly_quiesce(struct xilly_endpoint *endpoint)
 
 	if (endpoint->idtlen < 0) {
 		dev_err(endpoint->dev,
-			"Failed to quiesce the device on exit. Quitting while leaving a mess.\n");
+			"Failed to quiesce the device on exit.\n");
 		return -ENODEV;
 	}
 	return 0; /* Success */
@@ -2141,8 +2069,9 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 {
 	int rc = 0;
 
-	struct xilly_cleanup tmpmem;
+	void *bootstrap_resources;
 	int idtbuffersize = (1 << PAGE_SHIFT);
+	struct device *dev = endpoint->dev;
 
 	/*
 	 * The bogus IDT is used during bootstrap for allocating the initial
@@ -2155,10 +2084,6 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 				       3, 192, PAGE_SHIFT, 0 };
 	struct xilly_idt_handle idt_handle;
 
-	INIT_LIST_HEAD(&tmpmem.to_kfree);
-	INIT_LIST_HEAD(&tmpmem.to_pagefree);
-	INIT_LIST_HEAD(&tmpmem.to_unmap);
-
 	/*
 	 * Writing the value 0x00000001 to Endianness register signals which
 	 * endianness this processor is using, so the FPGA can swap words as
@@ -2170,12 +2095,16 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 
 	/* Bootstrap phase I: Allocate temporary message buffer */
 
+	bootstrap_resources = devres_open_group(dev, NULL, GFP_KERNEL);
+	if (!bootstrap_resources)
+		return -ENOMEM;
+
 	endpoint->num_channels = 0;
 
-	rc = xilly_setupchannels(endpoint, &tmpmem, bogus_idt, 1);
+	rc = xilly_setupchannels(endpoint, bogus_idt, 1);
 
 	if (rc)
-		goto failed_buffers;
+		return rc;
 
 	/* Clear the message subsystem (and counter in particular) */
 	iowrite32(0x04, &endpoint->registers[fpga_msg_ctrl_reg]);
@@ -2199,8 +2128,7 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 
 	if (endpoint->idtlen < 0) {
 		dev_err(endpoint->dev, "No response from FPGA. Aborting.\n");
-		rc = -ENODEV;
-		goto failed_quiesce;
+		return -ENODEV;
 	}
 
 	/* Enable DMA */
@@ -2216,7 +2144,7 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 
 	endpoint->num_channels = 1;
 
-	rc = xilly_setupchannels(endpoint, &tmpmem, bogus_idt, 2);
+	rc = xilly_setupchannels(endpoint, bogus_idt, 2);
 
 	if (rc)
 		goto failed_idt;
@@ -2234,10 +2162,12 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 		rc = -ENODEV;
 		goto failed_idt;
 	}
+
+	devres_close_group(dev, bootstrap_resources);
+
 	/* Bootstrap phase III: Allocate buffers according to IDT */
 
 	rc = xilly_setupchannels(endpoint,
-				 &endpoint->cleanup,
 				 idt_handle.chandesc,
 				 idt_handle.entries);
 
@@ -2260,7 +2190,7 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	if (rc)
 		goto failed_chrdevs;
 
-	xillybus_do_cleanup(&tmpmem, endpoint);
+	devres_release_group(dev, bootstrap_resources);
 
 	return 0;
 
@@ -2270,16 +2200,8 @@ failed_chrdevs:
 	mutex_unlock(&ep_list_lock);
 
 failed_idt:
-	/* Quiesce the device. Now it's serious to do it */
-	rc = xilly_quiesce(endpoint);
-
-	if (rc)
-		return rc; /* FPGA may still DMA, so no release */
-
+	xilly_quiesce(endpoint);
 	flush_workqueue(xillybus_wq);
-failed_quiesce:
-failed_buffers:
-	xillybus_do_cleanup(&tmpmem, endpoint);
 
 	return rc;
 }
diff --git a/drivers/staging/xillybus/xillybus_of.c b/drivers/staging/xillybus/xillybus_of.c
index 46ea010..0e4a67c 100644
--- a/drivers/staging/xillybus/xillybus_of.c
+++ b/drivers/staging/xillybus/xillybus_of.c
@@ -62,44 +62,13 @@ static void xilly_dma_sync_single_nop(struct xilly_endpoint *ep,
 {
 }
 
-static dma_addr_t xilly_map_single_of(struct xilly_cleanup *mem,
-				      struct xilly_endpoint *ep,
+static dma_addr_t xilly_map_single_of(struct xilly_endpoint *ep,
 				      void *ptr,
 				      size_t size,
 				      int direction
 	)
 {
-
-	dma_addr_t addr = 0;
-	struct xilly_dma *this;
-
-	this = kmalloc(sizeof(struct xilly_dma), GFP_KERNEL);
-	if (!this)
-		return 0;
-
-	addr = dma_map_single(ep->dev, ptr, size, direction);
-	this->direction = direction;
-
-	if (dma_mapping_error(ep->dev, addr)) {
-		kfree(this);
-		return 0;
-	}
-
-	this->dma_addr = addr;
-	this->dev = ep->dev;
-	this->size = size;
-
-	list_add_tail(&this->node, &mem->to_unmap);
-
-	return addr;
-}
-
-static void xilly_unmap_single_of(struct xilly_dma *entry)
-{
-	dma_unmap_single(entry->dev,
-			 entry->dma_addr,
-			 entry->size,
-			 entry->direction);
+	return dmam_map_single(ep->dev, ptr, size, direction);
 }
 
 static struct xilly_endpoint_hardware of_hw = {
@@ -107,7 +76,6 @@ static struct xilly_endpoint_hardware of_hw = {
 	.hw_sync_sgl_for_cpu = xilly_dma_sync_single_for_cpu_of,
 	.hw_sync_sgl_for_device = xilly_dma_sync_single_for_device_of,
 	.map_single = xilly_map_single_of,
-	.unmap_single = xilly_unmap_single_of
 };
 
 static struct xilly_endpoint_hardware of_hw_coherent = {
@@ -115,7 +83,6 @@ static struct xilly_endpoint_hardware of_hw_coherent = {
 	.hw_sync_sgl_for_cpu = xilly_dma_sync_single_nop,
 	.hw_sync_sgl_for_device = xilly_dma_sync_single_nop,
 	.map_single = xilly_map_single_of,
-	.unmap_single = xilly_unmap_single_of
 };
 
 static int xilly_drv_probe(struct platform_device *op)
@@ -138,12 +105,6 @@ static int xilly_drv_probe(struct platform_device *op)
 	dev_set_drvdata(dev, endpoint);
 
 	rc = of_address_to_resource(dev->of_node, 0, &res);
-	if (rc) {
-		dev_warn(endpoint->dev,
-			 "Failed to obtain device tree resource\n");
-		return rc;
-	}
-
 	endpoint->registers = devm_ioremap_resource(dev, &res);
 
 	if (IS_ERR(endpoint->registers))
@@ -159,14 +120,7 @@ static int xilly_drv_probe(struct platform_device *op)
 		return -ENODEV;
 	}
 
-	rc = xillybus_endpoint_discovery(endpoint);
-
-	if (!rc)
-		return 0;
-
-	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
-
-	return rc;
+	return xillybus_endpoint_discovery(endpoint);
 }
 
 static int xilly_drv_remove(struct platform_device *op)
@@ -176,8 +130,6 @@ static int xilly_drv_remove(struct platform_device *op)
 
 	xillybus_endpoint_remove(endpoint);
 
-	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
-
 	return 0;
 }
 
diff --git a/drivers/staging/xillybus/xillybus_pcie.c b/drivers/staging/xillybus/xillybus_pcie.c
index a4fe51c..a7d60c7 100644
--- a/drivers/staging/xillybus/xillybus_pcie.c
+++ b/drivers/staging/xillybus/xillybus_pcie.c
@@ -78,46 +78,16 @@ static void xilly_dma_sync_single_for_device_pci(struct xilly_endpoint *ep,
  * but that can change.
  */
 
-static dma_addr_t xilly_map_single_pci(struct xilly_cleanup *mem,
-				       struct xilly_endpoint *ep,
+static dma_addr_t xilly_map_single_pci(struct xilly_endpoint *ep,
 				       void *ptr,
 				       size_t size,
 				       int direction
 	)
 {
-
-	dma_addr_t addr = 0;
-	struct xilly_dma *this;
 	int pci_direction;
 
-	this = kmalloc(sizeof(struct xilly_dma), GFP_KERNEL);
-	if (!this)
-		return 0;
-
 	pci_direction = xilly_pci_direction(direction);
-	addr = pci_map_single(ep->pdev, ptr, size, pci_direction);
-	this->direction = pci_direction;
-
-	if (pci_dma_mapping_error(ep->pdev, addr)) {
-		kfree(this);
-		return 0;
-	}
-
-	this->dma_addr = addr;
-	this->pdev = ep->pdev;
-	this->size = size;
-
-	list_add_tail(&this->node, &mem->to_unmap);
-
-	return addr;
-}
-
-static void xilly_unmap_single_pci(struct xilly_dma *entry)
-{
-	pci_unmap_single(entry->pdev,
-			 entry->dma_addr,
-			 entry->size,
-			 entry->direction);
+	return pcim_map_single(ep->pdev, ptr, size, pci_direction);
 }
 
 static struct xilly_endpoint_hardware pci_hw = {
@@ -125,7 +95,6 @@ static struct xilly_endpoint_hardware pci_hw = {
 	.hw_sync_sgl_for_cpu = xilly_dma_sync_single_for_cpu_pci,
 	.hw_sync_sgl_for_device = xilly_dma_sync_single_for_device_pci,
 	.map_single = xilly_map_single_pci,
-	.unmap_single = xilly_unmap_single_pci
 };
 
 static int xilly_probe(struct pci_dev *pdev,
@@ -199,14 +168,7 @@ static int xilly_probe(struct pci_dev *pdev,
 		return -ENODEV;
 	}
 
-	rc = xillybus_endpoint_discovery(endpoint);
-
-	if (!rc)
-		return 0;
-
-	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
-
-	return rc;
+	return xillybus_endpoint_discovery(endpoint);
 }
 
 static void xilly_remove(struct pci_dev *pdev)
@@ -214,8 +176,6 @@ static void xilly_remove(struct pci_dev *pdev)
 	struct xilly_endpoint *endpoint = pci_get_drvdata(pdev);
 
 	xillybus_endpoint_remove(endpoint);
-
-	xillybus_do_cleanup(&endpoint->cleanup, endpoint);
 }
 
 MODULE_DEVICE_TABLE(pci, xillyids);
-- 
1.7.2.3

Re: [PATCH 1/5] devres: Add devm_get_free_pages API

[Tejun Heo]

On Fri, May 16, 2014 at 11:26:35AM +0300, Eli Billauer wrote:
> devm_get_free_pages() and devm_free_pages() are the managed counterparts
> for __get_free_pages() and free_pages().
> 
> Signed-off-by: Eli Billauer <eli.billauer@gmail.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 2/5] dma-mapping: Add devm_ interface for dma_map_single()

[Tejun Heo]

Hello,

On Fri, May 16, 2014 at 11:26:36AM +0300, Eli Billauer wrote:
> +dma_addr_t dmam_map_single(struct device *dev, void *ptr, size_t size,
> +			   enum dma_data_direction direction)
> +
> +{
> +	struct dma_devres *dr;
> +	dma_addr_t dma_handle;
> +
> +	dr = devres_alloc(dmam_map_single_release, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return 0;
> +
> +	dma_handle = dma_map_single(dev, ptr, size, direction);

Don't we wanna map the underlying operation - dma_map_single_attrs() -
instead?

> +	if (dma_mapping_error(dev, dma_handle)) {
> +		devres_free(dr);
> +		return 0;

Can't we just keep returning dma_handle?  Even if that means invoking
->mapping_error() twice?  It's yucky to have subtly different error
return especially because in most cases it won't fail.

Thanks.

-- 
tejun

Re: [PATCH 3/5] dma-mapping: pci: Add devm_ interface for pci_map_single

[Tejun Heo]

On Fri, May 16, 2014 at 11:26:37AM +0300, Eli Billauer wrote:
> 
> Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
> ---
>  Documentation/driver-model/devres.txt |    2 ++
>  include/asm-generic/pci-dma-compat.h  |   17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)

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

Thanks.

-- 
tejun

Re: [PATCH 2/5] dma-mapping: Add devm_ interface for dma_map_single()

[Eli Billauer]

Hello Tejun,

On 17/05/14 00:08, Tejun Heo wrote:
>
> Don't we wanna map the underlying operation - dma_map_single_attrs() -
> instead?
>    

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.

>    
>> +	if (dma_mapping_error(dev, dma_handle)) {
>> +		devres_free(dr);
>> +		return 0;
>>      
> Can't we just keep returning dma_handle?  Even if that means invoking
> ->mapping_error() twice?  It's yucky to have subtly different error
> return especially because in most cases it won't fail.
>    
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

Re: [PATCH 2/5] dma-mapping: Add devm_ interface for dma_map_single()

[Tejun Heo]

Hello, Eli.

On Sat, May 17, 2014 at 03:19:21PM +0300, Eli Billauer wrote:
> >>+	if (dma_mapping_error(dev, dma_handle)) {
> >>+		devres_free(dr);
> >>+		return 0;
> >Can't we just keep returning dma_handle?  Even if that means invoking
> >->mapping_error() twice?  It's yucky to have subtly different error
> >return especially because in most cases it won't fail.
> 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.

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

Re: [PATCH 2/5] dma-mapping: Add devm_ interface for dma_map_single()

[Eli Billauer]

Hello, Tejun.

On 19/05/14 23:17, Tejun Heo wrote:
>
> 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?
>    
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


> Thanks.
>
>    

Re: [PATCH 2/5] dma-mapping: Add devm_ interface for dma_map_single()

[Tejun Heo]

Hello,

On Tue, May 20, 2014 at 08:54:09AM +0300, Eli Billauer wrote:
> 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?

Ah, okay.  No idea.

> 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?

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