Re: [PATCH v7 3/5] dma-buf: heaps: Add system heap to dmabuf heaps

["Andrew F. Davis" <afd@ti.com>]

On 7/25/19 9:02 AM, Christoph Hellwig wrote:
>> +struct system_heap {
>> +	struct dma_heap *heap;
>> +} sys_heap;
> 
> It seems like this structure could be removed and if would improve
> the code flow.
> 
>> +static struct dma_heap_ops system_heap_ops = {
>> +	.allocate = system_heap_allocate,
>> +};
>> +
>> +static int system_heap_create(void)
>> +{
>> +	struct dma_heap_export_info exp_info;
>> +	int ret = 0;
>> +
>> +	exp_info.name = "system_heap";
>> +	exp_info.ops = &system_heap_ops;
>> +	exp_info.priv = &sys_heap;
>> +
>> +	sys_heap.heap = dma_heap_add(&exp_info);
>> +	if (IS_ERR(sys_heap.heap))
>> +		ret = PTR_ERR(sys_heap.heap);
>> +
>> +	return ret;
> 
> The data structures here seem a little odd.  I think you want to:
> 
>  - mark all dma_heap_ops instanes consts, as we generally do that for
>    all structures containing function pointers
>  - move the name into dma_heap_ops.
>  - remove the dma_heap_export_info structure, which is a bit pointless


The idea here is to keep the struct dma_heap as an opaque pointer for
everyone but the core framework. No one should be touching the guts of
that struct (would be 'private' if we were using C++ but this is the
best we can do with C), exposing it to the exporters or the importers
will break this isolation.

This export style also matches DMA-BUF: you pass in a filled out _export
struct and it passes back a dma_buf handle. DMA-BUF unfortunately made
the internals of that struct public so they are widely used directly
(and abused in some cases) and that prevents the internals from being
easily refactored when needed.

Andrew


>  - don't bother setting a private data, as you don't need it.
>    If other heaps need private data I'd suggest to switch to embedding
>    the dma_heap structure into containing structure insted so that you
>    can use container_of to get at it.
>  - also why is the free callback passed as a callback rather than
>    kept in dma_heap_ops, next to the paired alloc one?
>