[LTP] [PATCH v6] fw_load: new test of device firmware loading

[LTP] [PATCH v6] fw_load: new test of device firmware loading

[Alexey Kodanev]

This test checks the device firmware loading. Since Linux 3.7 it can be loaded
directly (by-pass udev). The test consists of the two parts: userspace and
kernelspace.

Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 runtest/kernel_misc                                |    1 +
 testcases/kernel/Makefile                          |    1 +
 testcases/kernel/firmware/Makefile                 |   44 ++++
 .../kernel/firmware/fw_load_kernel/.gitignore      |    6 +
 testcases/kernel/firmware/fw_load_kernel/Makefile  |   37 ++++
 testcases/kernel/firmware/fw_load_kernel/README    |   16 ++
 .../kernel/firmware/fw_load_kernel/ltp_fw_load.c   |  173 +++++++++++++++
 testcases/kernel/firmware/fw_load_user/.gitignore  |    1 +
 testcases/kernel/firmware/fw_load_user/Makefile    |   20 ++
 testcases/kernel/firmware/fw_load_user/README      |   11 +
 testcases/kernel/firmware/fw_load_user/fw_load.c   |  219 ++++++++++++++++++++
 11 files changed, 529 insertions(+), 0 deletions(-)
 create mode 100644 testcases/kernel/firmware/Makefile
 create mode 100644 testcases/kernel/firmware/fw_load_kernel/.gitignore
 create mode 100644 testcases/kernel/firmware/fw_load_kernel/Makefile
 create mode 100644 testcases/kernel/firmware/fw_load_kernel/README
 create mode 100644 testcases/kernel/firmware/fw_load_kernel/ltp_fw_load.c
 create mode 100644 testcases/kernel/firmware/fw_load_user/.gitignore
 create mode 100644 testcases/kernel/firmware/fw_load_user/Makefile
 create mode 100644 testcases/kernel/firmware/fw_load_user/README
 create mode 100644 testcases/kernel/firmware/fw_load_user/fw_load.c

diff --git a/runtest/kernel_misc b/runtest/kernel_misc
index 47a667e..73e21a7 100644
--- a/runtest/kernel_misc
+++ b/runtest/kernel_misc
@@ -1 +1,2 @@
 kmsg01 kmsg01
+fw_load fw_load
diff --git a/testcases/kernel/Makefile b/testcases/kernel/Makefile
index 4b4800d..256a574 100644
--- a/testcases/kernel/Makefile
+++ b/testcases/kernel/Makefile
@@ -38,6 +38,7 @@ ifneq ($(UCLINUX),1)
 SUBDIRS			+= connectors \
 			   containers \
 			   controllers \
+			   firmware \
 			   fs \
 			   hotplug \
 			   io \
