[PATCH 0/2] staging: ion: Add ion-physmem driver

[PATCH 0/2] staging: ion: Add ion-physmem driver

[Andrew Andrianov]

Please do not merge the following patchset yet, it's only a very early 
proof-of-concept and it needs cleanup, discussion and advice.
Since I'm quite new to ION, so I may be missing a few vital details.

The following ion driver implements a simple way to define ION heaps from a 
devicetree description. e.g.

>From on-chip SRAM:
                ion_im0: ion@00100000 {
                     compatible = "ion,physmem";
                     reg = <0x00100000 0x40000>;
                     reg-names = "memory";
                     ion-heap-id   = <2>;
                     ion-heap-type = <ION_HEAP_TYPE_DMA>;
                     ion-heap-align = <0x10>;
                     ion-heap-name = "IM0";
                };

or 
		ion_crv: ion@deadbeef {
		     compatible = "ion,physmem";
		     reg = <0x00000000 0x100000>;
		     reg-names = "memory";
		     ion-heap-id   = <3>;
		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
		     ion-heap-align = <0x10>;
		     ion-heap-name = "carveout";
		};

This way you can define any ION heap, so it pretty much supersedes the dummy 
driver that has everything hardcoded and is good for nothing but tests.
For ION_HEAP_TYPE_DMA, if 'reg' range is present in devicetree it does a 
proper declare_dma_coherent_memory() and parses reg field as a physical address 
range. E.g. this way you can pass on-chip SRAM or dedicated RAM banks
to ION to be used as a heap.
If reg is not present - behavior is identical to a DMA heap of ion_dummy driver.
 
For carveout and chunk heaps it behaves just like the dummy driver, allocating 
some free pages as a heap and freeing them when unloaded. reg range is used 
for size calculations via resource_size() only. 

For system/system_contig things stay pretty much the same, except for it 
doesn't do any page allocations by itself and just passes all
parameters down to ION

My use case: 
The SoC I'm making this for is K1879XB1YA (К1879ХБ1Я) / MB77.07:
Product page: http://www.module.ru/en/catalog/micro/micro_pc/
Hopefully I'll get basic support for this SoC ready for submission by
the next merge window. 
It has dedicated 128MiB 'multimedia' memory that ARM core can't execute
code from, but can write/read to and several huge (256KiB) banks of
on-chip SRAM. All mapped into physical address space.  
ION's job will be pretty much allocating from those banks, each making up it's 
own heap and sharing the buffers between DSP, ARM and a few multimedia devices. 

Remaining questions about this driver:
* Should this ION driver take care to optionally enable/disable clocks the way 
drivers/misc/sram.c does?
* Not sure if try_module_get/module_put magic is really needed there and used 
properly. Building as a module and unloading has NOT yet been tested, is on my 
TODO list.


Regards, 
Andrew

Andrew Andrianov (2):
  staging: ion: Add ion-physmem driver
  staging: ion: Add ion-physmem documentation

 Documentation/devicetree/bindings/ion,physmem.txt |   80 +++++++
 drivers/staging/android/ion/Kconfig               |    7 +
 drivers/staging/android/ion/Makefile              |    5 +-
 drivers/staging/android/ion/ion_physmem.c         |  237 +++++++++++++++++++++
 4 files changed, 327 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
 create mode 100644 drivers/staging/android/ion/ion_physmem.c

-- 
1.7.10.4

[PATCH 1/2] staging: ion: Add ion-physmem driver

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 drivers/staging/android/ion/Kconfig       |    7 +
 drivers/staging/android/ion/Makefile      |    5 +-
 drivers/staging/android/ion/ion_physmem.c |  237 +++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion_physmem.c

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 3452346..c558cf8 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -33,3 +33,10 @@ config ION_TEGRA
 	help
 	  Choose this option if you wish to use ion on an nVidia Tegra.
 
+config ION_PHYSMEM
+	bool "Generic PhysMem ION driver"
+	depends on ION
+	help
+	  Provides a generic ION driver that registers the
+	  /dev/ion device with heaps from devicetree entries.
+	  This heaps are made of chunks of physical memory
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index b56fd2b..ac9856d 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT
 obj-$(CONFIG_ION) += compat_ion.o
 endif
 
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
 
