On Wed, May 22, 2024 at 2:02 AM Barry Song 21cnbao@gmail.com wrote:

From: Barry Song v-songbaohua@oppo.com

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@google.com