On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
is generated. As there is no flawless way to determine whether a given
machine is capable of simulating a keypress when this hotkey is pressed,
a new module parameter is added so that the user can decide whether the
WMI event should be processed or ignored.
Signed-off-by: Michał Kępień <[email protected]>
---
As my last message [1] in the rather lengthy thread failed to elicit any
response, I guess I might just as well post the proposed patch so that
we have something specific to discuss.
[1] http://www.spinics.net/lists/platform-driver-x86/msg07679.html
drivers/platform/x86/dell-wmi.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 8cb0f57..e68ce3b 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -44,6 +44,10 @@ MODULE_LICENSE("GPL");
#define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
static int acpi_video;
+static bool process_dil;
+
+module_param(process_dil, bool, 0644);
+MODULE_PARM_DESC(process_dil, "Generate an input event when the WMI event for Dell Instant Launch hotkey is received");
MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
@@ -87,7 +91,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
{ KE_IGNORE, 0xe020, { KEY_MUTE } },
/* Shortcut and audio panel keys */
- { KE_IGNORE, 0xe025, { KEY_RESERVED } },
+ { KE_KEY, 0xe025, { KEY_PROG4 } },
{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
@@ -183,6 +187,9 @@ static void dell_wmi_process_key(int reported_key)
key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
return;
+ if (key->keycode == KEY_PROG4 && !process_dil)
+ return;
+
sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
}
--
1.7.10.4
On Thursday 26 November 2015 15:18:32 Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated. As there is no flawless way to determine whether a given
> machine is capable of simulating a keypress when this hotkey is pressed,
> a new module parameter is added so that the user can decide whether the
> WMI event should be processed or ignored.
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> As my last message [1] in the rather lengthy thread failed to elicit any
> response, I guess I might just as well post the proposed patch so that
> we have something specific to discuss.
>
> [1] http://www.spinics.net/lists/platform-driver-x86/msg07679.html
>
Can you wait just a little bit? Till end of month two Dell kernel devs
are on vacation, so after that they maybe answer to question about new
hotkey format/support in kernel.
> drivers/platform/x86/dell-wmi.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 8cb0f57..e68ce3b 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -44,6 +44,10 @@ MODULE_LICENSE("GPL");
> #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
>
> static int acpi_video;
> +static bool process_dil;
> +
> +module_param(process_dil, bool, 0644);
> +MODULE_PARM_DESC(process_dil, "Generate an input event when the WMI event for Dell Instant Launch hotkey is received");
I do not like name "dil". It takes me few minutes to interpret it as
Dell Instant Launch...
Also I do not know if this is the best approach.
> /* Shortcut and audio panel keys */
> - { KE_IGNORE, 0xe025, { KEY_RESERVED } },
> + { KE_KEY, 0xe025, { KEY_PROG4 } },
> { KE_IGNORE, 0xe026, { KEY_RESERVED } },
I'm trying to figure out if those two keys are really reported via
keyboard controller or not. They were added 4 years ago in commit
f1566f0dc07ec9b5409b348070f5a700032d7881. But from bug report
http://bugs.launchpad.net/bugs/815914 there is no information if those
two keys are really reported by keyboard controller or not.
And if not our problem could be easier...
--
Pali Rohár
[email protected]
Hi Pali,
> Can you wait just a little bit? Till end of month two Dell kernel devs
> are on vacation, so after that they maybe answer to question about new
> hotkey format/support in kernel.
Absolutely, after all this issue is already several months old (or even
years, depending on how you interpret it). I just didn't want it to
fade into the void.
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 8cb0f57..e68ce3b 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -44,6 +44,10 @@ MODULE_LICENSE("GPL");
> > #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
> >
> > static int acpi_video;
> > +static bool process_dil;
> > +
> > +module_param(process_dil, bool, 0644);
> > +MODULE_PARM_DESC(process_dil, "Generate an input event when the WMI event for Dell Instant Launch hotkey is received");
>
> I do not like name "dil". It takes me few minutes to interpret it as
> Dell Instant Launch...
Yes, it's ugly. I really wanted to avoid process_dell_instant_launch.
Perhaps I shouldn't have?
> Also I do not know if this is the best approach.
Same here, that's why I submitted this patch for discussion.
> > /* Shortcut and audio panel keys */
> > - { KE_IGNORE, 0xe025, { KEY_RESERVED } },
> > + { KE_KEY, 0xe025, { KEY_PROG4 } },
> > { KE_IGNORE, 0xe026, { KEY_RESERVED } },
>
> I'm trying to figure out if those two keys are really reported via
> keyboard controller or not. They were added 4 years ago in commit
> f1566f0dc07ec9b5409b348070f5a700032d7881. But from bug report
> http://bugs.launchpad.net/bugs/815914 there is no information if those
> two keys are really reported by keyboard controller or not.
>
> And if not our problem could be easier...
That would indeed be sweet as this patch could then be shrinked to just
changing the entry in the sparse keymap. Does anyone have a Dell XPS
L502X handy? Also, any ideas for making sure no other model is
generating that keypress?
--
Best regards,
Michał Kępień
On Thursday 26 November 2015 15:55:56 Michał Kępień wrote:
> > > /* Shortcut and audio panel keys */
> > >
> > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } },
> > > + { KE_KEY, 0xe025, { KEY_PROG4 } },
> > >
> > > { KE_IGNORE, 0xe026, { KEY_RESERVED } },
> >
> > I'm trying to figure out if those two keys are really reported via
> > keyboard controller or not. They were added 4 years ago in commit
> > f1566f0dc07ec9b5409b348070f5a700032d7881. But from bug report
> > http://bugs.launchpad.net/bugs/815914 there is no information if
> > those two keys are really reported by keyboard controller or not.
> >
> > And if not our problem could be easier...
>
> That would indeed be sweet as this patch could then be shrinked to
> just changing the entry in the sparse keymap. Does anyone have a
> Dell XPS L502X handy? Also, any ideas for making sure no other
> model is generating that keypress?
And now I have info how keys are reported on Dell XPS L502X. Sadly it is
worse as I expected :-( Here is output from Jean-Louis Dupond notebook:
$ sudo /usr/bin/input-events 4
/dev/input/event4
bustype : BUS_I8042
vendor : 0x1
product : 0x1
version : 43841
name : "AT Translated Set 2 keyboard"
phys : "isa0060/serio0/input0"
bits ev : EV_SYN EV_KEY EV_MSC EV_LED EV_REP
waiting for events
10:26:29.945739: EV_MSC MSC_SCAN 219
10:26:29.945739: EV_KEY KEY_LEFTMETA (0x7d) pressed
10:26:29.945739: EV_SYN code=0 value=0
10:26:29.946468: EV_MSC MSC_SCAN 45
10:26:29.946468: EV_KEY KEY_X (0x2d) pressed
10:26:29.946468: EV_SYN code=0 value=0
10:26:29.948469: EV_MSC MSC_SCAN 45
10:26:29.948469: EV_KEY KEY_X (0x2d) released
10:26:29.948469: EV_SYN code=0 value=0
10:26:29.951473: EV_MSC MSC_SCAN 219
10:26:29.951473: EV_KEY KEY_LEFTMETA (0x7d) released
10:26:29.951473: EV_SYN code=0 value=0
x
(Press+release first key with name "Windows Mobility Center control")
(key X was printed to console)
10:26:32.898689: EV_MSC MSC_SCAN 133
10:26:32.898689: EV_KEY KEY_BRIGHTNESSDOWN (0xe0) pressed
10:26:32.898689: EV_SYN code=0 value=0
10:26:32.898730: EV_MSC MSC_SCAN 133
10:26:32.898730: EV_KEY KEY_BRIGHTNESSDOWN (0xe0) released
10:26:32.898730: EV_SYN code=0 value=0
(Press+release second key with name "Instant launch control")
10:26:35.090018: EV_MSC MSC_SCAN 132
10:26:35.090018: EV_KEY KEY_NEXTSONG (0xa3) pressed
10:26:35.090018: EV_SYN code=0 value=0
10:26:35.092765: EV_MSC MSC_SCAN 132
10:26:35.092765: EV_KEY KEY_NEXTSONG (0xa3) released
10:26:35.092765: EV_SYN code=0 value=0
(Press+release third key with name "Audio control-panel control")
As you can see events are send also via keyboard controller!
Key codes are configured by userspace (udev/systemd) and looks like
there is bug in userspace rules (reason for brightnes or nextsong), see:
https://wiki.ubuntu.com/HardwareSupport/Machines/Laptops/Dell/XPS/15
So it is not easy to make both machines (Dell XPS L502X and Dell Vostro
V131) works correctly :-( At least I do not see how.
And that mapping "Windows Mobility Center control" key to combination of
two keys (KEY_LEFTMETA + X) is some total stupid nonsense...
If anybody has idea how to fix this big firmware/BIOS mess please let us
know...
--
Pali Rohár
[email protected]
Hi Pali,
Thanks again for your efforts.
> $ sudo /usr/bin/input-events 4
> /dev/input/event4
> bustype : BUS_I8042
> vendor : 0x1
> product : 0x1
> version : 43841
> name : "AT Translated Set 2 keyboard"
> phys : "isa0060/serio0/input0"
> bits ev : EV_SYN EV_KEY EV_MSC EV_LED EV_REP
>
> waiting for events
>
> 10:26:29.945739: EV_MSC MSC_SCAN 219
> 10:26:29.945739: EV_KEY KEY_LEFTMETA (0x7d) pressed
> 10:26:29.945739: EV_SYN code=0 value=0
> 10:26:29.946468: EV_MSC MSC_SCAN 45
> 10:26:29.946468: EV_KEY KEY_X (0x2d) pressed
> 10:26:29.946468: EV_SYN code=0 value=0
> 10:26:29.948469: EV_MSC MSC_SCAN 45
> 10:26:29.948469: EV_KEY KEY_X (0x2d) released
> 10:26:29.948469: EV_SYN code=0 value=0
> 10:26:29.951473: EV_MSC MSC_SCAN 219
> 10:26:29.951473: EV_KEY KEY_LEFTMETA (0x7d) released
> 10:26:29.951473: EV_SYN code=0 value=0
> x
>
> (Press+release first key with name "Windows Mobility Center control")
> (key X was printed to console)
>
> 10:26:32.898689: EV_MSC MSC_SCAN 133
> 10:26:32.898689: EV_KEY KEY_BRIGHTNESSDOWN (0xe0) pressed
> 10:26:32.898689: EV_SYN code=0 value=0
> 10:26:32.898730: EV_MSC MSC_SCAN 133
> 10:26:32.898730: EV_KEY KEY_BRIGHTNESSDOWN (0xe0) released
> 10:26:32.898730: EV_SYN code=0 value=0
>
> (Press+release second key with name "Instant launch control")
>
> 10:26:35.090018: EV_MSC MSC_SCAN 132
> 10:26:35.090018: EV_KEY KEY_NEXTSONG (0xa3) pressed
> 10:26:35.090018: EV_SYN code=0 value=0
> 10:26:35.092765: EV_MSC MSC_SCAN 132
> 10:26:35.092765: EV_KEY KEY_NEXTSONG (0xa3) released
> 10:26:35.092765: EV_SYN code=0 value=0
>
> (Press+release third key with name "Audio control-panel control")
>
> As you can see events are send also via keyboard controller!
>
> Key codes are configured by userspace (udev/systemd) and looks like
> there is bug in userspace rules (reason for brightnes or nextsong), see:
> https://wiki.ubuntu.com/HardwareSupport/Machines/Laptops/Dell/XPS/15
Well, the V131 also sends its second hotkey (Dell Support Center) as a
scancode, which can be mapped in userspace. But I fail to understand
why this would be problematic, see below.
> So it is not easy to make both machines (Dell XPS L502X and Dell Vostro
> V131) works correctly :-( At least I do not see how.
As far as I understand, it's not the keyboard events that are a problem
for us, it's the WMI events, because some models generate them along
with the scancode (L502x) while on others the WMI event is the only
notification the OS can get that the hotkey was pressed (V131). So the
only sparse keymap entry which has to vary between models seems to be
the 0xe025 entry (obviously that's just the current state of our
knowledge, not vendor-provided information). That is why I submitted a
patch attempting to resolve this conflict.
If you disagree with the above, could you please elaborate?
> And that mapping "Windows Mobility Center control" key to combination of
> two keys (KEY_LEFTMETA + X) is some total stupid nonsense...
Well, it is unfortunate, but at least I can map it to what I please in
my window manager. What I am really struggling to understand is why on
earth would one employ something as complicated as ACPI/WMI for handling
keypresses. Not to mention requiring cryptic SMI calls for enabling the
notifications in the first place.
--
Best regards,
Michał Kępień
On Monday 30 November 2015 15:14:00 Michał Kępień wrote:
> Hi Pali,
>
> Thanks again for your efforts.
>
> > $ sudo /usr/bin/input-events 4
> > /dev/input/event4
> > bustype : BUS_I8042
> > vendor : 0x1
> > product : 0x1
> > version : 43841
> > name : "AT Translated Set 2 keyboard"
> > phys : "isa0060/serio0/input0"
> > bits ev : EV_SYN EV_KEY EV_MSC EV_LED EV_REP
> >
> > waiting for events
> >
> > 10:26:29.945739: EV_MSC MSC_SCAN 219
> > 10:26:29.945739: EV_KEY KEY_LEFTMETA (0x7d) pressed
> > 10:26:29.945739: EV_SYN code=0 value=0
> > 10:26:29.946468: EV_MSC MSC_SCAN 45
> > 10:26:29.946468: EV_KEY KEY_X (0x2d) pressed
> > 10:26:29.946468: EV_SYN code=0 value=0
> > 10:26:29.948469: EV_MSC MSC_SCAN 45
> > 10:26:29.948469: EV_KEY KEY_X (0x2d) released
> > 10:26:29.948469: EV_SYN code=0 value=0
> > 10:26:29.951473: EV_MSC MSC_SCAN 219
> > 10:26:29.951473: EV_KEY KEY_LEFTMETA (0x7d) released
> > 10:26:29.951473: EV_SYN code=0 value=0
> > x
> >
> > (Press+release first key with name "Windows Mobility Center control")
> > (key X was printed to console)
> >
> > 10:26:32.898689: EV_MSC MSC_SCAN 133
> > 10:26:32.898689: EV_KEY KEY_BRIGHTNESSDOWN (0xe0) pressed
> > 10:26:32.898689: EV_SYN code=0 value=0
> > 10:26:32.898730: EV_MSC MSC_SCAN 133
> > 10:26:32.898730: EV_KEY KEY_BRIGHTNESSDOWN (0xe0) released
> > 10:26:32.898730: EV_SYN code=0 value=0
> >
> > (Press+release second key with name "Instant launch control")
> >
> > 10:26:35.090018: EV_MSC MSC_SCAN 132
> > 10:26:35.090018: EV_KEY KEY_NEXTSONG (0xa3) pressed
> > 10:26:35.090018: EV_SYN code=0 value=0
> > 10:26:35.092765: EV_MSC MSC_SCAN 132
> > 10:26:35.092765: EV_KEY KEY_NEXTSONG (0xa3) released
> > 10:26:35.092765: EV_SYN code=0 value=0
> >
> > (Press+release third key with name "Audio control-panel control")
> >
> > As you can see events are send also via keyboard controller!
> >
> > Key codes are configured by userspace (udev/systemd) and looks like
> > there is bug in userspace rules (reason for brightnes or nextsong), see:
> > https://wiki.ubuntu.com/HardwareSupport/Machines/Laptops/Dell/XPS/15
>
> Well, the V131 also sends its second hotkey (Dell Support Center) as a
> scancode, which can be mapped in userspace. But I fail to understand
> why this would be problematic, see below.
>
Same reason what you wrote below... On some machines WMI event must be
ignored and on other must be sent to userspace.
The best would be if embedded controller could be configured to send
events via i8042 bus...
I'm starting to thing that blacklist or whitelist of machines is needed
to collect. And via it decide if WMI event will be accepted or dropped.
> > So it is not easy to make both machines (Dell XPS L502X and Dell Vostro
> > V131) works correctly :-( At least I do not see how.
>
> As far as I understand, it's not the keyboard events that are a problem
> for us, it's the WMI events, because some models generate them along
> with the scancode (L502x) while on others the WMI event is the only
> notification the OS can get that the hotkey was pressed (V131). So the
> only sparse keymap entry which has to vary between models seems to be
> the 0xe025 entry (obviously that's just the current state of our
> knowledge, not vendor-provided information). That is why I submitted a
> patch attempting to resolve this conflict.
>
> If you disagree with the above, could you please elaborate?
>
> > And that mapping "Windows Mobility Center control" key to combination of
> > two keys (KEY_LEFTMETA + X) is some total stupid nonsense...
>
> Well, it is unfortunate, but at least I can map it to what I please in
> my window manager. What I am really struggling to understand is why on
> earth would one employ something as complicated as ACPI/WMI for handling
> keypresses. Not to mention requiring cryptic SMI calls for enabling the
> notifications in the first place.
>
I remember that on older Acer laptops was needed to send some PS/2
command to enable additional Fn keys to generate scan codes... Dell just
upgraded this "feature" that OS needs to send SMI call!
--
Pali Rohár
[email protected]
> The best would be if embedded controller could be configured to send
> events via i8042 bus...
Yes, but if I correctly understand Mario's message back from July [1],
V131's EC simply does not support generating scancodes at all.
> I'm starting to thing that blacklist or whitelist of machines is needed
> to collect. And via it decide if WMI event will be accepted or dropped.
If you believe this is the way to go, I'm perfectly fine with that.
[1] http://www.spinics.net/lists/platform-driver-x86/msg07220.html
--
Best regards,
Michał Kępień
On Mon, Nov 30, 2015 at 03:54:58PM +0100, Michał Kępień wrote:
> > The best would be if embedded controller could be configured to send
> > events via i8042 bus...
>
> Yes, but if I correctly understand Mario's message back from July [1],
> V131's EC simply does not support generating scancodes at all.
>
> > I'm starting to thing that blacklist or whitelist of machines is needed
> > to collect. And via it decide if WMI event will be accepted or dropped.
>
> If you believe this is the way to go, I'm perfectly fine with that.
This is a common approach among platform drivers since we try to support
multiple products with a single driver.
Ideally, we could detect if this was necessary by the response of some ACPI call
or another, but failing that, DMI matching is our fallback.
--
Darren Hart
Intel Open Source Technology Center
On Thu, Nov 26, 2015 at 03:18:32PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated. As there is no flawless way to determine whether a given
> machine is capable of simulating a keypress when this hotkey is pressed,
> a new module parameter is added so that the user can decide whether the
> WMI event should be processed or ignored.
>
> Signed-off-by: Michał Kępień <[email protected]>
Module parameters are to be avoided wherever possible, especially for things
like this which set precedent and don't scale well. If we cannot detect which
machines use the EC and which only use WMI, then we can fall back to checking
DMI for specific models.
Per the above, and Pali's feedback. I'll be dropping this one in anticipation of
a V2.
Thanks,
--
Darren Hart
Intel Open Source Technology Center
> Module parameters are to be avoided wherever possible, especially for things
> like this which set precedent and don't scale well. If we cannot detect which
> machines use the EC and which only use WMI, then we can fall back to checking
> DMI for specific models.
>
> Per the above, and Pali's feedback. I'll be dropping this one in anticipation of
> a V2.
Thanks for reviewing, I'll prepare version 2 using DMI matching.
--
Best regards,
Michał Kępień
On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
is generated. As there seems to be no ACPI method or SMI call to
determine without possible side effects whether a given machine is
capable of simulating a keypress when this hotkey is pressed, DMI
matching is used to whitelist the models for which an input event should
be generated when WMI event 0xe025 is received.
Signed-off-by: Michał Kępień <[email protected]>
---
Changes from v1:
- Use DMI matching instead of a module parameter
- Change flag name to improve readability
Darren, I am aware this patch conflicts with Andy's WMI rework series
posted yesterday, so please just let me know if you want me to rebase.
drivers/platform/x86/dell-wmi.c | 39 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 8cb0f57..27c8f49 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -47,6 +47,38 @@ static int acpi_video;
MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
+struct quirk_entry {
+ bool process_dell_instant_launch;
+};
+
+static struct quirk_entry *quirks;
+
+static struct quirk_entry quirk_unknown = {
+};
+
+static struct quirk_entry quirk_dell_vostro_v131 = {
+ .process_dell_instant_launch = true,
+};
+
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+ quirks = dmi->driver_data;
+ return 1;
+}
+
+static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
+ {
+ .callback = dmi_matched,
+ .ident = "Dell Vostro V131",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+ },
+ .driver_data = &quirk_dell_vostro_v131,
+ },
+ { }
+};
+
/*
* Certain keys are flagged as KE_IGNORE. All of these are either
* notifications (rather than requests for change) or are also sent
@@ -87,7 +119,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
{ KE_IGNORE, 0xe020, { KEY_MUTE } },
/* Shortcut and audio panel keys */
- { KE_IGNORE, 0xe025, { KEY_RESERVED } },
+ { KE_KEY, 0xe025, { KEY_PROG4 } },
{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
@@ -183,6 +215,9 @@ static void dell_wmi_process_key(int reported_key)
key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
return;
+ if (key->keycode == KEY_PROG4 && !quirks->process_dell_instant_launch)
+ return;
+
sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
}
@@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
return -ENODEV;
}
+ quirks = &quirk_unknown;
+ dmi_check_system(dell_wmi_quirks);
dmi_walk(find_hk_type, NULL);
acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
--
1.7.10.4
On Tue, Dec 01, 2015 at 08:51:34PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated. As there seems to be no ACPI method or SMI call to
> determine without possible side effects whether a given machine is
> capable of simulating a keypress when this hotkey is pressed, DMI
> matching is used to whitelist the models for which an input event should
> be generated when WMI event 0xe025 is received.
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> Changes from v1:
>
> - Use DMI matching instead of a module parameter
> - Change flag name to improve readability
>
> Darren, I am aware this patch conflicts with Andy's WMI rework series
> posted yesterday, so please just let me know if you want me to rebase.
Thanks for the heads' up. Andy's series is longer and likely going to need more
review from more people, so I don't necessarily want to hold this one up on it -
unless it makes sense to include it in that series so they can evolve together.
I'll leave that to you and Andy to decide.
This looks fine to me, and if Pali will ack it, I'll move it from for-review to
testing and Andy will need to update patch 14/14 to accomodate - unless you guys
decide to include this in his.
For now, this is queued to for-review.
Thanks!
--
Darren Hart
Intel Open Source Technology Center
On Tuesday 01 December 2015 20:51:34 Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated. As there seems to be no ACPI method or SMI call to
> determine without possible side effects whether a given machine is
> capable of simulating a keypress when this hotkey is pressed, DMI
> matching is used to whitelist the models for which an input event should
> be generated when WMI event 0xe025 is received.
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> Changes from v1:
>
> - Use DMI matching instead of a module parameter
> - Change flag name to improve readability
>
> Darren, I am aware this patch conflicts with Andy's WMI rework series
> posted yesterday, so please just let me know if you want me to rebase.
>
> drivers/platform/x86/dell-wmi.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 8cb0f57..27c8f49 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -47,6 +47,38 @@ static int acpi_video;
>
> MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>
> +struct quirk_entry {
> + bool process_dell_instant_launch;
> +};
> +
> +static struct quirk_entry *quirks;
> +
> +static struct quirk_entry quirk_unknown = {
> +};
> +
> +static struct quirk_entry quirk_dell_vostro_v131 = {
> + .process_dell_instant_launch = true,
> +};
> +
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> + quirks = dmi->driver_data;
> + return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Vostro V131",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> + },
> + .driver_data = &quirk_dell_vostro_v131,
> + },
> + { }
> +};
> +
It is not possible to simplify this part of code? Currently we only need
boolean variable: ignore 0xe025 event or not. I think that whole quirk
structure is not needed yet (and I would be happy if never in future).
> /*
> * Certain keys are flagged as KE_IGNORE. All of these are either
> * notifications (rather than requests for change) or are also sent
> @@ -87,7 +119,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> { KE_IGNORE, 0xe020, { KEY_MUTE } },
>
> /* Shortcut and audio panel keys */
> - { KE_IGNORE, 0xe025, { KEY_RESERVED } },
> + { KE_KEY, 0xe025, { KEY_PROG4 } },
> { KE_IGNORE, 0xe026, { KEY_RESERVED } },
>
> { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> @@ -183,6 +215,9 @@ static void dell_wmi_process_key(int reported_key)
> key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
> return;
>
> + if (key->keycode == KEY_PROG4 && !quirks->process_dell_instant_launch)
> + return;
> +
> sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
> }
>
> @@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
> return -ENODEV;
> }
>
> + quirks = &quirk_unknown;
Unknown sounds like something is not know or we do not know what it is.
But here we know exactly what is needed (= ignore 0xe025 key).
> + dmi_check_system(dell_wmi_quirks);
> dmi_walk(find_hk_type, NULL);
> acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
>
--
Pali Rohár
[email protected]
On Thursday 03 December 2015 17:16:06 Darren Hart wrote:
> This looks fine to me, and if Pali will ack it, I'll move it from for-review to
> testing and Andy will need to update patch 14/14 to accomodate - unless you guys
> decide to include this in his.
This patch is not enough for enabling 0xe025 key on that Vostro machine.
Some extra SMBIOS call is needed, without them ACPI will not send WMI
keypress event.
Dell SMBIOS call can be done either via WMI or via SMI inb/outb
instructions which implements dcdbas.ko driver. Module dell-laptop.ko
uses only SMBIOS API for all functionality and uses dcdbas.ko.
There is another driver which uses SMBIOS API, but use WMI calls. It is
dell-led.ko in leds subsystem.
So now I do not know where to put that needed SMBIOS call for enabling
0xe025 hotkey event and also I do not know if it is better to use WMI or
dcdbas.ko.
Maybe now it could make sense to unify Dell SMBIOS API in kernel and
move common functions to one place and let drivers to use just common
functions. According to older Dell ACPI WMI documentation in DMI is bit
which specify if BIOS support SMBIOS via WMI or not.
At least I think this one patch should not be included into kernel until
there will be full support for 0xe025 key (adding that SMBIOS call).
--
Pali Rohár
[email protected]
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -47,6 +47,38 @@ static int acpi_video;
> >
> > MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
> >
> > +struct quirk_entry {
> > + bool process_dell_instant_launch;
> > +};
> > +
> > +static struct quirk_entry *quirks;
> > +
> > +static struct quirk_entry quirk_unknown = {
> > +};
> > +
> > +static struct quirk_entry quirk_dell_vostro_v131 = {
> > + .process_dell_instant_launch = true,
> > +};
> > +
> > +static int __init dmi_matched(const struct dmi_system_id *dmi)
> > +{
> > + quirks = dmi->driver_data;
> > + return 1;
> > +}
> > +
> > +static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
> > + {
> > + .callback = dmi_matched,
> > + .ident = "Dell Vostro V131",
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> > + },
> > + .driver_data = &quirk_dell_vostro_v131,
> > + },
> > + { }
> > +};
> > +
>
> It is not possible to simplify this part of code? Currently we only need
> boolean variable: ignore 0xe025 event or not. I think that whole quirk
> structure is not needed yet (and I would be happy if never in future).
Of course it is possible. I just got the feeling that using a quirk
structure is the way to go for this subsystem as it currently contains 7
drivers using something like this. Moreover, I saw that in some commits
initially adding a quirk structure to a driver (2d8b90be, a979e2e2) that
structure contained just one field.
So, between KISS and following the beaten path, I chose the latter. If
you still think this patch should be reworked to use a single global
boolean instead, please let me know and I'll prepare a v3.
> > @@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
> > return -ENODEV;
> > }
> >
> > + quirks = &quirk_unknown;
>
> Unknown sounds like something is not know or we do not know what it is.
> But here we know exactly what is needed (= ignore 0xe025 key).
Again, not my idea, I just thought it would be wise to follow what seems
to be an established pattern:
$ git grep 'quirk.*unknown' drivers/platform/x86/
--
Best regards,
Michał Kępień
> > On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> > Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> > is generated. As there seems to be no ACPI method or SMI call to
> > determine without possible side effects whether a given machine is
> > capable of simulating a keypress when this hotkey is pressed, DMI
> > matching is used to whitelist the models for which an input event should
> > be generated when WMI event 0xe025 is received.
> >
> > Signed-off-by: Michał Kępień <[email protected]>
> > ---
> > Changes from v1:
> >
> > - Use DMI matching instead of a module parameter
> > - Change flag name to improve readability
> >
> > Darren, I am aware this patch conflicts with Andy's WMI rework series
> > posted yesterday, so please just let me know if you want me to rebase.
>
> Thanks for the heads' up. Andy's series is longer and likely going to need more
> review from more people, so I don't necessarily want to hold this one up on it -
> unless it makes sense to include it in that series so they can evolve together.
> I'll leave that to you and Andy to decide.
As merging my patch with Andy's work is just a matter of moving code
around, not actually changing it, personally I don't feel this patch
should be somehow linked to the WMI rework. Andy, please let me know if
you disagree, I don't really have any strong feelings about this.
--
Best regards,
Michał Kępień
> This patch is not enough for enabling 0xe025 key on that Vostro machine.
> Some extra SMBIOS call is needed, without them ACPI will not send WMI
> keypress event.
Indeed. But have you read the last e-mail I wrote before submitting the
original patch [1]? Brightness control on the V131 is already broken
"out of the box" with newer kernels (flickering upon brightness change),
but if we do what you're suggesting and include the SMI call in the
kernel, we'll break it even more, to the point where pressing one of the
brightness control keys might not result in any brightness change at
all. Sure, we can fix that by overriding an arbitrary ACPI method. Oh,
wait, did I say "fix"?
I posted the patch without the SMI call because that way if you want to
use the Dell Instant Launch hotkey, you just fire up a userspace script
(which uses libsmbios and takes care of overriding the ACPI method) and
chances are you will end up with a fully functional system. Of course
you need to understand that using this script is not an elegant solution
and that it might break something else, but it's your choice, not the
kernel's. And the patch itself does not change kernel's default
behavior, so we're not risking breaking any other models out there.
> At least I think this one patch should not be included into kernel until
> there will be full support for 0xe025 key (adding that SMBIOS call).
Again, fully supporting the Dell Instant Launch hotkey makes brightness
control even more broken than it has to be. In other words, everything
is terrible.
The only real solution to all these issues is a BIOS fix and I'm pretty
sure it's not happening.
[1] http://www.spinics.net/lists/platform-driver-x86/msg07679.html
--
Best regards,
Michał Kępień
On Fri, Dec 4, 2015 at 4:55 AM, Michał Kępień <[email protected]> wrote:
>> > On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
>> > Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
>> > is generated. As there seems to be no ACPI method or SMI call to
>> > determine without possible side effects whether a given machine is
>> > capable of simulating a keypress when this hotkey is pressed, DMI
>> > matching is used to whitelist the models for which an input event should
>> > be generated when WMI event 0xe025 is received.
>> >
>> > Signed-off-by: Michał Kępień <[email protected]>
>> > ---
>> > Changes from v1:
>> >
>> > - Use DMI matching instead of a module parameter
>> > - Change flag name to improve readability
>> >
>> > Darren, I am aware this patch conflicts with Andy's WMI rework series
>> > posted yesterday, so please just let me know if you want me to rebase.
>>
>> Thanks for the heads' up. Andy's series is longer and likely going to need more
>> review from more people, so I don't necessarily want to hold this one up on it -
>> unless it makes sense to include it in that series so they can evolve together.
>> I'll leave that to you and Andy to decide.
>
> As merging my patch with Andy's work is just a matter of moving code
> around, not actually changing it, personally I don't feel this patch
> should be somehow linked to the WMI rework. Andy, please let me know if
> you disagree, I don't really have any strong feelings about this.
>'
I think it's just a matter of which lands first. It shouldn't be a big deal.
--Andy