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(-)
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>
#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,
+ struct serio *port)
+{
+ static bool extended;
+
+ if (str & I8042_STR_AUXDATA)
+ return false;
+
+ if (data == 0xe0) {
+ extended = true;
+ return true;
+ }
+
+ if (!extended)
+ return false;
+
+ extended = false;
+ 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;
+ /* 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);
+
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);
--
2.1.2
On Sat, Oct 18, 2014 at 11:59:22PM +0300, Giedrius Statkevicius wrote:
> Hello,
Hi Giedrius,
> 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?
> 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]>
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 | 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>
>
> #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,
> + 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?
> + }
> +
> + if (!extended)
> + return false;
I'm now confused :-)
> +
> + extended = false;
Wait... what?
Please review the use of extended here as well as the possibility
of using it uninitialized.
> + 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;
>
> + /* 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().
> +
> 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);
>
> --
> 2.1.2
>
>
--
Darren Hart
Intel Open Source Technology Center
On 2014.10.22 00:45, Darren Hart wrote:
> On Sat, Oct 18, 2014 at 11:59:22PM +0300, Giedrius Statkevicius wrote:
>> Hello,
>
> Hi Giedrius,
>
>> 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?
>
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.
>> 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]>
>
> 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.
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.
>
> More below...
>
[...]
>> +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?
>
>> + }
>> +
>> + if (!extended)
>> + return false;
>
> I'm now confused :-)
>
>> +
>> + extended = false;
>
> Wait... what?
>
> 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 be
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?"
>
>> + 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;
>>
>> + /* 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().
>
Thanks for mentioning this. When we discuss the issues mentioned
previously I'll resend the fixed patch in the canonical and proper form.
Thanks,
Giedrius
On 22/10/14 15:20, Giedrius Statkevicius wrote:
:
> 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.
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,
Éric
On 2014.10.22 17:19, Éric Piel wrote:
> On the HP laptop I had (with HPQ0004), no fake keys were reported.
I guess this is a new "feature", then.
> 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
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.
> "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.
> 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.
>
> 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