Messages in this thread Patch in this message | | | Date | Tue, 26 Jan 2021 11:24:43 -0800 | From | Minchan Kim <> | Subject | Re: [PATCH v4 4/4] dma-buf: heaps: add chunk heap to dmabuf heaps |
| |
On Mon, Jan 25, 2021 at 11:32:57PM -0800, John Stultz wrote: > On Thu, Jan 21, 2021 at 9:55 AM Minchan Kim <minchan@kernel.org> wrote: > > Hey Minchan, > Thanks for sending this out! I'm still working through testing with > this patch set, so I may have some more feedback tomorrow, but a few > quick items I did hit below. > > > + > > +#define CHUNK_PREFIX "chunk-" > > + > > +static int register_chunk_heap(struct chunk_heap *chunk_heap_info) > > +{ > > + struct dma_heap_export_info exp_info; > > + const char *name = cma_get_name(chunk_heap_info->cma); > > + size_t len = strlen(CHUNK_PREFIX) + strlen(name) + 1; > > + char *buf = kmalloc(len, GFP_KERNEL); > > + > > + if (!buf) > > + return -ENOMEM; > > + > > + sprintf(buf, CHUNK_PREFIX"%s", cma_get_name(chunk_heap_info->cma)); > > + buf[len] = '\0'; > > + > > + exp_info.name = buf; > > + exp_info.name = cma_get_name(chunk_heap_info->cma); > > I think you intended to delete this line, as it's overwriting your > prefixed name.
Hi John,
You're right. Will fix it.
> > > + exp_info.ops = &chunk_heap_ops; > > + exp_info.priv = chunk_heap_info; > > + > > + chunk_heap_info->heap = dma_heap_add(&exp_info); > > + if (IS_ERR(chunk_heap_info->heap)) { > > + kfree(buf); > > + return PTR_ERR(chunk_heap_info->heap); > > + } > > + > > + return 0; > > +} > > + > > +static int __init chunk_heap_init(void) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < chunk_heap_count; i++) > > + register_chunk_heap(&chunk_heaps[i]); > > + > > + return 0; > > +} > > +module_init(chunk_heap_init); > > + > > +#ifdef CONFIG_OF_EARLY_FLATTREE > > + > > +static int __init dmabuf_chunk_heap_area_init(struct reserved_mem *rmem) > > +{ > > + int ret; > > + struct cma *cma; > > + struct chunk_heap *chunk_heap_info; > > + const __be32 *chunk_order; > > + > > + phys_addr_t align = PAGE_SIZE << max(MAX_ORDER - 1, pageblock_order); > > + phys_addr_t mask = align - 1; > > + > > + if ((rmem->base & mask) || (rmem->size & mask)) { > > + pr_err("Incorrect alignment for CMA region\n"); > > + return -EINVAL; > > Passing this check can be tough if you're using dynamically assigned > rmem, so it might be helpful for debugging to print the base/size/mask > values?
Let me fold this into next respin.
diff --git a/drivers/dma-buf/heaps/chunk_heap.c b/drivers/dma-buf/heaps/chunk_heap.c index 6fe8e69d108f..cc2ed5341b54 100644 --- a/drivers/dma-buf/heaps/chunk_heap.c +++ b/drivers/dma-buf/heaps/chunk_heap.c @@ -456,7 +456,8 @@ static int __init dmabuf_chunk_heap_area_init(struct reserved_mem *rmem) phys_addr_t mask = align - 1;
if ((rmem->base & mask) || (rmem->size & mask)) { - pr_err("Incorrect alignment for CMA region\n"); + pr_err("Incorrect alignment for CMA region: base %pa size %pa mask %pa\n", + rmem->base, rmem->size, mask); return -EINVAL; }
Thanks for the review, John!
| |