All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] Add driver to force WMI Thunderbolt controller power status
@ 2017-09-08 15:23 Mario Limonciello
  2017-09-09  7:09 ` Mika Westerberg
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Mario Limonciello @ 2017-09-08 15:23 UTC (permalink / raw)
  To: dvhart
  Cc: LKML, platform-driver-x86, Richard Hughes, Yehezkel Bernat,
	Mika Westerberg, Mario Limonciello

Current implementations of Intel Thunderbolt controllers will go
into a low power mode when not in use.

Many machines containing these controllers also have a GPIO wired up
that can force the controller awake.  This is offered via a ACPI-WMI
interface intended to be manipulated by a userspace utility.

This mechanism is provided by Intel to OEMs to include in BIOS.
It uses an industry wide GUID that is populated in a separate _WDG
entry with no binary MOF.

This interface allows software such as fwupd to wake up thunderbolt
controllers to query the firmware version or flash new firmware.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
changes from v2 to v3:
 * Fix typographical error
 * Send KOBJ_CHANGE event
 
changes from v1 to v2:
 * Add ABI documentation
 * Update thunderbolt.rst
 * Remove unnecessary cast
 * Remove unneeded whitespace
 * Adjust references of "Intel Wmi thunderbolt" -> 
   "Intel WMI thunderbolt force power"

 .../testing/sysfs-platform-intel-wmi-thunderbolt   |  11 +++
 Documentation/admin-guide/thunderbolt.rst          |  15 +++
 MAINTAINERS                                        |   5 +
 drivers/platform/x86/Kconfig                       |  13 +++
 drivers/platform/x86/Makefile                      |   1 +
 drivers/platform/x86/intel-wmi-thunderbolt.c       | 101 +++++++++++++++++++++
 6 files changed, 146 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-intel-wmi-thunderbolt
 create mode 100644 drivers/platform/x86/intel-wmi-thunderbolt.c

diff --git a/Documentation/ABI/testing/sysfs-platform-intel-wmi-thunderbolt b/Documentation/ABI/testing/sysfs-platform-intel-wmi-thunderbolt
new file mode 100644
index 0000000..bf3dedb
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-intel-wmi-thunderbolt
@@ -0,0 +1,11 @@
+What:		/sys/devices/platform/<platform>/force_power
+Date:		September 2017
+KernelVersion:	4.14
+Contact:	"Mario Limonciello" <mario.limonciello@dell.com>
+Description:
+		Modify the platform force power state, influencing
+		Thunderbolt controllers to turn on or off when no
+		devices are connected (write-only)
+		There are two available states:
+		    * 0 -> Force power disabled
+		    * 1 -> Force power enabled
diff --git a/Documentation/admin-guide/thunderbolt.rst b/Documentation/admin-guide/thunderbolt.rst
index 6a4cd1f..dadcd66 100644
--- a/Documentation/admin-guide/thunderbolt.rst
+++ b/Documentation/admin-guide/thunderbolt.rst
@@ -197,3 +197,18 @@ information is missing.
 
 To recover from this mode, one needs to flash a valid NVM image to the
 host host controller in the same way it is done in the previous chapter.
+
+Forcing power
+-------------
+Many OEMs include a method that can be used to force the power of a
+thunderbolt controller to an "On" state even if nothing is connected.
+If supported by your machine this will be exposed by the WMI bus with
+a sysfs attribute called "force_power".
+
+For example the intel-wmi-thunderbolt driver exposes this attribute in:
+  /sys/devices/platform/PNP0C14:00/wmi_bus/wmi_bus-PNP0C14:00/86CCFD48-205E-4A77-9C48-2021CBEDE341/force_power
+
+  To force the power to on, write 1 to this attribute file.
+  To disable force power, write 0 to this attribute file.
+
+Note: it's currently not possible to query the force power state of a platform.
diff --git a/MAINTAINERS b/MAINTAINERS
index 1c3feff..9a6b73e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3949,6 +3949,11 @@ M:	Pali Rohár <pali.rohar@gmail.com>
 S:	Maintained
 F:	drivers/platform/x86/dell-wmi.c
 
+INTEL WMI THUNDERBOLT FORCE POWER DRIVER
+M:	Mario Limonciello <mario.limonciello@dell.com>
+S:	Maintained
+F:	drivers/platform/x86/intel-wmi-thunderbolt.c
+
 DELTA ST MEDIA DRIVER
 M:	Hugues Fruchet <hugues.fruchet@st.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 80b8795..f401ae4 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -658,6 +658,19 @@ config WMI_BMOF
 	  To compile this driver as a module, choose M here: the module will
 	  be called wmi-bmof.
 
+config INTEL_WMI_THUNDERBOLT
+	tristate "Intel WMI thunderbolt force power driver"
+	depends on ACPI_WMI
+	default ACPI_WMI
+	---help---
+	  Say Y here if you want to be able to use the WMI interface on select
+	  systems to force the power control of Intel Thunderbolt controllers.
+	  This is useful for updating the firmware when devices are not plugged
+	  into the controller.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel-wmi-thunderbolt.
+
 config MSI_WMI
 	tristate "MSI WMI extras"
 	depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 91cec17..2b315d0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PEAQ_WMI)		+= peaq-wmi.o
 obj-$(CONFIG_SURFACE3_WMI)	+= surface3-wmi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 obj-$(CONFIG_WMI_BMOF)		+= wmi-bmof.o
+obj-$(CONFIG_INTEL_WMI_THUNDERBOLT)	+= intel-wmi-thunderbolt.o
 
 # toshiba_acpi must link after wmi to ensure that wmi devices are found
 # before toshiba_acpi initializes
diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c b/drivers/platform/x86/intel-wmi-thunderbolt.c
new file mode 100644
index 0000000..32fb6cc
--- /dev/null
+++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
@@ -0,0 +1,101 @@
+/*
+ * WMI Thunderbolt driver
+ *
+ * Copyright (C) 2017 Dell Inc. All Rights Reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License version 2 as published
+ *  by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/wmi.h>
+
+#define INTEL_WMI_THUNDERBOLT_GUID "86CCFD48-205E-4A77-9C48-2021CBEDE341"
+
+static ssize_t force_power_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct acpi_buffer input;
+	acpi_status status;
+	u8 mode;
+
+	input.length = sizeof(u8);
+	input.pointer = &mode;
+	mode = hex_to_bin(buf[0]);
+	if (mode == 0 || mode == 1) {
+		status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, 0, 1,
+					     &input, NULL);
+		if (ACPI_FAILURE(status)) {
+			pr_err("intel-wmi-thunderbolt: failed setting %s\n",
+			       buf);
+			return -ENODEV;
+		}
+	} else {
+		pr_err("intel-wmi-thunderbolt: unsupported mode: %d", mode);
+	}
+	return count;
+}
+
+static DEVICE_ATTR_WO(force_power);
+
+static struct attribute *tbt_attrs[] = {
+	&dev_attr_force_power.attr,
+	NULL
+};
+
+static const struct attribute_group tbt_attribute_group = {
+	.attrs = tbt_attrs,
+};
+
+static int intel_wmi_thunderbolt_probe(struct wmi_device *wdev)
+{
+	int ret;
+
+	ret = sysfs_create_group(&wdev->dev.kobj, &tbt_attribute_group);
+	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
+	return ret;
+}
+
+static int intel_wmi_thunderbolt_remove(struct wmi_device *wdev)
+{
+	sysfs_remove_group(&wdev->dev.kobj, &tbt_attribute_group);
+	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
+	return 0;
+}
+
+static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
+	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
+	{ },
+};
+
+static struct wmi_driver intel_wmi_thunderbolt_driver = {
+	.driver = {
+		.name = "intel-wmi-thunderbolt",
+	},
+	.probe = intel_wmi_thunderbolt_probe,
+	.remove = intel_wmi_thunderbolt_remove,
+	.id_table = intel_wmi_thunderbolt_id_table,
+};
+
+module_wmi_driver(intel_wmi_thunderbolt_driver);
+
+MODULE_ALIAS("wmi:" INTEL_WMI_THUNDERBOLT_GUID);
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-08 15:23 [PATCH v3] Add driver to force WMI Thunderbolt controller power status Mario Limonciello
@ 2017-09-09  7:09 ` Mika Westerberg
  2017-09-09 18:39   ` Yehezkel Bernat
  2017-09-11 21:44 ` Darren Hart
  2017-09-13 17:20 ` Lukas Wunner
  2 siblings, 1 reply; 17+ messages in thread
From: Mika Westerberg @ 2017-09-09  7:09 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: dvhart, LKML, platform-driver-x86, Richard Hughes, Yehezkel Bernat

On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> Current implementations of Intel Thunderbolt controllers will go
> into a low power mode when not in use.
> 
> Many machines containing these controllers also have a GPIO wired up
> that can force the controller awake.  This is offered via a ACPI-WMI
> interface intended to be manipulated by a userspace utility.
> 
> This mechanism is provided by Intel to OEMs to include in BIOS.
> It uses an industry wide GUID that is populated in a separate _WDG
> entry with no binary MOF.
> 
> This interface allows software such as fwupd to wake up thunderbolt
> controllers to query the firmware version or flash new firmware.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-09  7:09 ` Mika Westerberg
@ 2017-09-09 18:39   ` Yehezkel Bernat
  0 siblings, 0 replies; 17+ messages in thread
From: Yehezkel Bernat @ 2017-09-09 18:39 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Mario Limonciello, dvhart, LKML, platform-driver-x86, Richard Hughes

(Now in plain text. Sorry about that.)

On Sat, Sep 9, 2017 at 10:09 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
>> Current implementations of Intel Thunderbolt controllers will go
>> into a low power mode when not in use.
>>
>> Many machines containing these controllers also have a GPIO wired up
>> that can force the controller awake.  This is offered via a ACPI-WMI
>> interface intended to be manipulated by a userspace utility.
>>
>> This mechanism is provided by Intel to OEMs to include in BIOS.
>> It uses an industry wide GUID that is populated in a separate _WDG
>> entry with no binary MOF.
>>
>> This interface allows software such as fwupd to wake up thunderbolt
>> controllers to query the firmware version or flash new firmware.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

FWIW,

Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-08 15:23 [PATCH v3] Add driver to force WMI Thunderbolt controller power status Mario Limonciello
  2017-09-09  7:09 ` Mika Westerberg
