[PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[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 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ń <kernel@kempniu.pl>
---
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

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Pali Rohár]

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ń <kernel@kempniu.pl>
> ---
> 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
pali.rohar@gmail.com

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Michał Kępień]

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ń

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3344 bytes --]

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
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Michał Kępień]

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ń

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Pali Rohár]

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
pali.rohar@gmail.com

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Michał Kępień]

> 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ń

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Darren Hart]

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

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Darren Hart]

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ń <kernel@kempniu.pl>

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

Re: [PATCH] dell-wmi: add module param to control Dell Instant Launch hotkey processing

[Michał Kępień]

> 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ń

[PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[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ń <kernel@kempniu.pl>
---
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

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Darren Hart]

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ń <kernel@kempniu.pl>
> ---
> 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

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

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
pali.rohar@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[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ń

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Darren Hart]

On Fri, Dec 04, 2015 at 02:27:45PM +0100, Michał Kępień wrote:
> > 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
> 

I have this patch as still pending review, and I *think* where we are is Pali
had some objections but Michal felt they didn't help address the problem.

Where are we?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2557 bytes --]

On Monday 11 January 2016 20:12:26 Darren Hart wrote:
> On Fri, Dec 04, 2015 at 02:27:45PM +0100, Michał Kępień wrote:
> > > 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
> 
> I have this patch as still pending review, and I *think* where we are
> is Pali had some objections but Michal felt they didn't help address
> the problem.
> 
> Where are we?

Hi Darren! Hans fixed our big problem and with his patches (to acpi 
video keypresses) we are able to finally fix support for that Vostro 
machine. I had discussion with Michał and he will prepare new patch 
series with other changes in smbios... So for now just wait when Michał 
is ready.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

Michał, can you prepare new (v3) version of this patch? Now required
acpi video changes are included and so dell-wmi changes should go to...
To finally fix this keypress bug on Dell Vostro V131 machine.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> Michał, can you prepare new (v3) version of this patch? Now required
> acpi video changes are included and so dell-wmi changes should go to...
> To finally fix this keypress bug on Dell Vostro V131 machine.

I keep this on my to-do list, but the updated patch will depend on the
final version of the SMBIOS API rework, so I guess there is little point
in posting it now as that API is subject to change.  But rest assured
that as soon as the final version of the API rework series (which I have
yet to prepare, of course) gets applied by Darren, I will post a v3 of
this patch - after all, it is the very reason I am working on the API
rework.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 21 January 2016 11:52:03 Michał Kępień wrote:
> > Michał, can you prepare new (v3) version of this patch? Now required
> > acpi video changes are included and so dell-wmi changes should go to...
> > To finally fix this keypress bug on Dell Vostro V131 machine.
> 
> I keep this on my to-do list, but the updated patch will depend on the
> final version of the SMBIOS API rework, so I guess there is little point
> in posting it now as that API is subject to change.  But rest assured
> that as soon as the final version of the API rework series (which I have
> yet to prepare, of course) gets applied by Darren, I will post a v3 of
> this patch - after all, it is the very reason I am working on the API
> rework.

There is still need to patch dell-wmi.c? And this change does not depend
on another SMBIOS change (in dell-laptop), right?

I still have in my mind some changes for dell-wmi.c code but I'm waiting
until all other changes for dell-wmi.c will be merged to prevent
conflicts and needed rebase.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > > Michał, can you prepare new (v3) version of this patch? Now required
> > > acpi video changes are included and so dell-wmi changes should go to...
> > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > 
> > I keep this on my to-do list, but the updated patch will depend on the
> > final version of the SMBIOS API rework, so I guess there is little point
> > in posting it now as that API is subject to change.  But rest assured
> > that as soon as the final version of the API rework series (which I have
> > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > this patch - after all, it is the very reason I am working on the API
> > rework.
> 
> There is still need to patch dell-wmi.c? And this change does not depend
> on another SMBIOS change (in dell-laptop), right?

Well, back in December, you wrote [1]:

> 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.
> 
> (...)
> 
> 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).

>From the above I understood that first you want to unify the Dell SMBIOS
API used throughout the kernel (that's currently in progress), so that
it can then be used in dell-wmi as well to perform the SMBIOS call
needed on the Vostro V131.

If you want me to just rework the patch so that it doesn't introduce a
quirk structure, I recalled another reason to use it after all: there
are other Dell laptops which require the special SMI for enabling WMI
events, but report the Dell Instant Launch Hotkey using a different WMI
event code [2].  So I'd say that two separate issues should be fixed
using DMI matching:

  * whether the special SMBIOS call is required,
  * whether the 0x0e25 event should be translated into a keypress.

A quirk structure looks like an elegant way to deal with this.

Could you please advise how you would like me to proceed with this?

[1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > > Michał, can you prepare new (v3) version of this patch? Now required
> > > acpi video changes are included and so dell-wmi changes should go to...
> > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > 
> > I keep this on my to-do list, but the updated patch will depend on the
> > final version of the SMBIOS API rework, so I guess there is little point
> > in posting it now as that API is subject to change.  But rest assured
> > that as soon as the final version of the API rework series (which I have
> > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > this patch - after all, it is the very reason I am working on the API
> > rework.
> 
> There is still need to patch dell-wmi.c? And this change does not depend
> on another SMBIOS change (in dell-laptop), right?

Well, back in December, you wrote [1]:

> 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.
> 
> (...)
> 
> 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).

From the above I understood that first you want to unify the Dell SMBIOS
API used throughout the kernel (that's currently in progress), so that
it can then be used in dell-wmi as well to perform the SMBIOS call
needed on the Vostro V131.

If you want me to just rework the patch so that it doesn't introduce a
quirk structure, I recalled another reason to use it after all: there
are other Dell laptops which require the special SMI for enabling WMI
events, but report the Dell Instant Launch Hotkey using a different WMI
event code [2].  So I'd say that two separate issues should be fixed
using DMI matching:

  * whether the special SMBIOS call is required,
  * whether the 0x0e25 event should be translated into a keypress.

A quirk structure looks like an elegant way to deal with this.

Could you please advise how you would like me to proceed with this?

[1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 21 January 2016 15:56:12 Michał Kępień wrote:
> > > > Michał, can you prepare new (v3) version of this patch? Now required
> > > > acpi video changes are included and so dell-wmi changes should go to...
> > > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > > 
> > > I keep this on my to-do list, but the updated patch will depend on the
> > > final version of the SMBIOS API rework, so I guess there is little point
> > > in posting it now as that API is subject to change.  But rest assured
> > > that as soon as the final version of the API rework series (which I have
> > > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > > this patch - after all, it is the very reason I am working on the API
> > > rework.
> > 
> > There is still need to patch dell-wmi.c? And this change does not depend
> > on another SMBIOS change (in dell-laptop), right?
> 
> Well, back in December, you wrote [1]:
> 
> > 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.
> > 
> > (...)
> > 
> > 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).
> 
> From the above I understood that first you want to unify the Dell SMBIOS
> API used throughout the kernel (that's currently in progress), so that
> it can then be used in dell-wmi as well to perform the SMBIOS call
> needed on the Vostro V131.
> 
> If you want me to just rework the patch so that it doesn't introduce a
> quirk structure, I recalled another reason to use it after all: there
> are other Dell laptops which require the special SMI for enabling WMI
> events, but report the Dell Instant Launch Hotkey using a different WMI
> event code [2].  So I'd say that two separate issues should be fixed
> using DMI matching:
> 
>   * whether the special SMBIOS call is required,
>   * whether the 0x0e25 event should be translated into a keypress.
> 
> A quirk structure looks like an elegant way to deal with this.
> 
> Could you please advise how you would like me to proceed with this?
> 
> [1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12
> 

Hm...

For full support we need:

1) invoke special SMBIOS call for some machine

2) patch dell-wmi.c to do not drop some events for some machines

Part 2) needs to touch only dell-wmi.c code, this is obvious. But I
thought that part 1) will be done in dell-laptop.c code where are all
others SMBIOS calls. Reason is just because dell-wmi.c is doing WMI
stuff. Do you want to include this is part 1) also to dell-wmi.c file?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> For full support we need:
> 
> 1) invoke special SMBIOS call for some machine
> 
> 2) patch dell-wmi.c to do not drop some events for some machines
> 
> Part 2) needs to touch only dell-wmi.c code, this is obvious. But I
> thought that part 1) will be done in dell-laptop.c code where are all
> others SMBIOS calls. Reason is just because dell-wmi.c is doing WMI
> stuff. Do you want to include this is part 1) also to dell-wmi.c file?

Yes, that was my idea.  That SMBIOS call is basically WMI initialization
code.  Consider the case of Vostro V131 - there is no WMI on that
machine without that SMBIOS call and there is no need to issue that
SMBIOS call if you don't want to use WMI.  Given that dell-wmi does not
depend on dell-laptop, consider the extreme case where the user selects
CONFIG_DELL_LAPTOP=n and CONFIG_DELL_WMI=m, expecting WMI to work.  If
we put the WMI-enabling SMBIOS call in dell-laptop, it won't.  Also,
once my Dell SMBIOS API rework series gets applied, adding the relevant
call to dell-wmi will be easy.

-- 
Best regards,
Michał Kępień

[PATCH v3 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

[Michał Kępień]

Changes from v2:

  - Use a static variable instead of a quirk structure

  - Use the API exported by dell-smbios to issue the SMBIOS request
    required for generating WMI events, returning with error from
    dell_wmi_init() if it fails

  - Move dell_smi_error() from dell-laptop to dell-smbios and use it to
    determine the error code for returning from dell_wmi_init() when
    enabling WMI fails

  - Support Dell Inspiron M5110

Changes from v1:

  - Use DMI matching instead of a module parameter
  - Change flag name to improve readability

This patch series makes use of the API exported by dell-smbios, so it
should be applied to either testing or dell-smbios.

This patch series was tested on a Dell Inspiron M5110 by Darek Stojaczyk
(CC'd), who reported that it had similar issues as Dell Vostro V131 back
in July 2015 [1].

Pali, returning to our debate whether to place the WMI-enabling SMBIOS
request in dell-laptop or in dell-wmi, I still strongly recommend the
latter.  Apart from the reasons I had discussed before [2], I came up
with another one.  The SMBIOS request in question registers/unregisters
an event listener (dell-wmi).  If we only register for WMI events in
dell-laptop and dell-wmi gets unloaded at some point further on,
brightness keys will not be properly processed any more as they will
only be reported using WMI.  If we properly unregister in
dell_wmi_exit(), brightness keys will be properly reported by ACPI as if
dell-wmi was never loaded.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12
[2] http://www.spinics.net/lists/platform-driver-x86/msg08289.html

 drivers/platform/x86/Kconfig       |    1 +
 drivers/platform/x86/dell-laptop.c |   30 +++++------------
 drivers/platform/x86/dell-smbios.c |   16 +++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 drivers/platform/x86/dell-wmi.c    |   63 +++++++++++++++++++++++++++++++++++-
 5 files changed, 89 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[PATCH v3 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Michał Kępień]

The dell_smi_error() method could be used by modules other than
dell-laptop for convenient translation of SMBIOS request errors into
errno values.  Thus, move it to dell-smbios.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-laptop.c |   14 --------------
 drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 76064c8..cbafb95 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static inline int dell_smi_error(int value)
-{
-	switch (value) {
-	case 0: /* Completed successfully */
-		return 0;
-	case -1: /* Completed with error */
-		return -EIO;
-	case -2: /* Function not supported */
-		return -ENXIO;
-	default: /* Unknown error */
-		return -EINVAL;
-	}
-}
-
 /*
  * Derived from information in smbios-wireless-ctl:
  *
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 2a4992a..942572f 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -39,6 +40,21 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(dell_smi_error);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 4f69b16..52febe6 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,8 @@ struct calling_interface_token {
 	};
 };
 
+int dell_smi_error(int value);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);
-- 
1.7.10.4

[PATCH v3 2/5] dell-smbios: rename dell_smi_error() to dell_smbios_error()

[Michał Kępień]

As dell_smi_error() is exported by dell-smbios, its prefix should be
consistent with other exported symbols, so change function name to
dell_smbios_error().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-laptop.c |   16 ++++++++--------
 drivers/platform/x86/dell-smbios.c |    4 ++--
 drivers/platform/x86/dell-smbios.h |    2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index cbafb95..2c2f02b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -433,7 +433,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 
  out:
 	dell_smbios_release_buffer();
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 /* Must be called with the buffer held */
@@ -876,7 +876,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	else
 		dell_smbios_send_request(1, 1);
 
-	ret = dell_smi_error(buffer->output[0]);
+	ret = dell_smbios_error(buffer->output[0]);
 
 	dell_smbios_release_buffer();
 	return ret;
@@ -901,7 +901,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 		dell_smbios_send_request(0, 1);
 
 	if (buffer->output[0])
-		ret = dell_smi_error(buffer->output[0]);
+		ret = dell_smbios_error(buffer->output[0]);
 	else
 		ret = buffer->output[1];
 
@@ -1160,7 +1160,7 @@ static int kbd_get_info(struct kbd_info *info)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1249,7 +1249,7 @@ static int kbd_get_state(struct kbd_state *state)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1286,7 +1286,7 @@ static int kbd_set_state(struct kbd_state *state)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
@@ -1329,7 +1329,7 @@ static int kbd_set_token_bit(u8 bit)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_get_token_bit(u8 bit)
@@ -1354,7 +1354,7 @@ static int kbd_get_token_bit(u8 bit)
 	dell_smbios_release_buffer();
 
 	if (ret)
-		return dell_smi_error(ret);
+		return dell_smbios_error(ret);
 
 	return (val == token->value);
 }
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 942572f..d2412ab 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -40,7 +40,7 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
-int dell_smi_error(int value)
+int dell_smbios_error(int value)
 {
 	switch (value) {
 	case 0: /* Completed successfully */
@@ -53,7 +53,7 @@ int dell_smi_error(int value)
 		return -EINVAL;
 	}
 }
-EXPORT_SYMBOL_GPL(dell_smi_error);
+EXPORT_SYMBOL_GPL(dell_smbios_error);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 52febe6..ec7d40a 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,7 +35,7 @@ struct calling_interface_token {
 	};
 };
 
