[PATCH v2 0/7] firmware: add partial read support in request_firmware_into_buf

[PATCH v2 0/7] firmware: add partial read support in request_firmware_into_buf

[Scott Branden]

This patch series adds partial read support in request_firmware_into_buf.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.

Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.

Scott Branden (7):
  fs: introduce kernel_pread_file* support
  firmware: add offset to request_firmware_into_buf
  test_firmware: add partial read support for request_firmware_into_buf
  firmware: test partial file reads of request_firmware_into_buf
  bcm-vk: add bcm_vk UAPI
  misc: bcm-vk: add Broadcom VK driver
  MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver

 MAINTAINERS                                   |    7 +
 drivers/base/firmware_loader/firmware.h       |    5 +
 drivers/base/firmware_loader/main.c           |   49 +-
 drivers/misc/Kconfig                          |    1 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/bcm-vk/Kconfig                   |   42 +
 drivers/misc/bcm-vk/Makefile                  |   11 +
 drivers/misc/bcm-vk/bcm_vk.h                  |  357 +++++
 drivers/misc/bcm-vk/bcm_vk_dev.c              | 1197 +++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.c              | 1359 +++++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.h              |  210 +++
 drivers/misc/bcm-vk/bcm_vk_sg.c               |  273 ++++
 drivers/misc/bcm-vk/bcm_vk_sg.h               |   60 +
 drivers/misc/bcm-vk/bcm_vk_tty.c              |  327 ++++
 drivers/soc/qcom/mdt_loader.c                 |    7 +-
 fs/exec.c                                     |   77 +-
 include/linux/firmware.h                      |    8 +-
 include/linux/fs.h                            |   15 +
 include/uapi/linux/misc/bcm_vk.h              |  117 ++
 lib/test_firmware.c                           |  139 +-
 .../selftests/firmware/fw_filesystem.sh       |   80 +
 21 files changed, 4305 insertions(+), 37 deletions(-)
 create mode 100644 drivers/misc/bcm-vk/Kconfig
 create mode 100644 drivers/misc/bcm-vk/Makefile
 create mode 100644 drivers/misc/bcm-vk/bcm_vk.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_tty.c
 create mode 100644 include/uapi/linux/misc/bcm_vk.h

-- 
2.17.1

[PATCH v2 1/7] fs: introduce kernel_pread_file* support

[Scott Branden]

Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.  Existing kernel_read_file
functions call new kernel_pread_file functions with offset=0 and
flags=KERNEL_PREAD_FLAG_WHOLE.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 fs/exec.c          | 77 ++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h | 15 +++++++++
 2 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..a38f8fb432fa 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -896,10 +896,14 @@ struct file *open_exec(const char *name)
 }
 EXPORT_SYMBOL(open_exec);
 
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
-		     loff_t max_size, enum kernel_read_file_id id)
-{
-	loff_t i_size, pos;
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
+		      loff_t pos, loff_t max_size, unsigned int flags,
+		      enum kernel_read_file_id id)
+{
+	loff_t alloc_size;
+	loff_t buf_pos;
+	loff_t read_end;
+	loff_t i_size;
 	ssize_t bytes = 0;
 	int ret;
 
@@ -919,21 +923,31 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 		ret = -EINVAL;
 		goto out;
 	}
-	if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+
+	/* Default read to end of file */
+	read_end = i_size;
+
+	/* Allow reading partial portion of file */
+	if ((flags & KERNEL_PREAD_FLAG_PART) &&
+	    (i_size > (pos + max_size)))
+		read_end = pos + max_size;
+
+	alloc_size = read_end - pos;
+	if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) {
 		ret = -EFBIG;
 		goto out;
 	}
 
 	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
-		*buf = vmalloc(i_size);
+		*buf = vmalloc(alloc_size);
 	if (!*buf) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	pos = 0;
-	while (pos < i_size) {
-		bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
+	buf_pos = 0;
+	while (pos < read_end) {
+		bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos);
 		if (bytes < 0) {
 			ret = bytes;
 			goto out_free;
@@ -941,14 +955,16 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 
 		if (bytes == 0)
 			break;
+
+		buf_pos += bytes;
 	}
 
-	if (pos != i_size) {
+	if (pos != read_end) {
 		ret = -EIO;
 		goto out_free;
 	}
 
-	ret = security_kernel_post_read_file(file, *buf, i_size, id);
+	ret = security_kernel_post_read_file(file, *buf, alloc_size, id);
 	if (!ret)
 		*size = pos;
 
@@ -964,10 +980,20 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 	allow_write_access(file);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(kernel_pread_file);
+
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
+		     loff_t max_size, enum kernel_read_file_id id)
+{
+	return kernel_pread_file(file, buf, size, 0, max_size,
+				 KERNEL_PREAD_FLAG_WHOLE, id);
+}
 EXPORT_SYMBOL_GPL(kernel_read_file);
 
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
-			       loff_t max_size, enum kernel_read_file_id id)
+int kernel_pread_file_from_path(const char *path, void **buf,
+				loff_t *size, loff_t pos,
+				loff_t max_size, unsigned int flags,
+				enum kernel_read_file_id id)
 {
 	struct file *file;
 	int ret;
@@ -979,14 +1005,23 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
-	ret = kernel_read_file(file, buf, size, max_size, id);
+	ret = kernel_pread_file(file, buf, size, pos, max_size, flags, id);
 	fput(file);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(kernel_pread_file_from_path);
+
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+			       loff_t max_size, enum kernel_read_file_id id)
+{
+	return kernel_pread_file_from_path(path, buf, size, 0, max_size,
+					   KERNEL_PREAD_FLAG_WHOLE, id);
+}
 EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
 
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
-			     enum kernel_read_file_id id)
+int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, loff_t pos,
+			      loff_t max_size, unsigned int flags,
+			      enum kernel_read_file_id id)
 {
 	struct fd f = fdget(fd);
 	int ret = -EBADF;
@@ -994,11 +1029,19 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
 	if (!f.file)
 		goto out;
 
-	ret = kernel_read_file(f.file, buf, size, max_size, id);
+	ret = kernel_pread_file(f.file, buf, size, pos, max_size, flags, id);
 out:
 	fdput(f);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(kernel_pread_file_from_fd);
+
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
+			     enum kernel_read_file_id id)
+{
+	return kernel_pread_file_from_fd(fd, buf, size, 0, max_size,
+					 KERNEL_PREAD_FLAG_WHOLE, id);
+}
 EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
 
 ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t len)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3cd4fe6b845e..8f39530cfcc2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3008,10 +3008,25 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 	return kernel_read_file_str[id];
 }
 
+/* Flags used by kernel_pread_file functions */
+#define KERNEL_PREAD_FLAG_WHOLE	0x0000 /* Only Allow reading of whole file */
+#define KERNEL_PREAD_FLAG_PART	0x0001 /* Allow reading part of file */
+
+extern int kernel_pread_file(struct file *file, void **buf, loff_t *size,
+			     loff_t pos, loff_t max_size, unsigned int flags,
+			     enum kernel_read_file_id id);
 extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
 			    enum kernel_read_file_id);
+extern int kernel_pread_file_from_path(const char *path, void **buf,
+				       loff_t *size, loff_t pos,
+				       loff_t max_size, unsigned int flags,
+				       enum kernel_read_file_id id);
 extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
 				      enum kernel_read_file_id);
+extern int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
+				    loff_t pos, loff_t max_size,
+				    unsigned int flags,
+				    enum kernel_read_file_id id);
 extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
 				    enum kernel_read_file_id);
 extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
-- 
2.17.1

[PATCH v2 2/7] firmware: add offset to request_firmware_into_buf

[Scott Branden]

Add offset to request_firmware_into_buf to allow for portions
of firmware file to be read into a buffer.  Necessary where firmware
needs to be loaded in portions from file in memory constrained systems.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/base/firmware_loader/firmware.h |  5 +++
 drivers/base/firmware_loader/main.c     | 49 +++++++++++++++++--------
 drivers/soc/qcom/mdt_loader.c           |  7 +++-
 include/linux/firmware.h                |  8 +++-
 lib/test_firmware.c                     |  4 +-
 5 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 8656e5239a80..9f4fddcb1ab7 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -29,6 +29,8 @@
  *	firmware caching mechanism.
  * @FW_OPT_NOFALLBACK_SYSFS: Disable the sysfs fallback mechanism. Takes
  *	precedence over &FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
+ *	entire file.
  */
 enum fw_opt {
 	FW_OPT_UEVENT			= BIT(0),
@@ -37,6 +39,7 @@ enum fw_opt {
 	FW_OPT_NO_WARN			= BIT(3),
 	FW_OPT_NOCACHE			= BIT(4),
 	FW_OPT_NOFALLBACK_SYSFS		= BIT(5),
+	FW_OPT_PARTIAL			= BIT(6),
 };
 
 enum fw_status {
@@ -64,6 +67,8 @@ struct fw_priv {
 	void *data;
 	size_t size;
 	size_t allocated_size;
+	size_t offset;
+	unsigned int flags;
 #ifdef CONFIG_FW_LOADER_PAGED_BUF
 	bool is_paged_buf;
 	struct page **pages;
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 57133a9dad09..28312309aab4 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -167,7 +167,8 @@ static int fw_cache_piggyback_on_request(const char *name);
 
 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 					  struct firmware_cache *fwc,
-					  void *dbuf, size_t size)
+					  void *dbuf, size_t size,
+					  size_t offset, unsigned int flags)
 {
 	struct fw_priv *fw_priv;
 
@@ -185,6 +186,8 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 	fw_priv->fwc = fwc;
 	fw_priv->data = dbuf;
 	fw_priv->allocated_size = size;
+	fw_priv->offset = offset;
+	fw_priv->flags = flags;
 	fw_state_init(fw_priv);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&fw_priv->pending_list);
@@ -210,9 +213,11 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
 static int alloc_lookup_fw_priv(const char *fw_name,
 				struct firmware_cache *fwc,
 				struct fw_priv **fw_priv, void *dbuf,
-				size_t size, enum fw_opt opt_flags)
+				size_t size, enum fw_opt opt_flags,
+				size_t offset)
 {
 	struct fw_priv *tmp;
+	unsigned int pread_flags;
 
 	spin_lock(&fwc->lock);
 	if (!(opt_flags & FW_OPT_NOCACHE)) {
@@ -226,7 +231,12 @@ static int alloc_lookup_fw_priv(const char *fw_name,
 		}
 	}
 
-	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
+	if (opt_flags & FW_OPT_PARTIAL)
+		pread_flags = KERNEL_PREAD_FLAG_PART;
+	else
+		pread_flags = KERNEL_PREAD_FLAG_WHOLE;
+
+	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);
 	if (tmp) {
 		INIT_LIST_HEAD(&tmp->list);
 		if (!(opt_flags & FW_OPT_NOCACHE))
@@ -493,8 +503,9 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 		}
 
 		fw_priv->size = 0;
