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

[Scott Branden]

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.
>          Arnd