-int dell_smi_error(int value);
+int dell_smbios_error(int value);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
-- 
1.7.10.4

[PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

On some laptop models (e.g. Dell Vostro V131), WMI events are not
generated until a specific SMBIOS request is issued to register an event
listener [1].  As there seems to be no ACPI method or SMBIOS request to
determine without possible side effects whether a given machine needs to
issue this SMBIOS request in order to receive WMI events, DMI matching
is used to whitelist the models which need it.

[1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/Kconfig    |    1 +
 drivers/platform/x86/dell-wmi.c |   49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3e4d9c3..5ceb53a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -122,6 +122,7 @@ config DELL_WMI
 	depends on ACPI_WMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on DELL_SMBIOS
 	select INPUT_SPARSEKMAP
 	---help---
 	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 368e193..ca8233a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -37,6 +37,7 @@
 #include <linux/string.h>
 #include <linux/dmi.h>
 #include <acpi/video.h>
+#include "dell-smbios.h"
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static u32 dell_wmi_interface_version;
+static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+	wmi_requires_smbios_request = 1;
+	return 1;
+}
+
+static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
+	{
+		.callback = dmi_matched,
+		.ident = "Dell Vostro V131",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+	},
+	{ }
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
@@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
 {
 	int err;
 	acpi_status status;
+	struct calling_interface_buffer *buffer;
 
 	if (!wmi_has_guid(DELL_EVENT_GUID) ||
 	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
@@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
+	dmi_check_system(dell_wmi_smbios_list);
+
+	if (wmi_requires_smbios_request) {
+		buffer = dell_smbios_get_buffer();
+		buffer->input[0] = 0x10000;
+		buffer->input[1] = 0x51534554;
+		buffer->input[3] = 0x1;
+		dell_smbios_send_request(17, 3);
+		err = buffer->output[0];
+		dell_smbios_release_buffer();
+		if (err) {
+			pr_err("Failed to enable WMI (error %d)\n", err);
+			wmi_remove_notify_handler(DELL_EVENT_GUID);
+			dell_wmi_input_destroy();
+			return dell_smbios_error(err);
+		}
+	}
+
 	return 0;
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
+	struct calling_interface_buffer *buffer;
+
+	if (wmi_requires_smbios_request) {
+		buffer = dell_smbios_get_buffer();
+		buffer->input[0] = 0x10000;
+		buffer->input[1] = 0x51534554;
+		dell_smbios_send_request(17, 3);
+		dell_smbios_release_buffer();
+	}
+
 	wmi_remove_notify_handler(DELL_EVENT_GUID);
 	dell_wmi_input_destroy();
 }
-- 
1.7.10.4

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Tuesday 16 February 2016 15:50:28 Michał Kępień wrote:
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		buffer->input[3] = 0x1;
> +		dell_smbios_send_request(17, 3);
> +		err = buffer->output[0];
> +		dell_smbios_release_buffer();
> +		if (err) {
> +			pr_err("Failed to enable WMI (error %d)\n", err);
> +			wmi_remove_notify_handler(DELL_EVENT_GUID);
> +			dell_wmi_input_destroy();
> +			return dell_smbios_error(err);
> +		}
> +	}
> +
>  	return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> +	struct calling_interface_buffer *buffer;
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		dell_smbios_send_request(17, 3);
> +		dell_smbios_release_buffer();
> +	}
> +
>  	wmi_remove_notify_handler(DELL_EVENT_GUID);
>  	dell_wmi_input_destroy();
>  }

Hi! I would propose moving this get_buffer, send, release code into own
function with boolean argument on/off. This de-duplicate same code plus
decrease level of indentation in _init function. And maybe adding ascii
string comment representation for that 0x5153... number can be useful.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> >  static void __exit dell_wmi_exit(void)
> >  {
> > +	struct calling_interface_buffer *buffer;
> > +
> > +	if (wmi_requires_smbios_request) {
> > +		buffer = dell_smbios_get_buffer();
> > +		buffer->input[0] = 0x10000;
> > +		buffer->input[1] = 0x51534554;
> > +		dell_smbios_send_request(17, 3);
> > +		dell_smbios_release_buffer();
> > +	}
> > +
> >  	wmi_remove_notify_handler(DELL_EVENT_GUID);
> >  	dell_wmi_input_destroy();
> >  }
> 
> Hi! I would propose moving this get_buffer, send, release code into own
> function with boolean argument on/off. This de-duplicate same code plus
> decrease level of indentation in _init function. And maybe adding ascii
> string comment representation for that 0x5153... number can be useful.

Ah, yes, sure.  I will do that in v4.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Tue, Feb 16, 2016 at 03:50:28PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), WMI events are not
> generated until a specific SMBIOS request is issued to register an event
> listener [1].  As there seems to be no ACPI method or SMBIOS request to
> determine without possible side effects whether a given machine needs to
> issue this SMBIOS request in order to receive WMI events, DMI matching
> is used to whitelist the models which need it.
> 
> [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/Kconfig    |    1 +
>  drivers/platform/x86/dell-wmi.c |   49 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3e4d9c3..5ceb53a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -122,6 +122,7 @@ config DELL_WMI
>  	depends on ACPI_WMI
>  	depends on INPUT
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> +	depends on DELL_SMBIOS
>  	select INPUT_SPARSEKMAP
>  	---help---
>  	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 368e193..ca8233a 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -37,6 +37,7 @@
>  #include <linux/string.h>
>  #include <linux/dmi.h>
>  #include <acpi/video.h>
> +#include "dell-smbios.h"
>  
>  MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
>  MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
>  #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
>  static u32 dell_wmi_interface_version;
> +static bool wmi_requires_smbios_request;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
>  
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> +	wmi_requires_smbios_request = 1;
> +	return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
> +	{
> +		.callback = dmi_matched,
> +		.ident = "Dell Vostro V131",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> +		},
> +	},
> +	{ }
> +};
> +
>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
>  {
>  	int err;
>  	acpi_status status;
> +	struct calling_interface_buffer *buffer;

Please place the longest line first, and move int err to the last declaration.
When changing declarations of local variables, please use "Reverse Christmas
Tree" order (longest line to shortest line) wherever possible.

>  
>  	if (!wmi_has_guid(DELL_EVENT_GUID) ||
>  	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
> @@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
>  		return -ENODEV;
>  	}
>  
> +	dmi_check_system(dell_wmi_smbios_list);
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		buffer->input[3] = 0x1;
> +		dell_smbios_send_request(17, 3);
> +		err = buffer->output[0];
> +		dell_smbios_release_buffer();
> +		if (err) {
> +			pr_err("Failed to enable WMI (error %d)\n", err);
> +			wmi_remove_notify_handler(DELL_EVENT_GUID);
> +			dell_wmi_input_destroy();
> +			return dell_smbios_error(err);
> +		}
> +	}
> +
>  	return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> +	struct calling_interface_buffer *buffer;
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		dell_smbios_send_request(17, 3);
> +		dell_smbios_release_buffer();
> +	}
> +

Pali's point about documenting the hardcoded values and eliminating the code
duplication with a function (inline) is a good one.

Otherwise, this series looks good to me. Looking forward to merging v4.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> >  /*
> >   * Certain keys are flagged as KE_IGNORE. All of these are either
> >   * notifications (rather than requests for change) or are also sent
> > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> >  {
> >  	int err;
> >  	acpi_status status;
> > +	struct calling_interface_buffer *buffer;
> 
> Please place the longest line first, and move int err to the last declaration.
> When changing declarations of local variables, please use "Reverse Christmas
> Tree" order (longest line to shortest line) wherever possible.

Thanks, I'll keep that in mind for the future, though putting the
WMI-enabling SMBIOS request in a separate function renders the need for
the buffer variable in dell_wmi_init() void, so v4 won't touch this area
any more.

> Pali's point about documenting the hardcoded values and eliminating the code
> duplication with a function (inline) is a good one.

I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
just something pulled out of a hat (as the link provided in the commit
message proves) and input[3] should be self-explanatory due to the name
of the variable whose value is put into it.

By the way, is there any kernel-wide or subsystem-wide policy for
marking a function inline?  I mean, this is hardly time-critical code,
so is your suggestion to make it inline just a preference or am I
unaware of some rule?

> Otherwise, this series looks good to me. Looking forward to merging v4.

I'll try to post a v4 within the next couple of days.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Monday 22 February 2016 09:56:50 Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >  	int err;
> > >  	acpi_status status;
> > > +	struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.

Maybe you can add documentation which we got from Dell on some ML about
this SMI call. Similarly what I added in dell-laptop.c...

> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

IIRC recent versions of gcc ignores "inline" keyword and inline
functions as needed when doing optimizations.

If there is some functions which must be inlined you need to to use gcc
attrbiute always_inline.

But if there is policy? I do not know, maybe somebody else should
comment it.

> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> > > Pali's point about documenting the hardcoded values and eliminating the code
> > > duplication with a function (inline) is a good one.
> > 
> > I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
> > just something pulled out of a hat (as the link provided in the commit
> > message proves) and input[3] should be self-explanatory due to the name
> > of the variable whose value is put into it.
> 
> Maybe you can add documentation which we got from Dell on some ML about
> this SMI call. Similarly what I added in dell-laptop.c...

Sure, I can do that.

> > By the way, is there any kernel-wide or subsystem-wide policy for
> > marking a function inline?  I mean, this is hardly time-critical code,
> > so is your suggestion to make it inline just a preference or am I
> > unaware of some rule?
> 
> IIRC recent versions of gcc ignores "inline" keyword and inline
> functions as needed when doing optimizations.

This was my hunch as well, but I couldn't find any proof immediately,
hence the question.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Mon, Feb 22, 2016 at 09:56:50AM +0100, Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >  	int err;
> > >  	acpi_status status;
> > > +	struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.
> 
> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

I suggested inline because the code was inline before and there would be no size
overhead to continue to make it inline. That said, the best source of guidance
on this is in CodingStyle, Chapter 15. While this function is quite small and
static to the file, it is not performance critical as you say. So, upon closer
inspection, there is no real need to be inline. And in fact, as there is no need
for it, it perhaps should not be. Thanks for raising the question.

> 
> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

Great, thank you.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v3 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

On models on which an SMBIOS request needs to be issued in order for WMI
events to be generated, pressing the Dell Instant Launch hotkey does not
raise an i8042 interrupt - only a WMI event is generated (0xe025 on Dell
Vostro V131).  Thus, the 0xe025 event should only be ignored on machines
which do not require an SMBIOS request for enabling WMI.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-wmi.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index ca8233a..ad7504d 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -111,7 +111,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 } },
@@ -208,6 +208,9 @@ static void dell_wmi_process_key(int reported_key)
 	    acpi_video_handles_brightness_key_presses())
 		return;
 
+	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
+		return;
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
1.7.10.4

[PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Michał Kępień]

Similarly to Dell Vostro V131, Dell Inspiron M5110 also requires an
SMBIOS request to be issued in order for WMI events to be generated and
does not raise an i8042 interrupt when the Dell Instant Launch hotkey is
pressed.  However, the event code for that hotkey on this machine is
0xe029, so add it to the legacy keymap.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
Tested-by: Darek Stojaczyk <darek.stojaczyk@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index ad7504d..650ca45 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -62,6 +62,14 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 	{
 		.callback = dmi_matched,
+		.ident = "Dell Inspiron M5110",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron M5110"),
+		},
+	},
+	{
+		.callback = dmi_matched,
 		.ident = "Dell Vostro V131",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -113,6 +121,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	/* Shortcut and audio panel keys */
 	{ KE_KEY, 0xe025, { KEY_PROG4 } },
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
+	{ KE_KEY, 0xe029, { KEY_PROG4 } },
 
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
-- 
1.7.10.4