diff --git a/testcases/kernel/firmware/Makefile b/testcases/kernel/firmware/Makefile
new file mode 100644
index 0000000..59817f6
--- /dev/null
+++ b/testcases/kernel/firmware/Makefile
@@ -0,0 +1,44 @@
+# Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+
+top_srcdir	?= ../../..
+
+include $(top_srcdir)/include/mk/env_pre.mk
+
+SUBDIRS			=
+REQ_VERSION_MAJOR	= 3
+REQ_VERSION_PATCH	= 7
+
+ifeq ($(MAKECMDGOALS),clean)
+PROCEED = 1
+endif
+
+ifeq ($(WITH_MODULES),yes)
+PROCEED ?= $(shell expr $(LINUX_VERSION_MAJOR) '>' $(REQ_VERSION_MAJOR))
+ifeq ($(PROCEED),0)
+PROCEED = $(shell expr $(LINUX_VERSION_MAJOR) '=' $(REQ_VERSION_MAJOR))
+ifeq ($(PROCEED),1)
+PROCEED = $(shell expr $(LINUX_VERSION_PATCH) '>=' $(REQ_VERSION_PATCH))
+endif
+endif
+endif
+
+ifeq ($(PROCEED),1)
+SUBDIRS			+= fw_load_kernel
+SUBDIRS			+= fw_load_user
+endif
+
+include $(top_srcdir)/include/mk/generic_trunk_target.mk
diff --git a/testcases/kernel/firmware/fw_load_kernel/.gitignore b/testcases/kernel/firmware/fw_load_kernel/.gitignore
new file mode 100644
index 0000000..180072a
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_kernel/.gitignore
@@ -0,0 +1,6 @@
+/ltp_fw_load.ko
+/*.cmd
+/modules.order
+/Module.symvers
+/ltp_fw_load.mod.c
+/.tmp_versions/
diff --git a/testcases/kernel/firmware/fw_load_kernel/Makefile b/testcases/kernel/firmware/fw_load_kernel/Makefile
new file mode 100644
index 0000000..4586501
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_kernel/Makefile
@@ -0,0 +1,37 @@
+# Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+
+ifneq ($(KERNELRELEASE),)
+
+ifdef CONFIG_FW_LOADER
+obj-m		:= ltp_fw_load.o
+endif
+
+else
+
+top_srcdir	?= ../../../..
+include $(top_srcdir)/include/mk/env_pre.mk
+
+MAKE_TARGETS	:= ltp_fw_load.ko
+ltp_fw_load.ko: ltp_fw_load.c
+	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir)
+	-mv ltp_fw_load.ko ltp_fw_load.ko~
+	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir) clean
+	-mv ltp_fw_load.ko~ ltp_fw_load.ko
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
+
+endif
diff --git a/testcases/kernel/firmware/fw_load_kernel/README b/testcases/kernel/firmware/fw_load_kernel/README
new file mode 100644
index 0000000..320d956
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_kernel/README
@@ -0,0 +1,16 @@
+The aim of the test is to check device firmware loading. Since kernel 3.7
+firmware loading changed to direct loading (by-pass udev). The test consists
+of the two parts:
+ - userspace part
+ - kernelspace part
+
+This is a kernel module, which is a part of the device firmware loading test.
+It allows to call request_firmware kernel function with specified parameters.
+Template firmware file name and expected firmware file's data size are passed
+as the insmod command line parameters. Then, the number of firmware test files
+should be written to sysfs file 'fwnum'. This write will initiate request
+firmware procedure. In the end, results can be read from 'result' device sysfs
+file. Also, some information regarding module loading, can be obtained by
+looking at kernel log file.
+
+It is automatically used by userspace part of the test.
diff --git a/testcases/kernel/firmware/fw_load_kernel/ltp_fw_load.c b/testcases/kernel/firmware/fw_load_kernel/ltp_fw_load.c
new file mode 100644
index 0000000..d18a5a1
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_kernel/ltp_fw_load.c
@@ -0,0 +1,173 @@
+/*
+ * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Author:
+ * Alexey Kodanev <alexey.kodanev@oracle.com>
+ *
+ * This module is trying to load external test firmware files (n#_load_tst.fw).
+ * In the end, it writes results to /sys/devices/fw_load/result file.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/string.h>
+#include <linux/firmware.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexey Kodanev <alexey.kodanev@oracle.com>");
+MODULE_DESCRIPTION("This module is checking device firmware loading");
+
+#define TCID		"ltp_fw_load"
+
+static char *fw_name	= "load_tst.fw";
+static int fw_size	= 0x1000;
+static int max_name	= 64;
+static int fw;
+
+module_param(fw_name, charp, 0444);
+MODULE_PARM_DESC(fw_name, "Template firmware file name: n#_name");
+
+module_param(fw_size, int, 0444);
+MODULE_PARM_DESC(fw_size, "Firmware file size");
+
+/*
+ * bit mask for each test-case,
+ * if test is passed, bit will be set to 1
+ */
+static int test_result;
+
+/* read and print firmware data */
+static int fw_read(const u8 *data, size_t size);
+
+static void device_release(struct device *dev);
+
+static struct device tdev = {
+	.init_name	= TCID,
+	.release	= device_release,
+};
+
+static int try_request_fw(const char *name);
+
+/* print test result to sysfs file */
+static ssize_t sys_result(struct device *dev,
+	struct device_attribute *attr, char *buf);
+static DEVICE_ATTR(result, S_IRUSR, sys_result, NULL);
+
+/*
+ * get the number of firmware files and
+ * perform firmware requests
+ */
+static ssize_t sys_fwnum(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count);
+static DEVICE_ATTR(fwnum, S_IWUSR, NULL, sys_fwnum);
+
+static int test_init(void)
+{
+	int err;
+
+	err = device_register(&tdev);
+	if (err) {
+		pr_err(TCID ": Unable to register device\n");
+		return err;
+	}
+	pr_info(TCID ": device registered\n");
+
+	err = device_create_file(&tdev, &dev_attr_result);
+	if (err != 0)
+		pr_info(TCID ": Can't create sysfs file 'result'\n");
+	err = device_create_file(&tdev, &dev_attr_fwnum);
+	if (err != 0)
+		pr_info(TCID ": Can't create sysfs file 'fwnum'\n");
+
+	return err;
+}
+
+static void test_exit(void)
+{
+	device_remove_file(&tdev, &dev_attr_result);
+	device_remove_file(&tdev, &dev_attr_fwnum);
+
+	device_unregister(&tdev);
+	pr_info(TCID ": module exited\n");
+}
+
+static void device_release(struct device *dev)
+{
+	pr_info(TCID ": device released\n");
+}
+
+static ssize_t sys_result(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%d\n", test_result);
+}
+
+static ssize_t sys_fwnum(struct device *dev,
+	struct device_attribute *attr,  const char *buf, size_t count)
+{
+	int err, fw_num = 0;
+
+	sscanf(buf, "%d", &fw_num);
+	if (fw_num <= 0) {
+		pr_err(TCID ": Unexpected number of firmwares '%d'", fw_num);
+		return count;
+	}
+	for (fw = 0; fw < fw_num; ++fw) {
+		char name[max_name];
+		snprintf(name, max_name, "n%d_%s", fw, fw_name);
+		err = try_request_fw(name);
+		test_result |= (err == 0) << fw;
+	}
+	return count;
+}
+
+static int fw_read(const u8 *data, size_t size)
+{
+	size_t i;
+	pr_info(TCID ": Firmware has size '%d'\n", (unsigned int) size);
+	if (size != fw_size) {
+		pr_err(TCID ": Expected firmware size '%d'\n",
+		(unsigned int) fw_size);
+		return -1;
+	}
+	for (i = 0; i < size; ++i) {
+		if (data[i] != (u8)fw) {
+			pr_err(TCID ": Unexpected firmware data\n");
+			return -1;
+		}
+	}
+	return 0;
+}
+
+static int try_request_fw(const char *name)
+{
+	int err;
+	const struct firmware *fw_entry = NULL;
+	err = request_firmware(&fw_entry, name, &tdev);
+	if (!err) {
+		pr_info(TCID ": firmware '%s' requested\n", name);
+		err = fw_read(fw_entry->data, fw_entry->size);
+	} else
+		pr_err(TCID ": Can't request firmware '%s'\n", name);
+	release_firmware(fw_entry);
+	return err;
+}
+
+module_init(test_init);
+module_exit(test_exit);
diff --git a/testcases/kernel/firmware/fw_load_user/.gitignore b/testcases/kernel/firmware/fw_load_user/.gitignore
new file mode 100644
index 0000000..1d08149
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_user/.gitignore
@@ -0,0 +1 @@
+/fw_load
diff --git a/testcases/kernel/firmware/fw_load_user/Makefile b/testcases/kernel/firmware/fw_load_user/Makefile
new file mode 100644
index 0000000..effd5da
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_user/Makefile
@@ -0,0 +1,20 @@
+# Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/firmware/fw_load_user/README b/testcases/kernel/firmware/fw_load_user/README
new file mode 100644
index 0000000..702fac9
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_user/README
@@ -0,0 +1,11 @@
+The aim of the test is to check device firmware loading. Since kernel 3.7
+firmware loading changed to direct loading (by-pass udev). The test consists
+of the two parts:
+ - userspace part
+ - kernelspace part
+
+This is the userspace part, its tasks are:
+ - create firmware files in the standard firmware paths
+ - load the module and initiate firmware request procedure
+ - read device's result file and print final results
+ - unload the module.
diff --git a/testcases/kernel/firmware/fw_load_user/fw_load.c b/testcases/kernel/firmware/fw_load_user/fw_load.c
new file mode 100644
index 0000000..7fd58c9
--- /dev/null
+++ b/testcases/kernel/firmware/fw_load_user/fw_load.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Author:
+ * Alexey Kodanev <alexey.kodanev@oracle.com>
+ *
+ * Test checks device firmware loading.
+ */
+
+#define _GNU_SOURCE
+#include <sys/utsname.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "test.h"
+#include "usctest.h"
+#include "safe_macros.h"
+#include "tst_module.h"
+
+/* number of test firmware files */
+#define FW_FILES	5
+
+char *TCID = "fw_load";
+int TST_TOTAL = FW_FILES;
+
+static int fw_size = 0x1000;
+
+static const char fw_name[]	= "load_tst.fw";
+static const char module_name[]	= "ltp_fw_load.ko";
+
+/* paths to module's sysfs files */
+static const char dev_fwnum[]	= "/sys/devices/ltp_fw_load/fwnum";
+static const char dev_result[]	= "/sys/devices/ltp_fw_load/result";
+
+struct fw_file_info {
+	char *file;
+	char *dir;
+	int fake;
+	int remove_dir;
+	int remove_file;
+};
+
+static struct fw_file_info fw[FW_FILES];
+static int fw_num;
+
+/* test options */
+static char *narg;
+static int nflag;
+static int skip_cleanup;
+static int verbose;
+static const option_t options[] = {
+	{"n:", &nflag, &narg},
+	{"s", &skip_cleanup, NULL},
+	{"v", &verbose, NULL},
+	{NULL, NULL, NULL}
+};
+
+static void help(void);
+static void setup(int argc, char *argv[]);
+static void test_run(void);
+static void cleanup(void);
+
+/*
+ * create firmware files in the fw_paths
+ * @fw_paths: it must be termintated by a NULL pointer
+ */
+static void create_firmware(char *const fw_paths[]);
+
+int main(int argc, char *argv[])
+{
+	setup(argc, argv);
+
+	test_run();
+
+	cleanup();
+
+	tst_exit();
+}
+
+static void help(void)
+{
+	printf("  -n x    Write x bytes to firmware file, default is %d\n",
+		fw_size);
+	printf("  -s      Skip cleanup\n");
+	printf("  -v      Verbose\n");
+}
+
+void setup(int argc, char *argv[])
+{
+	char *msg;
+	msg = parse_opts(argc, argv, options, help);
+	if (msg != NULL)
+		tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
+
+	if (nflag) {
+		if (sscanf(narg, "%i", &fw_size) != 1)
+			tst_brkm(TBROK, NULL, "-n option arg is not a number");
+		if (fw_size < 0)
+			tst_brkm(TBROK, NULL, "-n option arg is less than 0");
+	}
+
+	tst_require_root(NULL);
+
+	if (tst_kvercmp(3, 7, 0) < 0) {
+		tst_brkm(TCONF, NULL,
+			"Test must be run with kernel 3.7 or newer");
+	}
+
+	char fw_size_param[19];
+	snprintf(fw_size_param, 19, "fw_size=%d", fw_size);
+	char *const mod_params[2] = { fw_size_param, NULL };
+	tst_module_load(NULL, module_name, mod_params);
+
+	tst_sig(FORK, DEF_HANDLER, cleanup);
+
+	/* get current Linux version and make firmware paths */
+	struct utsname uts_name;
+	uname(&uts_name);
+
+	/* 4 hard coded firmware paths + NULL */
+	char *fw_paths[5] = { "/lib/firmware", "/lib/firmware/updates" };
+	asprintf(&fw_paths[2], "%s/%s", fw_paths[0], uts_name.release);
+	asprintf(&fw_paths[3], "%s/%s", fw_paths[1], uts_name.release);
+
+	/* create firmware in the hard coded firmware search paths */
+	create_firmware(fw_paths);
+
+	free(fw_paths[2]);
+	free(fw_paths[3]);
+
+	/* make non-existent firmware file */
+	asprintf(&fw[fw_num].file, "/n%d_%s", fw_num, fw_name);
+	fw[fw_num].fake = 1;
+	++fw_num;
+}
+
+static void test_run(void)
+{
+	/* initiate firmware requests */
+	SAFE_FILE_PRINTF(cleanup, dev_fwnum, "%d", fw_num);
+
+	/* get module results by reading result bit mask */
+	int result = 0;
+	SAFE_FILE_SCANF(cleanup, dev_result, "%d", &result);
+
+	int i, fail, offset;
+	for (i = 0; i < fw_num; ++i) {
+		fail = (result & (1 << i)) == 0 && !fw[i].fake;
+		offset = (fw[i].dir) ? strlen(fw[i].dir) : 0;
+		tst_resm((fail) ? TFAIL : TPASS,
+			"Expect: %s load firmware '...%s'",
+			(fw[i].fake) ? "can't" : "can",
+			fw[i].file + offset);
+	}
+}
+
+static void cleanup(void)
+{
+	if (skip_cleanup)
+		return;
+
+	int i;
+	/* remove subdirs first and then upper level dirs */
+	for (i = fw_num - 1; i >= 0; --i) {
+		if (fw[i].remove_file && remove(fw[i].file) == -1)
+			tst_resm(TWARN, "Can't remove: %s", fw[i].file);
+		free(fw[i].file);
+
+		if (fw[i].remove_dir && remove(fw[i].dir) == -1)
+			tst_resm(TWARN, "Can't remove %s", fw[i].dir);
+		free(fw[i].dir);
+	}
+
+	tst_module_unload(NULL, module_name);
+
+	TEST_CLEANUP;
+}
+
+static void create_firmware(char *const fw_paths[])
+{
+	int i = 0;
+	while (fw_paths[i] != NULL) {
+		struct fw_file_info *fi = &fw[fw_num];
+		fi->dir = strdup(fw_paths[i]);
+		if (access(fi->dir, X_OK) == -1) {
+			/* create dir */
+			SAFE_MKDIR(cleanup, fi->dir, 0755);
+			fi->remove_dir = 1;
+		}
+
+		/* create test firmware file */
+		asprintf(&fi->file, "%s/n%d_%s", fi->dir, fw_num, fw_name);
+
+		FILE *f = SAFE_FOPEN(cleanup, fi->file, "w");
+		fi->remove_file = 1;
+		int k, byte = fw_num;
+		++fw_num;
+		for (k = 0; k < fw_size; ++k)
+			fputc(byte, f);
+		SAFE_FCLOSE(cleanup, f);
+		++i;
+	}
+}
-- 
1.7.1


