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

[Greg Kroah-Hartman]

On Thu, Feb 20, 2020 at 05:15:58PM -0800, Scott Branden wrote:
> Hi Greg,
> 
> Thanks for the review.  Comments inline.
> 
> On 2020-02-19 11:50 p.m., Greg Kroah-Hartman wrote:
> > On Wed, Feb 19, 2020 at 04:48:23PM -0800, Scott Branden wrote:
> > > Add user space api for bcm-vk driver.
> > > 
> > > Signed-off-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
> > > ---
> > >   include/uapi/linux/misc/bcm_vk.h | 117 +++++++++++++++++++++++++++++++
> > >   1 file changed, 117 insertions(+)
> > >   create mode 100644 include/uapi/linux/misc/bcm_vk.h
> > > 
> > > diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h
> > > new file mode 100644
> > > index 000000000000..56a2178e06f5
> > > --- /dev/null
> > > +++ b/include/uapi/linux/misc/bcm_vk.h
> > > @@ -0,0 +1,117 @@
> > > +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
> > > +/*
> > > + * Copyright 2018-2020 Broadcom.
> > > + */
> > > +
> > > +#ifndef __UAPI_LINUX_MISC_BCM_VK_H
> > > +#define __UAPI_LINUX_MISC_BCM_VK_H
> > > +
> > > +#include <linux/ioctl.h>
> > > +#include <linux/types.h>
> > > +
> > > +struct vk_image {
> > > +	__u32 type;     /* Type of image */
> > > +#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */
> > > +#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */
> > > +	char filename[64]; /* Filename of image */
> > __u8?
> I don't understand why char is not appropriate for a filename.
> Would like to understand why __u8 is correct to use here vs. char.

Why is __u8 not correct?  It's the data type we use for ioctls.

> > > +};
> > > +
> > > +/* default firmware images names */
> > > +#define VK_BOOT1_DEF_VALKYRIE_FILENAME	"vk-boot1.bin"
> > > +#define VK_BOOT2_DEF_VALKYRIE_FILENAME	"vk-boot2.bin"
> > > +
> > > +#define VK_BOOT1_DEF_VIPER_FILENAME	"vp-boot1.bin"
> > > +#define VK_BOOT2_DEF_VIPER_FILENAME	"vp-boot2.bin"
> > Why do these need to be in a uapi .h file?  Shouldn't they just be part
> > of the normal MODULE_FIRMWARE() macro in the driver itself?
> ioctl VK_IOCTL_LOAD_IMAGE passes in type of image to load and filename.
> These are the default names used if the images are autoloaded by the driver.

Then put them in the driver, not in the user api file.

> But if userspace app wishes to load (or reload) the default images then it
> needs to know the name of the file to pass in ioctl.

That's up to userspace.

> I guess I could change the API at this point to lookup the default filename
> if NULL filename passed into ioctl.

Yes please.

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

Use pahole to see the holes you have in here and please fix that up.

> > > +	__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.

We all do, see the link that Arnd sent you.

> I don't think there isn't even enough memory in such systems for the number
> of streams of video buffers needed for transcoding.

32bit systems have lots of memory.

> This driver is only used in high end 64-bit x86 servers.

For today, what about in 2 years?

> But, VK_IOCTL_ACCESS_BAR can go away entirely if standard user space
> approach already exists as you imply.

Yes, please use that interface, as you should never duplicate existing
functionality.

> > > +};
> > And isn't this just a normal PCI write thing?  Can't you do it from
> > userspace using the existing userspace PCI accesses?  Why do you need a
> > special ioctl for it?
> This follows how pci_endpoint_test reads and writes BARS via ioctl.
> It also abstracts the accesses all into the device node being opened.
> 
> I am not familiar with userspace PCI accesses.  Would this be through some
> sys entries?

Yes, it can read PCI config space that way, and if you use the uio
interface, you can read PCI memory.

thanks,

greg k-h