Received-SPF: pass (google.com: domain of linux-kernel+bounces-193964-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240522090158.121797-1-21cnbao@gmail.com>
In-Reply-To: <20240522090158.121797-1-21cnbao@gmail.com>
From: John Stultz <jstultz@google.com>
Date: Wed, 29 May 2024 03:31:22 -0700
Message-ID: <CANDhNCqr4MtkEXG4uBOViPAODQnuQOgpdNZCmP4yvVSNwnU5ew@mail.gmail.com>
Subject: Re: [RFC PATCH] dma-buf: align fd_flags and heap_flags with dma_heap_allocation_data
To: Barry Song <21cnbao@gmail.com>
Cc: sumit.semwal@linaro.org, linux-media@vger.kernel.org, 
	linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org, benjamin.gaignard@collabora.com, 
	Brian.Starkey@arm.com, tjmercier@google.com, christian.koenig@amd.com, 
	Barry Song <v-songbaohua@oppo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 22, 2024 at 2:02=E2=80=AFAM Barry Song <21cnbao@gmail.com> wrot=
e:
>
> 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 uncle=
ar
> 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 stru=
ct
> 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>