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