-		rc = kernel_read_file_from_path(path, &buffer, &size,
-						msize, id);
+		rc = kernel_pread_file_from_path(path, &buffer, &size,
+						 fw_priv->offset, msize,
+						 fw_priv->flags, id);
 		if (rc) {
 			if (rc != -ENOENT)
 				dev_warn(device, "loading %s failed with error %d\n",
@@ -685,7 +696,7 @@ int assign_fw(struct firmware *fw, struct device *device,
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 			  struct device *device, void *dbuf, size_t size,
-			  enum fw_opt opt_flags)
+			  enum fw_opt opt_flags, size_t offset)
 {
 	struct firmware *firmware;
 	struct fw_priv *fw_priv;
@@ -704,7 +715,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	}
 
 	ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
-				  opt_flags);
+				  opt_flags, offset);
 
 	/*
 	 * bind with 'priv' now to avoid warning in failure path
@@ -751,7 +762,7 @@ static void fw_abort_batch_reqs(struct firmware *fw)
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
 		  struct device *device, void *buf, size_t size,
-		  enum fw_opt opt_flags)
+		  enum fw_opt opt_flags, size_t offset)
 {
 	struct firmware *fw = NULL;
 	int ret;
@@ -765,7 +776,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	}
 
 	ret = _request_firmware_prepare(&fw, name, device, buf, size,
-					opt_flags);
+					opt_flags, offset);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
@@ -825,7 +836,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT);
+				FW_OPT_UEVENT, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -852,7 +863,7 @@ int firmware_request_nowarn(const struct firmware **firmware, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+				FW_OPT_UEVENT | FW_OPT_NO_WARN, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -877,7 +888,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK_SYSFS);
+				FW_OPT_NOFALLBACK_SYSFS, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -914,6 +925,8 @@ EXPORT_SYMBOL_GPL(firmware_request_cache);
  * @device: device for which firmware is being loaded and DMA region allocated
  * @buf: address of buffer to load firmware into
  * @size: size of buffer
+ * @offset: offset into file to read
+ * @pread_flags: KERNEL_PREAD_FLAG_PART to allow partial file read
  *
  * This function works pretty much like request_firmware(), but it doesn't
  * allocate a buffer to hold the firmware data. Instead, the firmware
@@ -924,16 +937,22 @@ EXPORT_SYMBOL_GPL(firmware_request_cache);
  */
 int
 request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
-			  struct device *device, void *buf, size_t size)
+			  struct device *device, void *buf, size_t size,
+			  size_t offset, unsigned int pread_flags)
 {
 	int ret;
+	enum fw_opt opt_flags;
 
 	if (fw_cache_is_setup(device, name))
 		return -EOPNOTSUPP;
 
 	__module_get(THIS_MODULE);
+	opt_flags = FW_OPT_UEVENT | FW_OPT_NOCACHE;
+	if (pread_flags & KERNEL_PREAD_FLAG_PART)
+		opt_flags |= FW_OPT_PARTIAL;
+
 	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_NOCACHE);
+				opt_flags, offset);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -972,7 +991,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	fw_work = container_of(work, struct firmware_work, work);
 
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
-			  fw_work->opt_flags);
+			  fw_work->opt_flags, 0);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193dec55..00f3359f4f61 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -246,8 +246,11 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 		} else if (phdr->p_filesz) {
 			/* Firmware not large enough, load split-out segments */
 			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
-							ptr, phdr->p_filesz);
+			ret = request_firmware_into_buf
+						(&seg_fw, fw_name, dev,
+						 ptr, phdr->p_filesz,
+						 0,
+						 KERNEL_PREAD_FLAG_WHOLE);
 			if (ret) {
 				dev_err(dev, "failed to load %s\n", fw_name);
 				break;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 2dd566c91d44..c81162a8d709 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -4,6 +4,7 @@
 
 #include <linux/types.h>
 #include <linux/compiler.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 
 #define FW_ACTION_NOHOTPLUG 0
@@ -51,7 +52,9 @@ int request_firmware_nowait(
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size);
+			      const char *name, struct device *device,
+			      void *buf, size_t size,
+			      size_t offset, unsigned int pread_flags);
 
 void release_firmware(const struct firmware *fw);
 #else
@@ -89,7 +92,8 @@ static inline int request_firmware_direct(const struct firmware **fw,
 }
 
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size)
+	const char *name, struct device *device, void *buf, size_t size,
+	size_t offset, unsigned int pread_flags);
 {
 	return -EINVAL;
 }
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 251213c872b5..7d1d97fa9a23 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -622,7 +622,9 @@ static int test_fw_run_batch_request(void *data)
 						    req->name,
 						    req->dev,
 						    test_buf,
