RE: Fwd: [PATCH] Add driver to force WMI Thunderbolt controller power status

[<Mario.Limonciello@dell.com>]

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