Make power-button key report the button-up event when the 5-button array does not exist
diff mbox

Message ID 13513063.74pX3ZfeIS@sanji
State Under Review
Delegated to: Andy Shevchenko
Headers show

Commit Message

Tristian Celestin April 21, 2018, 10:25 p.m. UTC
I am running Fedora 28 and Android-x86 on a Dell Latitude 5175 tablet. The 
power button functionality is driven by the intel-hid driver. I am using 
kernel version 4.16.

Currently, the intel-hid driver does not supply a KEY_POWER up event in cases 
where the platform doesn't expose the 5-button array. Without this patch, the 
power button can't reliably respond when the platform is running Android.

When running Fedora, I can use the power button to suspend and resume the 
tablet. I can initiate this suspend by short-pressing the power button for a 
second, and can resume it using another short-press.

When running Android-x86, I can only short-press the power button once. After 
the press, the button seems to no longer respond. This is problematic when 
using a short-press to initiate a suspend, since a subsequent short press will 
not wake the tablet.

I used getevent to display the KeyEvents[1] detected by Android, and a 
combination of 'cat /proc/kmsg' and debug statements in the intel-hid driver 
to display the events generated by the driver. I found the block in the intel-
hid driver that generates power button events for my device. On line 253 of 
intel-hid.c:

        if (!priv->array) {
                if (event == 0xce) {
                        input_report_key(priv->input_dev, KEY_POWER, 1);
                        input_sync(priv->input_dev);
                        return;
                }

                if (event == 0xcf)
                        return;
}

When I short-press the power button, intel-hid produces a KEY_POWER down 
event, but doesn't produce a KEY_POWER up event when I release the power 
button. Suppose intel-hid has been mapped to the input device /dev/input/
event19. Then, on Android-x86, the command "getevent -lt" produces the 
following output:

/dev/input/event19: EV_KEY       KEY_POWER            DOWN                
/dev/input/event19: EV_SYN       SYN_REPORT           00000000

Subsequent presses produced no output for that input device.

When I added a call to input_report_key(...) and input_sync(...) on the 
KEY_POWER up event in the intel-hid driver, I could repeatedly short-press the 
power button and have Android respond appropriately, including resuming the 
device from suspend. My hunch as to why this is the case is that Android needs 
a paired KEY_POWER DOWN and UP event before it will handle the press.

Comments

Andy Shevchenko April 23, 2018, 2:36 p.m. UTC | #1
On Sun, Apr 22, 2018 at 1:25 AM, Tristian Celestin
<tristiancelestin@fastmail.com> wrote:

Thanks for the patch.

First of all, please, include all PDx86 maintainers to the discussion as well.
Second, please, use `git send-email` tool to send patches, it avoids
attachments.

> I am running Fedora 28 and Android-x86 on a Dell Latitude 5175 tablet. The
> power button functionality is driven by the intel-hid driver. I am using
> kernel version 4.16.
>
> Currently, the intel-hid driver does not supply a KEY_POWER up event in cases
> where the platform doesn't expose the 5-button array. Without this patch, the
> power button can't reliably respond when the platform is running Android.
>
> When running Fedora, I can use the power button to suspend and resume the
> tablet. I can initiate this suspend by short-pressing the power button for a
> second, and can resume it using another short-press.
>
> When running Android-x86, I can only short-press the power button once. After
> the press, the button seems to no longer respond. This is problematic when
> using a short-press to initiate a suspend, since a subsequent short press will
> not wake the tablet.
>
> I used getevent to display the KeyEvents[1] detected by Android, and a
> combination of 'cat /proc/kmsg' and debug statements in the intel-hid driver
> to display the events generated by the driver. I found the block in the intel-
> hid driver that generates power button events for my device. On line 253 of
> intel-hid.c:
>
>         if (!priv->array) {
>                 if (event == 0xce) {
>                         input_report_key(priv->input_dev, KEY_POWER, 1);
>                         input_sync(priv->input_dev);
>                         return;
>                 }
>
>                 if (event == 0xcf)
>                         return;
> }
>
> When I short-press the power button, intel-hid produces a KEY_POWER down
> event, but doesn't produce a KEY_POWER up event when I release the power
> button. Suppose intel-hid has been mapped to the input device /dev/input/
> event19. Then, on Android-x86, the command "getevent -lt" produces the
> following output:
>
> /dev/input/event19: EV_KEY       KEY_POWER            DOWN
> /dev/input/event19: EV_SYN       SYN_REPORT           00000000
>
> Subsequent presses produced no output for that input device.
>
> When I added a call to input_report_key(...) and input_sync(...) on the
> KEY_POWER up event in the intel-hid driver, I could repeatedly short-press the
> power button and have Android respond appropriately, including resuming the
> device from suspend. My hunch as to why this is the case is that Android needs
> a paired KEY_POWER DOWN and UP event before it will handle the press.