diff --git a/drivers/staging/android/ion/ion_physmem.c b/drivers/staging/android/ion/ion_physmem.c
new file mode 100644
index 0000000..e922d74
--- /dev/null
+++ b/drivers/staging/android/ion/ion_physmem.c
@@ -0,0 +1,237 @@
+/*
+ * Copyright (C) 2015 RC Module
+ * Andrew Andrianov <andrew@ncrmnt.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Generic devicetree physmem driver for ION Memory Manager
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include "ion.h"
+#include "ion_priv.h"
+
+#define DRVNAME "ion-physmem"
+
+static struct ion_device *idev;
+
+static const struct of_device_id of_match_table[] = {
+	{ .compatible = "ion,physmem", },
+	{ /* end of list */ }
+};
+MODULE_DEVICE_TABLE(of, of_match_table);
+
+/*
+#define PHYSMAP_ION_DEBUG y
+*/
+
+struct physmem_ion_dev {
+	struct ion_platform_heap  data;
+	struct ion_heap          *heap;
+	int                       need_free_coherent;
+	void                     *freepage_ptr;
+};
+
+static long ion_physmem_custom_ioctl(struct ion_client *client,
+				     unsigned int cmd,
+				     unsigned long arg)
+{
+/* Just an ugly debugging hack. */
+#ifdef PHYSMAP_ION_DEBUG
+	if ((cmd == 0xff)) {
+		ion_phys_addr_t addr;
+		size_t len;
+		struct ion_handle *hndl;
+
+		printk(KERN_INFO "=== ION PHYSMEM DEBUG PRINTOUT ===\n");
+		printk(KERN_INFO "N.B. DO disable CONFIG_ION_PHYSMEM_DEBUG in production\n");
+		printk(KERN_INFO "Shared fd is %d\n", (int) arg);
+		hndl = ion_import_dma_buf(client, arg);
+		if (!hndl) {
+			printk(KERN_INFO "Failed to import shared descriptor.\n");
+			return 0;
+		}
+		ion_phys(client, hndl, &addr, &len);
+		printk(KERN_INFO "shared buffer: phys 0x%lx len %ul\n",
+		       addr, len);
+		printk(KERN_INFO "=== ION PHYSMEM DEBUG PRINTOUT ===\n");
+	}
+#endif
+	return 0;
+}
+
+static int ion_physmem_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 ion_heap_id, ion_heap_align, ion_heap_type;
+	ion_phys_addr_t addr;
+	size_t size = 0;
+	const char *ion_heap_name;
+	struct resource *res;
+	struct physmem_ion_dev *ipdev;
+
+	/*
+	   Looks like we can only have one ION device in our system.
+	   Therefore we call ion_device_create on first probe and only
+	   add heaps to it on subsequent probe calls.
+	   FixMe: Do we need to hold a spinlock here once device probing
+	   becomes async?
+	*/
+
+	if (!idev) {
+		idev = ion_device_create(ion_physmem_custom_ioctl);
+		dev_info(&pdev->dev, "ion-physmem: ION PhysMem Driver. (c) RC Module 2015\n");
+		if (!idev)
+			return -ENOMEM;
+	}
+
+	ipdev = kzalloc(sizeof(struct physmem_ion_dev), GFP_KERNEL);
+	if (!ipdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipdev);
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+				    &ion_heap_id);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+				    &ion_heap_type);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+				    &ion_heap_align);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+				       &ion_heap_name);
+	if (ret != 0)
+		goto errfreeipdev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	/* Not always needed, throw no error if missing */
+	if (res) {
+		/* Fill in some defaults */
+		addr = res->start;
+		size = resource_size(res);
+	}
+
+	switch (ion_heap_type) {
+	case ION_HEAP_TYPE_DMA:
+		if (res) {
+			ret = dma_declare_coherent_memory(&pdev->dev,
+							  res->start,
+							  res->start,
+							  resource_size(res),
+							  DMA_MEMORY_MAP |
+							  DMA_MEMORY_EXCLUSIVE);
+			if (ret == 0) {
+				ret = -ENODEV;
+				goto errfreeipdev;
+			}
+		}
+		/*
+		 *  If no memory region declared in dt we assume that
+		 *  the user is okay with plain dma_alloc_coherent.
+		 */
+		break;
+	case ION_HEAP_TYPE_CARVEOUT:
+	case ION_HEAP_TYPE_CHUNK:
+	{
+		if (size == 0) {
+			ret = -EIO;
+			goto errfreeipdev;
+		}
+		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
+		if (ipdev->freepage_ptr) {
+			addr = virt_to_phys(ipdev->freepage_ptr);
+		} else {
+			ret = -ENOMEM;
+			goto errfreeipdev;
+		}
+		break;
+	}
+	}
+
+	ipdev->data.id    = ion_heap_id;
+	ipdev->data.type  = ion_heap_type;
+	ipdev->data.name  = ion_heap_name;
+	ipdev->data.align = ion_heap_align;
+	ipdev->data.base  = addr;
+	ipdev->data.size  = size;
+	/* This one make dma_declare_coherent_memory actually work */
+	ipdev->data.priv  = &pdev->dev;
+
+
+	ipdev->heap = ion_heap_create(&ipdev->data);
+	if (!ipdev->heap)
+		goto errfreeipdev;
+
+	if (!try_module_get(THIS_MODULE))
+		goto errfreeheap;
+
+	ion_device_add_heap(idev, ipdev->heap);
+
+	dev_info(&pdev->dev, "ion-physmem: heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
+	       ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
+	       (unsigned long int) addr, ((unsigned long int) size / 1024));
+
+	return 0;
+
+errfreeheap:
+	kfree(ipdev->heap);
+errfreeipdev:
+	kfree(ipdev);
+	return -ENOMEM;
+}
+
+static int ion_physmem_remove(struct platform_device *pdev)
+{
+	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
+
+	ion_heap_destroy(ipdev->heap);
+	if (ipdev->need_free_coherent)
+		dma_release_declared_memory(&pdev->dev);
+	if (ipdev->freepage_ptr)
+		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
+	kfree(ipdev->heap);
+	kfree(ipdev);
+	module_put(THIS_MODULE);
+	return 0;
+}
+
+static void __exit ion_physmem_exit(void)
+{
+	if (idev)
+		ion_device_destroy(idev);
+}
+__exitcall(ion_physmem_exit);
+
+static struct platform_driver ion_physmem_driver = {
+	.probe		= ion_physmem_probe,
+	.remove		= ion_physmem_remove,
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "ion-physmem",
+		.of_match_table = of_match_ptr(of_match_table),
+	},
+};
+module_platform_driver(ion_physmem_driver);
+
+MODULE_DESCRIPTION("Generic physmem driver for ION");
+MODULE_AUTHOR("Andrew Andrianov <andrew@ncrmnt.org>");
+MODULE_LICENSE("GPL");
-- 
1.7.10.4

