[PATCH 1/2] eeepc-laptop: Fix user after free

[Matthew Garrett]

eeepc-laptop uses the hwmon struct after unregistering the device,
causing an oops on module unload. Flip the ordering to fix.

Signed-off-by: Matthew Garrett <m...@redhat.com>

---

diff --git a/drivers/misc/eeepc-laptop.c b/drivers/misc/eeepc-laptop.c
index 9e8d79e..facdb98 100644
--- a/drivers/misc/eeepc-laptop.c
+++ b/drivers/misc/eeepc-laptop.c
@@ -553,9 +553,9 @@ static void eeepc_hwmon_exit(void)
hwmon = eeepc_hwmon_device;
if (!hwmon)
return ;
- hwmon_device_unregister(hwmon);
sysfs_remove_group(&hwmon->kobj,
&hwmon_attribute_group);
+ hwmon_device_unregister(hwmon);
eeepc_hwmon_device = NULL;
}

--
Matthew Garrett | mj...@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Matthew Garrett]

eeepc-laptop currently only sends key events via ACPI and has
non-standard rfkill control. Add an input device and use the rfkill
infrastructure.

Signed-off-by: Matthew Garrett <m...@redhat.com>

---

This attempts to ensure that bluetoth and wlan rfkill devices are only
created if the device is prseent, but I don't have a Bluetooth-enabled
Eee to hand so I'm not certain it's correct. Testing with rfkill-input
shows that the wifi interface works, though.

commit 0656cf909274db0e59bb570c2bddd242cf075e7f
Author: Matthew Garrett <mj...@srcf.ucam.org>
Date: Mon Aug 4 18:00:57 2008 +0100

Rationalise interfacse

diff --git a/drivers/misc/eeepc-laptop.c b/drivers/misc/eeepc-laptop.c
index facdb98..ad55e60 100644
--- a/drivers/misc/eeepc-laptop.c
+++ b/drivers/misc/eeepc-laptop.c
@@ -28,6 +28,8 @@
#include <acpi/acpi_drivers.h>
#include <acpi/acpi_bus.h>
#include <linux/uaccess.h>
+#include <linux/input.h>
+#include <linux/rfkill.h>

#define EEEPC_LAPTOP_VERSION "0.1"

@@ -125,6 +127,10 @@ struct eeepc_hotk {
by this BIOS */
uint init_flag; /* Init flags */
u16 event_count[128]; /* count for each event */
+ struct input_dev *inputdev;
+ u16 *keycode_map;
+ struct rfkill *eeepc_wlan_rfkill;
+ struct rfkill *eeepc_bluetooth_rfkill;
};

/* The actual device the driver binds to */
@@ -140,6 +146,27 @@ static struct platform_driver platform_driver = {

static struct platform_device *platform_device;

+struct key_entry {
+ char type;
+ u8 code;
+ u16 keycode;
+};
+
+enum { KE_KEY, KE_END };
+
+static struct key_entry eeepc_keymap[] = {
+ /* Sleep already handled via generic ACPI code */
+ {KE_KEY, 0x10, KEY_WLAN },
+ {KE_KEY, 0x12, KEY_PROG1 },
+ {KE_KEY, 0x13, KEY_MUTE },
+ {KE_KEY, 0x14, KEY_VOLUMEDOWN },
+ {KE_KEY, 0x15, KEY_VOLUMEUP },
+ {KE_KEY, 0x30, KEY_SWITCHVIDEOMODE },
+ {KE_KEY, 0x31, KEY_SWITCHVIDEOMODE },
+ {KE_KEY, 0x32, KEY_SWITCHVIDEOMODE },
+ {KE_END, 0},
+};
+
/*
* The hotkey driver declaration
*/
@@ -261,6 +288,32 @@ static int update_bl_status(struct backlight_device *bd)
}