WRT, patch contents:
- please, do a proper commit message
- while it has crucial semantic mistake (missing {}) it suddenly works
because nothing behind the condition you had touched
- I would rather unify conditionals, though I would like to hear from
Alex and Dmitry if it's fine to do what you are trying to do in the
patch
Alex Hung April 24, 2018, 3:55 a.m. UTC | #2
On Mon, Apr 23, 2018 at 7:36 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Apr 22, 2018 at 1:25 AM, Tristian Celestin
> <tristiancelestin@fastmail.com> wrote:
>
> Thanks for the patch.
>
> First of all, please, include all PDx86 maintainers to the discussion as well.
> Second, please, use `git send-email` tool to send patches, it avoids
> attachments.
>
>> I am running Fedora 28 and Android-x86 on a Dell Latitude 5175 tablet. The
>> power button functionality is driven by the intel-hid driver. I am using
>> kernel version 4.16.
>>
>> Currently, the intel-hid driver does not supply a KEY_POWER up event in cases
>> where the platform doesn't expose the 5-button array. Without this patch, the
>> power button can't reliably respond when the platform is running Android.
>>
>> When running Fedora, I can use the power button to suspend and resume the
>> tablet. I can initiate this suspend by short-pressing the power button for a
>> second, and can resume it using another short-press.
>>
>> When running Android-x86, I can only short-press the power button once. After
>> the press, the button seems to no longer respond. This is problematic when
>> using a short-press to initiate a suspend, since a subsequent short press will
>> not wake the tablet.
>>
>> I used getevent to display the KeyEvents[1] detected by Android, and a
>> combination of 'cat /proc/kmsg' and debug statements in the intel-hid driver
>> to display the events generated by the driver. I found the block in the intel-
>> hid driver that generates power button events for my device. On line 253 of
>> intel-hid.c:
>>
>>         if (!priv->array) {
>>                 if (event == 0xce) {
>>                         input_report_key(priv->input_dev, KEY_POWER, 1);
>>                         input_sync(priv->input_dev);
>>                         return;
>>                 }
>>
>>                 if (event == 0xcf)
>>                         return;
>> }

Thanks for the work. This somehow sounds similar to Wacom MobileStudio
Pro that we worked on before. A quirk was added to enable 5 button
array, and the commit is c454a99d4ce1cebb.

Could you please try to add a DMI entry in button_array_table[] and
verify the power button again? If this works, we can use the DMI quirk
instead.

>>
>> When I short-press the power button, intel-hid produces a KEY_POWER down
>> event, but doesn't produce a KEY_POWER up event when I release the power
>> button. Suppose intel-hid has been mapped to the input device /dev/input/
>> event19. Then, on Android-x86, the command "getevent -lt" produces the
>> following output:
>>
>> /dev/input/event19: EV_KEY       KEY_POWER            DOWN
>> /dev/input/event19: EV_SYN       SYN_REPORT           00000000
>>
>> Subsequent presses produced no output for that input device.
>>
>> When I added a call to input_report_key(...) and input_sync(...) on the
>> KEY_POWER up event in the intel-hid driver, I could repeatedly short-press the
>> power button and have Android respond appropriately, including resuming the
>> device from suspend. My hunch as to why this is the case is that Android needs
>> a paired KEY_POWER DOWN and UP event before it will handle the press.
>
> WRT, patch contents:
> - please, do a proper commit message
> - while it has crucial semantic mistake (missing {}) it suddenly works
> because nothing behind the condition you had touched
> - I would rather unify conditionals, though I would like to hear from
> Alex and Dmitry if it's fine to do what you are trying to do in the
> patch
>
> --
> With Best Regards,
> Andy Shevchenko
Tristian Celestin April 30, 2018, 3:45 a.m. UTC | #3
On Mon, Apr 23, 2018, at 8:55 PM, Alex Hung wrote:
> On Mon, Apr 23, 2018 at 7:36 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Sun, Apr 22, 2018 at 1:25 AM, Tristian Celestin
>> <tristiancelestin@fastmail.com> wrote:
>> 
>> Thanks for the patch.
>> 
>> First of all, please, include all PDx86 maintainers to the discussion as well.
>> Second, please, use `git send-email` tool to send patches, it avoids
>> attachments.

Thank you for the guidance. Will do.

