I started reviewing this patch but then part way through I relized I
must be missing quite a bit of it...
On Tue, Jun 30, 2015 at 10:56:27AM -0700, Laura Abbott wrote:
> On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
> >+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 = NULL;
> >+ 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:
> >+ 1. Do we need to hold a spinlock here?
> >+ 2. Can several probes race here on SMP?
> >+ */
Comment style.
> >+
> >+ if (!idev) {
> >+ idev = ion_device_create(NULL);
> >+ dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n");
> >+ if (!idev)
> >+ return -ENOMEM;
> >+ }
>
> Yeah this is a bit messy as your comments note. Since there can only be one Ion
> device in the system, it seems like it would make more sense to have a top level
> DT node and then have the heaps be subnodes to avoid this 'guess when to create
> the device' bit.
>
> >+
> >+ ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
> >+ if (!ipdev)
> >+ return -ENOMEM;
> >+
> >+ platform_set_drvdata(pdev, ipdev);
> >+
> >+ /* Read out name first for the sake of sane error-reporting */
> >+ ret = of_property_read_string(pdev->dev.of_node, "ion-heap-name",
> >+ &ion_heap_name);
Extra space after =.
> >+ if (ret != 0)
> >+ goto errfreeipdev;
Remove the double negative. "if (ret) ".
> >+
> >+ ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
> >+ &ion_heap_id);
> >+ if (ret != 0)
> >+ goto errfreeipdev;
> >+
> >+ /* Check id to be sane first */
> >+ if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
Too many parens. ion_heap_id is unsigned.
if (ion_heap_id >= ION_NUM_HEAP_IDS) {
> >+ dev_err(&pdev->dev, "bad heap id specified: %d\n",
> >+ ion_heap_id);
> >+ goto errfreeipdev;
Set an error before the return.
> >+ }
> >+
> >+ if ((1 << ion_heap_id) & claimed_heap_ids) {
> >+ dev_err(&pdev->dev, "heap id %d is already claimed\n",
> >+ ion_heap_id);
> >+ goto errfreeipdev;
Missing error code.
> >+ }
>
> I thought the core Ion framework was already checking this but
> apparently not. This is so fundamental it should really be moved into
> the core framework and not at the client level.
>
> >+
> >+ ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
> >+ &ion_heap_type);
Space.
> >+ if (ret != 0)
> >+ goto errfreeipdev;
Double negative.
> >+
> >+ ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
> >+ &ion_heap_align);
Space.
> >+ if (ret != 0)
> >+ goto errfreeipdev;
Double negative.
> >+
> >+ 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;
> >+ }
> >+ }
>
> The name is ION_HEAP_TYPE_DMA but the real point of the heap was to
> be able to use CMA memory. Calling dma_declare_coherent_memory defeats
> the point since we won't use CMA memory. The coherent memory pool is closer
> to a carveout anyway so I'd argue if you want the effects of a coherent
> memory pool you should be using carveout memory anyway.
>
> >+ /*
> >+ * 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;
> >+ }
Could you flip this around so it's error handling instead of success
handling?
ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL);
if (!ipdev->freepage_ptr) {
ret = -ENOMEM;
goto errfreeipdev;
}
addr = virt_to_phys(ipdev->freepage_ptr);
break;
> >+ break;
> >+ }
> >+
>
> This won't work if the carveout region is larger than the buddy allocator
> allows. That's why ion_reserve used the memblock APIs, to avoid being
> limited by buddy allocator sizes.
>
> >+ 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;
Set an error code.
> >+
> >+ /* If it's needed - take care enable clocks */
> >+ ipdev->clk = devm_clk_get(&pdev->dev, NULL);
> >+ if (IS_ERR(ipdev->clk))
> >+ ipdev->clk = NULL;
> >+ else
> >+ clk_prepare_enable(ipdev->clk);
> >+
>
> Probe deferal for the clocks here?
>
> >+ ion_device_add_heap(idev, ipdev->heap);
> >+ claimed_heap_ids |= (1 << ion_heap_id);
> >+ ipdev->heap_id = ion_heap_id;
> >+
> >+ dev_dbg(&pdev->dev, "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)));
To be honest, I don't understand ion_phys_addr_t. This code works but
I kind of feel that instead of "unsigned long int" we should be casting
to u64 the same as we would for a phys_addr_t. We should use %zx for
(size / 1024).
> >+ return 0;
> >+
> >+errfreeipdev:
> >+ kfree(ipdev);
> >+ dev_err(&pdev->dev, "Failed to register heap: %s\n",
> >+ ion_heap_name);
> >+ return -ENOMEM;
We set "ret" on most paths. I sort of assumed we were going to return
it. :P Ignore what I said earlier about missing return codes, I
suppose.
> >+}
> >+
> >+static int ion_physmem_remove(struct platform_device *pdev)
> >+{
> >+ struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev);
> >+
> >+ ion_heap_destroy(ipdev->heap);
> >+ claimed_heap_ids &= ~(1 << ipdev->heap_id);
> >+ if (ipdev->need_free_coherent)
Am I missing parts of this patch? Where do we set this? Never mind...
I guess I'm just going to send the review so far.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
