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

Groups > linux.kernel > #1741158 > unrolled thread

[PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

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

  [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers Mario Limonciello <mario.limonciello@dell.com> - 2017-09-28 06:10 +0200
    Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers Darren Hart <dvhart@infradead.org> - 2017-09-30 04:00 +0200
      Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-30 10:20 +0200
        RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers <Mario.Limonciello@dell.com> - 2017-09-30 21:30 +0200
          Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers Greg KH <gregkh@linuxfoundation.org> - 2017-10-01 15:30 +0200
            RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers <Mario.Limonciello@dell.com> - 2017-10-01 16:30 +0200
              Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers Greg KH <gregkh@linuxfoundation.org> - 2017-10-01 20:10 +0200
        Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers Darren Hart <dvhart@infradead.org> - 2017-10-02 03:00 +0200
          Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when  requested by drivers Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-10-02 11:30 +0200

#1741158 — [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

For WMI operations that are only Set or Query read or write sysfs
attributes created by WMI vendor drivers make sense.

For other WMI operations that are run on Method, there needs to be a
way to guarantee to userspace that the results from the method call
belong to the data request to the method call.  Sysfs attributes don't
work well in this scenario because two userspace processes may be
competing at reading/writing an attribute and step on each other's
data.

When a WMI vendor driver declares a set of functions in a
file_operations object the WMI bus driver will create a character
device that maps to those file operations.

That character device will correspond to this path:
/dev/wmi/$driver

This policy is selected as one driver may map and use multiple
GUIDs and it would be better to only expose a single character
device.

The WMI vendor drivers will be responsible for managing access to
this character device and proper locking on it.

When a WMI vendor driver is unloaded the WMI bus driver will clean
up the character device.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
 include/linux/wmi.h        |  1 +
 2 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4d73a87c2ddf..17399df87948 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -34,7 +34,9 @@
 #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
 
 #include <linux/acpi.h>
+#include <linux/cdev.h>
 #include <linux/device.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho");
 MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
 MODULE_LICENSE("GPL");
 
+#define WMI_MAX_DEVS  MINORMASK
+static DEFINE_IDR(wmi_idr);
+static DEFINE_MUTEX(wmi_minor_lock);
 static LIST_HEAD(wmi_block_list);
 
 struct guid_block {
@@ -69,6 +74,8 @@ struct wmi_block {
 	struct wmi_device dev;
 	struct list_head list;
 	struct guid_block gblock;
+	struct cdev *cdev;
+	int minor;
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
@@ -86,6 +93,7 @@ struct wmi_block {
 #define ACPI_WMI_STRING      0x4	/* GUID takes & returns a string */
 #define ACPI_WMI_EVENT       0x8	/* GUID is an event */
 
+static dev_t wmi_devt;
 static bool debug_event;
 module_param(debug_event, bool, 0444);
 MODULE_PARM_DESC(debug_event,
@@ -782,21 +790,88 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	return 0;
 }
 
+static struct class wmi_bus_class = {
+	.name = "wmi_bus",
+};
+
+static int wmi_minor_get(struct wmi_block *wblock)
+{
+	int ret;
+
+	mutex_lock(&wmi_minor_lock);
+	ret = idr_alloc(&wmi_idr, wblock, 0, WMI_MAX_DEVS, GFP_KERNEL);
+	if (ret >= 0)
+		wblock->minor = ret;
+	else if (ret == -ENOSPC)
+		dev_err(&wblock->dev.dev, "too many wmi devices\n");
+
+	mutex_unlock(&wmi_minor_lock);
+	return ret;
+}
+
+static void wmi_minor_free(struct wmi_block *wblock)
+{
+	mutex_lock(&wmi_minor_lock);
+	idr_remove(&wmi_idr, wblock->minor);
+	mutex_unlock(&wmi_minor_lock);
+}
+
+
 static int wmi_dev_probe(struct device *dev)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	struct wmi_driver *wdriver =
 		container_of(dev->driver, struct wmi_driver, driver);
-	int ret = 0;
+	struct device *clsdev;
+	int ret = 0, devno;
 
 	if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");
 
+	/* driver wants a character device made */
+	if (wdriver->file_operations) {
+		dev->devt = wmi_devt;
+		wblock->cdev = cdev_alloc();
+		if (!wblock->cdev) {
+			dev_err(dev, "failed to allocate cdev\n");
+			return -ENOMEM;
+		}
+		cdev_init(wblock->cdev, wdriver->file_operations);
+		wblock->cdev->owner = wdriver->file_operations->owner;
+		ret = wmi_minor_get(wblock);
+		if (ret < 0)
+			return ret;
+		devno = MKDEV(MAJOR(wmi_devt), wblock->minor);
+		ret = cdev_add(wblock->cdev, devno, 1);
+		if (ret) {
+			dev_err(dev, "unable to create device %d:%d\n",
+			MAJOR(wmi_devt), wblock->minor);
+			goto err_probe_cdev;
+		}
+		clsdev = device_create(&wmi_bus_class, dev,
+				       MKDEV(MAJOR(wmi_devt), wblock->minor),
+				       NULL, "wmi/%s", wdriver->driver.name);
+
+		if (IS_ERR(clsdev)) {
+			dev_err(dev, "unable to create device %d:%d\n",
+			MAJOR(wmi_devt), wblock->minor);
+			ret = PTR_ERR(clsdev);
+			goto err_probe_class;
+		}
+	}
+
 	if (wdriver->probe) {
 		ret = wdriver->probe(dev_to_wdev(dev));
 		if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
 			dev_warn(dev, "failed to disable device\n");
 	}
+	return ret;
+
+err_probe_class:
+	cdev_del(wblock->cdev);
+
+err_probe_cdev:
+	wmi_minor_free(wblock);
 
 	return ret;
 }
@@ -808,6 +883,13 @@ static int wmi_dev_remove(struct device *dev)
 		container_of(dev->driver, struct wmi_driver, driver);
 	int ret = 0;
 
+	if (wdriver->file_operations) {
+		device_destroy(&wmi_bus_class,
+			       MKDEV(MAJOR(wmi_devt), wblock->minor));
+		cdev_del(wblock->cdev);
+		wmi_minor_free(wblock);
+	}
+
 	if (wdriver->remove)
 		ret = wdriver->remove(dev_to_wdev(dev));
 
@@ -817,10 +899,6 @@ static int wmi_dev_remove(struct device *dev)
 	return ret;
 }
 
-static struct class wmi_bus_class = {
-	.name = "wmi_bus",
-};
-
 static struct bus_type wmi_bus_type = {
 	.name = "wmi",
 	.dev_groups = wmi_groups,
@@ -1270,8 +1348,17 @@ static int __init acpi_wmi_init(void)
 		goto err_unreg_bus;
 	}
 
+	error = alloc_chrdev_region(&wmi_devt, 0, WMI_MAX_DEVS, "wmi");
+	if (error < 0) {
+		pr_err("unable to allocate char dev region\n");
+		goto err_unreg_platform;
+	}
+
 	return 0;
 
+err_unreg_platform:
+	platform_driver_unregister(&acpi_wmi_driver);
+
 err_unreg_bus:
 	bus_unregister(&wmi_bus_type);
 
@@ -1283,6 +1370,7 @@ static int __init acpi_wmi_init(void)
 
 static void __exit acpi_wmi_exit(void)
 {
+	unregister_chrdev_region(wmi_devt, WMI_MAX_DEVS);
 	platform_driver_unregister(&acpi_wmi_driver);
 	bus_unregister(&wmi_bus_type);
 	class_unregister(&wmi_bus_class);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 2cd10c3b89e9..ba5f75a32f4c 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -47,6 +47,7 @@ struct wmi_device_id {
 struct wmi_driver {
 	struct device_driver driver;
 	const struct wmi_device_id *id_table;
+	const struct file_operations *file_operations;
 
 	int (*probe)(struct wmi_device *wdev);
 	int (*remove)(struct wmi_device *wdev);
-- 
2.14.1

[toc] | [next] | [standalone]

#1742449 — Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
> 
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call.  Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
> 
> When a WMI vendor driver declares a set of functions in a
> file_operations object the WMI bus driver will create a character
> device that maps to those file operations.
> 
> That character device will correspond to this path:
> /dev/wmi/$driver
> 
> This policy is selected as one driver may map and use multiple
> GUIDs and it would be better to only expose a single character
> device.
> 
> The WMI vendor drivers will be responsible for managing access to
> this character device and proper locking on it.
> 
> When a WMI vendor driver is unloaded the WMI bus driver will clean
> up the character device.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/wmi.h        |  1 +
>  2 files changed, 94 insertions(+), 5 deletions(-)

+Greg, Rafael, Matthew, and Christoph

You each provided feedback regarding the method of exposing WMI methods
to userspace. This and subsequent patches from Mario lay some of the
core groundwork.

They implement an implicit whitelist as only drivers requesting the char
dev will see it created.

https://lkml.org/lkml/2017/9/28/8

-- 
Darren Hart
VMware Open Source Technology Center

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

#1742560 — Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> 
> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > For WMI operations that are only Set or Query read or write sysfs
> > attributes created by WMI vendor drivers make sense.
> > 
> > For other WMI operations that are run on Method, there needs to be a
> > way to guarantee to userspace that the results from the method call
> > belong to the data request to the method call.  Sysfs attributes don't
> > work well in this scenario because two userspace processes may be
> > competing at reading/writing an attribute and step on each other's
> > data.
> > 
> > When a WMI vendor driver declares a set of functions in a
> > file_operations object the WMI bus driver will create a character
> > device that maps to those file operations.
> > 
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> > 
> > This policy is selected as one driver may map and use multiple
> > GUIDs and it would be better to only expose a single character
> > device.
> > 
> > The WMI vendor drivers will be responsible for managing access to
> > this character device and proper locking on it.
> > 
> > When a WMI vendor driver is unloaded the WMI bus driver will clean
> > up the character device.
> > 
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/wmi.h        |  1 +
> >  2 files changed, 94 insertions(+), 5 deletions(-)
> 
> +Greg, Rafael, Matthew, and Christoph
> 
> You each provided feedback regarding the method of exposing WMI methods
> to userspace. This and subsequent patches from Mario lay some of the
> core groundwork.
> 
> They implement an implicit whitelist as only drivers requesting the char
> dev will see it created.
> 
> https://lkml.org/lkml/2017/9/28/8

If you want patchs reviewed, it's best to actually cc: us on the patch
itself :(

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

#1742678 — RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Saturday, September 30, 2017 3:12 AM
> To: Darren Hart <dvhart@infradead.org>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>;
> platform-driver-x86@vger.kernel.org; Andy Lutomirski <luto@kernel.org>;
> quasisec@google.com; pali.rohar@gmail.com; Rafael Wysocki
> <rjw@rjwysocki.net>; Matthew Garrett <mjg59@google.com>; Christoph Hellwig
> <hch@lst.de>
> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> >
> > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > For WMI operations that are only Set or Query read or write sysfs
> > > attributes created by WMI vendor drivers make sense.
> > >
> > > For other WMI operations that are run on Method, there needs to be a
> > > way to guarantee to userspace that the results from the method call
> > > belong to the data request to the method call.  Sysfs attributes don't
> > > work well in this scenario because two userspace processes may be
> > > competing at reading/writing an attribute and step on each other's
> > > data.
> > >
> > > When a WMI vendor driver declares a set of functions in a
> > > file_operations object the WMI bus driver will create a character
> > > device that maps to those file operations.
> > >
> > > That character device will correspond to this path:
> > > /dev/wmi/$driver
> > >
> > > This policy is selected as one driver may map and use multiple
> > > GUIDs and it would be better to only expose a single character
> > > device.
> > >
> > > The WMI vendor drivers will be responsible for managing access to
> > > this character device and proper locking on it.
> > >
> > > When a WMI vendor driver is unloaded the WMI bus driver will clean
> > > up the character device.
> > >
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > ---
> > >  drivers/platform/x86/wmi.c | 98
> +++++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/wmi.h        |  1 +
> > >  2 files changed, 94 insertions(+), 5 deletions(-)
> >
> > +Greg, Rafael, Matthew, and Christoph
> >
> > You each provided feedback regarding the method of exposing WMI methods
> > to userspace. This and subsequent patches from Mario lay some of the
> > core groundwork.
> >
> > They implement an implicit whitelist as only drivers requesting the char
> > dev will see it created.
> >
> > https://lkml.org/lkml/2017/9/28/8
> 
> If you want patchs reviewed, it's best to actually cc: us on the patch
> itself :(

Greg,

I'll make sure to CC you on V4 after I address the other concerns that have
been recently raised.

I think what's Darren's most interested in is the that conceptually this is
an approach you can agree with.

"A WMI vendor driver binds to the WMI bus and requests the WMI bus to create
a character device.  The WMI bus creates a character device /dev/wmi/$driver
which the WMI vendor driver will process all various file operations."

Thanks,

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

#1742802 — Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote:
> I think what's Darren's most interested in is the that conceptually this is
> an approach you can agree with.
> 
> "A WMI vendor driver binds to the WMI bus and requests the WMI bus to create
> a character device.  The WMI bus creates a character device /dev/wmi/$driver
> which the WMI vendor driver will process all various file operations."

We've been over this before, let's see that actual implementation which
defines the "will process all..." implementation to see if that is
actually an acceptable thing.

thanks,

greg k-h

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

#1742814 — RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Sunday, October 1, 2017 8:24 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net;
> mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote:
> > I think what's Darren's most interested in is the that conceptually this is
> > an approach you can agree with.
> >
> > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to
> create
> > a character device.  The WMI bus creates a character device /dev/wmi/$driver
> > which the WMI vendor driver will process all various file operations."
> 
> We've been over this before, let's see that actual implementation which
> defines the "will process all..." implementation to see if that is
> actually an acceptable thing.
> 
> thanks,
> 
> greg k-h

I'm working on changes still (that you'll be CC on), but I don't expect this specific 
concept to change in the future changes:

Bus creating character device:
https://patchwork.kernel.org/patch/9975283/

Driver requesting character device:
https://patchwork.kernel.org/patch/9975263/

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

#1742855 — Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

On Sun, Oct 01, 2017 at 02:25:00PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Sunday, October 1, 2017 8:24 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> > quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net;
> > mjg59@google.com; hch@lst.de
> > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> > requested by drivers
> > 
> > On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote:
> > > I think what's Darren's most interested in is the that conceptually this is
> > > an approach you can agree with.
> > >
> > > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to
> > create
> > > a character device.  The WMI bus creates a character device /dev/wmi/$driver
> > > which the WMI vendor driver will process all various file operations."
> > 
> > We've been over this before, let's see that actual implementation which
> > defines the "will process all..." implementation to see if that is
> > actually an acceptable thing.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I'm working on changes still (that you'll be CC on), but I don't expect this specific 
> concept to change in the future changes:
> 
> Bus creating character device:
> https://patchwork.kernel.org/patch/9975283/

A bit hard to comment on web pages, and I would like to point out your
obvious memory leak, but it's hard to do that this way :(

> Driver requesting character device:
> https://patchwork.kernel.org/patch/9975263/

There's other issues with this patch as well, I'll wait for a real one
in my inbox to be able to actually review it :(

thanks,

greg k-h

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

#1742919 — Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> > 
> > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > For WMI operations that are only Set or Query read or write sysfs
> > > attributes created by WMI vendor drivers make sense.
> > > 
> > > For other WMI operations that are run on Method, there needs to be a
> > > way to guarantee to userspace that the results from the method call
> > > belong to the data request to the method call.  Sysfs attributes don't
> > > work well in this scenario because two userspace processes may be
> > > competing at reading/writing an attribute and step on each other's
> > > data.
> > > 
> > > When a WMI vendor driver declares a set of functions in a
> > > file_operations object the WMI bus driver will create a character
> > > device that maps to those file operations.
> > > 
> > > That character device will correspond to this path:
> > > /dev/wmi/$driver
> > > 
> > > This policy is selected as one driver may map and use multiple
> > > GUIDs and it would be better to only expose a single character
> > > device.
> > > 
> > > The WMI vendor drivers will be responsible for managing access to
> > > this character device and proper locking on it.
> > > 
> > > When a WMI vendor driver is unloaded the WMI bus driver will clean
> > > up the character device.
> > > 
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > ---
> > >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/wmi.h        |  1 +
> > >  2 files changed, 94 insertions(+), 5 deletions(-)
> > 
> > +Greg, Rafael, Matthew, and Christoph
> > 
> > You each provided feedback regarding the method of exposing WMI methods
> > to userspace. This and subsequent patches from Mario lay some of the
> > core groundwork.
> > 
> > They implement an implicit whitelist as only drivers requesting the char
> > dev will see it created.
> > 
> > https://lkml.org/lkml/2017/9/28/8
> 
> If you want patchs reviewed, it's best to actually cc: us on the patch
> itself :(
> 

Of course. I didn't send the series, but thought you should see it. I
could have asked Mario to resend, but I thought a pointer would have
made it easy enough to find in your lkml folder, and it would avoid
splitting the conversation which resending would inevitably lead to. I
pruned this one because Christoph gets upset if I don't.

We can wait for v4 I guess. And next time I want to get your take on
something someone doesn't Cc you on, I'll just ask them to resend the
whole series with you on Cc.

-- 
Darren Hart
VMware Open Source Technology Center

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

#1743060 — Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

On Sun, Oct 01, 2017 at 05:57:20PM -0700, Darren Hart wrote:
> On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> > > 
> > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > > For WMI operations that are only Set or Query read or write sysfs
> > > > attributes created by WMI vendor drivers make sense.
> > > > 
> > > > For other WMI operations that are run on Method, there needs to be a
> > > > way to guarantee to userspace that the results from the method call
> > > > belong to the data request to the method call.  Sysfs attributes don't
> > > > work well in this scenario because two userspace processes may be
> > > > competing at reading/writing an attribute and step on each other's
> > > > data.
> > > > 
> > > > When a WMI vendor driver declares a set of functions in a
> > > > file_operations object the WMI bus driver will create a character
> > > > device that maps to those file operations.
> > > > 
> > > > That character device will correspond to this path:
> > > > /dev/wmi/$driver
> > > > 
> > > > This policy is selected as one driver may map and use multiple
> > > > GUIDs and it would be better to only expose a single character
> > > > device.
> > > > 
> > > > The WMI vendor drivers will be responsible for managing access to
> > > > this character device and proper locking on it.
> > > > 
> > > > When a WMI vendor driver is unloaded the WMI bus driver will clean
> > > > up the character device.
> > > > 
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > ---
> > > >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> > > >  include/linux/wmi.h        |  1 +
> > > >  2 files changed, 94 insertions(+), 5 deletions(-)
> > > 
> > > +Greg, Rafael, Matthew, and Christoph
> > > 
> > > You each provided feedback regarding the method of exposing WMI methods
> > > to userspace. This and subsequent patches from Mario lay some of the
> > > core groundwork.
> > > 
> > > They implement an implicit whitelist as only drivers requesting the char
> > > dev will see it created.
> > > 
> > > https://lkml.org/lkml/2017/9/28/8
> > 
> > If you want patchs reviewed, it's best to actually cc: us on the patch
> > itself :(
> > 
> 
> Of course. I didn't send the series, but thought you should see it. I
> could have asked Mario to resend, but I thought a pointer would have
> made it easy enough to find in your lkml folder, and it would avoid
> splitting the conversation which resending would inevitably lead to. I
> pruned this one because Christoph gets upset if I don't.
> 
> We can wait for v4 I guess. And next time I want to get your take on
> something someone doesn't Cc you on, I'll just ask them to resend the
> whole series with you on Cc.

Dude, that's the way it's always been, you know this!  Nothing new here,
respining a patch series is normal, and asking people to review patch
sets that you know already needs to be changed is strange.  Just do the
fixes, and resend it, that's the simplest, quickest, and easiest thing
to do for everyone involved.

Asking someone to review a patch based on a url just doesn't work, as
how can I point out where the memory leak is exactly?  :)

Reviewers get thousands of emails a week (or a day in some cases), and
making it as easy as possible for them to review the code is the key
here.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web