/*
+ * Rfkill helpers
+ */
+
+static int eeepc_wlan_rfkill_set(void *data, enum rfkill_state state)
+{
+ return set_acpi(CM_ASL_WLAN, state);
+}
+
+static int eeepc_wlan_rfkill_state(void *data, enum rfkill_state *state)
+{
+ *state = get_acpi(CM_ASL_WLAN);
+ return 0;
+}
+
+static int eeepc_bluetooth_rfkill_set(void *data, enum rfkill_state state)
+{
+ return set_acpi(CM_ASL_BLUETOOTH, state);
+}
+
+static int eeepc_bluetooth_rfkill_state(void *data, enum rfkill_state *state)
+{
+ *state = get_acpi(CM_ASL_BLUETOOTH);
+ return 0;
+}
+
+/*
* Sys helpers
*/
static int parse_arg(const char *buf, unsigned long count, int *val)
@@ -311,13 +364,11 @@ static ssize_t show_sys_acpi(int cm, char *buf)
EEEPC_CREATE_DEVICE_ATTR(camera, CM_ASL_CAMERA);
EEEPC_CREATE_DEVICE_ATTR(cardr, CM_ASL_CARDREADER);
EEEPC_CREATE_DEVICE_ATTR(disp, CM_ASL_DISPLAYSWITCH);
-EEEPC_CREATE_DEVICE_ATTR(wlan, CM_ASL_WLAN);

static struct attribute *platform_attributes[] = {
&dev_attr_camera.attr,
&dev_attr_cardr.attr,
&dev_attr_disp.attr,
- &dev_attr_wlan.attr,
NULL
};

