[PATCH 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.
Colin Ian King
colin.king at canonical.com
Wed Jan 2 12:15:03 UTC 2013
On 02/01/13 08:15, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at 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.
>
More information about the fwts-devel
mailing list