@ 2017-09-11 21:44 ` Darren Hart
  2017-09-12 19:19     ` Mario.Limonciello
  2017-09-13 17:20 ` Lukas Wunner
  2 siblings, 1 reply; 17+ messages in thread
From: Darren Hart @ 2017-09-11 21:44 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: LKML, platform-driver-x86, Richard Hughes, Yehezkel Bernat,
	Mika Westerberg

On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> Current implementations of Intel Thunderbolt controllers will go
> into a low power mode when not in use.
> 
> Many machines containing these controllers also have a GPIO wired up
> that can force the controller awake.  This is offered via a ACPI-WMI
> interface intended to be manipulated by a userspace utility.
> 
> This mechanism is provided by Intel to OEMs to include in BIOS.
> It uses an industry wide GUID that is populated in a separate _WDG
> entry with no binary MOF.
> 
> This interface allows software such as fwupd to wake up thunderbolt
> controllers to query the firmware version or flash new firmware.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>

Queued for testing, thanks everyone.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-11 21:44 ` Darren Hart
@ 2017-09-12 19:19     ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-12 19:19 UTC (permalink / raw)
  To: dvhart
  Cc: linux-kernel, platform-driver-x86, hughsient, yehezkelshb,
	mika.westerberg

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Monday, September 11, 2017 4:45 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Richard Hughes <hughsient@gmail.com>; Yehezkel Bernat
> <yehezkelshb@gmail.com>; Mika Westerberg <mika.westerberg@linux.intel.com>
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > Current implementations of Intel Thunderbolt controllers will go
> > into a low power mode when not in use.
> >
> > Many machines containing these controllers also have a GPIO wired up
> > that can force the controller awake.  This is offered via a ACPI-WMI
> > interface intended to be manipulated by a userspace utility.
> >
> > This mechanism is provided by Intel to OEMs to include in BIOS.
> > It uses an industry wide GUID that is populated in a separate _WDG
> > entry with no binary MOF.
> >
> > This interface allows software such as fwupd to wake up thunderbolt
> > controllers to query the firmware version or flash new firmware.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> 
> Queued for testing, thanks everyone.
> 
> --
> Darren Hart
> VMware Open Source Technology Center