[PATCH 2/2] staging: ion: Add ion-physmem documentation

[Andrew Andrianov]

Signed-off-by: Andrew Andrianov <andrew@ncrmnt.org>
---
 Documentation/devicetree/bindings/ion,physmem.txt |   80 +++++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt b/Documentation/devicetree/bindings/ion,physmem.txt
new file mode 100644
index 0000000..df18b5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,80 @@
+ION PhysMem Driver
+#include <dt-bindings/ion,physmem.h>
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows you to 
+define ION Memory Manager heaps using device tree. This is mostly useful if 
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory banks, 
+etc) that are present in the physical memory map and you want to add them to 
+ION as heaps of memory. 
+
+
+Examples: 
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as a physical
+   address range
+
+	ion_im0: ion@0x00100000 { 
+	     compatible = "ion,physmem";
+	     reg = <0x00100000 0x40000>;
+	     reg-names = "memory";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "IM0";
+	};
+
+2. The same, but using system DMA memory. 
+
+	ion_dma: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "SYSDMA";
+	};
+
+2. Carveout heap, 1MiB size, ion-physmem will alloc pages for it using 
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+		ion_crv: ion@deadbeef {
+		     compatible = "ion,physmem";
+		     reg = <0x00000000 0x100000>;
+		     reg-names = "memory";
+		     ion-heap-id   = <3>;
+		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
+		     ion-heap-align = <0x10>;
+		     ion-heap-name = "carveout";
+		};
+
+3. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using 
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "chunky";
+	};
+
+
+4. vmalloc();
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "sys";
+	};
+
+5. kmalloc();
+
+	ion_chunk: ion@0xdeadbeef { 
+	     compatible = "ion,physmem";
+	     ion-heap-id   = <2>;
+	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
+	     ion-heap-align = <0x10>;		     
+	     ion-heap-name = "syscont";
+	};
-- 
1.7.10.4

