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

[Dan Carpenter <dan.carpenter@oracle.com>]

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