Darren,

Thanks.

FYI  to those that would like to test this, the associated userspace
code that is paired with this was just merged to fwupd master.
https://github.com/hughsie/fwupd/commit/8f17e1ccf4f68b3fb7015a41acc4cbb784c1f776

It's done in a way that if another GUID ever needs to be added for force-power
it will be no changes for userspace, and if another driver is introduced it will
be minimal changes (what drivers the code matches on is hardcoded).

If you would like to experiment with it, instructions for building fwupd
are available here:
https://github.com/hughsie/fwupd/wiki/Compilation

If you find any problems, feel free to file an issue with fwupd on Github.

Thanks,

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
@ 2017-09-12 19:19     ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-12 19:19 UTC (permalink / raw)
  To: dvhart
  Cc: linux-kernel, platform-driver-x86, hughsient, yehezkelshb,
	mika.westerberg

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Monday, September 11, 2017 4:45 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Richard Hughes <hughsient@gmail.com>; Yehezkel Bernat
> <yehezkelshb@gmail.com>; Mika Westerberg <mika.westerberg@linux.intel.com>
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > Current implementations of Intel Thunderbolt controllers will go
> > into a low power mode when not in use.
> >
> > Many machines containing these controllers also have a GPIO wired up
> > that can force the controller awake.  This is offered via a ACPI-WMI
> > interface intended to be manipulated by a userspace utility.
> >
> > This mechanism is provided by Intel to OEMs to include in BIOS.
> > It uses an industry wide GUID that is populated in a separate _WDG
> > entry with no binary MOF.
> >
> > This interface allows software such as fwupd to wake up thunderbolt
> > controllers to query the firmware version or flash new firmware.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> 
> Queued for testing, thanks everyone.
> 
> --
> Darren Hart
> VMware Open Source Technology Center

