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

[Mike Frysinger <vapier@gentoo.org>]

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