-						    TEST_FIRMWARE_BUF_SIZE);
+						    TEST_FIRMWARE_BUF_SIZE,
+						    0,
+						    KERNEL_PREAD_FLAG_WHOLE);
 		if (!req->fw)
 			kfree(test_buf);
 	} else {
-- 
2.17.1

[PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf

[Scott Branden]

Add additional hooks to test_firmware to pass in support
for partial file read using request_firmware_into_buf.
buf_size: size of buffer to request firmware into
partial: indicates that a partial file request is being made
file_offset: to indicate offset into file to request

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 lib/test_firmware.c | 141 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 3 deletions(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 7d1d97fa9a23..6050d3113f92 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -49,6 +49,9 @@ struct test_batched_req {
  * @name: the name of the firmware file to look for
  * @into_buf: when the into_buf is used if this is true
  *	request_firmware_into_buf() will be used instead.
+ * @buf_size: size of buf to allocate when into_buf is true
+ * @file_offset: file offset to request when calling request_firmware_into_buf
+ * @partial: partial read flag value when calling request_firmware_into_buf
  * @sync_direct: when the sync trigger is used if this is true
  *	request_firmware_direct() will be used instead.
  * @send_uevent: whether or not to send a uevent for async requests
@@ -88,6 +91,9 @@ struct test_batched_req {
 struct test_config {
 	char *name;
 	bool into_buf;
+	size_t buf_size;
+	size_t file_offset;
+	bool partial;
 	bool sync_direct;
 	bool send_uevent;
 	u8 num_requests;
@@ -182,6 +188,9 @@ static int __test_firmware_config_init(void)
 	test_fw_config->num_requests = TEST_FIRMWARE_NUM_REQS;
 	test_fw_config->send_uevent = true;
 	test_fw_config->into_buf = false;
+	test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE;
+	test_fw_config->file_offset = 0;
+	test_fw_config->partial = false;
 	test_fw_config->sync_direct = false;
 	test_fw_config->req_firmware = request_firmware;
 	test_fw_config->test_result = 0;
@@ -253,6 +262,13 @@ static ssize_t config_show(struct device *dev,
 	len += scnprintf(buf+len, PAGE_SIZE - len,
 			"into_buf:\t\t%s\n",
 			test_fw_config->into_buf ? "true" : "false");
+	len += scnprintf(buf+len, PAGE_SIZE - len,
+			"buf_size:\t%zu\n", test_fw_config->buf_size);
+	len += scnprintf(buf+len, PAGE_SIZE - len,
+			"file_offset:\t%zu\n", test_fw_config->file_offset);
+	len += scnprintf(buf+len, PAGE_SIZE - len,
+			"partial:\t\t%s\n",
+			test_fw_config->partial ? "true" : "false");
 	len += scnprintf(buf+len, PAGE_SIZE - len,
 			"sync_direct:\t\t%s\n",
 			test_fw_config->sync_direct ? "true" : "false");
@@ -322,6 +338,39 @@ test_dev_config_show_bool(char *buf,
 	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
+static int test_dev_config_update_size_t(const char *buf,
+					 size_t size,
+					 size_t *cfg)
+{
+	int ret;
+	long new;
+
+	ret = kstrtol(buf, 10, &new);
+	if (ret)
+		return ret;
+
+	if (new > SIZE_MAX)
+		return -EINVAL;
+
+	mutex_lock(&test_fw_mutex);
+	*(size_t *)cfg = new;
+	mutex_unlock(&test_fw_mutex);
+
+	/* Always return full write size even if we didn't consume all */
+	return size;
+}
+
+static ssize_t test_dev_config_show_size_t(char *buf, int cfg)
+{
+	size_t val;
+
+	mutex_lock(&test_fw_mutex);
+	val = cfg;
+	mutex_unlock(&test_fw_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%zu\n", val);
+}
+
 static ssize_t test_dev_config_show_int(char *buf, int cfg)
 {
 	int val;
@@ -419,6 +468,83 @@ static ssize_t config_into_buf_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(config_into_buf);
 
+static ssize_t config_buf_size_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	int rc;
+
+	mutex_lock(&test_fw_mutex);
+	if (test_fw_config->reqs) {
+		pr_err("Must call release_all_firmware prior to changing config\n");
+		rc = -EINVAL;
+		mutex_unlock(&test_fw_mutex);
+		goto out;
+	}
+	mutex_unlock(&test_fw_mutex);
+
+	rc = test_dev_config_update_size_t(buf, count,
+					   &test_fw_config->buf_size);
+
+out:
+	return rc;
+}
+
+static ssize_t config_buf_size_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	return test_dev_config_show_size_t(buf, test_fw_config->buf_size);
+}
+static DEVICE_ATTR_RW(config_buf_size);
+
+static ssize_t config_file_offset_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	int rc;
+
+	mutex_lock(&test_fw_mutex);
+	if (test_fw_config->reqs) {
+		pr_err("Must call release_all_firmware prior to changing config\n");
+		rc = -EINVAL;
+		mutex_unlock(&test_fw_mutex);
+		goto out;
+	}
+	mutex_unlock(&test_fw_mutex);
+
+	rc = test_dev_config_update_size_t(buf, count,
+					   &test_fw_config->file_offset);
+
+out:
+	return rc;
+}
+
+static ssize_t config_file_offset_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return test_dev_config_show_size_t(buf, test_fw_config->file_offset);
+}
+static DEVICE_ATTR_RW(config_file_offset);
+
+static ssize_t config_partial_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	return test_dev_config_update_bool(buf,
+					   count,
+					   &test_fw_config->partial);
+}
+
+static ssize_t config_partial_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	return test_dev_config_show_bool(buf, test_fw_config->partial);
+}
+static DEVICE_ATTR_RW(config_partial);
+
 static ssize_t config_sync_direct_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t count)
@@ -613,18 +739,24 @@ static int test_fw_run_batch_request(void *data)
 
 	if (test_fw_config->into_buf) {
 		void *test_buf;
+		unsigned int pread_flags;
 
 		test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL);
 		if (!test_buf)
 			return -ENOSPC;
 
+		if (test_fw_config->partial)
+			pread_flags = KERNEL_PREAD_FLAG_PART;
+		else
+			pread_flags = KERNEL_PREAD_FLAG_WHOLE;
+
 		req->rc = request_firmware_into_buf(&req->fw,
 						    req->name,
 						    req->dev,
 						    test_buf,
-						    TEST_FIRMWARE_BUF_SIZE,
-						    0,
-						    KERNEL_PREAD_FLAG_WHOLE);
+						    test_fw_config->buf_size,
+						    test_fw_config->file_offset,
+						    pread_flags);
 		if (!req->fw)
 			kfree(test_buf);
 	} else {
@@ -897,6 +1029,9 @@ static struct attribute *test_dev_attrs[] = {
 	TEST_FW_DEV_ATTR(config_name),
 	TEST_FW_DEV_ATTR(config_num_requests),
 	TEST_FW_DEV_ATTR(config_into_buf),
+	TEST_FW_DEV_ATTR(config_buf_size),
+	TEST_FW_DEV_ATTR(config_file_offset),
+	TEST_FW_DEV_ATTR(config_partial),
 	TEST_FW_DEV_ATTR(config_sync_direct),
 	TEST_FW_DEV_ATTR(config_send_uevent),
 	TEST_FW_DEV_ATTR(config_read_fw_idx),
-- 
2.17.1

[PATCH v2 4/7] firmware: test partial file reads of request_firmware_into_buf

[Scott Branden]

Add firmware tests for partial file reads of request_firmware_into_buf.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 .../selftests/firmware/fw_filesystem.sh       | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 56894477c8bd..e973c658fe1a 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -126,6 +126,26 @@ config_unset_into_buf()
 	echo 0 >  $DIR/config_into_buf
 }
 
+config_set_buf_size()
+{
+	echo $1 >  $DIR/config_buf_size
+}
+
+config_set_file_offset()
+{
+	echo $1 >  $DIR/config_file_offset
+}
+
+config_set_partial()
+{
+	echo 1 >  $DIR/config_partial
+}
+
+config_unset_partial()
+{
+	echo 0 >  $DIR/config_partial
+}
+
 config_set_sync_direct()
 {
 	echo 1 >  $DIR/config_sync_direct
@@ -184,6 +204,35 @@ read_firmwares()
 	done
 }
 
+read_firmwares_partial()
+{
+	if [ "$(cat $DIR/config_into_buf)" == "1" ]; then
+		fwfile="${FW_INTO_BUF}"
+	else
+		fwfile="${FW}"
+	fi
+
+	if [ "$1" = "xzonly" ]; then
+		fwfile="${fwfile}-orig"
+	fi
+
+	# Strip fwfile down to match partial offset and length
+	partial_data="$(cat $fwfile)"
+	partial_data="${partial_data:$2:$3}"
+
+	for i in $(seq 0 3); do
+		config_set_read_fw_idx $i
+
+		read_firmware="$(cat $DIR/read_firmware)"
+
+		# Verify the contents are what we expect.
+		if [ $read_firmware != $partial_data ]; then
+			echo "request #$i: partial firmware was not loaded" >&2
+			exit 1
+		fi
+	done
+}
+
 read_firmwares_expect_nofile()
 {
 	for i in $(seq 0 3); do
@@ -296,6 +345,21 @@ test_batched_request_firmware_into_buf()
 	echo "OK"
 }
 
+test_batched_request_firmware_into_buf_partial()
+{
+	echo -n "Batched request_firmware_into_buf_partial() $2 off=$3 size=$4 try #$1: "
+	config_reset
+	config_set_name $TEST_FIRMWARE_INTO_BUF_FILENAME
+	config_set_into_buf
+	config_set_partial
+	config_set_buf_size $4
+	config_set_file_offset $3
+	config_trigger_sync
+	read_firmwares_partial $2 $3 $4
+	release_all_firmware
+	echo "OK"
+}
+
 test_batched_request_firmware_direct()
 {
 	echo -n "Batched request_firmware_direct() $2 try #$1: "
@@ -348,6 +412,22 @@ for i in $(seq 1 5); do
 	test_batched_request_firmware_into_buf $i normal
 done
 
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 0 10
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 0 5
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 1 6
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 2 10
+done
+
 for i in $(seq 1 5); do
 	test_batched_request_firmware_direct $i normal
 done
-- 
2.17.1

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

[Scott Branden]

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 */
+};
+
+/* 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"
+
+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 */
+	__u64 offset;   /* offset in BAR */
+	__u32 *data;    /* where to read/write data to */
+};
+
+struct vk_reset {
+	__u32 arg1;
+	__u32 arg2;
+};
+
+#define VK_MAGIC              0x5E
+
+/* Load image to Valkyrie */
+#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
+
+/* Read data from Valkyrie */
+#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
+
+/* Send Reset to Valkyrie */
+#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
+
+/*
+ * message block - basic unit in the message where a message's size is always
+ *		   N x sizeof(basic_block)
+ */
+struct vk_msg_blk {
+	__u8 function_id;
+#define VK_FID_TRANS_BUF 5
+#define VK_FID_SHUTDOWN  8
+	__u8 size;
+	__u16 queue_id:4;
+	__u16 msg_id:12;
+	__u32 context_id;
+	__u32 args[2];
+#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
+#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
+#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
+#define VK_CMD_MASK        0x0F00 /* command mask */
+};
+
+#define VK_BAR_FWSTS			0x41C
+/* VK_FWSTS definitions */
+#define VK_FWSTS_RELOCATION_ENTRY	BIT(0)
+#define VK_FWSTS_RELOCATION_EXIT	BIT(1)
+#define VK_FWSTS_INIT_START		BIT(2)
+#define VK_FWSTS_ARCH_INIT_DONE		BIT(3)
+#define VK_FWSTS_PRE_KNL1_INIT_DONE	BIT(4)
+#define VK_FWSTS_PRE_KNL2_INIT_DONE	BIT(5)
+#define VK_FWSTS_POST_KNL_INIT_DONE	BIT(6)
+#define VK_FWSTS_INIT_DONE		BIT(7)
+#define VK_FWSTS_APP_INIT_START		BIT(8)
+#define VK_FWSTS_APP_INIT_DONE		BIT(9)
+#define VK_FWSTS_MASK			0xFFFFFFFF
+#define VK_FWSTS_READY			(VK_FWSTS_INIT_START | \
+					 VK_FWSTS_ARCH_INIT_DONE | \
+					 VK_FWSTS_PRE_KNL1_INIT_DONE | \
+					 VK_FWSTS_PRE_KNL2_INIT_DONE | \
+					 VK_FWSTS_POST_KNL_INIT_DONE | \
+					 VK_FWSTS_INIT_DONE | \
+					 VK_FWSTS_APP_INIT_START | \
+					 VK_FWSTS_APP_INIT_DONE)
+/* Deinit */
+#define VK_FWSTS_APP_DEINIT_START	BIT(23)
+#define VK_FWSTS_APP_DEINIT_DONE	BIT(24)
+#define VK_FWSTS_DRV_DEINIT_START	BIT(25)
+#define VK_FWSTS_DRV_DEINIT_DONE	BIT(26)
+#define VK_FWSTS_RESET_DONE		BIT(27)
+#define VK_FWSTS_DEINIT_TRIGGERED	(VK_FWSTS_APP_DEINIT_START | \
+					 VK_FWSTS_APP_DEINIT_DONE  | \
+					 VK_FWSTS_DRV_DEINIT_START | \
+					 VK_FWSTS_DRV_DEINIT_DONE)
+/* Last nibble for reboot reason */
+#define VK_FWSTS_RESET_REASON_SHIFT	28
+#define VK_FWSTS_RESET_REASON_MASK	(0xF << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_SYS_PWRUP	(0x0 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_MBOX_DB		(0x1 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_M7_WDOG		(0x2 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_TEMP		(0x3 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_FLR		(0x4 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_HOT		(0x5 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_WARM		(0x6 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_COLD		(0x7 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_L1		(0x8 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_L0		(0x9 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_UNKNOWN		(0xF << VK_FWSTS_RESET_REASON_SHIFT)
+
+#endif /* __UAPI_LINUX_MISC_BCM_VK_H */
-- 
2.17.1

[PATCH v2 7/7] MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver

[Scott Branden]

Add maintainer entry for new Broadcom VK Driver

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4beb8dc4c7eb..c55f34f00b85 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3564,6 +3564,13 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	drivers/net/ethernet/broadcom/tg3.*
 
+BROADCOM VK DRIVER
+M:	Scott Branden <scott.branden@broadcom.com>
+L:	bcm-kernel-feedback-list@broadcom.com
+S:	Supported
+F:	drivers/misc/bcm-vk/
+F:	include/uapi/linux/misc/bcm_vk.h
+
 BROCADE BFA FC SCSI DRIVER
 M:	Anil Gurumurthy <anil.gurumurthy@qlogic.com>
 M:	Sudarsana Kalluru <sudarsana.kalluru@qlogic.com>
-- 
2.17.1

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Randy Dunlap]

Hi,

On 2/19/20 4:48 PM, Scott Branden wrote:
> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> new file mode 100644
> index 000000000000..c75dfb89a38d
> --- /dev/null
> +++ b/drivers/misc/bcm-vk/Kconfig
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Broadcom VK device
> +#
> +config BCM_VK
> +	tristate "Support for Broadcom VK Accelerators"
> +	depends on PCI_MSI
> +	default m

Need to justify default m. Normally we don't add drivers as enabled unless
they are required for basic (boot) operation.

> +	help
> +	  Select this option to enable support for Broadcom
> +	  VK Accelerators.  VK is used for performing
> +	  specific video offload processing.  This driver enables
> +	  userspace programs to access these accelerators via /dev/bcm-vk.N
> +	  devices.
> +
> +	  If unsure, say N.
> +
> +if BCM_VK
> +
> +config BCM_VK_H2VK_VERIFY_AND_RETRY
> +	bool "Host To VK Verifiy Data and Retry"

	                 Verify

> +	help
> +	  Turn on to verify the data passed down to VK is good,
> +	  and if not, do a retry until it succeeds.

No timeout on that retry?