Darren,

Thanks.

FYI  to those that would like to test this, the associated userspace
code that is paired with this was just merged to fwupd master.
https://github.com/hughsie/fwupd/commit/8f17e1ccf4f68b3fb7015a41acc4cbb784c1f776

It's done in a way that if another GUID ever needs to be added for force-power
it will be no changes for userspace, and if another driver is introduced it will
be minimal changes (what drivers the code matches on is hardcoded).

If you would like to experiment with it, instructions for building fwupd
are available here:
https://github.com/hughsie/fwupd/wiki/Compilation

If you find any problems, feel free to file an issue with fwupd on Github.

Thanks,

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-08 15:23 [PATCH v3] Add driver to force WMI Thunderbolt controller power status Mario Limonciello
  2017-09-09  7:09 ` Mika Westerberg
  2017-09-11 21:44 ` Darren Hart
@ 2017-09-13 17:20 ` Lukas Wunner
  2017-09-14  6:42     ` Mario.Limonciello
  2 siblings, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2017-09-13 17:20 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: dvhart, LKML, platform-driver-x86, Richard Hughes,
	Yehezkel Bernat, Mika Westerberg

Sorry, late to the party.

On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> +	mode = hex_to_bin(buf[0]);
> +	if (mode == 0 || mode == 1) {
> +		status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, 0, 1,
> +					     &input, NULL);
> +		if (ACPI_FAILURE(status)) {
> +			pr_err("intel-wmi-thunderbolt: failed setting %s\n",
> +			       buf);
> +			return -ENODEV;
> +		}
> +	} else {
> +		pr_err("intel-wmi-thunderbolt: unsupported mode: %d", mode);
> +	}
> +	return count;
> +}

Seems odd to allow user space to fill the log by writing invalid data
to sysfs, likewise that success is returned in the else case.
I'd drop both pr_err() and return -EINVAL in the else case.


> +static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
> +	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
> +	{ },
> +};