Re: [PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Pali Rohár]

On Tuesday 16 February 2016 15:50:30 Michał Kępień wrote:
>  	/* Shortcut and audio panel keys */
>  	{ KE_KEY, 0xe025, { KEY_PROG4 } },
>  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> +	{ KE_KEY, 0xe029, { KEY_PROG4 } },

Hi! Above comment (Shortcut and audio panel keys) is not relevant for
this 0xe029, right? Then please break this new key from above two and
adds useful comment what is that key.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Michał Kępień]

> >  	/* Shortcut and audio panel keys */
> >  	{ KE_KEY, 0xe025, { KEY_PROG4 } },
> >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > +	{ KE_KEY, 0xe029, { KEY_PROG4 } },
> 
> Hi! Above comment (Shortcut and audio panel keys) is not relevant for
> this 0xe029, right?

I believe it is relevant.  The commit which added that comment
(f1566f0d) was made by Seth Forshee to support Dell XPS L502X.  The term
"shortcut key" is used in the bug report linked from that commit [1],
but the manual for that model [2] describes the hotkey in question as
"Instant launch control" (page 43), so I believe both Seth's commit and
my patch refer to the same thing.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/815914/comments/6
[2] http://downloads.dell.com/Manuals/all-products/esuprt_laptop/esuprt_xps_laptop/xps-l502x_setup%20guide_en-us.pdf

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Pali Rohár]

On Tuesday 16 February 2016 23:03:12 Michał Kępień wrote:
> > >  	/* Shortcut and audio panel keys */
> > >  	{ KE_KEY, 0xe025, { KEY_PROG4 } },
> > >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > > +	{ KE_KEY, 0xe029, { KEY_PROG4 } },
> > 
> > Hi! Above comment (Shortcut and audio panel keys) is not relevant for
> > this 0xe029, right?
> 
> I believe it is relevant.  The commit which added that comment
> (f1566f0d) was made by Seth Forshee to support Dell XPS L502X.  The term
> "shortcut key" is used in the bug report linked from that commit [1],
> but the manual for that model [2] describes the hotkey in question as
> "Instant launch control" (page 43), so I believe both Seth's commit and
> my patch refer to the same thing.
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/815914/comments/6
> [2] http://downloads.dell.com/Manuals/all-products/esuprt_laptop/esuprt_xps_laptop/xps-l502x_setup%20guide_en-us.pdf

Then please split above group of keys and add comment which key is
shortcut and which audio panel. Because now there will be three keys and
comment just for two -- in future will not know which code 0x... is
mapped to which key.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Michał Kępień]

> > > >  	/* Shortcut and audio panel keys */
> > > >  	{ KE_KEY, 0xe025, { KEY_PROG4 } },
> > > >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > > > +	{ KE_KEY, 0xe029, { KEY_PROG4 } },
> > > 
> > > Hi! Above comment (Shortcut and audio panel keys) is not relevant for
> > > this 0xe029, right?
> > 
> > I believe it is relevant.  The commit which added that comment
> > (f1566f0d) was made by Seth Forshee to support Dell XPS L502X.  The term
> > "shortcut key" is used in the bug report linked from that commit [1],
> > but the manual for that model [2] describes the hotkey in question as
> > "Instant launch control" (page 43), so I believe both Seth's commit and
> > my patch refer to the same thing.
> > 
> > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/815914/comments/6
> > [2] http://downloads.dell.com/Manuals/all-products/esuprt_laptop/esuprt_xps_laptop/xps-l502x_setup%20guide_en-us.pdf
> 
> Then please split above group of keys and add comment which key is
> shortcut and which audio panel. Because now there will be three keys and
> comment just for two -- in future will not know which code 0x... is
> mapped to which key.

Please note that except for the first 5 entries, the keymap is sorted by
event code in ascending order.  If I understand correctly, doing what
you ask would break that ordering:

    /* Dell Instant Launch key */
    { KE_KEY, 0xe025, { KEY_PROG4 } },
    { KE_KEY, 0xe029, { KEY_PROG4 } },

    /* Audio panel key */
    { KE_IGNORE, 0xe026, { KEY_RESERVED } },

Is this acceptable?  Or perhaps you had something else on your mind?

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Pali Rohár]

On Wednesday 17 February 2016 13:01:58 Michał Kępień wrote:
> > > > >  	/* Shortcut and audio panel keys */
> > > > >  	{ KE_KEY, 0xe025, { KEY_PROG4 } },
> > > > >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > > > > +	{ KE_KEY, 0xe029, { KEY_PROG4 } },
> > > > 
> > > > Hi! Above comment (Shortcut and audio panel keys) is not relevant for
> > > > this 0xe029, right?
> > > 
> > > I believe it is relevant.  The commit which added that comment
> > > (f1566f0d) was made by Seth Forshee to support Dell XPS L502X.  The term
> > > "shortcut key" is used in the bug report linked from that commit [1],
> > > but the manual for that model [2] describes the hotkey in question as
> > > "Instant launch control" (page 43), so I believe both Seth's commit and
> > > my patch refer to the same thing.
> > > 
> > > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/815914/comments/6
> > > [2] http://downloads.dell.com/Manuals/all-products/esuprt_laptop/esuprt_xps_laptop/xps-l502x_setup%20guide_en-us.pdf
> > 
> > Then please split above group of keys and add comment which key is
> > shortcut and which audio panel. Because now there will be three keys and
> > comment just for two -- in future will not know which code 0x... is
> > mapped to which key.
> 
> Please note that except for the first 5 entries, the keymap is sorted by
> event code in ascending order.

I have prepared some patches which sort all event codes plus adds
missing comments... So after all dell patches are in Darren tree, I will
rewrite/rebase my and can send them.

> If I understand correctly, doing what you ask would break that ordering:
> 
>     /* Dell Instant Launch key */
>     { KE_KEY, 0xe025, { KEY_PROG4 } },
>     { KE_KEY, 0xe029, { KEY_PROG4 } },
> 
>     /* Audio panel key */
>     { KE_IGNORE, 0xe026, { KEY_RESERVED } },
> 
> Is this acceptable?  Or perhaps you had something else on your mind?

Thats better, now we know which event is audio and which launch key.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v3 5/5] dell-wmi: support Dell Inspiron M5110

[Michał Kępień]

> > Please note that except for the first 5 entries, the keymap is sorted by
> > event code in ascending order.
> 
> I have prepared some patches which sort all event codes plus adds
> missing comments... So after all dell patches are in Darren tree, I will
> rewrite/rebase my and can send them.
> 
> > If I understand correctly, doing what you ask would break that ordering:
> > 
> >     /* Dell Instant Launch key */
> >     { KE_KEY, 0xe025, { KEY_PROG4 } },
> >     { KE_KEY, 0xe029, { KEY_PROG4 } },
> > 
> >     /* Audio panel key */
> >     { KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > 
> > Is this acceptable?  Or perhaps you had something else on your mind?
> 
> Thats better, now we know which event is audio and which launch key.

Ok, then I will do that in v4.

-- 
Best regards,
Michał Kępień

[PATCH v4 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

[Michał Kępień]

This patch series makes use of the API exported by dell-smbios, so it
should be applied to either testing or dell-smbios.

Changes from v3:

  - Extract code issuing the WMI-controlling SMBIOS request into a
    separate function

  - Document the SMBIOS request used

  - Split keymap section "Shortcut and audio panel keys" into two
    separate sections

Changes from v2:

  - Use a static variable instead of a quirk structure

  - Use API exported by dell-smbios to issue the SMBIOS request required
    for generating WMI events, returning with error from dell_wmi_init()
    if it fails

  - Move dell_smi_error() from dell-laptop to dell-smbios and use it to
    determine error code for returning from dell_wmi_init() when
    enabling WMI fails

  - Support Dell Inspiron M5110

Changes from v1:

  - Use DMI matching instead of a module parameter
  - Change flag name to improve readability

 drivers/platform/x86/Kconfig       |    1 +
 drivers/platform/x86/dell-laptop.c |   30 ++++---------
 drivers/platform/x86/dell-smbios.c |   16 +++++++
 drivers/platform/x86/dell-smbios.h |    2 +
 drivers/platform/x86/dell-wmi.c    |   84 +++++++++++++++++++++++++++++++++++-
 5 files changed, 109 insertions(+), 24 deletions(-)

-- 
1.7.10.4

[PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Michał Kępień]

The dell_smi_error() method could be used by modules other than
dell-laptop for convenient translation of SMBIOS request errors into
errno values.  Thus, move it to dell-smbios.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-laptop.c |   14 --------------
 drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 76064c8..cbafb95 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static inline int dell_smi_error(int value)
-{
-	switch (value) {
-	case 0: /* Completed successfully */
-		return 0;
-	case -1: /* Completed with error */
-		return -EIO;
-	case -2: /* Function not supported */
-		return -ENXIO;
-	default: /* Unknown error */
-		return -EINVAL;
-	}
-}
-
 /*
  * Derived from information in smbios-wireless-ctl:
  *
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 2a4992a..942572f 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -39,6 +40,21 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(dell_smi_error);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 4f69b16..52febe6 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,8 @@ struct calling_interface_token {
 	};
 };
 
+int dell_smi_error(int value);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);
-- 
1.7.10.4

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Pali Rohár]

On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> The dell_smi_error() method could be used by modules other than
> dell-laptop for convenient translation of SMBIOS request errors into
> errno values.  Thus, move it to dell-smbios.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/dell-laptop.c |   14 --------------
>  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |    2 ++
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 76064c8..cbafb95 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> -static inline int dell_smi_error(int value)
> -{
> -	switch (value) {
> -	case 0: /* Completed successfully */
> -		return 0;
> -	case -1: /* Completed with error */
> -		return -EIO;
> -	case -2: /* Function not supported */
> -		return -ENXIO;
> -	default: /* Unknown error */
> -		return -EINVAL;
> -	}
> -}
> -
>  /*
>   * Derived from information in smbios-wireless-ctl:
>   *
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 2a4992a..942572f 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -16,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/dmi.h>
> +#include <linux/err.h>
>  #include <linux/gfp.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> @@ -39,6 +40,21 @@ static int da_command_code;
>  static int da_num_tokens;
>  static struct calling_interface_token *da_tokens;
>  
> +int dell_smi_error(int value)
> +{
> +	switch (value) {
> +	case 0: /* Completed successfully */
> +		return 0;
> +	case -1: /* Completed with error */
> +		return -EIO;
> +	case -2: /* Function not supported */
> +		return -ENXIO;
> +	default: /* Unknown error */
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(dell_smi_error);
> +
>  struct calling_interface_buffer *dell_smbios_get_buffer(void)
>  {
>  	mutex_lock(&buffer_mutex);
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index 4f69b16..52febe6 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -35,6 +35,8 @@ struct calling_interface_token {
>  	};
>  };
>  
> +int dell_smi_error(int value);
> +
>  struct calling_interface_buffer *dell_smbios_get_buffer(void);
>  void dell_smbios_clear_buffer(void);
>  void dell_smbios_release_buffer(void);