> +	  This is a debug/workaround on FPGA PCIe timing issues
> +	  but may be found useful for debugging other PCIe hardware issues.
> +	  Small performance loss by enabling this debug config.
> +	  For properly operating PCIe hardware no need to enable this.
> +
> +	  If unsure, say N.
> +
> +config BCM_VK_QSTATS
> +	bool "VK Queue Statistics"
> +	help
> +	  Turn on to enable Queue Statistics.
> +	  These are useful for debugging purposes.
> +	  Some performance loss by enabling this debug config.
> +	  For properly operating PCIe hardware no need to enable this.
> +
> +	  If unsure, say N.
> +
> +endif

cheers.
-- 
~Randy

Re: [PATCH v2 2/7] firmware: add offset to request_firmware_into_buf

[Luis Chamberlain]

On Wed, Feb 19, 2020 at 04:48:20PM -0800, Scott Branden wrote:
> Add offset to request_firmware_into_buf to allow for portions
> of firmware file to be read into a buffer.  Necessary where firmware
> needs to be loaded in portions from file in memory constrained systems.
> 
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>

Thanks for following up Scott! However you failed to address the
feedback last time by Takashi, so until then, this remains a blocker for
this.

http://lkml.kernel.org/r/s5hwoeyj3i5.wl-tiwai@suse.de

  Luis

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Greg Kroah-Hartman]

On Wed, Feb 19, 2020 at 04:48:24PM -0800, Scott Branden wrote:
> Add Broadcom VK driver offload engine.
> This driver interfaces to the VK PCIe offload engine to perform
> should offload functions as video transcoding on multiple streams
> in parallel.  VK device is booted from files loaded using
> request_firmware_into_buf mechanism.  After booted card status is updated
> and messages can then be sent to the card.
> Such messages contain scatter gather list of addresses
> to pull data from the host to perform operations on.

Why is this a tty driver?

Have you worked with the V4L developers to tie this into the proper
in-kernel apis for this type of functionality?

Using a tty driver seems like the totally incorrect way to do this, what
am I missing?

Also, do not make up random error values, you return "-1" a lot here,
that is not ok.  Please fix up to return the correct -Ewhatever values
instead.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

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?

> +};
> +
> +/* 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?

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

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

> +};

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?

> +
> +struct vk_reset {
> +	__u32 arg1;
> +	__u32 arg2;
> +};
> +
> +#define VK_MAGIC              0x5E
> +
> +/* Load image to Valkyrie */
> +#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
> +
> +/* Read data from Valkyrie */
> +#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
> +
> +/* Send Reset to Valkyrie */
> +#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
> +
> +/*
> + * message block - basic unit in the message where a message's size is always
> + *		   N x sizeof(basic_block)
> + */
> +struct vk_msg_blk {
> +	__u8 function_id;
> +#define VK_FID_TRANS_BUF 5
> +#define VK_FID_SHUTDOWN  8
> +	__u8 size;
> +	__u16 queue_id:4;
> +	__u16 msg_id:12;

Do not use bitfields in ioctls, they will not work properly on all
systems.  Use masks and shifts instead.

> +	__u32 context_id;
> +	__u32 args[2];
> +#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
> +#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
> +#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
> +#define VK_CMD_MASK        0x0F00 /* command mask */
> +};
> +
> +#define VK_BAR_FWSTS			0x41C
> +/* VK_FWSTS definitions */
> +#define VK_FWSTS_RELOCATION_ENTRY	BIT(0)
> +#define VK_FWSTS_RELOCATION_EXIT	BIT(1)
> +#define VK_FWSTS_INIT_START		BIT(2)
> +#define VK_FWSTS_ARCH_INIT_DONE		BIT(3)
> +#define VK_FWSTS_PRE_KNL1_INIT_DONE	BIT(4)
> +#define VK_FWSTS_PRE_KNL2_INIT_DONE	BIT(5)
> +#define VK_FWSTS_POST_KNL_INIT_DONE	BIT(6)
> +#define VK_FWSTS_INIT_DONE		BIT(7)
> +#define VK_FWSTS_APP_INIT_START		BIT(8)
> +#define VK_FWSTS_APP_INIT_DONE		BIT(9)

I do not think that BIT() is exported to userspace properly, is it
really ok here?

> +#define VK_FWSTS_MASK			0xFFFFFFFF
> +#define VK_FWSTS_READY			(VK_FWSTS_INIT_START | \
> +					 VK_FWSTS_ARCH_INIT_DONE | \
> +					 VK_FWSTS_PRE_KNL1_INIT_DONE | \
> +					 VK_FWSTS_PRE_KNL2_INIT_DONE | \
> +					 VK_FWSTS_POST_KNL_INIT_DONE | \
> +					 VK_FWSTS_INIT_DONE | \
> +					 VK_FWSTS_APP_INIT_START | \
> +					 VK_FWSTS_APP_INIT_DONE)
> +/* Deinit */
> +#define VK_FWSTS_APP_DEINIT_START	BIT(23)
> +#define VK_FWSTS_APP_DEINIT_DONE	BIT(24)
> +#define VK_FWSTS_DRV_DEINIT_START	BIT(25)
> +#define VK_FWSTS_DRV_DEINIT_DONE	BIT(26)
> +#define VK_FWSTS_RESET_DONE		BIT(27)
> +#define VK_FWSTS_DEINIT_TRIGGERED	(VK_FWSTS_APP_DEINIT_START | \
> +					 VK_FWSTS_APP_DEINIT_DONE  | \
> +					 VK_FWSTS_DRV_DEINIT_START | \
> +					 VK_FWSTS_DRV_DEINIT_DONE)
> +/* Last nibble for reboot reason */
> +#define VK_FWSTS_RESET_REASON_SHIFT	28
> +#define VK_FWSTS_RESET_REASON_MASK	(0xF << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_SYS_PWRUP	(0x0 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_MBOX_DB		(0x1 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_M7_WDOG		(0x2 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_TEMP		(0x3 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_FLR		(0x4 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_HOT		(0x5 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_WARM		(0x6 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_COLD		(0x7 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_L1		(0x8 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_L0		(0x9 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_UNKNOWN		(0xF << VK_FWSTS_RESET_REASON_SHIFT)

What are all of these #defines doing in an uapi file?  How is userspace
going to use them?

thanks,

greg k-h

Re: [PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf

[Dan Carpenter]

On Wed, Feb 19, 2020 at 04:48:21PM -0800, Scott Branden wrote:
> +static int test_dev_config_update_size_t(const char *buf,
> +					 size_t size,
> +					 size_t *cfg)
> +{
> +	int ret;
> +	long new;
> +
> +	ret = kstrtol(buf, 10, &new);
> +	if (ret)
> +		return ret;
> +
> +	if (new > SIZE_MAX)

This "new" variable is long and SIZE_MAX is ULONG_MAX so the condition
can't be true.

> +		return -EINVAL;
> +
> +	mutex_lock(&test_fw_mutex);
> +	*(size_t *)cfg = new;
> +	mutex_unlock(&test_fw_mutex);
> +
> +	/* Always return full write size even if we didn't consume all */
> +	return size;
> +}
> +
> +static ssize_t test_dev_config_show_size_t(char *buf, int cfg)
> +{
> +	size_t val;
> +
> +	mutex_lock(&test_fw_mutex);
> +	val = cfg;
> +	mutex_unlock(&test_fw_mutex);

Both val and cfg are stack variables so there is no need for locking.
Probably you meant to pass a pointer to cfg?

> +
> +	return snprintf(buf, PAGE_SIZE, "%zu\n", val);
> +}
> +
>  static ssize_t test_dev_config_show_int(char *buf, int cfg)
>  {
>  	int val;

regards,
dan carpenter

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Scott Branden]

Hi Randy,

On 2020-02-19 5:04 p.m., Randy Dunlap wrote:
> Hi,
>
> On 2/19/20 4:48 PM, Scott Branden wrote:
>> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
>> new file mode 100644
>> index 000000000000..c75dfb89a38d
>> --- /dev/null
>> +++ b/drivers/misc/bcm-vk/Kconfig
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Broadcom VK device
>> +#
>> +config BCM_VK
>> +	tristate "Support for Broadcom VK Accelerators"
>> +	depends on PCI_MSI
>> +	default m
> Need to justify default m. Normally we don't add drivers as enabled unless
> they are required for basic (boot) operation.
Will remove default m as not needed to boot.  Interesting other offload 
engines misc/ocxl/Kconfig and misc/cxl/Kconfig have default m.
>
>> +	help
>> +	  Select this option to enable support for Broadcom
>> +	  VK Accelerators.  VK is used for performing
>> +	  specific video offload processing.  This driver enables
>> +	  userspace programs to access these accelerators via /dev/bcm-vk.N
>> +	  devices.
>> +
>> +	  If unsure, say N.
>> +
>> +if BCM_VK
>> +
>> +config BCM_VK_H2VK_VERIFY_AND_RETRY
>> +	bool "Host To VK Verifiy Data and Retry"
> 	                 Verify
>
>> +	help
>> +	  Turn on to verify the data passed down to VK is good,
>> +	  and if not, do a retry until it succeeds.
> No timeout on that retry?
This is only enabled for debug purposes or fpga workarounds - no need 
for a timeout.
>
>> +	  This is a debug/workaround on FPGA PCIe timing issues
>> +	  but may be found useful for debugging other PCIe hardware issues.
>> +	  Small performance loss by enabling this debug config.
>> +	  For properly operating PCIe hardware no need to enable this.
>> +
>> +	  If unsure, say N.
>> +
>> +config BCM_VK_QSTATS
>> +	bool "VK Queue Statistics"
>> +	help
>> +	  Turn on to enable Queue Statistics.
>> +	  These are useful for debugging purposes.
>> +	  Some performance loss by enabling this debug config.
>> +	  For properly operating PCIe hardware no need to enable this.
>> +
>> +	  If unsure, say N.
>> +
>> +endif
> cheers.

Re: [PATCH v2 2/7] firmware: add offset to request_firmware_into_buf

[Scott Branden]

Hi Luis,

On 2020-02-19 5:22 p.m., Luis Chamberlain wrote:
> On Wed, Feb 19, 2020 at 04:48:20PM -0800, Scott Branden wrote:
>> Add offset to request_firmware_into_buf to allow for portions
>> of firmware file to be read into a buffer.  Necessary where firmware
>> needs to be loaded in portions from file in memory constrained systems.
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> Thanks for following up Scott! However you failed to address the
> feedback last time by Takashi, so until then, this remains a blocker for
> this.
>
> http://lkml.kernel.org/r/s5hwoeyj3i5.wl-tiwai@suse.de
>
>    Luis
I responded to the email query.  Hopefully this addresses your concern.

Regards,
  Scott

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Randy Dunlap]

