Re: [RFC PATCH] dma-buf: align fd_flags and heap_flags with dma_heap_allocation_data

[John Stultz]

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>