I'm not familiar with WMI, but don't you need a MODULE_DEVICE_TABLE here?
How does user space know which module to load upon receiving the uevent?

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-13 17:20 ` Lukas Wunner
@ 2017-09-14  6:42     ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-14  6:42 UTC (permalink / raw)
  To: lukas
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient,
	yehezkelshb, mika.westerberg

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Wednesday, September 13, 2017 12:20 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; LKML <linux-kernel@vger.kernel.org>; platform-driver-
> x86@vger.kernel.org; Richard Hughes <hughsient@gmail.com>; Yehezkel Bernat
> <yehezkelshb@gmail.com>; Mika Westerberg <mika.westerberg@linux.intel.com>
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> Sorry, late to the party.
> 
> On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > +	mode = hex_to_bin(buf[0]);
> > +	if (mode == 0 || mode == 1) {
> > +		status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID,
> 0, 1,
> > +					     &input, NULL);
> > +		if (ACPI_FAILURE(status)) {
> > +			pr_err("intel-wmi-thunderbolt: failed setting %s\n",
> > +			       buf);
> > +			return -ENODEV;
> > +		}
> > +	} else {
> > +		pr_err("intel-wmi-thunderbolt: unsupported mode: %d", mode);
> > +	}
> > +	return count;
> > +}
> 
> Seems odd to allow user space to fill the log by writing invalid data
> to sysfs, likewise that success is returned in the else case.
> I'd drop both pr_err() and return -EINVAL in the else case.
> 

Seems fine to me.  As Darren already queued the patch, I'll send a follow up patch 
to fix these two cases.

> 
> > +static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
> > +	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
> > +	{ },
> > +};
> 
> I'm not familiar with WMI, but don't you need a MODULE_DEVICE_TABLE here?
> How does user space know which module to load upon receiving the uevent?

Some macros for WMI bus devices.
https://github.com/torvalds/linux/blob/e0f25a3f2d052e36ff67a9b4db835c3e27e950d8/include/linux/wmi.h#L55
https://github.com/torvalds/linux/blob/master/include/linux/device.h#L1487
 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
@ 2017-09-14  6:42     ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-14  6:42 UTC (permalink / raw)
  To: lukas
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient,
	yehezkelshb, mika.westerberg

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Wednesday, September 13, 2017 12:20 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; LKML <linux-kernel@vger.kernel.org>; platform-driver-
> x86@vger.kernel.org; Richard Hughes <hughsient@gmail.com>; Yehezkel Bernat
> <yehezkelshb@gmail.com>; Mika Westerberg <mika.westerberg@linux.intel.com>
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> Sorry, late to the party.
> 
> On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > +	mode = hex_to_bin(buf[0]);
> > +	if (mode == 0 || mode == 1) {
> > +		status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID,
> 0, 1,
> > +					     &input, NULL);
> > +		if (ACPI_FAILURE(status)) {
> > +			pr_err("intel-wmi-thunderbolt: failed setting %s\n",
> > +			       buf);
> > +			return -ENODEV;
> > +		}
> > +	} else {
> > +		pr_err("intel-wmi-thunderbolt: unsupported mode: %d", mode);
> > +	}
> > +	return count;
> > +}
> 
> Seems odd to allow user space to fill the log by writing invalid data
> to sysfs, likewise that success is returned in the else case.
> I'd drop both pr_err() and return -EINVAL in the else case.
> 

Seems fine to me.  As Darren already queued the patch, I'll send a follow up patch 
to fix these two cases.

> 
> > +static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
> > +	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
> > +	{ },
> > +};
> 
> I'm not familiar with WMI, but don't you need a MODULE_DEVICE_TABLE here?
> How does user space know which module to load upon receiving the uevent?

Some macros for WMI bus devices.
https://github.com/torvalds/linux/blob/e0f25a3f2d052e36ff67a9b4db835c3e27e950d8/include/linux/wmi.h#L55
https://github.com/torvalds/linux/blob/master/include/linux/device.h#L1487
 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-14  6:42     ` Mario.Limonciello
  (?)
