[2/2] oem: wireless: add tests for 1) toggling the state of wireless devices, including WIFI, Bluetooth and WWAN (3G), via a kernel interfaces, and 2) detect hardware block of wireless devices.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/Makefile.am             |    3 +-
 src/lib/include/fwts-oem.h  |   53 ++++++++
 src/oem/wireless/wireless.c |  291 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+), 1 deletion(-)
 create mode 100644 src/lib/include/fwts-oem.h
 create mode 100644 src/oem/wireless/wireless.c

On 02/01/13 08:15, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>   src/Makefile.am             |    3 +-
>   src/lib/include/fwts-oem.h  |   53 ++++++++
>   src/oem/wireless/wireless.c |  291 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 346 insertions(+), 1 deletion(-)
>   create mode 100644 src/lib/include/fwts-oem.h
>   create mode 100644 src/oem/wireless/wireless.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 478c522..3a6c374 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -68,7 +68,8 @@ fwts_SOURCES = main.c \
>   	kernel/oops/oops.c \
>   	uefi/csm/csm.c \
>   	uefi/uefidump/uefidump.c \
> -	uefi/uefirttime/uefirttime.c
> +	uefi/uefirttime/uefirttime.c \
> +	oem/wireless/wireless.c

Just for future reference, I'd like to ensure these are added in 
alphabetical order to the Makefile.am - I think your patch is out of 
sync with the latest fwts.

>
>   fwts_LDFLAGS = -ljson -lm
>
> diff --git a/src/lib/include/fwts-oem.h b/src/lib/include/fwts-oem.h
> new file mode 100644
> index 0000000..e09499c
> --- /dev/null
> +++ b/src/lib/include/fwts-oem.h
> @@ -0,0 +1,53 @@
> +
> +#ifndef __FWTS_OEM_H__
> +#define __FWTS_OEM_H__
> +
> +//#define OEM_WIRELESS_CMD	0x01

This #define should be removed if it's not being used.

> +
> +#define	u8	int8_t
> +#define	u16	int16_t

Hmmmm. u8 is uint8_t isn't it and not int8_t?  For the userspace side, 
why can't we just use the stdint.h types (uint8_t, etc) rather than try 
and mush it around #defines?

> +
> +typedef struct {
> +	union {
> +		u16 func;
> +		u16 func_status;
> +	};
> +} fwts_oem_parameter;
> +
> +#define FWTS_SUCCESS		0
> +#define FWTS_FAIL		1
> +#define FWTS_NOT_PRESENT	2

Perhaps FWTS_OEM_SUCCESS, FWTS_OEM_FAIL, FWTS_OEM_NOT_PRESENT are better 
names for this header.

> +
> +enum fwts_oem_wireless_cmd {
> +	GET_DEVICE	=	0x00,
> +	SET_DEVICE	=	0x01,
> +};
> +
> +enum fwts_oem_wireless_type {
> +	FWTS_WIFI	=	0x00,
> +	FWTS_BLUETOOTH	=	0x01,
> +	FWTS_WWAN	=	0x03,
> +	FWTS_UNKNOWN	=	0xFF,
> +};
> +
> +typedef struct {
> +	u8	device_type;
> +	u8	device_bus;
> +	u16	vendor_id;
> +	u16	device_id;
> +	u16	sub_vendor_id;
> +	u16	sub_device_id;
> +	u8	soft_kill_status;
> +	u8	hard_kill_status;
> +} fwts_oem_wireless_device;
> +
> +struct fwts_oem_wireless {
> +	fwts_oem_parameter		oem_parameter;
> +	fwts_oem_wireless_device	device;
> +} __attribute__ ((packed));

Unless we are doing a 1-to-1 mapping of a struct onto I/O registers we 
don't really need packed here. Also, why not use:

typedef struct {

} fwts_oem_wireless;

..since you are already doing that with fwts_oem_wireless_device and 
that's the convention I try use in fwts where possible.

