[PATCH v5 0/5] dell fixes and Skylake updates

[PATCH v5 0/5] dell fixes and Skylake updates

[Andy Lutomirski]

This is the successor to v3 of the dmi walk fixes and v4 of the Skylake
stuff.  The Skylake history is in that patch's notes.

Changes from v3 of the dmi walk fix:
 - Add the new hotkey patch.
 - Add the dell-rbtn patch.
 - Fix up a commit log message.

Changes from v2:
 - Dropped the dmi_walk return value change.  I'll resend it separately.
 - Rewrite the hotkey_num change
 - Add the select DMI patch.
 - Fix patch 1 error handling.

Changes from v1:
 - Patch 1 has minor tweaks from Jean.
 - Patches 2 and 3 are new.
 
Andy Lutomirski (5):
  dell-wmi: Stop storing pointers to DMI tables
  dell-wmi, dell-laptop: select DMI
  dell-wmi: Clean up hotkey table size check
  dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)
  dell-rbtn: Add a comment about the XPS 13 9350

 drivers/platform/x86/Kconfig     |   2 +
 drivers/platform/x86/dell-rbtn.c |  15 ++++
 drivers/platform/x86/dell-wmi.c  | 154 ++++++++++++++++++++++++++++++---------
 3 files changed, 137 insertions(+), 34 deletions(-)

-- 
2.5.0

[PATCH v5 1/5] dell-wmi: Stop storing pointers to DMI tables

[Andy Lutomirski]

The dmi_walk function maps the DMI table, walks it, and unmaps it.
This means that the dell_bios_hotkey_table that find_hk_type stores
points to unmapped memory by the time it gets read.

I've been able to trigger crashes caused by the stale pointer a
couple of times, but never on a stock kernel.

Fix it by generating the keymap in the dmi_walk callback instead of
storing a pointer.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v3: None

Changes from v2:
 - dmi_walk failure is no longer fatal (Jean)

Changes from v1:
 - Rename handle_dmi_table to handle_dmi_entry (Jean)
 - Remove useless assignment to results->err (Jean)

 drivers/platform/x86/dell-wmi.c | 74 +++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 368e193c2741..d6ae69e0a787 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -120,7 +120,10 @@ struct dell_bios_hotkey_table {
 
 };
 
-static const struct dell_bios_hotkey_table *dell_bios_hotkey_table;
+struct dell_dmi_results {
+	int err;
+	struct key_entry *keymap;
+};
 
 /* Uninitialized entries here are KEY_RESERVED == 0. */
 static const u16 bios_to_linux_keycode[256] __initconst = {
@@ -337,20 +340,34 @@ static void dell_wmi_notify(u32 value, void *context)
 	kfree(obj);
 }
 