Re: [PATCH 1/2] staging: ion: Add ion-physmem driver

[Paul Bolle]

This patch adds a mismatch between the Kconfig symbol (a bool) and the
code (which suggests it could be built modular too).

On Mon, 2015-04-06 at 18:58 +0300, Andrew Andrianov wrote:
> +config ION_PHYSMEM
> +	bool "Generic PhysMem ION driver"
> +	depends on ION
> +	help
> +	  Provides a generic ION driver that registers the
> +	  /dev/ion device with heaps from devicetree entries.
> +	  This heaps are made of chunks of physical memory

> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile

> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
> -obj-$(CONFIG_ION_TEGRA) += tegra/
> +obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
> +obj-$(CONFIG_ION_TEGRA)   += tegra/

To make absolutely sure I'm reading this correctly: there's no way
ion_physmem.o can ever be part of a module, right?

(If I'm not reading this correctly you can stop reading here.)

> --- /dev/null
> +++ b/drivers/staging/android/ion/ion_physmem.c
> @@ -0,0 +1,237 @@
> +/*
> + * Copyright (C) 2015 RC Module
> + * Andrew Andrianov <andrew@ncrmnt.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Generic devicetree physmem driver for ION Memory Manager
> + *
> + */

> +#include <linux/module.h>

If this file can only be built-in this include might not be needed.

> +MODULE_DEVICE_TABLE(of, of_match_table);

MODULE_DEVICE_TABLE will always be preprocessed away for built-in code
(see include/linux/module.h).

> +static int ion_physmem_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	u32 ion_heap_id, ion_heap_align, ion_heap_type;
> +	ion_phys_addr_t addr;
> +	size_t size = 0;
> +	const char *ion_heap_name;
> +	struct resource *res;
> +	struct physmem_ion_dev *ipdev;
> +
> +	/*
> +	   Looks like we can only have one ION device in our system.
> +	   Therefore we call ion_device_create on first probe and only
> +	   add heaps to it on subsequent probe calls.
> +	   FixMe: Do we need to hold a spinlock here once device probing
> +	   becomes async?
> +	*/
> +
> +	if (!idev) {
> +		idev = ion_device_create(ion_physmem_custom_ioctl);
> +		dev_info(&pdev->dev, "ion-physmem: ION PhysMem Driver. (c) RC Module 2015\n");
> +		if (!idev)
> +			return -ENOMEM;
> +	}
> +
> +	ipdev = kzalloc(sizeof(struct physmem_ion_dev), GFP_KERNEL);
> +	if (!ipdev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ipdev);
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
> +				    &ion_heap_id);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
> +				    &ion_heap_type);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
> +				    &ion_heap_align);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
> +				       &ion_heap_name);
> +	if (ret != 0)
> +		goto errfreeipdev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
> +	/* Not always needed, throw no error if missing */
> +	if (res) {
> +		/* Fill in some defaults */
> +		addr = res->start;
> +		size = resource_size(res);
> +	}
> +
> +	switch (ion_heap_type) {
> +	case ION_HEAP_TYPE_DMA:
> +		if (res) {
> +			ret = dma_declare_coherent_memory(&pdev->dev,
> +							  res->start,
> +							  res->start,
> +							  resource_size(res),
> +							  DMA_MEMORY_MAP |
> +							  DMA_MEMORY_EXCLUSIVE);
> +			if (ret == 0) {
> +				ret = -ENODEV;
> +				goto errfreeipdev;
> +			}
> +		}
> +		/*
> +		 *  If no memory region declared in dt we assume that
> +		 *  the user is okay with plain dma_alloc_coherent.
> +		 */
> +		break;
> +	case ION_HEAP_TYPE_CARVEOUT:
> +	case ION_HEAP_TYPE_CHUNK:
> +	{
> +		if (size == 0) {
> +			ret = -EIO;
> +			goto errfreeipdev;
> +		}
> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
> +		if (ipdev->freepage_ptr) {
> +			addr = virt_to_phys(ipdev->freepage_ptr);
> +		} else {
> +			ret = -ENOMEM;
> +			goto errfreeipdev;
> +		}
> +		break;
> +	}
> +	}
> +
> +	ipdev->data.id    = ion_heap_id;
> +	ipdev->data.type  = ion_heap_type;
> +	ipdev->data.name  = ion_heap_name;
> +	ipdev->data.align = ion_heap_align;
> +	ipdev->data.base  = addr;
> +	ipdev->data.size  = size;
> +	/* This one make dma_declare_coherent_memory actually work */
> +	ipdev->data.priv  = &pdev->dev;
> +
> +
> +	ipdev->heap = ion_heap_create(&ipdev->data);
> +	if (!ipdev->heap)
> +		goto errfreeipdev;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		goto errfreeheap;