On 2/20/20 4:06 PM, Scott Branden wrote:
> Hi Randy,
> 
> On 2020-02-19 5:04 p.m., Randy Dunlap wrote:
>> Hi,
>>
>> On 2/19/20 4:48 PM, Scott Branden wrote:
>>> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
>>> new file mode 100644
>>> index 000000000000..c75dfb89a38d
>>> --- /dev/null
>>> +++ b/drivers/misc/bcm-vk/Kconfig
>>> @@ -0,0 +1,42 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +#
>>> +# Broadcom VK device
>>> +#
>>> +config BCM_VK
>>> +    tristate "Support for Broadcom VK Accelerators"
>>> +    depends on PCI_MSI
>>> +    default m
>> Need to justify default m. Normally we don't add drivers as enabled unless
>> they are required for basic (boot) operation.
> Will remove default m as not needed to boot.  Interesting other offload engines misc/ocxl/Kconfig and misc/cxl/Kconfig have default m.

Thanks.

Uh, yes, that is interesting.  They both depend on PPC_POWERNV,
which I know nothing about.

>>> +    help
>>> +      Select this option to enable support for Broadcom
>>> +      VK Accelerators.  VK is used for performing
>>> +      specific video offload processing.  This driver enables
>>> +      userspace programs to access these accelerators via /dev/bcm-vk.N
>>> +      devices.
>>> +
>>> +      If unsure, say N.

-- 
~Randy

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

[Scott Branden]

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.
>
>> +};
>> +
>> +/* 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.
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.
I guess I could change the API at this point to lookup the default 
filename if NULL filename passed into ioctl.
>> +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.
But, VK_IOCTL_ACCESS_BAR can go away entirely if standard user space 
approach already exists as you imply.
>> +};
> 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?
>
>> +
>> +struct vk_reset {
>> +	__u32 arg1;
>> +	__u32 arg2;
>> +};
>> +
>> +#define VK_MAGIC              0x5E
>> +
>> +/* Load image to Valkyrie */
>> +#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
>> +
>> +/* Read data from Valkyrie */
>> +#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
>> +
>> +/* Send Reset to Valkyrie */
>> +#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
>> +
>> +/*
>> + * message block - basic unit in the message where a message's size is always
>> + *		   N x sizeof(basic_block)
>> + */
>> +struct vk_msg_blk {
>> +	__u8 function_id;
>> +#define VK_FID_TRANS_BUF 5
>> +#define VK_FID_SHUTDOWN  8
>> +	__u8 size;
>> +	__u16 queue_id:4;
>> +	__u16 msg_id:12;
> Do not use bitfields in ioctls, they will not work properly on all
> systems.  Use masks and shifts instead.
I don't like the bitfields either - structure inherited from firmware code.
Will work on getting these removed.
>
>> +	__u32 context_id;
>> +	__u32 args[2];
>> +#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
>> +#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
>> +#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
>> +#define VK_CMD_MASK        0x0F00 /* command mask */
>> +};
>> +
>> +#define VK_BAR_FWSTS			0x41C
>> +/* VK_FWSTS definitions */
>> +#define VK_FWSTS_RELOCATION_ENTRY	BIT(0)
>> +#define VK_FWSTS_RELOCATION_EXIT	BIT(1)
>> +#define VK_FWSTS_INIT_START		BIT(2)
>> +#define VK_FWSTS_ARCH_INIT_DONE		BIT(3)
>> +#define VK_FWSTS_PRE_KNL1_INIT_DONE	BIT(4)
>> +#define VK_FWSTS_PRE_KNL2_INIT_DONE	BIT(5)
>> +#define VK_FWSTS_POST_KNL_INIT_DONE	BIT(6)
>> +#define VK_FWSTS_INIT_DONE		BIT(7)
>> +#define VK_FWSTS_APP_INIT_START		BIT(8)
>> +#define VK_FWSTS_APP_INIT_DONE		BIT(9)
> I do not think that BIT() is exported to userspace properly, is it
> really ok here?
Works fine.  Also in uapi/linux/rtc.h.
>
>> +#define VK_FWSTS_MASK			0xFFFFFFFF
>> +#define VK_FWSTS_READY			(VK_FWSTS_INIT_START | \
>> +					 VK_FWSTS_ARCH_INIT_DONE | \
>> +					 VK_FWSTS_PRE_KNL1_INIT_DONE | \
>> +					 VK_FWSTS_PRE_KNL2_INIT_DONE | \
>> +					 VK_FWSTS_POST_KNL_INIT_DONE | \
>> +					 VK_FWSTS_INIT_DONE | \
>> +					 VK_FWSTS_APP_INIT_START | \
>> +					 VK_FWSTS_APP_INIT_DONE)
>> +/* Deinit */
>> +#define VK_FWSTS_APP_DEINIT_START	BIT(23)
>> +#define VK_FWSTS_APP_DEINIT_DONE	BIT(24)
>> +#define VK_FWSTS_DRV_DEINIT_START	BIT(25)
>> +#define VK_FWSTS_DRV_DEINIT_DONE	BIT(26)
>> +#define VK_FWSTS_RESET_DONE		BIT(27)
>> +#define VK_FWSTS_DEINIT_TRIGGERED	(VK_FWSTS_APP_DEINIT_START | \
>> +					 VK_FWSTS_APP_DEINIT_DONE  | \
>> +					 VK_FWSTS_DRV_DEINIT_START | \
>> +					 VK_FWSTS_DRV_DEINIT_DONE)
>> +/* Last nibble for reboot reason */
>> +#define VK_FWSTS_RESET_REASON_SHIFT	28
>> +#define VK_FWSTS_RESET_REASON_MASK	(0xF << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_SYS_PWRUP	(0x0 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_MBOX_DB		(0x1 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_M7_WDOG		(0x2 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_TEMP		(0x3 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_FLR		(0x4 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_HOT		(0x5 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_WARM		(0x6 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_COLD		(0x7 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_L1		(0x8 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_L0		(0x9 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_UNKNOWN		(0xF << VK_FWSTS_RESET_REASON_SHIFT)
> What are all of these #defines doing in an uapi file?  How is userspace
> going to use them?
There are actually 2 linux user spaces that use this header.
One is the x86 host with the bcm-vk PCI driver.
The x86 host user space could use them to check the firmware status and 
find out what state VK is in.

The other user space is a coprocessor inside the VK SOC.
The app running in user space needs to know the state of the FWSTS in 
order to proceed.
It includes this header in its user space app (even though it doesn't 
user the linux driver, it needs access to the same FWSTS register 
directly).
> thanks,
>
> greg k-h

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

[Arnd Bergmann]

On Fri, Feb 21, 2020 at 2:16 AM Scott Branden
<scott.branden@broadcom.com> 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.

        Arnd

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@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

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Scott Branden]

On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
> On Wed, Feb 19, 2020 at 04:48:24PM -0800, Scott Branden wrote:
>> Add Broadcom VK driver offload engine.
>> This driver interfaces to the VK PCIe offload engine to perform
>> should offload functions as video transcoding on multiple streams
>> in parallel.  VK device is booted from files loaded using
>> request_firmware_into_buf mechanism.  After booted card status is updated
>> and messages can then be sent to the card.
>> Such messages contain scatter gather list of addresses
>> to pull data from the host to perform operations on.
> Why is this a tty driver?
We have a tty driver here as there are (multiple) console interfaces to 
the card.
The only viable interface to the card is through the PCIe bus.  We can't 
hook up cables to the card to get at the consoles.
As such we implemented a tty driver to access the console via a circular 
buffer in PCIe BAR space.

It is extremely useful.  You get console access to virtual serial ports 
connected to each processor inside the VK SoC.
Future enhancement is to connect the tty driver to use an MSIX interrupt 
rather than polling.
Once that is done lrz/sz transfers will be much quicker.  I'd also look 
at if I could use ser2net to get network access
for the processors on the VK SoC over this interface as well.
>
> Have you worked with the V4L developers to tie this into the proper
> in-kernel apis for this type of functionality?
We looked at the V4L model doesn't have any support for anything we are 
doing in this driver.
We also want a driver that doesn't care about video.  It could be 
offloading crypto or other operations.
We talked with Olof about all of this previously and he said leave it as 
a misc driver for now.
He was going to discuss at linux plumbers conference that we need some 
sort of offload engine model that such devices could fit into.
> Using a tty driver seems like the totally incorrect way to do this, what
> am I missing?
tty driver is used to provide console access to the processors running 
on vk.
Data is sent using the bcm_vk_msg interface by read/write operations 
from user space.
VK then gets the messages and DMA's the data to/from host memory when 
needed to process.
>
> Also, do not make up random error values, you return "-1" a lot here,
> that is not ok.  Please fix up to return the correct -Ewhatever values
> instead.
OK.
>
> thanks,
>
> greg k-h

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