@ 2017-09-14  9:14     ` Lukas Wunner
  2017-09-14 14:52         ` Mario.Limonciello
  -1 siblings, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2017-09-14  9:14 UTC (permalink / raw)
  To: Mario.Limonciello
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient,
	yehezkelshb, mika.westerberg

On Thu, Sep 14, 2017 at 06:42:03AM +0000, Mario.Limonciello@dell.com wrote:
> > On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > > +static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
> > > +	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
> > > +	{ },
> > > +};
> > 
> > I'm not familiar with WMI, but don't you need a MODULE_DEVICE_TABLE here?
> > How does user space know which module to load upon receiving the uevent?
> 
> Some macros for WMI bus devices.
> https://github.com/torvalds/linux/blob/e0f25a3f2d052e36ff67a9b4db835c3e27e950d8/include/linux/wmi.h#L55
> https://github.com/torvalds/linux/blob/master/include/linux/device.h#L1487

No, the init and exit hooks defined by this macro are executed
*after* the module has been loaded.  The question was, how does
the module get loaded in the first place?

Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
For udevd to then load the module, I suspect you need to add a
MODULE_DEVICE_TABLE(wmi, ...) to your driver.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-14  9:14     ` Lukas Wunner
@ 2017-09-14 14:52         ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-14 14:52 UTC (permalink / raw)
  To: lukas
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient,
	yehezkelshb, mika.westerberg

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Thursday, September 14, 2017 4:14 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; hughsient@gmail.com; yehezkelshb@gmail.com;
> mika.westerberg@linux.intel.com
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> On Thu, Sep 14, 2017 at 06:42:03AM +0000, Mario.Limonciello@dell.com wrote:
> > > On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > > > +static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
> > > > +	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
> > > > +	{ },
> > > > +};
> > >
> > > I'm not familiar with WMI, but don't you need a MODULE_DEVICE_TABLE here?
> > > How does user space know which module to load upon receiving the uevent?
> >
> > Some macros for WMI bus devices.
> >
> https://github.com/torvalds/linux/blob/e0f25a3f2d052e36ff67a9b4db835c3e27e9
> 50d8/include/linux/wmi.h#L55
> > https://github.com/torvalds/linux/blob/master/include/linux/device.h#L1487
> 
> No, the init and exit hooks defined by this macro are executed
> *after* the module has been loaded.  The question was, how does
> the module get loaded in the first place?
> 
> Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> For udevd to then load the module, I suspect you need to add a
> MODULE_DEVICE_TABLE(wmi, ...) to your driver.

Ah, you're looking for this code from the WMI bus driver:
https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724

That happens when the bus is initialized.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
@ 2017-09-14 14:52         ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-14 14:52 UTC (permalink / raw)
  To: lukas
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient,
	yehezkelshb, mika.westerberg

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Thursday, September 14, 2017 4:14 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; hughsient@gmail.com; yehezkelshb@gmail.com;
> mika.westerberg@linux.intel.com
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> On Thu, Sep 14, 2017 at 06:42:03AM +0000, Mario.Limonciello@dell.com wrote:
> > > On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote:
> > > > +static const struct wmi_device_id intel_wmi_thunderbolt_id_table[] = {
> > > > +	{ .guid_string = INTEL_WMI_THUNDERBOLT_GUID },
> > > > +	{ },
> > > > +};
> > >
> > > I'm not familiar with WMI, but don't you need a MODULE_DEVICE_TABLE here?
> > > How does user space know which module to load upon receiving the uevent?
> >
> > Some macros for WMI bus devices.
> >
> https://github.com/torvalds/linux/blob/e0f25a3f2d052e36ff67a9b4db835c3e27e9
> 50d8/include/linux/wmi.h#L55
> > https://github.com/torvalds/linux/blob/master/include/linux/device.h#L1487
> 
> No, the init and exit hooks defined by this macro are executed
> *after* the module has been loaded.  The question was, how does
> the module get loaded in the first place?
> 
> Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> For udevd to then load the module, I suspect you need to add a
> MODULE_DEVICE_TABLE(wmi, ...) to your driver.

Ah, you're looking for this code from the WMI bus driver:
https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724

That happens when the bus is initialized.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-14 14:52         ` Mario.Limonciello
  (?)