And... here what about inline vs EXPORT_SYMBOL function? Just asking...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Michał Kępień]

> On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> > The dell_smi_error() method could be used by modules other than
> > dell-laptop for convenient translation of SMBIOS request errors into
> > errno values.  Thus, move it to dell-smbios.
> > 
> > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > ---
> >  drivers/platform/x86/dell-laptop.c |   14 --------------
> >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> >  drivers/platform/x86/dell-smbios.h |    2 ++
> >  3 files changed, 18 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> > index 76064c8..cbafb95 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
> >  	{ }
> >  };
> >  
> > -static inline int dell_smi_error(int value)
> > -{
> > -	switch (value) {
> > -	case 0: /* Completed successfully */
> > -		return 0;
> > -	case -1: /* Completed with error */
> > -		return -EIO;
> > -	case -2: /* Function not supported */
> > -		return -ENXIO;
> > -	default: /* Unknown error */
> > -		return -EINVAL;
> > -	}
> > -}
> > -
> >  /*
> >   * Derived from information in smbios-wireless-ctl:
> >   *
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> > index 2a4992a..942572f 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/dmi.h>
> > +#include <linux/err.h>
> >  #include <linux/gfp.h>
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> > @@ -39,6 +40,21 @@ static int da_command_code;
> >  static int da_num_tokens;
> >  static struct calling_interface_token *da_tokens;
> >  
> > +int dell_smi_error(int value)
> > +{
> > +	switch (value) {
> > +	case 0: /* Completed successfully */
> > +		return 0;
> > +	case -1: /* Completed with error */
> > +		return -EIO;
> > +	case -2: /* Function not supported */
> > +		return -ENXIO;
> > +	default: /* Unknown error */
> > +		return -EINVAL;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > +
> >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> >  {
> >  	mutex_lock(&buffer_mutex);
> > diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> > index 4f69b16..52febe6 100644
> > --- a/drivers/platform/x86/dell-smbios.h
> > +++ b/drivers/platform/x86/dell-smbios.h
> > @@ -35,6 +35,8 @@ struct calling_interface_token {
> >  	};
> >  };
> >  
> > +int dell_smi_error(int value);
> > +
> >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> >  void dell_smbios_clear_buffer(void);
> >  void dell_smbios_release_buffer(void);
> 
> And... here what about inline vs EXPORT_SYMBOL function? Just asking...

Well, what about it? :)  The commit message is pretty explicit in
describing what happens here, i.e. a previously static function is moved
to another module so that it can be reused.  Thus, keeping the inline
keyword makes no sense.  What exactly is your concern?

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3850 bytes --]

On Monday 29 February 2016 21:22:54 Michał Kępień wrote:
> > On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> > > The dell_smi_error() method could be used by modules other than
> > > dell-laptop for convenient translation of SMBIOS request errors
> > > into errno values.  Thus, move it to dell-smbios.
> > > 
> > > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > > ---
> > > 
> > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > 100644
> > > --- a/drivers/platform/x86/dell-laptop.c
> > > +++ b/drivers/platform/x86/dell-laptop.c
> > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > dell_quirks[] __initconst = {
> > > 
> > >  	{ }
> > >  
> > >  };
> > > 
> > > -static inline int dell_smi_error(int value)
> > > -{
> > > -	switch (value) {
> > > -	case 0: /* Completed successfully */
> > > -		return 0;
> > > -	case -1: /* Completed with error */
> > > -		return -EIO;
> > > -	case -2: /* Function not supported */
> > > -		return -ENXIO;
> > > -	default: /* Unknown error */
> > > -		return -EINVAL;
> > > -	}
> > > -}
> > > -
> > > 
> > >  /*
> > >  
> > >   * Derived from information in smbios-wireless-ctl:
> > >   *
> > > 
> > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > 100644
> > > --- a/drivers/platform/x86/dell-smbios.c
> > > +++ b/drivers/platform/x86/dell-smbios.c
> > > @@ -16,6 +16,7 @@
> > > 
> > >  #include <linux/kernel.h>
> > >  #include <linux/module.h>
> > >  #include <linux/dmi.h>
> > > 
> > > +#include <linux/err.h>
> > > 
> > >  #include <linux/gfp.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/slab.h>
> > > 
> > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > 
> > >  static int da_num_tokens;
> > >  static struct calling_interface_token *da_tokens;
> > > 
> > > +int dell_smi_error(int value)
> > > +{
> > > +	switch (value) {
> > > +	case 0: /* Completed successfully */
> > > +		return 0;
> > > +	case -1: /* Completed with error */
> > > +		return -EIO;
> > > +	case -2: /* Function not supported */
> > > +		return -ENXIO;
> > > +	default: /* Unknown error */
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > +
> > > 
> > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > >  {
> > >  
> > >  	mutex_lock(&buffer_mutex);
> > > 
> > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > 100644
> > > --- a/drivers/platform/x86/dell-smbios.h
> > > +++ b/drivers/platform/x86/dell-smbios.h
> > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > 
> > >  	};
> > >  
> > >  };
> > > 
> > > +int dell_smi_error(int value);
> > > +
> > > 
> > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > >  void dell_smbios_clear_buffer(void);
> > >  void dell_smbios_release_buffer(void);
> > 
> > And... here what about inline vs EXPORT_SYMBOL function? Just
> > asking...
> 
> Well, what about it? :)  The commit message is pretty explicit in
> describing what happens here, i.e. a previously static function is
> moved to another module so that it can be reused.  Thus, keeping the
> inline keyword makes no sense.  What exactly is your concern?

Just asking if this function should be or not be inline (of course in 
header file, not in module .c).

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Michał Kępień]

> On Monday 29 February 2016 21:22:54 Michał Kępień wrote:
> > > On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> > > > The dell_smi_error() method could be used by modules other than
> > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > into errno values.  Thus, move it to dell-smbios.
> > > > 
> > > > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > > > ---
> > > > 
> > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > 100644
> > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > dell_quirks[] __initconst = {
> > > > 
> > > >  	{ }
> > > >  
> > > >  };
> > > > 
> > > > -static inline int dell_smi_error(int value)
> > > > -{
> > > > -	switch (value) {
> > > > -	case 0: /* Completed successfully */
> > > > -		return 0;
> > > > -	case -1: /* Completed with error */
> > > > -		return -EIO;
> > > > -	case -2: /* Function not supported */
> > > > -		return -ENXIO;
> > > > -	default: /* Unknown error */
> > > > -		return -EINVAL;
> > > > -	}
> > > > -}
> > > > -
> > > > 
> > > >  /*
> > > >  
> > > >   * Derived from information in smbios-wireless-ctl:
> > > >   *
> > > > 
> > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > 100644
> > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > @@ -16,6 +16,7 @@
> > > > 
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/dmi.h>
> > > > 
> > > > +#include <linux/err.h>
> > > > 
> > > >  #include <linux/gfp.h>
> > > >  #include <linux/mutex.h>
> > > >  #include <linux/slab.h>
> > > > 
> > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > 
> > > >  static int da_num_tokens;
> > > >  static struct calling_interface_token *da_tokens;
> > > > 
> > > > +int dell_smi_error(int value)
> > > > +{
> > > > +	switch (value) {
> > > > +	case 0: /* Completed successfully */
> > > > +		return 0;
> > > > +	case -1: /* Completed with error */
> > > > +		return -EIO;
> > > > +	case -2: /* Function not supported */
> > > > +		return -ENXIO;
> > > > +	default: /* Unknown error */
> > > > +		return -EINVAL;
> > > > +	}
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > +
> > > > 
> > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > >  {
> > > >  
> > > >  	mutex_lock(&buffer_mutex);
> > > > 
> > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > 100644
> > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > 
> > > >  	};
> > > >  
> > > >  };
> > > > 
> > > > +int dell_smi_error(int value);
> > > > +
> > > > 
> > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > >  void dell_smbios_clear_buffer(void);
> > > >  void dell_smbios_release_buffer(void);
> > > 
> > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > asking...
> > 
> > Well, what about it? :)  The commit message is pretty explicit in
> > describing what happens here, i.e. a previously static function is
> > moved to another module so that it can be reused.  Thus, keeping the
> > inline keyword makes no sense.  What exactly is your concern?
> 
> Just asking if this function should be or not be inline (of course in 
> header file, not in module .c).

If you mark a function as inline in the header file, you have to provide
its definition, otherwise you'll get a compilation error.  Given that
this is in no way performance-critical code, I see no point in
clobbering the header file with the body of this function.

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Darren Hart]

On Mon, Feb 29, 2016 at 09:41:36PM +0100, Michał Kępień wrote:
> > On Monday 29 February 2016 21:22:54 Michał Kępień wrote:
> > > > On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> > > > > The dell_smi_error() method could be used by modules other than
> > > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > > into errno values.  Thus, move it to dell-smbios.
> > > > > 
> > > > > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > > > > ---
> > > > > 
> > > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > > 100644
> > > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > > dell_quirks[] __initconst = {
> > > > > 
> > > > >  	{ }
> > > > >  
> > > > >  };
> > > > > 
> > > > > -static inline int dell_smi_error(int value)
> > > > > -{
> > > > > -	switch (value) {
> > > > > -	case 0: /* Completed successfully */
> > > > > -		return 0;
> > > > > -	case -1: /* Completed with error */
> > > > > -		return -EIO;
> > > > > -	case -2: /* Function not supported */
> > > > > -		return -ENXIO;
> > > > > -	default: /* Unknown error */
> > > > > -		return -EINVAL;
> > > > > -	}
> > > > > -}
> > > > > -
> > > > > 
> > > > >  /*
> > > > >  
> > > > >   * Derived from information in smbios-wireless-ctl:
> > > > >   *
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > > 100644
> > > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > > @@ -16,6 +16,7 @@
> > > > > 
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/dmi.h>
> > > > > 
> > > > > +#include <linux/err.h>
> > > > > 
> > > > >  #include <linux/gfp.h>
> > > > >  #include <linux/mutex.h>
> > > > >  #include <linux/slab.h>
> > > > > 
> > > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > > 
> > > > >  static int da_num_tokens;
> > > > >  static struct calling_interface_token *da_tokens;
> > > > > 
> > > > > +int dell_smi_error(int value)
> > > > > +{
> > > > > +	switch (value) {
> > > > > +	case 0: /* Completed successfully */
> > > > > +		return 0;
> > > > > +	case -1: /* Completed with error */
> > > > > +		return -EIO;
> > > > > +	case -2: /* Function not supported */
> > > > > +		return -ENXIO;
> > > > > +	default: /* Unknown error */
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > > +
> > > > > 
> > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > > >  {
> > > > >  
> > > > >  	mutex_lock(&buffer_mutex);
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > > 100644
> > > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > > 
> > > > >  	};
> > > > >  
> > > > >  };
> > > > > 
> > > > > +int dell_smi_error(int value);
> > > > > +
> > > > > 
> > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > > >  void dell_smbios_clear_buffer(void);
> > > > >  void dell_smbios_release_buffer(void);
> > > > 
> > > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > > asking...
> > > 
> > > Well, what about it? :)  The commit message is pretty explicit in
> > > describing what happens here, i.e. a previously static function is
> > > moved to another module so that it can be reused.  Thus, keeping the
> > > inline keyword makes no sense.  What exactly is your concern?
> > 
> > Just asking if this function should be or not be inline (of course in 
> > header file, not in module .c).
> 
> If you mark a function as inline in the header file, you have to provide
> its definition, otherwise you'll get a compilation error.  Given that
> this is in no way performance-critical code, I see no point in
> clobbering the header file with the body of this function.

Agreed, please leave it as is.

For a discussion on inline, please see:
CodingStyle: Chapter 15: The inline disease

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Michał Kępień]

