Message ID | 1504720440-24423-1-git-send-email-mario.limonciello@dell.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Darren Hart |
Headers | show |
(Resending with a fixed CC list. Sorry.) > 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 allow software such as fwupd to wake up thunderbolt > controllers to query the firmware version or flash new firmware. As this is a Thunderbolt specific function, maybe it's better to be exposed from the Thunderbolt driver? > + > +static DEVICE_ATTR_WO(force_power); > + I'm not sure what is the convention for permissions for this type of attributes but I feel like this worth being root-only writable, as it can be used to power-off the controller in the middle of a FW update, for example.
> -----Original Message----- > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > Sent: Wednesday, September 6, 2017 2:41 PM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org; platform- > driver-x86@vger.ke; dvhart@infradead.org; hughsient@gmail.com > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power > status > > > > 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 allow software such as fwupd to wake up thunderbolt > > controllers to query the firmware version or flash new firmware. > > As this is a Thunderbolt specific function, maybe it's better to be > exposed from the Thunderbolt driver? > I thought about this too, but the thunderbolt driver won't load if the controller doesn't exist in the first place, whereas this is a platform BIOS feature. I'll be interested to hear if Mika has a different perspective on if this should live in the TBT driver and the proper way to do it. > > > + > > +static DEVICE_ATTR_WO(force_power); > > + > > I'm not sure what is the convention for permissions for this type of > attributes but I feel like this worth being root-only writable, as it > can be used to power-off the controller in the middle of a FW update, > for example. Yeah I think you're right. I'll adjust it in a follow up patch if this is the correct way to go afterall. Thanks,
On Wed, Sep 06, 2017 at 07:49:32PM +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > > Sent: Wednesday, September 6, 2017 2:41 PM > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org; platform- > > driver-x86@vger.ke; dvhart@infradead.org; hughsient@gmail.com > > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power > > status > > > > > > > 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 allow software such as fwupd to wake up thunderbolt > > > controllers to query the firmware version or flash new firmware. > > > > As this is a Thunderbolt specific function, maybe it's better to be > > exposed from the Thunderbolt driver? > > > > I thought about this too, but the thunderbolt driver won't load if the > controller doesn't exist in the first place, whereas this is a platform > BIOS feature. I'll be interested to hear if Mika has a different perspective > on if this should live in the TBT driver and the proper way to do it. > The other question I had about this was if the typical use case involves the OS, or if the firmware update (for example) would be performed as part of the general platform firmware update (from the UEFI update utility). > > > > > + > > > +static DEVICE_ATTR_WO(force_power); > > > + > > > > I'm not sure what is the convention for permissions for this type of > > attributes but I feel like this worth being root-only writable, as it > > can be used to power-off the controller in the middle of a FW update, > > for example. > > Yeah I think you're right. I'll adjust it in a follow up patch if this is the > correct way to go afterall. Ahhhrg, that was something I meant to follow up on as I was discussing using the macros with Mario previously, and I forgot. Sorry about that Mario.
On Wed, 2017-09-06 at 13:09 -0700, Darren Hart wrote: > The other question I had about this was if the typical use case > involves the OS, > or if the firmware update (for example) would be performed as part of > the > general platform firmware update (from the UEFI update utility). First, there is the use-case of add-in card, where it's impossible to use UEFI-based update, as much as I understand, as the BIOS isn't expected to expose an ESRT entry for it. Even for built-in controller, my impression is that most OEMs use a FW update application (running on Windows) and are not publishing a UEFI- based solution.
> -----Original Message----- > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > Sent: Wednesday, September 6, 2017 3:27 PM > To: dvhart@infradead.org; Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org; platform- > driver-x86@vger.kernel.org; hughsient@gmail.com > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power > status > > On Wed, 2017-09-06 at 13:09 -0700, Darren Hart wrote: > > The other question I had about this was if the typical use case > > involves the OS, > > or if the firmware update (for example) would be performed as part of > > the > > general platform firmware update (from the UEFI update utility). > > First, there is the use-case of add-in card, where it's impossible to > use UEFI-based update, as much as I understand, as the BIOS isn't > expected to expose an ESRT entry for it. > > Even for built-in controller, my impression is that most OEMs use a FW > update application (running on Windows) and are not publishing a UEFI- > based solution. Yeah I'd agree with that impression. Even if an OEM does choose to publish a UEFI based solution, it's still useful to present FW information for the TBT controller in fwupd however too. Similar to how fwupd displays the information for the ME even though the ME is typically updated via UEFI.
> -----Original Message----- > From: Darren Hart [mailto:dvhart@infradead.org] > Sent: Wednesday, September 6, 2017 3:10 PM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: yehezkel.bernat@intel.com; mika.westerberg@linux.intel.com; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; > hughsient@gmail.com > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power > status > > On Wed, Sep 06, 2017 at 07:49:32PM +0000, Mario.Limonciello@dell.com wrote: > > > -----Original Message----- > > > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > > > Sent: Wednesday, September 6, 2017 2:41 PM > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org; platform- > > > driver-x86@vger.ke; dvhart@infradead.org; hughsient@gmail.com > > > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller > power > > > status > > > > > > > > > > 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 allow software such as fwupd to wake up thunderbolt > > > > controllers to query the firmware version or flash new firmware. > > > > > > As this is a Thunderbolt specific function, maybe it's better to be > > > exposed from the Thunderbolt driver? > > > > > > > I thought about this too, but the thunderbolt driver won't load if the > > controller doesn't exist in the first place, whereas this is a platform > > BIOS feature. I'll be interested to hear if Mika has a different perspective > > on if this should live in the TBT driver and the proper way to do it. > > > > The other question I had about this was if the typical use case involves the OS, > or if the firmware update (for example) would be performed as part of the > general platform firmware update (from the UEFI update utility). > > > > > > > > + > > > > +static DEVICE_ATTR_WO(force_power); > > > > + > > > > > > I'm not sure what is the convention for permissions for this type of > > > attributes but I feel like this worth being root-only writable, as it > > > can be used to power-off the controller in the middle of a FW update, > > > for example. > > > > Yeah I think you're right. I'll adjust it in a follow up patch if this is the > > correct way to go afterall. > > > Ahhhrg, that was something I meant to follow up on as I was discussing using the > macros with Mario previously, and I forgot. Sorry about that Mario. > I double checked and with the way it's done right now, permissions are: --w------- Looks like the macro DTRT without me needing to override.
On Wed, Sep 06, 2017 at 09:40:02PM +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > > Sent: Wednesday, September 6, 2017 3:27 PM > > To: dvhart@infradead.org; Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org; platform- > > driver-x86@vger.kernel.org; hughsient@gmail.com > > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power > > status > > > > On Wed, 2017-09-06 at 13:09 -0700, Darren Hart wrote: > > > The other question I had about this was if the typical use case > > > involves the OS, > > > or if the firmware update (for example) would be performed as part of > > > the > > > general platform firmware update (from the UEFI update utility). > > > > First, there is the use-case of add-in card, where it's impossible to > > use UEFI-based update, as much as I understand, as the BIOS isn't > > expected to expose an ESRT entry for it. > > > > Even for built-in controller, my impression is that most OEMs use a FW > > update application (running on Windows) and are not publishing a UEFI- > > based solution. > > Yeah I'd agree with that impression. > > Even if an OEM does choose to publish a UEFI based solution, it's still > useful to present FW information for the TBT controller in fwupd however too. > > Similar to how fwupd displays the information for the ME even though > the ME is typically updated via UEFI. So this raises the question: can we come up with a mechanism as part of the tb driver that will work on both on-board controllers and add on cards? In it's current form, this driver will only address on-board controllers. The TB driver could use the WMI method if it exists, or some other method to power it up, but present the same sysfs interface to userspace...
On Wed, 2017-09-06 at 15:27 -0700, Darren Hart wrote: > On Wed, Sep 06, 2017 at 09:40:02PM +0000, Mario.Limonciello@dell.com > wrote: > > > > > > > > -----Original Message----- > > > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > > > Sent: Wednesday, September 6, 2017 3:27 PM > > > To: dvhart@infradead.org; Limonciello, Mario <Mario_Limonciello@D > > > ell.com> > > > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org > > > ; platform- > > > driver-x86@vger.kernel.org; hughsient@gmail.com > > > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt > > > controller power > > > status > > > > > > On Wed, 2017-09-06 at 13:09 -0700, Darren Hart wrote: > > > > > > > > The other question I had about this was if the typical use case > > > > involves the OS, > > > > or if the firmware update (for example) would be performed as > > > > part of > > > > the > > > > general platform firmware update (from the UEFI update > > > > utility). > > > First, there is the use-case of add-in card, where it's > > > impossible to > > > use UEFI-based update, as much as I understand, as the BIOS isn't > > > expected to expose an ESRT entry for it. > > > > > > Even for built-in controller, my impression is that most OEMs use > > > a FW > > > update application (running on Windows) and are not publishing a > > > UEFI- > > > based solution. > > Yeah I'd agree with that impression. > > > > Even if an OEM does choose to publish a UEFI based solution, it's > > still > > useful to present FW information for the TBT controller in fwupd > > however too. > > > > Similar to how fwupd displays the information for the ME even > > though > > the ME is typically updated via UEFI. > So this raises the question: can we come up with a mechanism as part > of the tb > driver that will work on both on-board controllers and add on cards? > In it's > current form, this driver will only address on-board controllers. Both this wmi driver and Thunderbolt driver are relevant for both on- board controllers and add-in cards. Maybe I'm missing something. Would you mind to elaborate? > > The TB driver could use the WMI method if it exists, or some other > method to > power it up, but present the same sysfs interface to userspace... >
> -----Original Message----- > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > Sent: Wednesday, September 6, 2017 5:34 PM > To: dvhart@infradead.org; Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org; platform- > driver-x86@vger.kernel.org; hughsient@gmail.com > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power > status > > On Wed, 2017-09-06 at 15:27 -0700, Darren Hart wrote: > > On Wed, Sep 06, 2017 at 09:40:02PM +0000, Mario.Limonciello@dell.com > > wrote: > > > > > > > > > > > -----Original Message----- > > > > From: Bernat, Yehezkel [mailto:yehezkel.bernat@intel.com] > > > > Sent: Wednesday, September 6, 2017 3:27 PM > > > > To: dvhart@infradead.org; Limonciello, Mario <Mario_Limonciello@D > > > > ell.com> > > > > Cc: mika.westerberg@linux.intel.com; linux-kernel@vger.kernel.org > > > > ; platform- > > > > driver-x86@vger.kernel.org; hughsient@gmail.com > > > > Subject: Re: Fwd: [PATCH] Add driver to force WMI Thunderbolt > > > > controller power > > > > status > > > > > > > > On Wed, 2017-09-06 at 13:09 -0700, Darren Hart wrote: > > > > > > > > > > The other question I had about this was if the typical use case > > > > > involves the OS, > > > > > or if the firmware update (for example) would be performed as > > > > > part of > > > > > the > > > > > general platform firmware update (from the UEFI update > > > > > utility). > > > > First, there is the use-case of add-in card, where it's > > > > impossible to > > > > use UEFI-based update, as much as I understand, as the BIOS isn't > > > > expected to expose an ESRT entry for it. > > > > > > > > Even for built-in controller, my impression is that most OEMs use > > > > a FW > > > > update application (running on Windows) and are not publishing a > > > > UEFI- > > > > based solution. > > > Yeah I'd agree with that impression. > > > > > > Even if an OEM does choose to publish a UEFI based solution, it's > > > still > > > useful to present FW information for the TBT controller in fwupd > > > however too. > > > > > > Similar to how fwupd displays the information for the ME even > > > though > > > the ME is typically updated via UEFI. > > So this raises the question: can we come up with a mechanism as part > > of the tb > > driver that will work on both on-board controllers and add on cards? > > In it's > > current form, this driver will only address on-board controllers. > > Both this wmi driver and Thunderbolt driver are relevant for both on- > board controllers and add-in cards. > Maybe I'm missing something. Would you mind to elaborate? > What this WMI driver I submitted does is modifies a "platform" feature (a GPIO) that turns on the on-board controller to a forced power on state. It typically shouldn't remain in this state if not in use as that will waste power. If a separate TBT device is plugged in that would cause the TBT controller to also wake up. In that case you wouldn't need to use this WMI driver. That’s why I say this should really be a platform feature that makes the thunderbolt host controller behave as expected whether something is plugged in or not when queried from fwupd. From the userspace fwupd perspective the (unwritten patch) behavior would be: 1) fwupd starts up and does coldplug routine 2a) If any TBT devices are plugged in, enumerate everything up the chain from udev, don't use force power 2b) If no TBT devices are plugged in but the force power sysfs file is available, try to write 1 to force power the device 3) wait a few seconds. 4) If new devices show up after the waiting, enumerate. 5) Write 0 to the force power to turn off the TBT host device if nothing is plugged in > > > > The TB driver could use the WMI method if it exists, or some other > > method to > > power it up, but present the same sysfs interface to userspace... > >
On Wed, Sep 06, 2017 at 07:49:32PM +0000, Mario.Limonciello@dell.com wrote: > > As this is a Thunderbolt specific function, maybe it's better to be > > exposed from the Thunderbolt driver? > > I thought about this too, but the thunderbolt driver won't load if the > controller doesn't exist in the first place, whereas this is a platform > BIOS feature. I'll be interested to hear if Mika has a different perspective > on if this should live in the TBT driver and the proper way to do it. I think it makes sense to keep it separate from the Thunderbolt driver.
On Wed, Sep 06, 2017 at 12:54:00PM -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 allow 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> > --- > MAINTAINERS | 5 ++ > drivers/platform/x86/Kconfig | 13 ++++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/intel-wmi-thunderbolt.c | 97 ++++++++++++++++++++++++++++ > 4 files changed, 116 insertions(+) > create mode 100644 drivers/platform/x86/intel-wmi-thunderbolt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 1c3feff..375bea9 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 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..6670a8d 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 driver" "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..98f60f2 > --- /dev/null > +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c > @@ -0,0 +1,97 @@ > +/* > + * 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" > + Remove extra newline. > + > +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 = (acpi_size) (sizeof(u8)); Is this cast really needed? > + 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[] = { Can this be const? > + &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) > +{ > + return sysfs_create_group(&wdev->dev.kobj, &tbt_attribute_group); You need to document this in Documentation/ABI. While there, I think it make sense to update Documentation/admin-guide/thunderbolt.rst accordingly. Also you are adding an attribute to a device that is already announced to the userspace (I think). So it is possible userspace does not find this when it deals with the uevent. Not sure if it is really a problem in this case, though. Other than that, looks nice to me :)
> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Thursday, September 7, 2017 1:50 AM > 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> > Subject: Re: [PATCH] Add driver to force WMI Thunderbolt controller power status > > On Wed, Sep 06, 2017 at 12:54:00PM -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 allow 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> > > --- > > MAINTAINERS | 5 ++ > > drivers/platform/x86/Kconfig | 13 ++++ > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/intel-wmi-thunderbolt.c | 97 > ++++++++++++++++++++++++++++ > > 4 files changed, 116 insertions(+) > > create mode 100644 drivers/platform/x86/intel-wmi-thunderbolt.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 1c3feff..375bea9 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 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..6670a8d 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 driver" > > "Intel WMI Thunderbolt force power driver" Adjusted. > > > + 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..98f60f2 > > --- /dev/null > > +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c > > @@ -0,0 +1,97 @@ > > +/* > > + * 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" > > + > > Remove extra newline. Removed. > > > + > > +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 = (acpi_size) (sizeof(u8)); > > Is this cast really needed? Nope, adjusted. > > > + 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[] = { > > Can this be const? No, "initialization from incompatible pointer type" if set like that. Other drivers in platform/ seem to follow the same approach too. > > > + &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) > > +{ > > + return sysfs_create_group(&wdev->dev.kobj, &tbt_attribute_group); > > You need to document this in Documentation/ABI. While there, I think it > make sense to update Documentation/admin-guide/thunderbolt.rst > accordingly. > Thanks, will fix for v2. > Also you are adding an attribute to a device that is already announced > to the userspace (I think). So it is possible userspace does not find > this when it deals with the uevent. Not sure if it is really a problem > in this case, though. > I don't think it will matter in this case. It will come down to how fwupd decides to use this (which will be TBD and is under discussion still). > Other than that, looks nice to me :) Thanks for your feedback. I'll send a follow up v2 in a few moments.
diff --git a/MAINTAINERS b/MAINTAINERS index 1c3feff..375bea9 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 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..6670a8d 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 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..98f60f2 --- /dev/null +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c @@ -0,0 +1,97 @@ +/* + * 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 = (acpi_size) (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) +{ + return sysfs_create_group(&wdev->dev.kobj, &tbt_attribute_group); +} + +static int intel_wmi_thunderbolt_remove(struct wmi_device *wdev) +{ + sysfs_remove_group(&wdev->dev.kobj, &tbt_attribute_group); + 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");
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 allow 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> --- MAINTAINERS | 5 ++ drivers/platform/x86/Kconfig | 13 ++++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel-wmi-thunderbolt.c | 97 ++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+) create mode 100644 drivers/platform/x86/intel-wmi-thunderbolt.c