dell fixes and Skylake updates

PATCH In review Andy Lutomirski 5 participants 26 messages Feb 15, 2016 – Feb 26, 2016 x86 platform drivers

Share & tools

Andy Lutomirski

author

2016-02-15 16:32 UTC #0 lore raw

v5 cover dell fixes and Skylake updates

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

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-02-15 16:32 UTC #1 lore raw

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

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 <[email protected]>

---

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

drivers/platform/x86/dell-wmi.c +46 ~~-28~~

index 368e193c2741..d6ae69e0a787 100644
@@ -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;

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-02-15 16:32 UTC #4 lore raw

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

In reply to Andy Lutomirski

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

Signed-off-by Andy Lutomirski <[email protected]>

---

Changes from v2 and v3: None

Changes from v1:
- New patch.

drivers/platform/x86/Kconfig | 2 ++

 1 file changed, 2 insertions(+)

drivers/platform/x86/Kconfig +2 -0

index 3e4d9c3e83fd..659e13b1e6f0 100644
@@ -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.

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-02-15 16:32 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---

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

drivers/platform/x86/dell-wmi.c +64 -7

index 32808a463325..e38258a82be5 100644
@@ -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;
 }

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-02-15 16:32 UTC #7 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

Changes from v3: None

Changes from v2:
- Total rewrite.

drivers/platform/x86/dell-wmi.c | 15 +++++++++++++--

 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/platform/x86/dell-wmi.c +13 -2

index d6ae69e0a787..32808a463325 100644
@@ -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) {

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-02-15 16:32 UTC #15 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/platform/x86/dell-rbtn.c +15 -0

index cd410e392550..b51a2008d782 100644
@@ -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 },
 };

Signature · 1 line

2.5.0

Pali Rohár

2016-02-15 17:20 UTC #10 lore raw

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

In reply to Andy Lutomirski

drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] __initconst = { · line 193

Quoted 2 lines · On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:

drivers/platform/x86/dell-wmi.c +2 -0

+       /* 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...

Signature · 2 lines

Pali Rohár
[email protected]

Mario Limonciello

2016-02-15 17:30 UTC #11 lore raw

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

Quoted 11 lines · Pali Rohár

drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] __initconst = { · line 193
Quoted 2 lines · On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
drivers/platform/x86/dell-wmi.c +2 -0
+       /* 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,

Pali Rohár

2016-02-15 17:37 UTC #12 lore raw

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

Quoted 15 lines · On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:

Quoted 11 lines · Pali Rohár
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] __initconst = { · line 193
Quoted 2 lines · On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
drivers/platform/x86/dell-wmi.c +2 -0
+       /* 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,

Quoted 8 lines

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.

Signature · 2 lines

Pali Rohár
[email protected]

Darren Hart

2016-02-17 06:37 UTC #2 lore raw

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

In reply to Andy Lutomirski

Quoted 7 lines · 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.

Quoted 7 lines

Fix it by generating the keymap in the dmi_walk callback instead of storing a pointer.
Signed-off-by Andy Lutomirski <[email protected]>
---
Changes from v3: None

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

Quoted 15 lines

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(-)
drivers/platform/x86/dell-wmi.c +0 -0
index 368e193c2741..d6ae69e0a787 100644

...

drivers/platform/x86/dell-wmi.c static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) · line 399

Quoted 6 lines

drivers/platform/x86/dell-wmi.c +1 -1

@@ -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?

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Darren Hart

2016-02-17 06:39 UTC #5 lore raw

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

In reply to Andy Lutomirski

Quoted 2 lines · Andy Lutomirski

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.

Quoted 36 lines

Signed-off-by Andy Lutomirski <[email protected]>

---

Changes from v2 and v3: None

Changes from v1:
- New patch.

drivers/platform/x86/Kconfig | 2 ++

 1 file changed, 2 insertions(+)

drivers/platform/x86/Kconfig +2 -0

index 3e4d9c3e83fd..659e13b1e6f0 100644
@@ -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.

Signature · 1 line

2.5.0

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Darren Hart

2016-02-17 06:46 UTC #8 lore raw

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

In reply to Andy Lutomirski

Quoted 25 lines · 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 <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
Changes from v3: None
Changes from v2:
- Total rewrite.
drivers/platform/x86/dell-wmi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/platform/x86/dell-wmi.c +0 -0
index d6ae69e0a787..32808a463325 100644

drivers/platform/x86/dell-wmi.c static void __init handle_dmi_entry(const struct dmi_header *dm, · line 354

Quoted 5 lines

drivers/platform/x86/dell-wmi.c +1 -1

@@ -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.

Quoted 25 lines

drivers/platform/x86/dell-wmi.c +12 -1

 		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) {

Signature · 1 line

2.5.0

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Darren Hart

2016-02-17 07:00 UTC #23 lore raw

In reply to Andy Lutomirski

Quoted 2 lines · 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.

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.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Darren Hart

2016-02-17 07:04 UTC #3 lore raw

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

In reply to Darren Hart

Quoted 38 lines · Darren Hart

Quoted 7 lines · 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.
Quoted 7 lines
Fix it by generating the keymap in the dmi_walk callback instead of storing a pointer.
Signed-off-by Andy Lutomirski <[email protected]>
---
Changes from v3: None
Did you have any Acked-by's thus far?
Quoted 15 lines
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(-)
drivers/platform/x86/dell-wmi.c +0 -0
index 368e193c2741..d6ae69e0a787 100644
...
Quoted 2 lines
drivers/platform/x86/dell-wmi.c +0 -0
@@ -379,11 +396,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
 

Quoted 8 lines

drivers/platform/x86/dell-wmi.c static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) · line 399
Quoted 4 lines
drivers/platform/x86/dell-wmi.c +1 -1
 	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.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Darren Hart

2016-02-17 07:29 UTC #13 lore raw

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

In reply to Pali Rohár

Quoted 23 lines · Pali Rohár

Quoted 22 lines · On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
Quoted 11 lines · Pali Rohár
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] __initconst = { · line 193
Quoted 2 lines · On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
drivers/platform/x86/dell-wmi.c +2 -0
+       /* 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.

Quoted 8 lines

Quoted 1 line
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?

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Pali Rohár

2016-02-17 11:16 UTC #16 lore raw

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

In reply to Andy Lutomirski

Quoted 16 lines · 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 <[email protected]>
---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/platform/x86/dell-rbtn.c +0 -0
index cd410e392550..b51a2008d782 100644
@@ -217,6 +217,21 @@ static void rbtn_notify(struct acpi_device *device, u32 event);
 static const struct acpi_device_id rbtn_ids[] = {
 	{ "DELRBTN", 0 },

drivers/platform/x86/dell-rbtn.c static void rbtn_notify(struct acpi_device *device, u32 event); · line 228

Quoted 10 lines

drivers/platform/x86/dell-rbtn.c +9 -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)?

Quoted 8 lines

drivers/platform/x86/dell-rbtn.c +5 -0

+	 * 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 },
 };

Signature · 2 lines

Pali Rohár
[email protected]

Pali Rohár

2016-02-17 11:19 UTC #14 lore raw

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

In reply to Darren Hart

Quoted 33 lines · On Tuesday 16 February 2016 23:29:42 Darren Hart wrote:

Quoted 23 lines · Pali Rohár
Quoted 22 lines · On Monday 15 February 2016 18:26:24 Mario Limonciello wrote:
Quoted 11 lines · Pali Rohár
drivers/platform/x86/dell-wmi.c static const u16 bios_to_linux_keycode[256] __initconst = { · line 193
Quoted 2 lines · On Monday 15 February 2016 17:32:36 Andy Lutomirski wrote:
drivers/platform/x86/dell-wmi.c +2 -0
+       /* 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.
Quoted 8 lines
Quoted 1 line
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.

Quoted 5 lines

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.

Signature · 2 lines

Pali Rohár
[email protected]

Mario Limonciello

2016-02-17 13:11 UTC #17 lore raw

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

In reply to Pali Rohár

Quoted 21 lines · Pali Rohár

Quoted 20 lines · 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 <[email protected]>
---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/platform/x86/dell-rbtn.c +3 -0
index cd410e392550..b51a2008d782 100644
@@ -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

Quoted 8 lines

drivers/platform/x86/dell-rbtn.c static void rbtn_notify(struct acpi_device *device, u32 event); · line 228
Quoted 6 lines
drivers/platform/x86/dell-rbtn.c +6 -0
+	 * 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.

Quoted 8 lines

drivers/platform/x86/dell-rbtn.c +5 -0

+	 * 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 },
 };

Andy Lutomirski

2016-02-17 20:32 UTC #6 lore raw

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

In reply to Darren Hart

Quoted 7 lines · Darren Hart

Quoted 2 lines · Andy Lutomirski
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

Andy Lutomirski

2016-02-17 20:33 UTC #24 lore raw

In reply to Darren Hart

Quoted 8 lines · Darren Hart

Quoted 2 lines · 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.
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

Darren Hart

2016-02-18 05:03 UTC #25 lore raw

Quoted 5 lines · Andy Lutomirski

Quoted 4 lines · Darren Hart
Quoted 2 lines · 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.

Quoted 7 lines

Quoted 4 lines
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.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Pali Rohár

2016-02-23 12:01 UTC #18 lore raw

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

In reply to Mario Limonciello

Quoted 32 lines · On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:

Quoted 21 lines · Pali Rohár
Quoted 20 lines · 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 <[email protected]>
---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/platform/x86/dell-rbtn.c +3 -0
index cd410e392550..b51a2008d782 100644
@@ -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
Quoted 8 lines
drivers/platform/x86/dell-rbtn.c static void rbtn_notify(struct acpi_device *device, u32 event); · line 228
Quoted 6 lines
drivers/platform/x86/dell-rbtn.c +6 -0
+	 * 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)?

Quoted 7 lines

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?

Quoted 9 lines

Quoted 8 lines

drivers/platform/x86/dell-rbtn.c +5 -0

+	 * 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 },
 };

Signature · 2 lines

Pali Rohár
[email protected]

Andy Lutomirski

2016-02-23 17:36 UTC #19 lore raw

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

Quoted 45 lines · Pali Rohár

Quoted 32 lines · On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
Quoted 21 lines · Pali Rohár
Quoted 20 lines · 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 <[email protected]>
---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/platform/x86/dell-rbtn.c +3 -0
index cd410e392550..b51a2008d782 100644
@@ -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
Quoted 8 lines
drivers/platform/x86/dell-rbtn.c static void rbtn_notify(struct acpi_device *device, u32 event); · line 228
Quoted 6 lines
drivers/platform/x86/dell-rbtn.c +6 -0
+   * 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)?
Quoted 7 lines
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.

Quoted 7 lines

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

Mario Limonciello

2016-02-23 17:42 UTC #20 lore raw

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

Quoted 4 lines · Andy Lutomirski

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.

Pali Rohár

2016-02-25 10:45 UTC #21 lore raw

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

In reply to Andy Lutomirski

Quoted 67 lines · On Tuesday 23 February 2016 09:35:47 Andy Lutomirski wrote:

Quoted 45 lines · Pali Rohár
Quoted 32 lines · On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
Quoted 21 lines · Pali Rohár
Quoted 20 lines · 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 <[email protected]>
---
 drivers/platform/x86/dell-rbtn.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/platform/x86/dell-rbtn.c +3 -0
index cd410e392550..b51a2008d782 100644
@@ -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
Quoted 8 lines
drivers/platform/x86/dell-rbtn.c static void rbtn_notify(struct acpi_device *device, u32 event); · line 228
Quoted 6 lines
drivers/platform/x86/dell-rbtn.c +6 -0
+   * 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)?
Quoted 7 lines
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.
Quoted 7 lines
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.

Quoted 5 lines

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

Signature · 2 lines

Pali Rohár
[email protected]

Darren Hart

2016-02-26 20:13 UTC #22 lore raw

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

Quoted 37 lines · Pali Rohár

Quoted 36 lines · On Tuesday 23 February 2016 09:35:47 Andy Lutomirski wrote:
Quoted 10 lines · Pali Rohár
Quoted 8 lines · On Wednesday 17 February 2016 07:07:28 Mario Limonciello wrote:
Quoted 5 lines · Pali Rohár
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Signed-off-by Andy Lutomirski <[email protected]>
---
 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.

Quoted 8 lines

Quoted 1 line
--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.

Quoted 5 lines

Signature · 3 lines
-- 
Pali Rohár
[email protected]

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center