>>> I am running Fedora 28 and Android-x86 on a Dell Latitude 5175 tablet. The
>>> power button functionality is driven by the intel-hid driver. I am using
>>> kernel version 4.16.
>>> 
>>> Currently, the intel-hid driver does not supply a KEY_POWER up event in cases
>>> where the platform doesn't expose the 5-button array. Without this patch, the
>>> power button can't reliably respond when the platform is running Android.
>>> 
>>> When running Fedora, I can use the power button to suspend and resume the
>>> tablet. I can initiate this suspend by short-pressing the power button for a
>>> second, and can resume it using another short-press.
>>> 
>>> When running Android-x86, I can only short-press the power button once. After
>>> the press, the button seems to no longer respond. This is problematic when
>>> using a short-press to initiate a suspend, since a subsequent short press will
>>> not wake the tablet.
>>> 
>>> I used getevent to display the KeyEvents[1] detected by Android, and a
>>> combination of 'cat /proc/kmsg' and debug statements in the intel-hid driver
>>> to display the events generated by the driver. I found the block in the intel-
>>> hid driver that generates power button events for my device. On line 253 of
>>> intel-hid.c:
>>> 
>>>       if (!priv->array) {
>>>               if (event == 0xce) {
>>>                       input_report_key(priv->input_dev, KEY_POWER, 1);
>>>                       input_sync(priv->input_dev);
>>>                       return;
>>>               }
>>> 
>>>               if (event == 0xcf)
>>>                       return;
>>> }
> 
> Thanks for the work. This somehow sounds similar to Wacom MobileStudio
> Pro that we worked on before. A quirk was added to enable 5 button
> array, and the commit is c454a99d4ce1cebb.
> 
> Could you please try to add a DMI entry in button_array_table[] and
> verify the power button again? If this works, we can use the DMI quirk
> instead.

Thank you for the guidance. I added a DMI entry to button_array_table[] for the Latitude 5175, and the
tablet now also responds to short presses while suspended.

>>> 
>>> When I short-press the power button, intel-hid produces a KEY_POWER down
>>> event, but doesn't produce a KEY_POWER up event when I release the power
>>> button. Suppose intel-hid has been mapped to the input device /dev/input/
>>> event19. Then, on Android-x86, the command "getevent -lt" produces the
>>> following output:
>>> 
>>> /dev/input/event19: EV_KEY       KEY_POWER            DOWN
>>> /dev/input/event19: EV_SYN       SYN_REPORT           00000000
>>> 
>>> Subsequent presses produced no output for that input device.
>>> 
>>> When I added a call to input_report_key(...) and input_sync(...) on the
>>> KEY_POWER up event in the intel-hid driver, I could repeatedly short-press the
>>> power button and have Android respond appropriately, including resuming the
>>> device from suspend. My hunch as to why this is the case is that Android needs
>>> a paired KEY_POWER DOWN and UP event before it will handle the press.
>> 
>> WRT, patch contents:
>> - please, do a proper commit message
>> - while it has crucial semantic mistake (missing {}) it suddenly works
>> because nothing behind the condition you had touched
>> - I would rather unify conditionals, though I would like to hear from
>> Alex and Dmitry if it's fine to do what you are trying to do in the
>> patch
>> 
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> 
> 
> --
> Cheers,
> Alex Hung
Tristian Celestin April 30, 2018, 4:35 a.m. UTC | #4
I have a patch ready, but I don't know the underlying cause of the problem, and this is preventing from writing a meaningful commit message.