------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[chrubis]

Hi!
> This test checks the device firmware loading. Since Linux 3.7 it can be loaded
> directly (by-pass udev). The test consists of the two parts: userspace and
> kernelspace.
> 
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>

This version looks good to me.

But there is one catch there, as we divided the module load function
into the load_module and run_cmd functions the testcase will end up with
TBROK rather than with TCONF when the kernel module couldn't be loaded
due to version mismatch and I would preffer the TCONF in this case.

But that does not block the inclusion of this testcase (and the
configure patch that needs to be applied beforhand) and if we agree that
TCONF is better we can fix the module_load later. One solution that
seems easy enough is to make the run_cmd() return the wait status (as
system() does).

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 3531 bytes --]

On Tuesday 02 July 2013 07:43:41 Alexey Kodanev wrote:
> --- /dev/null
> +++ b/testcases/kernel/firmware/Makefile
>
> +ifeq ($(WITH_MODULES),yes)
> +PROCEED ?= $(shell expr $(LINUX_VERSION_MAJOR) '>' $(REQ_VERSION_MAJOR))

use `test` rather than `expr`.  people are more familiar with that.

	test $(LINUX_VERSION_MAJOR) -gt $(REQ_VERSION_MAJOR)

> --- /dev/null
> +++ b/testcases/kernel/firmware/fw_load_kernel/Makefile
>
> +MAKE_TARGETS	:= ltp_fw_load.ko
> +ltp_fw_load.ko: ltp_fw_load.c
> +	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir)
> +	-mv ltp_fw_load.ko ltp_fw_load.ko~
> +	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir) clean
> +	-mv ltp_fw_load.ko~ ltp_fw_load.ko