@ 2017-09-14 14:59         ` Mika Westerberg
  2017-09-14 15:05           ` Mika Westerberg
  2017-09-15  7:44           ` Lukas Wunner
  -1 siblings, 2 replies; 17+ messages in thread
From: Mika Westerberg @ 2017-09-14 14:59 UTC (permalink / raw)
  To: Mario.Limonciello
  Cc: lukas, dvhart, linux-kernel, platform-driver-x86, hughsient, yehezkelshb

On Thu, Sep 14, 2017 at 02:52:27PM +0000, Mario.Limonciello@dell.com wrote:
> > Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> > a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> > For udevd to then load the module, I suspect you need to add a
> > MODULE_DEVICE_TABLE(wmi, ...) to your driver.
> 
> Ah, you're looking for this code from the WMI bus driver:
> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724
> 
> That happens when the bus is initialized.

That's right you get the uevent and whatnot but Lucas means that if you
don't have MODULE_DEVICE_TABLE(wmi, ...) in the driver, udev cannot load
the module automatically when the device appears.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-14 14:59         ` Mika Westerberg
@ 2017-09-14 15:05           ` Mika Westerberg
  2017-09-15  7:44           ` Lukas Wunner
  1 sibling, 0 replies; 17+ messages in thread
From: Mika Westerberg @ 2017-09-14 15:05 UTC (permalink / raw)
  To: Mario.Limonciello
  Cc: lukas, dvhart, linux-kernel, platform-driver-x86, hughsient, yehezkelshb

On Thu, Sep 14, 2017 at 05:59:19PM +0300, Mika Westerberg wrote:
> On Thu, Sep 14, 2017 at 02:52:27PM +0000, Mario.Limonciello@dell.com wrote:
> > > Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> > > a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> > > For udevd to then load the module, I suspect you need to add a
> > > MODULE_DEVICE_TABLE(wmi, ...) to your driver.
> > 
> > Ah, you're looking for this code from the WMI bus driver:
> > https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724
> > 
> > That happens when the bus is initialized.
> 
> That's right you get the uevent and whatnot but Lucas means that if you
> don't have MODULE_DEVICE_TABLE(wmi, ...) in the driver, udev cannot load
> the module automatically when the device appears.

I meant to say Lukas, not Lucas. Sorry about that.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-14 14:59         ` Mika Westerberg
  2017-09-14 15:05           ` Mika Westerberg
@ 2017-09-15  7:44           ` Lukas Wunner
  2017-09-15 16:34               ` Mario.Limonciello
  1 sibling, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2017-09-15  7:44 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Mario.Limonciello, dvhart, linux-kernel, platform-driver-x86,
	hughsient, yehezkelshb

On Thu, Sep 14, 2017 at 05:59:19PM +0300, Mika Westerberg wrote:
> On Thu, Sep 14, 2017 at 02:52:27PM +0000, Mario.Limonciello@dell.com wrote:
> > > Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> > > a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> > > For udevd to then load the module, I suspect you need to add a
> > > MODULE_DEVICE_TABLE(wmi, ...) to your driver.
> > 
> > Ah, you're looking for this code from the WMI bus driver:
> > https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724
> > 
> > That happens when the bus is initialized.
> 
> That's right you get the uevent and whatnot but Lucas means that if you
> don't have MODULE_DEVICE_TABLE(wmi, ...) in the driver, udev cannot load
> the module automatically when the device appears.

