login
Search
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 <mjg@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 | mjg59@srcf.ucam.org --
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 <mjg@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 <mjg59@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 ...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. --
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. -- Matthew Garrett <mjg59@srcf.ucam.org> --
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 --
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; 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 --
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 <mjg@redhat.com>
---
commit 95f4bc41ef256b8e3dfa94cabe1faf0776ac988c
Author: Matthew Garrett <mjg59@srcf.ucam.org>
Date: Mon Aug 4 17:57:40 2008 +0100
Fix use after free
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 | mjg59@srcf.ucam.org
--
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 <mjg@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
@@ -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,44 @@ static int update_bl_status(struct backlight_device *bd)
}
...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. --
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 <mjg@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
--- 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,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 ...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 --
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 | mjg59@srcf.ucam.org --
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 --
The firmware does not perform any compulsory change. -- Matthew Garrett | mjg59@srcf.ucam.org --
Hi Matthew! 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 --
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 | mjg59@srcf.ucam.org --
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 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 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 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 ...
applied to acpi-test. Corentin Chary, you're the driver maintainer, do you Ack? thanks, -Len --
