diff mbox

[11/12] platform/x86: dell-wmi-smbios: introduce character device for userspace

Message ID a7829a31691da18e4d28e0760645ca2a3f302b66.1505999739.git.mario.limonciello@dell.com (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Limonciello, Mario Sept. 21, 2017, 1:57 p.m. UTC
This userspace character device will be used to perform SMBIOS calls
from any applications sending a properly formatted 4k calling interface
buffer.

This character device is intended to deprecate the dcdbas kernel module
and the interface that it provides to userspace.

It's important for the driver to provide a R/W ioctl to ensure that
two competing userspace processes don't race to provide or read each
others data.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 Documentation/ABI/testing/dell-wmi-smbios |  19 ++++++
 drivers/platform/x86/dell-wmi-smbios.c    | 108 ++++++++++++++++++++++++++----
 drivers/platform/x86/dell-wmi-smbios.h    |   5 ++
 3 files changed, 120 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/dell-wmi-smbios

Comments

Pali Rohár Sept. 25, 2017, 4:31 p.m. UTC | #1
On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications sending a properly formatted 4k calling interface
> buffer.
> 
> This character device is intended to deprecate the dcdbas kernel module
> and the interface that it provides to userspace.
> 
> It's important for the driver to provide a R/W ioctl to ensure that
> two competing userspace processes don't race to provide or read each
> others data.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  Documentation/ABI/testing/dell-wmi-smbios |  19 ++++++
>  drivers/platform/x86/dell-wmi-smbios.c    | 108 ++++++++++++++++++++++++++----
>  drivers/platform/x86/dell-wmi-smbios.h    |   5 ++
>  3 files changed, 120 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
> 
> diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
> new file mode 100644
> index 000000000000..54dcf73b3031
> --- /dev/null
> +++ b/Documentation/ABI/testing/dell-wmi-smbios
> @@ -0,0 +1,19 @@
> +What:		/dev/wmi-dell-wmi-smbios

What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
related and I think userspace does not have to care if it is via WMI or
direct SMM mode... Important is that it provides character device for
SMBIOS API and not how it is implemented.

Anyway, Darren, Andy, do we have some convention for naming platform
character devices?

> +Date:		October 2017
> +KernelVersion:	4.15
> +Contact:	"Mario Limonciello" <mario.limonciello@dell.com>
> +Description:
> +		Perform an SMBIOS call on a supported Dell machine
> +		through the Dell ACPI-WMI interface.
> +
> +		To make a call prepare a 4k buffer like this:
> +		struct buffer {
> +			u16 class;
> +			u16 select;
> +			u32 input[4];
> +			u32 output[4];
> +			u8 data[4060];
> +		} __packed;
> +
> +		Perform this RW IOCTL to get the result:
> +		_IOWR('D', 0, struct calling_interface_buffer)

I would suggest to provide uapi header file with all needed structures
and defines. So userspace application would have it and would not need
to implement own buffer...

> diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
> index 9deb851ff517..22e47fba6a59 100644
> --- a/drivers/platform/x86/dell-wmi-smbios.c
> +++ b/drivers/platform/x86/dell-wmi-smbios.c
> @@ -21,6 +21,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/wmi.h>
> +#include <linux/uaccess.h>
>  #include "dell-wmi-smbios.h"
>  
>  #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
> @@ -34,7 +35,8 @@ struct calling_interface_structure {
>  	struct calling_interface_token tokens[];
>  } __packed;
>  
> -static struct calling_interface_buffer *buffer;
> +static struct calling_interface_buffer *internal_buffer;
> +static struct calling_interface_buffer *sysfs_buffer;
>  static DEFINE_MUTEX(buffer_mutex);
>  
>  static int da_command_address;
> @@ -61,13 +63,13 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
>  {
>  	mutex_lock(&buffer_mutex);
>  	dell_smbios_clear_buffer();
> -	return buffer;
> +	return internal_buffer;
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
>  
>  void dell_smbios_clear_buffer(void)
>  {
> -	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> +	memset(internal_buffer, 0, sizeof(struct calling_interface_buffer));
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
>  
> @@ -107,9 +109,9 @@ int run_wmi_smbios_call(struct calling_interface_buffer *buffer)
>  
>  void dell_smbios_send_request(int class, int select)
>  {
> -	buffer->class = class;
> -	buffer->select = select;
> -	run_wmi_smbios_call(buffer);
> +	internal_buffer->class = class;
> +	internal_buffer->select = select;
> +	run_wmi_smbios_call(internal_buffer);
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
>  
> @@ -218,6 +220,68 @@ static const struct attribute_group smbios_attribute_group = {
>  	.attrs = smbios_attrs,
>  };
>  
> +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> +{
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static ssize_t dell_wmi_smbios_read(struct file *file, char __user *data,
> +				   size_t len, loff_t *ppos)
> +{
> +	ssize_t size = sizeof(struct calling_interface_buffer);
> +	void *src;
> +
> +	if (*ppos >= size)
> +		return 0;
> +	if (len >= size)
> +		len = size;
> +	if (*ppos + len > size)
> +		len = size - *ppos;
> +	src = (void __force *) (sysfs_buffer + *ppos);
> +	if (copy_to_user(data, src, len))
> +		return -EFAULT;
> +
> +	*ppos += len;
> +	return len;
> +}
> +
> +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	int ret = 0;
> +	size_t size;
> +
> +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> +		return -ENOTTY;
> +
> +	switch (cmd) {
> +	case DELL_WMI_SMBIOS_CMD:
> +		size = sizeof(struct calling_interface_buffer);
> +		mutex_lock(&buffer_mutex);
> +		if (copy_from_user(sysfs_buffer, (void __user *) arg, size)) {
> +			ret = -EFAULT;
> +			goto fail_smbios_cmd;
> +		}
> +		ret = run_wmi_smbios_call(sysfs_buffer);
> +		if (ret != 0)
> +			goto fail_smbios_cmd;
> +		if (copy_to_user((void __user *) arg, sysfs_buffer, size))
> +			ret = -EFAULT;
> +fail_smbios_cmd:
> +		mutex_unlock(&buffer_mutex);
> +		break;
> +	default:
> +		pr_err("unsupported ioctl: %d.\n", cmd);
> +		ret = -ENOIOCTLCMD;
> +	}
> +	return ret;
> +}
> +
>  /*
>   * Descriptor buffer is 128 byte long and contains:
>   *
> @@ -306,12 +370,19 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)
>  	if (ret)
>  		return ret;
>  
> -	buffer = (void *)__get_free_page(GFP_KERNEL);
> -	if (!buffer) {
> +	internal_buffer = (void *)__get_free_page(GFP_KERNEL);
> +	if (!internal_buffer) {
>  		ret = -ENOMEM;
> -		goto fail_buffer;
> +		goto fail_internal_buffer;
>  	}
>  
> +	sysfs_buffer = (void *)__get_free_page(GFP_KERNEL);
> +	if (!sysfs_buffer) {
> +		ret = -ENOMEM;
> +		goto fail_sysfs_buffer;
> +	}
> +	memset(sysfs_buffer, 0, sizeof(struct calling_interface_buffer));
> +
>  	ret = sysfs_create_group(&wdev->dev.kobj, &smbios_attribute_group);
>  	if (ret)
>  		goto fail_create_group;
> @@ -320,9 +391,12 @@ static int dell_wmi_smbios_probe(struct wmi_device *wdev)
>  	return 0;
>  
>  fail_create_group:
> -	free_page((unsigned long)buffer);
> +	free_page((unsigned long)sysfs_buffer);
>  
> -fail_buffer:
> +fail_sysfs_buffer:
> +	free_page((unsigned long)internal_buffer);
> +
> +fail_internal_buffer:
>  	kfree(da_tokens);
>  	return ret;
>  }
> @@ -331,7 +405,8 @@ static int dell_wmi_smbios_remove(struct wmi_device *wdev)
>  {
>  	sysfs_remove_group(&wdev->dev.kobj, &smbios_attribute_group);
>  	kfree(da_tokens);
> -	free_page((unsigned long)buffer);
> +	free_page((unsigned long)internal_buffer);
> +	free_page((unsigned long)sysfs_buffer);
>  	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
>  	return 0;
>  }
> @@ -341,6 +416,14 @@ static const struct wmi_device_id dell_wmi_smbios_id_table[] = {
>  	{ },
>  };
>  
> +static const struct file_operations dell_wmi_smbios_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= dell_wmi_smbios_read,
> +	.unlocked_ioctl	= dell_wmi_smbios_ioctl,
> +	.open		= dell_wmi_smbios_open,
> +	.release	= dell_wmi_smbios_release,
> +};
> +
>  static struct wmi_driver dell_wmi_smbios_driver = {
>  	.driver = {
>  		.name = "dell-wmi-smbios",
> @@ -348,6 +431,7 @@ static struct wmi_driver dell_wmi_smbios_driver = {
>  	.probe = dell_wmi_smbios_probe,
>  	.remove = dell_wmi_smbios_remove,
>  	.id_table = dell_wmi_smbios_id_table,
> +	.file_operations = &dell_wmi_smbios_fops,
>  };
>  module_wmi_driver(dell_wmi_smbios_driver);
>  
> diff --git a/drivers/platform/x86/dell-wmi-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
> index 0521ec5d437b..b8215eb879b2 100644
> --- a/drivers/platform/x86/dell-wmi-smbios.h
> +++ b/drivers/platform/x86/dell-wmi-smbios.h
> @@ -18,6 +18,7 @@
>  #define _DELL_WMI_SMBIOS_H_
>  
>  #include <linux/wmi.h>
> +#include <linux/ioctl.h>
>  
>  struct notifier_block;
>  
> @@ -40,6 +41,10 @@ struct calling_interface_token {
>  	};
>  };
>  
> +#define DELL_WMI_SMBIOS_IOC   'D'
> +#define DELL_WMI_SMBIOS_CMD   _IOWR(DELL_WMI_SMBIOS_IOC, 0, \
> +				   struct calling_interface_buffer)
> +
>  int dell_smbios_error(int value);
>  
>  struct calling_interface_buffer *dell_smbios_get_buffer(void);
Andy Shevchenko Sept. 25, 2017, 4:58 p.m. UTC | #2
On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
>> This userspace character device will be used to perform SMBIOS calls
>> from any applications sending a properly formatted 4k calling interface
>> buffer.
>>
>> This character device is intended to deprecate the dcdbas kernel module
>> and the interface that it provides to userspace.
>>
>> It's important for the driver to provide a R/W ioctl to ensure that
>> two competing userspace processes don't race to provide or read each
>> others data.

>> +What:                /dev/wmi-dell-wmi-smbios
>
> What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> related and I think userspace does not have to care if it is via WMI or
> direct SMM mode... Important is that it provides character device for
> SMBIOS API and not how it is implemented.
>
> Anyway, Darren, Andy, do we have some convention for naming platform
> character devices?

For me, looking to this case, seems better to expose a folder like
/dev/smbios/
with actual vendor device nodes inside like
/dev/smbios/dell

Darren?

P.S. Interestingly I wasn't in Cc list for this series at all.
Limonciello, Mario Sept. 25, 2017, 5:46 p.m. UTC | #3
> -----Original Message-----

> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> Sent: Monday, September 25, 2017 12:58 PM

> To: Pali Rohár <pali.rohar@gmail.com>

> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; dvhart@infradead.org;

> LKML <linux-kernel@vger.kernel.org>; Platform Driver <platform-driver-

> x86@vger.kernel.org>; quasisec@google.com

> Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character

> device for userspace

> 

> On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali.rohar@gmail.com> wrote:

> > On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:

> >> This userspace character device will be used to perform SMBIOS calls

> >> from any applications sending a properly formatted 4k calling interface

> >> buffer.

> >>

> >> This character device is intended to deprecate the dcdbas kernel module

> >> and the interface that it provides to userspace.

> >>

> >> It's important for the driver to provide a R/W ioctl to ensure that

> >> two competing userspace processes don't race to provide or read each

> >> others data.

> 

> >> +What:                /dev/wmi-dell-wmi-smbios

> >

> > What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS

> > related and I think userspace does not have to care if it is via WMI or

> > direct SMM mode... Important is that it provides character device for

> > SMBIOS API and not how it is implemented.

> >

> > Anyway, Darren, Andy, do we have some convention for naming platform

> > character devices?

> 

> For me, looking to this case, seems better to expose a folder like

> /dev/smbios/

> with actual vendor device nodes inside like

> /dev/smbios/dell


I disagree with this.  Dell exposes smbios calling in this kernel interface but
other vendor drivers may also expose different methods for character devices
that are not SMBIOS.

I'm envisioning that this is just the first kernel module that will use a WMI
character device to userspace.  That's why I wanted to use a generic namespace.

I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want 
something more generic.
As long as it's discovereable from uevent that's fine to me.

> 

> Darren?

> 

> P.S. Interestingly I wasn't in Cc list for this series at all.


Sorry, my mistake in gitconfig.  I'll add you in next version submission.
Darren Hart Sept. 27, 2017, 4:59 p.m. UTC | #4
On Mon, Sep 25, 2017 at 05:46:54PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > Sent: Monday, September 25, 2017 12:58 PM
> > To: Pali Rohár <pali.rohar@gmail.com>
> > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; dvhart@infradead.org;
> > LKML <linux-kernel@vger.kernel.org>; Platform Driver <platform-driver-
> > x86@vger.kernel.org>; quasisec@google.com
> > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> > device for userspace
> > 
> > On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> > >> This userspace character device will be used to perform SMBIOS calls
> > >> from any applications sending a properly formatted 4k calling interface
> > >> buffer.
> > >>
> > >> This character device is intended to deprecate the dcdbas kernel module
> > >> and the interface that it provides to userspace.
> > >>
> > >> It's important for the driver to provide a R/W ioctl to ensure that
> > >> two competing userspace processes don't race to provide or read each
> > >> others data.
> > 
> > >> +What:                /dev/wmi-dell-wmi-smbios
> > >
> > > What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> > > related and I think userspace does not have to care if it is via WMI or
> > > direct SMM mode... Important is that it provides character device for
> > > SMBIOS API and not how it is implemented.

I agree with this point, the implementation (WMI under the covers) is
not relevant. That said, this is an example of exposing WMI
functionality to userspace, and I DO NOT want to have a naming
discussion for every single WMI GUID that we choose to expose.

> > >
> > > Anyway, Darren, Andy, do we have some convention for naming platform
> > > character devices?
> > 
> > For me, looking to this case, seems better to expose a folder like
> > /dev/smbios/
> > with actual vendor device nodes inside like
> > /dev/smbios/dell
> 
> I disagree with this.  Dell exposes smbios calling in this kernel interface but
> other vendor drivers may also expose different methods for character devices
> that are not SMBIOS.
> 
> I'm envisioning that this is just the first kernel module that will use a WMI
> character device to userspace.  That's why I wanted to use a generic namespace.
> 
> I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want 
> something more generic.
> As long as it's discovereable from uevent that's fine to me.
> 
> > 
> > Darren?

I would like to see an automated and definiitive way to export WMI
GUIDs. Something we can look at, compare to a set of rules, and say
yay/nay. From that perspective, I like Mario's general proposal. It is
not clear to me if the $driver- prefix adds any value though - would we
ever have need of DRIVER_X-GUID_A and DRIVER_Y-GUID_A ??? I'm thinking
not.

/dev/wmi/$GUID
Limonciello, Mario Sept. 27, 2017, 6:10 p.m. UTC | #5
> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Wednesday, September 27, 2017 1:00 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andy.shevchenko@gmail.com; pali.rohar@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> quasisec@google.com
> Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Mon, Sep 25, 2017 at 05:46:54PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > > Sent: Monday, September 25, 2017 12:58 PM
> > > To: Pali Rohár <pali.rohar@gmail.com>
> > > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; dvhart@infradead.org;
> > > LKML <linux-kernel@vger.kernel.org>; Platform Driver <platform-driver-
> > > x86@vger.kernel.org>; quasisec@google.com
> > > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> > > device for userspace
> > >
> > > On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> > > >> This userspace character device will be used to perform SMBIOS calls
> > > >> from any applications sending a properly formatted 4k calling interface
> > > >> buffer.
> > > >>
> > > >> This character device is intended to deprecate the dcdbas kernel module
> > > >> and the interface that it provides to userspace.
> > > >>
> > > >> It's important for the driver to provide a R/W ioctl to ensure that
> > > >> two competing userspace processes don't race to provide or read each
> > > >> others data.
> > >
> > > >> +What:                /dev/wmi-dell-wmi-smbios
> > > >
> > > > What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> > > > related and I think userspace does not have to care if it is via WMI or
> > > > direct SMM mode... Important is that it provides character device for
> > > > SMBIOS API and not how it is implemented.
> 
> I agree with this point, the implementation (WMI under the covers) is
> not relevant. That said, this is an example of exposing WMI
> functionality to userspace, and I DO NOT want to have a naming
> discussion for every single WMI GUID that we choose to expose.

Yes, I agree let's make sure it's stable and automatic for all drivers that will use it.

> 
> > > >
> > > > Anyway, Darren, Andy, do we have some convention for naming platform
> > > > character devices?
> > >
> > > For me, looking to this case, seems better to expose a folder like
> > > /dev/smbios/
> > > with actual vendor device nodes inside like
> > > /dev/smbios/dell
> >
> > I disagree with this.  Dell exposes smbios calling in this kernel interface but
> > other vendor drivers may also expose different methods for character devices
> > that are not SMBIOS.
> >
> > I'm envisioning that this is just the first kernel module that will use a WMI
> > character device to userspace.  That's why I wanted to use a generic namespace.
> >
> > I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want
> > something more generic.
> > As long as it's discovereable from uevent that's fine to me.
> >
> > >
> > > Darren?
> 
> I would like to see an automated and definiitive way to export WMI
> GUIDs. Something we can look at, compare to a set of rules, and say
> yay/nay. From that perspective, I like Mario's general proposal. It is
> not clear to me if the $driver- prefix adds any value though - would we
> ever have need of DRIVER_X-GUID_A and DRIVER_Y-GUID_A ??? I'm thinking
> not.
> 
> /dev/wmi/$GUID
> 

I don't think you should allow two drivers to use the same GUID, but what if
a driver needs to use multiple GUIDs?

This obviously isn't a problem yet.
I envision that as we get MOF interpretation as part of the bus we "may" get into 
that situation with more complicated MOF design.  The vendor driver could 
register with the bus for each GUID and  request a character device for each but 
that's meaning userspace has to worry about knowing which GUIDs do what.  
I think it's potentially asking for a complicated interface to userspace for the future.

Here's a hypothetical scenario based on some more advanced MOF design I know of:

Separate GUID provided for each:
* All integer settings (data type)
* All string settings (data type)
* Notifications (event type)
* enumeration types (data type)
* Applying a Setting (method type)
* Managing users (method type)

How would a new driver for that look?  I would think you would want one driver to register
and encapsulate at least all of those GUIDs.

I would say all the data should be exported into sysfs, but that you really only want
one character device that is used for both methods.

The interface to userspace would then have two IOCTL's that the driver would internally
apply to the correct GUID.  Example: 
"/dev/wmi/$driver" supports 2 different IOCTL's.
Internally the driver would then ferry the request through the WMI bus to the correct
GUID based on the ioctl call number.

If you went with the pass the GUID information up and create character device for each
GUID, userspace will have to know:
"I use /dev/wmi/$GUID_X" for applying the setting IOCTL on Acme Inc machine.
And
"I use /dev/wmi/$GUID_Y" for managing users IOCTL on Acme Inc machine.

And what if the notifications are valuable to userspace?  Would you want to create another
character device for those?  I think you would just want to keep them in the same device
and add a poll/select file operation.
Darren Hart Sept. 27, 2017, 6:50 p.m. UTC | #6
On Wed, Sep 27, 2017 at 06:10:55PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Wednesday, September 27, 2017 1:00 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: andy.shevchenko@gmail.com; pali.rohar@gmail.com; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > quasisec@google.com
> > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> > device for userspace
> > 
> > On Mon, Sep 25, 2017 at 05:46:54PM +0000, Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > > > Sent: Monday, September 25, 2017 12:58 PM
> > > > To: Pali Rohár <pali.rohar@gmail.com>
> > > > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; dvhart@infradead.org;
> > > > LKML <linux-kernel@vger.kernel.org>; Platform Driver <platform-driver-
> > > > x86@vger.kernel.org>; quasisec@google.com
> > > > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> > > > device for userspace
> > > >
> > > > On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> > > > >> This userspace character device will be used to perform SMBIOS calls
> > > > >> from any applications sending a properly formatted 4k calling interface
> > > > >> buffer.
> > > > >>
> > > > >> This character device is intended to deprecate the dcdbas kernel module
> > > > >> and the interface that it provides to userspace.
> > > > >>
> > > > >> It's important for the driver to provide a R/W ioctl to ensure that
> > > > >> two competing userspace processes don't race to provide or read each
> > > > >> others data.
> > > >
> > > > >> +What:                /dev/wmi-dell-wmi-smbios
> > > > >
> > > > > What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> > > > > related and I think userspace does not have to care if it is via WMI or
> > > > > direct SMM mode... Important is that it provides character device for
> > > > > SMBIOS API and not how it is implemented.
> > 
> > I agree with this point, the implementation (WMI under the covers) is
> > not relevant. That said, this is an example of exposing WMI
> > functionality to userspace, and I DO NOT want to have a naming
> > discussion for every single WMI GUID that we choose to expose.
> 
> Yes, I agree let's make sure it's stable and automatic for all drivers that will use it.
> 
> > 
> > > > >
> > > > > Anyway, Darren, Andy, do we have some convention for naming platform
> > > > > character devices?
> > > >
> > > > For me, looking to this case, seems better to expose a folder like
> > > > /dev/smbios/
> > > > with actual vendor device nodes inside like
> > > > /dev/smbios/dell
> > >
> > > I disagree with this.  Dell exposes smbios calling in this kernel interface but
> > > other vendor drivers may also expose different methods for character devices
> > > that are not SMBIOS.
> > >
> > > I'm envisioning that this is just the first kernel module that will use a WMI
> > > character device to userspace.  That's why I wanted to use a generic namespace.
> > >
> > > I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you want
> > > something more generic.
> > > As long as it's discovereable from uevent that's fine to me.
> > >
> > > >
> > > > Darren?
> > 
> > I would like to see an automated and definiitive way to export WMI
> > GUIDs. Something we can look at, compare to a set of rules, and say
> > yay/nay. From that perspective, I like Mario's general proposal. It is
> > not clear to me if the $driver- prefix adds any value though - would we
> > ever have need of DRIVER_X-GUID_A and DRIVER_Y-GUID_A ??? I'm thinking
> > not.
> > 
> > /dev/wmi/$GUID
> > 
> 
> I don't think you should allow two drivers to use the same GUID, but what if
> a driver needs to use multiple GUIDs?
> 
> This obviously isn't a problem yet.
> I envision that as we get MOF interpretation as part of the bus we "may" get into 
> that situation with more complicated MOF design.  The vendor driver could 
> register with the bus for each GUID and  request a character device for each but 
> that's meaning userspace has to worry about knowing which GUIDs do what.  
> I think it's potentially asking for a complicated interface to userspace for the future.
> 
> Here's a hypothetical scenario based on some more advanced MOF design I know of:
> 
> Separate GUID provided for each:
> * All integer settings (data type)
> * All string settings (data type)
> * Notifications (event type)
> * enumeration types (data type)
> * Applying a Setting (method type)
> * Managing users (method type)
> 
> How would a new driver for that look?  I would think you would want one driver to register
> and encapsulate at least all of those GUIDs.
> 
> I would say all the data should be exported into sysfs, but that you really only want
> one character device that is used for both methods.
> 
> The interface to userspace would then have two IOCTL's that the driver would internally
> apply to the correct GUID.  Example: 
> "/dev/wmi/$driver" supports 2 different IOCTL's.
> Internally the driver would then ferry the request through the WMI bus to the correct
> GUID based on the ioctl call number.
> 
> If you went with the pass the GUID information up and create character device for each
> GUID, userspace will have to know:
> "I use /dev/wmi/$GUID_X" for applying the setting IOCTL on Acme Inc machine.
> And
> "I use /dev/wmi/$GUID_Y" for managing users IOCTL on Acme Inc machine.
> 
> And what if the notifications are valuable to userspace?  Would you want to create another
> character device for those?  I think you would just want to keep them in the same device
> and add a poll/select file operation.
> 

Thanks for the concrete example, this helps a lot.

One guiding principle of this series and something Pali correctly pointed out,
is that for Linux, we're keeping WMI as the implementation and exposing features
to userspace through char devs and IOCTLS. You points above are consistent with
that theme, and the GUIDs are not an appropriate namespace.

The driver namespace does make more sense as you suggested above:

/dev/wmi/$driver

This still has wmi in the name, but given that is the bus name, I think it's a
reasonable way organize the files - and is still a well-defined naming scheme.
Limonciello, Mario Sept. 27, 2017, 9:12 p.m. UTC | #7
> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Wednesday, September 27, 2017 2:50 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andy.shevchenko@gmail.com; pali.rohar@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> quasisec@google.com
> Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Wed, Sep 27, 2017 at 06:10:55PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Darren Hart [mailto:dvhart@infradead.org]
> > > Sent: Wednesday, September 27, 2017 1:00 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: andy.shevchenko@gmail.com; pali.rohar@gmail.com; linux-
> > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > > quasisec@google.com
> > > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce character
> > > device for userspace
> > >
> > > On Mon, Sep 25, 2017 at 05:46:54PM +0000, Mario.Limonciello@dell.com
> wrote:
> > > > > -----Original Message-----
> > > > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > > > > Sent: Monday, September 25, 2017 12:58 PM
> > > > > To: Pali Rohár <pali.rohar@gmail.com>
> > > > > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>;
> dvhart@infradead.org;
> > > > > LKML <linux-kernel@vger.kernel.org>; Platform Driver <platform-driver-
> > > > > x86@vger.kernel.org>; quasisec@google.com
> > > > > Subject: Re: [PATCH 11/12] platform/x86: dell-wmi-smbios: introduce
> character
> > > > > device for userspace
> > > > >
> > > > > On Mon, Sep 25, 2017 at 7:31 PM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > > > > > On Thursday 21 September 2017 08:57:16 Mario Limonciello wrote:
> > > > > >> This userspace character device will be used to perform SMBIOS calls
> > > > > >> from any applications sending a properly formatted 4k calling interface
> > > > > >> buffer.
> > > > > >>
> > > > > >> This character device is intended to deprecate the dcdbas kernel module
> > > > > >> and the interface that it provides to userspace.
> > > > > >>
> > > > > >> It's important for the driver to provide a R/W ioctl to ensure that
> > > > > >> two competing userspace processes don't race to provide or read each
> > > > > >> others data.
> > > > >
> > > > > >> +What:                /dev/wmi-dell-wmi-smbios
> > > > > >
> > > > > > What about just /dev/dell-smbios? IOCTL provided here is just SMBIOS
> > > > > > related and I think userspace does not have to care if it is via WMI or
> > > > > > direct SMM mode... Important is that it provides character device for
> > > > > > SMBIOS API and not how it is implemented.
> > >
> > > I agree with this point, the implementation (WMI under the covers) is
> > > not relevant. That said, this is an example of exposing WMI
> > > functionality to userspace, and I DO NOT want to have a naming
> > > discussion for every single WMI GUID that we choose to expose.
> >
> > Yes, I agree let's make sure it's stable and automatic for all drivers that will use it.
> >
> > >
> > > > > >
> > > > > > Anyway, Darren, Andy, do we have some convention for naming platform
> > > > > > character devices?
> > > > >
> > > > > For me, looking to this case, seems better to expose a folder like
> > > > > /dev/smbios/
> > > > > with actual vendor device nodes inside like
> > > > > /dev/smbios/dell
> > > >
> > > > I disagree with this.  Dell exposes smbios calling in this kernel interface but
> > > > other vendor drivers may also expose different methods for character devices
> > > > that are not SMBIOS.
> > > >
> > > > I'm envisioning that this is just the first kernel module that will use a WMI
> > > > character device to userspace.  That's why I wanted to use a generic
> namespace.
> > > >
> > > > I would say it could be /dev/wmi-$GUID or /dev/wmi/$driver-$GUID if you
> want
> > > > something more generic.
> > > > As long as it's discovereable from uevent that's fine to me.
> > > >
> > > > >
> > > > > Darren?
> > >
> > > I would like to see an automated and definiitive way to export WMI
> > > GUIDs. Something we can look at, compare to a set of rules, and say
> > > yay/nay. From that perspective, I like Mario's general proposal. It is
> > > not clear to me if the $driver- prefix adds any value though - would we
> > > ever have need of DRIVER_X-GUID_A and DRIVER_Y-GUID_A ??? I'm thinking
> > > not.
> > >
> > > /dev/wmi/$GUID
> > >
> >
> > I don't think you should allow two drivers to use the same GUID, but what if
> > a driver needs to use multiple GUIDs?
> >
> > This obviously isn't a problem yet.
> > I envision that as we get MOF interpretation as part of the bus we "may" get into
> > that situation with more complicated MOF design.  The vendor driver could
> > register with the bus for each GUID and  request a character device for each but
> > that's meaning userspace has to worry about knowing which GUIDs do what.
> > I think it's potentially asking for a complicated interface to userspace for the
> future.
> >
> > Here's a hypothetical scenario based on some more advanced MOF design I know
> of:
> >
> > Separate GUID provided for each:
> > * All integer settings (data type)
> > * All string settings (data type)
> > * Notifications (event type)
> > * enumeration types (data type)
> > * Applying a Setting (method type)
> > * Managing users (method type)
> >
> > How would a new driver for that look?  I would think you would want one driver
> to register
> > and encapsulate at least all of those GUIDs.
> >
> > I would say all the data should be exported into sysfs, but that you really only
> want
> > one character device that is used for both methods.
> >
> > The interface to userspace would then have two IOCTL's that the driver would
> internally
> > apply to the correct GUID.  Example:
> > "/dev/wmi/$driver" supports 2 different IOCTL's.
> > Internally the driver would then ferry the request through the WMI bus to the
> correct
> > GUID based on the ioctl call number.
> >
> > If you went with the pass the GUID information up and create character device for
> each
> > GUID, userspace will have to know:
> > "I use /dev/wmi/$GUID_X" for applying the setting IOCTL on Acme Inc machine.
> > And
> > "I use /dev/wmi/$GUID_Y" for managing users IOCTL on Acme Inc machine.
> >
> > And what if the notifications are valuable to userspace?  Would you want to
> create another
> > character device for those?  I think you would just want to keep them in the same
> device
> > and add a poll/select file operation.
> >
> 
> Thanks for the concrete example, this helps a lot.
> 
> One guiding principle of this series and something Pali correctly pointed out,
> is that for Linux, we're keeping WMI as the implementation and exposing features
> to userspace through char devs and IOCTLS. You points above are consistent with
> that theme, and the GUIDs are not an appropriate namespace.
> 
> The driver namespace does make more sense as you suggested above:
> 
> /dev/wmi/$driver
> 
> This still has wmi in the name, but given that is the bus name, I think it's a
> reasonable way organize the files - and is still a well-defined naming scheme.
> 

Thanks Darren.  I'll adjust the patch.
Would you like me to document this chosen policy somewhere in Documentation?
If so, where?
Or to put more about our conversation in the commit message?
Darren Hart Sept. 27, 2017, 9:59 p.m. UTC | #8
On Wed, Sep 27, 2017 at 09:12:34PM +0000, Mario.Limonciello@dell.com wrote:
...
> > Thanks for the concrete example, this helps a lot.
> > 
> > One guiding principle of this series and something Pali correctly pointed out,
> > is that for Linux, we're keeping WMI as the implementation and exposing features
> > to userspace through char devs and IOCTLS. You points above are consistent with
> > that theme, and the GUIDs are not an appropriate namespace.
> > 
> > The driver namespace does make more sense as you suggested above:
> > 
> > /dev/wmi/$driver
> > 
> > This still has wmi in the name, but given that is the bus name, I think it's a
> > reasonable way organize the files - and is still a well-defined naming scheme.
> > 
> 
> Thanks Darren.  I'll adjust the patch.
> Would you like me to document this chosen policy somewhere in Documentation?
> If so, where?
> Or to put more about our conversation in the commit message? 
> 

Some justification in the commit message for now would be best. Thanks!
diff mbox

Patch

diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
new file mode 100644
index 000000000000..54dcf73b3031
--- /dev/null
+++ b/Documentation/ABI/testing/dell-wmi-smbios
@@ -0,0 +1,19 @@ 
+What:		/dev/wmi-dell-wmi-smbios
+Date:		October 2017
+KernelVersion:	4.15
+Contact:	"Mario Limonciello" <mario.limonciello@dell.com>
+Description:
+		Perform an SMBIOS call on a supported Dell machine
+		through the Dell ACPI-WMI interface.
+
+		To make a call prepare a 4k buffer like this:
+		struct buffer {
+			u16 class;
+			u16 select;
+			u32 input[4];
+			u32 output[4];
+			u8 data[4060];
+		} __packed;
+
+		Perform this RW IOCTL to get the result:
+		_IOWR('D', 0, struct calling_interface_buffer)
diff --git a/drivers/platform/x86/dell-wmi-smbios.c b/drivers/platform/x86/dell-wmi-smbios.c
index 9deb851ff517..22e47fba6a59 100644
--- a/drivers/platform/x86/dell-wmi-smbios.c
+++ b/drivers/platform/x86/dell-wmi-smbios.c
@@ -21,6 +21,7 @@ 
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/wmi.h>
+#include <linux/uaccess.h>
 #include "dell-wmi-smbios.h"
 
 #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
@@ -34,7 +35,8 @@  struct calling_interface_structure {
 	struct calling_interface_token tokens[];
 } __packed;
 
-static struct calling_interface_buffer *buffer;
+static struct calling_interface_buffer *internal_buffer;
+static struct calling_interface_buffer *sysfs_buffer;
 static DEFINE_MUTEX(buffer_mutex);
 
 static int da_command_address;
@@ -61,13 +63,13 @@  struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
 	dell_smbios_clear_buffer();
-	return buffer;
+	return internal_buffer;
 }
 EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
 
 void dell_smbios_clear_buffer(void)
 {
-	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+	memset(internal_buffer, 0, sizeof(struct calling_interface_buffer));
 }
 EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
 
@@ -107,9 +109,9 @@  int run_wmi_smbios_call(struct calling_interface_buffer *buffer)
 
 void dell_smbios_send_request(int class, int select)
 {
-	buffer->class = class;
-	buffer->select = select;
-	run_wmi_smbios_call(buffer);
+	internal_buffer->class = class;
+	internal_buffer->select = select;
+	run_wmi_smbios_call(internal_buffer);
 }
 EXPORT_SYMBOL_GPL(dell_smbios_send_request);
 
@@ -218,6 +220,68 @@  static const struct attribute_group smbios_attribute_group = {
 	.attrs = smbios_attrs,
 };
 
+static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
+{
+	return nonseekable_open(inode, file);
+}
+
+static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static ssize_t dell_wmi_smbios_read(struct file *file, char __user *data,
+				   size_t len, loff_t *ppos)
+{
+	ssize_t size = sizeof(struct calling_interface_buffer);
+	void *src;
+
+	if (*ppos >= size)
+		return 0;
+	if (len >= size)
+		len = size;
+	if (*ppos + len > size)
+		len = size - *ppos;
+	src = (void __force *) (sysfs_buffer + *ppos);
+	if (copy_to_user(data, src, len))
+		return -EFAULT;
+
+	*ppos += len;
+	return len;
+}
+
+static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
+	unsigned long arg)
+{
+	int ret = 0;
+	size_t size;
+
+	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
+		return -ENOTTY;
+
+	switch (cmd) {
+	case DELL_WMI_SMBIOS_CMD:
+		size = sizeof(struct calling_interface_buffer);
+		mutex_lock(&buffer_mutex);
+		if (copy_from_user(sysfs_buffer, (void __user *) arg, size)) {
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		ret = run_wmi_smbios_call(sysfs_buffer);
+		if (ret != 0)
+			goto fail_smbios_cmd;
+		if (copy_to_user((void __user *) arg, sysfs_buffer, size))
+			ret = -EFAULT;
+fail_smbios_cmd:
+		mutex_unlock(&buffer_mutex);
+		break;
+	default:
+		pr_err("unsupported ioctl: %d.\n", cmd);
+		ret = -ENOIOCTLCMD;
+	}
+	return ret;
+}
+
 /*
  * Descriptor buffer is 128 byte long and contains:
  *
@@ -306,12 +370,19 @@  static int dell_wmi_smbios_probe(struct wmi_device *wdev)
 	if (ret)
 		return ret;
 
-	buffer = (void *)__get_free_page(GFP_KERNEL);
-	if (!buffer) {
+	internal_buffer = (void *)__get_free_page(GFP_KERNEL);
+	if (!internal_buffer) {
 		ret = -ENOMEM;
-		goto fail_buffer;
+		goto fail_internal_buffer;
 	}
 
+	sysfs_buffer = (void *)__get_free_page(GFP_KERNEL);
+	if (!sysfs_buffer) {
+		ret = -ENOMEM;
+		goto fail_sysfs_buffer;
+	}
+	memset(sysfs_buffer, 0, sizeof(struct calling_interface_buffer));
+
 	ret = sysfs_create_group(&wdev->dev.kobj, &smbios_attribute_group);
 	if (ret)
 		goto fail_create_group;
@@ -320,9 +391,12 @@  static int dell_wmi_smbios_probe(struct wmi_device *wdev)
 	return 0;
 
 fail_create_group:
-	free_page((unsigned long)buffer);
+	free_page((unsigned long)sysfs_buffer);
 
-fail_buffer:
+fail_sysfs_buffer:
+	free_page((unsigned long)internal_buffer);
+
+fail_internal_buffer:
 	kfree(da_tokens);
 	return ret;
 }
@@ -331,7 +405,8 @@  static int dell_wmi_smbios_remove(struct wmi_device *wdev)
 {
 	sysfs_remove_group(&wdev->dev.kobj, &smbios_attribute_group);
 	kfree(da_tokens);
-	free_page((unsigned long)buffer);
+	free_page((unsigned long)internal_buffer);
+	free_page((unsigned long)sysfs_buffer);
 	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
 	return 0;
 }
@@ -341,6 +416,14 @@  static const struct wmi_device_id dell_wmi_smbios_id_table[] = {
 	{ },
 };
 
+static const struct file_operations dell_wmi_smbios_fops = {
+	.owner		= THIS_MODULE,
+	.read		= dell_wmi_smbios_read,
+	.unlocked_ioctl	= dell_wmi_smbios_ioctl,
+	.open		= dell_wmi_smbios_open,
+	.release	= dell_wmi_smbios_release,
+};
+
 static struct wmi_driver dell_wmi_smbios_driver = {
 	.driver = {
 		.name = "dell-wmi-smbios",
@@ -348,6 +431,7 @@  static struct wmi_driver dell_wmi_smbios_driver = {
 	.probe = dell_wmi_smbios_probe,
 	.remove = dell_wmi_smbios_remove,
 	.id_table = dell_wmi_smbios_id_table,
+	.file_operations = &dell_wmi_smbios_fops,
 };
 module_wmi_driver(dell_wmi_smbios_driver);
 
diff --git a/drivers/platform/x86/dell-wmi-smbios.h b/drivers/platform/x86/dell-wmi-smbios.h
index 0521ec5d437b..b8215eb879b2 100644
--- a/drivers/platform/x86/dell-wmi-smbios.h
+++ b/drivers/platform/x86/dell-wmi-smbios.h
@@ -18,6 +18,7 @@ 
 #define _DELL_WMI_SMBIOS_H_
 
 #include <linux/wmi.h>
+#include <linux/ioctl.h>
 
 struct notifier_block;
 
@@ -40,6 +41,10 @@  struct calling_interface_token {
 	};
 };
 
+#define DELL_WMI_SMBIOS_IOC   'D'
+#define DELL_WMI_SMBIOS_CMD   _IOWR(DELL_WMI_SMBIOS_IOC, 0, \
+				   struct calling_interface_buffer)
+
 int dell_smbios_error(int value);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void);