> On Mon, Feb 29, 2016 at 09:41:36PM +0100, Michał Kępień wrote:
> > > On Monday 29 February 2016 21:22:54 Michał Kępień wrote:
> > > > > On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> > > > > > The dell_smi_error() method could be used by modules other than
> > > > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > > > into errno values.  Thus, move it to dell-smbios.
> > > > > > 
> > > > > > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > > > 100644
> > > > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > > > dell_quirks[] __initconst = {
> > > > > > 
> > > > > >  	{ }
> > > > > >  
> > > > > >  };
> > > > > > 
> > > > > > -static inline int dell_smi_error(int value)
> > > > > > -{
> > > > > > -	switch (value) {
> > > > > > -	case 0: /* Completed successfully */
> > > > > > -		return 0;
> > > > > > -	case -1: /* Completed with error */
> > > > > > -		return -EIO;
> > > > > > -	case -2: /* Function not supported */
> > > > > > -		return -ENXIO;
> > > > > > -	default: /* Unknown error */
> > > > > > -		return -EINVAL;
> > > > > > -	}
> > > > > > -}
> > > > > > -
> > > > > > 
> > > > > >  /*
> > > > > >  
> > > > > >   * Derived from information in smbios-wireless-ctl:
> > > > > >   *
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > > > 100644
> > > > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > > > @@ -16,6 +16,7 @@
> > > > > > 
> > > > > >  #include <linux/kernel.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/dmi.h>
> > > > > > 
> > > > > > +#include <linux/err.h>
> > > > > > 
> > > > > >  #include <linux/gfp.h>
> > > > > >  #include <linux/mutex.h>
> > > > > >  #include <linux/slab.h>
> > > > > > 
> > > > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > > > 
> > > > > >  static int da_num_tokens;
> > > > > >  static struct calling_interface_token *da_tokens;
> > > > > > 
> > > > > > +int dell_smi_error(int value)
> > > > > > +{
> > > > > > +	switch (value) {
> > > > > > +	case 0: /* Completed successfully */
> > > > > > +		return 0;
> > > > > > +	case -1: /* Completed with error */
> > > > > > +		return -EIO;
> > > > > > +	case -2: /* Function not supported */
> > > > > > +		return -ENXIO;
> > > > > > +	default: /* Unknown error */
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > > > +
> > > > > > 
> > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > > > >  {
> > > > > >  
> > > > > >  	mutex_lock(&buffer_mutex);
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > > > 100644
> > > > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > > > 
> > > > > >  	};
> > > > > >  
> > > > > >  };
> > > > > > 
> > > > > > +int dell_smi_error(int value);
> > > > > > +
> > > > > > 
> > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > > > >  void dell_smbios_clear_buffer(void);
> > > > > >  void dell_smbios_release_buffer(void);
> > > > > 
> > > > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > > > asking...
> > > > 
> > > > Well, what about it? :)  The commit message is pretty explicit in
> > > > describing what happens here, i.e. a previously static function is
> > > > moved to another module so that it can be reused.  Thus, keeping the
> > > > inline keyword makes no sense.  What exactly is your concern?
> > > 
> > > Just asking if this function should be or not be inline (of course in 
> > > header file, not in module .c).
> > 
> > If you mark a function as inline in the header file, you have to provide
> > its definition, otherwise you'll get a compilation error.  Given that
> > this is in no way performance-critical code, I see no point in
> > clobbering the header file with the body of this function.
> 
> Agreed, please leave it as is.

Pali,

Given Darren's remark, are you okay with acking this patch?

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Pali Rohár]

On Wednesday 02 March 2016 12:49:39 Michał Kępień wrote:
> > On Mon, Feb 29, 2016 at 09:41:36PM +0100, Michał Kępień wrote:
> > > > On Monday 29 February 2016 21:22:54 Michał Kępień wrote:
> > > > > > On Wednesday 24 February 2016 08:20:11 Michał Kępień wrote:
> > > > > > > The dell_smi_error() method could be used by modules other than
> > > > > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > > > > into errno values.  Thus, move it to dell-smbios.
> > > > > > > 
> > > > > > > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > > > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > > > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > > > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > > > > 100644
> > > > > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > > > > dell_quirks[] __initconst = {
> > > > > > > 
> > > > > > >  	{ }
> > > > > > >  
> > > > > > >  };
> > > > > > > 
> > > > > > > -static inline int dell_smi_error(int value)
> > > > > > > -{
> > > > > > > -	switch (value) {
> > > > > > > -	case 0: /* Completed successfully */
> > > > > > > -		return 0;
> > > > > > > -	case -1: /* Completed with error */
> > > > > > > -		return -EIO;
> > > > > > > -	case -2: /* Function not supported */
> > > > > > > -		return -ENXIO;
> > > > > > > -	default: /* Unknown error */
> > > > > > > -		return -EINVAL;
> > > > > > > -	}
> > > > > > > -}
> > > > > > > -
> > > > > > > 
> > > > > > >  /*
> > > > > > >  
> > > > > > >   * Derived from information in smbios-wireless-ctl:
> > > > > > >   *
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > > > > 100644
> > > > > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > > > > @@ -16,6 +16,7 @@
> > > > > > > 
> > > > > > >  #include <linux/kernel.h>
> > > > > > >  #include <linux/module.h>
> > > > > > >  #include <linux/dmi.h>
> > > > > > > 
> > > > > > > +#include <linux/err.h>
> > > > > > > 
> > > > > > >  #include <linux/gfp.h>
> > > > > > >  #include <linux/mutex.h>
> > > > > > >  #include <linux/slab.h>
> > > > > > > 
> > > > > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > > > > 
> > > > > > >  static int da_num_tokens;
> > > > > > >  static struct calling_interface_token *da_tokens;
> > > > > > > 
> > > > > > > +int dell_smi_error(int value)
> > > > > > > +{
> > > > > > > +	switch (value) {
> > > > > > > +	case 0: /* Completed successfully */
> > > > > > > +		return 0;
> > > > > > > +	case -1: /* Completed with error */
> > > > > > > +		return -EIO;
> > > > > > > +	case -2: /* Function not supported */
> > > > > > > +		return -ENXIO;
> > > > > > > +	default: /* Unknown error */
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > > > > +
> > > > > > > 
> > > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > > > > >  {
> > > > > > >  
> > > > > > >  	mutex_lock(&buffer_mutex);
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > > > > 100644
> > > > > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > > > > 
> > > > > > >  	};
> > > > > > >  
> > > > > > >  };
> > > > > > > 
> > > > > > > +int dell_smi_error(int value);
> > > > > > > +
> > > > > > > 
> > > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > > > > >  void dell_smbios_clear_buffer(void);
> > > > > > >  void dell_smbios_release_buffer(void);
> > > > > > 
> > > > > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > > > > asking...
> > > > > 
> > > > > Well, what about it? :)  The commit message is pretty explicit in
> > > > > describing what happens here, i.e. a previously static function is
> > > > > moved to another module so that it can be reused.  Thus, keeping the
> > > > > inline keyword makes no sense.  What exactly is your concern?
> > > > 
> > > > Just asking if this function should be or not be inline (of course in 
> > > > header file, not in module .c).
> > > 
> > > If you mark a function as inline in the header file, you have to provide
> > > its definition, otherwise you'll get a compilation error.  Given that
> > > this is in no way performance-critical code, I see no point in
> > > clobbering the header file with the body of this function.
> > 
> > Agreed, please leave it as is.
> 
> Pali,
> 
> Given Darren's remark, are you okay with acking this patch?

I'm just thinking if such function should not be macro as it translate 3
error codes from firmware to standard errno... As external function
exported by EXPORT_SYMBOL looks for me as overkill...

Patch is anyway OK, so add my Reviewed-by.

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH v4 2/5] dell-smbios: rename dell_smi_error() to dell_smbios_error()

[Michał Kępień]

As dell_smi_error() is exported by dell-smbios, its prefix should be
consistent with other exported symbols, so change function name to
dell_smbios_error().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-laptop.c |   16 ++++++++--------
 drivers/platform/x86/dell-smbios.c |    4 ++--
 drivers/platform/x86/dell-smbios.h |    2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index cbafb95..2c2f02b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -433,7 +433,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 
  out:
 	dell_smbios_release_buffer();
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 /* Must be called with the buffer held */
@@ -876,7 +876,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	else
 		dell_smbios_send_request(1, 1);
 
-	ret = dell_smi_error(buffer->output[0]);
+	ret = dell_smbios_error(buffer->output[0]);
 
 	dell_smbios_release_buffer();
 	return ret;
@@ -901,7 +901,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 		dell_smbios_send_request(0, 1);
 
 	if (buffer->output[0])
-		ret = dell_smi_error(buffer->output[0]);
+		ret = dell_smbios_error(buffer->output[0]);
 	else
 		ret = buffer->output[1];
 
@@ -1160,7 +1160,7 @@ static int kbd_get_info(struct kbd_info *info)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1249,7 +1249,7 @@ static int kbd_get_state(struct kbd_state *state)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1286,7 +1286,7 @@ static int kbd_set_state(struct kbd_state *state)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
@@ -1329,7 +1329,7 @@ static int kbd_set_token_bit(u8 bit)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_get_token_bit(u8 bit)
@@ -1354,7 +1354,7 @@ static int kbd_get_token_bit(u8 bit)
 	dell_smbios_release_buffer();
 
 	if (ret)
-		return dell_smi_error(ret);
+		return dell_smbios_error(ret);
 
 	return (val == token->value);
 }
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 942572f..d2412ab 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -40,7 +40,7 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
-int dell_smi_error(int value)
+int dell_smbios_error(int value)
 {
 	switch (value) {
 	case 0: /* Completed successfully */
@@ -53,7 +53,7 @@ int dell_smi_error(int value)
 		return -EINVAL;
 	}
 }
-EXPORT_SYMBOL_GPL(dell_smi_error);
+EXPORT_SYMBOL_GPL(dell_smbios_error);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 52febe6..ec7d40a 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,7 +35,7 @@ struct calling_interface_token {
 	};
 };
 
-int dell_smi_error(int value);
+int dell_smbios_error(int value);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
-- 
1.7.10.4

Re: [PATCH v4 2/5] dell-smbios: rename dell_smi_error() to dell_smbios_error()

[Pali Rohár]

On Wednesday 24 February 2016 08:20:12 Michał Kępień wrote:
> As dell_smi_error() is exported by dell-smbios, its prefix should be
> consistent with other exported symbols, so change function name to
> dell_smbios_error().
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>

Acked-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH v4 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

On some laptop models (e.g. Dell Vostro V131), WMI events are not
generated until a specific SMBIOS request is issued to register an event
listener [1].  As there seems to be no ACPI method or SMBIOS request to
determine without possible side effects whether a given machine needs to
issue this SMBIOS request in order to receive WMI events, DMI matching
is used to whitelist the models which need it.

[1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/Kconfig    |    1 +
 drivers/platform/x86/dell-wmi.c |   66 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 659e13b..176c666 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -123,6 +123,7 @@ config DELL_WMI
 	depends on ACPI_WMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on DELL_SMBIOS
 	select INPUT_SPARSEKMAP
 	select DMI
 	---help---
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index e38258a..65edd93 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -37,6 +37,7 @@
 #include <linux/string.h>
 #include <linux/dmi.h>
 #include <acpi/video.h>
+#include "dell-smbios.h"
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static u32 dell_wmi_interface_version;
+static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+	wmi_requires_smbios_request = 1;
+	return 1;
+}
+
+static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
+	{
+		.callback = dmi_matched,
+		.ident = "Dell Vostro V131",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+	},
+	{ }
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
@@ -597,6 +617,38 @@ static int __init dell_wmi_check_descriptor_buffer(void)
 	return 0;
 }
 