what's with ignoring the exit status of all these ?  and why clean when you're 
done ?  doesn't seem like you'd want to do either of these.

> --- /dev/null
> +++ b/testcases/kernel/firmware/fw_load_kernel/ltp_fw_load.c

> +/* read and print firmware data */
> +static int fw_read(const u8 *data, size_t size);
> +
> +static void device_release(struct device *dev);
> +
> +static struct device tdev = {
> +	.init_name	= TCID,
> +	.release	= device_release,
> +};
> +
> +static int try_request_fw(const char *name);
> +
> +/* print test result to sysfs file */
> +static ssize_t sys_result(struct device *dev,
> +	struct device_attribute *attr, char *buf);
> +static DEVICE_ATTR(result, S_IRUSR, sys_result, NULL);
> +
> +/*
> + * get the number of firmware files and
> + * perform firmware requests
> + */
> +static ssize_t sys_fwnum(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count);
> +static DEVICE_ATTR(fwnum, S_IWUSR, NULL, sys_fwnum);

generally it's preferable to organize the code such that forward decls aren't 
even necessary.  seems like it'd be trivial to reorganize this file as such.

> +static int test_init(void)
> +{
> +	int err;
> +
> +	err = device_register(&tdev);
> +	if (err) {
> +		pr_err(TCID ": Unable to register device\n");

define pr_fmt at the top of the file rather than manually inserting TCID in 
every call point

> +	err = device_create_file(&tdev, &dev_attr_result);
> +	if (err != 0)

typically it's better to just do:
	if (err)

> +		pr_info(TCID ": Can't create sysfs file 'result'\n");

pr_err

> +	err = device_create_file(&tdev, &dev_attr_fwnum);
> +	if (err != 0)
> +		pr_info(TCID ": Can't create sysfs file 'fwnum'\n");

if 'result' failed but 'fwnum' passed, this func incorrectly returns 0

you should have this code return immediately, and if fwnum fails, have it call 
device_remove_file on 'result'

> +static ssize_t sys_fwnum(struct device *dev,
> +	struct device_attribute *attr,  const char *buf, size_t count)
> +{
> +	int err, fw_num = 0;
> +
> +	sscanf(buf, "%d", &fw_num);
> +	if (fw_num <= 0) {
> +		pr_err(TCID ": Unexpected number of firmwares '%d'", fw_num);
> +		return count;
> +	}
> +	for (fw = 0; fw < fw_num; ++fw) {
> +		char name[max_name];
> +		snprintf(name, max_name, "n%d_%s", fw, fw_name);
> +		err = try_request_fw(name);
> +		test_result |= (err == 0) << fw;

mmm and what if fw_num > sizeof(test_result) ?

> +static int fw_read(const u8 *data, size_t size)
> +{
> +	size_t i;
> +	pr_info(TCID ": Firmware has size '%d'\n", (unsigned int) size);

don't cast.  use a proper format string: %zu.

> +module_init(test_init);
> +module_exit(test_exit);

i'd move those to below the respective func rather than at the bottom of the 
file
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 184 bytes --]

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[alexey.kodanev]

Hi!

On 07/03/2013 12:50 AM, Mike Frysinger wrote:
>> --- /dev/null
>> >  +++ b/testcases/kernel/firmware/fw_load_kernel/Makefile
>> >
>> >  +MAKE_TARGETS	:= ltp_fw_load.ko
>> >  +ltp_fw_load.ko: ltp_fw_load.c
>> >  +	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir)
>> >  +	-mv ltp_fw_load.ko ltp_fw_load.ko~
>> >  +	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir) clean
>> >  +	-mv ltp_fw_load.ko~ ltp_fw_load.ko
> what's with ignoring the exit status of all these ?  and why clean when you're
> done ?  doesn't seem like you'd want to do either of these.
>
Ignoring the exit status, allow to proceed with the LTP build, despite 
of any error during the module's build. Then, user-space test will 
return TCONF, if it doesn't find the module.

There are a lot of files that left after module's build and If I don't 
clean there, I have to manually add files to the clean target and 
perhaps, even more.

Thanks,
Alexey


------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[chrubis]

Hi!
> >> >  +++ b/testcases/kernel/firmware/fw_load_kernel/Makefile
> >> >
> >> >  +MAKE_TARGETS	:= ltp_fw_load.ko
> >> >  +ltp_fw_load.ko: ltp_fw_load.c
> >> >  +	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir)
> >> >  +	-mv ltp_fw_load.ko ltp_fw_load.ko~
> >> >  +	-$(MAKE) -C $(LINUX_DIR) M=$(abs_srcdir) clean
> >> >  +	-mv ltp_fw_load.ko~ ltp_fw_load.ko
> > what's with ignoring the exit status of all these ?  and why clean when you're
> > done ?  doesn't seem like you'd want to do either of these.
> >
> Ignoring the exit status, allow to proceed with the LTP build, despite 
> of any error during the module's build. Then, user-space test will 
> return TCONF, if it doesn't find the module.

This is done so that LTP is forward compatible with kernel internal API
changes. The test tries to load the module and returns TCONF if module
was not found (i.e. wasn't build either due to kernel-devel missing or
module build failure).

> There are a lot of files that left after module's build and If I don't 
> clean there, I have to manually add files to the clean target and 
> perhaps, even more.

Making make clean work would be better, but I'm not sure how to make
that work, the kernel build leaves various files there and I'm not sure
if these have stable names, but we can do some effort to clean up all
the *.cmd.o and maybe some of the rest of them.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[chrubis]

Hi!
> generally it's preferable to organize the code such that forward decls aren't 
> even necessary.  seems like it'd be trivial to reorganize this file as such.

I keep telling that to people yet they are stubborn to organize the
files more to the actual code flow, i.e. main() first then all the
functions uses in main and so forth...

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[chrubis]

Hi!
> >>> There are a lot of files that left after module's build and If I don't
> >>> clean there, I have to manually add files to the clean target and
> >>> perhaps, even more.
> >> Making make clean work would be better, but I'm not sure how to make
> >> that work, the kernel build leaves various files there and I'm not sure
> >> if these have stable names, but we can do some effort to clean up all
> >> the *.cmd.o and maybe some of the rest of them.
> > have it output to a subdir and add that subdir to the ignore
> > -mike
> But how to do it with external kernel modules and kernel build?
> 
> I still don't understand why is the current approach not appropriate? 
> 'make clean' works and it deletes make target file. 'make' works fine, 
> even if module doesn't compile. And we get only what is needed for the test.

The intention here is to make the 'git status' ignore all the generated
files and/or to make 'make clean' to clean them. Which is not criticall
in any way and I'm fine with fixing these once the testcase is merged.

> if we need to clean those files by 'make clean', I think it is better to 
> make LINUX_DIR variable available when clean is invoked, and add 
> additional rule with kbuild clean.

If kbuild clean works this satisfies the second part but not the first.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH v6] fw_load: new test of device firmware loading

