Re: [PATCH v2 5/7] bcm-vk: add bcm_vk UAPI

From: Scott Branden
Date: Fri Feb 21 2020 - 13:28:09 EST

On 2020-02-21 12:34 a.m., Arnd Bergmann wrote:
On Fri, Feb 21, 2020 at 2:16 AM Scott Branden
<scott.branden@xxxxxxxxxxxx> wrote:

+struct vk_access {
+ __u8 barno; /* BAR number to use */
+ __u8 type; /* Type of access */
+#define VK_ACCESS_READ 0
+#define VK_ACCESS_WRITE 1
+ __u32 len; /* length of data */
Horrible padding issues, are you sure this all works properly?
Haven't had any issues.
+ __u64 offset; /* offset in BAR */
+ __u32 *data; /* where to read/write data to */
Are you _SURE_ you want a pointer here? How do you handle the compat
issues with 32/64 user/kernel space?
Don't care about 32-bit user space for this driver.
I don't think there isn't even enough memory in such systems for the
number of streams of video buffers needed for transcoding.
This driver is only used in high end 64-bit x86 servers.
Please see Documentation/core-api/ioctl.rst

All ioctl interfaces should be written in a portable way that works with
compat user space and avoids all padding in order to not leak kernel
data into user space.

If the driver is passing video buffers for transcoding, shouldn't the driver
use the existing drivers/media interfaces for that? If it needs features
that are not present there, they can probably be added.
It doesn't utilize any media interfaces. It is just an offload engine. Really, it could be offloading anything.
There is no infrastructure for other drivers in place that perform such transcoding operations.
Perhaps I shouldn't mention it is doing video in this driver.