On Wed, May 22, 2024 at 2:02 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
>
> From: Barry Song <v-songbaohua@xxxxxxxx>
>
> dma_heap_allocation_data defines the UAPI as follows:
>
> struct dma_heap_allocation_data {
> __u64 len;
> __u32 fd;
> __u32 fd_flags;
> __u64 heap_flags;
> };
>
> However, dma_heap_buffer_alloc() casts them into unsigned int. It's unclear
> whether this is intentional or what the purpose is, but it can be quite
> confusing for users.
>
> Adding to the confusion, dma_heap_ops.allocate defines both of these as
> unsigned long. Fortunately, since dma_heap_ops is not part of the UAPI,
> it is less of a concern.
>
> struct dma_heap_ops {
> struct dma_buf *(*allocate)(struct dma_heap *heap,
> unsigned long len,
> unsigned long fd_flags,
> unsigned long heap_flags);
> };
>
> I am sending this RFC in hopes of clarifying these confusions.
>
> If the goal is to constrain both flags to 32 bits while ensuring the struct
> is aligned to 64 bits, it would have been more suitable to define
> dma_heap_allocation_data accordingly from the beginning, like so:
>
> struct dma_heap_allocation_data {
> __u64 len;
> __u32 fd;
> __u32 fd_flags;
> __u32 heap_flags;
> __u32 padding;
> };
So here, if I recall, the intent was to keep 64bits for potential
future heap_flags.
But your point above that we're inconsistent with types in the non
UAPI arguments is valid.
So I think your patch makes sense.
Thanks for raising this issue!
Acked-by: John Stultz <jstultz@xxxxxxxxxx>