> +
> +#define OEM_WIRELESS_CMD \
> +        _IOWR('p', 0x01, struct fwts_oem_wireless)
> +
> +
> +#endif
> diff --git a/src/oem/wireless/wireless.c b/src/oem/wireless/wireless.c
> new file mode 100644
> index 0000000..af14bd9
> --- /dev/null
> +++ b/src/oem/wireless/wireless.c
> @@ -0,0 +1,291 @@
> +/*
> + * Copyright (C) 2012 Canonical

2012-2013 now :-)

> + *
> + * 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 will 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 to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
> + * USA.
> + *
> + */
> +
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stddef.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +
> +#include "fwts.h"
> +#include "fwts-oem.h"
> +
> +static int get_wireless_device(int fd, int device, struct fwts_oem_wireless *wd)

Where possible, can you make fd and device const, e.g const int fd, etc

> +{
> +	long ioret;
> +
> +	wd->oem_parameter.func = GET_DEVICE;
> +	wd->device.device_type = device;
> +
> +	ioret = ioctl(fd, OEM_WIRELESS_CMD, wd);

ioctl() returns int; ioret has been declared long. ioret should be int.

> +	if (ioret)
> +		return FWTS_ERROR;
> +	return FWTS_OK;
> +}
> +
> +static int set_wireless_device(int fd, int device, int sw_status)
> +{
> +	struct fwts_oem_wireless wd;
> +	long ioret;
> +
> +	wd.oem_parameter.func = SET_DEVICE;
> +	wd.device.device_type = device;
> +	wd.device.soft_kill_status = sw_status;
> +	ioret = ioctl(fd, OEM_WIRELESS_CMD, &wd);

ioctl() returns int; ioret has been declared long. ioret should be int.

> +	if (ioret)
> +		return -1;
> +	return 0;
> +}
> +
> +static int test_wireless_device(fwts_framework *fw, int fd, int radio)
> +{
> +	struct fwts_oem_wireless wd;
> +	u8 sw_kill_status;
> +	int err;
> +
> +	err = get_wireless_device(fd, radio, &wd);
> +	if (err)
> +		goto test_failed;
> +
> +	if (wd.oem_parameter.func_status == FWTS_NOT_PRESENT)
> +		goto no_such_device;

How about making the ioctl() return -ENODEV rather than passing back a 
func_status FWTS_NOT_PRESENT?

I'd prefer:

	if (....) {
		fwts_skipped(fw, "Wireless device is not present.\n");	
		return FWTS_SKIP;
	}

rather than a goto.  And please capitalise the first letter in messages 
being written to the log.

> +
> +	sw_kill_status = wd.device.soft_kill_status;
> +	err = set_wireless_device(fd, radio, !wd.device.soft_kill_status);
> +	if (err)
> +		goto test_failed;

how about just:
		return FWTS_ERROR;

> +	err = get_wireless_device(fd, radio, &wd);
> +	if (err)
> +		goto test_failed;

		return FWTS_ERROR;

> +	if (sw_kill_status == wd.device.soft_kill_status)
> +		fwts_warning(fw, "failed to change from %x to %x",
> +					sw_kill_status, !sw_kill_status);
> +

I'd like to use inttypes.h for this and hex values should be prefix with 
0x to make it clear.

		fwts_warning(fw, "Failed to change from 0x%" PRIx8 " to 0x%" PRIx8 ",
			sw_kill_status, !sw_kill_status);

> +	sw_kill_status = wd.device.soft_kill_status;
> +	err = set_wireless_device(fd, radio, !wd.device.soft_kill_status);
> +	if (err)
> +		goto test_failed;

		return FWTS_ERROR;

> +	err = get_wireless_device(fd, radio, &wd);
> +	if (err)
> +		goto test_failed;

		return FWTS_ERROR;

> +	if (sw_kill_status == wd.device.soft_kill_status)
> +		fwts_warning(fw, "failed to change from %x to %x",
> +					sw_kill_status, !sw_kill_status);



> +
> +	fwts_passed(fw, "OEM wireless interface is able to toggle its state\n");
> +
> +	return FWTS_OK;
> +
> + no_such_device:
> +	fwts_skipped(fw, "wireless device is not present.\n");
> +	return FWTS_SKIP;
> +
> + test_failed:
> +	return FWTS_ERROR;
> +}
> +
> +static int oem_wireless_init(fwts_framework *fw)
> +{
> +	int ret = FWTS_OK;
> +	int fd;
> +
> +	fd = open("/dev/fwts_oem", O_RDONLY);
> +	if (fd == -1) {
> +		fwts_log_info(fw, "Cannot open fwts_oem driver. Aborted.");
> +		ret = FWTS_ABORTED;
> +	}
> +
> +	close(fd);
> +	return ret;
> +}

