lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 25 Jul 2019 14:55:18 -0400
From:   "Andrew F. Davis" <afd@...com>
To:     Christoph Hellwig <hch@...radead.org>,
        John Stultz <john.stultz@...aro.org>
CC:     lkml <linux-kernel@...r.kernel.org>,
        Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Pratik Patel <pratikp@...eaurora.org>,
        Brian Starkey <Brian.Starkey@....com>,
        Vincent Donnefort <Vincent.Donnefort@....com>,
        Sudipto Paul <Sudipto.Paul@....com>,
        Chenbo Feng <fengc@...gle.com>,
        Alistair Strachan <astrachan@...gle.com>,
        Hridya Valsaraju <hridya@...gle.com>,
        <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v7 3/5] dma-buf: heaps: Add system heap to dmabuf heaps

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?
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