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

From: Andrew
Date: Tue Apr 07 2015 - 07:35:59 EST

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@xxxxxxxxxx>
+ *
+ * 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@xxxxxxxxxx>");
+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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/