Re: [PATCH RFC] platform: hp_accel: add a i8042 filter to remove accelerometer data

2014-10-22 Thread Giedrius Statkevicius 


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 giedriusw...@gmail.com
 
 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
--
To unsubscribe from this list: send the line unsubscribe platform-driver-x86 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] platform: hp_accel: add a i8042 filter to remove accelerometer data

2014-10-22 Thread Éric Piel 
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
--
To unsubscribe from this list: send the line unsubscribe platform-driver-x86 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] platform: hp_accel: add a i8042 filter to remove accelerometer data

2014-10-22 Thread Giedrius Statkevicius 
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
--
To unsubscribe from this list: send the line unsubscribe platform-driver-x86 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] platform: hp_accel: add a i8042 filter to remove accelerometer data

2014-10-21 Thread Darren Hart 
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 giedriusw...@gmail.com

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
--
To unsubscribe from this list: send the line unsubscribe platform-driver-x86 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html