Re: [PATCH v3 6/8] test_firmware: Add test support for firmware upload

Re: [PATCH v3 6/8] test_firmware: Add test support for firmware upload

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4737 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220418223753.639058-7-russell.h.weight@intel.com>
References: <20220418223753.639058-7-russell.h.weight@intel.com>
TO: Russ Weight <russell.h.weight@intel.com>
TO: mcgrof(a)kernel.org
TO: rafael(a)kernel.org
TO: linux-kernel(a)vger.kernel.org
CC: trix(a)redhat.com
CC: lgoncalv(a)redhat.com
CC: yilun.xu(a)intel.com
CC: hao.wu(a)intel.com
CC: matthew.gerlach(a)linux.intel.com
CC: basheer.ahmed.muddebihal(a)intel.com
CC: tianfei.zhang(a)intel.com
CC: Russ Weight <russell.h.weight@intel.com>

Hi Russ,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on shuah-kselftest/next mcgrof/sysctl-next linus/master v5.18-rc3 next-20220419]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Russ-Weight/Extend-FW-framework-for-user-FW-uploads/20220419-064126
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
:::::: branch date: 13 hours ago
:::::: commit date: 13 hours ago
config: powerpc64-randconfig-c003-20220418 (https://download.01.org/0day-ci/archive/20220419/202204191900.1DwwOU9L-lkp(a)intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>


cocci warnings: (new ones prefixed by >>)
>> lib/test_firmware.c:1291:5-8: ERROR: invalid reference to the index variable of the iterator on line 1287

vim +1291 lib/test_firmware.c

c92316bf8e94830a Luis R. Rodriguez 2017-07-20  1273  
cb4d12e8d7596410 Russ Weight       2022-04-18  1274  static ssize_t upload_read_show(struct device *dev,
cb4d12e8d7596410 Russ Weight       2022-04-18  1275  				struct device_attribute *attr,
cb4d12e8d7596410 Russ Weight       2022-04-18  1276  				char *buf)
cb4d12e8d7596410 Russ Weight       2022-04-18  1277  {
cb4d12e8d7596410 Russ Weight       2022-04-18  1278  	struct test_firmware_upload *tst;
cb4d12e8d7596410 Russ Weight       2022-04-18  1279  	int ret = -EINVAL;
cb4d12e8d7596410 Russ Weight       2022-04-18  1280  
cb4d12e8d7596410 Russ Weight       2022-04-18  1281  	if (!test_fw_config->upload_name) {
cb4d12e8d7596410 Russ Weight       2022-04-18  1282  		pr_err("Set config_upload_name before using upload_read\n");
cb4d12e8d7596410 Russ Weight       2022-04-18  1283  		return -EINVAL;
cb4d12e8d7596410 Russ Weight       2022-04-18  1284  	}
cb4d12e8d7596410 Russ Weight       2022-04-18  1285  
cb4d12e8d7596410 Russ Weight       2022-04-18  1286  	mutex_lock(&test_fw_mutex);
cb4d12e8d7596410 Russ Weight       2022-04-18 @1287  	list_for_each_entry(tst, &test_upload_list, node)
cb4d12e8d7596410 Russ Weight       2022-04-18  1288  		if (tst->name == test_fw_config->upload_name)
cb4d12e8d7596410 Russ Weight       2022-04-18  1289  			break;
cb4d12e8d7596410 Russ Weight       2022-04-18  1290  
cb4d12e8d7596410 Russ Weight       2022-04-18 @1291  	if (tst->name != test_fw_config->upload_name) {
cb4d12e8d7596410 Russ Weight       2022-04-18  1292  		pr_err("Firmware name not found: %s\n",
cb4d12e8d7596410 Russ Weight       2022-04-18  1293  		       test_fw_config->upload_name);
cb4d12e8d7596410 Russ Weight       2022-04-18  1294  		goto out;
cb4d12e8d7596410 Russ Weight       2022-04-18  1295  	}
cb4d12e8d7596410 Russ Weight       2022-04-18  1296  
cb4d12e8d7596410 Russ Weight       2022-04-18  1297  	if (tst->size > PAGE_SIZE) {
cb4d12e8d7596410 Russ Weight       2022-04-18  1298  		pr_err("Testing interface must use PAGE_SIZE firmware for now\n");
cb4d12e8d7596410 Russ Weight       2022-04-18  1299  		goto out;
cb4d12e8d7596410 Russ Weight       2022-04-18  1300  	}
cb4d12e8d7596410 Russ Weight       2022-04-18  1301  
cb4d12e8d7596410 Russ Weight       2022-04-18  1302  	memcpy(buf, tst->buf, tst->size);
cb4d12e8d7596410 Russ Weight       2022-04-18  1303  	ret = tst->size;
cb4d12e8d7596410 Russ Weight       2022-04-18  1304  out:
cb4d12e8d7596410 Russ Weight       2022-04-18  1305  	mutex_unlock(&test_fw_mutex);
cb4d12e8d7596410 Russ Weight       2022-04-18  1306  	return ret;
cb4d12e8d7596410 Russ Weight       2022-04-18  1307  }
cb4d12e8d7596410 Russ Weight       2022-04-18  1308  static DEVICE_ATTR_RO(upload_read);
cb4d12e8d7596410 Russ Weight       2022-04-18  1309  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v3 6/8] test_firmware: Add test support for firmware upload

