[PATCH 1/2] oem: add fwts-hp-wmi module for testing hp wireless control interfaces.
Colin Ian King
colin.king at canonical.com
Wed Jan 2 13:08:14 UTC 2013
On 02/01/13 08:15, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
> debian/control | 8 +
> debian/fwts-hp-wmi-dkms.dkms | 6 +
> debian/rules | 4 +
> oem/Makefile | 6 +
> oem/fwts-hp-wmi.c | 452 ++++++++++++++++++++++++++++++++++++++++++
> oem/fwts-oem.h | 53 +++++
> 6 files changed, 529 insertions(+)
> create mode 100644 debian/fwts-hp-wmi-dkms.dkms
> create mode 100644 oem/Makefile
> create mode 100644 oem/fwts-hp-wmi.c
> create mode 100644 oem/fwts-oem.h
I'd rather not keep adding directories containing source to the top
level, can we put this oem driver into something like:
src/drivers/oem
I'm also thinking of putting the uefi driver into src/drivers/uefi
sometime soon...
>
> diff --git a/debian/control b/debian/control
> index f299037..2022115 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -60,3 +60,11 @@ Depends: ${shlibs:Depends}, ${misc:Depends}, dkms, linux-headers-generic | linux
> Description: Firmware Test Suite UEFI Runtime Service kernel driver
> This package provides the efi_runtime kernel driver in DKMS format,
> which is required for accessing UEFI Runtime Services.
> +
> +Package: fwts-hp-wmi-dkms
> +Architecture: any
> +Priority: optional
> +Depends: ${shlibs:Depends}, ${misc:Depends}, dkms, linux-headers-generic | linux-headers
> +Description: Firmware Test Suite OEM Runtime Service for HP WMI device
> + This package provides the fwts-hp-wmi kernel driver in DKMS format,
> + which is required for accessing HP WMI runtime Services.
> diff --git a/debian/fwts-hp-wmi-dkms.dkms b/debian/fwts-hp-wmi-dkms.dkms
> new file mode 100644
> index 0000000..69a9fba
> --- /dev/null
> +++ b/debian/fwts-hp-wmi-dkms.dkms
> @@ -0,0 +1,6 @@
> +PACKAGE_NAME="fwts-hp-wmi-dkms"
> +PACKAGE_VERSION="#MODULE_VERSION#"
> +MAKE[0]="make"
> +BUILT_MODULE_NAME[0]="fwts-hp-wmi"
> +DEST_MODULE_LOCATION[0]="/updates"
> +AUTOINSTALL="yes"
> diff --git a/debian/rules b/debian/rules
> index 503c19d..0a1554e 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -8,11 +8,15 @@ DEBVERS := $(shell dpkg-parsechangelog | grep ^Version: | cut -d' ' -f2 \
> VERSION := $(shell echo '$(DEBVERS)' | sed -e 's/[+-].*//' -e 's/~//g')
>
> DKMS_SRC_DIR := $(CURDIR)/debian/fwts-efi-runtime-dkms/usr/src/fwts-efi-runtime-dkms-$(VERSION)
> +DKMS_SRC_OEM_DIR := $(CURDIR)/debian/fwts-hp-wmi-dkms/usr/src/fwts-hp-wmi-dkms-$(VERSION)
>
> override_dh_auto_install:
> install -d $(DKMS_SRC_DIR)
> cp -a efi_runtime/* $(DKMS_SRC_DIR)
> dh_auto_install
> + install -d $(DKMS_SRC_OEM_DIR)
> + cp -a oem/* $(DKMS_SRC_OEM_DIR)
> + dh_auto_install
>
> override_dh_dkms:
> dh_dkms -V $(VERSION)
> diff --git a/oem/Makefile b/oem/Makefile
> new file mode 100644
> index 0000000..c8970a6
> --- /dev/null
> +++ b/oem/Makefile
> @@ -0,0 +1,6 @@
> +obj-m += fwts-hp-wmi.o
> +all:
> + make -C /lib/modules/`uname -r`/build M=`pwd` modules
> +
> +clean:
> + make -C /lib/modules/`uname -r`/build M=`pwd` clean
> diff --git a/oem/fwts-hp-wmi.c b/oem/fwts-hp-wmi.c
> new file mode 100644
> index 0000000..8948a7a
> --- /dev/null
> +++ b/oem/fwts-hp-wmi.c
> @@ -0,0 +1,452 @@
> +/*
> + * Copyright (C) 2011-2012 Canonical
How about 2012-2013 (as I am sure this wasn't first written in 2011)
> + *
> + * Portions based on fwts-hp-wmi.c:
> + * Copyright (C) 2008 Red Hat <mjg at redhat.com>
> + * Copyright (C) 2010, 2011 Anssi Hannula <anssi.hannula at iki.fi>
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME "-fwts: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/string.h>
> +
> +#include <linux/proc_fs.h>
> +#include <linux/miscdevice.h>
> +
> +#include "fwts-oem.h"
> +
> +MODULE_AUTHOR("Alex Hung <alex.hung at canonical.com>");
> +MODULE_LICENSE("GPL");
> +
> +MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C");
> +MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
> +
> +#define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
> +#define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> +
> +#define HPWMI_DISPLAY_QUERY 0x1
> +#define HPWMI_HDDTEMP_QUERY 0x2
> +#define HPWMI_ALS_QUERY 0x3
> +#define HPWMI_HARDWARE_QUERY 0x4
> +#define HPWMI_WIRELESS_QUERY 0x5
> +#define HPWMI_HOTKEY_QUERY 0xc
> +#define HPWMI_WIRELESS2_QUERY 0x1b
> +
> +enum hp_wmi_radio {
> + HPWMI_WIFI = 0,
> + HPWMI_BLUETOOTH = 1,
> + HPWMI_WWAN = 2,
> +};
> +
> +enum hp_wmi_event_ids {
> + HPWMI_DOCK_EVENT = 1,
> + HPWMI_PARK_HDD = 2,
> + HPWMI_SMART_ADAPTER = 3,
> + HPWMI_BEZEL_BUTTON = 4,
> + HPWMI_WIRELESS = 5,
> + HPWMI_CPU_BATTERY_THROTTLE = 6,
> + HPWMI_LOCK_SWITCH = 7,
> +};
> +
> +static int __devinit hp_wmi_bios_setup(struct platform_device *device);
> +static int __exit hp_wmi_bios_remove(struct platform_device *device);
> +static int hp_wmi_resume_handler(struct device *device);
> +
> +struct bios_args {
> + u32 signature;
> + u32 command;
> + u32 commandtype;
> + u32 datasize;
> + u32 data;
> +};
> +
> +struct bios_return {
> + u32 sigpass;
> + u32 return_code;
> +};
> +
> +enum hp_return_value {
> + HPWMI_RET_WRONG_SIGNATURE = 0x02,
> + HPWMI_RET_UNKNOWN_COMMAND = 0x03,
> + HPWMI_RET_UNKNOWN_CMDTYPE = 0x04,
> + HPWMI_RET_INVALID_PARAMETERS = 0x05,
> +};
> +
> +enum hp_wireless2_bits {
> + HPWMI_POWER_STATE = 0x01,
> + HPWMI_POWER_SOFT = 0x02,
> + HPWMI_POWER_BIOS = 0x04,
> + HPWMI_POWER_HARD = 0x08,
> +};
> +
> +#define IS_HWBLOCKED(x) ((x & (HPWMI_POWER_BIOS | HPWMI_POWER_HARD)) \
> + != (HPWMI_POWER_BIOS | HPWMI_POWER_HARD))
> +#define IS_SWBLOCKED(x) !(x & HPWMI_POWER_SOFT)
> +
> +struct bios_rfkill2_device_state {
> + u8 radio_type;
> + u8 bus_type;
> + u16 vendor_id;
> + u16 product_id;
> + u16 subsys_vendor_id;
> + u16 subsys_product_id;
> + u8 rfkill_id;
> + u8 power;
> + u8 unknown[4];
> +};
> +
> +/* 7 devices fit into the 128 byte buffer */
> +#define HPWMI_MAX_RFKILL2_DEVICES 7
> +
> +struct bios_rfkill2_state {
> + u8 unknown[7];
> + u8 count;
> + u8 pad[8];
> + struct bios_rfkill2_device_state device[HPWMI_MAX_RFKILL2_DEVICES];
> +};
> +
> +static struct platform_device *hp_wmi_platform_dev;
> +
> +static const struct dev_pm_ops hp_wmi_pm_ops = {
> + .resume = hp_wmi_resume_handler,
> + .restore = hp_wmi_resume_handler,
> +};
> +
> +static struct platform_driver hp_wmi_driver = {
> + .driver = {
> + .name = "fwts-hp-wmi",
> + .owner = THIS_MODULE,
> + .pm = &hp_wmi_pm_ops,
> + },
> + .probe = hp_wmi_bios_setup,
> + .remove = hp_wmi_bios_remove,
> +};
> +
> +static int hp_wmi_perform_query(int query, int write, void *buffer,
> + int insize, int outsize)
> +{
> + struct bios_return *bios_return;
> + int actual_outsize;
> + union acpi_object *obj;
> + struct bios_args args = {
> + .signature = 0x55434553,
> + .command = write ? 0x2 : 0x1,
> + .commandtype = query,
> + .datasize = insize,
> + .data = 0,
> + };
> + struct acpi_buffer input = { sizeof(struct bios_args), &args };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + u32 rc;
> +
> + if (WARN_ON(insize > sizeof(args.data)))
> + return -EINVAL;
> + memcpy(&args.data, buffer, insize);
> +
> + wmi_evaluate_method(HPWMI_BIOS_GUID, 0, 0x3, &input, &output);
> +
> + obj = output.pointer;
> +
> + if (!obj)
> + return -EINVAL;
> + else if (obj->type != ACPI_TYPE_BUFFER) {
> + kfree(obj);
> + return -EINVAL;
> + }
> +
> + bios_return = (struct bios_return *)obj->buffer.pointer;
I think it may be wise to add a check to see if bios_return is NULL
before we dereference it.
> + rc = bios_return->return_code;
> +
> + if (rc) {
> + if (rc != HPWMI_RET_UNKNOWN_CMDTYPE)
> + pr_warn("query 0x%x returned error 0x%x\n", query, rc);
> + kfree(obj);
> + return rc;
> + }
> +
> + if (!outsize) {
> + /* ignore output data */
> + kfree(obj);
> + return 0;
> + }
> +
> + actual_outsize = min(outsize,
> + (int)(obj->buffer.length - sizeof(*bios_return)));
> + memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return),
> + actual_outsize);
> + memset(buffer + actual_outsize, 0, outsize - actual_outsize);
> + kfree(obj);
> + return 0;
> +}
> +
> +static int hp_wmi_type_1b_write(u8 power_ctrl, int state)
> +{
> + char buffer[4] = { 0x01, 0x00, power_ctrl, state };
> +
> + if (hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 1,
> + buffer, sizeof(buffer), 0)) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int hp_wmi_type_1b_read(struct bios_rfkill2_state *rf_info)
> +{
> + int err;
> +
> + err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 0, rf_info,
> + 0, sizeof(struct bios_rfkill2_state));
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int get_wireless_dev_structure_wmi_1b(
> + struct bios_rfkill2_device_state *rfkill_dev, enum hp_wmi_radio r)
> +{
> + struct bios_rfkill2_state rf_info;
> + struct bios_rfkill2_device_state *dev = NULL;
> + int err, i;
> + bool found = false;
> +
> + err = hp_wmi_type_1b_read(&rf_info);
> + if (err)
> + return -EINVAL;
> +
> + for (i = 0; i < rf_info.count; i++) {
> + dev = &rf_info.device[i];
> + if (dev->radio_type == r) {
> + found = true;
> + break;
> + }
> + }
> + if (!found)
> + return FWTS_NOT_PRESENT;
how about returning -ENODEV?
> + memcpy(rfkill_dev, dev, sizeof(struct bios_rfkill2_device_state));
> +
> + return 0;
> +}
> +
> +static int convert_wireless_device_type(int radio)
> +{
> + int dev_type = HPWMI_WIFI;
> +
> + switch (radio) {
> + case FWTS_WIFI:
> + dev_type = HPWMI_WIFI;
> + break;
> + case FWTS_BLUETOOTH:
> + dev_type = HPWMI_BLUETOOTH;
> + break;
> + case FWTS_WWAN:
> + dev_type = HPWMI_WWAN;
> + break;
> + default:
> + break;
> + }
is returning HPWMI_WIFI on the default radio case the correct thing to do?
> +
> + return dev_type;
> +}
> +
> +static int get_wireless_device(struct fwts_oem_wireless __user *wd)
> +{
> + struct bios_rfkill2_device_state rfdev;
> + int err;
> + int dev_type;
> +
> + dev_type = convert_wireless_device_type(wd->device.device_type);
Shouldn't we be getting data from user space usiong get_user()?
> + err = get_wireless_dev_structure_wmi_1b(&rfdev, dev_type);
> + if (err == FWTS_NOT_PRESENT) {
> + wd->oem_parameter.func_status = FWTS_NOT_PRESENT;
> + return 0;
> + } else if (err == FWTS_SUCCESS) {
I'd rather FWTS_NOT_PRESENT, FWTS_SUCCESS etc should be named
FWTS_OEM_NOT_PRESENT, FWTS_OEM_SUCCESS.. just to give the names some
context.
how about:
if (err == -ENODEV) {
wd->oem_parameter.func_status = FWTS_OEM_NOT_PRESENT;
return 0;
} else if (err == 0) {
> + wd->oem_parameter.func_status = FWTS_SUCCESS;
> + wd->device.device_bus = rfdev.bus_type;
> + wd->device.vendor_id = rfdev.vendor_id;
> + wd->device.device_id = rfdev.product_id;
> + wd->device.soft_kill_status = IS_SWBLOCKED(rfdev.power);
> + wd->device.hard_kill_status = IS_HWBLOCKED(rfdev.power);
> + return 0;
> + }
wd->oem_parameter.func_status is not being set on the -EINVAL case. Is
that correct?
Shouldn't we passing back data to user space using put_user()?
> +
> + return err;
> +}
> +
> +static int set_wireless_device(struct fwts_oem_wireless __user *wd)
> +{
> + int err;
> + struct bios_rfkill2_device_state dev;
> + int dev_type;
> + int target_power;
> +
> + dev_type = convert_wireless_device_type(wd->device.device_type);
> + err = get_wireless_dev_structure_wmi_1b(&dev, dev_type);
> + if (err)
> + return err;
> + target_power = !wd->device.soft_kill_status;
> + pr_info("soft_kill_status = %x\n", wd->device.soft_kill_status);
> + pr_info("target_power = %x\n", target_power);
I'd prefer any value being printed out in hex should have a 0x prefix so
we don't mistake it is a decimal or octal value.
> + err = hp_wmi_type_1b_write(dev.rfkill_id, target_power);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int handle_oem_wireless_cmd(struct fwts_oem_wireless __user *wd)
> +{
> + int ret = 0;
> + u8 cmd;
> +
> + cmd = wd->oem_parameter.func;
> + switch (cmd) {
> + case GET_DEVICE:
> + ret = get_wireless_device(wd);
> + break;
> + case SET_DEVICE:
> + pr_info("set_wireless_device\n");
> + ret = set_wireless_device(wd);
> + break;
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static long fwts_oem_runtime_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + switch (cmd) {
> + case OEM_WIRELESS_CMD:
> + handle_oem_wireless_cmd((struct fwts_oem_wireless __user *) arg);
> + break;
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> +static int fwts_oem_runtime_open(struct inode *inode, struct file *file)
> +{
> + return 0;
> +}
> +
> +static int fwts_oem_runtime_close(struct inode *inode, struct file *file)
> +{
> + return 0;
> +}
> +
> +static const struct file_operations fwts_oem_runtime_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = fwts_oem_runtime_ioctl,
> + .open = fwts_oem_runtime_open,
> + .release = fwts_oem_runtime_close,
> + .llseek = no_llseek,
> +};
> +
> +static struct miscdevice fwts_oem_runtime_dev = {
> + MISC_DYNAMIC_MINOR,
> + "fwts_oem",
> + &fwts_oem_runtime_fops
> +};
> +
> +static void cleanup_sysfs(struct platform_device *device)
> +{
> +
> +}
> +
> +
> +static int __devinit hp_wmi_bios_setup(struct platform_device *device)
> +{
> + return 0;
> +}
> +
> +static int __exit hp_wmi_bios_remove(struct platform_device *device)
> +{
> + cleanup_sysfs(device);
cleanup_sysfs() is a no-op, so why not just remove the call as well as
cleanup_sysfs()?
> +
> + return 0;
> +}
> +
> +static int hp_wmi_resume_handler(struct device *device)
> +{
> + return 0;
> +}
> +
> +static int __init hp_wmi_init(void)
> +{
> + int err;
> + int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
> + int bios_capable = wmi_has_guid(HPWMI_BIOS_GUID);
> +
> + if (bios_capable) {
> + err = platform_driver_register(&hp_wmi_driver);
> + if (err)
> + goto err_driver_reg;
> + hp_wmi_platform_dev = platform_device_alloc("fwts-hp-wmi", -1);
> + if (!hp_wmi_platform_dev) {
> + err = -ENOMEM;
> + goto err_device_alloc;
> + }
> + err = platform_device_add(hp_wmi_platform_dev);
> + if (err)
> + goto err_device_add;
> + }
> +
> + if (!bios_capable && !event_capable)
> + return -ENODEV;
> +
> + err = misc_register(&fwts_oem_runtime_dev);
> + if (err) {
> + printk(KERN_ERR "fwts_oem: can't misc_register on minor=%d\n",
> + MISC_DYNAMIC_MINOR);
fwts_oem? That should be fwts-hp-wmi shouldn't it?
> + return err;
> + }
> +
> + return 0;
> +
> +err_device_add:
> + platform_device_put(hp_wmi_platform_dev);
> +err_device_alloc:
> + platform_driver_unregister(&hp_wmi_driver);
> +err_driver_reg:
> +
> + return err;
> +}
> +
> +static void __exit hp_wmi_exit(void)
> +{
> + if (hp_wmi_platform_dev) {
> + platform_device_unregister(hp_wmi_platform_dev);
> + platform_driver_unregister(&hp_wmi_driver);
> + }
> +
> + misc_deregister(&fwts_oem_runtime_dev);
> +}
> +
> +module_init(hp_wmi_init);
> +module_exit(hp_wmi_exit);
> diff --git a/oem/fwts-oem.h b/oem/fwts-oem.h
> new file mode 100644
> index 0000000..e09499c
> --- /dev/null
> +++ b/oem/fwts-oem.h
> @@ -0,0 +1,53 @@
> +
> +#ifndef __FWTS_OEM_H__
> +#define __FWTS_OEM_H__
> +
> +//#define OEM_WIRELESS_CMD 0x01
Please remove commented out unused macros.
> +
> +#define u8 int8_t
> +#define u16 int16_t
u8 is an int8_t? uint8_t perhaps.
> +
> +typedef struct {
> + union {
> + u16 func;
> + u16 func_status;
> + };
> +} fwts_oem_parameter;
> +
> +#define FWTS_SUCCESS 0
> +#define FWTS_FAIL 1
> +#define FWTS_NOT_PRESENT 2
FWTS_OEM_SUCCESS, FWTS_OEM_FAIL, FWTS_OEM_NOT_PRESENT is preferred.
> +
> +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));
I'd prefer:
typedef struct {
..
} fwts_oem_wireless;
and packed is not really required.
> +
> +#define OEM_WIRELESS_CMD \
> + _IOWR('p', 0x01, struct fwts_oem_wireless)
> +
> +
> +#endif
>
More information about the fwts-devel
mailing list