LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Scott Branden <scott.branden@broadcom.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
	David Brown <david.brown@linaro.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Shuah Khan <shuah@kernel.org>,
	bjorn.andersson@linaro.org,
	Shuah Khan <skhan@linuxfoundation.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	BCM Kernel Feedback <bcm-kernel-feedback-list@broadcom.com>,
	Olof Johansson <olof@lixom.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Colin Ian King <colin.king@canonical.com>,
	Kees Cook <keescook@chromium.org>, Takashi Iwai <tiwai@suse.de>,
	linux-kselftest@vger.kernel.org, Andy Gross <agross@kernel.org>
Subject: Re: [PATCH v2 5/7] bcm-vk: add bcm_vk UAPI
Date: Fri, 21 Feb 2020 10:22:17 +0100
Message-ID: <20200221092217.GA60193@kroah.com> (raw)
In-Reply-To: <030219dc-539a-a2db-5ab2-1de7336a811c@broadcom.com>

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@broadcom.com>
> > > ---
> > >   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

  parent reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-20  0:48 [PATCH v2 0/7] firmware: add partial read support in request_firmware_into_buf Scott Branden
2020-02-20  0:48 ` [PATCH v2 1/7] fs: introduce kernel_pread_file* support Scott Branden
2020-02-20  0:48 ` [PATCH v2 2/7] firmware: add offset to request_firmware_into_buf Scott Branden
2020-02-20  1:22   ` Luis Chamberlain
2020-02-21  0:14     ` Scott Branden
2020-02-20  0:48 ` [PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf Scott Branden
2020-02-20  8:42   ` Dan Carpenter
2020-02-21 18:30     ` Scott Branden
2020-02-22  1:13       ` Scott Branden
2020-02-24  8:08         ` Dan Carpenter
2020-02-25 19:11         ` Luis Chamberlain
2020-02-20  0:48 ` [PATCH v2 4/7] firmware: test partial file reads of request_firmware_into_buf Scott Branden
2020-02-20  0:48 ` [PATCH v2 5/7] bcm-vk: add bcm_vk UAPI Scott Branden
2020-02-20  7:50   ` Greg Kroah-Hartman
2020-02-21  1:15     ` Scott Branden
2020-02-21  8:34       ` Arnd Bergmann
2020-02-21 18:27         ` Scott Branden
2020-02-21  9:22       ` Greg Kroah-Hartman [this message]
2020-02-20  0:48 ` [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver Scott Branden
2020-02-20  1:04   ` Randy Dunlap
2020-02-21  0:06     ` Scott Branden
2020-02-21  0:21       ` Randy Dunlap
2020-02-20  7:47   ` Greg Kroah-Hartman
2020-02-21 18:19     ` Scott Branden
2020-02-22  8:02       ` Arnd Bergmann
2020-02-23 10:00         ` Greg Kroah-Hartman
2020-02-23 23:55         ` Olof Johansson
2020-02-25 22:37           ` Scott Branden
2020-02-20 10:43   ` Dan Carpenter
2020-02-21 18:29     ` Scott Branden
2020-04-17 21:49     ` Scott Branden
2020-04-18 11:45       ` Dan Carpenter
2020-04-18 11:47         ` Dan Carpenter
2020-04-18 17:25           ` Scott Branden
2020-02-22 16:44   ` kbuild test robot
2020-02-22 16:44   ` [RFC PATCH] misc: bcm-vk: image_tab[] can be static kbuild test robot
2020-02-20  0:48 ` [PATCH v2 7/7] MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver Scott Branden

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200221092217.GA60193@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=agross@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=colin.king@canonical.com \
    --cc=dan.carpenter@oracle.com \
    --cc=david.brown@linaro.org \
    --cc=keescook@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=olof@lixom.net \
    --cc=rafael@kernel.org \
    --cc=scott.branden@broadcom.com \
    --cc=shuah@kernel.org \
    --cc=skhan@linuxfoundation.org \
    --cc=tiwai@suse.de \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git