[chrubis]

Hi!
> >>>> There are a lot of files that left after module's build and If I don't
> >>>> clean there, I have to manually add files to the clean target and
> >>>> perhaps, even more.
> >>> Making make clean work would be better, but I'm not sure how to make
> >>> that work, the kernel build leaves various files there and I'm not sure
> >>> if these have stable names, but we can do some effort to clean up all
> >>> the *.cmd.o and maybe some of the rest of them.
> >> have it output to a subdir and add that subdir to the ignore
> >> -mike
> > But how to do it with external kernel modules and kernel build?
> >
> > I still don't understand why is the current approach not appropriate? 
> > 'make clean' works and it deletes make target file. 'make' works fine, 
> > even if module doesn't compile. And we get only what is needed for the 
> > test.
> >
> > if we need to clean those files by 'make clean', I think it is better 
> > to make LINUX_DIR variable available when clean is invoked, and add 
> > additional rule with kbuild clean.
> >
> What about the following way of building module:
> 1. add include/mk/module.mk.in, which will contain LINUX_DIR, 
> WITH_MODULES, etc. variables.
> 2. then include it in env_pre.mk and env_post.mk

Hmm, all this to isolate the variables from config.mk? I think that it
would be easier to include config.mk in env_pre.mk even on clean target.

The only thing this will break is the ability to run make clean without
doing configure... If that is needed we can include it only when it
exists with '-include' and check if LINUX_DIR exists and do the clean
only when it does.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list