@@ -328,8 +379,64 @@ static struct attribute_group platform_attribute_group = {
/*
* Hotkey functions
*/
+static struct key_entry *eepc_get_entry_by_scancode(int code)
+{
+ struct key_entry *key;
+
+ for (key = eeepc_keymap; key->type != KE_END; key++)
+ if (code == key->code)
+ return key;
+
+ return NULL;
+}
+
+static struct key_entry *eepc_get_entry_by_keycode(int code)
+{
+ struct key_entry *key;
+
+ for (key = eeepc_keymap; key->type != KE_END; key++)
+ if (code == key->keycode && key->type == KE_KEY)
+ return key;
+
+ return NULL;
+}
+
+static int eeepc_getkeycode(struct input_dev *dev, int scancode, int *keycode)
+{
+ struct key_entry *key = eepc_get_entry_by_scancode(scancode);
+
+ if (key && key->type == KE_KEY) {
+ *keycode = key->keycode;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int eeepc_setkeycode(struct input_dev *dev, int scancode, int keycode)
+{
+ struct key_entry *key;
+ int old_keycode;
+
+ if (keycode < 0 || keycode > KEY_MAX)
+ return -EINVAL;
+
+ key = eepc_get_entry_by_scancode(scancode);
+ if (key && key->type == KE_KEY) {
+ old_keycode = key->keycode;
+ key->keycode = keycode;
+ set_bit(keycode, dev->keybit);
+ if (!eepc_get_entry_by_keycode(old_keycode))
+ clear_bit(old_keycode, dev->keybit);
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
static int eeepc_hotk_check(void)
{
+ const struct key_entry *key;
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
int result;

@@ -356,6 +463,31 @@ static int eeepc_hotk_check(void)
"Get control methods supported: 0x%x\n",
ehotk->cm_supported);
}
+ ehotk->inputdev = input_allocate_device();
+ if (!ehotk->inputdev) {
+ printk(EEEPC_INFO "Unable to allocate input device\n");
+ return 0;
+ }
+ ehotk->inputdev->name = "Asus EeePC extra buttons";
+ ehotk->inputdev->phys = EEEPC_HOTK_FILE "/input0";
+ ehotk->inputdev->id.bustype = BUS_HOST;
+ ehotk->inputdev->getkeycode = eeepc_getkeycode;
+ ehotk->inputdev->setkeycode = eeepc_setkeycode;
+
+ for (key = eeepc_keymap; key->type != KE_END; key++) {
+ switch (key->type) {
+ case KE_KEY:
+ set_bit(EV_KEY, ehotk->inputdev->evbit);
+ set_bit(key->keycode, ehotk->inputdev->keybit);
+ break;
+ }
+ }
+ result = input_register_device(ehotk->inputdev);
+ if (result) {
+ printk(EEEPC_INFO "Unable to register input device\n");
+ input_free_device(ehotk->inputdev);
+ return 0;
+ }
} else {
printk(EEEPC_ERR "Hotkey device not present, aborting\n");
return -EINVAL;
@@ -363,21 +495,6 @@ static int eeepc_hotk_check(void)
return 0;
}

-static void notify_wlan(u32 *event)
-{
- /* if DISABLE_ASL_WLAN is set, the notify code for fn+f2
- will always be 0x10 */
- if (ehotk->cm_supported & (0x1 << CM_ASL_WLAN)) {
- const char *method = cm_getv[CM_ASL_WLAN];
- int value;
- if (read_acpi_int(ehotk->handle, method, &value))
- printk(EEEPC_WARNING "Error reading %s\n",
- method);
- else if (value == 1)
- *event = 0x11;
- }
-}
-
static void notify_brn(void)
{
struct backlight_device *bd = eeepc_backlight_device;
@@ -386,14 +503,28 @@ static void notify_brn(void)

static void eeepc_hotk_notify(acpi_handle handle, u32 event, void *data)
{
+ static struct key_entry *key;
if (!ehotk)
return;
- if (event == NOTIFY_WLAN_ON && (DISABLE_ASL_WLAN & ehotk->init_flag))
- notify_wlan(&event);
if (event >= NOTIFY_BRN_MIN && event <= NOTIFY_BRN_MAX)
notify_brn();
acpi_bus_generate_proc_event(ehotk->device, event,
ehotk->event_count[event % 128]++);
+ if (ehotk->inputdev) {
+ key = eepc_get_entry_by_scancode(event);
+ if (key) {
+ switch (key->type) {
+ case KE_KEY:
+ input_report_key(ehotk->inputdev, key->keycode,
+ 1);
+ input_sync(ehotk->inputdev);
+ input_report_key(ehotk->inputdev, key->keycode,
+ 0);
+ input_sync(ehotk->inputdev);
+ break;
+ }
+ }
+ }
}

static int eeepc_hotk_add(struct acpi_device *device)
@@ -420,6 +551,37 @@ static int eeepc_hotk_add(struct acpi_device *device)
eeepc_hotk_notify, ehotk);
if (ACPI_FAILURE(status))
printk(EEEPC_ERR "Error installing notify handler\n");
+
+ if (get_acpi(CM_ASL_WLAN) != -1) {
+ ehotk->eeepc_wlan_rfkill = rfkill_allocate(&device->dev,
+ RFKILL_TYPE_WLAN);
+
+ if (!ehotk->eeepc_wlan_rfkill)
+ goto end;
+
+ ehotk->eeepc_wlan_rfkill->name = "eeepc-wlan";
+ ehotk->eeepc_wlan_rfkill->toggle_radio = eeepc_wlan_rfkill_set;
+ ehotk->eeepc_wlan_rfkill->get_state = eeepc_wlan_rfkill_state;
+ ehotk->eeepc_wlan_rfkill->user_claim_unsupported = 0;
+ rfkill_register(ehotk->eeepc_wlan_rfkill);
+ }
+
+ if (get_acpi(CM_ASL_BLUETOOTH) != -1) {
+ ehotk->eeepc_bluetooth_rfkill =
+ rfkill_allocate(&device->dev, RFKILL_TYPE_BLUETOOTH);
+
+ if (!ehotk->eeepc_bluetooth_rfkill)
+ goto end;
+
+ ehotk->eeepc_bluetooth_rfkill->name = "eeepc-bluetooth";
+ ehotk->eeepc_bluetooth_rfkill->toggle_radio =
+ eeepc_bluetooth_rfkill_set;
+ ehotk->eeepc_bluetooth_rfkill->get_state =
+ eeepc_bluetooth_rfkill_state;
+ ehotk->eeepc_bluetooth_rfkill->user_claim_unsupported = 0;
+ rfkill_register(ehotk->eeepc_bluetooth_rfkill);
+ }
+
end:
if (result) {
kfree(ehotk);
@@ -543,6 +705,12 @@ static void eeepc_backlight_exit(void)
{
if (eeepc_backlight_device)
backlight_device_unregister(eeepc_backlight_device);
+ if (ehotk->inputdev)
+ input_unregister_device(ehotk->inputdev);
+ if (ehotk->eeepc_wlan_rfkill)
+ rfkill_unregister(ehotk->eeepc_wlan_rfkill);
+ if (ehotk->eeepc_bluetooth_rfkill)
+ rfkill_unregister(ehotk->eeepc_bluetooth_rfkill);
eeepc_backlight_device = NULL;

[Matthew Garrett]

On Mon, Aug 04, 2008 at 07:53:38PM +0200, Ivo van Doorn wrote:

> On Monday 04 August 2008, Matthew Garrett wrote:
> > eeepc-laptop currently only sends key events via ACPI and has
> > non-standard rfkill control. Add an input device and use the rfkill
> > infrastructure.
> >
> > Signed-off-by: Matthew Garrett <m...@redhat.com>
>

> Please use the rfkill_force_state() to report state changes,
> that will ensure that the events are immediately send to the
> rfkill layer.

Why does writing to the sysfs file not generate an update implicitly?
This is purely a software rfkill device, the input hotkey just generates
KEY_WLAN and needs rfkill-input to handle it.

[Ivo van Doorn]

On Monday 04 August 2008, Matthew Garrett wrote:

> eeepc-laptop currently only sends key events via ACPI and has
> non-standard rfkill control. Add an input device and use the rfkill
> infrastructure.
>
> Signed-off-by: Matthew Garrett <m...@redhat.com>

Please use the rfkill_force_state() to report state changes,

that will ensure that the events are immediately send to the
rfkill layer.

Otherwise events will not be reported untill the next get_state() event.

Ivo

--

[Henrique de Moraes Holschuh]

On Mon, 04 Aug 2008, Matthew Garrett wrote:
> On Mon, Aug 04, 2008 at 07:53:38PM +0200, Ivo van Doorn wrote:
> > On Monday 04 August 2008, Matthew Garrett wrote:
> > > eeepc-laptop currently only sends key events via ACPI and has
> > > non-standard rfkill control. Add an input device and use the rfkill
> > > infrastructure.
> > >
> > > Signed-off-by: Matthew Garrett <m...@redhat.com>
> >
> > Please use the rfkill_force_state() to report state changes,
> > that will ensure that the events are immediately send to the
> > rfkill layer.
>
> Why does writing to the sysfs file not generate an update implicitly?

It does. rfkill_force_state() is used to propagate state changes done by
outside sources back into rfkill. All internal changes caused by rfkill or
known to rfkill are dealt with by rfkill itself.

If there are no such outside sources, you don't need rfkill_force_state() or
get_state() at all, as long as you set rfkill->state properly before you
call rfkill_register.

BTW: if it has a hardware rfkill switch that overrides the rfkill
controller, that DOES count as an outside source of changes.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

[Henrique de Moraes Holschuh]

On Mon, 04 Aug 2008, Matthew Garrett wrote:

> +static int eeepc_wlan_rfkill_set(void *data, enum rfkill_state state)
> +{
> + return set_acpi(CM_ASL_WLAN, state);
> +}
> +
> +static int eeepc_wlan_rfkill_state(void *data, enum rfkill_state *state)
> +{
> + *state = get_acpi(CM_ASL_WLAN);
> + return 0;
> +}

This might just be style, but I'd rather you didn't do this unless get_acpi
and set_acpi do take enum rfkill_state... Doing state conversion would be a
lot more future-proof, as well:

Something like:
set_acpi(CM_ASL_WLAN, (state == RFKILL_STATE_UNBLOCKED) ? 0 : 1);

And:

*state = get_acpi(CM_ASL_WLAN) ? RFKILL_STATE_SOFT_BLOCKED :
RFKILL_STATE_UNBLOCKED;

Adjust as needed.

> + ehotk->eeepc_wlan_rfkill->user_claim_unsupported = 0;

This is not needed, and if we get rid of user_claim_unsupported later, your
code wouldn't need changes if you get rid of that line.

I have not paid much attention to it yet, but so far I have failed to
understand what user_claim_unsupported is good for in the new style rfkill
interface.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

[Matthew Garrett]

eeepc-laptop uses the hwmon struct after unregistering the device,
causing an oops on module unload. Flip the ordering to fix.

Signed-off-by: Matthew Garrett <m...@redhat.com>

---

commit 95f4bc41ef256b8e3dfa94cabe1faf0776ac988c
Author: Matthew Garrett <mj...@srcf.ucam.org>
Date: Mon Aug 4 17:57:40 2008 +0100

Fix use after free

[Matthew Garrett]

eeepc-laptop currently only sends key events via ACPI and has
non-standard rfkill control. Add an input device and use the rfkill
infrastructure.

Signed-off-by: Matthew Garrett <m...@redhat.com>

---

Updated to explicitly set state at registration time and use the enums
rather than relying on their ordering (as suggested by Henrique)

diff --git a/drivers/misc/eeepc-laptop.c b/drivers/misc/eeepc-laptop.c
index facdb98..f7f1eae 100644
--- a/drivers/misc/eeepc-laptop.c
+++ b/drivers/misc/eeepc-laptop.c

@@ -261,6 +288,44 @@ static int update_bl_status(struct backlight_device *bd)

}

/*
+ * Rfkill helpers
+ */
+

+static int eeepc_wlan_rfkill_set(void *data, enum rfkill_state state)
+{

+ if (state == RFKILL_STATE_SOFT_BLOCKED)
+ return set_acpi(CM_ASL_WLAN, 0);
+ else
+ return set_acpi(CM_ASL_WLAN, 1);

+}
+
+static int eeepc_wlan_rfkill_state(void *data, enum rfkill_state *state)
+{

+ if (get_acpi(CM_ASL_WLAN) == 1)
+ *state = RFKILL_STATE_UNBLOCKED;
+ else
+ *state = RFKILL_STATE_SOFT_BLOCKED;

+ return 0;
+}
+

+static int eeepc_wlan_bluetooth_set(void *data, enum rfkill_state state)
+{
+ if (state == RFKILL_STATE_SOFT_BLOCKED)
+ return set_acpi(CM_ASL_BLUETOOTH, 0);
+ else
+ return set_acpi(CM_ASL_BLUETOOTH, 1);
+}
+
+static int eeepc_wlan_bluetooth_state(void *data, enum rfkill_state *state)
+{
+ if (get_acpi(CM_ASL_BLUETOOTH) == 1)
+ *state = RFKILL_STATE_UNBLOCKED;
+ else
+ *state = RFKILL_STATE_SOFT_BLOCKED;

+ return 0;
+}
+
+/*
* Sys helpers
*/
static int parse_arg(const char *buf, unsigned long count, int *val)

@@ -311,13 +376,11 @@ static ssize_t show_sys_acpi(int cm, char *buf)

EEEPC_CREATE_DEVICE_ATTR(camera, CM_ASL_CAMERA);
EEEPC_CREATE_DEVICE_ATTR(cardr, CM_ASL_CARDREADER);
EEEPC_CREATE_DEVICE_ATTR(disp, CM_ASL_DISPLAYSWITCH);
-EEEPC_CREATE_DEVICE_ATTR(wlan, CM_ASL_WLAN);

static struct attribute *platform_attributes[] = {
&dev_attr_camera.attr,
&dev_attr_cardr.attr,
&dev_attr_disp.attr,
- &dev_attr_wlan.attr,
NULL
};

@@ -328,8 +391,64 @@ static struct attribute_group platform_attribute_group = {

@@ -356,6 +475,31 @@ static int eeepc_hotk_check(void)

@@ -363,21 +507,6 @@ static int eeepc_hotk_check(void)

return 0;
}

-static void notify_wlan(u32 *event)
-{
- /* if DISABLE_ASL_WLAN is set, the notify code for fn+f2
- will always be 0x10 */
- if (ehotk->cm_supported & (0x1 << CM_ASL_WLAN)) {
- const char *method = cm_getv[CM_ASL_WLAN];
- int value;
- if (read_acpi_int(ehotk->handle, method, &value))
- printk(EEEPC_WARNING "Error reading %s\n",
- method);
- else if (value == 1)
- *event = 0x11;
- }
-}
-
static void notify_brn(void)
{
struct backlight_device *bd = eeepc_backlight_device;

@@ -386,14 +515,28 @@ static void notify_brn(void)

@@ -420,6 +563,47 @@ static int eeepc_hotk_add(struct acpi_device *device)

eeepc_hotk_notify, ehotk);
if (ACPI_FAILURE(status))
printk(EEEPC_ERR "Error installing notify handler\n");
+
+ if (get_acpi(CM_ASL_WLAN) != -1) {
+ ehotk->eeepc_wlan_rfkill = rfkill_allocate(&device->dev,
+ RFKILL_TYPE_WLAN);
+
+ if (!ehotk->eeepc_wlan_rfkill)
+ goto end;
+
+ ehotk->eeepc_wlan_rfkill->name = "eeepc-wlan";
+ ehotk->eeepc_wlan_rfkill->toggle_radio = eeepc_wlan_rfkill_set;
+ ehotk->eeepc_wlan_rfkill->get_state = eeepc_wlan_rfkill_state;

+ if (get_acpi(CM_ASL_WIFI) == 1)
+ ehotk->eeepc_wlan_rfkill->state =
+ RFKILL_STATE_UNBLOCKED;
+ else
+ ehotk->eeepc_wlan_rfkill->state =
+ RFKILL_STATE_SOFT_BLOCKED;

+ rfkill_register(ehotk->eeepc_wlan_rfkill);
+ }
+
+ if (get_acpi(CM_ASL_BLUETOOTH) != -1) {
+ ehotk->eeepc_bluetooth_rfkill =
+ rfkill_allocate(&device->dev, RFKILL_TYPE_BLUETOOTH);
+
+ if (!ehotk->eeepc_bluetooth_rfkill)
+ goto end;
+
+ ehotk->eeepc_bluetooth_rfkill->name = "eeepc-bluetooth";
+ ehotk->eeepc_bluetooth_rfkill->toggle_radio =
+ eeepc_bluetooth_rfkill_set;
+ ehotk->eeepc_bluetooth_rfkill->get_state =
+ eeepc_bluetooth_rfkill_state;

+ if (get_acpi(CM_ASL_BLUETOOTH) == 1)
+ ehotk->eeepc_bluetooth_rfkill->state =
+ RFKILL_STATE_UNBLOCKED;
+ else
+ ehotk->eeepc_bluetooth_rfkill->state =
+ RFKILL_STATE_SOFT_BLOCKED;

+ rfkill_register(ehotk->eeepc_bluetooth_rfkill);
+ }
+
end:
if (result) {
kfree(ehotk);

@@ -543,6 +727,12 @@ static void eeepc_backlight_exit(void)

{
if (eeepc_backlight_device)
backlight_device_unregister(eeepc_backlight_device);
+ if (ehotk->inputdev)
+ input_unregister_device(ehotk->inputdev);
+ if (ehotk->eeepc_wlan_rfkill)
+ rfkill_unregister(ehotk->eeepc_wlan_rfkill);
+ if (ehotk->eeepc_bluetooth_rfkill)
+ rfkill_unregister(ehotk->eeepc_bluetooth_rfkill);

eeepc_backlight_device = NULL;

[Andrew Morton]

On Wed, 6 Aug 2008 10:25:23 +0100 Matthew Garrett <mj...@srcf.ucam.org> wrote:

> eeepc-laptop currently only sends key events via ACPI and has
> non-standard rfkill control. Add an input device and use the rfkill
> infrastructure.
>

drivers/misc/eeepc-laptop.c: In function 'eeepc_hotk_add':
drivers/misc/eeepc-laptop.c:577: error: 'CM_ASL_WIFI' undeclared (first use in this function)
drivers/misc/eeepc-laptop.c:577: error: (Each undeclared identifier is reported only once
drivers/misc/eeepc-laptop.c:577: error: for each function it appears in.)
drivers/misc/eeepc-laptop.c:595: error: 'eeepc_bluetooth_rfkill_set' undeclared (first use in this function)
drivers/misc/eeepc-laptop.c:597: error: 'eeepc_bluetooth_rfkill_state' undeclared (first use in this function)

maybe this had some dependencies which I didn't know about.

[Matthew Garrett]

eeepc-laptop currently only sends key events via ACPI and has
non-standard rfkill control. Add an input device and use the rfkill
infrastructure.

Signed-off-by: Matthew Garrett <m...@redhat.com>

---

Sorry, managed to send the wrong version before. This one actually
builds.

diff --git a/drivers/misc/eeepc-laptop.c b/drivers/misc/eeepc-laptop.c
index facdb98..017fa89 100644

+static int eeepc_bluetooth_rfkill_set(void *data, enum rfkill_state state)

+{
+ if (state == RFKILL_STATE_SOFT_BLOCKED)
+ return set_acpi(CM_ASL_BLUETOOTH, 0);
+ else
+ return set_acpi(CM_ASL_BLUETOOTH, 1);
+}
+

+static int eeepc_bluetooth_rfkill_state(void *data, enum rfkill_state *state)

+ if (get_acpi(CM_ASL_WLAN) == 1)

--
Matthew Garrett | mj...@srcf.ucam.org

[Henrique de Moraes Holschuh]

Looks fine to me at first glance. I do wonder if there are no events at all
you could hook to rfkill_force_state() instead of (or in addition to) using
the get_state()?

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

[Matthew Garrett]

On Tue, Aug 19, 2008 at 08:09:50PM -0300, Henrique de Moraes Holschuh wrote:
> Looks fine to me at first glance. I do wonder if there are no events at all
> you could hook to rfkill_force_state() instead of (or in addition to) using
> the get_state()?

The only change event is generated by hitting the wifi key, which ties
into rfkill-input. Won't that already force a state update on the event?

--
Matthew Garrett | mj...@srcf.ucam.org

[Henrique de Moraes Holschuh]

On Wed, 20 Aug 2008, Matthew Garrett wrote:
> On Tue, Aug 19, 2008 at 08:09:50PM -0300, Henrique de Moraes Holschuh wrote:
> > Looks fine to me at first glance. I do wonder if there are no events at all
> > you could hook to rfkill_force_state() instead of (or in addition to) using
> > the get_state()?
>
> The only change event is generated by hitting the wifi key, which ties
> into rfkill-input. Won't that already force a state update on the event?

Yes, but one must keep this in mind:

1. If the event has no bearing on compulsory hardware state changes (i.e.
the hardware won't change state by itself when the event happens, it is
really just reporting a simple key press that will do nothing by itself),
you just report the key as an event.

2. If the hardware/firmware *ALSO* compulsory changes the rfkill state (i.e.
the event also means the real rfkill controller state probably changed), you
take the opportunity to do a forced immediate state poll and
rfkill_force_state() the new state.

So, it basically depends whether the EEEPC hardware/firmware does (1) or
(2). If it is (1), the patch is correct. If it is (2), it should do a bit
more stuff that ends up with a call to rfkill_force_state().

The reason is that there is absolutely *NO* reason why rfkill-input (or
anything else for that matter) has to obey the input event. They can just
discard it if they want: it is a local system policy matter. Therefore, if
there is anything in *hardware* or *firmware* that is already acting on that
event, you *MUST* arrange for a call to rfkill_force_state().

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

[Matthew Garrett]

On Tue, Aug 19, 2008 at 10:18:29PM -0300, Henrique de Moraes Holschuh wrote:

> 1. If the event has no bearing on compulsory hardware state changes (i.e.
> the hardware won't change state by itself when the event happens, it is
> really just reporting a simple key press that will do nothing by itself),
> you just report the key as an event.

Yes. That's all I do.

> 2. If the hardware/firmware *ALSO* compulsory changes the rfkill state (i.e.
> the event also means the real rfkill controller state probably changed), you
> take the opportunity to do a forced immediate state poll and
> rfkill_force_state() the new state.

The firmware does not perform any compulsory change.

--
Matthew Garrett | mj...@srcf.ucam.org

[Henrique de Moraes Holschuh]

Hi Matthew!

On Wed, 20 Aug 2008, Matthew Garrett wrote:

> On Tue, Aug 19, 2008 at 10:18:29PM -0300, Henrique de Moraes Holschuh wrote:
>
> > 1. If the event has no bearing on compulsory hardware state changes (i.e.
> > the hardware won't change state by itself when the event happens, it is
> > really just reporting a simple key press that will do nothing by itself),
> > you just report the key as an event.
>
> Yes. That's all I do.
>
> > 2. If the hardware/firmware *ALSO* compulsory changes the rfkill state (i.e.
> > the event also means the real rfkill controller state probably changed), you
> > take the opportunity to do a forced immediate state poll and
> > rfkill_force_state() the new state.
>
> The firmware does not perform any compulsory change.

Then, I have no futher comments. Looks good to me.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

[Matthew Garrett]

On Tue, Aug 19, 2008 at 10:32:14PM -0300, Henrique de Moraes Holschuh wrote:

> Then, I have no futher comments. Looks good to me.

Excellent, glad I've got that right.

One completely unrelated question. In the following situation (relevant
to Dells, not the Eee)

* The system has a key (not a switch) that in firmware disables the
hardware (HARD_BLOCKED)
* That key generates an event through the keyboard controller, but not
through any other obviously detectable means
* The radio control is also controllable through software (SOFT_BLOCKED)

Should pressing the key generate a KEY_WLAN event?

I note that rfkill-input will, if the device is in HARD_BLOCKED state,
attempt to set it to UNBLOCKED. This sounds like generating the keycode
is the wrong thing to do, since it'll cause rfkill-input to try to undo
the change that's just been made. However, if the key isn't mapped
there's no obvious way for any of the stack to determine that a change
has been made and propagate that to userspace. What should we be doing
here?

--
Matthew Garrett | mj...@srcf.ucam.org

[Henrique de Moraes Holschuh]

On Wed, 20 Aug 2008, Matthew Garrett wrote:

> One completely unrelated question. In the following situation (relevant
> to Dells, not the Eee)
>
> * The system has a key (not a switch) that in firmware disables the
> hardware (HARD_BLOCKED)

Ick. I sure hope you can query the firmware, so that you can look at it as
if it were a switch instead of a key (and look at the key press event as a
"switch changed state" event -- never mind it is difficult to hook to that
event right now)?

I mean, trying to deal with firmware/hardware that changes states on its own
in any other basis than "it is a switch", is error prone. You miss one
event, you go out of sync. That's bad.

> * That key generates an event through the keyboard controller, but not
> through any other obviously detectable means

Ok.

> * The radio control is also controllable through software (SOFT_BLOCKED)
>
> Should pressing the key generate a KEY_WLAN event?

Frankly? I think so. It would be nice if you could hook the key as a "hint
that the rfkill controller may have changed state" on the driver, and ALSO
as a normal input device (so that it can command more than just that one
WLAN radio).

But I think it would be EVEN BETTER if you could somehow suppress that KEY_*
event, and synthesize an EV_SW SW_WLAN in its place (you will have to ask
Dmitry to add it, since it is a first use) instead. If you cannot, KEY_WLAN
will have to make do.

> I note that rfkill-input will, if the device is in HARD_BLOCKED state,
> attempt to set it to UNBLOCKED. This sounds like generating the keycode

I have some patches in flight that will make rfkill-input smarter about it.
But that's just an enhancement. The current way it operates is annoying,
but last time I checked, it doesn't blow up.

Just return -EPERM on your device driver's toggle_radio() callback if you
are at HARD_BLOCKED and someone asks you to go to UNBLOCKED. That is what
the API calls for (if it is not clear enough, we can improve the docs).

> is the wrong thing to do, since it'll cause rfkill-input to try to undo
> the change that's just been made. However, if the key isn't mapped
> there's no obvious way for any of the stack to determine that a change
> has been made and propagate that to userspace. What should we be doing
> here?

Never worry about propagating rfkill state to userspace in a driver. rfkill
will do it by itself using uevents, that code has already been accepted.
The rfkill class will do it for all rfkill controllers, and rfkill input may
be taught to do it later if userspace people ask for it (nobody did it yet
because it is not very useful, since what you want is reports of what really
IS happening to the radios, and those come from the rfkill class. All
rfkill-input could tell userspace is what it is *trying* to change radio
states, but not whether they did really happen).

The reason why you'd want to send a KEY_WLAN event (or, if you can, an EV_SW
SW_WLAN event) is to change that key from something that controls an
specific WLAN radio, to something that can *potentially* control every WLAN
radio attached to the box. It is *not* done to report status to userspace
[unless you meat report state to something in userspace that is doing what
rfkill-input does in the kernel, but we haven't enhanced rfkill-input and
rfkill to export all states needed for such an userspace implementation
yet].

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

[Len Brown]

On Tue, 19 Aug 2008, Matthew Garrett wrote:

> eeepc-laptop currently only sends key events via ACPI and has
> non-standard rfkill control. Add an input device and use the rfkill
> infrastructure.
>
> Signed-off-by: Matthew Garrett <m...@redhat.com>

applied to acpi-test.
Corentin Chary, you're the driver maintainer, do you Ack?

thanks,
-Len