-static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
+static void __init handle_dmi_entry(const struct dmi_header *dm,
+				    void *opaque)
 {
-	int hotkey_num = (dell_bios_hotkey_table->header.length - 4) /
-				sizeof(struct dell_bios_keymap_entry);
+	struct dell_dmi_results *results = opaque;
+	struct dell_bios_hotkey_table *table;
 	struct key_entry *keymap;
-	int i;
+	int hotkey_num, i;
+
+	if (results->err || results->keymap)
+		return;		/* We already found the hotkey table. */
+
+	if (dm->type != 0xb2 || dm->length <= 6)
+		return;
+
+	table = container_of(dm, struct dell_bios_hotkey_table, header);
+
+	hotkey_num = (table->header.length - 4) /
+				sizeof(struct dell_bios_keymap_entry);
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
-	if (!keymap)
-		return NULL;
+	if (!keymap) {
+		results->err = -ENOMEM;
+		return;
+	}
 
 	for (i = 0; i < hotkey_num; i++) {
 		const struct dell_bios_keymap_entry *bios_entry =
-					&dell_bios_hotkey_table->keymap[i];
+					&table->keymap[i];
 
 		/* Uninitialized entries are 0 aka KEY_RESERVED. */
 		u16 keycode = (bios_entry->keycode <
@@ -379,11 +396,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
 
 	keymap[hotkey_num].type = KE_END;
 
-	return keymap;
+	results->keymap = keymap;
 }
 
 static int __init dell_wmi_input_setup(void)
 {
+	struct dell_dmi_results dmi_results = {};
 	int err;
 
 	dell_wmi_input_dev = input_allocate_device();
@@ -394,20 +412,31 @@ static int __init dell_wmi_input_setup(void)
 	dell_wmi_input_dev->phys = "wmi/input0";
 	dell_wmi_input_dev->id.bustype = BUS_HOST;
 
-	if (dell_new_hk_type) {
-		const struct key_entry *keymap = dell_wmi_prepare_new_keymap();
-		if (!keymap) {
-			err = -ENOMEM;
-			goto err_free_dev;
-		}
+	if (dmi_walk(handle_dmi_entry, &dmi_results)) {
+		/*
+		 * Historically, dell-wmi ignored dmi_walk errors.  A failure
+		 * is certainly surprising, but it probably just indicates
+		 * a very old laptop.
+		 */
+		pr_warn("no DMI; using the old-style hotkey interface\n");
+	}
+
+	if (dmi_results.err) {
+		err = dmi_results.err;
+		goto err_free_dev;
+	}
+
+	if (dmi_results.keymap) {
+		dell_new_hk_type = true;
 
-		err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL);
+		err = sparse_keymap_setup(dell_wmi_input_dev,
+					  dmi_results.keymap, NULL);
 
 		/*
 		 * Sparse keymap library makes a copy of keymap so we
 		 * don't need the original one that was allocated.
 		 */
-		kfree(keymap);
+		kfree(dmi_results.keymap);
 	} else {
 		err = sparse_keymap_setup(dell_wmi_input_dev,
 					  dell_wmi_legacy_keymap, NULL);
@@ -434,15 +463,6 @@ static void dell_wmi_input_destroy(void)
 	input_unregister_device(dell_wmi_input_dev);
 }
 
-static void __init find_hk_type(const struct dmi_header *dm, void *dummy)
-{
-	if (dm->type == 0xb2 && dm->length > 6) {
-		dell_new_hk_type = true;
-		dell_bios_hotkey_table =
-			container_of(dm, struct dell_bios_hotkey_table, header);
-	}
-}
-
 /*
  * Descriptor buffer is 128 byte long and contains:
  *
@@ -524,8 +544,6 @@ static int __init dell_wmi_init(void)
 	if (err)
 		return err;
 
-	dmi_walk(find_hk_type, NULL);
-
 	err = dell_wmi_input_setup();
 	if (err)
 		return err;
-- 
2.5.0

[PATCH v5 2/5] dell-wmi, dell-laptop: select DMI

[Andy Lutomirski]

dell-wmi and dell-laptop will compile but won't work right if DMI
isn't selected.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v2 and v3: None

Changes from v1:
 - New patch.

drivers/platform/x86/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3e4d9c3e83fd..659e13b1e6f0 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -112,6 +112,7 @@ config DELL_LAPTOP
 	select POWER_SUPPLY
 	select LEDS_CLASS
 	select NEW_LEDS
+	select DMI
 	default n
 	---help---
 	This driver adds support for rfkill and backlight control to Dell
@@ -123,6 +124,7 @@ config DELL_WMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
 	select INPUT_SPARSEKMAP
+	select DMI
 	---help---
 	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
 
-- 
2.5.0

[PATCH v5 3/5] dell-wmi: Clean up hotkey table size check

[Andy Lutomirski]

Checking the table for a minimum size of 7 bytes makes no sense: any valid
hotkey table has a size that's a multiple of 4.

Clean this up: replace the hardcoded header length with a sizeof and
change the check to ignore an empty hotkey table.  The only behavior
change is that a 7-byte table (which is nonsensical) will now be
treated as absent instead of as valid but empty.

Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v3: None

Changes from v2:
 - Total rewrite.

drivers/platform/x86/dell-wmi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index d6ae69e0a787..32808a463325 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -351,13 +351,24 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
 
-	if (dm->type != 0xb2 || dm->length <= 6)
+	if (dm->type != 0xb2)
 		return;
 
 	table = container_of(dm, struct dell_bios_hotkey_table, header);
 
-	hotkey_num = (table->header.length - 4) /
+	hotkey_num = (table->header.length -
+		      sizeof(struct dell_bios_hotkey_table)) /
 				sizeof(struct dell_bios_keymap_entry);
+	if (hotkey_num < 1) {
+		/*
+		 * Historically, dell-wmi would ignore a DMI entry of
+		 * fewer than 7 bytes.  Sizes between 4 and 8 bytes are
+		 * nonsensical (both the header and all entries are 4
+		 * bytes), so we approximate the old behavior by
+		 * ignoring tables with fewer than one entry.
+		 */
+		return;
+	}
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
 	if (!keymap) {
-- 
2.5.0

[PATCH v5 4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

[Andy Lutomirski]

The XPS 13 9350 sends WMI keypress events that aren't enumerated in
the DMI table.  Add a table listing them.  To avoid breaking things
that worked before, these un-enumerated hotkeys won't be used if the
DMI table maps them to something else.

FWIW, it appears that the DMI table may be a legacy thing and we
might want to rethink how we handle events in general.  As an
example, a whole lot of things map to KEY_PROG3 via the DMI table.

This doesn't send keypress events for any of the new events.  They
appear to all be handled by other means (keyboard illumination is
handled automatically and rfkill is handled by intel-hid).

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v4:
 - Update changelog.

Changes from v3:
 - Rebase in top of interface version stuff.  (This changes context
   but not any diff lines.)
Changes from v2:
 - Factor check for already-known scancodes into a helper.
 - Un-abbreviate comments.
 - Fix off-by-one.
 - Rebase on top of of dmi_walk fixes.

Changes from v1:
 - The new hotkey code matches reality better.
 - Don't send key events for the new hotkeys.

 drivers/platform/x86/dell-wmi.c | 71 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 32808a463325..e38258a82be5 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -169,6 +169,30 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
 	[255]	= KEY_PROG3,
 };
 
+/*
+ * These are applied if the 0xB2 DMI hotkey table is present and doesn't
+ * override them.
+ */
+static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
+	/* Fn-lock */
+	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
+
+	/* Change keyboard illumination */
+	{ KE_IGNORE, 0x152, { KEY_KBDILLUMTOGGLE } },
+
+	/*
+	 * Radio disable (notify only -- there is no model for which the
+	 * WMI event is supposed to trigger an action).
+	 */
+	{ KE_IGNORE, 0x153, { KEY_RFKILL } },
+
+	/* RGB keyboard backlight control */
+	{ KE_IGNORE, 0x154, { KEY_RESERVED } },
+
+	/* Stealth mode toggle */
+	{ KE_IGNORE, 0x155, { KEY_RESERVED } },
+};
+
 static struct input_dev *dell_wmi_input_dev;
 
 static void dell_wmi_process_key(int reported_key)
@@ -340,13 +364,27 @@ static void dell_wmi_notify(u32 value, void *context)
 	kfree(obj);
 }
 
+static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (keymap[i].code == scancode)
+			return true;
+
+	return false;
+}
+
 static void __init handle_dmi_entry(const struct dmi_header *dm,
+
 				    void *opaque)
