Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731736 > unrolled thread

Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status

Started byLukas Wunner <lukas@wunner.de>
First post2017-09-13 19:30 +0200
Last post2017-09-15 18:40 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status Lukas Wunner <lukas@wunner.de> - 2017-09-13 19:30 +0200
    RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status <Mario.Limonciello@dell.com> - 2017-09-14 09:50 +0200
      Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status Lukas Wunner <lukas@wunner.de> - 2017-09-14 11:30 +0200
        RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status <Mario.Limonciello@dell.com> - 2017-09-14 17:00 +0200
          Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-09-14 17:10 +0200
            Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-09-14 17:20 +0200
            Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status Lukas Wunner <lukas@wunner.de> - 2017-09-15 09:50 +0200
              RE: [PATCH v3] Add driver to force WMI Thunderbolt controller power  status <Mario.Limonciello@dell.com> - 2017-09-15 18:40 +0200

#1731736 — Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status

FromLukas Wunner <lukas@wunner.de>
Date2017-09-13 19:30 +0200
SubjectRe: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
Message-ID<upjE5-1RR-7@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1732085

From<Mario.Limonciello@dell.com>
Date2017-09-14 09:50 +0200
Message-ID<upx4n-293-11@gated-at.bofh.it>
In reply to#1731736
> -----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

[toc] | [prev] | [next] | [standalone]

#1732137

FromLukas Wunner <lukas@wunner.de>
Date2017-09-14 11:30 +0200
Message-ID<upyD7-3bN-17@gated-at.bofh.it>
In reply to#1732085
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

[toc] | [prev] | [next] | [standalone]

#1732337

From<Mario.Limonciello@dell.com>
Date2017-09-14 17:00 +0200
Message-ID<upDMu-6ln-29@gated-at.bofh.it>
In reply to#1732137
> -----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.

[toc] | [prev] | [next] | [standalone]

#1732346

FromMika Westerberg <mika.westerberg@linux.intel.com>
Date2017-09-14 17:10 +0200
Message-ID<upDWa-6Ei-31@gated-at.bofh.it>
In reply to#1732337
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.

[toc] | [prev] | [next] | [standalone]

#1732348

FromMika Westerberg <mika.westerberg@linux.intel.com>
Date2017-09-14 17:20 +0200
Message-ID<upE5P-6HF-3@gated-at.bofh.it>
In reply to#1732346
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.

[toc] | [prev] | [next] | [standalone]

#1732756

FromLukas Wunner <lukas@wunner.de>
Date2017-09-15 09:50 +0200
Message-ID<upTxT-8aA-1@gated-at.bofh.it>
In reply to#1732346
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

[toc] | [prev] | [next] | [standalone]

#1732947

From<Mario.Limonciello@dell.com>
Date2017-09-15 18:40 +0200
Message-ID<uq1ON-5t8-3@gated-at.bofh.it>
In reply to#1732756
> -----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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web