For built-in only code THIS_MODULE will be, basically, always NULL. So,
I think try_module_get() will always return true.

> +	ion_device_add_heap(idev, ipdev->heap);
> +
> +	dev_info(&pdev->dev, "ion-physmem: heap %s id %d type %d align 0x%x at phys 0x%lx len %lu KiB\n",
> +	       ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
> +	       (unsigned long int) addr, ((unsigned long int) size / 1024));
> +
> +	return 0;
> +
> +errfreeheap:
> +	kfree(ipdev->heap);
> +errfreeipdev:
> +	kfree(ipdev);
> +	return -ENOMEM;
> +}
> +
> +static int ion_physmem_remove(struct platform_device *pdev)
> +{
> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
> +
> +	ion_heap_destroy(ipdev->heap);
> +	if (ipdev->need_free_coherent)
> +		dma_release_declared_memory(&pdev->dev);
> +	if (ipdev->freepage_ptr)
> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
> +	kfree(ipdev->heap);
> +	kfree(ipdev);
> +	module_put(THIS_MODULE);

Again, THIS_MODULE will be, basically, always NULL.

> +	return 0;
> +}
> +
> +static struct platform_driver ion_physmem_driver = {
> +	.probe		= ion_physmem_probe,
> +	.remove		= ion_physmem_remove,
> +	.driver		= {
> +		.owner	= THIS_MODULE,

Ditto.

> +		.name	= "ion-physmem",
> +		.of_match_table = of_match_ptr(of_match_table),
> +	},
> +};
> +module_platform_driver(ion_physmem_driver);

For built-in only code this is equivalent to, if I remember correctly,
having a small wrapper call
    platform_driver_register(&ion_physmem_driver);

and mark that wrapper with device_initcall().

> +MODULE_DESCRIPTION("Generic physmem driver for ION");
> +MODULE_AUTHOR("Andrew Andrianov <andrew@ncrmnt.org>");
> +MODULE_LICENSE("GPL");

These macros will always be effectively preprocessed away for built-in
only code.

(If you plan to make ION_PHYSMEM tristate, you should note that "GPL"
doesn't match the license stated in the comment at the top of this file,
see include/linux/module.h.)


Paul Bolle

Re: [PATCH 1/2] staging: ion: Add ion-physmem driver

[Greg Kroah-Hartman]

On Tue, Apr 07, 2015 at 10:35:45AM +0200, Paul Bolle wrote:
> > +	ipdev->heap = ion_heap_create(&ipdev->data);
> > +	if (!ipdev->heap)
> > +		goto errfreeipdev;
> > +
> > +	if (!try_module_get(THIS_MODULE))
> > +		goto errfreeheap;
> 
> For built-in only code THIS_MODULE will be, basically, always NULL. So,
> I think try_module_get() will always return true.

Even if this is a module, this code is racy and broken, no module should
_ever_ call try_module_get(THIS_MODULE).  That's a sign of broken code,
and/or a broken API.

thanks,

greg k-h

Re: [PATCH 1/2] staging: ion: Add ion-physmem driver

[Andrew]