+
 {
 	struct dell_dmi_results *results = opaque;
 	struct dell_bios_hotkey_table *table;
+	int hotkey_num, i, pos = 0;
 	struct key_entry *keymap;
-	int hotkey_num, i;
+	int num_bios_keys;
 
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
@@ -370,7 +408,8 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 		return;
 	}
 
-	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
+	keymap = kcalloc(hotkey_num + ARRAY_SIZE(dell_wmi_extra_keymap) + 1,
+			 sizeof(struct key_entry), GFP_KERNEL);
 	if (!keymap) {
 		results->err = -ENOMEM;
 		return;
@@ -398,14 +437,32 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 		}
 
 		if (keycode == KEY_KBDILLUMTOGGLE)
-			keymap[i].type = KE_IGNORE;
+			keymap[pos].type = KE_IGNORE;
 		else
-			keymap[i].type = KE_KEY;
-		keymap[i].code = bios_entry->scancode;
-		keymap[i].keycode = keycode;
+			keymap[pos].type = KE_KEY;
+		keymap[pos].code = bios_entry->scancode;
+		keymap[pos].keycode = keycode;
+
+		pos++;
+	}
+
+	num_bios_keys = pos;
+
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_extra_keymap); i++) {
+		const struct key_entry *entry = &dell_wmi_extra_keymap[i];
+
+		/*
+		 * Check if we've already found this scancode.  This takes
+		 * quadratic time, but it doesn't matter unless the list
+		 * of extra keys gets very long.
+		 */
+		if (!have_scancode(entry->code, keymap, num_bios_keys)) {
+			keymap[pos] = *entry;
+			pos++;
+		}
 	}
 
-	keymap[hotkey_num].type = KE_END;
+	keymap[pos].type = KE_END;
 
 	results->keymap = keymap;
 }
-- 
2.5.0

[PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Andy Lutomirski]

On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
the DSDT turns it off if a new enough _OSI is supported.  Add a
comment about why we don't bother supporting it.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
index cd410e392550..b51a2008d782 100644
--- a/drivers/platform/x86/dell-rbtn.c
+++ b/drivers/platform/x86/dell-rbtn.c
@@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
 static const struct acpi_device_id rbtn_ids[] = {
 	{ "DELRBTN", 0 },
 	{ "DELLABCE", 0 },
+
+	/*
+	 * This driver can also handle the "DELLABC6" device that
+	 * appears on the XPS 13 9350, but that device is disabled
+	 * by the DSDT unless booted with acpi_osi="!Windows 2012"
+	 * acpi_osi="!Windows 2013".  Even if we boot that and bind
+	 * the driver, we seem to have inconsistent behavior in
+	 * which NetworkManager can get out of sync with the rfkill
+	 * state.
+	 *
+	 * On the XPS 13 9350 and similar laptops, we're not supposed to
+	 * use DELLABC6 at all.  Instead, we handle the rfkill button
+	 * via the intel-hid driver.
+	 */
+
 	{ "", 0 },
 };
 
-- 
2.5.0

Re: [PATCH v5 4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

[Pali Rohár]

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

On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> +       /* Stealth mode toggle */
> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },

Hi! Just one question, what does this "Stealth mode" means and what this 
toggle key/button doing?

I would propose for Laptops manufactures to revert back normal 
nonchiclet keyboard with full 105 normal keys (with F1-F12) instead 
inventing such useless and crappy/funny names for keys/buttons on 
laptops which replace PgUP/PgDown/SysRq and etc...

-- 
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 v5 4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

[Mario Limonciello]

On 02/15/2016 11:20 AM, Pali Rohár wrote:
> On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
>> +       /* Stealth mode toggle */
>> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> Hi! Just one question, what does this "Stealth mode" means and what this 
> toggle key/button doing?
>
> I would propose for Laptops manufactures to revert back normal 
> nonchiclet keyboard with full 105 normal keys (with F1-F12) instead 
> inventing such useless and crappy/funny names for keys/buttons on 
> laptops which replace PgUP/PgDown/SysRq and etc...
>
Pali,

Stealth mode will "disable all lights and sounds".  The event is for
notification only.  The actual change is performed by a combination of
the BIOS and EC. 

There is also a BIOS setting that disables the hotkey from doing anything.

Thanks,

Re: [PATCH v5 4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

[Pali Rohár]

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

On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
> On 02/15/2016 11:20 AM, Pali Rohár wrote:
> > On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> >> +       /* Stealth mode toggle */
> >> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> > 
> > Hi! Just one question, what does this "Stealth mode" means and what
> > this toggle key/button doing?
> > 
> > I would propose for Laptops manufactures to revert back normal
> > nonchiclet keyboard with full 105 normal keys (with F1-F12) instead
> > inventing such useless and crappy/funny names for keys/buttons on
> > laptops which replace PgUP/PgDown/SysRq and etc...
> 
> Pali,
> 
> Stealth mode will "disable all lights and sounds".  The event is for
> notification only.  The actual change is performed by a combination
> of the BIOS and EC.
> 
> There is also a BIOS setting that disables the hotkey from doing
> anything.
> 
> Thanks,

Thank you! Now I can image what this line in diff means :-)

