platform: hp_accel: add a i8042 filter to remove accelerometer data

Hello,

this patch fixes bug #84941 from the kernel bugzilla. Basically, it seems that the accelerometer sends some signals as button presses through the keyboard bus. The keys in the report are 0xa5-0xa8 but in the filter function they are reported as 0x25-0x28. This patch adds a

i8042 filter that removes these scancodes from the keyboard stream in a similar fashion to how idealpad_sidebar.c does this. I've done a RFC because I'm not sure if there is more portable way to do this and if these codes are the same for all machines. So could please someone respond who uses this driver and tell which invalid keypresses appear (if they do) in your dmesg that are reported by atkbd?

Also, I'm not sure if there is a publicly available documentation for hp 3d driveguard (I couldn't find it). That would definetly make it clear if this patch is correct or not.

Adding a signed off by line incase you find this patch good and want to apply it.

Signed-off-by Giedrius Statkevičius <[email protected]>

---
 drivers/platform/x86/hp_accel.c | 42 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

drivers/platform/x86/hp_accel.c +10 -1

index 13e14ec..b1bea8c 100644
@@ -38,6 +38,8 @@
 #include <linux/atomic.h>
 #include <linux/acpi.h>
 #include "../../misc/lis3lv02d/lis3lv02d.h"
+#include <linux/i8042.h>
+#include <linux/serio.h>
 
 #define DRIVER_NAME     "hp_accel"
 #define ACPI_MDPS_CLASS "accelerometer"

