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

Commit Message

Barry Song May 22, 2024, 9:01 a.m. UTC

From: Barry Song <[e-mail]>

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;
 };

Signed-off-by: Barry Song <[e-mail]>
---
 drivers/dma-buf/dma-heap.c    | 4 ++--
 include/uapi/linux/dma-heap.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

John Stultz May 29, 2024, 10:31 a.m. UTC | #1

On Wed, May 22, 2024 at 2:02 AM Barry Song <[e-mail]> wrote:
>
> From: Barry Song <[e-mail]>
>
> 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 <[e-mail]>

Patch

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 84ae708fafe7..2298ca5e112e 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -50,8 +50,8 @@  static struct class *dma_heap_class;
 static DEFINE_XARRAY_ALLOC(dma_heap_minors);
 
 static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
-				 unsigned int fd_flags,
-				 unsigned int heap_flags)
+				 u32 fd_flags,
+				 u64 heap_flags)
 {
 	struct dma_buf *dmabuf;
 	int fd;
diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
index 6f84fa08e074..a4cf716a49fa 100644
--- a/include/uapi/linux/dma-heap.h
+++ b/include/uapi/linux/dma-heap.h
@@ -19,7 +19,7 @@ 
 #define DMA_HEAP_VALID_FD_FLAGS (O_CLOEXEC | O_ACCMODE)
 
 /* Currently no heap flags */
-#define DMA_HEAP_VALID_HEAP_FLAGS (0)
+#define DMA_HEAP_VALID_HEAP_FLAGS (0ULL)
 
 /**
  * struct dma_heap_allocation_data - metadata passed from userspace for