Anyway, I would propose some rule to and longer description for newly 
invented hot key events which are marked as KEY_RESERVED in kernel 
source code. Really sometimes it is hard to guess what it can means and 
constant KEY_RESERVED does not help much more.

-- 
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 v5 1/5] dell-wmi: Stop storing pointers to DMI tables

[Darren Hart]

On Mon, Feb 15, 2016 at 08:32:33AM -0800, Andy Lutomirski wrote:
> The dmi_walk function maps the DMI table, walks it, and unmaps it.
> This means that the dell_bios_hotkey_table that find_hk_type stores
> points to unmapped memory by the time it gets read.
> 
> I've been able to trigger crashes caused by the stale pointer a
> couple of times, but never on a stock kernel.
> 
> Fix it by generating the keymap in the dmi_walk callback instead of
> storing a pointer.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Changes from v3: None

Did you have any Acked-by's thus far?

> 
> Changes from v2:
>  - dmi_walk failure is no longer fatal (Jean)
> 
> Changes from v1:
>  - Rename handle_dmi_table to handle_dmi_entry (Jean)
>  - Remove useless assignment to results->err (Jean)
> 
>  drivers/platform/x86/dell-wmi.c | 74 +++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 368e193c2741..d6ae69e0a787 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c

...

> @@ -379,11 +396,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
>  
>  	keymap[hotkey_num].type = KE_END;
>  
> -	return keymap;
> +	results->keymap = keymap;

This appears to  change this function not to return anything, while the
declaration appears to still expect a "const struct key_entry *". Is my visual
diff parser broken?


-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 2/5] dell-wmi, dell-laptop: select DMI

[Darren Hart]

On Mon, Feb 15, 2016 at 08:32:34AM -0800, Andy Lutomirski wrote:
> dell-wmi and dell-laptop will compile but won't work right if DMI
> isn't selected.

Was this necessitated by patch 1/5, or was this an existing problem? If the
former, I'd prefer to just include this in 1/5. If the latter, this is fine.

> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Changes from v2 and v3: None
> 
> Changes from v1:
>  - New patch.
> 
> drivers/platform/x86/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3e4d9c3e83fd..659e13b1e6f0 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -112,6 +112,7 @@ config DELL_LAPTOP
>  	select POWER_SUPPLY
>  	select LEDS_CLASS
>  	select NEW_LEDS
> +	select DMI
>  	default n
>  	---help---
>  	This driver adds support for rfkill and backlight control to Dell
> @@ -123,6 +124,7 @@ config DELL_WMI
>  	depends on INPUT
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
>  	select INPUT_SPARSEKMAP
> +	select DMI
>  	---help---
>  	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
>  
> -- 
> 2.5.0
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 3/5] dell-wmi: Clean up hotkey table size check

[Darren Hart]

On Mon, Feb 15, 2016 at 08:32:35AM -0800, Andy Lutomirski wrote:
> Checking the table for a minimum size of 7 bytes makes no sense: any valid
> hotkey table has a size that's a multiple of 4.
> 
> Clean this up: replace the hardcoded header length with a sizeof and
> change the check to ignore an empty hotkey table.  The only behavior
> change is that a 7-byte table (which is nonsensical) will now be
> treated as absent instead of as valid but empty.
> 
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Changes from v3: None
> 
> Changes from v2:
>  - Total rewrite.
> 
> drivers/platform/x86/dell-wmi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index d6ae69e0a787..32808a463325 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -351,13 +351,24 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>  	if (results->err || results->keymap)
>  		return;		/* We already found the hotkey table. */
>  
> -	if (dm->type != 0xb2 || dm->length <= 6)
> +	if (dm->type != 0xb2)

I was wondering about this return in the previous patch, where you documented
the meaning of the return. Since we have a magic value here, adding a comment
explaning why we're returning would aid in readability and maintainability.

>  		return;
>  
>  	table = container_of(dm, struct dell_bios_hotkey_table, header);
>  
> -	hotkey_num = (table->header.length - 4) /
> +	hotkey_num = (table->header.length -
> +		      sizeof(struct dell_bios_hotkey_table)) /
>  				sizeof(struct dell_bios_keymap_entry);
> +	if (hotkey_num < 1) {
> +		/*
> +		 * Historically, dell-wmi would ignore a DMI entry of
> +		 * fewer than 7 bytes.  Sizes between 4 and 8 bytes are
> +		 * nonsensical (both the header and all entries are 4
> +		 * bytes), so we approximate the old behavior by
> +		 * ignoring tables with fewer than one entry.
> +		 */
> +		return;
> +	}
>  
>  	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
>  	if (!keymap) {
> -- 
> 2.5.0
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 0/5] dell fixes and Skylake updates

[Darren Hart]

On Mon, Feb 15, 2016 at 08:32:32AM -0800, Andy Lutomirski wrote:
> This is the successor to v3 of the dmi walk fixes and v4 of the Skylake
> stuff.  The Skylake history is in that patch's notes.

Only mostly minor comments from me and one from Pali. I've pushed this to a
dell-skylake topic branch to let 0-day beat on it for a day. Please have a look
at my review and Pali's and send any necessary updates.

Thanks Andy, appreciate your patience and time on this series.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 1/5] dell-wmi: Stop storing pointers to DMI tables

[Darren Hart]