On Sun, Apr 29, 2018, at 8:45 PM, Tristian Celestin wrote:
> 
> 
> 
> On Mon, Apr 23, 2018, at 8:55 PM, Alex Hung wrote:
>> On Mon, Apr 23, 2018 at 7:36 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Sun, Apr 22, 2018 at 1:25 AM, Tristian Celestin
>>> <tristiancelestin@fastmail.com> wrote:
>>> 
>>> Thanks for the patch.
>>> 
>>> First of all, please, include all PDx86 maintainers to the discussion as well.
>>> Second, please, use `git send-email` tool to send patches, it avoids
>>> attachments.
> 
> Thank you for the guidance. Will do.
> 
>>>> I am running Fedora 28 and Android-x86 on a Dell Latitude 5175 tablet. The
>>>> power button functionality is driven by the intel-hid driver. I am using
>>>> kernel version 4.16.
>>>> 
>>>> Currently, the intel-hid driver does not supply a KEY_POWER up event in cases
>>>> where the platform doesn't expose the 5-button array. Without this patch, the
>>>> power button can't reliably respond when the platform is running Android.
>>>> 
>>>> When running Fedora, I can use the power button to suspend and resume the
>>>> tablet. I can initiate this suspend by short-pressing the power button for a
>>>> second, and can resume it using another short-press.
>>>> 
>>>> When running Android-x86, I can only short-press the power button once. After
>>>> the press, the button seems to no longer respond. This is problematic when
>>>> using a short-press to initiate a suspend, since a subsequent short press will
>>>> not wake the tablet.
>>>> 
>>>> I used getevent to display the KeyEvents[1] detected by Android, and a
>>>> combination of 'cat /proc/kmsg' and debug statements in the intel-hid driver
>>>> to display the events generated by the driver. I found the block in the intel-
>>>> hid driver that generates power button events for my device. On line 253 of
>>>> intel-hid.c:
>>>> 
>>>>       if (!priv->array) {
>>>>               if (event == 0xce) {
>>>>                       input_report_key(priv->input_dev, KEY_POWER, 1);
>>>>                       input_sync(priv->input_dev);
>>>>                       return;
>>>>               }
>>>> 
>>>>               if (event == 0xcf)
>>>>                       return;
>>>> }
>> 
>> Thanks for the work. This somehow sounds similar to Wacom MobileStudio
>> Pro that we worked on before. A quirk was added to enable 5 button
>> array, and the commit is c454a99d4ce1cebb.
>> 
>> Could you please try to add a DMI entry in button_array_table[] and
>> verify the power button again? If this works, we can use the DMI quirk
>> instead.
> 
> Thank you for the guidance. I added a DMI entry to button_array_table[] for the Latitude 5175, and the
> tablet now also responds to short presses while suspended.
> 
>>>> 
>>>> When I short-press the power button, intel-hid produces a KEY_POWER down
>>>> event, but doesn't produce a KEY_POWER up event when I release the power
>>>> button. Suppose intel-hid has been mapped to the input device /dev/input/
>>>> event19. Then, on Android-x86, the command "getevent -lt" produces the
>>>> following output:
>>>> 
>>>> /dev/input/event19: EV_KEY       KEY_POWER            DOWN
>>>> /dev/input/event19: EV_SYN       SYN_REPORT           00000000
>>>> 
>>>> Subsequent presses produced no output for that input device.
>>>> 
>>>> When I added a call to input_report_key(...) and input_sync(...) on the
>>>> KEY_POWER up event in the intel-hid driver, I could repeatedly short-press the
>>>> power button and have Android respond appropriately, including resuming the
>>>> device from suspend. My hunch as to why this is the case is that Android needs
>>>> a paired KEY_POWER DOWN and UP event before it will handle the press.
>>> 
>>> WRT, patch contents:
>>> - please, do a proper commit message
>>> - while it has crucial semantic mistake (missing {}) it suddenly works
>>> because nothing behind the condition you had touched
>>> - I would rather unify conditionals, though I would like to hear from
>>> Alex and Dmitry if it's fine to do what you are trying to do in the
>>> patch
>>> 
>>> --
>>> With Best Regards,
>>> Andy Shevchenko
>> 
>> 
>> 
>> --
>> Cheers,
>> Alex Hung
Alex Hung April 30, 2018, 5:20 a.m. UTC | #5
On Sun, Apr 29, 2018 at 9:35 PM, Tristian Celestin
<tristiancelestin@fastmail.com> wrote:
> I have a patch ready, but I don't know the underlying cause of the problem, and this is preventing from writing a meaningful commit message.

According to my understanding, the original intel-hid spec supported
0xC0 notification and events are reported by HDEM method, and an
update of "5 button array" added other notification numbers such as
0xce for power button; however, some BIOS failed to report 5 button
array is supported via HEBC method, and the DMI quirk was a workaround
to always enable 5 button array.

I personally think a commit message similar to c454a99d4ce1cebb is
good enough, but Andy or Darren will provide more feedbacks if they
think a refinement is necessary.