+/*
+ * According to Dell SMBIOS documentation:
+ *
+ * 17  3  Application Program Registration
+ *
+ *     cbArg1 Application ID 1 = 0x00010000
+ *     cbArg2 Application ID 2
+ *            QUICKSET/DCP = 0x51534554 "QSET"
+ *            ALS Driver   = 0x416c7353 "AlsS"
+ *            Latitude ON  = 0x4c6f6e52 "LonR"
+ *     cbArg3 Application version or revision number
+ *     cbArg4 0 = Unregister application
+ *            1 = Register application
+ *     cbRes1 Standard return codes (0, -1, -2)
+ */
+
+static int dell_wmi_events_set_enabled(bool enable)
+{
+	struct calling_interface_buffer *buffer;
+	int ret;
+
+	buffer = dell_smbios_get_buffer();
+	buffer->input[0] = 0x10000;
+	buffer->input[1] = 0x51534554;
+	buffer->input[3] = enable;
+	dell_smbios_send_request(17, 3);
+	ret = buffer->output[0];
+	dell_smbios_release_buffer();
+
+	return dell_smbios_error(ret);
+}
+
 static int __init dell_wmi_init(void)
 {
 	int err;
@@ -624,12 +676,26 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
+	dmi_check_system(dell_wmi_smbios_list);
+
+	if (wmi_requires_smbios_request) {
+		err = dell_wmi_events_set_enabled(true);
+		if (err) {
+			pr_err("Failed to enable WMI events\n");
+			wmi_remove_notify_handler(DELL_EVENT_GUID);
+			dell_wmi_input_destroy();
+			return err;
+		}
+	}
+
 	return 0;
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
+	if (wmi_requires_smbios_request)
+		dell_wmi_events_set_enabled(false);
 	wmi_remove_notify_handler(DELL_EVENT_GUID);
 	dell_wmi_input_destroy();
 }
-- 
1.7.10.4

Re: [PATCH v4 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Wednesday 24 February 2016 08:20:13 Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), WMI events are not
> generated until a specific SMBIOS request is issued to register an event
> listener [1].  As there seems to be no ACPI method or SMBIOS request to
> determine without possible side effects whether a given machine needs to
> issue this SMBIOS request in order to receive WMI events, DMI matching
> is used to whitelist the models which need it.
> 
> [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

On models on which an SMBIOS request needs to be issued in order for WMI
events to be generated, pressing the Dell Instant Launch hotkey does not
raise an i8042 interrupt - only a WMI event is generated (0xe025 on Dell
Vostro V131).  Thus, the 0xe025 event should only be ignored on machines
which do not require an SMBIOS request for enabling WMI.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-wmi.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 65edd93..ffc957b5 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -111,7 +111,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 } },
@@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key)
 	    acpi_video_handles_brightness_key_presses())
 		return;
 
+	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
+		return;
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
1.7.10.4

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Pali Rohár]

On Wednesday 24 February 2016 08:20:14 Michał Kępień wrote:
> On models on which an SMBIOS request needs to be issued in order for WMI
> events to be generated, pressing the Dell Instant Launch hotkey does not
> raise an i8042 interrupt - only a WMI event is generated (0xe025 on Dell
> Vostro V131).  Thus, the 0xe025 event should only be ignored on machines
> which do not require an SMBIOS request for enabling WMI.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/dell-wmi.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 65edd93..ffc957b5 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -111,7 +111,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 } },
> @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key)
>  	    acpi_video_handles_brightness_key_presses())
>  		return;
>  
> +	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
> +		return;
> +

Here I would rather test against reported_key, not keycode. If somebody
in future adds KEY_PROG4 for something else we will have problem...

>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>  }
>  

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 65edd93..ffc957b5 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -111,7 +111,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 } },
> > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key)
> >  	    acpi_video_handles_brightness_key_presses())
> >  		return;
> >  
> > +	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
> > +		return;
> > +
> 
> Here I would rather test against reported_key, not keycode. If somebody
> in future adds KEY_PROG4 for something else we will have problem...

As 0xe025 is currently the only event we know about that should be
ignored on some machines and processed on others, this makes sense, at
least for now.  If I change the first condition to:

    reported_key == 0xe025

will you be okay with adding your Reviewed-by for this patch?  Then, for
Darren's convenience, I could post a v5 of the whole series with the
above change and all your Acked-by and Reviewed-by tags added.

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2005 bytes --]

On Monday 29 February 2016 21:31:23 Michał Kępień wrote:
> > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > > @@ -111,7 +111,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 } },
> > > 
> > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > reported_key)
> > > 
> > >  	    acpi_video_handles_brightness_key_presses())
> > >  		
> > >  		return;
> > > 
> > > +	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
> > > +		return;
> > > +
> > 
> > Here I would rather test against reported_key, not keycode. If
> > somebody in future adds KEY_PROG4 for something else we will have
> > problem...
> 
> As 0xe025 is currently the only event we know about that should be
> ignored on some machines and processed on others, this makes sense,
> at least for now.  If I change the first condition to:
> 
>     reported_key == 0xe025

There will be need also change for 5/5 patch...

> will you be okay with adding your Reviewed-by for this patch?  Then,
> for Darren's convenience, I could post a v5 of the whole series with
> the above change and all your Acked-by and Reviewed-by tags added.

This is my suggestion as I'm thinking about future changes to this 
driver... But it is also on Darren as maintainer of platform x86 
subsystem.

Basically you do not want to check if pressed key is KEY_PROG4. But you 
want to check if dell wmi sent event 0xe025. This is what I say.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

> On Monday 29 February 2016 21:31:23 Michał Kępień wrote:
> > > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 100644
> > > > --- a/drivers/platform/x86/dell-wmi.c
> > > > +++ b/drivers/platform/x86/dell-wmi.c
> > > > @@ -111,7 +111,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 } },
> > > > 
> > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > reported_key)
> > > > 
> > > >  	    acpi_video_handles_brightness_key_presses())
> > > >  		
> > > >  		return;
> > > > 
> > > > +	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
> > > > +		return;
> > > > +
> > > 
> > > Here I would rather test against reported_key, not keycode. If
> > > somebody in future adds KEY_PROG4 for something else we will have
> > > problem...
> > 
> > As 0xe025 is currently the only event we know about that should be
> > ignored on some machines and processed on others, this makes sense,
> > at least for now.  If I change the first condition to:
> > 
> >     reported_key == 0xe025
> 
> There will be need also change for 5/5 patch...

Why?  Are you aware of any model which sends a 0xe029 WMI event _and_
generates an i8042 interrupt?  If not, WMI event 0xe029 should always be
turned into a key event, as per the keymap.

> > will you be okay with adding your Reviewed-by for this patch?  Then,
> > for Darren's convenience, I could post a v5 of the whole series with
> > the above change and all your Acked-by and Reviewed-by tags added.
> 
> This is my suggestion as I'm thinking about future changes to this 
> driver... But it is also on Darren as maintainer of platform x86 
> subsystem.
> 
> Basically you do not want to check if pressed key is KEY_PROG4. But you 
> want to check if dell wmi sent event 0xe025. This is what I say.

Well, FWIW, I agree with you and I'll be happy to make that change.  It
would make the code and the commit message more coherent.

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2060 bytes --]

On Monday 29 February 2016 21:49:27 you wrote:
> > On Monday 29 February 2016 21:31:23 Michał Kępień wrote:
> > > > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5
> > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c
> > > > > +++ b/drivers/platform/x86/dell-wmi.c
> > > > > @@ -111,7 +111,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 } },
> > > > > 
> > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > reported_key)
> > > > > 
> > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > >  		
> > > > >  		return;
> > > > > 
> > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > !wmi_requires_smbios_request) +		return;
> > > > > +
> > > > 
> > > > Here I would rather test against reported_key, not keycode. If
> > > > somebody in future adds KEY_PROG4 for something else we will
> > > > have problem...
> > > 
> > > As 0xe025 is currently the only event we know about that should
> > > be ignored on some machines and processed on others, this makes
> > > sense,
> > > 
> > > at least for now.  If I change the first condition to:
> > >     reported_key == 0xe025
> > 
> > There will be need also change for 5/5 patch...
> 
> Why?  Are you aware of any model which sends a 0xe029 WMI event _and_
> generates an i8042 interrupt?  If not, WMI event 0xe029 should always
> be turned into a key event, as per the keymap.

No, but your current patch 4/5 and 5/5 do that (because it checks 
KEY_PROG4). But if it is not needed, I'm happy because of one hook less.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Darren Hart]

On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote:
> On Monday 29 February 2016 21:49:27 you wrote:
> > > On Monday 29 February 2016 21:31:23 Michał Kępień wrote:
> > > > > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5
> > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c
> > > > > > +++ b/drivers/platform/x86/dell-wmi.c
> > > > > > @@ -111,7 +111,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 } },
> > > > > > 
> > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > > reported_key)
> > > > > > 
> > > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > > >  		
> > > > > >  		return;
> > > > > > 
> > > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > > !wmi_requires_smbios_request) +		return;
> > > > > > +
> > > > > 
> > > > > Here I would rather test against reported_key, not keycode. If
> > > > > somebody in future adds KEY_PROG4 for something else we will
> > > > > have problem...
> > > > 
> > > > As 0xe025 is currently the only event we know about that should
> > > > be ignored on some machines and processed on others, this makes
> > > > sense,
> > > > 
> > > > at least for now.  If I change the first condition to:
> > > >     reported_key == 0xe025
> > > 
> > > There will be need also change for 5/5 patch...
> > 
> > Why?  Are you aware of any model which sends a 0xe029 WMI event _and_
> > generates an i8042 interrupt?  If not, WMI event 0xe029 should always
> > be turned into a key event, as per the keymap.
> 
> No, but your current patch 4/5 and 5/5 do that (because it checks 
> KEY_PROG4). But if it is not needed, I'm happy because of one hook less.

>From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be
ignored, so the test would need to include both if it isn't using the common
keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not
the correct reading of 5/5?

> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com



-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Darren Hart]

On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote:
> On Monday 29 February 2016 21:49:27 you wrote:
> > > On Monday 29 February 2016 21:31:23 Michał Kępień wrote:
> > > > > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5
> > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c
> > > > > > +++ b/drivers/platform/x86/dell-wmi.c
> > > > > > @@ -111,7 +111,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 } },
> > > > > > 
> > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > > reported_key)
> > > > > > 
> > > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > > >  		
> > > > > >  		return;
> > > > > > 
> > > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > > !wmi_requires_smbios_request) +		return;
> > > > > > +
> > > > > 
> > > > > Here I would rather test against reported_key, not keycode. If
> > > > > somebody in future adds KEY_PROG4 for something else we will
> > > > > have problem...
> > > > 
> > > > As 0xe025 is currently the only event we know about that should
> > > > be ignored on some machines and processed on others, this makes
> > > > sense,
> > > > 
> > > > at least for now.  If I change the first condition to:
> > > >     reported_key == 0xe025
> > > 
> > > There will be need also change for 5/5 patch...
> > 
> > Why?  Are you aware of any model which sends a 0xe029 WMI event _and_
> > generates an i8042 interrupt?  If not, WMI event 0xe029 should always
> > be turned into a key event, as per the keymap.
> 
> No, but your current patch 4/5 and 5/5 do that (because it checks 
> KEY_PROG4). But if it is not needed, I'm happy because of one hook less.

From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be
ignored, so the test would need to include both if it isn't using the common
keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not
the correct reading of 5/5?

> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com



-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

> On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote:
> > On Monday 29 February 2016 21:49:27 you wrote:
> > > > On Monday 29 February 2016 21:31:23 Michał Kępień wrote:
> > > > > > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5
> > > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c
> > > > > > > +++ b/drivers/platform/x86/dell-wmi.c
> > > > > > > @@ -111,7 +111,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 } },
> > > > > > > 
> > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > > > reported_key)
> > > > > > > 
> > > > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > > > >  		
> > > > > > >  		return;
> > > > > > > 
> > > > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > > > !wmi_requires_smbios_request) +		return;
> > > > > > > +
> > > > > > 
> > > > > > Here I would rather test against reported_key, not keycode. If
> > > > > > somebody in future adds KEY_PROG4 for something else we will
> > > > > > have problem...
> > > > > 
> > > > > As 0xe025 is currently the only event we know about that should
> > > > > be ignored on some machines and processed on others, this makes
> > > > > sense,
> > > > > 
> > > > > at least for now.  If I change the first condition to:
> > > > >     reported_key == 0xe025
> > > > 
> > > > There will be need also change for 5/5 patch...
> > > 
> > > Why?  Are you aware of any model which sends a 0xe029 WMI event _and_
> > > generates an i8042 interrupt?  If not, WMI event 0xe029 should always
> > > be turned into a key event, as per the keymap.
> > 
> > No, but your current patch 4/5 and 5/5 do that (because it checks 
> > KEY_PROG4). But if it is not needed, I'm happy because of one hook less.
> 
> From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be
> ignored, so the test would need to include both if it isn't using the common
> keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not
> the correct reading of 5/5?