On Tue, Feb 16, 2016 at 10:37:09PM -0800, Darren Hart wrote:
> On Mon, Feb 15, 2016 at 08:32:33AM -0800, Andy Lutomirski wrote:
> > The dmi_walk function maps the DMI table, walks it, and unmaps it.
> > This means that the dell_bios_hotkey_table that find_hk_type stores
> > points to unmapped memory by the time it gets read.
> > 
> > I've been able to trigger crashes caused by the stale pointer a
> > couple of times, but never on a stock kernel.
> > 
> > Fix it by generating the keymap in the dmi_walk callback instead of
> > storing a pointer.
> > 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> > 
> > Changes from v3: None
> 
> Did you have any Acked-by's thus far?
> 
> > 
> > Changes from v2:
> >  - dmi_walk failure is no longer fatal (Jean)
> > 
> > Changes from v1:
> >  - Rename handle_dmi_table to handle_dmi_entry (Jean)
> >  - Remove useless assignment to results->err (Jean)
> > 
> >  drivers/platform/x86/dell-wmi.c | 74 +++++++++++++++++++++++++----------------
> >  1 file changed, 46 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 368e193c2741..d6ae69e0a787 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> 
> ...
> 
> > @@ -379,11 +396,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
> >  
> >  	keymap[hotkey_num].type = KE_END;
> >  
> > -	return keymap;
> > +	results->keymap = keymap;
> 
> This appears to  change this function not to return anything, while the
> declaration appears to still expect a "const struct key_entry *". Is my visual
> diff parser broken?

Duh, nevermind. The context matching the before-renamed and new signature
function threw me initially. Apologies.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

[Darren Hart]

On Mon, Feb 15, 2016 at 06:37:16PM +0100, Pali Rohár wrote:
> On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
> > On 02/15/2016 11:20 AM, Pali Rohár wrote:
> > > On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> > >> +       /* Stealth mode toggle */
> > >> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> > > 
> > > Hi! Just one question, what does this "Stealth mode" means and what
> > > this toggle key/button doing?
> > > 
> > > I would propose for Laptops manufactures to revert back normal
> > > nonchiclet keyboard with full 105 normal keys (with F1-F12) instead
> > > inventing such useless and crappy/funny names for keys/buttons on
> > > laptops which replace PgUP/PgDown/SysRq and etc...
> > 
> > Pali,
> > 
> > Stealth mode will "disable all lights and sounds".  The event is for
> > notification only.  The actual change is performed by a combination
> > of the BIOS and EC.
> > 
> > There is also a BIOS setting that disables the hotkey from doing
> > anything.
> > 
> > Thanks,
> 
> Thank you! Now I can image what this line in diff means :-)
> 
> Anyway, I would propose some rule to and longer description for newly 
> invented hot key events which are marked as KEY_RESERVED in kernel 
> source code. Really sometimes it is hard to guess what it can means and 
> constant KEY_RESERVED does not help much more.

Since we got the update from Mario, could you include the "disable all lights
and sounds" blurb in the KEY_RESERVED comment for stealth mode as part of the
next spin.

Pali, is that your only concern with this series?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Pali Rohár]

On Monday 15 February 2016 08:32:37 Andy Lutomirski wrote:
> On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
> the DSDT turns it off if a new enough _OSI is supported.  Add a
> comment about why we don't bother supporting it.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
> index cd410e392550..b51a2008d782 100644
> --- a/drivers/platform/x86/dell-rbtn.c
> +++ b/drivers/platform/x86/dell-rbtn.c
> @@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
>  static const struct acpi_device_id rbtn_ids[] = {
>  	{ "DELRBTN", 0 },
>  	{ "DELLABCE", 0 },
> +
> +	/*
> +	 * This driver can also handle the "DELLABC6" device that
> +	 * appears on the XPS 13 9350, but that device is disabled
> +	 * by the DSDT unless booted with acpi_osi="!Windows 2012"
> +	 * acpi_osi="!Windows 2013".  Even if we boot that and bind
> +	 * the driver, we seem to have inconsistent behavior in
> +	 * which NetworkManager can get out of sync with the rfkill
> +	 * state.

Do you know reason for such behaviour? It is because event is send
duplicated (by dell-rbtn and also by intel-hid)?

> +	 * On the XPS 13 9350 and similar laptops, we're not supposed to
> +	 * use DELLABC6 at all.  Instead, we handle the rfkill button
> +	 * via the intel-hid driver.
> +	 */
> +
>  	{ "", 0 },
>  };
>  

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

Re: [PATCH v5 4/5] dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

[Pali Rohár]

On Tuesday 16 February 2016 23:29:42 Darren Hart wrote:
> On Mon, Feb 15, 2016 at 06:37:16PM +0100, Pali Rohár wrote:
> > On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
> > > On 02/15/2016 11:20 AM, Pali Rohár wrote:
> > > > On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
> > > >> +       /* Stealth mode toggle */
> > > >> +       { KE_IGNORE, 0x155, { KEY_RESERVED } },
> > > > 
> > > > Hi! Just one question, what does this "Stealth mode" means and what
> > > > this toggle key/button doing?
> > > > 
> > > > I would propose for Laptops manufactures to revert back normal
> > > > nonchiclet keyboard with full 105 normal keys (with F1-F12) instead
> > > > inventing such useless and crappy/funny names for keys/buttons on
> > > > laptops which replace PgUP/PgDown/SysRq and etc...
> > > 
> > > Pali,
> > > 
> > > Stealth mode will "disable all lights and sounds".  The event is for
> > > notification only.  The actual change is performed by a combination
> > > of the BIOS and EC.
> > > 
> > > There is also a BIOS setting that disables the hotkey from doing
> > > anything.
> > > 
> > > Thanks,
> > 
> > Thank you! Now I can image what this line in diff means :-)
> > 
> > Anyway, I would propose some rule to and longer description for newly 
> > invented hot key events which are marked as KEY_RESERVED in kernel 
> > source code. Really sometimes it is hard to guess what it can means and 
> > constant KEY_RESERVED does not help much more.
> 
> Since we got the update from Mario, could you include the "disable all lights
> and sounds" blurb in the KEY_RESERVED comment for stealth mode as part of the
> next spin.
> 
> Pali, is that your only concern with this series?