Re: [PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf

[Scott Branden]

Hi Dan,

Thanks for your review and valuable comments.
Will have to investigate fully and correct anything wrong.

On 2020-02-20 12:42 a.m., Dan Carpenter wrote:
> On Wed, Feb 19, 2020 at 04:48:21PM -0800, Scott Branden wrote:
>> +static int test_dev_config_update_size_t(const char *buf,
>> +					 size_t size,
>> +					 size_t *cfg)
>> +{
>> +	int ret;
>> +	long new;
>> +
>> +	ret = kstrtol(buf, 10, &new);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (new > SIZE_MAX)
> This "new" variable is long and SIZE_MAX is ULONG_MAX so the condition
> can't be true.
>
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&test_fw_mutex);
>> +	*(size_t *)cfg = new;
>> +	mutex_unlock(&test_fw_mutex);
>> +
>> +	/* Always return full write size even if we didn't consume all */
>> +	return size;
>> +}
>> +
>> +static ssize_t test_dev_config_show_size_t(char *buf, int cfg)
>> +{
>> +	size_t val;
>> +
>> +	mutex_lock(&test_fw_mutex);
>> +	val = cfg;
>> +	mutex_unlock(&test_fw_mutex);
> Both val and cfg are stack variables so there is no need for locking.
> Probably you meant to pass a pointer to cfg?
>
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%zu\n", val);
>> +}
>> +
>>   static ssize_t test_dev_config_show_int(char *buf, int cfg)
>>   {
>>   	int val;
> regards,
> dan carpenter
>
>

Re: [PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf

[Scott Branden]

Reponses inline.

Luis - please have a look as well.

On 2020-02-21 10:30 a.m., Scott Branden wrote:
> Hi Dan,
>
> Thanks for your review and valuable comments.
> Will have to investigate fully and correct anything wrong.
>
> On 2020-02-20 12:42 a.m., Dan Carpenter wrote:
>> On Wed, Feb 19, 2020 at 04:48:21PM -0800, Scott Branden wrote:
>>> +static int test_dev_config_update_size_t(const char *buf,
>>> +                     size_t size,
>>> +                     size_t *cfg)
>>> +{
>>> +    int ret;
>>> +    long new;
>>> +
>>> +    ret = kstrtol(buf, 10, &new);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (new > SIZE_MAX)
>> This "new" variable is long and SIZE_MAX is ULONG_MAX so the condition
>> can't be true.
Removed the check.
>>
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&test_fw_mutex);
>>> +    *(size_t *)cfg = new;
>>> +    mutex_unlock(&test_fw_mutex);
>>> +
>>> +    /* Always return full write size even if we didn't consume all */
>>> +    return size;
>>> +}
>>> +
>>> +static ssize_t test_dev_config_show_size_t(char *buf, int cfg)
>>> +{
>>> +    size_t val;
>>> +
>>> +    mutex_lock(&test_fw_mutex);
>>> +    val = cfg;
>>> +    mutex_unlock(&test_fw_mutex);
>> Both val and cfg are stack variables so there is no need for locking.
>> Probably you meant to pass a pointer to cfg?
I am following the existing code as was done for
test_dev_config_show_bool(),
test_dev_config_show_int(),
test_dev_config_show_u8()

Mutex probably not needed but I don't think I need to deviate from the 
rest of the test code.

Luis, could you please explain what the rest of your code is doing?
>>
>>> +
>>> +    return snprintf(buf, PAGE_SIZE, "%zu\n", val);
>>> +}
>>> +
>>>   static ssize_t test_dev_config_show_int(char *buf, int cfg)
>>>   {
>>>       int val;
>> regards,
>> dan carpenter
>>
>>
>

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Arnd Bergmann]

On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
<scott.branden@broadcom.com> wrote:
> On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:

> > Have you worked with the V4L developers to tie this into the proper
> > in-kernel apis for this type of functionality?
> We looked at the V4L model doesn't have any support for anything we are
> doing in this driver.
> We also want a driver that doesn't care about video.  It could be
> offloading crypto or other operations.
> We talked with Olof about all of this previously and he said leave it as
> a misc driver for now.
> He was going to discuss at linux plumbers conference that we need some
> sort of offload engine model that such devices could fit into.

I see. Have you looked at the "uacce" driver submission? It seems
theirs is similar enough that there might be some way to share interfaces.

> > Using a tty driver seems like the totally incorrect way to do this, what
> > am I missing?
> tty driver is used to provide console access to the processors running
> on vk.
> Data is sent using the bcm_vk_msg interface by read/write operations
> from user space.
> VK then gets the messages and DMA's the data to/from host memory when
> needed to process.

In turn here, it sounds like you'd want to look at what drivers/misc/mic/
and the mellanox bluefield drivers are doing. As I understand, they have the
same requirements for console, but have a nicer approach of providing
abstract 'virtio' channels between the PCIe endpoint and the host, and
then run regular virtio based drivers (console, tty, block, filesystem,
network, ...) along with application specific ones to provide the custom
high-level protocols. This is also similar to what the drivers/pci/endpoint
(from the other end) as the drivers/ntb (pci host on both ends) frameworks
and of course the rpmsg/remoteproc framework do.

In the long run, I would want much more consolidation between the
low-level parts of all these frameworks, but moving your high-level
protocols to the same virtio method would sound like a step in the
direction towards a generialized framework and easier sharing of
the abstractions.

       Arnd

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[kbuild test robot]

Hi Scott,

I love your patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on kselftest/next linus/master v5.6-rc2]
[cannot apply to driver-core/driver-core-testing next-20200221]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Scott-Branden/firmware-add-partial-read-support-in-request_firmware_into_buf/20200222-032851
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 06e33595e96f212811066df192ae8bf802174296
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-173-ge0787745-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/misc/bcm-vk/bcm_vk_dev.c:34:29: sparse: sparse: symbol 'image_tab' was not declared. Should it be static?
>> drivers/misc/bcm-vk/bcm_vk_dev.c:185:15: sparse: sparse: incorrect type in assignment (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:185:15: sparse:    expected struct bcm_vk_peer_log *p_ctl
>> drivers/misc/bcm-vk/bcm_vk_dev.c:185:15: sparse:    got void [noderef] <asn:2> *
   drivers/misc/bcm-vk/bcm_vk_dev.c:415:22: sparse: sparse: incorrect type in assignment (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:415:22: sparse:    expected void *bufp
   drivers/misc/bcm-vk/bcm_vk_dev.c:415:22: sparse:    got void [noderef] <asn:2> *
>> drivers/misc/bcm-vk/bcm_vk_dev.c:654:36: sparse: sparse: incorrect type in argument 2 (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:654:36: sparse:    expected void const [noderef] <asn:1> *from
>> drivers/misc/bcm-vk/bcm_vk_dev.c:654:36: sparse:    got struct vk_image *arg
   drivers/misc/bcm-vk/bcm_vk_dev.c:694:37: sparse: sparse: incorrect type in argument 2 (different address spaces)
   drivers/misc/bcm-vk/bcm_vk_dev.c:694:37: sparse:    expected void const [noderef] <asn:1> *from
>> drivers/misc/bcm-vk/bcm_vk_dev.c:694:37: sparse:    got struct vk_access *arg
   drivers/misc/bcm-vk/bcm_vk_dev.c:717:31: sparse: sparse: incorrect type in argument 1 (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:717:31: sparse:    expected void const volatile [noderef] <asn:1> *
>> drivers/misc/bcm-vk/bcm_vk_dev.c:717:31: sparse:    got unsigned int [usertype] *
   drivers/misc/bcm-vk/bcm_vk_dev.c:728:31: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/misc/bcm-vk/bcm_vk_dev.c:728:31: sparse:    expected void const volatile [noderef] <asn:1> *
   drivers/misc/bcm-vk/bcm_vk_dev.c:728:31: sparse:    got unsigned int [usertype] *
   drivers/misc/bcm-vk/bcm_vk_dev.c:793:36: sparse: sparse: incorrect type in argument 2 (different address spaces)
   drivers/misc/bcm-vk/bcm_vk_dev.c:793:36: sparse:    expected void const [noderef] <asn:1> *from
>> drivers/misc/bcm-vk/bcm_vk_dev.c:793:36: sparse:    got struct vk_reset *arg
   drivers/misc/bcm-vk/bcm_vk_dev.c:873:45: sparse: sparse: incorrect type in argument 2 (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:873:45: sparse:    expected struct vk_image *arg
>> drivers/misc/bcm-vk/bcm_vk_dev.c:873:45: sparse:    got void [noderef] <asn:1> *argp
   drivers/misc/bcm-vk/bcm_vk_dev.c:877:45: sparse: sparse: incorrect type in argument 2 (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:877:45: sparse:    expected struct vk_access *arg
   drivers/misc/bcm-vk/bcm_vk_dev.c:877:45: sparse:    got void [noderef] <asn:1> *argp
   drivers/misc/bcm-vk/bcm_vk_dev.c:881:40: sparse: sparse: incorrect type in argument 2 (different address spaces)
>> drivers/misc/bcm-vk/bcm_vk_dev.c:881:40: sparse:    expected struct vk_reset *arg
   drivers/misc/bcm-vk/bcm_vk_dev.c:881:40: sparse:    got void [noderef] <asn:1> *argp
--
>> drivers/misc/bcm-vk/bcm_vk_msg.c:102:6: sparse: sparse: symbol 'bcm_vk_update_qstats' was not declared. Should it be static?
>> drivers/misc/bcm-vk/bcm_vk_msg.c:455:17: sparse: sparse: cast removes address space '<asn:2>' of expression
   drivers/misc/bcm-vk/bcm_vk_msg.c:656:15: sparse: sparse: cast removes address space '<asn:2>' of expression
   drivers/misc/bcm-vk/bcm_vk_msg.c:664:23: sparse: sparse: cast removes address space '<asn:2>' of expression
>> drivers/misc/bcm-vk/bcm_vk_msg.c:734:5: sparse: sparse: symbol 'bcm_vk_handle_last_sess' was not declared. Should it be static?
   drivers/misc/bcm-vk/bcm_vk_msg.c:823:31: sparse: sparse: cast removes address space '<asn:2>' of expression
   drivers/misc/bcm-vk/bcm_vk_msg.c:851:47: sparse: sparse: cast removes address space '<asn:2>' of expression

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[RFC PATCH] misc: bcm-vk: image_tab[] can be static

[kbuild test robot]

Fixes: b6354450d21b ("misc: bcm-vk: add Broadcom VK driver")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 bcm_vk_dev.c |    2 +-
 bcm_vk_msg.c |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
index d8f59e898ca8a..24cdab04c8703 100644
--- a/drivers/misc/bcm-vk/bcm_vk_dev.c
+++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
@@ -31,7 +31,7 @@ enum soc_idx {
 };
 
 #define NUM_BOOT_STAGES 2
-const struct load_image_tab image_tab[][NUM_BOOT_STAGES] = {
+static const struct load_image_tab image_tab[][NUM_BOOT_STAGES] = {
 	[VALKYRIE] = {
 		{VK_IMAGE_TYPE_BOOT1, VK_BOOT1_DEF_VALKYRIE_FILENAME},
 		{VK_IMAGE_TYPE_BOOT2, VK_BOOT2_DEF_VALKYRIE_FILENAME}
diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c
index 651fba2149e28..9247d4bc50288 100644
--- a/drivers/misc/bcm-vk/bcm_vk_msg.c
+++ b/drivers/misc/bcm-vk/bcm_vk_msg.c
@@ -99,8 +99,8 @@ static void bcm_vk_h2vk_verify_blk
 #define BCM_VK_QSTATS_ACC_CNT 20000
 #endif
 
-void bcm_vk_update_qstats(struct bcm_vk *vk, const char *tag,
-			  struct bcm_vk_qstats *qstats, uint32_t occupancy)
+static void bcm_vk_update_qstats(struct bcm_vk *vk, const char *tag,
+				 struct bcm_vk_qstats *qstats, uint32_t occupancy)
 {
 	struct bcm_vk_qs_cnts *qcnts = &qstats->qcnts;
 
@@ -731,7 +731,7 @@ int bcm_vk_send_shutdown_msg(struct bcm_vk *vk, uint32_t shut_type,
 	return rc;
 }
 
-int bcm_vk_handle_last_sess(struct bcm_vk *vk, struct task_struct *ppid)
+static int bcm_vk_handle_last_sess(struct bcm_vk *vk, struct task_struct *ppid)
 {
 	int rc = 0;
 	pid_t pid = task_pid_nr(ppid);

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Greg Kroah-Hartman]

On Sat, Feb 22, 2020 at 09:02:44AM +0100, Arnd Bergmann wrote:
> On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
> <scott.branden@broadcom.com> wrote:
> > On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
> 
> > > Have you worked with the V4L developers to tie this into the proper
> > > in-kernel apis for this type of functionality?
> > We looked at the V4L model doesn't have any support for anything we are
> > doing in this driver.
> > We also want a driver that doesn't care about video.  It could be
> > offloading crypto or other operations.
> > We talked with Olof about all of this previously and he said leave it as
> > a misc driver for now.
> > He was going to discuss at linux plumbers conference that we need some
> > sort of offload engine model that such devices could fit into.
> 
> I see. Have you looked at the "uacce" driver submission? It seems
> theirs is similar enough that there might be some way to share interfaces.
> 
> > > Using a tty driver seems like the totally incorrect way to do this, what
> > > am I missing?
> > tty driver is used to provide console access to the processors running
> > on vk.
> > Data is sent using the bcm_vk_msg interface by read/write operations
> > from user space.
> > VK then gets the messages and DMA's the data to/from host memory when
> > needed to process.
> 
> In turn here, it sounds like you'd want to look at what drivers/misc/mic/
> and the mellanox bluefield drivers are doing. As I understand, they have the
> same requirements for console, but have a nicer approach of providing
> abstract 'virtio' channels between the PCIe endpoint and the host, and
> then run regular virtio based drivers (console, tty, block, filesystem,
> network, ...) along with application specific ones to provide the custom
> high-level protocols. This is also similar to what the drivers/pci/endpoint
> (from the other end) as the drivers/ntb (pci host on both ends) frameworks
> and of course the rpmsg/remoteproc framework do.
> 
> In the long run, I would want much more consolidation between the
> low-level parts of all these frameworks, but moving your high-level
> protocols to the same virtio method would sound like a step in the
> direction towards a generialized framework and easier sharing of
> the abstractions.

I agree, please do not override the generic tty api with something so
hardware-specific like this as it really is not a serial device here.

thanks,

greg k-h

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Olof Johansson]

On Sat, Feb 22, 2020 at 12:03 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
> <scott.branden@broadcom.com> wrote:
> > On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
>
> > > Have you worked with the V4L developers to tie this into the proper
> > > in-kernel apis for this type of functionality?
> > We looked at the V4L model doesn't have any support for anything we are
> > doing in this driver.
> > We also want a driver that doesn't care about video.  It could be
> > offloading crypto or other operations.
> > We talked with Olof about all of this previously and he said leave it as
> > a misc driver for now.
> > He was going to discuss at linux plumbers conference that we need some
> > sort of offload engine model that such devices could fit into.
>
> I see. Have you looked at the "uacce" driver submission? It seems
> theirs is similar enough that there might be some way to share interfaces.

Uacce isn't a driver (or wasn't last time I looked at it, when it had
a different name). It's more of a framework for standardized direct HW
access from userspace, and relies on I/O virtualization to keep DMA
secure/partitioned, etc. VK is more of a classic PCIe device, it'll
handle DMA to/from the host, etc.

> > > Using a tty driver seems like the totally incorrect way to do this, what
> > > am I missing?
> > tty driver is used to provide console access to the processors running
> > on vk.
> > Data is sent using the bcm_vk_msg interface by read/write operations
> > from user space.
> > VK then gets the messages and DMA's the data to/from host memory when
> > needed to process.
>
> In turn here, it sounds like you'd want to look at what drivers/misc/mic/
> and the mellanox bluefield drivers are doing. As I understand, they have the
> same requirements for console, but have a nicer approach of providing
> abstract 'virtio' channels between the PCIe endpoint and the host, and
> then run regular virtio based drivers (console, tty, block, filesystem,
> network, ...) along with application specific ones to provide the custom
> high-level protocols.

This has more value on the device than on the host, as far as I've
seen it used (if you want to boot Linux on it and have things
exposed).

virtio isn't necessarily a match if all you really want is a character
stream for a console and don't need (or have performance requirements
beyond what virtio offers) other types of communication.

> This is also similar to what the drivers/pci/endpoint
> (from the other end) as the drivers/ntb (pci host on both ends) frameworks
> and of course the rpmsg/remoteproc framework do.

remoteproc is more about booting a tightly integrated device on an
embedded system. Also not a match here IMHO.

> In the long run, I would want much more consolidation between the
> low-level parts of all these frameworks, but moving your high-level
> protocols to the same virtio method would sound like a step in the
> direction towards a generialized framework and easier sharing of
> the abstractions.

For a simple naive console/character stream, doing something on top of
hvc might be easier -- it already does polling for you, etc.

Of course, the intent is not to ever use it as a console for the host
here, so that aspect of hvc isn't useful. But it gives you a bunch of
other stuff for free with just getchar/putchar interfaces to
implement.


-Olof

Re: [PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf

[Dan Carpenter]

On Fri, Feb 21, 2020 at 05:13:08PM -0800, Scott Branden wrote:
> > > > +static ssize_t test_dev_config_show_size_t(char *buf, int cfg)
> > > > +{
> > > > +    size_t val;
> > > > +
> > > > +    mutex_lock(&test_fw_mutex);
> > > > +    val = cfg;
> > > > +    mutex_unlock(&test_fw_mutex);
> > > Both val and cfg are stack variables so there is no need for locking.
> > > Probably you meant to pass a pointer to cfg?
> I am following the existing code as was done for
> test_dev_config_show_bool(),
> test_dev_config_show_int(),
> test_dev_config_show_u8()

Heh.  Yes.  Those are buggy as well.  Good eyes.  Could you send a patch
to fix them?

regards,
dan caprnter

Re: [PATCH v2 3/7] test_firmware: add partial read support for request_firmware_into_buf

[Luis Chamberlain]

On Fri, Feb 21, 2020 at 05:13:08PM -0800, Scott Branden wrote:
> > > > +static ssize_t test_dev_config_show_size_t(char *buf, int cfg)
> > > > +{
> > > > +    size_t val;
> > > > +
> > > > +    mutex_lock(&test_fw_mutex);
> > > > +    val = cfg;
> > > > +    mutex_unlock(&test_fw_mutex);
> > > Both val and cfg are stack variables so there is no need for locking.
> > > Probably you meant to pass a pointer to cfg?
> I am following the existing code as was done for
> test_dev_config_show_bool(),
> test_dev_config_show_int(),
> test_dev_config_show_u8()
> 
> Mutex probably not needed but I don't think I need to deviate from the rest
> of the test code.
> 
> Luis, could you please explain what the rest of your code is doing?

The lock is indeed not needed in the functions you mentioned, so you can
also remove the other locks as a precursor patch. It would be a seperate
patch.

  Luis

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Scott Branden]

Hi Olof/All,

I'm trying to digest all the feedback of what needs to be done.
I will be fixing up all the valuable comments about general issues but 
would like to know
what needs to be done about the tty interface.

The VK devices are configured to write serial data to circular buffers 
in memory or out a UART.
When we configure a system using the UART we connect a cable to the host 
and open a tty device.
When we don't connect a UART cable to the host we open the tty device in 
our driver instead.
In this case the memory is exposed to the host via PCI BAR memory space.
The bcm-vk host driver then accesses PCI space and presents a tty 
interface to the host.
We implemented a tty device to present the tty interface.
Host doesn't change anything other than opening a different devnode in 
UART vs. PCI case.

Based on all the comments: what interface should we be providing in 
driver instead?

On 2020-02-23 3:55 p.m., Olof Johansson wrote:
> On Sat, Feb 22, 2020 at 12:03 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Feb 21, 2020 at 7:19 PM Scott Branden
>> <scott.branden@broadcom.com> wrote:
>>> On 2020-02-19 11:47 p.m., Greg Kroah-Hartman wrote:
>>>> Have you worked with the V4L developers to tie this into the proper
>>>> in-kernel apis for this type of functionality?
>>> We looked at the V4L model doesn't have any support for anything we are
>>> doing in this driver.
>>> We also want a driver that doesn't care about video.  It could be
>>> offloading crypto or other operations.
>>> We talked with Olof about all of this previously and he said leave it as
>>> a misc driver for now.
>>> He was going to discuss at linux plumbers conference that we need some
>>> sort of offload engine model that such devices could fit into.
>> I see. Have you looked at the "uacce" driver submission? It seems
>> theirs is similar enough that there might be some way to share interfaces.
> Uacce isn't a driver (or wasn't last time I looked at it, when it had
> a different name). It's more of a framework for standardized direct HW
> access from userspace, and relies on I/O virtualization to keep DMA
> secure/partitioned, etc. VK is more of a classic PCIe device, it'll
> handle DMA to/from the host, etc.
>
>>>> Using a tty driver seems like the totally incorrect way to do this, what
>>>> am I missing?
>>> tty driver is used to provide console access to the processors running
>>> on vk.
>>> Data is sent using the bcm_vk_msg interface by read/write operations
>>> from user space.
>>> VK then gets the messages and DMA's the data to/from host memory when
>>> needed to process.
>> In turn here, it sounds like you'd want to look at what drivers/misc/mic/
>> and the mellanox bluefield drivers are doing. As I understand, they have the
>> same requirements for console, but have a nicer approach of providing
>> abstract 'virtio' channels between the PCIe endpoint and the host, and
>> then run regular virtio based drivers (console, tty, block, filesystem,
>> network, ...) along with application specific ones to provide the custom
>> high-level protocols.
> This has more value on the device than on the host, as far as I've
> seen it used (if you want to boot Linux on it and have things
> exposed).
>
> virtio isn't necessarily a match if all you really want is a character
> stream for a console and don't need (or have performance requirements
> beyond what virtio offers) other types of communication.
>
>> This is also similar to what the drivers/pci/endpoint
>> (from the other end) as the drivers/ntb (pci host on both ends) frameworks
>> and of course the rpmsg/remoteproc framework do.
> remoteproc is more about booting a tightly integrated device on an
> embedded system. Also not a match here IMHO.
>
>> In the long run, I would want much more consolidation between the
>> low-level parts of all these frameworks, but moving your high-level
>> protocols to the same virtio method would sound like a step in the
>> direction towards a generialized framework and easier sharing of
>> the abstractions.
> For a simple naive console/character stream, doing something on top of
> hvc might be easier -- it already does polling for you, etc.
>
> Of course, the intent is not to ever use it as a console for the host
> here, so that aspect of hvc isn't useful. But it gives you a bunch of
> other stuff for free with just getchar/putchar interfaces to
> implement.
>
>
> -Olof

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Dan Carpenter]

On Fri, Apr 17, 2020 at 02:49:11PM -0700, Scott Branden wrote:
> > > +static int bcm_vk_dma_alloc(struct device *dev,
> > > +			    struct bcm_vk_dma *dma,
> > > +			    int direction,
> > > +			    struct _vk_data *vkdata)
> > > +{
> > > +	dma_addr_t addr, sg_addr;
> > > +	int err;
> > > +	int i;
> > > +	int offset;
> > > +	uint32_t size;
> > > +	uint32_t remaining_size;
> > > +	uint32_t transfer_size;
> > > +	uint64_t data;
> > > +	unsigned long first, last;
> > > +	struct _vk_data *sgdata;
> > > +
> > > +	/* Get 64-bit user address */
> > > +	data = get_unaligned(&(vkdata->address));
> > Extra parens.
> removed
> > 
> > > +
> > > +	/* offset into first page */
> > > +	offset = offset_in_page(data);
> > > +
> > > +	/* Calculate number of pages */
> > > +	first = (data & PAGE_MASK) >> PAGE_SHIFT;
> > > +	last  = ((data + vkdata->size - 1) & PAGE_MASK) >> PAGE_SHIFT;
> > > +	dma->nr_pages = last - first + 1;
> > > +
> > > +	/* Allocate DMA pages */
> > > +	dma->pages = kmalloc_array(dma->nr_pages,
> > > +				   sizeof(struct page *),
> > > +				   GFP_KERNEL);
> > > +	if (dma->pages == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_dbg(dev, "Alloc DMA Pages [0x%llx+0x%x => %d pages]\n",
> > > +		data, vkdata->size, dma->nr_pages);
> > > +
> > > +	dma->direction = direction;
> > > +
> > > +	/* Get user pages into memory */
> > > +	err = get_user_pages_fast(data & PAGE_MASK,
> > > +				  dma->nr_pages,
> > > +				  direction == DMA_FROM_DEVICE,
> > > +				  dma->pages);
> > > +	if (err != dma->nr_pages) {
> > > +		dma->nr_pages = (err >= 0) ? err : 0;
> > > +		dev_err(dev, "get_user_pages_fast, err=%d [%d]\n",
> > > +			err, dma->nr_pages);
> > > +		return err < 0 ? err : -EINVAL;
> > > +	}
> > > +
> > > +	/* Max size of sg list is 1 per mapped page + fields at start */
> > > +	dma->sglen = (dma->nr_pages * sizeof(*sgdata)) +
> > > +		     (sizeof(uint32_t) * SGLIST_VKDATA_START);
> > > +
> > > +	/* Allocate sglist */
> > > +	dma->sglist = dma_alloc_coherent(dev,
> > > +					 dma->sglen,
> > > +					 &dma->handle,
> > > +					 GFP_KERNEL);
> > 
> > 	dma->sglist = dma_alloc_coherent(dev, dma->sglen, &dma->handle,
> > 					 GFP_KERNEL);
> done
> > 
> > 
> > 
> > > +	if (!dma->sglist)
> > > +		return -ENOMEM;
> > No cleanup?
> what needs to be cleaned up?

dma->pages should be freed probably?  And a put_user_pages_fast()?

regards,
dan carpenter

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Dan Carpenter]

On Sat, Apr 18, 2020 at 02:45:16PM +0300, Dan Carpenter wrote:
> On Fri, Apr 17, 2020 at 02:49:11PM -0700, Scott Branden wrote:
> > > > +static int bcm_vk_dma_alloc(struct device *dev,
> > > > +			    struct bcm_vk_dma *dma,
> > > > +			    int direction,
> > > > +			    struct _vk_data *vkdata)
> > > > +{
> > > > +	dma_addr_t addr, sg_addr;
> > > > +	int err;
> > > > +	int i;
> > > > +	int offset;
> > > > +	uint32_t size;
> > > > +	uint32_t remaining_size;
> > > > +	uint32_t transfer_size;
> > > > +	uint64_t data;
> > > > +	unsigned long first, last;
> > > > +	struct _vk_data *sgdata;
> > > > +
> > > > +	/* Get 64-bit user address */
> > > > +	data = get_unaligned(&(vkdata->address));
> > > Extra parens.
> > removed
> > > 
> > > > +
> > > > +	/* offset into first page */
> > > > +	offset = offset_in_page(data);
> > > > +
> > > > +	/* Calculate number of pages */
> > > > +	first = (data & PAGE_MASK) >> PAGE_SHIFT;
> > > > +	last  = ((data + vkdata->size - 1) & PAGE_MASK) >> PAGE_SHIFT;
> > > > +	dma->nr_pages = last - first + 1;
> > > > +
> > > > +	/* Allocate DMA pages */
> > > > +	dma->pages = kmalloc_array(dma->nr_pages,
> > > > +				   sizeof(struct page *),
> > > > +				   GFP_KERNEL);
> > > > +	if (dma->pages == NULL)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	dev_dbg(dev, "Alloc DMA Pages [0x%llx+0x%x => %d pages]\n",
> > > > +		data, vkdata->size, dma->nr_pages);
> > > > +
> > > > +	dma->direction = direction;
> > > > +
> > > > +	/* Get user pages into memory */
> > > > +	err = get_user_pages_fast(data & PAGE_MASK,
> > > > +				  dma->nr_pages,
> > > > +				  direction == DMA_FROM_DEVICE,
> > > > +				  dma->pages);
> > > > +	if (err != dma->nr_pages) {
> > > > +		dma->nr_pages = (err >= 0) ? err : 0;
> > > > +		dev_err(dev, "get_user_pages_fast, err=%d [%d]\n",
> > > > +			err, dma->nr_pages);
> > > > +		return err < 0 ? err : -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* Max size of sg list is 1 per mapped page + fields at start */
> > > > +	dma->sglen = (dma->nr_pages * sizeof(*sgdata)) +
> > > > +		     (sizeof(uint32_t) * SGLIST_VKDATA_START);
> > > > +
> > > > +	/* Allocate sglist */
> > > > +	dma->sglist = dma_alloc_coherent(dev,
> > > > +					 dma->sglen,
> > > > +					 &dma->handle,
> > > > +					 GFP_KERNEL);
> > > 
> > > 	dma->sglist = dma_alloc_coherent(dev, dma->sglen, &dma->handle,
> > > 					 GFP_KERNEL);
> > done
> > > 
> > > 
> > > 
> > > > +	if (!dma->sglist)
> > > > +		return -ENOMEM;
> > > No cleanup?
> > what needs to be cleaned up?
> 
> dma->pages should be freed probably?  And a put_user_pages_fast()?

Sorry put_user_pages_fast() isn't a function.  My bad.

regards,
dan carpenter

Re: [PATCH v2 6/7] misc: bcm-vk: add Broadcom VK driver

[Scott Branden]

Thanks Dan, I'll send out new version as soon as my other patch (you had 
requested for test_fx mutex cleanups)
https://lore.kernel.org/linux-arm-msm/20200415002517.4328-1-scott.branden@broadcom.com/

hits the linux-next tree so this patch series will apply cleanly to 
linux-next.




On 2020-04-18 4:47 a.m., Dan Carpenter wrote:
> On Sat, Apr 18, 2020 at 02:45:16PM +0300, Dan Carpenter wrote:
>> On Fri, Apr 17, 2020 at 02:49:11PM -0700, Scott Branden wrote:
>>>>> +static int bcm_vk_dma_alloc(struct device *dev,
>>>>> +			    struct bcm_vk_dma *dma,
>>>>> +			    int direction,
>>>>> +			    struct _vk_data *vkdata)
>>>>> +{
>>>>> +	dma_addr_t addr, sg_addr;
>>>>> +	int err;
>>>>> +	int i;
>>>>> +	int offset;
>>>>> +	uint32_t size;
>>>>> +	uint32_t remaining_size;
>>>>> +	uint32_t transfer_size;
>>>>> +	uint64_t data;
>>>>> +	unsigned long first, last;
>>>>> +	struct _vk_data *sgdata;
>>>>> +
>>>>> +	/* Get 64-bit user address */
>>>>> +	data = get_unaligned(&(vkdata->address));
>>>> Extra parens.
>>> removed
>>>>> +
>>>>> +	/* offset into first page */
>>>>> +	offset = offset_in_page(data);
>>>>> +
>>>>> +	/* Calculate number of pages */
>>>>> +	first = (data & PAGE_MASK) >> PAGE_SHIFT;
>>>>> +	last  = ((data + vkdata->size - 1) & PAGE_MASK) >> PAGE_SHIFT;
>>>>> +	dma->nr_pages = last - first + 1;
>>>>> +
>>>>> +	/* Allocate DMA pages */
>>>>> +	dma->pages = kmalloc_array(dma->nr_pages,
>>>>> +				   sizeof(struct page *),
>>>>> +				   GFP_KERNEL);
>>>>> +	if (dma->pages == NULL)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	dev_dbg(dev, "Alloc DMA Pages [0x%llx+0x%x => %d pages]\n",
>>>>> +		data, vkdata->size, dma->nr_pages);
>>>>> +
>>>>> +	dma->direction = direction;
>>>>> +
>>>>> +	/* Get user pages into memory */
>>>>> +	err = get_user_pages_fast(data & PAGE_MASK,
>>>>> +				  dma->nr_pages,
>>>>> +				  direction == DMA_FROM_DEVICE,
>>>>> +				  dma->pages);
>>>>> +	if (err != dma->nr_pages) {
>>>>> +		dma->nr_pages = (err >= 0) ? err : 0;
>>>>> +		dev_err(dev, "get_user_pages_fast, err=%d [%d]\n",
>>>>> +			err, dma->nr_pages);
>>>>> +		return err < 0 ? err : -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	/* Max size of sg list is 1 per mapped page + fields at start */
>>>>> +	dma->sglen = (dma->nr_pages * sizeof(*sgdata)) +
>>>>> +		     (sizeof(uint32_t) * SGLIST_VKDATA_START);
>>>>> +
>>>>> +	/* Allocate sglist */
>>>>> +	dma->sglist = dma_alloc_coherent(dev,
>>>>> +					 dma->sglen,
>>>>> +					 &dma->handle,
>>>>> +					 GFP_KERNEL);
>>>> 	dma->sglist = dma_alloc_coherent(dev, dma->sglen, &dma->handle,
>>>> 					 GFP_KERNEL);
>>> done
>>>>
>>>>> +	if (!dma->sglist)
>>>>> +		return -ENOMEM;
>>>> No cleanup?
>>> what needs to be cleaned up?
>> dma->pages should be freed probably?  And a put_user_pages_fast()?
> Sorry put_user_pages_fast() isn't a function.  My bad.
>
> regards,
> dan carpenter