Digging a bit deeper I notice the wmi drivers seem to solve this by
directly declaring a MODULE_ALIAS(), which is also present in Mario's
driver.  Mario, have you tested if auto-loading works if compiled as
a module?  If so, sorry for the noise.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
  2017-09-15  7:44           ` Lukas Wunner
@ 2017-09-15 16:34               ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-15 16:34 UTC (permalink / raw)
  To: lukas, mika.westerberg
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient, yehezkelshb

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Friday, September 15, 2017 2:45 AM
> To: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; dvhart@infradead.org;
> linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> hughsient@gmail.com; yehezkelshb@gmail.com
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> On Thu, Sep 14, 2017 at 05:59:19PM +0300, Mika Westerberg wrote:
> > On Thu, Sep 14, 2017 at 02:52:27PM +0000, Mario.Limonciello@dell.com wrote:
> > > > Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> > > > a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> > > > For udevd to then load the module, I suspect you need to add a
> > > > MODULE_DEVICE_TABLE(wmi, ...) to your driver.
> > >
> > > Ah, you're looking for this code from the WMI bus driver:
> > >
> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724
> > >
> > > That happens when the bus is initialized.
> >
> > That's right you get the uevent and whatnot but Lucas means that if you
> > don't have MODULE_DEVICE_TABLE(wmi, ...) in the driver, udev cannot load
> > the module automatically when the device appears.
> 
> Digging a bit deeper I notice the wmi drivers seem to solve this by
> directly declaring a MODULE_ALIAS(), which is also present in Mario's
> driver.  Mario, have you tested if auto-loading works if compiled as
> a module?  If so, sorry for the noise.
> 

Yes, I had tested that and that's why I was really baffled at needing to add
MODULE_DEVICE_TABLE.  I was going to dig further into this today, but I'm
glad you figured it out.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
@ 2017-09-15 16:34               ` Mario.Limonciello
  0 siblings, 0 replies; 17+ messages in thread
From: Mario.Limonciello @ 2017-09-15 16:34 UTC (permalink / raw)
  To: lukas, mika.westerberg
  Cc: dvhart, linux-kernel, platform-driver-x86, hughsient, yehezkelshb

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Friday, September 15, 2017 2:45 AM
> To: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; dvhart@infradead.org;
> linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> hughsient@gmail.com; yehezkelshb@gmail.com
> Subject: Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power
> status
> 
> On Thu, Sep 14, 2017 at 05:59:19PM +0300, Mika Westerberg wrote:
> > On Thu, Sep 14, 2017 at 02:52:27PM +0000, Mario.Limonciello@dell.com wrote:
> > > > Looking at drivers/platform/x86/wmi.c:wmi_dev_uevent() it seems that
> > > > a modalias consisting of "wmi:" followed by the GUID is sent to udevd.
> > > > For udevd to then load the module, I suspect you need to add a
> > > > MODULE_DEVICE_TABLE(wmi, ...) to your driver.
> > >
> > > Ah, you're looking for this code from the WMI bus driver:
> > >
> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/wmi.c#L724
> > >
> > > That happens when the bus is initialized.
> >
> > That's right you get the uevent and whatnot but Lucas means that if you
> > don't have MODULE_DEVICE_TABLE(wmi, ...) in the driver, udev cannot load
> > the module automatically when the device appears.
> 
> Digging a bit deeper I notice the wmi drivers seem to solve this by
> directly declaring a MODULE_ALIAS(), which is also present in Mario's
> driver.  Mario, have you tested if auto-loading works if compiled as
> a module?  If so, sorry for the noise.
> 

Yes, I had tested that and that's why I was really baffled at needing to add
MODULE_DEVICE_TABLE.  I was going to dig further into this today, but I'm
glad you figured it out.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-09-15 16:37 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-08 15:23 [PATCH v3] Add driver to force WMI Thunderbolt controller power status Mario Limonciello
2017-09-09  7:09 ` Mika Westerberg
2017-09-09 18:39   ` Yehezkel Bernat
2017-09-11 21:44 ` Darren Hart
2017-09-12 19:19   ` Mario.Limonciello
2017-09-12 19:19     ` Mario.Limonciello
2017-09-13 17:20 ` Lukas Wunner
2017-09-14  6:42   ` Mario.Limonciello
2017-09-14  6:42     ` Mario.Limonciello
2017-09-14  9:14     ` Lukas Wunner
2017-09-14 14:52       ` Mario.Limonciello
2017-09-14 14:52         ` Mario.Limonciello
2017-09-14 14:59         ` Mika Westerberg
2017-09-14 15:05           ` Mika Westerberg
2017-09-15  7:44           ` Lukas Wunner
2017-09-15 16:34             ` Mario.Limonciello
2017-09-15 16:34               ` Mario.Limonciello

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.