It's the other way round :)  Perhaps explaining the issue once again
will help.

Until this patch series, dell-wmi was only "aware" of laptops which
generate _both_ an i8042 interrupt and a WMI event when Dell Instant
Launch is pressed.  Thus, as the i8042 interrupt already caused a key
event to be generated, there was no point in generating another one for
the WMI event, hence the KE_IGNORE entry for 0xe025 in the keymap.

Enter Vostro V131, which does _not_ generate an i8042 interrupt when
Dell Instant Launch is pressed, yet still generates a WMI event.  In
other words, there was no way of generating a key event for Dell Instant
Launch on that model without changing the relevant keymap entry into a
KE_KEY one.  However, I still needed to ensure that for most machines
that WMI event would _not_ be turned into a key event.  That's why patch
4/5 changes the keymap entry to a KE_KEY one, while also adding a
conditional return to the key processing function, thus making sure that
the previous behavior (ignoring WMI event 0xe025) is preserved on most
machines.

I used KEY_PROG4 in the conditional expression because a mapped keycode
is also used in a comparison just above the code inserted by patch 4/5.
For event 0xe029, however, the conditional expression added by patch 4/5
should always evaluate to false.  Note that if we use KEY_PROG4 in the
comparison, the second condition will be false; if we use 0xe025 in the
comparison, the first condition will be false.  The latter variant
(suggested by Pali) will work fine until we hear of a model which
generates _both_ WMI event 0xe029 and an i8042 interrupt upon pressing
some hotkey.

I was hoping the commit message for patch 4/5 would be clear enough to
convey my intent, yet it seems I managed to confuse you.  Maybe that's a
sign that the commit message and/or code should be rephrased, but I
can't be a judge on this one myself.  Let me know what you think.  Pali,
if you think this is good enough as it is, please add your Reviewed-by.
If you have doubts, I'm all ears.

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Darren Hart]

On Wed, Mar 02, 2016 at 01:35:04PM +0100, Michał Kępień wrote:
> > On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote:
> > > On Monday 29 February 2016 21:49:27 you wrote:
> > > > > On Monday 29 February 2016 21:31:23 Michał Kępień wrote:

I understand this better now, thank you for the additional explanation.

> > > > > > > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5
> > > > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c
> > > > > > > > +++ b/drivers/platform/x86/dell-wmi.c
> > > > > > > > @@ -111,7 +111,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 } },
> > > > > > > > 

Your description below helped explain why the KE_KEY change was necessary, the
commit message didn't do that for me. Just explicitly stating "when there is no
i8042 interrupt, the WMI even must generate a valid KE_KEY" or something along
those lines would help.

> > > > > > > >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > > > > > > >  	
> > > > > > > >  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> > > > > > > > 
> > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > > > > reported_key)
> > > > > > > > 
> > > > > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > > > > >  		
> > > > > > > >  		return;
> > > > > > > > 
> > > > > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > > > > !wmi_requires_smbios_request) +		return;
> > > > > > > > +
> > > > > > > 
> > > > > > > Here I would rather test against reported_key, not keycode. If
> > > > > > > somebody in future adds KEY_PROG4 for something else we will
> > > > > > > have problem...

And ultimately, that is under our control. So let's just not do that :-)

A comment by the definition of KEY_PROG4 that notes it's meaning in this driver
should prevent any future attempts at overloading it and breaking this.

> > > > > > 
> > > > > > As 0xe025 is currently the only event we know about that should
> > > > > > be ignored on some machines and processed on others, this makes
> > > > > > sense,
> > > > > > 
> > > > > > at least for now.  If I change the first condition to:
> > > > > >     reported_key == 0xe025
> > > > > 
> > > > > There will be need also change for 5/5 patch...
> > > > 
> > > > Why?  Are you aware of any model which sends a 0xe029 WMI event _and_
> > > > generates an i8042 interrupt?  If not, WMI event 0xe029 should always
> > > > be turned into a key event, as per the keymap.
> > > 
> > > No, but your current patch 4/5 and 5/5 do that (because it checks 
> > > KEY_PROG4). But if it is not needed, I'm happy because of one hook less.
> > 
> > From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be
> > ignored, so the test would need to include both if it isn't using the common
> > keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not
> > the correct reading of 5/5?
> 
> It's the other way round :)  Perhaps explaining the issue once again
> will help.

Got it! Thanks! A couple of comments and I think this is a reasonable solution.


> 
> Until this patch series, dell-wmi was only "aware" of laptops which
> generate _both_ an i8042 interrupt and a WMI event when Dell Instant
> Launch is pressed.  Thus, as the i8042 interrupt already caused a key
> event to be generated, there was no point in generating another one for
> the WMI event, hence the KE_IGNORE entry for 0xe025 in the keymap.
> 
> Enter Vostro V131, which does _not_ generate an i8042 interrupt when
> Dell Instant Launch is pressed, yet still generates a WMI event.  In
> other words, there was no way of generating a key event for Dell Instant
> Launch on that model without changing the relevant keymap entry into a
> KE_KEY one.  However, I still needed to ensure that for most machines
> that WMI event would _not_ be turned into a key event.  That's why patch
> 4/5 changes the keymap entry to a KE_KEY one, while also adding a
> conditional return to the key processing function, thus making sure that
> the previous behavior (ignoring WMI event 0xe025) is preserved on most
> machines.
> 
> I used KEY_PROG4 in the conditional expression because a mapped keycode
> is also used in a comparison just above the code inserted by patch 4/5.
> For event 0xe029, however, the conditional expression added by patch 4/5
> should always evaluate to false.  Note that if we use KEY_PROG4 in the
> comparison, the second condition will be false; if we use 0xe025 in the
> comparison, the first condition will be false.  The latter variant
> (suggested by Pali) will work fine until we hear of a model which
> generates _both_ WMI event 0xe029 and an i8042 interrupt upon pressing
> some hotkey.
> 
> I was hoping the commit message for patch 4/5 would be clear enough to
> convey my intent, yet it seems I managed to confuse you.  Maybe that's a
> sign that the commit message and/or code should be rephrased, but I
> can't be a judge on this one myself.  Let me know what you think.  Pali,
> if you think this is good enough as it is, please add your Reviewed-by.
> If you have doubts, I'm all ears.
> 
> -- 
> Best regards,
> Michał Kępień
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

> Your description below helped explain why the KE_KEY change was necessary, the
> commit message didn't do that for me. Just explicitly stating "when there is no
> i8042 interrupt, the WMI even must generate a valid KE_KEY" or something along
> those lines would help.

I will do that in v5, then.

> > > > > > > > >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > > > > > > > >  	
> > > > > > > > >  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> > > > > > > > > 
> > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > > > > > reported_key)
> > > > > > > > > 
> > > > > > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > > > > > >  		
> > > > > > > > >  		return;
> > > > > > > > > 
> > > > > > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > > > > > !wmi_requires_smbios_request) +		return;
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > Here I would rather test against reported_key, not keycode. If
> > > > > > > > somebody in future adds KEY_PROG4 for something else we will
> > > > > > > > have problem...
> 
> And ultimately, that is under our control. So let's just not do that :-)
> 
> A comment by the definition of KEY_PROG4 that notes it's meaning in this driver
> should prevent any future attempts at overloading it and breaking this.

As I'll be sending a v5 anyway, do you think Pali's idea is bad?
Personally, I'm leaning towards it.  IMHO comparing against reported_key
would emphasize the fact that only event 0xe025 is "special" and chances
are that there are no other WMI event codes which need to be handled
this way.

-- 
Best regards,
Michał Kępień

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Darren Hart]

On Thu, Mar 03, 2016 at 07:46:32PM +0100, Michał Kępień wrote:
> > Your description below helped explain why the KE_KEY change was necessary, the
> > commit message didn't do that for me. Just explicitly stating "when there is no
> > i8042 interrupt, the WMI even must generate a valid KE_KEY" or something along
> > those lines would help.
> 
> I will do that in v5, then.
> 
> > > > > > > > > >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> > > > > > > > > >  	
> > > > > > > > > >  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> > > > > > > > > > 
> > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int
> > > > > > > > > > reported_key)
> > > > > > > > > > 
> > > > > > > > > >  	    acpi_video_handles_brightness_key_presses())
> > > > > > > > > >  		
> > > > > > > > > >  		return;
> > > > > > > > > > 
> > > > > > > > > > +	if (key->keycode == KEY_PROG4 &&
> > > > > > > > > > !wmi_requires_smbios_request) +		return;
> > > > > > > > > > +
> > > > > > > > > 
> > > > > > > > > Here I would rather test against reported_key, not keycode. If
> > > > > > > > > somebody in future adds KEY_PROG4 for something else we will
> > > > > > > > > have problem...
> > 
> > And ultimately, that is under our control. So let's just not do that :-)
> > 
> > A comment by the definition of KEY_PROG4 that notes it's meaning in this driver
> > should prevent any future attempts at overloading it and breaking this.
> 
> As I'll be sending a v5 anyway, do you think Pali's idea is bad?
> Personally, I'm leaning towards it.  IMHO comparing against reported_key
> would emphasize the fact that only event 0xe025 is "special" and chances
> are that there are no other WMI event codes which need to be handled
> this way.

I took another look at the discussion on 5/5 regarding event 0xe029, and after
that, no, no objection from me. Go ahead with Pali's recommendation to test
against reported_key.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Darren Hart]

On Mon, Feb 29, 2016 at 09:31:23PM +0100, Michał Kępień wrote:
> > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > > index 65edd93..ffc957b5 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > > @@ -111,7 +111,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 } },
> > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key)
> > >  	    acpi_video_handles_brightness_key_presses())
> > >  		return;
> > >  
> > > +	if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request)
> > > +		return;
> > > +
> > 
> > Here I would rather test against reported_key, not keycode. If somebody
> > in future adds KEY_PROG4 for something else we will have problem...
> 
> As 0xe025 is currently the only event we know about that should be
> ignored on some machines and processed on others, this makes sense, at
> least for now.  If I change the first condition to:
> 
>     reported_key == 0xe025
> 
> will you be okay with adding your Reviewed-by for this patch?  Then, for
> Darren's convenience, I could post a v5 of the whole series with the
> above change and all your Acked-by and Reviewed-by tags added.

Yes, please do. That way I'm sure I have the right bits.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v4 5/5] dell-wmi: support Dell Inspiron M5110

[Michał Kępień]

Similarly to Dell Vostro V131, Dell Inspiron M5110 also requires an
SMBIOS request to be issued in order for WMI events to be generated and
does not raise an i8042 interrupt when the Dell Instant Launch hotkey is
pressed.  However, the event code for that hotkey on this machine is
0xe029, so add it to the legacy keymap.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
Tested-by: Darek Stojaczyk <darek.stojaczyk@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index ffc957b5..b1479ab 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -62,6 +62,14 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 	{
 		.callback = dmi_matched,
+		.ident = "Dell Inspiron M5110",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron M5110"),
+		},
+	},
+	{
+		.callback = dmi_matched,
 		.ident = "Dell Vostro V131",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -110,8 +118,11 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 
 	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
 
-	/* Shortcut and audio panel keys */
+	/* Dell Instant Launch key */
 	{ KE_KEY, 0xe025, { KEY_PROG4 } },
+	{ KE_KEY, 0xe029, { KEY_PROG4 } },
+
+	/* Audio panel key */
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
 
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
-- 
1.7.10.4

Re: [PATCH v4 5/5] dell-wmi: support Dell Inspiron M5110

[Pali Rohár]

On Wednesday 24 February 2016 08:20:15 Michał Kępień wrote:
> Similarly to Dell Vostro V131, Dell Inspiron M5110 also requires an
> SMBIOS request to be issued in order for WMI events to be generated and
> does not raise an i8042 interrupt when the Dell Instant Launch hotkey is
> pressed.  However, the event code for that hotkey on this machine is
> 0xe029, so add it to the legacy keymap.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> Tested-by: Darek Stojaczyk <darek.stojaczyk@gmail.com>
> ---
>  drivers/platform/x86/dell-wmi.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index ffc957b5..b1479ab 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -62,6 +62,14 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
>  static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
>  	{
>  		.callback = dmi_matched,
> +		.ident = "Dell Inspiron M5110",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron M5110"),
> +		},
> +	},
> +	{
> +		.callback = dmi_matched,
>  		.ident = "Dell Vostro V131",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> @@ -110,8 +118,11 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  
>  	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
>  
> -	/* Shortcut and audio panel keys */
> +	/* Dell Instant Launch key */
>  	{ KE_KEY, 0xe025, { KEY_PROG4 } },
> +	{ KE_KEY, 0xe029, { KEY_PROG4 } },
> +
> +	/* Audio panel key */
>  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
>  
>  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },

That's better, add my Reviewed-by.

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH v5 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

[Michał Kępień]

This patch series makes use of the API exported by dell-smbios, so it
should be applied to either testing or dell-smbios.

Pali, I added your Reviewed-by/Acked-by tags to all patches except 4/5;
it is the last one awaiting your approval.

Changes from v4:

  - Filter Dell Instant Launch hotkey based on reported keycode instead
    of mapped keycode (patch 4/5)

  - Added further explanations to the commit message for patch 4/5

Changes from v3:

  - Extract code issuing the WMI-controlling SMBIOS request into a
    separate function

  - Document the SMBIOS request used

  - Split keymap section "Shortcut and audio panel keys" into two
    separate sections

Changes from v2:

  - Use a static variable instead of a quirk structure

  - Use API exported by dell-smbios to issue the SMBIOS request required
    for generating WMI events, returning with error from dell_wmi_init()
    if it fails

  - Move dell_smi_error() from dell-laptop to dell-smbios and use it to
    determine error code for returning from dell_wmi_init() when
    enabling WMI fails

  - Support Dell Inspiron M5110

Changes from v1:

  - Use DMI matching instead of a module parameter
  - Change flag name to improve readability

 drivers/platform/x86/Kconfig       |    1 +
 drivers/platform/x86/dell-laptop.c |   30 ++++---------
 drivers/platform/x86/dell-smbios.c |   16 +++++++
 drivers/platform/x86/dell-smbios.h |    2 +
 drivers/platform/x86/dell-wmi.c    |   84 +++++++++++++++++++++++++++++++++++-
 5 files changed, 109 insertions(+), 24 deletions(-)

-- 
1.7.10.4

[PATCH v5 1/5] dell-laptop: move dell_smi_error() to dell-smbios

[Michał Kępień]

The dell_smi_error() method could be used by modules other than
dell-laptop for convenient translation of SMBIOS request errors into
errno values.  Thus, move it to dell-smbios.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-laptop.c |   14 --------------
 drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 76064c8..cbafb95 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static inline int dell_smi_error(int value)
-{
-	switch (value) {
-	case 0: /* Completed successfully */
-		return 0;
-	case -1: /* Completed with error */
-		return -EIO;
-	case -2: /* Function not supported */
-		return -ENXIO;
-	default: /* Unknown error */
-		return -EINVAL;
-	}
-}
-
 /*
  * Derived from information in smbios-wireless-ctl:
  *
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 2a4992a..942572f 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -39,6 +40,21 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(dell_smi_error);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 4f69b16..52febe6 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,8 @@ struct calling_interface_token {
 	};
 };
 
+int dell_smi_error(int value);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);
-- 
1.7.10.4

[PATCH v5 2/5] dell-smbios: rename dell_smi_error() to dell_smbios_error()

[Michał Kępień]

As dell_smi_error() is exported by dell-smbios, its prefix should be
consistent with other exported symbols, so change function name to
dell_smbios_error().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
Acked-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-laptop.c |   16 ++++++++--------
 drivers/platform/x86/dell-smbios.c |    4 ++--
 drivers/platform/x86/dell-smbios.h |    2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index cbafb95..2c2f02b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -433,7 +433,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 
  out:
 	dell_smbios_release_buffer();
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 /* Must be called with the buffer held */
@@ -876,7 +876,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	else
 		dell_smbios_send_request(1, 1);
 
-	ret = dell_smi_error(buffer->output[0]);
+	ret = dell_smbios_error(buffer->output[0]);
 
 	dell_smbios_release_buffer();
 	return ret;
@@ -901,7 +901,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 		dell_smbios_send_request(0, 1);
 
 	if (buffer->output[0])
-		ret = dell_smi_error(buffer->output[0]);
+		ret = dell_smbios_error(buffer->output[0]);
 	else
 		ret = buffer->output[1];
 
@@ -1160,7 +1160,7 @@ static int kbd_get_info(struct kbd_info *info)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1249,7 +1249,7 @@ static int kbd_get_state(struct kbd_state *state)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1286,7 +1286,7 @@ static int kbd_set_state(struct kbd_state *state)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
@@ -1329,7 +1329,7 @@ static int kbd_set_token_bit(u8 bit)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_get_token_bit(u8 bit)
@@ -1354,7 +1354,7 @@ static int kbd_get_token_bit(u8 bit)
 	dell_smbios_release_buffer();
 
 	if (ret)
-		return dell_smi_error(ret);
+		return dell_smbios_error(ret);
 
 	return (val == token->value);
 }
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 942572f..d2412ab 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -40,7 +40,7 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
-int dell_smi_error(int value)
+int dell_smbios_error(int value)
 {
 	switch (value) {
 	case 0: /* Completed successfully */
@@ -53,7 +53,7 @@ int dell_smi_error(int value)
 		return -EINVAL;
 	}
 }
-EXPORT_SYMBOL_GPL(dell_smi_error);
+EXPORT_SYMBOL_GPL(dell_smbios_error);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 52febe6..ec7d40a 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,7 +35,7 @@ struct calling_interface_token {
 	};
 };
 
-int dell_smi_error(int value);
+int dell_smbios_error(int value);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
-- 
1.7.10.4

[PATCH v5 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

On some laptop models (e.g. Dell Vostro V131), WMI events are not
generated until a specific SMBIOS request is issued to register an event
listener [1].  As there seems to be no ACPI method or SMBIOS request to
determine without possible side effects whether a given machine needs to
issue this SMBIOS request in order to receive WMI events, DMI matching
is used to whitelist the models which need it.

[1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/Kconfig    |    1 +
 drivers/platform/x86/dell-wmi.c |   66 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index a65d974..ed2004b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -124,6 +124,7 @@ config DELL_WMI
 	depends on DMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on DELL_SMBIOS
 	select INPUT_SPARSEKMAP
 	---help---
 	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index e38258a..65edd93 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -37,6 +37,7 @@
 #include <linux/string.h>
 #include <linux/dmi.h>
 #include <acpi/video.h>
+#include "dell-smbios.h"
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static u32 dell_wmi_interface_version;
+static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+	wmi_requires_smbios_request = 1;
+	return 1;
+}
+
+static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
+	{
+		.callback = dmi_matched,
+		.ident = "Dell Vostro V131",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+	},
+	{ }
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
@@ -597,6 +617,38 @@ static int __init dell_wmi_check_descriptor_buffer(void)
 	return 0;
 }
 
+/*
+ * According to Dell SMBIOS documentation:
+ *
+ * 17  3  Application Program Registration
+ *
+ *     cbArg1 Application ID 1 = 0x00010000
+ *     cbArg2 Application ID 2
+ *            QUICKSET/DCP = 0x51534554 "QSET"
+ *            ALS Driver   = 0x416c7353 "AlsS"
+ *            Latitude ON  = 0x4c6f6e52 "LonR"
+ *     cbArg3 Application version or revision number
+ *     cbArg4 0 = Unregister application
+ *            1 = Register application
+ *     cbRes1 Standard return codes (0, -1, -2)
+ */
+
+static int dell_wmi_events_set_enabled(bool enable)
+{
+	struct calling_interface_buffer *buffer;
+	int ret;
+
+	buffer = dell_smbios_get_buffer();
+	buffer->input[0] = 0x10000;
+	buffer->input[1] = 0x51534554;
+	buffer->input[3] = enable;
+	dell_smbios_send_request(17, 3);
+	ret = buffer->output[0];
+	dell_smbios_release_buffer();
+
+	return dell_smbios_error(ret);
+}
+
 static int __init dell_wmi_init(void)
 {
 	int err;
@@ -624,12 +676,26 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
+	dmi_check_system(dell_wmi_smbios_list);
+
+	if (wmi_requires_smbios_request) {
+		err = dell_wmi_events_set_enabled(true);
+		if (err) {
+			pr_err("Failed to enable WMI events\n");
+			wmi_remove_notify_handler(DELL_EVENT_GUID);
+			dell_wmi_input_destroy();
+			return err;
+		}
+	}
+
 	return 0;
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
+	if (wmi_requires_smbios_request)
+		dell_wmi_events_set_enabled(false);
 	wmi_remove_notify_handler(DELL_EVENT_GUID);
 	dell_wmi_input_destroy();
 }
-- 
1.7.10.4

[PATCH v5 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Michał Kępień]

On models on which an SMBIOS request needs to be issued in order for WMI
events to be generated, pressing the Dell Instant Launch hotkey does not
raise an i8042 interrupt - only a WMI event is generated (0xe025 on Dell
Vostro V131).  As that WMI event is the only way the kernel will be
notified about pressing the Dell Instant Launch hotkey on such machines,
the relevant keymap entry has to be changed to a KE_KEY one.  However,
the same WMI event should still be ignored on machines which do not
require an SMBIOS request for enabling WMI, so filter it conditionally
in dell_wmi_process_key().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-wmi.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 65edd93..3ea959e 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -111,7 +111,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 } },
@@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key)
 	    acpi_video_handles_brightness_key_presses())
 		return;
 
+	if (reported_key == 0xe025 && !wmi_requires_smbios_request)
+		return;
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
1.7.10.4

[PATCH v5 5/5] dell-wmi: support Dell Inspiron M5110

[Michał Kępień]

Similarly to Dell Vostro V131, Dell Inspiron M5110 also requires an
SMBIOS request to be issued in order for WMI events to be generated and
does not raise an i8042 interrupt when the Dell Instant Launch hotkey is
pressed.  However, the event code for that hotkey on this machine is
0xe029, so add it to the legacy keymap.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
Tested-by: Darek Stojaczyk <darek.stojaczyk@gmail.com>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 3ea959e..15c6f11 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -62,6 +62,14 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 	{
 		.callback = dmi_matched,
+		.ident = "Dell Inspiron M5110",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron M5110"),
+		},
+	},
+	{
+		.callback = dmi_matched,
 		.ident = "Dell Vostro V131",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -110,8 +118,11 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 
 	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
 
-	/* Shortcut and audio panel keys */
+	/* Dell Instant Launch key */
 	{ KE_KEY, 0xe025, { KEY_PROG4 } },
+	{ KE_KEY, 0xe029, { KEY_PROG4 } },
+
+	/* Audio panel key */
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
 
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
-- 
1.7.10.4

Re: [PATCH v5 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

[Pali Rohár]

On Friday 04 March 2016 14:09:05 Michał Kępień wrote:
> This patch series makes use of the API exported by dell-smbios, so it
> should be applied to either testing or dell-smbios.
> 
> Pali, I added your Reviewed-by/Acked-by tags to all patches except 4/5;
> it is the last one awaiting your approval.

Darren, whole series looks good to me, add my Reviewed-by.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v5 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

[Darren Hart]

On Mon, Mar 07, 2016 at 09:27:45AM +0100, Pali Rohár wrote:
> On Friday 04 March 2016 14:09:05 Michał Kępień wrote:
> > This patch series makes use of the API exported by dell-smbios, so it
> > should be applied to either testing or dell-smbios.
> > 
> > Pali, I added your Reviewed-by/Acked-by tags to all patches except 4/5;
> > it is the last one awaiting your approval.
> 
> Darren, whole series looks good to me, add my Reviewed-by.

Queued up, thank you.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[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ń <kernel@kempniu.pl>
> > ---
> > 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ń

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Andy Lutomirski]

On Fri, Dec 4, 2015 at 4:55 AM, Michał Kępień <kernel@kempniu.pl> 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ń <kernel@kempniu.pl>
>> > ---
>> > 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

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

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ń <kernel@kempniu.pl>
> ---
> 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
pali.rohar@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > --- 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ń