Just I would like to know reason for NetworkManager in 5/5. But patches
looks good, so you can add my Acked-by.

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

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Mario Limonciello]

On 02/17/2016 05:16 AM, Pali Rohár wrote:
> On Monday 15 February 2016 08:32:37 Andy Lutomirski wrote:
>> On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
>> the DSDT turns it off if a new enough _OSI is supported.  Add a
>> comment about why we don't bother supporting it.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
>> index cd410e392550..b51a2008d782 100644
>> --- a/drivers/platform/x86/dell-rbtn.c
>> +++ b/drivers/platform/x86/dell-rbtn.c
>> @@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
>>  static const struct acpi_device_id rbtn_ids[] = {
>>  	{ "DELRBTN", 0 },
>>  	{ "DELLABCE", 0 },
>> +
>> +	/*
>> +	 * This driver can also handle the "DELLABC6" device that
>> +	 * appears on the XPS 13 9350, but that device is disabled
>> +	 * by the DSDT unless booted with acpi_osi="!Windows 2012"
>> +	 * acpi_osi="!Windows 2013".  Even if we boot that and bind
>> +	 * the driver, we seem to have inconsistent behavior in
>> +	 * which NetworkManager can get out of sync with the rfkill
>> +	 * state.
> Do you know reason for such behaviour? It is because event is send
> duplicated (by dell-rbtn and also by intel-hid)?
DELLABC6 is a custom interface that was created solely to have airplane
mode support for Windows 7. 
For Windows 10 the proper interface is to use that which is handled by
intel-hid.  A OEM airplane mode driver is not used.

Since the kernel doesn't identify as Windows 7 it would be incorrect to
do attempt to use that interface.
>> +	 * On the XPS 13 9350 and similar laptops, we're not supposed to
>> +	 * use DELLABC6 at all.  Instead, we handle the rfkill button
>> +	 * via the intel-hid driver.
>> +	 */
>> +
>>  	{ "", 0 },
>>  };
>>  

Re: [PATCH v5 2/5] dell-wmi, dell-laptop: select DMI

[Andy Lutomirski]

On Feb 16, 2016 10:39 PM, "Darren Hart" <dvhart@infradead.org> wrote:
>
> On Mon, Feb 15, 2016 at 08:32:34AM -0800, Andy Lutomirski wrote:
> > dell-wmi and dell-laptop will compile but won't work right if DMI
> > isn't selected.
>
> Was this necessitated by patch 1/5, or was this an existing problem? If the
> former, I'd prefer to just include this in 1/5. If the latter, this is fine.

It's an existing problem, I believe.  Jean noticed when reviewing an
earlier version of patch 1 IIRC.

--Andy

Re: [PATCH v5 0/5] dell fixes and Skylake updates

[Andy Lutomirski]

On Tue, Feb 16, 2016 at 11:00 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Feb 15, 2016 at 08:32:32AM -0800, Andy Lutomirski wrote:
>> This is the successor to v3 of the dmi walk fixes and v4 of the Skylake
>> stuff.  The Skylake history is in that patch's notes.
>
> Only mostly minor comments from me and one from Pali. I've pushed this to a
> dell-skylake topic branch to let 0-day beat on it for a day. Please have a look
> at my review and Pali's and send any necessary updates.
>

It looks like all necessary updates are comment changes.  Should I
just send follow-up patches to address them?

--Andy

Re: [PATCH v5 0/5] dell fixes and Skylake updates

[Darren Hart]

On Wed, Feb 17, 2016 at 12:33:01PM -0800, Andy Lutomirski wrote:
> On Tue, Feb 16, 2016 at 11:00 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Mon, Feb 15, 2016 at 08:32:32AM -0800, Andy Lutomirski wrote:
> >> This is the successor to v3 of the dmi walk fixes and v4 of the Skylake
> >> stuff.  The Skylake history is in that patch's notes.
> >
> > Only mostly minor comments from me and one from Pali. I've pushed this to a
> > dell-skylake topic branch to let 0-day beat on it for a day. Please have a look
> > at my review and Pali's and send any necessary updates.
> >
> 
> It looks like all necessary updates are comment changes.  Should I
> just send follow-up patches to address them?

That's fine with me. I'll add Pali's Acked-by and get these into next tonight.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Pali Rohár]