I was under the impression that the init and de-init code would load the 
module.  This code seems to assume it's already loaded.

> +
> +static int oem_wireless_deinit(fwts_framework *fw)
> +{
> +	FWTS_UNUSED(fw);
> +	return FWTS_OK;
> +}

..and unloading the module perhaps?

> +
> +static int oem_wireless_test1(fwts_framework *fw)
> +{
> +	int ret = FWTS_OK;
> +	int fd;
> +
> +	fd = open("/dev/fwts_oem", O_RDONLY);
> +
> +	if (fd == -1) {
> +		fwts_log_info(fw, "Cannot open fwts_oem driver. Aborted.");
> +		ret = FWTS_ABORTED;
> +	} else
> +		ret = test_wireless_device(fw, fd, FWTS_WIFI);
> +
> +	close(fd);

close(fd) when fd == -1 is a bug. I suggest:

	if (fd == -1) {
		fwts_log_info(....)
		return FWTS_ABORTED;
	}
	
	ret = test_wireless_device(fw, fd, FWTS_WIFI);
	close(fd);

	return ret;

> +	return ret;
> +}
> +
> +static int oem_wireless_test2(fwts_framework *fw)
> +{
> +	int ret = FWTS_OK;
> +	int fd;
> +
> +	fd = open("/dev/fwts_oem", O_RDONLY);
> +
> +	if (fd == -1) {
> +		fwts_log_info(fw, "Cannot open fwts_oem driver. Aborted.");
> +		ret = FWTS_ABORTED;
> +	} else
> +		ret = test_wireless_device(fw, fd, FWTS_BLUETOOTH);
> +
> +	close(fd);
> +	return ret;

again, close(fd) when fd == -1 is not good.

> +}
> +
> +static int oem_wireless_test3(fwts_framework *fw)
> +{
> +	int ret;
> +	int fd;
> +
> +	fd = open("/dev/fwts_oem", O_RDONLY);
> +
> +	if (fd == -1) {
> +		fwts_log_info(fw, "Cannot open fwts_oem driver. Aborted.");
> +		ret = FWTS_ABORTED;
> +	} else
> +		ret = test_wireless_device(fw, fd, FWTS_WWAN);
> +
> +	close(fd);
> +	return ret;

again, close(fd) when fd == -1 is not good.

> +}
> +
> +
> +static int test_wireless_device_hard_kill(fwts_framework *fw, int fd)
> +{
> +	int i, j;
> +	int err;
> +	struct fwts_oem_wireless wd;
> +	u8 hw_kill_status;

I'd rather use uint8_t from stdint for hw_kill_status if possible.

> +
> +	err = get_wireless_device(fd, FWTS_WIFI, &wd);
> +	if (err || wd.oem_parameter.func_status == FWTS_NOT_PRESENT)
> +		goto no_wifi_device;
> +
> +	for (j = 0; j < 2; j++) {
> +		i = 0;
> +		hw_kill_status = wd.device.hard_kill_status;
> +		fwts_printf(fw, "Please switch hard kill for WIFI to %s.\n",
> +					hw_kill_status ? "on" : "off");
> +		while (wd.device.hard_kill_status == hw_kill_status && i < 5) {
> +			sleep(1);
> +			fwts_printf(fw, "Waiting %2.2d/%d\r", 5 - i, 5);
> +			get_wireless_device(fd, FWTS_WIFI, &wd);
> +			i++;
> +		}
> +		if (wd.device.hard_kill_status == hw_kill_status)
> +			goto test_failed;
> +
> +	}

I believe other tests in fwts give the user 20 seconds. 5 is quite short 
IMHO.

> +
> + no_wifi_device:
> +
> +	err = get_wireless_device(fd, FWTS_BLUETOOTH, &wd);
> +	if (err || wd.oem_parameter.func_status == FWTS_NOT_PRESENT)
> +		goto no_bt_device;
> +
> +	for (j = 0; j < 2; j++) {
> +		i = 0;
> +		hw_kill_status = wd.device.hard_kill_status;
> +		fwts_printf(fw, "Please switch hard kill for BT to %s.\n",
> +					hw_kill_status ? "on" : "off");
> +		while (wd.device.hard_kill_status == hw_kill_status && i < 5) {
> +			sleep(1);
> +			fwts_printf(fw, "Waiting %2.2d/%d\r", 5 - i, 5);
> +			get_wireless_device(fd, FWTS_BLUETOOTH, &wd);
> +			i++;
> +		}
> +		if (wd.device.hard_kill_status == hw_kill_status)
> +			goto test_failed;
> +
> +	}
> +
> + no_bt_device:
> +
> +	err = get_wireless_device(fd, FWTS_WWAN, &wd);
> +	if (err || wd.oem_parameter.func_status == FWTS_NOT_PRESENT)
> +		goto no_wwan_device;
> +
> +	for (j = 0; j < 2; j++) {
> +		i = 0;
> +		hw_kill_status = wd.device.hard_kill_status;
> +		fwts_printf(fw, "Please switch hard kill for WWAN to %s.\n",
> +					hw_kill_status ? "on" : "off");
> +		while (wd.device.hard_kill_status == hw_kill_status && i < 5) {
> +			sleep(1);
> +			fwts_printf(fw, "Waiting %2.2d/%d\r", 5 - i, 5);
> +			get_wireless_device(fd, FWTS_WWAN, &wd);
> +			i++;
> +		}
> +		if (wd.device.hard_kill_status == hw_kill_status)
> +			goto test_failed;
> +	}
> +
> + no_wwan_device:
> +
> +	fwts_passed(fw, "OEM wireless hard kill test completed.\n");
> +	return FWTS_OK;
> +
> + test_failed:
> +	fwts_failed(fw, LOG_LEVEL_HIGH, "OEM wireless hard kill test failed.", "\n");
> +	return FWTS_ERROR;


How about breaking this test into 3 tests, for Bluetooth, Wifi and WWAN?

> +
> +}
> +
> +static int oem_wireless_test4(fwts_framework *fw)
> +{
> +	int ret;
> +	int fd;
> +
> +	fd = open("/dev/fwts_oem", O_RDONLY);
> +
> +	if (fd == -1) {
> +		fwts_log_info(fw, "Cannot open fwts_oem driver. Aborted.");
> +		ret = FWTS_ABORTED;
> +	} else
> +		ret = test_wireless_device_hard_kill(fw, fd);
> +
> +	close(fd);

again, close(fd) when fd == -1 is not good.

> +	return ret;
> +}
> +
> +static fwts_framework_minor_test oem_wireless_tests[] = {
> +	{ oem_wireless_test1, "Test OEM's WIFI control interfaces." },
> +	{ oem_wireless_test2, "Test OEM's Bluetooth control interfaces." },
> +	{ oem_wireless_test3, "Test OEM's WWAN control interfaces." },
> +	{ oem_wireless_test4, "Test OEM's hard block switch interface." },
> +	{ NULL, NULL }
> +};
> +
> +static fwts_framework_ops oem_wireless_ops = {
> +	.description = "OEM wireless control interface tests.",
> +	.init        = oem_wireless_init,
> +	.deinit      = oem_wireless_deinit,
> +	.minor_tests = oem_wireless_tests
> +};
> +
> +FWTS_REGISTER(oem_wireless, &oem_wireless_ops, FWTS_TEST_ANYTIME,
> +				FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);

test3 is interactive, so we need the FWTS_FLAG_INTERACTIVE flag too.

>