Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751126AbbGAHkA (ORCPT ); Wed, 1 Jul 2015 03:40:00 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:21236 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750958AbbGAHjy (ORCPT ); Wed, 1 Jul 2015 03:39:54 -0400 Date: Wed, 1 Jul 2015 10:39:20 +0300 From: Dan Carpenter To: Laura Abbott Cc: Andrew Andrianov , Greg Kroah-Hartman , pebolle@tiscali.nl, Chen Gang , Arve =?utf-8?B?SGrvv71ubmV277+9Zw==?= , linux-kernel@vger.kernel.org, Fabian Frederick , Riley Andrews , john.stultz@linaro.org, devel@linuxdriverproject.org, Android Kernel Team , Sudip Mukherjee Subject: Re: [PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver Message-ID: <20150701073919.GS28762@mwanda> References: <1435678490-7515-1-git-send-email-andrew@ncrmnt.org> <1435678490-7515-2-git-send-email-andrew@ncrmnt.org> <5592D84B.6070801@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5592D84B.6070801@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7180 Lines: 259 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 -- 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/