On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
> 
> 
> On 02/17/2016 05:16 AM, Pali Rohár wrote:
> > On Monday 15 February 2016 08:32:37 Andy Lutomirski wrote:
> >> On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
> >> the DSDT turns it off if a new enough _OSI is supported.  Add a
> >> comment about why we don't bother supporting it.
> >>
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >> ---
> >>  drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
> >> index cd410e392550..b51a2008d782 100644
> >> --- a/drivers/platform/x86/dell-rbtn.c
> >> +++ b/drivers/platform/x86/dell-rbtn.c
> >> @@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
> >>  static const struct acpi_device_id rbtn_ids[] = {
> >>  	{ "DELRBTN", 0 },
> >>  	{ "DELLABCE", 0 },
> >> +
> >> +	/*
> >> +	 * This driver can also handle the "DELLABC6" device that
> >> +	 * appears on the XPS 13 9350, but that device is disabled
> >> +	 * by the DSDT unless booted with acpi_osi="!Windows 2012"
> >> +	 * acpi_osi="!Windows 2013".  Even if we boot that and bind
> >> +	 * the driver, we seem to have inconsistent behavior in
> >> +	 * which NetworkManager can get out of sync with the rfkill
> >> +	 * state.
> > Do you know reason for such behaviour? It is because event is send
> > duplicated (by dell-rbtn and also by intel-hid)?
> DELLABC6 is a custom interface that was created solely to have airplane
> mode support for Windows 7. 
> For Windows 10 the proper interface is to use that which is handled by
> intel-hid.  A OEM airplane mode driver is not used.
> 
> Since the kernel doesn't identify as Windows 7 it would be incorrect to
> do attempt to use that interface.

Ok, I understand. But what user can is to tell linux kernel to identify
as Windows 7.

And I would like to know reason for that inconsistent behaviour. It is
because of bug in NetworkManager or because of some hidden bug in
dell-rbnt.c or in rfkill kernel subsystem? If it is in kernel there is
really big change that it can occur also on other machines which uses
dell-rbtn and so we should fix it.

Andy, can you look at it and try identify where is the problem?

> >> +	 * On the XPS 13 9350 and similar laptops, we're not supposed to
> >> +	 * use DELLABC6 at all.  Instead, we handle the rfkill button
> >> +	 * via the intel-hid driver.
> >> +	 */
> >> +
> >>  	{ "", 0 },
> >>  };
> >>  
> 

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

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Andy Lutomirski]