>
>
> On Sun, Apr 29, 2018, at 8:45 PM, Tristian Celestin wrote:
>>
>>
>>
>> On Mon, Apr 23, 2018, at 8:55 PM, Alex Hung wrote:
>>> On Mon, Apr 23, 2018 at 7:36 AM, Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Sun, Apr 22, 2018 at 1:25 AM, Tristian Celestin
>>>> <tristiancelestin@fastmail.com> wrote:
>>>>
>>>> Thanks for the patch.
>>>>
>>>> First of all, please, include all PDx86 maintainers to the discussion as well.
>>>> Second, please, use `git send-email` tool to send patches, it avoids
>>>> attachments.
>>
>> Thank you for the guidance. Will do.
>>
>>>>> I am running Fedora 28 and Android-x86 on a Dell Latitude 5175 tablet. The
>>>>> power button functionality is driven by the intel-hid driver. I am using
>>>>> kernel version 4.16.
>>>>>
>>>>> Currently, the intel-hid driver does not supply a KEY_POWER up event in cases
>>>>> where the platform doesn't expose the 5-button array. Without this patch, the
>>>>> power button can't reliably respond when the platform is running Android.
>>>>>
>>>>> When running Fedora, I can use the power button to suspend and resume the
>>>>> tablet. I can initiate this suspend by short-pressing the power button for a
>>>>> second, and can resume it using another short-press.
>>>>>
>>>>> When running Android-x86, I can only short-press the power button once. After
>>>>> the press, the button seems to no longer respond. This is problematic when
>>>>> using a short-press to initiate a suspend, since a subsequent short press will
>>>>> not wake the tablet.
>>>>>
>>>>> I used getevent to display the KeyEvents[1] detected by Android, and a
>>>>> combination of 'cat /proc/kmsg' and debug statements in the intel-hid driver
>>>>> to display the events generated by the driver. I found the block in the intel-
>>>>> hid driver that generates power button events for my device. On line 253 of
>>>>> intel-hid.c:
>>>>>
>>>>>       if (!priv->array) {
>>>>>               if (event == 0xce) {
>>>>>                       input_report_key(priv->input_dev, KEY_POWER, 1);
>>>>>                       input_sync(priv->input_dev);
>>>>>                       return;
>>>>>               }
>>>>>
>>>>>               if (event == 0xcf)
>>>>>                       return;
>>>>> }
>>>
>>> Thanks for the work. This somehow sounds similar to Wacom MobileStudio
>>> Pro that we worked on before. A quirk was added to enable 5 button
>>> array, and the commit is c454a99d4ce1cebb.
>>>
>>> Could you please try to add a DMI entry in button_array_table[] and
>>> verify the power button again? If this works, we can use the DMI quirk
>>> instead.
>>
>> Thank you for the guidance. I added a DMI entry to button_array_table[] for the Latitude 5175, and the
>> tablet now also responds to short presses while suspended.
>>
>>>>>
>>>>> When I short-press the power button, intel-hid produces a KEY_POWER down
>>>>> event, but doesn't produce a KEY_POWER up event when I release the power
>>>>> button. Suppose intel-hid has been mapped to the input device /dev/input/
>>>>> event19. Then, on Android-x86, the command "getevent -lt" produces the
>>>>> following output:
>>>>>
>>>>> /dev/input/event19: EV_KEY       KEY_POWER            DOWN
>>>>> /dev/input/event19: EV_SYN       SYN_REPORT           00000000
>>>>>
>>>>> Subsequent presses produced no output for that input device.
>>>>>
>>>>> When I added a call to input_report_key(...) and input_sync(...) on the
>>>>> KEY_POWER up event in the intel-hid driver, I could repeatedly short-press the
>>>>> power button and have Android respond appropriately, including resuming the
>>>>> device from suspend. My hunch as to why this is the case is that Android needs
>>>>> a paired KEY_POWER DOWN and UP event before it will handle the press.
>>>>
>>>> WRT, patch contents:
>>>> - please, do a proper commit message
>>>> - while it has crucial semantic mistake (missing {}) it suddenly works
>>>> because nothing behind the condition you had touched
>>>> - I would rather unify conditionals, though I would like to hear from
>>>> Alex and Dmitry if it's fine to do what you are trying to do in the
>>>> patch
>>>>
>>>> --
>>>> With Best Regards,
>>>> Andy Shevchenko
>>>
>>>
>>>
>>> --
>>> Cheers,
>>> Alex Hung

Patch
diff mbox

From 13f7f1cd9dfb71275f41dc598b17a92503fb991c Mon Sep 17 00:00:00 2001
From: Tristian Celestin <tristian.celestin@outlook.com>
Date: Tue, 10 Apr 2018 00:52:53 -0400
Subject: [PATCH] Make power-button key report the button-up event when the
 5-button array does not exist.

---
 drivers/platform/x86/intel-hid.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index 5e3df19..170ad950 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -260,6 +260,8 @@  static void notify_handler(acpi_handle handle, u32 event, void *context)
 		}
 
 		if (event == 0xcf)
+			input_report_key(priv->input_dev, KEY_POWER, 0);
+			input_sync(priv->input_dev);
 			return;
 	}
 
-- 
1.8.3.1