Discussion:
[PATCH RFC] platform: hp_accel: add a i8042 filter to remove accelerometer data
Giedrius Statkevicius
2014-10-18 20:59:22 UTC
Permalink
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 h=
p
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=C4=8Dius <***@gmail.com>
---
drivers/platform/x86/hp_accel.c | 42 +++++++++++++++++++++++++++++++++=
+++++++-
1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_=
accel.c
index 13e14ec..b1bea8c 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -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>
=20
#define DRIVER_NAME "hp_accel"
#define ACPI_MDPS_CLASS "accelerometer"
@@ -73,6 +75,13 @@ static inline void delayed_sysfs_set(struct led_clas=
sdev *led_cdev,
=20
/* HP-specific accelerometer driver ----------------------------------=
-- */
=20
+/* 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[] =3D {
{"HPQ0004", 0}, /* HP Mobile Data Protection System PNP */
@@ -82,7 +91,6 @@ static const struct acpi_device_id lis3lv02d_device_i=
ds[] =3D {
};
MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
=20
-
/**
* 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_d=
evice *device)
printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
}
=20
+static bool hp_accel_i8042_filter(unsigned char data, unsigned char st=
r,
+ struct serio *port)
+{
+ static bool extended;
+
+ if (str & I8042_STR_AUXDATA)
+ return false;
+
+ if (data =3D=3D 0xe0) {
+ extended =3D true;
+ return true;
+ }
+
+ if (!extended)
+ return false;
+
+ extended =3D false;
+ if (likely(data !=3D ACCEL_1) && likely(data !=3D ACCEL_2) &&
+ likely(data !=3D ACCEL_3) && likely(data !=3D 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 *devic=
e)
if (ret)
return ret;
=20
+ /* filter to remove accelerometer data from keyboard bus stream */
+ ret =3D i8042_install_filter(hp_accel_i8042_filter);
+ if (ret)
+ i8042_remove_filter(hp_accel_i8042_filter);
+
INIT_WORK(&hpled_led.work, delayed_set_status_worker);
ret =3D led_classdev_register(NULL, &hpled_led.led_classdev);
if (ret) {
@@ -343,6 +382,7 @@ static int lis3lv02d_remove(struct acpi_device *dev=
ice)
if (!device)
return -EINVAL;
=20
+ i8042_remove_filter(hp_accel_i8042_filter);
lis3lv02d_joystick_disable(&lis3_dev);
lis3lv02d_poweroff(&lis3_dev);
=20
--=20
2.1.2
Darren Hart
2014-10-21 21:45:08 UTC
Permalink
Post by Giedrius Statkevicius
Hello,
Hi Giedrius,
Post by Giedrius Statkevicius
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=
?
Post by Giedrius Statkevicius
i8042 filter that removes these scancodes from the keyboard stream in=
a
Post by Giedrius Statkevicius
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?
=20
Also, I'm not sure if there is a publicly available documentation for=
hp
Post by Giedrius Statkevicius
3d driveguard (I couldn't find it). That would definetly make it clea=
r
Post by Giedrius Statkevicius
if this patch is correct or not.
=20
Adding a signed off by line incase you find this patch good and want =
to
Post by Giedrius Statkevicius
apply it.
=20
As it appears this is untested and you are looking for testers, I'm goi=
ng to
wait for a Tested-by from someone with hardware. Please follow-up if th=
at fails
to happen.

More below...
Post by Giedrius Statkevicius
---
drivers/platform/x86/hp_accel.c | 42 +++++++++++++++++++++++++++++++=
+++++++++-
Post by Giedrius Statkevicius
1 file changed, 41 insertions(+), 1 deletion(-)
=20
diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/h=
p_accel.c
Post by Giedrius Statkevicius
index 13e14ec..b1bea8c 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -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>
=20
#define DRIVER_NAME "hp_accel"
#define ACPI_MDPS_CLASS "accelerometer"
@@ -73,6 +75,13 @@ static inline void delayed_sysfs_set(struct led_cl=
assdev *led_cdev,
Post by Giedrius Statkevicius
=20
/* HP-specific accelerometer driver --------------------------------=
---- */
Post by Giedrius Statkevicius
=20
+/* 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[] =3D {
{"HPQ0004", 0}, /* HP Mobile Data Protection System PNP */
@@ -82,7 +91,6 @@ static const struct acpi_device_id lis3lv02d_device=
_ids[] =3D {
Post by Giedrius Statkevicius
};
MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
=20
-
/**
* lis3lv02d_acpi_init - ACPI _INI method: initialize the device.
@@ -294,6 +302,32 @@ static void lis3lv02d_enum_resources(struct acpi=
_device *device)
Post by Giedrius Statkevicius
printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
}
=20
+static bool hp_accel_i8042_filter(unsigned char data, unsigned char =
str,
Post by Giedrius Statkevicius
+ struct serio *port)
+{
+ static bool extended;
+
+ if (str & I8042_STR_AUXDATA)
+ return false;
+
+ if (data =3D=3D 0xe0) {
+ extended =3D true;
+ return true;
If you are going to return, why bother setting extended?
Post by Giedrius Statkevicius
+ }
+
+ if (!extended)
+ return false;
I'm now confused :-)
Post by Giedrius Statkevicius
+
+ extended =3D false;
Wait... what?

Please review the use of extended here as well as the possibility
of using it uninitialized.
Post by Giedrius Statkevicius
+ if (likely(data !=3D ACCEL_1) && likely(data !=3D ACCEL_2) &&
+ likely(data !=3D ACCEL_3) && likely(data !=3D 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 *dev=
ice)
Post by Giedrius Statkevicius
if (ret)
return ret;
=20
+ /* filter to remove accelerometer data from keyboard bus stream */
+ ret =3D 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().
Post by Giedrius Statkevicius
+
INIT_WORK(&hpled_led.work, delayed_set_status_worker);
ret =3D led_classdev_register(NULL, &hpled_led.led_classdev);
if (ret) {
@@ -343,6 +382,7 @@ static int lis3lv02d_remove(struct acpi_device *d=
evice)
Post by Giedrius Statkevicius
if (!device)
return -EINVAL;
=20
+ i8042_remove_filter(hp_accel_i8042_filter);
lis3lv02d_joystick_disable(&lis3_dev);
lis3lv02d_poweroff(&lis3_dev);
=20
--=20
2.1.2
=20
=20
--=20
Darren Hart
Intel Open Source Technology Center
Giedrius Statkevicius
2014-10-22 13:20:31 UTC
Permalink
On Sat, Oct 18, 2014 at 11:59:22PM +0300, Giedrius Statkevicius wrote=
Post by Giedrius Statkevicius
Hello,
=20
Hi Giedrius,
=20
Post by Giedrius Statkevicius
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 i=
n
Post by Giedrius Statkevicius
the filter function they are reported as 0x25-0x28. This patch adds =
a
=20
Does this mean you were able to test it? On which platform did you te=
st?
=20
Sorry for not being specific enough. I have a HP ProBook 4540s laptop
with the sensor in it. It's ACPI id is HPQ6000. The reason I made this
patch is because I've been getting the following problem:

When I move the laptop I noticed that atkbd doesn't recognize some
keyboard "presses" which I wasn't doing and complains about that. The
range of the codes is in the patch: 0x25 - 0x28. The purpose of this
patch is to make hp_accel filter these out from the keyboard data
stream.

I've been testing this patch out on my system for about 5 days and
haven't ran into any issues.
Post by Giedrius Statkevicius
i8042 filter that removes these scancodes from the keyboard stream i=
n a
Post by Giedrius Statkevicius
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 appea=
r
Post by Giedrius Statkevicius
(if they do) in your `dmesg` that are reported by atkbd?
Also, I'm not sure if there is a publicly available documentation fo=
r hp
Post by Giedrius Statkevicius
3d driveguard (I couldn't find it). That would definetly make it cle=
ar
Post by Giedrius Statkevicius
if this patch is correct or not.
Adding a signed off by line incase you find this patch good and want=
to
Post by Giedrius Statkevicius
apply it.
=20
As it appears this is untested and you are looking for testers, I'm g=
oing to
wait for a Tested-by from someone with hardware. Please follow-up if =
that fails
to happen.
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=20
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.
=20
More below...
=20
[...]
Post by Giedrius Statkevicius
+static bool hp_accel_i8042_filter(unsigned char data, unsigned char=
str,
Post by Giedrius Statkevicius
+ struct serio *port)
+{
+ static bool extended;
+
+ if (str & I8042_STR_AUXDATA)
+ return false;
+
+ if (data =3D=3D 0xe0) {
+ extended =3D true;
+ return true;
=20
If you are going to return, why bother setting extended?
=20
Post by Giedrius Statkevicius
+ }
+
+ if (!extended)
+ return false;
=20
I'm now confused :-)
=20
Post by Giedrius Statkevicius
+
+ extended =3D false;
=20
Wait... what?
=20
Please review the use of extended here as well as the possibility
of using it uninitialized.
I'll review it and try to write it to be more concise and clearer. To b=
e
honest, I have used drivers/input/misc/ideapad_slidebar.c as a model to
write this function and I didn't like the way it is written. I just
thought: "Hey, this one driver already was checked by someone and does =
a
very similar thing so why reinvent the wheel?"
=20
Post by Giedrius Statkevicius
+ if (likely(data !=3D ACCEL_1) && likely(data !=3D ACCEL_2) &&
+ likely(data !=3D ACCEL_3) && likely(data !=3D 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 *de=
vice)
Post by Giedrius Statkevicius
if (ret)
return ret;
=20
+ /* filter to remove accelerometer data from keyboard bus stream */
+ ret =3D i8042_install_filter(hp_accel_i8042_filter);
+ if (ret)
+ i8042_remove_filter(hp_accel_i8042_filter);
=20
If it failed to install, you don't need to remove it afterward. See
the implementation for i8042_install_filter().
=20
Thanks for mentioning this. When we discuss the issues mentioned
previously I'll resend the fixed patch in the canonical and proper form=
=2E

Thanks,
Giedrius
Éric Piel
2014-10-22 14:19:50 UTC
Permalink
Post by Giedrius Statkevicius
- 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=
?
Post by Giedrius Statkevicius
HP doesn't seem to have published any.
=20
If other people have the same issue with the same scancodes on=20
accelerometers with different ACPI ids we can go ahead and do what th=
is
Post by Giedrius Statkevicius
patch does without reacting to what hardware the user has.
=20
And a general question about what other people think of doing this?
Maybe there is some better way I don't know of.
Hi,
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?

Cheers,
=C3=89ric
Giedrius Statkevicius
2014-10-22 16:45:33 UTC
Permalink
Post by Éric Piel
On the HP laptop I had (with HPQ0004), no fake keys were reported.
I guess this is a new "feature", then.
Post by Éric Piel
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 repo=
rt,
Post by Éric Piel
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
I'm sorry if I made the impression that it happens automatically but
actually I am running a daemon compiled from
Documentation/laptops/freefall.c. Nothing else is running on top of
linux to park the head when a free fall is detected.
Post by Éric Piel
"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
Well, these are not just a few messages but a lot of them and they clog
the system log, makes it hard to notice the actual useful information,
wastes disk space, etc.
Post by Éric Piel
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).
The free fall detection is already handled by lis3lv02d and hp_accel on
hp laptops with this feature and information is provided through
/dev/freefall so, in my opinion, the way to go is to completely drop
these scancodes.
Post by Éric Piel
=20
Is there maybe some general policy about what do to hardware that
generate such special scancodes?
Really not sure. BTW, I wonder if the same stuff happens on HPQ6007.

Thanks,
Giedrius

Loading...