[Russ Weight]

Add support for testing the firmware upload driver. There are four sysfs
nodes added:

upload_register: write-only
  Write the name of the firmware device node to be created

upload_unregister: write-only
  Write the name of the firmware device node to be destroyed

config_upload_name: read/write
  Set the name to be used by upload_read

upload_read: read-only
  Read back the data associated with the firmware device node named
  in config_upload_name

You can create multiple, concurrent firmware device nodes for firmware
upload testing. Read firmware back and validate it using config_upload_name
and upload_red.

Example:
    $ cd /sys/devices/virtual/misc/test_firmware
    $ echo -n fw1 > upload_register
    $ ls fw1
    cancel  data  device  error  loading  power  remaining_size  status
    subsystem  uevent
    $ dd if=/dev/urandom of=/tmp/random-firmware.bin bs=512 count=4
    4+0 records in
    4+0 records out
    2048 bytes (2.0 kB, 2.0 KiB) copied, 0.000131959 s, 15.5 MB/s
    $ echo 1 > fw1/loading
    $ cat /tmp/random-firmware.bin > fw1/data
    $ echo 0 > fw1/loading
    $ cat fw1/status
    idle
    $ cat fw1/error
    $ echo -n fw1 > config_upload_name
    $ cmp /tmp/random-firmware.bin upload_read
    $ echo $?
    0
    $ echo -n fw1 > upload_unregister

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v2:
  - No changes from v1
v3:
  - Added Reviewed-by tag
---
 lib/test_firmware.c | 261 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 261 insertions(+)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 1bccd6cd5f48..2b8c56d7bf37 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -31,9 +31,12 @@ MODULE_IMPORT_NS(TEST_FIRMWARE);
 #define TEST_FIRMWARE_NAME	"test-firmware.bin"
 #define TEST_FIRMWARE_NUM_REQS	4
 #define TEST_FIRMWARE_BUF_SIZE	SZ_1K
+#define TEST_UPLOAD_MAX_SIZE	SZ_2K
+#define TEST_UPLOAD_BLK_SIZE	37	/* Avoid powers of two in testing */
 
 static DEFINE_MUTEX(test_fw_mutex);
 static const struct firmware *test_firmware;
