linux-kernel - Re: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Jun 2020 15:24:26 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Alex Hung <alex.hung@...onical.com>
Cc:     Mario.Limonciello@...l.com, dvhart@...radead.org,
        andy@...radead.org, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
 platforms



> On Jun 11, 2020, at 01:41, Alex Hung <alex.hung@...onical.com> wrote:
> 
> On 2020-06-10 9:49 a.m., Mario.Limonciello@...l.com wrote:
>>> -----Original Message-----
>>> From: platform-driver-x86-owner@...r.kernel.org <platform-driver-x86-
>>> owner@...r.kernel.org> On Behalf Of Kai-Heng Feng
>>> Sent: Wednesday, June 10, 2020 10:38 AM
>>> To: alex.hung@...onical.com
>>> Cc: Kai-Heng Feng; Darren Hart; Andy Shevchenko; open list:INTEL HID EVENT
>>> DRIVER; open list
>>> Subject: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
>>> platforms
>>> 
>>> 
>>> [EXTERNAL EMAIL]
>>> 
>>> Wireless hotkey on HP platforms can trigger two events, if both
>>> hp-wireless and intel-hid are supported. Two events at the same time
>>> renders wireless hotkey useless.
>>> 
>>> HP confirmed that hp-wireless (HPQ6001) should always be the canonical
>>> source of wireless hotkey event, so skip registering rfkill hotkey if
>>> HPQ6001 is present.
>>> 
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
>>> ---
>>> drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
>>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-
>>> hid.c
>>> index 9ee79b74311c..31091c8faf70 100644
>>> --- a/drivers/platform/x86/intel-hid.c
>>> +++ b/drivers/platform/x86/intel-hid.c
>>> @@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
>>> };
>>> MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
>>> 
>>> +static bool hp_wireless_present;
>>> +
>>> /* In theory, these are HID usages. */
>>> static const struct key_entry intel_hid_keymap[] = {
>>> 	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>>> @@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
>>> 	{ KE_END },
>>> };
>>> 
>>> +static const struct key_entry intel_hid_no_rfkill_keymap[] = {
>>> +	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>>> +	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
>>> +	{ KE_KEY, 3, { KEY_NUMLOCK } },
>>> +	{ KE_KEY, 4, { KEY_HOME } },
>>> +	{ KE_KEY, 5, { KEY_END } },
>>> +	{ KE_KEY, 6, { KEY_PAGEUP } },
>>> +	{ KE_KEY, 7, { KEY_PAGEDOWN } },
>>> +	/* 8: rfkill -- use hp-wireless instead */
>>> +	{ KE_KEY, 9, { KEY_POWER } },
>>> +	{ KE_KEY, 11, { KEY_SLEEP } },
>>> +	/* 13 has two different meanings in the spec -- ignore it. */
>>> +	{ KE_KEY, 14, { KEY_STOPCD } },
>>> +	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
>>> +	{ KE_KEY, 16, { KEY_MUTE } },
>>> +	{ KE_KEY, 17, { KEY_VOLUMEUP } },
>>> +	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
>>> +	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
>>> +	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
>>> +	/* 27: wake -- needs special handling */
>>> +	{ KE_END },
>>> +};
>>> +
>>> /* 5 button array notification value. */
>>> static const struct key_entry intel_array_keymap[] = {
>>> 	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
>>> @@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device
>>> *device)
>>> 	if (!priv->input_dev)
>>> 		return -ENOMEM;
>>> 
>>> -	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
>>> +	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
>>> +			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
>>> 	if (ret)
>>> 		return ret;
>>> 
>>> @@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
>>> *context, void **rv)
>>> 			dev_info(&dev->dev,
>>> 				 "intel-hid: created platform device\n");
>>> 
>>> +	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
>>> +		hp_wireless_present = true;
> 
> (Resend with format removed)
> 
> This can impact all HP systems that do not have this problem.

HP is certain that HPQ6001 should always be used over INT33D5.

If this patch breaks other platform, then we should fix HPQ6001 instead.

> How about
> a DMI quirk that is limited to this particular system?

We should avoid using DMI quirk for this one, as this is to follow the HP's spec.

Kai-Heng

> 
> 
>> 
>> Just having the ACPI device present doesn't actually mean that the user
>> has a kernel compiled with hp-wireless or that it has finished initializing.
>> 
>> I would think this needs a better handshake in case hp-wireless was unloaded
>> or not present so the event could still come through intel-hid in this
>> circumstance.
>> 
>>> +
>>> 	return AE_OK;
>>> }
>>> 
>>> --
>>> 2.17.1
>> 
> 
> 
> -- 
> Cheers,
> Alex Hung