On Feb 23, 2016 4:01 AM, "Pali Rohár" <pali.rohar@gmail.com> wrote:
>
> On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
> >
> >
> > On 02/17/2016 05:16 AM, Pali Rohár wrote:
> > > On Monday 15 February 2016 08:32:37 Andy Lutomirski wrote:
> > >> On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
> > >> the DSDT turns it off if a new enough _OSI is supported.  Add a
> > >> comment about why we don't bother supporting it.
> > >>
> > >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > >> ---
> > >>  drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
> > >>  1 file changed, 15 insertions(+)
> > >>
> > >> diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
> > >> index cd410e392550..b51a2008d782 100644
> > >> --- a/drivers/platform/x86/dell-rbtn.c
> > >> +++ b/drivers/platform/x86/dell-rbtn.c
> > >> @@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
> > >>  static const struct acpi_device_id rbtn_ids[] = {
> > >>    { "DELRBTN", 0 },
> > >>    { "DELLABCE", 0 },
> > >> +
> > >> +  /*
> > >> +   * This driver can also handle the "DELLABC6" device that
> > >> +   * appears on the XPS 13 9350, but that device is disabled
> > >> +   * by the DSDT unless booted with acpi_osi="!Windows 2012"
> > >> +   * acpi_osi="!Windows 2013".  Even if we boot that and bind
> > >> +   * the driver, we seem to have inconsistent behavior in
> > >> +   * which NetworkManager can get out of sync with the rfkill
> > >> +   * state.
> > > Do you know reason for such behaviour? It is because event is send
> > > duplicated (by dell-rbtn and also by intel-hid)?
> > DELLABC6 is a custom interface that was created solely to have airplane
> > mode support for Windows 7.
> > For Windows 10 the proper interface is to use that which is handled by
> > intel-hid.  A OEM airplane mode driver is not used.
> >
> > Since the kernel doesn't identify as Windows 7 it would be incorrect to
> > do attempt to use that interface.
>
> Ok, I understand. But what user can is to tell linux kernel to identify
> as Windows 7.
>
> And I would like to know reason for that inconsistent behaviour. It is
> because of bug in NetworkManager or because of some hidden bug in
> dell-rbnt.c or in rfkill kernel subsystem? If it is in kernel there is
> really big change that it can occur also on other machines which uses
> dell-rbtn and so we should fix it.
>
> Andy, can you look at it and try identify where is the problem?

I think it's straightforward.  If we identify as Windows 7 and enable
this driver then, when we press the wireless button, dell-rbtn
switches its state *and* NetworkManager gets KEY_RFKILL from intel-hid
and changes its state.  Then you can tell NetworkManager to turn wifi
on using the menu, at which point dell-rbtn is off but NM's software
state is on.  Then you press the button again, turning on dell-rbtn
but turning NM off again.  The result is that the last button press
direct work as expected.

I haven't verified that this is actually what happens, but it's
certainly the case that a button that triggers a state toggle should
only change the state by *one* mechanism.

Presumably this works on Windows 7 because either there is no
equivalent of intel-hid or because the DSDT turns it off -- I haven't
checked which is the case.

--Andy

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Mario Limonciello]

On 02/23/2016 11:35 AM, Andy Lutomirski wrote:
>
> Presumably this works on Windows 7 because either there is no
> equivalent of intel-hid or because the DSDT turns it off -- I haven't
> checked which is the case.
That's correct, the DSDT doesn't offer the ACPI device corresponding to
intel-hid for Linux in Win7 otherwise it would be a "yellow bang" in
device manager that can't be resolved.

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Pali Rohár]

On Tuesday 23 February 2016 09:35:47 Andy Lutomirski wrote:
> On Feb 23, 2016 4:01 AM, "Pali Rohár" <pali.rohar@gmail.com> wrote:
> >
> > On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
> > >
> > >
> > > On 02/17/2016 05:16 AM, Pali Rohár wrote:
> > > > On Monday 15 February 2016 08:32:37 Andy Lutomirski wrote:
> > > >> On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
> > > >> the DSDT turns it off if a new enough _OSI is supported.  Add a
> > > >> comment about why we don't bother supporting it.
> > > >>
> > > >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > > >> ---
> > > >>  drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
> > > >>  1 file changed, 15 insertions(+)
> > > >>
> > > >> diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
> > > >> index cd410e392550..b51a2008d782 100644
> > > >> --- a/drivers/platform/x86/dell-rbtn.c
> > > >> +++ b/drivers/platform/x86/dell-rbtn.c
> > > >> @@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
> > > >>  static const struct acpi_device_id rbtn_ids[] = {
> > > >>    { "DELRBTN", 0 },
> > > >>    { "DELLABCE", 0 },
> > > >> +
> > > >> +  /*
> > > >> +   * This driver can also handle the "DELLABC6" device that
> > > >> +   * appears on the XPS 13 9350, but that device is disabled
> > > >> +   * by the DSDT unless booted with acpi_osi="!Windows 2012"
> > > >> +   * acpi_osi="!Windows 2013".  Even if we boot that and bind
> > > >> +   * the driver, we seem to have inconsistent behavior in
> > > >> +   * which NetworkManager can get out of sync with the rfkill
> > > >> +   * state.
> > > > Do you know reason for such behaviour? It is because event is send
> > > > duplicated (by dell-rbtn and also by intel-hid)?
> > > DELLABC6 is a custom interface that was created solely to have airplane
> > > mode support for Windows 7.
> > > For Windows 10 the proper interface is to use that which is handled by
> > > intel-hid.  A OEM airplane mode driver is not used.
> > >
> > > Since the kernel doesn't identify as Windows 7 it would be incorrect to
> > > do attempt to use that interface.
> >
> > Ok, I understand. But what user can is to tell linux kernel to identify
> > as Windows 7.
> >
> > And I would like to know reason for that inconsistent behaviour. It is
> > because of bug in NetworkManager or because of some hidden bug in
> > dell-rbnt.c or in rfkill kernel subsystem? If it is in kernel there is
> > really big change that it can occur also on other machines which uses
> > dell-rbtn and so we should fix it.
> >
> > Andy, can you look at it and try identify where is the problem?
> 
> I think it's straightforward.  If we identify as Windows 7 and enable
> this driver then, when we press the wireless button, dell-rbtn
> switches its state *and* NetworkManager gets KEY_RFKILL from intel-hid
> and changes its state.  Then you can tell NetworkManager to turn wifi
> on using the menu, at which point dell-rbtn is off but NM's software
> state is on.  Then you press the button again, turning on dell-rbtn
> but turning NM off again.  The result is that the last button press
> direct work as expected.
> 
> I haven't verified that this is actually what happens, but it's
> certainly the case that a button that triggers a state toggle should
> only change the state by *one* mechanism.
> 
> Presumably this works on Windows 7 because either there is no
> equivalent of intel-hid or because the DSDT turns it off -- I haven't
> checked which is the case.
> 
> --Andy

Understood, this is just big API mess in Dell APCI/EC/firmware and also
our kernel implementation in (dell-(laptop|wmi|rbtn)|intel-hid) drivers
is not ideal and clean...

Darren, add my Acked-by also for this patch (if you already have not
done it).

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

Re: [PATCH v5 5/5] dell-rbtn: Add a comment about the XPS 13 9350

[Darren Hart]

On Thu, Feb 25, 2016 at 11:45:28AM +0100, Pali Rohár wrote:
> On Tuesday 23 February 2016 09:35:47 Andy Lutomirski wrote:
> > On Feb 23, 2016 4:01 AM, "Pali Rohár" <pali.rohar@gmail.com> wrote:
> > >
> > > On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
> > > >
> > > >
> > > > On 02/17/2016 05:16 AM, Pali Rohár wrote:
> > > > > On Monday 15 February 2016 08:32:37 Andy Lutomirski wrote:
> > > > >> On the XPS 13 9350, the dell-rbtn mechanism has a new device id, and
> > > > >> the DSDT turns it off if a new enough _OSI is supported.  Add a
> > > > >> comment about why we don't bother supporting it.
> > > > >>
> > > > >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > > > >> ---
> > > > >>  drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
> > > > >>  1 file changed, 15 insertions(+)
> > > > >>

> > > Andy, can you look at it and try identify where is the problem?
> > 
> > I think it's straightforward.  If we identify as Windows 7 and enable
> > this driver then, when we press the wireless button, dell-rbtn
> > switches its state *and* NetworkManager gets KEY_RFKILL from intel-hid
> > and changes its state.  Then you can tell NetworkManager to turn wifi
> > on using the menu, at which point dell-rbtn is off but NM's software
> > state is on.  Then you press the button again, turning on dell-rbtn
> > but turning NM off again.  The result is that the last button press
> > direct work as expected.
> > 
> > I haven't verified that this is actually what happens, but it's
> > certainly the case that a button that triggers a state toggle should
> > only change the state by *one* mechanism.
> > 
> > Presumably this works on Windows 7 because either there is no
> > equivalent of intel-hid or because the DSDT turns it off -- I haven't
> > checked which is the case.
> > 
> > --Andy
> 
> Understood, this is just big API mess in Dell APCI/EC/firmware and also
> our kernel implementation in (dell-(laptop|wmi|rbtn)|intel-hid) drivers
> is not ideal and clean...
> 
> Darren, add my Acked-by also for this patch (if you already have not
> done it).

This is done and in next already.

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

-- 
Darren Hart
Intel Open Source Technology Center