@@ -73,6 +75,13 @@ static inline void delayed_sysfs_set(struct led_classdev *led_cdev,
 
 /* HP-specific accelerometer driver ------------------------------------ */
 
+/* Codes of signals that the accelerometer sends
+ * through the keyboard bus */
+#define ACCEL_1 0x25
+#define ACCEL_2 0x26
+#define ACCEL_3 0x27
+#define ACCEL_4 0x28
+
 /* For automatic insertion of the module */
 static const struct acpi_device_id lis3lv02d_device_ids[] = {
 	{"HPQ0004", 0}, /* HP Mobile Data Protection System PNP */

@@ -82,7 +91,6 @@ static const struct acpi_device_id lis3lv02d_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
 
-
 /**
  * lis3lv02d_acpi_init - ACPI _INI method: initialize the device.
  * @lis3: pointer to the device struct

@@ -294,6 +302,32 @@ static void lis3lv02d_enum_resources(struct acpi_device *device)
 		printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
 }
 
+static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,

drivers/platform/x86/hp_accel.c +9 -0

+	if (likely(data != ACCEL_1) && likely(data != ACCEL_2) &&
+	    likely(data != ACCEL_3) && likely(data != ACCEL_4)) {
+		serio_interrupt(port, 0xe0, 0);
+		return false;
+	}
+
+	return true;
+}
+
 static int lis3lv02d_add(struct acpi_device *device)
 {
 	int ret;

drivers/platform/x86/hp_accel.c +2 -0

+
 	INIT_WORK(&hpled_led.work, delayed_set_status_worker);
 	ret = led_classdev_register(NULL, &hpled_led.led_classdev);
 	if (ret) {

@@ -343,6 +382,7 @@ static int lis3lv02d_remove(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	i8042_remove_filter(hp_accel_i8042_filter);
 	lis3lv02d_joystick_disable(&lis3_dev);
 	lis3lv02d_poweroff(&lis3_dev);
 

Signature · 1 line

2.1.2

Quoted 1 line · Giedrius Statkevicius

Hello,

Hi Giedrius,

Quoted 4 lines

this patch fixes bug #84941 from the kernel bugzilla. Basically, it seems that the accelerometer sends some signals as button presses through the keyboard bus. The keys in the report are 0xa5-0xa8 but in the filter function they are reported as 0x25-0x28. This patch adds a

Does this mean you were able to test it? On which platform did you test?

Quoted 11 lines

i8042 filter that removes these scancodes from the keyboard stream in a similar fashion to how idealpad_sidebar.c does this. I've done a RFC because I'm not sure if there is more portable way to do this and if these codes are the same for all machines. So could please someone respond who uses this driver and tell which invalid keypresses appear (if they do) in your dmesg that are reported by atkbd?

Also, I'm not sure if there is a publicly available documentation for hp 3d driveguard (I couldn't find it). That would definetly make it clear if this patch is correct or not.

Quoted 4 lines

Adding a signed off by line incase you find this patch good and want to apply it.

Signed-off-by Giedrius Statkevičius <[email protected]>

As it appears this is untested and you are looking for testers, I'm going to wait for a Tested-by from someone with hardware. Please follow-up if that fails to happen.

More below...

drivers/platform/x86/hp_accel.c static void lis3lv02d_enum_resources(struct acpi_device *device) · line 315

Quoted 11 lines

drivers/platform/x86/hp_accel.c +11 -0

+static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,
+				  struct serio *port)
+{
+	static bool extended;
+
+	if (str & I8042_STR_AUXDATA)
+		return false;
+
+	if (data == 0xe0) {
+		extended = true;
+		return true;

If you are going to return, why bother setting extended?

drivers/platform/x86/hp_accel.c static void lis3lv02d_enum_resources(struct acpi_device *device) · line 319

Quoted 4 lines

drivers/platform/x86/hp_accel.c +4 -0

+	}
+
+	if (!extended)
+		return false;

I'm now confused :-)

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	extended = false;

Wait... what?

Please review the use of extended here as well as the possibility of using it uninitialized.

Quoted 17 lines

drivers/platform/x86/hp_accel.c +9 -0

+	if (likely(data != ACCEL_1) && likely(data != ACCEL_2) &&
+	    likely(data != ACCEL_3) && likely(data != ACCEL_4)) {
+		serio_interrupt(port, 0xe0, 0);
+		return false;
+	}
+
+	return true;
+}
+
 static int lis3lv02d_add(struct acpi_device *device)
 {
 	int ret;

@@ -326,6 +360,11 @@ static int lis3lv02d_add(struct acpi_device *device)
 	if (ret)
 		return ret;
 

drivers/platform/x86/hp_accel.c static int lis3lv02d_add(struct acpi_device *device) · line 366

Quoted 4 lines

drivers/platform/x86/hp_accel.c +4 -0

+	/* filter to remove accelerometer data from keyboard bus stream */
+	ret = i8042_install_filter(hp_accel_i8042_filter);
+	if (ret)
+		i8042_remove_filter(hp_accel_i8042_filter);

If it failed to install, you don't need to remove it afterward. See the implementation for i8042_install_filter().

My questions are these:
- Does any system with the accelerometer whose ACPI id is HPQ0004 or HPQ6007 run into the same issues?
- If so, what are the scancodes reported by atkbd?
- If not, then where can I find some documentation to find about this? HP doesn't seem to have published any.

If other people have the same issue with the same scancodes on accelerometers with different ACPI ids we can go ahead and do what this patch does without reacting to what hardware the user has.

And a general question about what other people think of doing this? Maybe there is some better way I don't know of.

On the HP laptop I had (with HPQ0004), no fake keys were reported.

It should be noted that on my laptop, the accelerometer is completely decoupled from the hard disk. For example, when freefall is detected, nothing else happens (that's why you need to run a daemon that will listen to the event, and park the disk head). Looking at the bug report, it seems your laptop does a lot behind the OS, because apparently the disk head is parked automatically. So maybe the scancodes is a new

"feature" they have added in order to more easily report what's happening in the back.
Now, more related to your proposed solution: is this really what we want? What's wrong with the current state excepted for some warning messages in dmesg? Do we really want to plain drop this information

provided by the hardware? How about just associating the scancodes to meaningful keycodes? (I guess one reason no to do that is that it'd likely require creating new keycodes corresponding to these pretty atypical events in the input layer).

Is there maybe some general policy about what do to hardware that generate such special scancodes?