+static LIST_HEAD(test_upload_list);
 
 struct test_batched_req {
 	u8 idx;
@@ -63,6 +66,7 @@ struct test_batched_req {
  * @reqs: stores all requests information
  * @read_fw_idx: index of thread from which we want to read firmware results
  *	from through the read_fw trigger.
+ * @upload_name: firmware name to be used with upload_read sysfs node
  * @test_result: a test may use this to collect the result from the call
  *	of the request_firmware*() calls used in their tests. In order of
  *	priority we always keep first any setup error. If no setup errors were
@@ -101,6 +105,7 @@ struct test_config {
 	bool send_uevent;
 	u8 num_requests;
 	u8 read_fw_idx;
+	char *upload_name;
 
 	/*
 	 * These below don't belong her but we'll move them once we create
@@ -112,8 +117,28 @@ struct test_config {
 			    struct device *device);
 };
 
+struct test_firmware_upload {
+	char *name;
+	struct list_head node;
+	char *buf;
+	size_t size;
+	bool cancel_request;
+	struct fw_upload *fwl;
+};
+
 static struct test_config *test_fw_config;
 
+static struct test_firmware_upload *upload_lookup_name(const char *name)
+{
+	struct test_firmware_upload *tst;
+
+	list_for_each_entry(tst, &test_upload_list, node)
+		if (strncmp(name, tst->name, strlen(tst->name)) == 0)
+			return tst;
+
+	return NULL;
+}
+
 static ssize_t test_fw_misc_read(struct file *f, char __user *buf,
 				 size_t size, loff_t *offset)
 {
@@ -198,6 +223,7 @@ static int __test_firmware_config_init(void)
 	test_fw_config->req_firmware = request_firmware;
 	test_fw_config->test_result = 0;
 	test_fw_config->reqs = NULL;
+	test_fw_config->upload_name = NULL;
 
 	return 0;
 
@@ -277,6 +303,13 @@ static ssize_t config_show(struct device *dev,
 			test_fw_config->sync_direct ? "true" : "false");
 	len += scnprintf(buf + len, PAGE_SIZE - len,
 			"read_fw_idx:\t%u\n", test_fw_config->read_fw_idx);
+	if (test_fw_config->upload_name)
+		len += scnprintf(buf + len, PAGE_SIZE - len,
+				"upload_name:\t%s\n",
+				test_fw_config->upload_name);
+	else
+		len += scnprintf(buf + len, PAGE_SIZE - len,
+				"upload_name:\tEMTPY\n");
 
 	mutex_unlock(&test_fw_mutex);
 
@@ -392,6 +425,32 @@ static ssize_t config_name_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(config_name);
 
+static ssize_t config_upload_name_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct test_firmware_upload *tst;
+	int ret = count;
+
+	mutex_lock(&test_fw_mutex);
+	tst = upload_lookup_name(buf);
+	if (tst)
+		test_fw_config->upload_name = tst->name;
+	else
+		ret = -EINVAL;
+	mutex_unlock(&test_fw_mutex);
+
+	return ret;
+}
+
+static ssize_t config_upload_name_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return config_test_show_str(buf, test_fw_config->upload_name);
+}
+static DEVICE_ATTR_RW(config_upload_name);
+
 static ssize_t config_num_requests_store(struct device *dev,
 					 struct device_attribute *attr,
 					 const char *buf, size_t count)
@@ -989,6 +1048,167 @@ ssize_t trigger_batched_requests_async_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(trigger_batched_requests_async);
 
+static void upload_release(struct test_firmware_upload *tst)
+{
+	firmware_upload_unregister(tst->fwl);
+	kfree(tst->buf);
+	kfree(tst->name);
+	kfree(tst);
+}
+
+static void upload_release_all(void)
+{
+	struct test_firmware_upload *tst, *tmp;
+
+	list_for_each_entry_safe(tst, tmp, &test_upload_list, node) {
+		list_del(&tst->node);
+		upload_release(tst);
+	}
+	test_fw_config->upload_name = NULL;
+}
+
+static enum fw_upload_err test_fw_upload_prepare(struct fw_upload *fwl,
+						 const u8 *data, u32 size)
+{
+	struct test_firmware_upload *tst = fwl->dd_handle;
+
+	tst->cancel_request = false;
+
+	if (!size || size > TEST_UPLOAD_MAX_SIZE)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	memset(tst->buf, 0, TEST_UPLOAD_MAX_SIZE);
+	tst->size = size;
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err test_fw_upload_write(struct fw_upload *fwl,
+					       const u8 *data, u32 offset,
+					       u32 size, u32 *written)
+{
+	struct test_firmware_upload *tst = fwl->dd_handle;
+	u32 blk_size;
+
+	if (tst->cancel_request)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	blk_size = min_t(u32, TEST_UPLOAD_BLK_SIZE, size);
+	memcpy(tst->buf + offset, data + offset, blk_size);
+
+	*written = blk_size;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err test_fw_upload_complete(struct fw_upload *fwl)
+{
+	struct test_firmware_upload *tst = fwl->dd_handle;
+
+	if (tst->cancel_request)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void test_fw_upload_cancel(struct fw_upload *fwl)
+{
+	struct test_firmware_upload *tst = fwl->dd_handle;
+
+	tst->cancel_request = true;
+}
+
+static const struct fw_upload_ops upload_test_ops = {
+	.prepare = test_fw_upload_prepare,
+	.write = test_fw_upload_write,
+	.poll_complete = test_fw_upload_complete,
+	.cancel = test_fw_upload_cancel,
+};
+
+static ssize_t upload_register_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct test_firmware_upload *tst;
+	struct fw_upload *fwl;
+	char *name;
+	int ret;
+
+	name = kstrndup(buf, count, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	mutex_lock(&test_fw_mutex);
+	tst = upload_lookup_name(name);
+	if (tst) {
+		ret = -EEXIST;
+		goto free_name;
+	}
+
+	tst = kzalloc(sizeof(*tst), GFP_KERNEL);
+	if (!tst) {
+		ret = -ENOMEM;
+		goto free_name;
+	}
+
+	tst->name = name;
+	tst->buf = kzalloc(TEST_UPLOAD_MAX_SIZE, GFP_KERNEL);
+	if (!tst->buf) {
+		ret = -ENOMEM;
+		goto free_tst;
+	}
+
+	fwl = firmware_upload_register(THIS_MODULE, dev, tst->name,
+				       &upload_test_ops, tst);
+	if (IS_ERR(fwl)) {
+		ret = PTR_ERR(fwl);
+		goto free_buf;
+	}
+
+	tst->fwl = fwl;
+	list_add_tail(&tst->node, &test_upload_list);
+	mutex_unlock(&test_fw_mutex);
+	return count;
+
+free_buf:
+	kfree(tst->buf);
+
+free_tst:
+	kfree(tst);
+
+free_name:
+	mutex_unlock(&test_fw_mutex);
+	kfree(name);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(upload_register);
+
+static ssize_t upload_unregister_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct test_firmware_upload *tst;
+	int ret = count;
+
+	mutex_lock(&test_fw_mutex);
+	tst = upload_lookup_name(buf);
+	if (!tst) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (test_fw_config->upload_name == tst->name)
+		test_fw_config->upload_name = NULL;
+
+	list_del(&tst->node);
+	upload_release(tst);
+
+out:
+	mutex_unlock(&test_fw_mutex);
+	return ret;
+}
+static DEVICE_ATTR_WO(upload_unregister);
+
 static ssize_t test_result_show(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
@@ -1051,6 +1271,42 @@ static ssize_t read_firmware_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(read_firmware);
 
+static ssize_t upload_read_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct test_firmware_upload *tst;
+	int ret = -EINVAL;
+
+	if (!test_fw_config->upload_name) {
+		pr_err("Set config_upload_name before using upload_read\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&test_fw_mutex);
+	list_for_each_entry(tst, &test_upload_list, node)
+		if (tst->name == test_fw_config->upload_name)
+			break;
+
+	if (tst->name != test_fw_config->upload_name) {
+		pr_err("Firmware name not found: %s\n",
+		       test_fw_config->upload_name);
+		goto out;
+	}
+
+	if (tst->size > PAGE_SIZE) {
+		pr_err("Testing interface must use PAGE_SIZE firmware for now\n");
+		goto out;
+	}
+
+	memcpy(buf, tst->buf, tst->size);
+	ret = tst->size;
+out:
+	mutex_unlock(&test_fw_mutex);
+	return ret;
+}
+static DEVICE_ATTR_RO(upload_read);
+
 #define TEST_FW_DEV_ATTR(name)          &dev_attr_##name.attr
 
 static struct attribute *test_dev_attrs[] = {
@@ -1066,6 +1322,7 @@ static struct attribute *test_dev_attrs[] = {
 	TEST_FW_DEV_ATTR(config_sync_direct),
 	TEST_FW_DEV_ATTR(config_send_uevent),
 	TEST_FW_DEV_ATTR(config_read_fw_idx),
+	TEST_FW_DEV_ATTR(config_upload_name),
 
 	/* These don't use the config at all - they could be ported! */
 	TEST_FW_DEV_ATTR(trigger_request),
@@ -1082,6 +1339,9 @@ static struct attribute *test_dev_attrs[] = {
 	TEST_FW_DEV_ATTR(release_all_firmware),
 	TEST_FW_DEV_ATTR(test_result),
 	TEST_FW_DEV_ATTR(read_firmware),
+	TEST_FW_DEV_ATTR(upload_read),
+	TEST_FW_DEV_ATTR(upload_register),
+	TEST_FW_DEV_ATTR(upload_unregister),
 	NULL,
 };
 
@@ -1128,6 +1388,7 @@ static void __exit test_firmware_exit(void)
 	mutex_lock(&test_fw_mutex);
 	release_firmware(test_firmware);
 	misc_deregister(&test_fw_misc_device);
+	upload_release_all();
 	__test_firmware_config_free();
 	kfree(test_fw_config);
 	mutex_unlock(&test_fw_mutex);
-- 
2.25.1