Paul Bolle писал 07.04.2015 11:35:
> This patch adds a mismatch between the Kconfig symbol (a bool) and the
> code (which suggests it could be built modular too).
> 
> On Mon, 2015-04-06 at 18:58 +0300, Andrew Andrianov wrote:
>> +config ION_PHYSMEM
>> +	bool "Generic PhysMem ION driver"
>> +	depends on ION
>> +	help
>> +	  Provides a generic ION driver that registers the
>> +	  /dev/ion device with heaps from devicetree entries.
>> +	  This heaps are made of chunks of physical memory
> 
>> --- a/drivers/staging/android/ion/Makefile
>> +++ b/drivers/staging/android/ion/Makefile
> 
>> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
>> -obj-$(CONFIG_ION_TEGRA) += tegra/
>> +obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
>> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
>> +obj-$(CONFIG_ION_TEGRA)   += tegra/
> 
> To make absolutely sure I'm reading this correctly: there's no way
> ion_physmem.o can ever be part of a module, right?
> 
> (If I'm not reading this correctly you can stop reading here.)
> 
>> --- /dev/null
>> +++ b/drivers/staging/android/ion/ion_physmem.c
>> @@ -0,0 +1,237 @@
>> +/*
>> + * Copyright (C) 2015 RC Module
>> + * Andrew Andrianov <andrew@ncrmnt.org>
>> + *
>> + * This program is free software; you can redistribute it and/or 
>> modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Generic devicetree physmem driver for ION Memory Manager
>> + *
>> + */
> 
>> +#include <linux/module.h>
> 
> If this file can only be built-in this include might not be needed.
> 
>> +MODULE_DEVICE_TABLE(of, of_match_table);
> 
> MODULE_DEVICE_TABLE will always be preprocessed away for built-in code
> (see include/linux/module.h).
> 
>> +static int ion_physmem_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	u32 ion_heap_id, ion_heap_align, ion_heap_type;
>> +	ion_phys_addr_t addr;
>> +	size_t size = 0;
>> +	const char *ion_heap_name;
>> +	struct resource *res;
>> +	struct physmem_ion_dev *ipdev;
>> +
>> +	/*
>> +	   Looks like we can only have one ION device in our system.
>> +	   Therefore we call ion_device_create on first probe and only
>> +	   add heaps to it on subsequent probe calls.
>> +	   FixMe: Do we need to hold a spinlock here once device probing
>> +	   becomes async?
>> +	*/
>> +
>> +	if (!idev) {
>> +		idev = ion_device_create(ion_physmem_custom_ioctl);
>> +		dev_info(&pdev->dev, "ion-physmem: ION PhysMem Driver. (c) RC 
>> Module 2015\n");
>> +		if (!idev)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	ipdev = kzalloc(sizeof(struct physmem_ion_dev), GFP_KERNEL);
>> +	if (!ipdev)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, ipdev);
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
>> +				    &ion_heap_id);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
>> +				    &ion_heap_type);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
>> +				    &ion_heap_align);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
>> +				       &ion_heap_name);
>> +	if (ret != 0)
>> +		goto errfreeipdev;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
>> +	/* Not always needed, throw no error if missing */
>> +	if (res) {
>> +		/* Fill in some defaults */
>> +		addr = res->start;
>> +		size = resource_size(res);
>> +	}
>> +
>> +	switch (ion_heap_type) {
>> +	case ION_HEAP_TYPE_DMA:
>> +		if (res) {
>> +			ret = dma_declare_coherent_memory(&pdev->dev,
>> +							  res->start,
>> +							  res->start,
>> +							  resource_size(res),
>> +							  DMA_MEMORY_MAP |
>> +							  DMA_MEMORY_EXCLUSIVE);
>> +			if (ret == 0) {
>> +				ret = -ENODEV;
>> +				goto errfreeipdev;
>> +			}
>> +		}
>> +		/*
>> +		 *  If no memory region declared in dt we assume that
>> +		 *  the user is okay with plain dma_alloc_coherent.
>> +		 */
>> +		break;
>> +	case ION_HEAP_TYPE_CARVEOUT:
>> +	case ION_HEAP_TYPE_CHUNK:
>> +	{
>> +		if (size == 0) {
>> +			ret = -EIO;
>> +			goto errfreeipdev;
>> +		}
>> +		ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
>> +		if (ipdev->freepage_ptr) {
>> +			addr = virt_to_phys(ipdev->freepage_ptr);
>> +		} else {
>> +			ret = -ENOMEM;
>> +			goto errfreeipdev;
>> +		}
>> +		break;
>> +	}
>> +	}
>> +
>> +	ipdev->data.id    = ion_heap_id;
>> +	ipdev->data.type  = ion_heap_type;
>> +	ipdev->data.name  = ion_heap_name;
>> +	ipdev->data.align = ion_heap_align;
>> +	ipdev->data.base  = addr;
>> +	ipdev->data.size  = size;
>> +	/* This one make dma_declare_coherent_memory actually work */
>> +	ipdev->data.priv  = &pdev->dev;
>> +
>> +
>> +	ipdev->heap = ion_heap_create(&ipdev->data);
>> +	if (!ipdev->heap)
>> +		goto errfreeipdev;
>> +
>> +	if (!try_module_get(THIS_MODULE))
>> +		goto errfreeheap;
> 
> For built-in only code THIS_MODULE will be, basically, always NULL. So,
> I think try_module_get() will always return true.
> 
>> +	ion_device_add_heap(idev, ipdev->heap);
>> +
>> +	dev_info(&pdev->dev, "ion-physmem: heap %s id %d type %d align 0x%x 
>> at phys 0x%lx len %lu KiB\n",
>> +	       ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align,
>> +	       (unsigned long int) addr, ((unsigned long int) size / 1024));
>> +
>> +	return 0;
>> +
>> +errfreeheap:
>> +	kfree(ipdev->heap);
>> +errfreeipdev:
>> +	kfree(ipdev);
>> +	return -ENOMEM;
>> +}
>> +
>> +static int ion_physmem_remove(struct platform_device *pdev)
>> +{
>> +	struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
>> +
>> +	ion_heap_destroy(ipdev->heap);
>> +	if (ipdev->need_free_coherent)
>> +		dma_release_declared_memory(&pdev->dev);
>> +	if (ipdev->freepage_ptr)
>> +		free_pages_exact(ipdev->freepage_ptr, ipdev->data.size);
>> +	kfree(ipdev->heap);
>> +	kfree(ipdev);
>> +	module_put(THIS_MODULE);
> 
> Again, THIS_MODULE will be, basically, always NULL.
> 
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver ion_physmem_driver = {
>> +	.probe		= ion_physmem_probe,
>> +	.remove		= ion_physmem_remove,
>> +	.driver		= {
>> +		.owner	= THIS_MODULE,
> 
> Ditto.
> 
>> +		.name	= "ion-physmem",
>> +		.of_match_table = of_match_ptr(of_match_table),
>> +	},
>> +};
>> +module_platform_driver(ion_physmem_driver);
> 
> For built-in only code this is equivalent to, if I remember correctly,
> having a small wrapper call
>     platform_driver_register(&ion_physmem_driver);
> 
> and mark that wrapper with device_initcall().
> 
>> +MODULE_DESCRIPTION("Generic physmem driver for ION");
>> +MODULE_AUTHOR("Andrew Andrianov <andrew@ncrmnt.org>");
>> +MODULE_LICENSE("GPL");
> 
> These macros will always be effectively preprocessed away for built-in
> only code.
> 
> (If you plan to make ION_PHYSMEM tristate, you should note that "GPL"
> doesn't match the license stated in the comment at the top of this 
> file,
> see include/linux/module.h.)
> 
> 
> Paul Bolle

Thanks for the detailed review, I'll clean things up and resubmit.
I've marked Kconfig option as bool for now, since I haven't tested 
module
unloading yet and didn't have a chance to look how ION handles it.
Right now it is unclean for me if ION device drivers _should_ be built 
as
modules (e.g. ion-dummy-driver can't be ever unloaded and it's the only
one upstream).
If we go the module way - I'm not (yet) sure what steps are necessary to
take to ensure that ION device driver will NOT get unloaded when any of
the heaps it provides are in use by anyone.
(module_get/module_put came from initial playing with that use case)

So far building as a module looks like the only sane way for generic arm
kernels supporting multiple SoCs, since we can only have ONE /dev/ion 
registered
in the system.

-- 
Regards,
Andrew