Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753366AbbDGLfz (ORCPT ); Tue, 7 Apr 2015 07:35:55 -0400 Received: from forward8l.mail.yandex.net ([84.201.143.141]:42669 "EHLO forward8l.mail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751162AbbDGLfy (ORCPT ); Tue, 7 Apr 2015 07:35:54 -0400 To: Paul Bolle , Greg Kroah-Hartman Subject: Re: [PATCH 1/2] staging: ion: Add ion-physmem driver X-PHP-Originating-Script: 501:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Tue, 07 Apr 2015 14:35:47 +0300 From: Andrew Cc: =?UTF-8?Q?Arve_Hj=C3=B8nnev=C3=A5g?= , Riley Andrews , Chen Gang , Fabian Frederick , linux-kernel@vger.kernel.org In-Reply-To: <1428395745.634.168.camel@x220> References: <1428335363-9854-1-git-send-email-andrew@ncrmnt.org> <1428335934-10026-1-git-send-email-andrew@ncrmnt.org> <1428395745.634.168.camel@x220> Message-ID: <257dbed8a02326d043b0504d844e9699@mail.ncrmnt.org> User-Agent: Roundcube Webmail/1.0.2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8099 Lines: 264 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 >> + * >> + * 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 > > 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 "); >> +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/